diff --git a/formal/sync/README.md b/formal/sync/README.md index 0c456f57..e431b4a4 100644 --- a/formal/sync/README.md +++ 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. diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 9b90779a..0acafdff 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/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)* diff --git a/plugins/kbagent/skills/kbagent/references/sync-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-workflow.md index 32a27d3f..cbc6b52d 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-workflow.md +++ b/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) diff --git a/src/keboola_agent_cli/services/_sync_baseline.py b/src/keboola_agent_cli/services/_sync_baseline.py index 616460e7..520101d4 100644 --- a/src/keboola_agent_cli/services/_sync_baseline.py +++ b/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( { diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index 0c02439b..1d5098e5 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/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 diff --git a/tests/test_sync_formal_counterexamples.py b/tests/test_sync_formal_counterexamples.py index d70f1734..11cf7786 100644 --- a/tests/test_sync_formal_counterexamples.py +++ b/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 diff --git a/tests/test_sync_pull_companion_files.py b/tests/test_sync_pull_companion_files.py new file mode 100644 index 00000000..a5f07ba3 --- /dev/null +++ b/tests/test_sync_pull_companion_files.py @@ -0,0 +1,208 @@ +"""`sync pull` protects local edits in companion files, not only _config.yml. + +Issue #792 finding B: pull decided "locally modified" from ``_config.yml`` +alone, so an edited ``transform.sql`` / ``_description.md`` was silently +overwritten when the remote changed -- plain pull AND ``--force`` alike. +The check now covers every file recorded in ``pull_extra_hashes`` (the set +diff/push merge back into the config). +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import pytest + +from helpers import setup_single_project +from keboola_agent_cli.config_store import ConfigStore +from keboola_agent_cli.constants import CONFIG_FILENAME +from keboola_agent_cli.errors import SyncConflictError +from keboola_agent_cli.models import TokenVerifyResponse +from keboola_agent_cli.services.sync_service import SyncService +from keboola_agent_cli.sync.manifest import load_manifest + +SQL_COMP = "keboola.snowflake-transformation" + + +def _sql_components(sql: str, description: str = "", row_value: str | None = None) -> list: + rows: list[dict[str, Any]] = [] + if row_value is not None: + rows = [ + { + "id": "r1", + "name": "Row One", + "description": "", + "configuration": {"parameters": {"value": row_value}}, + "isDisabled": False, + } + ] + return [ + { + "id": SQL_COMP, + "type": "transformation", + "configurations": [ + { + "id": "t1", + "name": "My SQL", + "description": description, + "rows": rows, + "configuration": { + "parameters": { + "blocks": [{"name": "B", "codes": [{"name": "C", "script": [sql]}]}] + } + }, + } + ], + } + ] + + +def _client(components: list | None = None) -> MagicMock: + c = MagicMock() + c.__enter__ = MagicMock(return_value=c) + c.__exit__ = MagicMock(return_value=False) + c.verify_token.return_value = TokenVerifyResponse( + token_id="t", token_description="d", project_id=258, project_name="P", owner_name="O" + ) + c.list_dev_branches.return_value = [{"id": 12345, "name": "Main", "isDefault": True}] + if components is not None: + c.list_components_with_configs.return_value = components + return c + + +def _svc(store: ConfigStore, components: list | None = None) -> SyncService: + client = _client(components) + return SyncService(config_store=store, client_factory=lambda url, token: client) + + +@pytest.fixture +def tree(tmp_path: Path) -> tuple[ConfigStore, Path]: + cfg_dir = tmp_path / "cfg" + cfg_dir.mkdir() + root = tmp_path / "project" + root.mkdir() + store = setup_single_project(cfg_dir) + _svc(store).init_sync(alias="prod", project_root=root) + return store, root + + +def _pull(store: ConfigStore, root: Path, components: list, **kw: Any) -> dict: + return _svc(store, components).pull(alias="prod", project_root=root, **kw) + + +def _detail(result: dict) -> dict: + return next(d for d in result["details"] if d["component_id"] == SQL_COMP) + + +def test_plain_pull_preserves_edited_sql_and_keeps_it_pushable( + tree: tuple[ConfigStore, Path], +) -> None: + store, root = tree + _pull(store, root, _sql_components("SELECT 1;")) + sql_file = next(root.rglob("transform.sql")) + sql_file.write_text(sql_file.read_text().replace("SELECT 1", "SELECT 42")) + + result = _pull(store, root, _sql_components("SELECT 100;")) + + assert "SELECT 42" in sql_file.read_text() + assert _detail(result)["action"] == "skipped" + assert _detail(result)["reason"] == "locally modified" + # The companion baseline survives the preserving pull, so diff/push still + # see the edit as a pending local change instead of "unchanged". + entry = next(c for c in load_manifest(root).configurations if c.id == "t1") + assert "transform.sql" in entry.metadata["pull_extra_hashes"] + status = _svc(store).status(project_root=root) + assert any(m["config_id"] == "t1" for m in status["modified"]) + + +def test_force_pull_reports_conflict_for_edited_sql(tree: tuple[ConfigStore, Path]) -> None: + store, root = tree + _pull(store, root, _sql_components("SELECT 1;")) + sql_file = next(root.rglob("transform.sql")) + sql_file.write_text(sql_file.read_text().replace("SELECT 1", "SELECT 42")) + + with pytest.raises(SyncConflictError) as exc: + _pull(store, root, _sql_components("SELECT 100;"), force=True) + + assert [c["config_id"] for c in exc.value.conflicts] == ["t1"] + assert "SELECT 42" in sql_file.read_text() + + +def test_force_pull_preserves_edited_sql_when_remote_unchanged( + tree: tuple[ConfigStore, Path], +) -> None: + store, root = tree + _pull(store, root, _sql_components("SELECT 1;")) + sql_file = next(root.rglob("transform.sql")) + sql_file.write_text(sql_file.read_text().replace("SELECT 1", "SELECT 42")) + + result = _pull(store, root, _sql_components("SELECT 1;"), force=True) + + assert "SELECT 42" in sql_file.read_text() + assert _detail(result)["action"] == "skipped" + + +def test_theirs_still_overwrites_edited_sql(tree: tuple[ConfigStore, Path]) -> None: + store, root = tree + _pull(store, root, _sql_components("SELECT 1;")) + sql_file = next(root.rglob("transform.sql")) + sql_file.write_text(sql_file.read_text().replace("SELECT 1", "SELECT 42")) + + _pull(store, root, _sql_components("SELECT 100;"), theirs=True) + + assert "SELECT 100" in sql_file.read_text() + + +def test_plain_pull_preserves_edited_description(tree: tuple[ConfigStore, Path]) -> None: + store, root = tree + _pull(store, root, _sql_components("SELECT 1;", description="old")) + desc_file = next(root.rglob("_description.md")) + desc_file.write_text("my local description") + + result = _pull(store, root, _sql_components("SELECT 1;", description="remote new")) + + assert desc_file.read_text() == "my local description" + assert _detail(result)["action"] == "skipped" + + +def test_unedited_companion_files_take_remote_change(tree: tuple[ConfigStore, Path]) -> None: + store, root = tree + _pull(store, root, _sql_components("SELECT 1;")) + + result = _pull(store, root, _sql_components("SELECT 100;")) + + assert "SELECT 100" in next(root.rglob("transform.sql")).read_text() + assert _detail(result)["action"] == "updated" + + +def test_deleted_config_dir_is_rematerialized(tree: tuple[ConfigStore, Path]) -> None: + """A missing _config.yml is not a local edit to protect (#472).""" + store, root = tree + _pull(store, root, _sql_components("SELECT 1;")) + config_dir = next(root.rglob("transform.sql")).parent + for f in config_dir.iterdir(): + if f.is_file(): + f.unlink() + + _pull(store, root, _sql_components("SELECT 100;")) + + assert (config_dir / CONFIG_FILENAME).exists() + assert "SELECT 100" in (config_dir / "transform.sql").read_text() + + +def test_edited_row_file_is_preserved_and_conflicts_under_force( + tree: tuple[ConfigStore, Path], +) -> None: + store, root = tree + _pull(store, root, _sql_components("SELECT 1;", row_value="a")) + row_file = next(p for p in root.rglob(CONFIG_FILENAME) if "rows" in p.parts) + row_file.write_text(row_file.read_text().replace("value: a", "value: local")) + + with pytest.raises(SyncConflictError) as exc: + _pull(store, root, _sql_components("SELECT 1;", row_value="remote"), force=True) + assert [c.get("row_id") for c in exc.value.conflicts] == ["r1"] + + _pull(store, root, _sql_components("SELECT 1;", row_value="remote")) + assert "value: local" in row_file.read_text()