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

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

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 (`<name>-<config id prefix>`), 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.

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

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)

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}