fix(sync): pull never deletes a dir it wrote or a locally edited dir (#792 A, C)

[padak]

Stacked on #793 (formal-verification pilot). Fixes findings A and C from #792.

Problem

sync pull drops manifest entries whose config is gone from the remote and deletes their directories. Two data-loss paths:

• A -- a config deleted in the UI and re-created under the same name: pull wrote the new config into the old directory, then the sweep deleted that same directory. The next sync push DELETEd the live new config remotely.
• C -- a config deleted remotely while it had un-pushed local edits: plain pull and pull --force deleted the edited directory with no warning or conflict.

Behavior change

• A new config never lands in a stale entry's directory that is still on disk; it gets the usual collision suffix (<name>-<id prefix>), and the next pull renames it back. The sweep also never deletes a path that an entry of the same pull owns. After pull, the manifest matches disk.
• A locally edited directory (_config.yml, companion files, row files) whose remote vanished:
  • plain pull: kept, manifest entry kept, reported as skipped with reason locally modified, deleted on remote
  • pull --force: aborts with SYNC_CONFLICT; the conflict carries reason: "deleted on remote"
  • pull --theirs: deleted (remote wins, unchanged)
• An unedited stale directory is still removed, as before.

Documented in gotchas.md and sync-workflow.md as (since vNEXT). The findings table in formal/sync/README.md marks A and C as fixed. No version bump, no changelog entry.

Tests

• test_a_* and test_c_* in tests/test_sync_formal_counterexamples.py no longer carry xfail, and they pass.
• New tests: plain pull reports skipped; --force raises SYNC_CONFLICT; --theirs still deletes; an unedited stale directory is still swept and the manifest matches disk.
• make check is green: 6909 passed, 8 xfailed (the remaining findings). loc-check is OK: the sweep moved to the new services/_sync_stale.py, so sync_service.py shrank.

[padak]

Review of #796 — fix(sync): pull never deletes a dir it wrote or a locally edited dir (#792 A, C)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR fixes two data-loss paths in sync pull's stale-entry sweep (findings A and C from the #792 formal-verification pilot): (A) a config deleted-then-recreated under the same name no longer has its new directory destroyed by the sweep, and (C) a locally-edited directory whose remote config was deleted is now preserved (skipped) instead of silently rmtree'd. The refactor is clean (sweep logic extracted to services/_sync_stale.py, sync_service.py shrank), make check is fully green (6909 passed, 8 xfailed, matches the PR's claim), and both fixes are empirically verified against the counterexample harness. No BLOCKING issues found. The main substantive concern is that the fix for C, by design, funnels straight into the still-open, documented Finding H (silent config resurrection on push) — confirmed by direct reproduction below — and this consequence is under-documented and untested end-to-end. Verdict: COMMENT.

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_stale.py:153-164 — fix for C hands the config straight to Finding H (silent resurrection with a new remote id) on the next push, with no test or explicit warning

Reproduced live against the test harness (World): edit a config locally, delete it remotely, run plain sync pull (preserves it, reports skipped) — then run plain sync push. diff_engine.compute_changeset classifies the kept entry as "added" (its remote key is gone from remote_configs), so push_create mints a brand-new remote config id (cfg-1 -> cfg-100 observed), silently overriding whatever the other actor intended by deleting it. This is exactly formal/sync/README.md's Finding H ("A config deleted remotely by another actor is silently re-created by the next push, no warning"), still xfail and unfixed in this same PR's own findings table — so C's fix for local-edit data loss trades it for a documented-but-unfixed resurrection bug, one step removed. gotchas.md/sync-workflow.md do mention "the next sync push re-creates the config" but don't call out that it lands under a new id or that this is literally Finding H; there is also no regression test covering the full pull-then-push chain (only the single-pull skipped state is tested). Recommend: cross-reference H explicitly in the docs, and/or have push_create surface a distinct warning (not a plain "added") when creating a config for a manifest entry that already carried an id.

[NB-2] src/keboola_agent_cli/services/sync_service.py:706-731 — the "rename back" half of finding A's fix is undocumented in tests and mislabels the event as "renamed"

Reproduced live: pull #1 writes the recreated config into orders-cfg-2 (suffixed); pull #2, with no remote change at all, git-mv's it back to orders and reports {"action": "renamed", "old_path": "extractor/.../orders-cfg-2"} — identical in shape to a genuine "config renamed in the UI" event. An agent or script consuming --json pull output cannot distinguish "we cleaned up our own naming collision" from "the user renamed this config in Keboola," which could trigger incorrect downstream automation (e.g. a rename notification). tests/test_sync_formal_counterexamples.py::test_a_unedited_stale_dir_is_still_swept only checks the first pull; no test exercises the second pull that performs this rename-back, even though it's the exact behavior documented in gotchas.md's new entry ("the next pull renames the directory back"). Recommend a dedicated regression test for the two-pull sequence, and consider a distinguishing action value (e.g. "reclaimed") or an extra field so downstream consumers don't conflate it with a real rename.

[NB-3] src/keboola_agent_cli/services/_sync_stale.py:105-118 — remote_deleted_conflicts always emits an empty config_name

config_name is hardcoded to "" in both remote_deleted_conflicts (line 112) and the skipped detail (apply_stale_sweep line 159, which reuses s.entry.path for config_name too, not an actual name). A human running sync pull --force on a real project with several conflicting configs sees a conflict list identifying entries only by component_id/path, not by the human-readable config name shown everywhere else in kbagent's conflict/diff output. Minor UX regression versus the sibling detect_force_pull_conflicts conflicts, which do carry names. Not blocking since the path still uniquely identifies the entry, but worth a follow-up.

Nits

• [NIT-1] formal/sync/README.md — the findings table marks A and C "fixed" but the discussion in NB-1 shows C's fix is a data-loss-to-resurrection trade rather than a full fix; consider a footnote pointing at H so a future reader doesn't assume C is closed independently of H.

Verification log

• gh pr view 796 --repo keboola/cli --json title,body,files,... → 6 files, +307/-75, base feat/792-formal-sync-pilot (stacked, as described), title fix(sync): ... matches conventional-commit type for a bug fix ✓
• Checked out PR in an isolated clone (gh pr checkout 796), never touched the caller's worktree ✓
• Read src/keboola_agent_cli/services/_sync_stale.py in full (174 lines) and the relevant sync_service.py pull/push/diff sections ✓
• grep for layer violations (typer/click/print in services, httpx in commands) → empty, no violation ✓
• uv run pytest tests/test_sync_formal_counterexamples.py -q → 7 passed, 8 xfailed ✓ (matches PR claim; test_a_*/test_c_* no longer xfail)
• make check → ruff check OK, ruff format --check OK, ty check OK, changelog-check OK (no entry required, correctly following the no-version-bump convention on a feature PR), check_error_codes.py OK, 6909 passed, 15 skipped, 8 xfailed ✓
• make loc-check → sync_service.py not flagged (confirms the claimed shrink), no ceiling regressions from the new _sync_stale.py ✓
• Reproduced end-to-end (custom script against the World test harness, not committed): edit-then-remote-delete -> plain pull (skipped, kept) -> plain push -> remote config re-created under a new id (cfg-1 popped, cfg-100 created), confirming NB-1 ✓
• Reproduced end-to-end: recreate-under-same-name -> pull #1 (orders-cfg-2, suffixed) -> pull #2 with no remote change (renamed back to orders), confirming NB-2 ✓
• No new CLI commands added/removed/renamed in this PR (diff touches only services/, formal/, plugins/.../gotchas.md+sync-workflow.md, and tests) → Plugin synchronization map (CONTRIBUTING.md) rows for command surfaces (context.py, CLAUDE.md, commands-reference.md, OPERATION_REGISTRY) do not apply; the two doc surfaces that DO apply for a behavior change (gotchas.md, sync-workflow.md) are both updated with (since vNEXT) tags ✓
• No version bump / no changelog entry — correct per CONTRIBUTING.md "Feature/fix PRs never bump the version" ✓

Open questions for the author

• Is the NB-1 push-resurrection consequence an accepted, deliberate trade-off (better than losing local edits) that will simply wait for Finding H's own fix, or should this PR add a push-time warning now, before the behavior ships? Either answer is reasonable — flagging so it's a conscious choice rather than an implicit one.