Repository navigation
fix(sync): promoting main/ to a dev branch is idempotent (#792 D) - #794
Conversation
When a dev branch lacks a production config, sync push --branch <dev> (KFR-07 promote) creates a dev copy and records it under the dev branch with the same relative path. Diff kept comparing the main/ dir against the production entry, so every further push created another dev copy. On the promote path the target-branch entry now shadows the production entry for the same dir (sync/branch_scope.py::_promoted_paths): a repeated push creates nothing and a later edit updates the one dev copy. Removes the xfail on test_d_promote_push_is_idempotent and adds a diff-clean / edit-updates regression test.
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Review of #794 — fix(sync): promoting main/ to a dev branch is idempotent (#792 D)
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-equivalent commands, not duplicated here.
Summary
This PR fixes finding D from the #792 formal-verification pilot: sync push --branch <dev> on a tree where only main/ exists on disk (the KFR-07 "promote" path) used to create a fresh dev-branch copy of every unaffected production config on every push, because the manifest scoping had no way to know a previous promote push had already recorded a dev-branch entry for that same main/ directory. sync/branch_scope.py::_promoted_paths now makes the target-branch entry shadow the production entry for the same on-disk dir, but only on the genuine promote path (target branch id has no materialized <branch>/ tree of its own). I traced the logic and empirically reproduced both the pre-fix bug and the post-fix behavior against a fresh isolated checkout, plus the requested focus areas (production push/pull, git-branching, branch-link/unlink, the I8 sync status quirk). No blocking issues found; the guard that makes production immune to the shadowing is correct and test-covered. One non-blocking documentation gap: the PR's own "known leftover" note about the I8 quirk understates its actual scope (it doesn't mention the production entry also drifts). Verdict: COMMENT.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 1
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/skills/kbagent/references/gotchas.md:861-875, formal/sync/README.md:82 — I8 leftover note undersells its own scope: production's own entry also shows false drift, not just the dev copy
I reproduced the promote push against an isolated checkout (sync/branch_scope.py unchanged in this part) and confirmed that a promote push rewrites the shared main/.../_config.yml file's _keboola.config_id from the production id (cfg-1) to the newly created dev id (cfg-100) — this happens in _sync_writeback.py's create-writeback path and is pre-existing (identical on the pre-#792D base commit f6a29c8, not introduced by this PR). Consequence: offline sync status (which hashes the raw file, unlike diff() which strips _keboola before comparing) now reports the production manifest entry (cfg-1) as modified — not merely the dev entry as "deleted" — even though nothing was actually edited and the online sync diff/sync push for production correctly report "unchanged" / "no_changes". I verified this live: after one promote push, sync status returns {"modified": [cfg-1 ...], "deleted": [cfg-100 ...]} while sync diff --branch <production> returns summary.unchanged == 1. The current gotchas.md/README.md text only says the dev entry looks "deleted"; a reader (human or AI agent) could reasonably read that as "harmless dev-side cosmetic drift" and not expect the production row to also show as dirty in sync status. Since this PR is explicitly not fixing I8, I'm not asking for a code fix here — just for the doc note to mention both sides of the drift so sync status output isn't misread as a real production change needing a push.
(none)
Nits
[NIT-1]src/keboola_agent_cli/sync/branch_scope.py:200-207— the shadowed production entry (promote_key in promotedandcfg.branch_id != target_branch_id) is silently dropped from the changeset (not appended toin_tree,orphaned, ornever_fetched). That's correct fordiff()'s summary counts (verified:added == 0), but it means there is no--jsonbreadcrumb explaining why a production config that exists in the manifest is invisible to async diff --branch <dev>run. Not required for this fix, but an optional future improvement (e.g. anorphaned-style entry with a distinct reason) would help debuggability.
Verification log
gh pr view 794 --repo keboola/cli --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ draft PR, basefeat/792-formal-sync-pilot, 6 files, +99/-20,fix(sync):prefix matches a bug fix ✓- Isolated clone +
gh pr checkout-equivalent (git fetch origin pull/794/headinto a scratch dir outside the review target worktree, never touched/Users/padak/.../worktrees/issue-689-pr-7ad2c7) ✓ git diff f6a29c8..0eaed1d -- src/keboola_agent_cli/sync/branch_scope.py→ reviewed the full_promoted_pathsaddition and its two guard conditions (target_branch_id is None,branch_tree_path(target) == source_branch_path)- Traced call sites:
scope_manifest(..., target_branch_id=branch_id)is only called fromSyncService.diff(), andpush()delegates todiff()with the samebranch_override— no separate code path for push, so the fix applies uniformly - Reproduced live with the repo's own
FakeApi/Worldtest harness (tests/test_sync_formal_counterexamples.py):w.push(branch=DEV)twice → second pushcreated: 0(bug fixed) ✓w.diff(branch=None)(production) after the promote push →{"unchanged": 1, "orphaned": 1},PROD ids still: ['cfg-1']— production diff stays clean, the dev copy is correctly excluded asorphaned/tracked_on_other_branch, never re-created or deleted ✓w.push(branch=None)(production) after the promote push →{"status": "no_changes", ...}— no accidental write to production ✓- Confirmed the safety guard algebraically: for a production target,
branch_tree_path(manifest, target_branch_id)andsource_branch_pathboth resolve to the default tree path, so_promoted_pathsalways returns an empty set on the production side — the shadowing can only ever apply when diffing/pushing the dev branch itself sync status(offline) after the same promote push →{"modified": [cfg-1], "deleted": [cfg-100], ...}— confirms the I8 leftover the PR documents, and additionally surfaces the production-side "modified" false positive (NB-1)- Re-ran the same repro against base commit
f6a29c8(pre-#792D fix) → the_config.ymlrewrite (config_id: cfg-1→config_id: cfg-100) is byte-identical, confirming it predates this PR - git-branching mode /
branch-link/branch-unlink:sync branch-link/unlinkonly writebranch-mapping.json, never touchmanifest.configurations;_resolve_branch_id()funnels the git-branching-resolved id through the exact samebranch_idvariable used by an explicit--branchoverride before callingdiff()/push()/scope_manifest()— no separate code path exists that could bypass the new guard, so no additional git-branching-specific risk was found
uv run pytest tests/ -q -k "sync or branch"→1029 passed, 18 skipped, 9 xfailed(the 9 xfailed are the pre-existing, unrelated formal counterexamples A/B/C/E/F/H/I/J/K)uv run ruff check+uv run ruff format --checkon the 3 changed Python files → all pass- Did not run the full
make check(skippedmake loc-check/make check-sentinel-guards/make check-error-codes— no new command surface, no new HTTP client, no new error codes in this diff, so those gates are not applicable) — targeted lint/format/test commands above cover everything this diff touches
Open questions for the author
(none)
# Conflicts: # formal/sync/README.md
keboola-pr-reviewer-bot
left a comment
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: auto_approve (risk 3/5) · profile keboola-mcp-server
Auto-approve: a well-scoped, test-backed bug fix making dev-branch promote pushes idempotent, with non-promote paths provably unchanged.
Concerns:
src/keboola_agent_cli/sync/branch_scope.py: Promote-path scoping is subtle; edge cases beyond tested scenarios rely on manifest correctness
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| return { | ||
| (cfg.component_id, cfg.path) | ||
| for cfg in manifest.configurations | ||
| if cfg.branch_id == target_branch_id | ||
| and (project_root / source_branch_path / cfg.path / CONFIG_FILENAME).exists() | ||
| } |
There was a problem hiding this comment.
🔴 Deleted promoted configs survive dev push
After a promoted config's main/ directory is deleted, _promoted_paths drops its path. The dev entry becomes orphaned, while the production entry's ID is absent on dev. sync push --branch dev leaves the deleted config live.
Learn more
A promote push creates a dev config with a new ID but records its manifest entry under the dev branch, while its file stays in the production tree. The next diff needs that entry in scope.in_tree even when the source file is gone: compute_changeset can then classify the missing file as a deletion. Requiring the source file to exist stops shadowing the production entry after local deletion. The production ID is absent from the dev remote, and the dev entry becomes orphaned; no deletion reaches push.
Example: Production has cfg-1 under main/extractor/orders; promoting creates cfg-100 on dev. Delete the main/ directory and push to dev again. cfg-100 remains live instead of being deleted.
Recommended fix: Keep the promoted dev entry in scope when its source file is removed, so tracked_keys includes its ID and diff plans its deletion. Identify the promotion relationship independently of source-file existence, while distinguishing genuinely promoted entries from those belonging only to a dev tree.
Was this helpful? React with 👍 or 👎 to provide feedback.
| promote_key = (cfg.component_id, cfg.path) | ||
| if promote_key in promoted: | ||
| if cfg.branch_id == target_branch_id: | ||
| in_tree.append(cfg) | ||
| continue |
There was a problem hiding this comment.
🔴 Promoted configuration rows never reach dev
When a production config has rows, scope_manifest replaces its entry with a new dev entry containing no rows. The row scanner treats production rows as tracked, so later row edits never reach dev.
Learn more
Each config's rows list in the manifest tracks the local row files. A promote create records a new dev config entry without copying rows in writeback_create_config_in_manifest. Shadowing the production entry means diff sees none of its rows. The separate find_untracked_rows walks the production entry and recognizes those row paths as already tracked. Thus edits to local rows never produce a dev row change, and the newly created dev config has no rows.
Example: Orders has a production values row. The first promote creates a dev Orders config without that row; editing main/Orders/rows/values/_config.yml and pushing again produces no row create or update.
Recommended fix: Ensure promotion maps each source row to a target row, creating missing rows on the new dev config and stamping their new IDs and baseline in its manifest entry. Make the untracked-row scan aware of the promoted parent rather than treating production row paths as already tracked for dev.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return { | ||
| (cfg.component_id, cfg.path) | ||
| for cfg in manifest.configurations | ||
| if cfg.branch_id == target_branch_id | ||
| and (project_root / source_branch_path / cfg.path / CONFIG_FILENAME).exists() | ||
| } |
There was a problem hiding this comment.
🔴 Unrelated dev configs can overwrite each other
If a dev tree is removed while main/ has the same component and path, _promoted_paths treats its dev entry as promoted. A dev push then compares the production file against that dev config, potentially overwriting unrelated dev work.
Learn more
The source tree falls back to main/ when the target branch tree has no config files; source resolution invokes that fallback. A dev entry might instead have been recorded by a dev pull and belong to a separate config with the same component and relative path as a production config. If its dev directory is removed, the new path-based test considers it promoted without checking its origin. scope_manifest compares the production file to that dev ID; a push update then writes it to the unrelated dev config whenever the diff classifies a local modification.
Example: main/extractor/orders tracks production cfg-1; an earlier dev pull tracked dev-only cfg-9 at dev/extractor/orders. Remove dev/, edit the production file, and push to dev. The dev entry cfg-9 receives the production file's content instead of remaining untouched.
Recommended fix: Record promotion provenance or the source config ID during writeback and use it to pair entries. A matching component/path and an existing source file cannot establish that the dev entry was promoted from that file.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def test_d_promote_push_diff_is_clean_and_edits_update_the_dev_copy(tmp_path: Path) -> None: | ||
| """After a promote push, `sync diff --branch dev` is clean (TLA I6), a | ||
| third push still creates nothing, and a later local edit UPDATES the dev | ||
| copy the first push created -- never production, never a new copy. | ||
|
|
||
| Issue #792 finding D. | ||
| """ | ||
| w = World(tmp_path) | ||
| w.api.put(PROD, "cfg-1", "Orders", "a") | ||
| w.init() | ||
| w.pull() | ||
|
|
||
| w.push(branch=DEV) | ||
| (dev_id,) = w.api.ids(DEV) | ||
| assert w.diff(branch=DEV)["summary"]["added"] == 0 | ||
| assert w.push(branch=DEV).get("created", 0) == 0 | ||
|
|
||
| cfg_file = w.config_dir("orders") / "_config.yml" | ||
| cfg_file.write_text(cfg_file.read_text().replace("value: a", "value: b")) | ||
| third = w.push(branch=DEV) | ||
| assert third.get("created", 0) == 0 | ||
| assert third.get("updated", 0) == 1 | ||
| assert w.api.ids(DEV) == [dev_id] | ||
| assert w.api.remote[DEV][dev_id]["configuration"]["parameters"]["value"] == "b" | ||
| assert w.api.remote[PROD]["cfg-1"]["configuration"]["parameters"]["value"] == "a" |
There was a problem hiding this comment.
🔍 Promotion tests cover only parent edits
The new regression test exercises a rowless config with its directory present. Extend it to cover rows and deletion of the promoted directory.
Was this helpful? React with 👍 or 👎 to provide feedback.
Problem (finding D from the #792 formal-verification pilot)
sync push --branch <dev>with onlymain/on disk promotes the default tree to the dev branch (KFR-07). For a production config the dev branch does not have (typically one created in production after the branch was cut), the push creates a dev copy under a new id. The nextsync diff --branch <dev>still reported that config asadded, and every further push created another dev copy -- N pushes, N copies of the same config on the branch.Configs the dev branch already holds under the production id (the normal case -- a branch starts as a copy of production) were never affected; verified with the same harness (diff
unchanged, push creates nothing).Behavior change
On the promote path (target branch has no materialized tree of its own), a target-branch manifest entry recorded for a
main/directory now shadows the production entry for that same directory (sync/branch_scope.py::_promoted_paths, wired viascope_manifest(target_branch_id=...)). Result:sync diff --branch <dev>after a promote push is clean;main/UPDATEs the one dev copy; production is untouched;Non-promote paths (production, or a target with its own
<branch>/subtree) are unchanged --_promoted_pathsreturns an empty set there.Docs:
gotchas.md(tagged(since vNEXT), incl. cleanup hint for older versions) andsync-workflow.mdpromote section.formal/sync/README.mdrow D marked fixed.Evidence
test_d_promote_push_is_idempotent: xfail(strict) removed, now passes.test_d_promote_push_diff_is_clean_and_edits_update_the_dev_copy: push, diff clean, push again (0 creates), edit, push -> 1 update of the same dev id, prod unchanged.cfg-101next tocfg-100on dev; after: dev keeps exactlycfg-100.uv run pytest tests/ -q -k sync: 728 passed, 9 xfailed.make check: 6905 passed, 9 xfailed;make loc-checkOK.Known leftover (not fixed here)
TLA I8 (manifest matches disk) still holds a gap: after a promote push, offline
sync statusreports the dev entry asdeleted(its file lives inmain/, not<dev>/). Pre-existing, unchanged by this PR.