fix(kanban): malformed recorded landed receipt HOLDs on reclamation (t_ad32cdab)

hermes_cli/kanban_survivor.py

@@ -1061,6 +1061,50 @@ def _verify_landed(entries, workspace):
     return verified
 
 
+def _recorded_landed_claims(previous):
+    """Rebuild the `landed` claim a RECORDED receipt made, or refuse legibly.
+
+    Reclamation re-verifies the live commits a `kind: "landed"` receipt names
+    before it may delete the workspace. The row is persisted JSON -- a
+    hand-edited, partially written or legacy row can carry `landed: null`, a
+    non-object entry, or an entry without `repository`/`sha`. Indexing such a
+    row raised `TypeError`/`KeyError`, which no `except` tuple on the reaper
+    path names: the workspace survived, but with no `held_reason` and no
+    `workspace_held` event, so the card's recovery state was silent.
+    Every malformed shape is a refusal, raised as `SurvivorUnavailable` so it
+    persists through `_hold()` like any other doubt.
+    """
+    entries = previous.get("landed")
+    if not isinstance(entries, list) or not entries:
+        raise SurvivorUnavailable(
+            "survivor_unavailable: recorded landed receipt is malformed (landed is not a non-empty list)"
+        )
+    claims = []
+    for entry in entries:
+        if not isinstance(entry, dict):
+            raise SurvivorUnavailable(
+                "survivor_unavailable: recorded landed receipt is malformed (entry is not an object)"
+            )
+        repository, sha = entry.get("repository"), entry.get("sha")
+        if not isinstance(repository, str) or not repository or not isinstance(sha, str) or not sha:
+            raise SurvivorUnavailable(
+                "survivor_unavailable: recorded landed receipt is malformed (entry needs repository and sha)"
+            )
+        claims.append({"repo_path": repository, "sha": sha})
+    return claims
+
+
+def _recorded_refs(previous):
+    """The recorded `refs` list, or empty when the row's `refs` is not a list.
+
+    A malformed `refs` (e.g. `null`) vouches for nothing; treating it as empty
+    turns it into the ordinary "recorded repository missing" refusal instead of
+    a `TypeError` that escapes `_hold()`.
+    """
+    refs = (previous or {}).get("refs")
+    return refs if isinstance(refs, list) else ()
+
+
 def _base(repo, published):
     # The nearest published ancestor of HEAD is a BOUNDARY commit of
     # `rev-list HEAD ^<every ref>`: git stops there precisely because the
