fix(sync): sync push no longer duplicates configs inherited from main on a dev branch - #494
Conversation
…482) sync pull replaces manifest.configurations with the pulled branch's entries, so after `branch use <dev>` + pull the previously pulled main/ tree is orphaned on disk. The untracked-config walker kept the default branch tree in scope unconditionally, so every orphaned file surfaced as "added" (with empty config_id) and push created a duplicate config on the dev branch for each of them -- 100 duplicates from one push in the report. The dev-branch listing itself returns inherited configs fine; the bug was purely local manifest bookkeeping. - diff/push now scan for untracked configs only in the resolved source branch subtree (the one push reads from); other branch trees can no longer leak phantom "added" entries. The issue #267 scaffold flow and the KFR-07 promote-default-tree flow keep working (source resolution is unchanged). - Adopt-by-id guard: an untracked _config.yml whose _keboola.config_id resolves on the target branch and is not claimed by any manifest entry now diffs against the existing remote config (unchanged or modified) instead of creating a duplicate. A claimed id (a tracked config dir copied to fork it) keeps the fresh-create semantics. - sync status scanning is unchanged. Fixes #482
| component_id = added_cfg.get("component_id", "unknown") | ||
| merge_code_files(component_id, local_data, config_dir) | ||
| # Adopt-by-id guard (issue #482): an untracked file whose | ||
| # ``_keboola.config_id`` resolves on the target branch and is not | ||
| # claimed by any manifest entry refers to an EXISTING remote | ||
| # config -- diff against it (unchanged/modified) instead of | ||
| # letting push create a duplicate. A claimed id means the user | ||
| # copied a tracked config dir to fork it: keep the create. | ||
| untracked_id = added_cfg.get("config_id", "") | ||
| untracked_key = f"{component_id}/{untracked_id}" | ||
| if not ( | ||
| untracked_id | ||
| and untracked_key in remote_configs | ||
| and untracked_key not in tracked_keys | ||
| ): | ||
| untracked_id = "" # new config, push creates it |
There was a problem hiding this comment.
🔍 Adopt-by-id 'modified' updates a config never added to the manifest
For the adopt-by-id path, an untracked file whose config_id resolves on the target branch is diffed as modified and pushed via push_update (src/keboola_agent_cli/services/sync_service.py:1563-1587). The post-update hash writeback loop at sync_service.py:1580-1585 searches manifest.configurations for a matching component_id/config_id, but the adopted config is by definition NOT tracked, so the loop finds nothing and no manifest entry is created. This is not a correctness bug (the next diff recomputes the adopt-by-id and finds local==remote → unchanged, so it is idempotent), but the adopted config remains permanently untracked in the manifest, and each subsequent sync diff re-scans and re-adopts it rather than treating it as a normal tracked entry. Worth confirming this matches intent.
Was this helpful? React with 👍 or 👎 to provide feedback.
padak
left a comment
There was a problem hiding this comment.
Review of #494 — fix(sync): sync push no longer duplicates configs inherited from main on a dev branch
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 #482: after branch use <dev> + sync pull, the previously-pulled main/ tree became orphaned in the manifest, and sync push treated every orphaned file as a brand-new config, duplicating up to ~100 configs onto the dev branch. The fix (a) scopes _find_untracked_configs to the single resolved source branch path instead of a set of "in-scope" branch ids, and (b) adds an adopt-by-id guard so an untracked file whose _keboola.config_id matches an existing, unclaimed remote config is diffed as modified rather than created as a duplicate. The change is service-layer only (no new/removed/renamed CLI command), comes with three targeted regression tests that directly encode the reported repro, and make check is fully green. Verdict: COMMENT — no blocking issues found; a few non-blocking follow-ups worth tracking (manifest reconciliation gap for adopted configs, deferred gotcha/E2E documentation).
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 3
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/sync_service.py:1580-1583 — adopted config is never written back into the manifest
When an untracked file is "adopted" via the new id-matching guard (sync_service.py:1253-1264) and pushed as modified, push_update() succeeds, but the post-update loop at sync_service.py:1580-1583 (for cfg in manifest.configurations: if cfg.component_id == component_id and cfg.id == config_id) finds nothing, because the adopted config was never added to manifest.configurations. The file therefore never converges into a normal tracked state: it stays permanently outside the manifest, so a later local deletion of that file is never detected as deleted (tracked_keys never contains it), and every future sync diff/push re-derives the adopt decision from scratch instead of reading a stable manifest entry. Consider adding a manifest writeback (similar to writeback_create_config_in_manifest) for the adopted-by-id path so the reconciliation is durable, not re-computed every run.
[NB-2] plugins/kbagent/skills/kbagent/references/gotchas.md / plugins/kbagent/agents/keboola-expert.md §3 — no gotcha entry for the pre-fix duplication risk
Given the severity of the underlying bug (up to ~100 duplicate configs created from a single push after a branch switch), this is exactly the class of behavior CONTRIBUTING.md's "Plugin synchronization map" wants tagged (since vX.Y.Z) once a version is cut, so AI agents on older kbagent installs following the §4.5 "Cross-project migration" workflow (branch use → pull → edit → push --dry-run) know to upgrade first. Not flagging BLOCKING here because the three most recent merged PRs on main (#486, #487, #488) show the team currently batches these hand-maintained doc updates at release time rather than per-PR (no version bump lands with this PR either — pyproject.toml stays at 0.70.0). Recommend adding the gotcha in the next release's manual-review pass (CONTRIBUTING.md "Releasing a new version" steps 5-6), tagged with whatever version this fix ships in.
[NB-3] tests/test_e2e.py — no E2E test for the specific branch-switch-then-push regression
test_sync_diff_branch_override (tests/test_e2e.py:10643) exercises sync diff --branch <id> but doesn't reproduce the reported sequence (pull main → branch use/pull dev → push with zero edits, asserting no_changes/no duplicate create_config calls). This isn't a "new command" so it doesn't strictly trigger CONTRIBUTING.md's per-command E2E checklist, but given the real-world impact (a live-project bug that created ~100 duplicate configs), an E2E test against a real disposable branch would catch anything the fully-mocked service-layer tests can't (real manifest serialization round-trip, real branch-id typing, real API duplicate-detection). Every CLI command must have E2E coverage per CONTRIBUTING.md, but environmental constraints can defer one cycle — flagging for awareness, not blocking.
Nits
[NIT-1]src/keboola_agent_cli/services/sync_service.py(2272 LOC) — already well past both the soft (1000) and hard (1500) ceiling CONTRIBUTING.md sets forservices/*.py, and this PR adds another net +28 lines to it. The fix itself is small and surgical so it isn't worth blocking on a refactor here, but the next PR that adds material functionality to this file should extract the untracked-config walker + adopt-by-id logic into a sibling module (following the existing_sync_push_ops.py/_sync_writeback.pyextraction pattern) before adding more.
Verification log
gh auth status→ authenticated aspadak, scopesrepo/workflow✓gh pr view 494 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN, 2 files (src/keboola_agent_cli/services/sync_service.py,tests/test_sync_service.py), +256/-32, titlefix(sync): ...— conventional-commit prefix matches (bug fix) ✓git rev-parse --abbrev-ref HEAD→claude/issue-482-4d01d4, matches<branch>input — working tree correctly checked out on the PR branch ✓- Read
CONTRIBUTING.mdin full (Coding Style, Code Quality Patterns, Security Principles, "Checklist: Adding a New CLI Command", "Plugin synchronization map", "Releasing a new version") ✓ - Read
CLAUDE.mdconvention #17 (silent-drift surfaces) and## All CLI Commands— confirmed this PR adds/removes/renames zero CLI commands, so the command-surface checklist (context.pyAGENT_CONTEXT,CLAUDE.md,OPERATION_REGISTRY,commands-reference.md,server/routers/) is not applicable ✓ - Read
plugins/kbagent/agents/keboola-expert.md§1 (non-negotiable rules) and §3 (inline gotchas) — no existing entry for issue #482; see NB-2 ✓ git diff --stat main...HEAD→ confirmed onlysrc/keboola_agent_cli/services/sync_service.py(+82/-27 counting context lines) andtests/test_sync_service.py(+206/-5... net +201/-5 test additions) changed; no command/CLI/docs files touched ✓- Layer-violation greps (
typer/click/formatter./console.printin services;httpx/requestsin commands) against the diff → all empty ✓ no 3-layer violation (change is service-layer only) - Convention greps (magic numbers, raw
error_code="..."strings, bareexcept:,print()in prod code, token/secret leakage, new baretuple[...]returns) against the diff → all empty ✓ - Read
diff_engine.compute_changeset(src/keboola_agent_cli/sync/diff_engine.py:299-430) to verify the adopt-by-id path correctly falls into the "no base hash → 2-way compare" branch, producingmodified(content differs) or a silent skip (content identical), and correctly marks the remote key asseenso it isn't double-flagged asdeleted✓ - Read
push_update(src/keboola_agent_cli/services/_sync_push_ops.py:318-370) and the push-loop's post-update manifest hash writeback (sync_service.py:1560-1584) to trace the manifest-reconciliation gap in NB-1 ✓ uv run pytest tests/test_sync_service.py -k Issue482 -v→ 3 passed (test_push_on_dev_branch_after_pull_is_noop,test_untracked_file_with_known_remote_id_updates_instead_of_creating,test_untracked_copy_of_tracked_config_still_creates_fresh) — the exact issue #482 repro is asserted asno_changes/ zerocreate_configcalls ✓make check(lint + format-check + typecheck + skill-check + version-check + command-sync-check + changelog-check + check-error-codes + test, chained with hard-stop on first failure) → completed through the final test run: 4317 passed, 8 skipped, 131 deselected ✓ — sincemakestops at the first failing prerequisite, this also confirms lint/format/typecheck/skill-freshness/version-sync/command-sync/changelog/error-code gates all passedwc -l src/keboola_agent_cli/services/sync_service.py→ 2272 LOC (pre-existing over-hard-ceiling condition, not newly introduced by this PR) — see NIT-1- Could not reproduce the fix against a live Keboola project (no token access, per policy that AI agents never handle Keboola API tokens); relied on code-level trace of the full diff/push/manifest interaction plus the new mocked-client regression test suite, which directly encodes the reported production repro — see NB-3 for the E2E gap this leaves
Open questions for the author
(none)
Backfills changelog entries for ten PRs merged since v0.66.1 without a version bump (#465 #486 #487 #488 #490 #492 #493 #494 #495 + #500), attributed to their bump windows (0.67.0 / 0.70.0 / 0.70.1 / 0.71.0), and aligns the version at 0.71.0 as the catch-up release: 0.67.0-0.70.1 were merged to main but never tagged or published, so auto-update users are still on 0.66.1. The v0.71.0 tag + GitHub Release follow.
Fixes #482
Root cause
Not the Storage API and not the branch listing —
sync pullon a dev branch receives all inherited configs fromGET /branch/{id}/components(that is exactly how pull materialized all 68 configs in the report). The bug is local manifest bookkeeping:sync pullreplacesmanifest.configurationswholesale with the pulled branch's entries (sync_service.py, end ofpull()). Afterbranch use <dev>+pull, the previously pulledmain/tree is still on disk but no longer tracked by the manifest._find_untracked_configskeeps the default branch tree in scope unconditionally (a deliberate choice from the issue sync git-branching: branch_id type confusion + walker scope guard block GitOps workflow #267 scaffold flow). Every orphanedmain/file is therefore reported as untracked.diff()turned each untracked file into anaddedchange with emptyconfig_id— matching the dry-run signature in the report ("66 of 68 configsaddedwith emptyconfig_id") — andpushthen POSTed a brand-new config onto the dev branch for each of them, reading the file content from the colliding path in the dev tree.Fix
diff/pushnow pass the resolved source branch path (_resolve_source_branch_path— the tree push actually reads from) to_find_untracked_configs, which scans exactly that subtree. Files belonging to another branch's tree can never surface asaddedfor the target branch. The issue sync git-branching: branch_id type confusion + walker scope guard block GitOps workflow #267 "scaffold locally then push" flow and the KFR-07 promote-default-tree flow (push --branch <id>with no per-branch subtree) both keep working, because they are defined by the same source-path resolution._config.ymlwhose_keboola.config_idresolves on the target branch and is not claimed by any manifest entry now diffs against the existing remote config (unchanged/modified) instead of being created as a duplicate. A claimed id (user copied a tracked config dir to fork it) keeps the fresh-create semantics, so the fork-by-copy workflow is unchanged, andsync cloneis unaffected (its tree is manifest-tracked and requires a fresh target).sync statusscanning is unchanged (it has no branch/remote context; orphaned trees still show there as local-onlyaddedfiles, which push now correctly ignores).Tests
TestIssue482BranchSwitchDuplicates::test_push_on_dev_branch_after_pull_is_noop— the exact repro from the issue (pull main → pull dev → push with zero edits). Fails on pre-fixmainwith 2 phantomaddedchanges (emptyconfig_id, one per orphaned config); now asserts diff is empty, push isno_changes, andcreate_configis never called.modified+update_config, nevercreate_config.addedwith emptyconfig_idand a fresh create.only_branch_pathparameter (same scenarios, expressed through the source-path resolution that diff/push now perform).make check: 4317 passed, 8 skipped.