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

formal/sync/README.md

@@ -100,7 +100,7 @@ the regression test for each in `tests/test_sync_formal_counterexamples.py`.
 | ID | Finding | Sources | Severity | Test |
 |----|---------|---------|----------|------|
 | A | Remote delete + recreate under the same name: pull writes the new config into the old directory, the stale sweep then deletes that directory; the next push DELETEs the live new config | Lean F8, TLA I2 | HIGH -- **fixed** (#792 A/C PR) | `test_a_recreate_under_same_name_does_not_delete_new_config` |
-| B | Pull compares only `_config.yml`: local edits in `transform.sql`/`code.py`/`_description.md` are silently overwritten when the remote changed, no SYNC_CONFLICT | Lean F6 | HIGH | `test_b_pull_never_overwrites_local_sql_edit` |
+| B | Pull compares only `_config.yml`: local edits in `transform.sql`/`code.py`/`_description.md` are silently overwritten when the remote changed, no SYNC_CONFLICT | Lean F6 | HIGH -- **FIXED** (pull's local-modification check now covers every `pull_extra_hashes` companion file) | `test_b_pull_never_overwrites_local_sql_edit` (unmarked regression guard) + `tests/test_sync_pull_companion_files.py` |
 | C | Remote delete + local edit: pull (plain/`--force`) deletes the locally-edited directory with no conflict | Lean F7, TLA I5 | HIGH -- **fixed** (#792 A/C PR) | `test_c_pull_never_deletes_locally_edited_dir_on_remote_delete` |
 | D | `sync push --branch dev` (promote) creates another dev copy of a prod-only config on every push | TLA I6/I1 | HIGH | `test_d_promote_push_is_idempotent` |
 | E | An untracked file carrying a config id (`config new --push --output-dir` scaffold / adopted orphan) is diffed 2-way: push overwrites a UI edit made after the scaffold was written | TLA I11 | MED | `test_e_adopted_scaffold_push_does_not_overwrite_remote_edit` |
@@ -120,9 +120,10 @@ and is deleted only by `--theirs`. That is what the I2 (sweep half), I5 and I8
 TLC/Lean rerun still reports them until the model's `Pull` is updated to match.
 Their tests are ordinary regression guards now.
 
-B, D..F, H and I reproduce on current code and are `xfail(strict=True)` --
+D..F, H and I reproduce on current code and are `xfail(strict=True)` --
 flipping to a hard failure the moment a fix lands is the point: delete the
 `xfail` marker to adopt the fix. G is kept `xfail` too even though the fix
 direction is a product decision (see the test's docstring). J reproduces and
 is `xfail`. K is deliberate, documented behavior, so it is an ordinary
-(unmarked) regression guard instead.
+(unmarked) regression guard instead. B is fixed: its marker was removed
+and the test is now an ordinary regression guard.

plugins/kbagent/skills/kbagent/references/gotchas.md

@@ -768,6 +768,31 @@ Versioning convention:
   `--allow-plaintext-on-encrypt-failure`, which would write the PAT in
   plaintext into Storage.
 
+## `sync pull` protects edits in `transform.sql` / `code.py` / `_description.md`, not only `_config.yml`
+
+*(since vNEXT, #792)*
+
+`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, so when the remote
+had also changed, the edit was **silently overwritten**: plain pull wrote the
+remote version, and `pull --force` wrote it too instead of raising
+`SYNC_CONFLICT`. `sync diff` / `sync push` always counted those files.
+
+Now pull checks every file recorded in the manifest's `pull_extra_hashes`
+(the same set diff/push merge back into the config):
+
+- Plain pull: the config is `skipped` with reason `locally modified`, the edit
+  stays, and it is still pending for `sync push`.
+- `pull --force`: remote changed too -> exit 1, `SYNC_CONFLICT`, nothing written;
+  remote unchanged -> preserved.
+- `pull --theirs`: remote wins, as before.
+
+Behavior change: deleting only a companion file now counts as a local edit, so
+plain pull no longer re-creates it when the remote changed (diff/push already
+treated it as a change). To drop local edits, use `sync pull --theirs` or delete
+the whole config directory and pull.
+
 ## `sync push` no longer leaves phantom `REMOTE MODIFIED` drift; `transform.sql` carries statement boundaries
 
 *(since 0.91.0, #686)*

plugins/kbagent/skills/kbagent/references/sync-workflow.md

@@ -414,6 +414,10 @@ Stored in `.keboola/branch-mapping.json`:
 
 - **Pull is idempotent**: re-running pull when nothing changed writes zero files
 - **Pull protects local edits**: locally-modified files are skipped by default
+  -- and "locally modified" covers the whole config, not only `_config.yml`:
+  an edit to a companion file (`transform.sql`, `code.py`, `_description.md`,
+  ...) protects the config the same way *(since vNEXT, #792)*. Before, such an
+  edit was silently overwritten whenever the remote changed, plain or `--force`
 - **`--force` is conflict-aware**: see below -- it no longer blindly overwrites
 - **Push only sends local changes**: remote_modified and conflict changes are skipped
 - **Push records the API's own view of what it wrote (since 0.91.0, #686)**: the
@@ -472,7 +476,10 @@ internal state:
 ## `sync pull --force` is conflict-aware (since 0.53.0)
 
 `--force` no longer blindly overwrites locally-modified configs. It branches on
-the 3-way diff state per config (and per row):
+the 3-way diff state per config (and per row). "Local edited" means any file
+of the config -- `_config.yml` or a companion file such as `transform.sql`
+*(since vNEXT, #792)*; before, a companion-only edit was never a conflict and
+was overwritten:
 
 - **Local edited, remote UNCHANGED** -> the file and its sync baseline are
   **preserved**. The pending delta stays visible to `sync diff` / `sync push`.
@@ -498,7 +505,10 @@ the 3-way diff state per config (and per row):
 > Safe to run `sync pull --force` to refresh an unrelated config even while you
 > have un-pushed edits elsewhere: non-conflicting edits survive; a real conflict
 > stops you loudly instead of losing work. To intentionally drop a local edit,
-> delete the file (or the config directory) and pull.
+> run `sync pull --theirs`, or delete the whole config directory and pull.
+> Deleting only a companion file (`transform.sql`, ...) counts as a local edit
+> *(since vNEXT, #792)* -- `sync diff` / `sync push` read it that way too -- so
+> plain pull keeps it deleted.
 
 ## Migrating a legacy sync tree (#686)
 

src/keboola_agent_cli/services/_sync_baseline.py

@@ -262,38 +262,13 @@ def effective_stored_hash(
     return stored
 
 
-def needs_shape_migration(
-    metadata: dict[str, Any],
-    *,
-    component_id: str,
-    config_id: str,
-    raw_remote: dict[str, Any],
-    api_cfg_hash: str,
-) -> bool:
-    """True iff this entry's baseline is a pre-#686 hash of the same remote.
-
-    The pull-side counterpart of :func:`effective_stored_hash`: the remote is
-    unchanged, only the recorded shape is old, so the pull must re-run
-    extraction (to write the boundary markers) and re-stamp -- unless the
-    local files were edited, in which case they are preserved untouched.
-    """
-    stored = str(metadata.get("pull_config_hash", "") or "")
-    if not stored or metadata.get(CONFIG_HASH_VERSION_KEY) or stored == api_cfg_hash:
-        return False
-    return is_legacy_hash(
-        stored, component_id=component_id, config_id=config_id, raw_remote=raw_remote
-    )
-
-
 def extras_modified(service: SyncService, config_dir: Path, extra_hashes: dict[str, str]) -> bool:
-    """True iff any companion file recorded at pull time changed on disk.
-
-    The pull overwrite-guard has only ever compared ``_config.yml``, so an
-    edited ``transform.sql`` beside an untouched ``_config.yml`` was
-    overwritten. That is pre-existing behaviour everywhere EXCEPT the shape
-    migration, which rewrites code files for a remote that did not change --
-    there, silently discarding a local edit would be new damage, so the
-    migration checks the companions too.
+    """True iff any companion file recorded at pull time changed or vanished.
+
+    Companion files are the ``pull_extra_hashes`` entries (``transform.sql``,
+    ``code.py``, ``_description.md``, ...): the same set ``sync diff`` /
+    ``sync push`` merge back into the config, so they are part of the
+    config's local representation exactly like ``_config.yml``.
     """
     for fname, stored_hash in (extra_hashes or {}).items():
         fpath = config_dir / fname
@@ -302,6 +277,33 @@ def extras_modified(service: SyncService, config_dir: Path, extra_hashes: dict[s
     return False
 
 
+def config_locally_modified(
+    service: SyncService,
+    config_dir: Path,
+    pull_hash: str,
+    extra_hashes: dict[str, str],
+) -> bool:
+    """True iff the config's local representation differs from the pull state.
+
+    Covers ``_config.yml`` AND every companion file recorded at pull time
+    (issue #792 finding B): pull used to hash ``_config.yml`` only, so an
+    edited ``transform.sql`` beside an untouched ``_config.yml`` was silently
+    overwritten by a remote change -- plain pull and ``--force`` alike.
+
+    Without a recorded ``pull_hash`` nothing can be proven (False, as
+    before). A missing ``_config.yml`` means the config dir is gone, which
+    pull re-materializes (#472) -- not a local edit to protect.
+    """
+    if not pull_hash:
+        return False
+    config_file = config_dir / CONFIG_FILENAME
+    if not config_file.exists():
+        return False
+    if service._file_hash(config_file) != pull_hash:
+        return True
+    return extras_modified(service, config_dir, extra_hashes)
+
+
 def _scripts_by_code(config_data: dict[str, Any]) -> list[list[Any]] | None:
     """Collect every ``blocks[].codes[].script`` array, in document order."""
     parameters = config_data.get("parameters")
@@ -426,22 +428,24 @@ def _is_conflict(
     old_pull_hash: str,
     old_cfg_hash: str,
     api_cfg_hash: str,
+    extra_hashes: dict[str, str] | None = None,
 ) -> bool:
-    """True iff the file is locally modified AND the remote also changed.
+    """True iff the local files are modified AND the remote also changed.
 
     A 3-way conflict needs both a stored ``pull_hash`` (the synced file
     state) and a stored ``pull_config_hash`` (the synced remote state);
     without either we cannot prove a conflict, so return False -- be
     conservative, ``--force`` must not abort on incomplete bookkeeping.
     A missing local file is not a content conflict (nothing to lose).
+    ``extra_hashes`` (configs only; rows have no companion files) extends
+    the local check to the companion files (issue #792 finding B).
     """
-    if not old_pull_hash or not old_cfg_hash:
+    if not old_cfg_hash:
         return False
-    if not config_file.exists():
-        return False
-    locally_modified = service._file_hash(config_file) != old_pull_hash
-    remote_changed = api_cfg_hash != old_cfg_hash
-    return locally_modified and remote_changed
+    locally_modified = config_locally_modified(
+        service, config_file.parent, old_pull_hash, extra_hashes or {}
+    )
+    return locally_modified and api_cfg_hash != old_cfg_hash
 
 
 def detect_force_pull_conflicts(
@@ -459,7 +463,8 @@ def detect_force_pull_conflicts(
     """Return configs/rows a ``--force`` pull would clobber as conflicts.
 
     A *conflict* is a config (or row) that is BOTH locally modified (its
-    on-disk ``_config.yml`` hash differs from the manifest ``pull_hash``)
+    on-disk ``_config.yml`` hash differs from the manifest ``pull_hash``,
+    or one of its companion files differs from ``pull_extra_hashes``)
     AND changed on the remote since the last pull (the freshly fetched
     config hash differs from ``pull_config_hash``).  That is the only case
     where ``--force`` must stop: local and remote have diverged, so neither
@@ -506,6 +511,7 @@ def detect_force_pull_conflicts(
                     remote_local=remote_local,
                 ),
                 config_hash(remote_local),
+                existing_metadata.get(lookup_key, {}).get("pull_extra_hashes") or {},
             ):
                 conflicts.append(
                     {

src/keboola_agent_cli/services/sync_service.py

@@ -73,10 +73,9 @@
     find_plaintext_secret_keys,
 )
 from ._sync_baseline import (
+    config_locally_modified,
     detect_force_pull_conflicts,
     effective_stored_hash,
-    extras_modified,
-    needs_shape_migration,
     raise_on_legacy_boundary,
 )
 from ._sync_bindings import resolve_flow_task_bindings, resolve_variable_bindings
@@ -781,30 +780,19 @@ def pull(
                 # would silently strand the un-pushed edits.  Preserving keeps
                 # the pending delta visible to ``sync push``.
                 # ``--theirs`` disables the preserve entirely: remote wins.
+                # The check covers every file the config's local
+                # representation consists of -- ``_config.yml`` plus the
+                # companion files recorded in ``pull_extra_hashes`` (the set
+                # diff/push merge back) -- so an edited ``transform.sql`` is
+                # protected like an edited ``_config.yml`` (issue #792 B).
                 locally_modified = False
                 if not is_new and not theirs:
-                    old_file_hash = existing_file_hashes.get(lookup_key, "")
-                    if old_file_hash:
-                        config_file = config_dir / CONFIG_FILENAME
-                        if config_file.exists():
-                            current_file_hash = self._file_hash(config_file)
-                            locally_modified = current_file_hash != old_file_hash
-                    # Shape migration (issue #686): the remote is unchanged, only
-                    # the recorded hash shape is old, so this pull re-extracts
-                    # (writing the boundary markers) and re-stamps. Because the
-                    # rewrite is not driven by a remote change, an edited
-                    # companion file must be preserved too -- the ordinary
-                    # overwrite-guard above only ever looks at ``_config.yml``.
-                    if not locally_modified and needs_shape_migration(
-                        existing_metadata.get(lookup_key, {}),
-                        component_id=component_id,
-                        config_id=config_id,
-                        raw_remote=cfg,
-                        api_cfg_hash=api_cfg_hash,
-                    ):
-                        locally_modified = extras_modified(
-                            self, config_dir, existing_extra_hashes.get(lookup_key, {})
-                        )
+                    locally_modified = config_locally_modified(
+                        self,
+                        config_dir,
+                        existing_file_hashes.get(lookup_key, ""),
+                        existing_extra_hashes.get(lookup_key, {}),
+                    )
 
                 remote_unchanged = False  # set in else branch; default for locally_modified path
                 if locally_modified and not dry_run:
@@ -1011,6 +999,10 @@ def pull(
                     cfg_metadata = {
                         "pull_hash": old_pull_hash,
                         "pull_config_hash": old_cfg_hash,
+                        # Carry the companion baseline over too: dropping it
+                        # would make diff/push read an edited transform.sql
+                        # as unchanged (issue #792 B).
+                        "pull_extra_hashes": existing_extra_hashes.get(lookup_key, {}),
                     }
                     # The preserved hash was NOT produced by the current
                     # producer, so its version marker is carried over verbatim

tests/test_sync_formal_counterexamples.py

@@ -355,24 +355,13 @@ def test_a_recreate_under_same_name_does_not_delete_new_config(tmp_path: Path) -
 
 
 # ===========================================================================
-# B -- pull compares only _config.yml; edits to companion files (SQL/code)
-#      are silently overwritten, plain or --force, with no conflict.
+# B -- pull compared only _config.yml; edits to companion files (SQL/code)
+#      were silently overwritten, plain or --force, with no conflict.
+#      FIXED: now an ordinary regression guard (more cases in
+#      tests/test_sync_pull_companion_files.py).
 # ===========================================================================
 
 
-@pytest.mark.xfail(
-    strict=True,
-    reason=(
-        "#792 B: pull's 'locally modified' guard (sync_service.py:766-790, "
-        "_sync_baseline.py:423-445) hashes only _config.yml. A companion "
-        "file (transform.sql / code.py / _description.md) that was edited "
-        "locally is not detected as modified, so a remote change to the same "
-        "transformation silently overwrites the local SQL edit -- plain pull "
-        "AND `pull --force` -- with no 'skipped' entry and no SYNC_CONFLICT. "
-        "Confirmed via Lean F6 and replayed live (scratchpad/repro/"
-        "test_lean_refutations.py::test_R1)."
-    ),
-)
 def test_b_pull_never_overwrites_local_sql_edit(tmp_path: Path) -> None:
     """Invariant: a locally-edited companion file (here: transform.sql on a
     tracked SQL transformation) must never be silently overwritten by pull