@@ -1947,8 +1991,7 @@ def preserve(conn, task_id, metadata=None, *, cleanup=False, workspace=None,
         landed = (metadata or {}).get("landed")
         if cleanup and not landed and previous and previous.get("kind") == "landed":
             # Revalidate the recorded live commits before removing the workspace.
-            landed = [{"repo_path": entry["repository"], "sha": entry["sha"]}
-                      for entry in previous["landed"]]
+            landed = _recorded_landed_claims(previous)
         if landed:
             if not isinstance(landed, list):
                 raise SurvivorUnavailable("survivor_unavailable: landed must be a list")
@@ -1962,7 +2005,7 @@ def preserve(conn, task_id, metadata=None, *, cleanup=False, workspace=None,
                 # repository and appear clean while its work is still here.
                 refs = {}
                 if cleanup and _reusable(previous):
-                    refs = {ref["repository"]: ref for ref in (previous or {}).get("refs", ())
+                    refs = {ref["repository"]: ref for ref in _recorded_refs(previous)
                             if isinstance(ref, dict) and ref.get("repository") in missing}
                 if explicit:
                     if None in explicit:
@@ -2217,6 +2260,20 @@ def preserve(conn, task_id, metadata=None, *, cleanup=False, workspace=None,
         # rides the re-raised exception instead, for the CLI to render.
         _hold(conn, task_id, reason)
         raise _refusal(reason, hint=bool(getattr(exc, "override_hint", ""))) from exc
+    except (TypeError, KeyError, AttributeError, IndexError) as exc:
+        # Backstop for the malformed-record class. `previous` and `bases` are
+        # persisted JSON that this function indexes in many places; a shape
+        # nobody validated (hand-edited, partially written, legacy) raises one
+        # of these, which the tuple above does not name. Escaping here left the
+        # workspace retained but the card's recovery state SILENT -- no
+        # `held_reason`, no `workspace_held` event (Argus r4 P10/P12, #924).
+        # Doubt fails closed: HOLD with a persisted reason, keep the traceback
+        # in the log for whoever repairs the row.
+        _log.exception("kanban survivor: unreadable recovery state for %s", task_id)
+        reason = ("survivor_unavailable: recorded survivor state is malformed "
+                  f"({type(exc).__name__}); repair the row or recover the workspace by hand")
+        _hold(conn, task_id, reason)
+        raise _refusal(reason) from exc
 
 
 def remove_workspace_dir(conn, task_id, path, *, worktree_root=None, board=False):

tests/hermes_cli/test_kanban_survivor_malformed_receipt.py

@@ -0,0 +1,165 @@
+"""A malformed RECORDED landed receipt must HOLD on reclamation, never go silent.
+
+Reclamation re-verifies a `kind: "landed"` receipt's live commits before it may
+delete the workspace. The receipt is persisted JSON; a hand-edited, partially
+written or legacy row (`landed: null`, a non-object entry, an entry without
+`sha`, `refs: null` while a recorded repository is missing) used to raise
+`TypeError`/`KeyError` out of `preserve()` and `remove_workspace_dir()`. The
+workspace survived, but with no `held_reason` and no `workspace_held` event --
+the card's recovery state was silent (Argus r4 P10/P12 on #924, t_ad32cdab).
+
+The contract pinned here: every malformed shape HOLDs through `_hold()` with a
+meaningful persisted reason and a `workspace_held` event, on both deletion
+entry points, and a well-formed receipt still reclaims.
+"""
+import json
+import subprocess
+from pathlib import Path
+
+import pytest
+
+from hermes_cli import kanban_db as kb
+from hermes_cli import kanban_survivor as ks
+
+
+def git(repo, *args):
+    return subprocess.run(
+        ["git", "-C", str(repo), *map(str, args)], stdin=subprocess.DEVNULL,
+        capture_output=True, check=True,
+    ).stdout.decode().strip()
+
+
+@pytest.fixture
+def board(tmp_path, monkeypatch):
+    monkeypatch.setattr(ks, "_temporary_roots", lambda: [tmp_path / "temporary"])
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home"))
+    monkeypatch.setattr(Path, "home", lambda: tmp_path)
+    with kb.connect_closing() as conn:
+        yield conn
+
+
+def landed_task(conn, tmp_path):
+    """A card whose completion recorded a VALID landed receipt; workspace kept."""
+    tid = kb.create_task(conn, title="landed receipt")
+    ws = kb.resolve_workspace(kb.get_task(conn, tid))
+    repo = ws / "w"
+    repo.mkdir(parents=True)
+    git(repo, "init", "-b", "main")
+    git(repo, "config", "user.name", "Worker")
+    git(repo, "config", "user.email", "worker@example.invalid")
+    (repo / "w.py").write_text("implementation = True\n")
+    git(repo, "add", "-A")
+    git(repo, "commit", "-m", "impl")
+    sha = git(repo, "rev-parse", "HEAD")
+    live = tmp_path / "live-w"
+    git(tmp_path, "clone", "--no-local", repo, live)
+    kb.set_workspace_path(conn, tid, ws)
+    ks.record_baseline(conn, tid, ws)
+    receipt = ks.preserve(conn, tid, {
+        "changed_files": ["w/w.py"], "landed": [{"repo_path": str(live), "sha": sha}],
+    })
+    assert receipt["kind"] == "landed", receipt
+    return tid, ws
+
+
+def corrupt(conn, tid, shape):
+    row = conn.execute(
+        "SELECT bases, survivor FROM task_workspace_survivors WHERE task_id = ?", (tid,),
+    ).fetchone()
+    bases, survivor = json.loads(row[0]), json.loads(row[1])
+    if shape == "landed_null":
+        survivor["landed"] = None
+    elif shape == "landed_not_list":
+        survivor["landed"] = {"repository": survivor["landed"][0]["repository"]}
+    elif shape == "landed_empty":
+        survivor["landed"] = []
+    elif shape == "entry_junk":
+        survivor["landed"] = ["junk"]
+    elif shape == "entry_no_sha":
+        survivor["landed"][0].pop("sha")
+    elif shape == "entry_no_repository":
+        survivor["landed"][0].pop("repository")
+    elif shape == "refs_null_with_missing_repo":
+        # A recorded repository has vanished, so reclamation must consult the
+        # recorded refs -- which are `null`. Before the fix: TypeError.
+        survivor["refs"] = None
+        bases["gone"] = "0" * 40
+    else:  # pragma: no cover - test typo guard
+        raise AssertionError(shape)
+    with kb.write_txn(conn):
+        conn.execute(
+            "UPDATE task_workspace_survivors SET bases = ?, survivor = ? WHERE task_id = ?",
+            (json.dumps(bases), json.dumps(survivor), tid),
+        )
+
+
+def held_events(conn, tid):
+    return [json.loads(r[0]).get("reason") for r in conn.execute(
+        "SELECT payload FROM task_events WHERE task_id = ? AND kind = 'workspace_held' ORDER BY id",
+        (tid,),
+    )]
+
+
+def reclaim(conn, tid, ws, entry):
+    if entry == "remove_workspace_dir":
+        return ks.remove_workspace_dir(conn, tid, ws)
+    return kb.safe_remove_workspace_dir(ws, task_id=tid, reason="test-reclaim", conn=conn)
+
+
+ENTRY_POINTS = ["remove_workspace_dir", "safe_remove_workspace_dir"]
+
+MALFORMED = {
+    "landed_null": "recorded landed receipt is malformed",
+    "landed_not_list": "recorded landed receipt is malformed",
+    "landed_empty": "recorded landed receipt is malformed",
+    "entry_junk": "recorded landed receipt is malformed",
+    "entry_no_sha": "recorded landed receipt is malformed",
+    "entry_no_repository": "recorded landed receipt is malformed",
+    "refs_null_with_missing_repo": "recorded repository missing",
+}
+
+
+@pytest.mark.parametrize("entry", ENTRY_POINTS)
+@pytest.mark.parametrize("shape", sorted(MALFORMED))
+def test_malformed_recorded_landed_receipt_holds_with_a_persisted_reason(board, tmp_path, shape, entry):
+    tid, ws = landed_task(board, tmp_path)
+    corrupt(board, tid, shape)
+
+    assert reclaim(board, tid, ws, entry) is False
+    assert ws.exists() and (ws / "w" / "w.py").exists()
+    held = ks._state(board, tid)[1]
+    assert held and held.startswith("survivor_unavailable:"), held
+    assert MALFORMED[shape] in held, held
+    reasons = held_events(board, tid)
+    assert reasons and MALFORMED[shape] in reasons[-1], reasons
+
+
+@pytest.mark.parametrize("entry", ENTRY_POINTS)
+def test_valid_recorded_landed_receipt_still_reclaims(board, tmp_path, entry):
+    tid, ws = landed_task(board, tmp_path)
+    assert reclaim(board, tid, ws, entry) is True
+    assert not ws.exists()
+    assert ks._state(board, tid)[1] is None
+    assert held_events(board, tid) == []
+
+
+@pytest.mark.parametrize("exc", [TypeError, KeyError, AttributeError, IndexError])
+def test_unvalidated_record_shape_is_held_by_the_backstop(board, tmp_path, monkeypatch, exc):
+    """Any future unvalidated read of the persisted row still fails closed.
+
+    The explicit validators cover the shapes known today; the backstop in
+    `preserve()` is what keeps the NEXT unvalidated index from escaping
+    `_hold()` again.
+    """
+    tid, ws = landed_task(board, tmp_path)
+
+    def boom(previous):
+        raise exc("synthetic malformed row")
+
+    monkeypatch.setattr(ks, "_recorded_landed_claims", boom)
+    assert ks.remove_workspace_dir(board, tid, ws) is False
+    assert ws.exists()
+    held = ks._state(board, tid)[1]
+    assert held == ("survivor_unavailable: recorded survivor state is malformed "
+                    f"({exc.__name__}); repair the row or recover the workspace by hand")
+    assert held in held_events(board, tid)