From 7939e5ce299d5cb953a06844e82075975a0a83f1 Mon Sep 17 00:00:00 2001 From: Petr Date: Sat, 26 Sep 2026 02:34:58 +0200 Subject: [PATCH 1/2] fix(sync): pull never deletes a dir it wrote or a locally edited dir (#792 A, C) Pull's stale-entry sweep could delete the directory of a config that was deleted and re-created remotely under the same name (the next push then deleted the live config), and silently deleted a locally edited directory whose remote config vanished. - a new config never lands on a stale entry's on-disk path (suffixed) - the sweep never deletes a path an entry of the same pull owns - a locally edited stale dir is kept + reported as skipped on plain pull, raises SYNC_CONFLICT under --force; --theirs still deletes it Removes the xfail markers for findings A and C. --- formal/sync/README.md | 15 +- .../skills/kbagent/references/gotchas.md | 21 +++ .../kbagent/references/sync-workflow.md | 11 ++ src/keboola_agent_cli/services/_sync_stale.py | 174 ++++++++++++++++++ .../services/sync_service.py | 67 +++---- tests/test_sync_formal_counterexamples.py | 94 +++++++--- 6 files changed, 307 insertions(+), 75 deletions(-) create mode 100644 src/keboola_agent_cli/services/_sync_stale.py diff --git a/formal/sync/README.md b/formal/sync/README.md index df9d8cd01..0c456f572 100644 --- a/formal/sync/README.md +++ b/formal/sync/README.md @@ -99,9 +99,9 @@ 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 | `test_a_recreate_under_same_name_does_not_delete_new_config` | +| 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` | -| C | Remote delete + local edit: pull (plain/`--force`) deletes the locally-edited directory with no conflict | Lean F7, TLA I5 | HIGH | `test_c_pull_never_deletes_locally_edited_dir_on_remote_delete` | +| 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` | | F | A push aborted by `ENCRYPTION_FAILED` leaves the manifest unsaved; the retry duplicates the change(s) the aborted push already applied | TLA I1b (model trace only; replayed live for this pilot) | MED | `test_f_aborted_push_does_not_duplicate_already_created_config` | @@ -111,7 +111,16 @@ the regression test for each in `tests/test_sync_formal_counterexamples.py`. | J | `sync pull --branch dev` reports untouched production configs as "removed" | Spec S4 | LOW | `test_j_branch_scoped_pull_does_not_report_other_branch_configs_removed` | | K | A cosmetic local edit (raw vs normalized hash) blocks pull from ever applying a real remote change; `--force` raises a conflict | Spec S3, TLA I12 | LOW (documented, conservative behavior) | `test_k_cosmetic_edit_is_conservative_not_unsafe` (unmarked regression guard, not xfail) | -A..F, H and I reproduce on current code and are `xfail(strict=True)` -- +**A and C are fixed**: a new config never lands on a stale entry's directory +(it gets a suffixed path), the sweep never deletes a path an entry of the same +pull owns, and a locally edited directory whose remote vanished is kept and +reported as `skipped` by plain pull, raises `SYNC_CONFLICT` under `--force`, +and is deleted only by `--theirs`. That is what the I2 (sweep half), I5 and I8 +(sweep half) counterexamples needed; the models themselves are unchanged, so a +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)` -- 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 diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index ed6e74fad..607cd8821 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -5323,3 +5323,24 @@ It carries the command name, the outcome, and the duration -- never argument val Key on the flag; recover with `kbagent branch reset` + `kbagent sync branch-unlink`. - **`branch merge` is deprecated** (still works, carries `deprecation` in `--json`): it only builds a UI URL and resets the active branch. + +## `sync pull` no longer deletes a re-created config or a locally edited directory (#792) + +*(since vNEXT)* Two data-loss paths in pull's stale-entry sweep (the step that +drops manifest entries whose config is gone from the remote) are closed: + +- **Remote delete + re-create under the same name.** Before, the new config was + written into the old config's directory and the sweep then deleted that same + directory; the next `sync push` DELETEd the live new config. Now the new + config lands in a suffixed directory (`-`), the old + one is removed, and the manifest matches disk. The next pull renames the + directory back to the plain name. +- **Remote delete + local edit.** Before, plain pull and `pull --force` deleted + the edited directory silently. Now plain pull **keeps** it (manifest entry + kept, pull action `skipped`, reason `locally modified, deleted on remote`); + `pull --force` aborts with `SYNC_CONFLICT` and the conflict carries + `reason: "deleted on remote"`. Only `pull --theirs` still deletes it (remote + wins). "Edited" covers `_config.yml`, companion files (`transform.sql`, + `code.py`, `_description.md`, ...) and row files. A kept directory is still + tracked, so the next `sync push` re-creates the config remotely -- delete the + directory if the remote delete was intended. diff --git a/plugins/kbagent/skills/kbagent/references/sync-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-workflow.md index ecbe6aaee..32a27d3fc 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-workflow.md @@ -483,6 +483,17 @@ the 3-way diff state per config (and per row): error code `SYNC_CONFLICT`, listing every conflicting config/row. Resolve with `sync diff`, then `sync push` your edits (or discard them), then pull again. - **Local untouched, remote changed** -> `--force` takes remote as before. +- **Local edited, remote DELETED** *(since vNEXT, #792)* -> `--force` aborts + with `SYNC_CONFLICT` (conflict `reason: "deleted on remote"`). Plain pull + keeps the edited directory and its manifest entry and reports it as + `skipped` (`locally modified, deleted on remote`). Only `--theirs` deletes it. + A kept directory stays tracked, so the next `sync push` re-creates the config; + delete the directory if the remote delete was intended. + +> A config deleted and re-created remotely under the same name *(since vNEXT, +> #792)* is written to a suffixed directory while the old one is removed; the +> next pull renames it back. Before, the sweep deleted the new config's files +> and the next push deleted the new config remotely. > 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 diff --git a/src/keboola_agent_cli/services/_sync_stale.py b/src/keboola_agent_cli/services/_sync_stale.py new file mode 100644 index 000000000..1ec7d2560 --- /dev/null +++ b/src/keboola_agent_cli/services/_sync_stale.py @@ -0,0 +1,174 @@ +"""Stale-entry sweep for ``sync pull`` (issue #792 findings A and C). + +A *stale* manifest entry is one the fresh remote listing no longer produces: +the config was deleted on the remote (``removed``) or its component is now +ignored (``ignored``, issue #689). Pull drops such entries and deletes their +directories. Two data-loss paths used to hide in that sweep: + +- **A** -- a config deleted and re-created under the same name got the NEW + config written into the OLD directory (paths are chosen per pull), and the + sweep then deleted that same directory; the next ``sync push`` deleted the + live new config remotely. Fixed twice over: :func:`reserved_paths` keeps a + new config from landing on a stale entry's directory, and the sweep never + deletes a path this pull wrote. +- **C** -- a directory carrying un-pushed local edits was deleted because + its remote vanished. Now plain pull preserves it (entry kept, reported as + ``skipped``), ``--force`` aborts with SYNC_CONFLICT, and only ``--theirs`` + (remote wins) still deletes it. +""" + +from __future__ import annotations + +import logging +import shutil +from dataclasses import dataclass +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from ..constants import CONFIG_FILENAME +from ..sync.manifest import ManifestConfiguration +from ._sync_baseline import extras_modified + +if TYPE_CHECKING: + from .sync_service import SyncService + +logger = logging.getLogger(__name__) + +REMOTE_DELETED_REASON = "locally modified, deleted on remote" + + +@dataclass +class StaleEntry: + """A manifest entry the current pull no longer produces.""" + + entry: ManifestConfiguration + action: str # "removed" (gone from the remote) or "ignored" (component ignored) + locally_modified: bool + + +def _entry_locally_modified( + service: SyncService, config_dir: Path, entry: ManifestConfiguration +) -> bool: + """True iff ``_config.yml``, a companion file or a row file changed since pull. + + Without a recorded ``pull_hash`` there is no base to compare against, so + the entry is not treated as modified (same conservatism as the force-pull + conflict guard). + """ + pull_hash = entry.metadata.get("pull_hash", "") + config_file = config_dir / CONFIG_FILENAME + if not pull_hash or not config_file.exists(): + return False + if service._file_hash(config_file) != pull_hash: + return True + if extras_modified(service, config_dir, entry.metadata.get("pull_extra_hashes") or {}): + return True + for row in entry.rows: + row_hash = row.metadata.get("pull_hash", "") + row_file = config_dir / row.path / CONFIG_FILENAME + if row_hash and row_file.exists() and service._file_hash(row_file) != row_hash: + return True + return False + + +def find_stale_entries( + service: SyncService, + entries: list[ManifestConfiguration], + components: list[dict[str, Any]], + branch_dir: Path, + ignored_components: frozenset[str], +) -> list[StaleEntry]: + """Manifest entries absent from the fresh (non-ignored) remote listing.""" + remote_keys = { + f"{component.get('id', '')}/{cfg.get('id', '')}" + for component in components + if component.get("id", "") not in ignored_components + for cfg in component.get("configurations", []) + } + stale: list[StaleEntry] = [] + for entry in entries: + if f"{entry.component_id}/{entry.id}" in remote_keys: + continue + action = "ignored" if entry.component_id in ignored_components else "removed" + modified = action == "removed" and _entry_locally_modified( + service, branch_dir / entry.path, entry + ) + stale.append(StaleEntry(entry=entry, action=action, locally_modified=modified)) + return stale + + +def reserved_paths(stale: list[StaleEntry], branch_dir: Path) -> set[str]: + """Stale paths still on disk -- a new config must not be written there (A).""" + return {s.entry.path for s in stale if s.entry.path and (branch_dir / s.entry.path).exists()} + + +def remote_deleted_conflicts(stale: list[StaleEntry]) -> list[dict[str, str]]: + """``--force`` conflicts: locally edited configs whose remote was deleted (C).""" + return [ + { + "scope": "config", + "component_id": s.entry.component_id, + "config_id": s.entry.id, + "config_name": "", + "path": s.entry.path, + "reason": "deleted on remote", + } + for s in stale + if s.locally_modified + ] + + +def _remove_dir(orphan_dir: Path, branch_dir: Path) -> None: + """rmtree ``orphan_dir`` and prune now-empty parents up to ``branch_dir``.""" + if not (orphan_dir.exists() and orphan_dir.is_dir()): + return + shutil.rmtree(orphan_dir) + logger.info("Removed orphaned directory: %s", orphan_dir) + parent = orphan_dir.parent + while parent != branch_dir and parent.exists() and not any(parent.iterdir()): + parent.rmdir() + logger.info("Removed empty parent directory: %s", parent) + parent = parent.parent + + +def apply_stale_sweep( + stale: list[StaleEntry], + branch_dir: Path, + *, + theirs: bool, + dry_run: bool, + new_configurations: list[ManifestConfiguration], + pull_details: list[dict[str, str]], +) -> None: + """Report stale entries and delete their directories, safely. + + A locally edited ``removed`` entry is preserved unless ``--theirs``: its + manifest entry is carried over unchanged (so the manifest still matches + disk) and it is reported as ``skipped``. ``--force`` never reaches that + branch -- the conflict guard has already aborted. A path an entry of this + pull owns (written or kept by the fetch loop) is never deleted. + """ + live_paths = {c.path for c in new_configurations} + for s in stale: + if s.locally_modified and not theirs: + new_configurations.append(s.entry) + pull_details.append( + { + "action": "skipped", + "component_id": s.entry.component_id, + "config_name": s.entry.path, + "path": s.entry.path, + "reason": REMOTE_DELETED_REASON, + } + ) + continue + pull_details.append( + { + "action": s.action, + "component_id": s.entry.component_id, + "config_name": "", + "path": s.entry.path, + } + ) + if not dry_run and s.entry.path and s.entry.path not in live_paths: + _remove_dir(branch_dir / s.entry.path, branch_dir) diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index 710de01b5..68fee7a8a 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -102,6 +102,12 @@ from ._sync_data_app import load_data_app_types, resolve_pull_type, type_needs_rewrite from ._sync_models import CreatedConfig, LocalConfigHashes from ._sync_push_ops import push_create, push_row_change, push_update +from ._sync_stale import ( + apply_stale_sweep, + find_stale_entries, + remote_deleted_conflicts, + reserved_paths, +) from ._sync_storage import ( fetch_jobs_per_config, fetch_samples, @@ -638,6 +644,12 @@ def pull( # Resolved once and shared by the conflict guard, the fetch loop and # the stale-entry sweep below -- see ``_effective_ignored_components``. ignored_components = self._effective_ignored_components(manifest) + # Entries the remote no longer lists (#792 A/C); their on-disk dirs are + # reserved so a same-named new config cannot land in one. + stale = find_stale_entries( + self, manifest.configurations, components, branch_dir, ignored_components + ) + used_paths |= reserved_paths(stale, branch_dir) # Force-pull conflict guard (force-pull baseline corruption fix). # ``--force`` bypasses the "preserve locally-modified files" guard @@ -662,7 +674,7 @@ def pull( existing_file_hashes=existing_file_hashes, existing_metadata=existing_metadata, existing_rows=existing_rows, - ) + ) + remote_deleted_conflicts(stale) if conflicts: raise SyncConflictError(conflicts) @@ -1050,48 +1062,17 @@ def pull( ) ) - # Detect configs dropped from the manifest (in old manifest but not in - # new). Two distinct causes, reported apart (issue #689): the config was - # deleted on the remote ("removed"), or its component is now ignored - # ("ignored" -- the fetch loop above never produced an entry for it). - # Conflating them would report a live production config as gone from - # the remote, which is exactly the wrong thing to tell a user deciding - # whether to restore it. The on-disk cleanup is identical either way: - # an ignored config has no business sitting in the tree, and git keeps - # the removal reviewable. - new_keys = {f"{c.component_id}/{c.id}" for c in new_configurations} - for old_cfg in manifest.configurations: - old_key = f"{old_cfg.component_id}/{old_cfg.id}" - if old_key not in new_keys: - stale_action = ( - "ignored" if old_cfg.component_id in ignored_components else "removed" - ) - pull_details.append( - { - "action": stale_action, - "component_id": old_cfg.component_id, - "config_name": "", - "path": old_cfg.path, - } - ) - - # Delete orphaned directories for removed / newly-ignored configurations - if not dry_run: - for detail in pull_details: - if detail["action"] in ("removed", "ignored") and detail.get("path"): - orphan_dir = branch_dir / detail["path"] - if orphan_dir.exists() and orphan_dir.is_dir(): - shutil.rmtree(orphan_dir) - logger.info("Removed orphaned directory: %s", orphan_dir) - # Clean up empty parent dirs up to (but not including) branch_dir - parent = orphan_dir.parent - while parent != branch_dir and parent.exists(): - if not any(parent.iterdir()): - parent.rmdir() - logger.info("Removed empty parent directory: %s", parent) - parent = parent.parent - else: - break + # Report configs dropped from the manifest ("removed" from the remote vs + # "ignored" component, issue #689) and delete their directories -- + # except a locally edited one (#792 C) or one this pull owns (#792 A). + apply_stale_sweep( + stale, + branch_dir, + theirs=theirs, + dry_run=dry_run, + new_configurations=new_configurations, + pull_details=pull_details, + ) # -- Storage metadata (read-only, not tracked in manifest) -- storage_stats: dict[str, int] = {"buckets": 0, "tables": 0, "samples": 0} diff --git a/tests/test_sync_formal_counterexamples.py b/tests/test_sync_formal_counterexamples.py index a4e30adc8..e89551d89 100644 --- a/tests/test_sync_formal_counterexamples.py +++ b/tests/test_sync_formal_counterexamples.py @@ -324,21 +324,6 @@ def changes(d: dict) -> list[tuple[str, str]]: # =========================================================================== -@pytest.mark.xfail( - strict=True, - reason=( - "#792 A: pull's stale-entry sweep (sync_service.py ~1062-1094) runs " - "AFTER the fetch loop has written the new config to the same path " - "(paths are per-pull, not globally unique). When a remote actor " - "deletes a config and creates a new one of the same name, the new " - "config lands on the old path, then the sweep rmtree's that same " - "path for the vanished old id -- deleting the new files. The next " - "push then classifies the (still manifest-tracked) new config as " - "DELETED and destroys it on the remote. Confirmed live via " - "Lean F8 + TLA I2 (independently found) and replayed against the " - "real SyncService (scratchpad/replay/r_stale_sweep.py)." - ), -) def test_a_recreate_under_same_name_does_not_delete_new_config(tmp_path: Path) -> None: """Invariant: a config that a pull just fetched and wrote to disk must never be deleted -- locally or remotely -- by that same pull's stale-entry @@ -445,20 +430,6 @@ def sql_client_pull(sql_stmt: str) -> None: # =========================================================================== -@pytest.mark.xfail( - strict=True, - reason=( - "#792 C: pull's stale-entry sweep (sync_service.py:1062-1094) runs " - "an unconditional rmtree for every manifest entry whose remote key " - "vanished -- it never checks pull_hash against the current file " - "content. A config edited locally and then deleted remotely is " - "silently rmtree'd by the next pull (plain or --force), with no " - "conflict raised even under --force (detect_force_pull_conflicts " - "only iterates remote configs, _sync_baseline.py:485). Confirmed " - "via Lean F7 + TLA I5 (independently found) and replayed " - "(scratchpad/replay/r_misc.py)." - ), -) def test_c_pull_never_deletes_locally_edited_dir_on_remote_delete(tmp_path: Path) -> None: """Invariant: pull must never destroy a directory carrying an unpushed local edit just because the remote config was deleted in the meantime -- @@ -483,6 +454,71 @@ def test_c_pull_never_deletes_locally_edited_dir_on_remote_delete(tmp_path: Path assert config_dir.exists(), "pull deleted a directory carrying an unpushed local edit" assert "MY-UNPUSHED-EDIT" in edited_file.read_text() + # Plain pull reports the preserved dir and keeps it tracked (manifest == disk). + assert "cfg-1" in [c for _, c, _ in w.manifest()] + + +def _edit_then_delete_remote(w: World) -> Path: + w.api.put(PROD, "cfg-1", "Orders", "a") + w.init() + w.pull() + edited_file = w.config_dir("orders") / CONFIG_FILENAME + data = yaml.safe_load(edited_file.read_text()) + data["parameters"]["value"] = "MY-UNPUSHED-EDIT" + edited_file.write_text(yaml.dump(data, default_flow_style=False)) + w.api.remote[PROD].pop("cfg-1") + return edited_file + + +def test_c_plain_pull_reports_preserved_dir_as_skipped(tmp_path: Path) -> None: + """#792 C: the preserved dir surfaces as ``skipped`` (not ``removed``).""" + w = World(tmp_path) + _edit_then_delete_remote(w) + result = w.pull() + actions = [(d["action"], d.get("reason", "")) for d in result["details"]] + assert ("skipped", "locally modified, deleted on remote") in actions + assert not any(a == "removed" for a, _ in actions) + + +def test_c_force_pull_raises_conflict_for_edited_dir_deleted_remotely(tmp_path: Path) -> None: + """#792 C: ``--force`` aborts with SYNC_CONFLICT instead of deleting the edit.""" + from keboola_agent_cli.errors import SyncConflictError + + w = World(tmp_path) + edited_file = _edit_then_delete_remote(w) + with pytest.raises(SyncConflictError) as exc: + w.pull(force=True) + assert [(c["config_id"], c.get("reason")) for c in exc.value.conflicts] == [ + ("cfg-1", "deleted on remote") + ] + assert "MY-UNPUSHED-EDIT" in edited_file.read_text() + + +def test_c_theirs_pull_still_deletes_edited_dir(tmp_path: Path) -> None: + """#792 C: ``--theirs`` keeps remote-wins -- the deleted config's dir goes.""" + w = World(tmp_path) + edited_file = _edit_then_delete_remote(w) + w.pull(theirs=True) + assert not edited_file.parent.exists() + assert w.manifest() == [] + + +def test_a_unedited_stale_dir_is_still_swept(tmp_path: Path) -> None: + """#792 A: the old dir of a re-created config is removed, the new one kept + and tracked -- the manifest matches disk after the pull.""" + w = World(tmp_path) + w.api.put(PROD, "cfg-1", "Orders", "a") + w.init() + w.pull() + w.api.remote[PROD].pop("cfg-1") + w.api.put(PROD, "cfg-2", "Orders", "b") + w.pull() + tracked = w.manifest() + assert [c for _, c, _ in tracked] == ["cfg-2"] + assert sorted(p for _, _, p in tracked) == [f.split("/", 1)[1] for f in w.files()], ( + "manifest does not match disk" + ) + # =========================================================================== # D -- `sync push --branch dev` (promote) re-creates the same config on From 3dcc2bf3c33ae499f4cdd90eaec4f0cad5cdc41b Mon Sep 17 00:00:00 2001 From: soustruh Date: Tue, 29 Sep 2026 14:39:04 +0200 Subject: [PATCH 2/2] test(sync): compare POSIX paths in World.files() so the #792 tests pass on Windows --- tests/test_sync_formal_counterexamples.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_sync_formal_counterexamples.py b/tests/test_sync_formal_counterexamples.py index e89551d89..d70f17345 100644 --- a/tests/test_sync_formal_counterexamples.py +++ b/tests/test_sync_formal_counterexamples.py @@ -300,7 +300,7 @@ def push(self, branch: int | None = None, **kw: Any) -> dict: def files(self) -> list[str]: return sorted( - str(p.parent.relative_to(self.root)) + p.parent.relative_to(self.root).as_posix() for p in self.root.rglob("_config.yml") if "rows" not in p.parts )