feat: SDK hardening -- typed contracts, idempotent run_job, sync clone (#426/#427/#428) - #432
Conversation
padak
left a comment
There was a problem hiding this comment.
Review of #432 — feat: SDK hardening -- typed contracts, idempotent run_job, sync clone (#426/#427/#428)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This collector PR delivers three hardening items for the importable in-process SDK: PEP 561 typed contracts (py.typed + pydantic result models at the facade), client-side idempotency for job run, and a sync clone composite that copies a reference synced tree, applies declarative overrides, and pushes fresh. The implementation is well-structured: Phase D (flow-task configId remap) is correctly scoped to created_configs only, so it cannot corrupt a normal update push; the idempotency store uses established atomic fcntl write patterns; the typed-at-facade strategy correctly leaves the service-layer dicts and --json CLI output unchanged. make check passes with 4089 tests. Verdict: APPROVE — no blocking findings. Four non-blocking items below are worth addressing before or shortly after the version bump to 0.63.0.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 4
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/agents/keboola-expert.md:97 — "Cross-project migration" matrix row not updated for sync clone
The §2 Tool Selection Matrix row for "Cross-project migration" still points to kbagent sync pull + edit + kbagent sync push --dry-run as the first choice. After this PR, kbagent sync clone (0.63.0+) is the correct first-choice when the user's intent is "provision a new instance from a golden reference" — the existing row covers the manual edit-then-push migration pattern. An agent that reads only the matrix will recommend the manual multi-step flow for the clone use case, missing the safer composite. The file has 51 KB headroom under the 62 KB budget. Fix: add one row | Clone a reference project into a fresh target project | \kbagent sync clone --source ./golden --target alias --target-dir ./clone` (0.63.0+) | -- | manual copy + id-surgery + push | and add a note to the §3 gotchas: "sync clone --dry-runcreates--target-dir` on disk (applies overrides, saves manifest) but does NOT push — the directory persists after a dry run."
[NB-2] plugins/kbagent/skills/kbagent/references/library-workflow.md:143 — CloneResult documented as returned by service but service returns dict
The line reads: "(CloneResult is returned by the service-layer SyncService.clone_project -- see GitOps sync)". The actual method signature (src/keboola_agent_cli/services/sync_service.py:1820) docstring says "Returns: A dict matching CloneResult" — the service returns a plain dict, not a CloneResult instance. The result models for JobResult, QueryResult, etc. are validated at the facade layer via model_validate; CloneResult has no such facade wrapper (there is no Client.clone_project). A programmatic consumer who imports CloneResult and calls SyncService.clone_project directly will get a dict, not a typed object. Fix: change the sentence to "(CloneResult documents the dict shape returned by SyncService.clone_project; callers can wrap it via CloneResult.model_validate(result))" OR add CloneResult.model_validate(push_result) to the return in clone_project. Also: CloneResult is absent from tests/test_result_models.py despite being exported from the package root — add a contract test mirroring TestJobResult.test_maps_real_queue_keys.
[NB-3] tests/test_e2e.py:4117 — sync clone E2E only exercises --dry-run; real push not covered
The E2E step for sync clone uses --dry-run and re-points the clone at the SAME project, so it exercises copy + manifest repoint + diff but not the actual push (Phase D flow remap, fresh-target guard, idempotent re-run). Per CONTRIBUTING.md §Tests: "Every CLI command MUST have a corresponding E2E test in tests/test_e2e.py" — the intent is push coverage. A full live clone push requires a second dedicated test project (fresh, empty), which may not be available in the nightly suite. If that second project is not available, document the skip explicitly in the test class docstring with the reason, and consider adding a nightly-only TestE2EFullClone class gated on a E2E_CLONE_TARGET_URL / E2E_CLONE_TARGET_TOKEN pair to catch Phase D regressions against the real API.
[NB-4] src/keboola_agent_cli/services/sync_service.py — adds 321 LOC to a file already 2.7× the hard ceiling
sync_service.py grows from 3 712 lines (main) to 4 033 lines (+321). The hard ceiling for services/*.py is 1 500 LOC (CONTRIBUTING.md §Code Quality Patterns). The file has been over budget for several releases and is called out by name in the guidelines as the known extreme case. This PR adds material and does not split first, as the guideline requires ("the next PR that adds material to it should split first"). The new Phase D code (_resolve_flow_task_bindings, _remap_flow_tasks_in_place, _apply_flow_task_binding) and clone_project are natural candidates for extraction into _flow_bindings.py and sync/clone.py (the clone helpers are already in sync/clone.py; the service orchestration could follow). Recommend opening a split issue tagged tech-debt and linking it from the PR description so the debt is tracked.
Nits
-
[NIT-1]src/keboola_agent_cli/commands/sync.py:517—_format_clone_result(formatter: Any, result: dict[str, Any])usesAnyforformatter. All sibling_format_*helpers use the concreteOutputFormattertype. Narrowing it improves IDE navigation and catches aformatter.success()call that only exists onOutputFormatter(not on arbitraryAny). -
[NIT-2]src/keboola_agent_cli/services/job_idempotency_store.py:232— theforce_rerun=Truepath silently bypasses the component/config collision guard. The--helptext says "Ignore any stored entry and always create a fresh job" and the error message for the collision case says "Use a distinct key, or--force-rerunto override." — so bypassing is intentional. But a one-line comment inside theif existing is not None and not force_rerun:guard would make it explicit thatforce_rerunis the documented escape hatch for the collision scenario, preventing a future reader from "fixing" it by hoisting the collision check outside thenot force_rerunbranch.
Verification log
gh auth status→ authenticated aspadak✓gh pr view 432 --json state,files,additions,deletions→ OPEN, 27 files, +3206/-46, title matchesfeat:prefix ✓git rev-parse --abbrev-ref HEAD→feat/sdk-hardening-426-427-428matches<branch>✓CONTRIBUTING.mdPlugin synchronization map walked row by row:src/keboola_agent_cli/commands/context.py(AGENT_CONTEXT):sync clone+job run --idempotency-keydocumented ✓CLAUDE.md ## All CLI Commands: both signatures updated ✓permissions.py OPERATION_REGISTRY:"sync.clone": "write"added at line 937 ✓plugins/kbagent/skills/kbagent/SKILL.md:sync clonerow added ✓plugins/kbagent/skills/kbagent/references/commands-reference.md: updated ✓plugins/kbagent/skills/kbagent/references/gotchas.md: three new sections, all tagged(since v0.63.0)✓plugins/kbagent/skills/kbagent/references/library-workflow.md: updated ✓plugins/kbagent/skills/kbagent/references/sync-workflow.md:sync clonesection added ✓plugins/kbagent/agents/keboola-expert.md: NOT updated → NON-BLOCKING [NB-1]src/keboola_agent_cli/hints/definitions/*.py: not checked (no new command group)
- Layer compliance:
grep typer/httpx in services/lib→ empty ✓ - Phase D safety check:
_resolve_flow_task_bindingsonly iteratescreated_configs(newly created this push); a normal update-only push hascreated_configs=[]andflow_binding.tasks_remapped=0→ no mutation of existing flows on non-clone pushes ✓ - Phase D scope: only remaps tasks whose
(componentId, configId)appears increated_id_map(reference→ULID map keyed by pre-writeback id) → an existing flow that references a pre-existing (not created-this-push) config is left untouched ✓ flow_bindingscope: always assigned inside thewith client:block before line 1774; early returns at lines 1505 (no_changes) and 1517 (dry_run) exit before that block ✓- Idempotency store atomicity:
record()opens lock file withO_RDONLY|O_CREAT, acquiresLOCK_EXviafcntl.flock, does read-modify-write under lock, writes to.tmpviaos.fdopencontext manager (fd closed on exception), thenos.replace(atomic rename). Pattern mirrorsConfigStore✓ force_rerun=True+ different component/config: collision guard is insidenot force_rerunbranch → intentional bypass per help text ("ignore stored entry") → NIT-2 (clarity comment, not a bug) ✓CloneResultusage: exported from package root, butSyncService.clone_projectreturnsdictnot a typedCloneResultinstance → [NB-2]make check(worktree,uv sync --extra server) → 4089 passed, 8 skipped, 15 warnings ✓sync_service.pyLOC: 3712 (main) → 4033 (+321) — pre-existing budget violation, worsened slightly → [NB-4]- Convention checks: no magic numbers, no raw
error_codestrings, no bareexcept:, noprint()in production code, no tokens in logs ✓ - Backward compat:
Client.query()still returnslist[dict](delegates to_run_query().rows) ✓; service-layer dicts unchanged;--jsonoutput unchanged ✓ - Behavior verification: could not run E2E against a live project (no fresh empty target project available); claimed behavior verified through code analysis and unit test coverage instead. Per-run note: test_e2e.py
sync clonestep exercises only--dry-runpath against self → [NB-3]
Open questions for the author
- Is the
sync clone --dry-runbehavior (creating--target-diron disk while not pushing) intentional and stable public contract? The help text ("Apply overrides and show the would-be diff without pushing") describes it correctly, but a user who runs--dry-runexpecting zero disk side effects will find a new directory at--target-dir. If this is intentional, a note in thesync-workflow.mdclone section would prevent confusion.
- context.py AGENT_CONTEXT (Devin BUG): the `sync clone` block was inserted between the `sync push` one-liner and its multi-paragraph continuation, so push semantics (encryption fail-closed, fresh-CREATE, --branch) mis-read as clone properties. Move the clone block AFTER the full push description. - keboola-expert.md (NB-1): add a `sync clone` row to the §2 tool matrix so the agent recommends the composite for "provision a new project from a reference", not the manual pull+edit+push flow. - library-workflow.md (NB-2): clarify CloneResult DOCUMENTS the dict shape that SyncService.clone_project returns (the service returns a plain dict; wrap via model_validate) -- it is not returned as a typed instance. - test_result_models.py (NB-2): add CloneResult contract tests (embedded SyncPushResult, ok property, dry_run-without-push) + cover it in the base loop. - test_e2e.py (NB-3): document why the clone E2E step is --dry-run only (no fresh second project in the single-project E2E harness; push path is unit-covered). - job_idempotency_store.py (NIT-2): comment that force_rerun intentionally bypasses the collision guard (don't hoist the check out of the branch). - sync-workflow.md: note that `sync clone --dry-run` still writes --target-dir. NIT-1 (formatter: Any) intentionally NOT applied: all sibling _format_* helpers in sync.py use `Any` -- matching the established pattern. NB-4 (sync_service.py LOC split) tracked as a follow-up tech-debt task. make check green (4092).
Review findings addressed (commit 118e687)Thanks to Devin Review and the kbagent-pr-reviewer. Resolution: Devin
kbagent-pr-reviewer
|
…y (tech-debt) `sync_service.py` had grown to ~4033 lines -- ~2.7x the 1500-LOC ceiling for `services/*.py` (CONTRIBUTING.md calls this file out by name). #432 added ~321 LOC without splitting; this is the deferred split. No behavior change -- pure mechanical extraction, public `SyncService` API identical: - `_sync_models.py`: the 5 dataclasses (WritebackResult, CreatedConfig, VariableBindingResult, FlowBindingResult, LocalConfigHashes) + the VARIABLES/FLOW component-id constants. `sync_service` re-exports `CreatedConfig` (the only externally-imported name) for back-compat. - `_sync_bindings.py`: the push-time link backfill -- Phase C (variable links) and Phase D (keboola.flow task configIds) -- as free functions taking the `SyncService` as first arg (for the 3 on-disk helpers they need). push() now calls `resolve_variable_bindings(self, ...)` / `resolve_flow_task_bindings(self, ...)`. - `_sync_clone.py`: the `clone_project` orchestration. `SyncService.clone_project` is now a thin delegator (public method preserved). `sync_service.py`: 4033 -> 3435 lines (-598). The free-function-taking-service pattern keeps typing explicit (ty-clean via a TYPE_CHECKING `SyncService` import) and makes the bindings/clone unit-testable in isolation. Further extractions (writeback, storage-metadata/jobs/samples) are natural follow-ups in the same pattern. make check green (4092 passed).
…odels (tech-debt) (#437) * refactor(sync): extract bindings + clone + models from sync_service.py (tech-debt) `sync_service.py` had grown to ~4033 lines -- ~2.7x the 1500-LOC ceiling for `services/*.py` (CONTRIBUTING.md calls this file out by name). #432 added ~321 LOC without splitting; this is the deferred split. No behavior change -- pure mechanical extraction, public `SyncService` API identical: - `_sync_models.py`: the 5 dataclasses (WritebackResult, CreatedConfig, VariableBindingResult, FlowBindingResult, LocalConfigHashes) + the VARIABLES/FLOW component-id constants. `sync_service` re-exports `CreatedConfig` (the only externally-imported name) for back-compat. - `_sync_bindings.py`: the push-time link backfill -- Phase C (variable links) and Phase D (keboola.flow task configIds) -- as free functions taking the `SyncService` as first arg (for the 3 on-disk helpers they need). push() now calls `resolve_variable_bindings(self, ...)` / `resolve_flow_task_bindings(self, ...)`. - `_sync_clone.py`: the `clone_project` orchestration. `SyncService.clone_project` is now a thin delegator (public method preserved). `sync_service.py`: 4033 -> 3435 lines (-598). The free-function-taking-service pattern keeps typing explicit (ty-clean via a TYPE_CHECKING `SyncService` import) and makes the bindings/clone unit-testable in isolation. Further extractions (writeback, storage-metadata/jobs/samples) are natural follow-ups in the same pattern. make check green (4092 passed). * refactor(sync): extract writeback + storage/jobs/samples helpers (tech-debt) Continues the sync_service.py split. No behavior change. - `_sync_writeback.py`: manifest + local-file writeback after a push (writeback_create_config_in_manifest, writeback_create_row_in_manifest, propagate_kbc_metadata, writeback_after_push). Only writeback_after_push needs the service (for _write_config_file); the rest are pure. - `_sync_storage.py`: pull-side data writers (write_storage_metadata, fetch_jobs_per_config, write_per_config_jobs, fetch_samples, mask_encrypted_columns) + the `_ensure_path_within` storage-write traversal guard moved here with its only callers. Only fetch_jobs_per_config needs the service (for _resolve_max_workers). push()/pull() call the free functions; tests updated to the relocated symbols (test_sync_service.py writeback calls, test_sync_storage_jobs.py storage calls). `sync_service.py`: 3435 -> 2931 lines. Cumulative over this PR: 4033 -> 2931 (-1102, ~27%), now across 5 cohesive modules (_sync_models, _sync_bindings, _sync_clone, _sync_writeback, _sync_storage). make check green (4092 passed, identical to pre-split). * refactor(sync): extract bulk (all-projects) + branch-linking ops (tech-debt) Continues the sync_service.py split. No behavior change. - `_sync_bulk.py`: the all-projects orchestrators (pull_all, diff_all, push_all) as free functions taking the service; the public methods are now thin delegators. - `_sync_branch.py`: git-branch <-> Keboola-dev-branch linking (branch_link, branch_unlink, branch_status). branch_link takes the service; the others are pure. `get_current_branch` is imported LOCALLY inside each function (as the original did) so tests that patch `keboola_agent_cli.sync.git_utils. get_current_branch` keep working -- a top-level import would capture the reference at import time and defeat source-module patching. Public methods preserved as shims (commands + tests call them unchanged). `sync_service.py`: 2581 lines (was 2931 this commit; 4033 at the PR base). make check green (4092 passed). * refactor(sync): extract per-change push CRUD ops (tech-debt) Continues the sync_service.py split. No behavior change. - `_sync_push_ops.py`: the per-change create/update/delete operations for configs and rows (push_create, push_update, push_row_change + the row create/update/delete helpers) as free functions taking the service. push() calls push_create(self, ...) / push_update(self, ...) / push_row_change(self, ...). The 2 direct test calls to _push_update_row were updated to the relocated push_update_row. `sync_service.py`: 2244 lines. Cumulative over this PR: 4033 -> 2244 (-1789, ~44%), across 8 cohesive modules (_sync_models, _sync_bindings, _sync_clone, _sync_writeback, _sync_storage, _sync_bulk, _sync_branch, _sync_push_ops). The remainder is the core orchestration (init/pull/diff/push/status) + shared branch-path/manifest helpers. make check green (4092 passed).
…nc clone (#438) Bump 0.62.0 -> 0.63.0 + changelog for the SDK-hardening features merged since 0.62.0 (#432: #426 sync clone + flow Phase-D remap, #427 client-side run_job idempotency, #428 typed result models + py.typed). version-sync propagates to plugin.json / marketplace.json / uv.lock. make check green (4092).
Collector PR for the three SDK-hardening issues. Each landed as its own reviewed sub-PR into this branch:
py.typedrun_jobsync clonecomposite + flow-task configId remapAll three harden the importable in-process SDK (
keboola_agent_cli.Client+ service layer) seeded by the 0.61.0 library facade, so a programmatic consumer (the FIIA Scaffold Kit) gets typed contracts, replay-safe job runs, and a tested clone primitive instead of hand-rolling them.#428 — typed contracts + py.typed
py.typedmarker (forced into the wheel) → downstreammypy/ty/IDEs treat the SDK as typed.JobResult,QueryResult,UploadTableResult,SyncPushResult,ConfigDetailResult) exported from the package root; allextra="allow"so backend drift never raises,populate_by_name+AliasChoicessomodel_validate(service_dict)works on raw API keys. Strategy: typed at the facade — the dict-returning service layer +--jsonCLI output are unchanged.Client.run_job/query_result/config_detail/upload_table.#427 — idempotent run_job (client-side)
POST /jobshas no server idempotency token (verified against the live spec v1.3.8 + thekeboola/job-queueserver source — an internaldeduplicationIdexists but is daemon-only). So dedup is client-side:JobIdempotencyStore(atomic, fcntl-locked, 0600) + a probe-before-create policy.job run --idempotency-key KEY [--force-rerun];Client.run_job(idempotency_key=..., idempotency_store=...). Prior non-failed → returned (idempotent_replay); prior failed → re-run; key reused for a different config →INVALID_ARGUMENT.#426 — sync clone + flow remap
kbagent sync clone/SyncService.clone_project→ typedCloneResult: copy a reference tree, apply declarativebucket_map/variable_values/instance_renameoverrides, push fresh.keboola.flowtask configIds reference→ULID viacreated_id_map(generic; benefits any fresh-create push). Fresh-target guard + idempotent re-run.Versioning
No version bump in this branch — it stays at 0.62.0. CI's
version-checkonly enforces plugin.json↔pyproject consistency (both 0.62.0). The version bump to 0.63.0 + the changelog entry are the agreed joint final step before merge; a ready-to-apply changelog entry is provided in a PR comment.Verification
make checkgreen on the merged branch: 4089 passed, 8 skipped. Per-phase doc-sync (CLAUDE.md, AGENT_CONTEXT, commands-reference, gotchas(since v0.63.0), workflow refs) done;command-sync-check+skill-checkpass.