fix(config): stamp created config identity into --push scaffolds and place them branch-aware - #653
Conversation
| local = api_config_to_local( | ||
| component_id, | ||
| {"name": name, "description": description, "configuration": configuration}, | ||
| str(config_id), | ||
| ) | ||
| content = yaml.dump( | ||
| local, | ||
| default_flow_style=False, | ||
| allow_unicode=True, | ||
| sort_keys=False, | ||
| width=120, | ||
| ) | ||
| return [ | ||
| { | ||
| "path": CONFIG_FILENAME, | ||
| "content": content, | ||
| "description": "Configuration mirroring the pushed body", | ||
| } | ||
| ] |
There was a problem hiding this comment.
🟡 Mirrored config drops scaffold companion files for transformations and apps
When --configuration is given, the mirror path replaces the file list with build_pushed_config_files, which returns only _config.yml. For SQL/Python transformations and custom-python apps the scaffold's transform.sql, transform.py, code.py and pyproject.toml are never written, so --push --output-dir yields an incomplete scaffold. The inlined code also diverges from the sync-pull layout that extracts blocks into code files.
Prompt for agents
build_pushed_config_files in services/component_service.py builds the local file list for the config new --push --output-dir path when an explicit --configuration body was pushed. It currently calls api_config_to_local and dumps a single _config.yml. For code-bearing components (SQL transformation, Python transformation, custom_python), the sync pull path additionally runs extract_code_files to move parameters.blocks/code into transform.sql/transform.py (and writes pyproject.toml/code.py), leaving _config.yml without inline code. Because the mirror path skips that extraction and returns only _config.yml, (a) the scaffold companion files that generate_scaffold would have produced are dropped, and (b) the on-disk layout diverges from what sync pull materializes, contradicting the docstring's claim of a clean follow-up sync diff. Consider running the same extract_code_files step (or reusing the pull-side writer) so the mirrored tree matches a pulled tree, or scope build_pushed_config_files to non-code components and fall back to the stamped scaffold (which already carries the code files) for transformations/apps.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed — this was the most substantive finding of the review round (independently reproduced by three internal review angles as well). Fixed in bf0d125: the mirrored-body path now materializes the directory exactly the way sync pull would — api_config_to_local + extract_code_files + the shared dump_config_yaml — so a pushed transformation body yields a real transform.sql/transform.py with the pushed code (not placeholders, which merge_code_files would have pushed over the real blocks on the next sync push; and not nothing). _description.md parity included. Covered by TestMaterializePushedConfig::test_transformation_body_extracts_real_code.
padak
left a comment
There was a problem hiding this comment.
Review of #653 — fix(config): stamp created config identity into --push scaffolds and place them branch-aware
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 PR fixes issue #644 (the 34-duplicate-config incident): config new --push --output-dir now stamps the created config's _keboola.config_id into the written scaffold and places it in the subtree of the branch it was actually created in (registering the branch in manifest.branches when unknown, falling back to branch-{id}/ — never to the default tree — on registration failure). It also mirrors the pushed body instead of writing placeholder scaffolding when --configuration was supplied. The change is well-isolated (pure helper functions, thin CLI wiring), the extraction of _ensure_branch_registered/new register_branch_dir into sync/branch_registry.py is a clean file-size-budget split, and all hand-maintained silent-drift doc surfaces (CLAUDE.md, context.py AGENT_CONTEXT, keboola-expert.md, commands-reference.md, gotchas.md, scaffold-workflow.md) were updated. make check is green (5877 passed, 12 skipped) and command-sync-check confirms no permission/registry drift (no new CLI command was added, so OPERATION_REGISTRY needs no change). No BLOCKING findings. Verdict: COMMENT.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 5
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/commands/config.py:1495 — new 2-element tuple return conflates two semantically distinct values
_resolve_push_scaffold_prefix(...) -> tuple[str | None, str | None] returns (branch_prefix, warning) — a directory-subtree string and a human-readable warning message, which are semantically distinct (this is almost the literal BAD example in CONTRIBUTING.md § "Return values -- name them with dataclasses, not tuples"). All pre-existing tuple[...] returns in this codebase are grandfathered, but new ones should not be added. Recommend a small frozen @dataclass ScaffoldPlacement: branch_prefix: str | None; warning: str | None so the two call sites (config.py:1440) read placement.branch_prefix / placement.warning instead of unpacking a positional pair.
[NB-2] src/keboola_agent_cli/commands/config.py:1244 — config new --help docstring not updated for the vNEXT behavior change
Every hand-maintained AI-facing doc surface (context.py, keboola-expert.md, commands-reference.md, gotchas.md, CLAUDE.md) was updated to describe the new config-id stamping + branch-aware placement, but the Typer command's own docstring (the --help text a human at a terminal actually sees) still reads exactly as before — no mention that --push --output-dir now records _keboola.config_id and lands in the created branch's subtree. CONTRIBUTING.md's per-command checklist calls --help text "the ultimate fallback when documentation drifts"; a user without an AI agent in the loop gets no signal of this important duplicate-prevention change from kbagent config new --help.
[NB-3] src/keboola_agent_cli/sync/branch_registry.py:69 (+ services/sync_service.py:2173) — new register_branch_dir function's real registration path and error branch are untested
The new public function (and its SyncService.register_branch_dir wrapper) has two branches with no direct unit-test coverage: (a) the ConfigError raised when the on-disk manifest's project.id does not match the resolved project, and (b) the actual "branch not yet in manifest" path that calls client_factory(...), uses the client as a context manager, and calls save_manifest. The only CLI-level test that exercises a not-yet-registered branch (test_branch_unknown_to_manifest_gets_registered in tests/test_config_create_cli.py) mocks sync_service.register_branch_dir itself, so it never touches the real implementation; the only test that reaches the real function (test_branch_create_writes_into_registered_branch_dir) hits the early "already registered" return and never calls the client factory. Recommend a focused unit test (e.g. in tests/test_sync_service.py or a new tests/test_branch_registry.py) with a mocked client_factory asserting client.close() is called and the manifest is saved, plus a test for the cross-project ConfigError.
[NB-4] src/keboola_agent_cli/commands/config.py — no new/updated E2E test for the branch-aware scaffold placement
tests/test_e2e.py step 19b covers config new --push (one-shot remote create) but not the specific --output-dir + dev-branch combination this PR fixes, which is exactly the scenario that produced the real incident (branch auto-activation + scaffold landing in main/). Per CONTRIBUTING.md, every CLI command needs E2E coverage; this is a behavior fix to an existing command rather than a new command, and environmental constraints (no live branch-creation harness available in this review) can defer this one cycle, but the author should add it before the next release since a mocked sync_service.register_branch_dir in the existing tests cannot catch a real API-shape mismatch in list_dev_branches().
[NB-5] src/keboola_agent_cli/commands/config.py:1527 — broad except Exception in _resolve_push_scaffold_prefix treats a ConfigError (real misconfiguration) the same as a transient API failure
If output_dir happens to point at a sync workspace whose manifest belongs to a different project than the one being pushed to, register_branch_dir raises ConfigError (a caller-configuration problem), which this handler catches identically to a network/API failure and downgrades to a branch-{id}/ fallback + warning. That is safe (never writes to the default tree, per the documented intent), but it silently masks a genuine "you pointed --output-dir at the wrong project's sync tree" situation as if it were a flaky API call. Consider distinguishing ConfigError (surface as a harder error or a more specific warning message) from other exceptions (API/network — keep the current soft fallback).
Nits
[NIT-1]src/keboola_agent_cli/sync/branch_registry.py:73—register_branch_dir'sproject: Anyparameter could be typedProjectConfig(from..models, a dependency-free leaf module already imported byservices/sync_service.pyand friends) instead ofAny, without introducing any circular import, for bettertycoverage on this new function.[NIT-2]src/keboola_agent_cli/commands/config.py:1495—_resolve_push_scaffold_prefix(branch resolution + user-facing warning text construction) is arguably service-layer business logic living in the CLI layer; it is consistent with this file's pre-existing pattern (_write_scaffold_to_disk,_detect_branch_prefixalready live here), so not flagged as a layer violation, but a future split intoservices/alongsideSyncService.register_branch_dirwould tidy the boundary.
Verification log
gh auth status→ authenticated aspadak✓gh pr view 653 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN,+753/-49, 12 files, prefixfix(config):matches a bug-fix change ✓git rev-parse --abbrev-ref HEAD→claude/issue-644-a0a49dmatches<branch>✓- Read
CONTRIBUTING.md(Checklist: Adding a New CLI Command; Plugin synchronization map; Releasing a new version) ✓ - Read
plugins/kbagent/agents/keboola-expert.md§1/§2/§3 — confirmed the §2 matrix row for "Create a new config" was updated in-place (existing row, not a new group, so no new-row requirement) ✓ gh pr diff 653(1049 lines) saved and read in full ✓- Layer-violation greps (typer/click/formatter in services; httpx/requests in commands; formatter/typer in clients) → all empty ✓
- Magic-number / raw error-code / bare-except /
print()/ token-leak greps on the diff → all empty (token-leak grep hits are docstrings and test fixtures with a fake#tokenplaceholder, not real secrets) ✓ - New
-> tuple[...]return grep → exactly one hit:_resolve_push_scaffold_prefix -> tuple[str | None, str | None]→ NB-1 ✓ - Confirmed no new CLI command was added (only existing
config newbehavior changed) →permissions.pyOPERATION_REGISTRYneeds no new entry;make check'scommand-sync-checkpassed, confirming this ✓ - Confirmed
_keboola:/component_id:line-based parsing assumption instamp_scaffold_config_idmatches both the standard scaffold builder (_build_config_yml) and the flow scaffold (_build_flow_config_yml, which has no_keboolablock, exercised bytest_appends_block_when_keboola_missing) ✓ - Confirmed
create_config's returned dict always carriesbranch_id(both dry-run and real paths) andeffective_branch_id = branch_id or project.active_branch_id, matching the PR description's "active-branch fallback" claim (leg 2 of the incident) ✓ - Confirmed
server/routers/configs.py'sconfig_createroute callsConfigService.create_configdirectly with no local-scaffold/output-dir concept — the fix is correctly scoped to the CLI-only filesystem path; no REST-router drift ✓ - Confirmed
"warnings"as an additive top-level envelope key is an established convention already used byflow_service.py,storage_service.py,project_service.py,doctor_service.py— not a novel shape ✓ - Checked "vNEXT" tag precedent: found a sibling gotcha (
A scaffolded keboola.flow config can now be pushed from disk (since vNEXT), PR #652 per the description) using the same placeholder convention, resolved at release time — not a one-off ✓ make check(background run) → exit code 0; tail shows5877 passed, 12 skipped, 169 deselected, 2 warnings in 133.06s— includes lint, format, typecheck, skill-check, version-check, version-gate-check, command-sync-check, changelog-check, check-error-codes, check-sentinel-guards, loc-check perMakefile'schecktarget ✓uv run kbagent config new --help→ renders correctly, confirms NB-2 (no mention of the vNEXT behavior in the docstring) ✓- Searched
tests/forregister_branch_dir/branch_registrycoverage → only reached via CLI tests, one of which mocks the function itself, the other hits an early-return branch → NB-3 ✓ - Searched
tests/test_e2e.pyforconfig new --output-dir+ branch coverage → only step 19b (--pushone-shot, no--output-dir/branch angle) exists → NB-4 ✓ - Did not attempt a live-project reproduction (branch create +
config new --push --output-dir+sync push) — no test credentials available in this review environment; relied on the PR's own unit/CLI test suite (which passed undermake check) as the verification signal instead.
Open questions for the author
(none)
…eview)
Substantive:
- Mirrored-body path (--configuration + --output-dir) now materializes the
directory EXACTLY like sync pull: api_config_to_local + extract_code_files
+ shared dump_config_yaml. A pushed SQL/Python body yields a real
transform.sql/.py (not placeholders, which merge_code_files would have
pushed over the real code; and not nothing, which dropped companion files
entirely -- the review's most-confirmed finding). _description.md parity
included. materialize_pushed_config replaces build_pushed_config_files.
- Placement policy moved out of the command layer into
sync/branch_registry.resolve_scaffold_placement + a thin SyncService
delegate (3-layer rule); returns a frozen ScaffoldPlacement dataclass
instead of a new bare tuple (CONTRIBUTING.md rule).
- default_branch_prefix does a tolerant raw-JSON peek (historical
_detect_branch_prefix semantics; full-manifest validation broke partial
manifests and was wasted work for a read).
- stamp purity: companion entries are copied, not shared; name fallback
handles a falsy API echo (str(push_result.get("name") or name));
"config_id recorded" banner only claims it when an id was stamped;
local_scaffold gains a config_id key.
- Windows: scaffold writes use newline="" (LF-only, same convention as
SyncService._write_config_file).
- dump settings deduplicated (dump_config_yaml), branch-{id} spelling
single-sourced (fallback_branch_dir), dead defensive tail removed,
_DETECT_BRANCH_PREFIX sentinel removed.
Tests: new tests/test_branch_registry.py (14 cases incl. project-mismatch
degrade and registration-failure fallback); real-generator stamp
integration test; transformation-body extraction test; CLI placement tests
reworked to the ScaffoldPlacement API; new E2E step 19c2 for
--push --output-dir stamping.
…place them branch-aware (#644) 'config new --push --output-dir' used to write the scaffold generated BEFORE the POST: no _keboola.config_id (the 'assigned on first push' comment was wrong on this path -- the config already existed) and always under the default branch tree, even when the config was created in a dev branch (--branch, or the active branch set by 'branch create'/'branch use'). The next 'sync push' then classified the directory as a brand-new configuration and POSTed a duplicate -- a real incident produced 34 duplicates in one dev branch. Verified live on a real project: with the ID stamped, the sync diff's adopt-by-id guard (issue #482) pairs the directory with the existing remote config ('modified' instead of 'added' + 'remote_only'). Changes: - stamp_scaffold_config_id() (component_service): pure rewrite of the scaffold _config.yml -- config_id double-quoted (legacy numeric IDs must stay YAML strings), misleading NOTE replaced, missing _keboola block appended wholesale (flow scaffolds, issue #650). - build_pushed_config_files() (component_service): with an explicit --configuration body the local file mirrors the pushed (already encrypted) configuration via api_config_to_local -- placeholder scaffolding would make the next push overwrite the real remote body with TODO templates. - Branch-aware placement: the scaffold lands in the subtree of the branch the config was ACTUALLY created in (push result branch_id). Unregistered branches are added to manifest.branches exactly like 'sync pull --branch' would; on failure files fall back to 'branch-{id}/' -- never to the default tree. New module sync/branch_registry.py hosts the logic (sync_service.py is over its grandfathered size budget; _ensure_branch_registered moved there too, thin delegates kept). - --json gains an additive local_scaffold {directory, files} key; human mode prints the write location and any placement warnings. - Docs: scaffold-workflow, gotchas (since vNEXT), commands-reference, keboola-expert matrix, CLAUDE.md, context.py. No version bump -- vNEXT placeholders resolve in the next release PR. Fixes #644
…eview)
Substantive:
- Mirrored-body path (--configuration + --output-dir) now materializes the
directory EXACTLY like sync pull: api_config_to_local + extract_code_files
+ shared dump_config_yaml. A pushed SQL/Python body yields a real
transform.sql/.py (not placeholders, which merge_code_files would have
pushed over the real code; and not nothing, which dropped companion files
entirely -- the review's most-confirmed finding). _description.md parity
included. materialize_pushed_config replaces build_pushed_config_files.
- Placement policy moved out of the command layer into
sync/branch_registry.resolve_scaffold_placement + a thin SyncService
delegate (3-layer rule); returns a frozen ScaffoldPlacement dataclass
instead of a new bare tuple (CONTRIBUTING.md rule).
- default_branch_prefix does a tolerant raw-JSON peek (historical
_detect_branch_prefix semantics; full-manifest validation broke partial
manifests and was wasted work for a read).
- stamp purity: companion entries are copied, not shared; name fallback
handles a falsy API echo (str(push_result.get("name") or name));
"config_id recorded" banner only claims it when an id was stamped;
local_scaffold gains a config_id key.
- Windows: scaffold writes use newline="" (LF-only, same convention as
SyncService._write_config_file).
- dump settings deduplicated (dump_config_yaml), branch-{id} spelling
single-sourced (fallback_branch_dir), dead defensive tail removed,
_DETECT_BRANCH_PREFIX sentinel removed.
Tests: new tests/test_branch_registry.py (14 cases incl. project-mismatch
degrade and registration-failure fallback); real-generator stamp
integration test; transformation-body extraction test; CLI placement tests
reworked to the ScaffoldPlacement API; new E2E step 19c2 for
--push --output-dir stamping.
bf0d125 to
68b2daa
Compare
- code_extraction: tolerate a body with 'parameters': null (key present,
value None) -- .get(default) does not fire and .get('blocks') on None
crashed AFTER the remote create succeeded. Latent in the sync pull
path too; guarded at all three extraction sites.
- resolve_scaffold_placement: the production path now runs the same
foreign-workspace check as the dev path -- a create pointed at another
project's sync workspace writes FLAT (inert, outside every branch
tree) with a warning naming the mismatch, instead of silently
landing in that workspace's main/ tree where its next sync push
would duplicate the config into the wrong project.
- materialize_pushed_config: report only files THIS call wrote (the
slugified dir can pre-exist with stray files), and remove a stale
_description.md when the pushed description is empty so it cannot
misattribute to the new config_id.
- CLI-level coverage for the mirror branch (transformation body ->
transform.sql through the real Typer command) and a Windows-safe
Path comparison in the envelope test (str.endswith on backslashed
paths failed the Windows CI job).
|
Follow-up to the posted reviews: commits 68b2daa (rebase over main after #645–#647) and f0cf8c6 address the full review round — Devin's companion-files finding (mirror path now materializes exactly like |
Why
config new --push --output-dir— a combination documented as valid ("Scaffold AND remote create") — wrote a scaffold that did not carry the identity of the configuration it had just created, and always wrote it into the default branch's tree. The nextsync pushclassified the directory as a brand-new configuration and created a duplicate. A real incident produced 34 duplicate configurations in a single dev branch (issue #644).Live verification on a real project (v0.88.0) before the fix confirmed all three legs:
config_id→sync diffreportsadded: 1+remote_only: 1→ push duplicates (exact issue repro).branch createauto-activates the new branch, andcreate_configfalls back to the active branch — so even without--branch, the config was created in the dev branch while the scaffold landed inmain/. This is the reporter's incident path._keboola.config_idflips the diff tomodified(the adopt-by-id guard from sync push creates duplicate configs for every config a dev branch inherits from main (even with zero local changes) #482 pairs the directory with the existing remote config) — proving the fix mechanism.What changed
stamp_scaffold_config_id()(services/component_service.py, pure function): after a successful POST the scaffold's_config.ymlrecords_keboola.config_id(double-quoted — legacy numeric IDs must stay YAML strings or the string-keyed adopt-by-id lookup never matches) and the misleading# NOTE: config_id will be assigned by Keboola on first pushis replaced. A_config.ymlwithout a_keboolablock (flow scaffolds, issue config new: flow scaffold _config.yml lacks the _keboola block, sync push sees component 'unknown' #650) gets one appended.branch_idfrom the push result — covers both--branchand the active-branch fallback). An unregistered branch is added tomanifest.branchesexactly the waysync pull --branchdoes (same naming, same fallback), so a later pull reuses the directory. If registration fails, files fall back tobranch-{id}/— never to the default tree — with a warning inwarnings[].build_pushed_config_files(): with an explicit--configurationbody the local file mirrors the pushed (already encrypted) configuration viaapi_config_to_local+ the pull-side YAML dump settings. Writing placeholder scaffolding there would make the nextsync pushoverwrite the real remote body with TODO templates; mirroring also means the follow-upsync diffis clean.sync/branch_registry.py: hosts the registration logic;_ensure_branch_registeredmoved there verbatim (thin delegates kept onSyncService) becausesync_service.pyis over its grandfathered size budget and must not grow.--jsongains an additivelocal_scaffold: {directory, files}key on this path; human mode prints the write location and any placement warnings.--push) is byte-for-byte unchanged, including the "assigned on first push" comment, which is correct there.Docs
scaffold-workflow.md,gotchas.md(tagged(since vNEXT)per the release-PR convention),commands-reference.md,keboola-expert.mdtool matrix (49,099 B, within the 70,000 B budget),CLAUDE.md,commands/context.py. No version bump / changelog entry — the next release PR resolves thevNEXTplaceholders.Tests
tests/test_component_service.py: unit coverage for the stamp function (existing block, missing block, numeric-ID quoting, purity) and the pushed-body mirror.tests/test_config_create_cli.py: CLI coverage — ID stamped in the written file,local_scaffoldin the envelope, placement into a registered branch dir, registration of an unknown branch,branch-{id}/fallback on registration failure (never the default tree), flat layout without a manifest, pushed-body mirroring.make checkgreen: 5877 passed, 12 skipped; LOC gate, version gates (419), command sync (262 commands), sentinel guards all pass.Related follow-ups filed and being fixed separately: #649 (orphaned
main/tree aftersync pull --branchmass-flags production diff), #650 (flow scaffold missing_keboolablock, PR #652).Fixes #644