fix(sync): pull protects local edits in companion files, not only _config.yml (#792 B)

[padak]

Fixes finding B from the #792 formal-verification pilot (stacked on #793).

Problem

sync pull decided "locally modified" from _config.yml alone. An edit that lived only in a companion file (transform.sql, transform.py, code.py, pyproject.toml, _description.md) was invisible to it. When the remote had also changed, the local edit was silently overwritten:

• plain pull wrote the remote version, with no skipped entry;
• pull --force wrote it too, instead of aborting with SYNC_CONFLICT.

That contradicts the documented rule "pull protects local edits". sync diff / sync push always counted those files.

Behavior change

Pull now checks every file the config's local representation is made of: _config.yml plus every file in the manifest's pull_extra_hashes, which is the same set diff/push merge back. The plain-pull guard and the --force conflict guard share one helper, config_locally_modified.

• Plain pull: the config is skipped / locally modified. The edit stays and is still pending for sync push, because a preserved entry now carries over its pull_extra_hashes.
• pull --force: if the remote changed too, the pull stops with SYNC_CONFLICT and writes nothing. If the remote is unchanged, the edit is preserved.
• pull --theirs: remote wins, same as before.
• A deleted config directory is still re-created (sync pull can register a config without materializing its dir; sync push --force then plans deleting that remote config #472).
• A cosmetic edit behaves as before (K; test_k_* still passes). The sync push stamps pull_config_hash from disk while diff compares an API-derived hash — every pushed config stays "REMOTE MODIFIED" forever (mirror of #466) #686 baseline and normalization logic is unchanged.
• Minor change: deleting only a companion file now counts as a local edit, as diff/push already treat it. To drop edits, use --theirs or delete the whole config directory. This is documented.

The needs_shape_migration special case, which checked companions only during the #686 shape migration, is now covered by the general check, so it has been removed.

Docs: gotchas.md gets a new section tagged (since vNEXT, #792), and the sync-workflow.md key behaviors and --force section are updated. The formal/sync/README.md findings table now marks B as fixed. There is no version bump and no changelog entry.

Tests

• test_b_pull_never_overwrites_local_sql_edit: xfail removed, now passes.
• New tests/test_sync_pull_companion_files.py covers:
  • an SQL edit under plain pull (preserved, still shows in status);
  • an SQL edit under --force (SYNC_CONFLICT, or preserved when the remote is unchanged);
  • --theirs overwrites;
  • a _description.md edit;
  • an unedited companion takes the remote change;
  • a deleted config dir is re-created;
  • a row file edit (preserved; conflict under --force).
• uv run pytest tests/ -q -k sync: 735 passed, 9 xfailed. Full make check: 6912 passed, 15 skipped, 9 xfailed.

---

[padak]

Review of #795 — fix(sync): pull protects local edits in companion files, not only _config.yml (#792 B)

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/targeted runs, not duplicated here.

Summary

The PR closes formal-verification finding B: pull's "locally modified" guard previously hashed only _config.yml, so an edit living solely in a companion file (transform.sql, _description.md, etc.) was silently overwritten by a remote change, both in plain pull and --force. The fix introduces one shared helper, config_locally_modified, used by both the plain-pull skip path and the --force conflict detector, and removes the now-redundant needs_shape_migration special case (its narrower protection is now subsumed by the general check). I traced the false-positive scenarios called out in the review brief (legacy manifests missing pull_extra_hashes, the #686 shape-migration path, deleted config directories, and the deliberate "deleted companion = local edit" behavior change) and found each one handled correctly and consistently with existing diff/status/push semantics. Verdict: APPROVE — no blocking issues found; one non-blocking note about CRLF-sensitivity is pre-existing repo-wide behavior, not something this PR introduces or worsens.

Verdict

• Verdict: APPROVE
• Blocking findings: 0
• Non-blocking findings: 1
• Nits: 0

Blocking findings

(none)

Non-blocking findings

[NB-1] src/keboola_agent_cli/services/sync_service.py:2250 (_file_hash) — companion-file protection now runs on every pull, inheriting the raw-byte-hash's CRLF sensitivity

_file_hash hashes raw bytes with no line-ending normalization. Before this PR, that raw-byte comparison for companion files (extras_modified) only ran during the narrow #686 shape-migration path; now config_locally_modified runs it on every plain pull for every config with recorded pull_extra_hashes. A companion file whose line endings get silently converted between pulls (e.g. a Windows checkout with core.autocrlf=true re-checking out a tracked working tree) would now read as "locally modified" on a plain pull where it previously would not have. This is not a new bug class — sync diff/sync push/status() already compare companion files this same byte-exact way (verified at sync_service.py:1176 and :1333) — so the behavior is at least internally consistent, but the surface area where it can trigger just grew from "migration only" to "every pull." Given project_windows_posix_file_assumptions is a known recurring bug class in this repo, worth a one-line callout in gotchas.md or a follow-up issue if Windows users hit spurious "locally modified" skips after this ships; not a merge blocker since the comparison semantics are unchanged from diff/push.

Nits

(none)

Verification log

• gh pr view 795 --repo keboola/cli --json ... → 7 files changed, +314/-83, fix(sync): prefix matches (bug fix), state OPEN ✓
• Isolated checkout via gh pr checkout 795 -R keboola/cli in a fresh clone under scratchpad (did not touch the /Users/padak/.../worktrees/issue-689-pr-7ad2c7 worktree) ✓
• Read full diff (gh pr diff 795) for _sync_baseline.py, sync_service.py, both test files, and the three doc files ✓
• Layer check: change confined to services/_sync_baseline.py + services/sync_service.py (Layer 2); no typer/httpx/formatter leakage ✓ (no layer violation)
• Traced config_locally_modified: if not pull_hash: return False and if not config_file.exists(): return False before touching companions → legacy manifests with no pull_hash yet, and pull's own "recreate deleted config dir" path (#472), are both correctly exempted (confirmed against test_deleted_config_dir_is_rematerialized) ✓
• Traced extras_modified: (extra_hashes or {}).items() on an empty/missing pull_extra_hashes dict never iterates → a legacy manifest entry pre-dating this feature cannot be falsely flagged on its first post-upgrade pull ✓ (no false positive)
• Traced #686 shape-migration interaction: detect_force_pull_conflicts compares against effective_stored_hash(...) (migration-lenient), so a legacy-shape baseline of an otherwise-unchanged remote is still not mistaken for a remote edit; config_locally_modified only inspects on-disk file bytes and is orthogonal to config-hash shape, so removing needs_shape_migration does not reintroduce or create a false positive here — its narrower protection (companions during a migration-triggered rewrite) is now a strict subset of the general check ✓
• Confirmed the "deleted companion file = local edit" behavior change matches pre-existing status()/push-diff semantics at sync_service.py:1176-1184 and :1333-1341 (both already treat a missing recorded companion file as "changed") — so this is a consistency fix, not new divergent behavior ✓
• Ran uv run pytest tests/ -q -k sync (after uv sync --extra server for fastapi) → 735 passed, 13 skipped, 9 xfailed — matches the PR's claimed 735 passed, 9 xfailed ✓
• Ran uv run pytest tests/test_sync_pull_companion_files.py tests/test_sync_formal_counterexamples.py -q → 10 passed, 9 xfailed (finding B's xfail marker correctly removed and now green) ✓
• Ran uv run ruff check + uv run ruff format --check on the four touched Python files → all pass ✓
• Docs: gotchas.md new section correctly tagged (since vNEXT, #792); sync-workflow.md updates are consistent with the code change; formal/sync/README.md finding-B row correctly updated to FIXED with the right test references ✓
• No new CLI command surface, no OPERATION_REGISTRY/permissions changes needed (pure internal pull-logic fix) ✓
• No version bump, no changelog entry — correct per this repo's "feature/fix PRs never bump the version" convention ✓

Open questions for the author

(none)

[keboola-pr-reviewer-bot]

Verdict: auto_approve (risk 2/5) · profile _default

Well-scoped, well-tested sync-pull bug fix that extends local-edit protection to companion files; safe to auto-approve.

[devin-ai-integration]

Devin Review found 1 potential issue.