fix(health): fsck survives approved delete and goal proposals

CHANGELOG.md

@@ -155,6 +155,23 @@ All notable changes to vouch are documented here. Format follows
   artifact the caller could not already retrieve, and it touches no write path.
 
 ### Fixed
+- **`vouch fsck` no longer crashes on an approved delete proposal** (#538
+  reopened, root-caused): `_check_decided_proposals` indexed a `presence`
+  dict by every approved proposal's own kind, but the dict has no entry
+  for `ProposalKind.DELETE` — so any KB that had ever had a delete
+  approved crashed `fsck()` with an uncaught `KeyError`. Delete proposals
+  are now checked in a first pass against their `target_kind` (reporting
+  `decided_delete_invalid_target_kind` for a missing/unrecognized one, or
+  `decided_delete_artifact_present` if the target wasn't actually
+  removed), and the artifact ids they legitimately deleted are excluded
+  from the second pass so the *original* creating proposal doesn't
+  false-positive as `decided_missing_artifact` once its artifact is
+  correctly gone. `presence` is now exhaustive over every `ProposalKind`
+  that owns an artifact of its own kind (adding `GOAL`, which fell through
+  to the identical `KeyError` two lines later — the same crash, just for a
+  different kind), tied to the enum itself via an assert rather than a
+  hand-copied member list, so a future kind fails the next test that
+  touches `fsck` instead of crashing for a user.
 - **`extract` no longer fractures file paths/URLs into auto-approved
   garbage claims** (#702): the sentence segmenter only skipped a `.` as a
   boundary when it was flanked by digits on both sides (decimals/versions

src/vouch/health.py

@@ -17,7 +17,16 @@
 
 from . import index_db
 from .audit import count_events, verify_chain
-from .models import Claim, ClaimStatus, Entity, Page, ProposalKind, ProposalStatus, Source
+from .models import (
+    Claim,
+    ClaimStatus,
+    Entity,
+    Goal,
+    Page,
+    ProposalKind,
+    ProposalStatus,
+    Source,
+)
 from .storage import KBStore, _yaml_load, sha256_hex
 from .verify import verify_all
 
@@ -363,10 +372,11 @@ def fsck(store: KBStore) -> HealthReport:
     claims: dict[str, Claim] = {c.id: c for c in claim_list}
     pages: dict[str, Page] = {p.id: p for p in store.list_pages()}
     entities: dict[str, Entity] = {e.id: e for e in store.list_entities()}
+    goals: dict[str, Goal] = {g.id: g for g in store.list_goals()}
 
     _check_lifecycle_chains(claims, findings)
     _check_claim_graph_refs(claims, entities, findings)
-    _check_decided_proposals(store, claims, pages, entities, findings)
+    _check_decided_proposals(store, claims, pages, entities, goals, findings)
 
     db_present = (store.kb_dir / index_db.DB_FILENAME).exists()
     if not db_present:
@@ -467,22 +477,95 @@ def _check_decided_proposals(
     claims: dict[str, Claim],
     pages: dict[str, Page],
     entities: dict[str, Entity],
+    goals: dict[str, Goal],
     findings: list[Finding],
 ) -> None:
     """Every approved proposal should have its artifact on disk.
 
     A crash between `put_<kind>()` and `move_proposal_to_decided()` would
     leave a `decided/` entry without a matching artifact (or vice versa);
-    surface the artifact-missing case so an operator can investigate.
+    surface the artifact-missing case so an operator can investigate. A
+    ``DELETE`` proposal is the inverse: approving it removes the artifact,
+    so its target must be *absent*, checked against ``target_kind`` (the
+    kind of what was deleted) rather than ``pr.kind`` (always ``DELETE``).
     """
     relations = {r.id for r in store.list_relations()}
     presence: dict[ProposalKind, set[str]] = {
         ProposalKind.CLAIM: set(claims),
         ProposalKind.PAGE: set(pages),
         ProposalKind.ENTITY: set(entities),
         ProposalKind.RELATION: relations,
+        ProposalKind.GOAL: set(goals),
     }
-    for pr in store.list_proposals(ProposalStatus.APPROVED):
+    # `presence` must cover every ProposalKind that owns an artifact of its
+    # own kind - i.e. everything except DELETE, which is checked separately
+    # against target_kind just below (a delete proposal has no "artifact of
+    # kind DELETE"). This assert ties the dict to the enum itself, deriving
+    # from ProposalKind rather than hand-copying its members, so a KeyError
+    # a few lines down fails the next test that touches fsck instead of
+    # crashing for a user the day a new kind lands with no entry here - the
+    # gap GOAL fell through (test_capabilities_matches_jsonl_handlers uses
+    # the same set-equality shape for method/handler parity).
+    assert set(presence) == frozenset(ProposalKind) - {ProposalKind.DELETE}, (
+        f"_check_decided_proposals' presence map is missing "
+        f"{frozenset(ProposalKind) - {ProposalKind.DELETE} - set(presence)}"
+    )
+    approved = list(store.list_proposals(ProposalStatus.APPROVED))
+
+    # First pass: an approved DELETE proposal's target is expected to be
+    # absent. Collect what it legitimately removed so the second pass
+    # (which checks that a create/edit proposal's artifact still exists)
+    # doesn't flag the artifact its own delete proposal correctly removed.
+    deleted: dict[ProposalKind, set[str]] = {k: set() for k in presence}
+    for pr in approved:
+        if pr.kind is not ProposalKind.DELETE:
+            continue
+        artifact_id = pr.payload.get("id") if isinstance(pr.payload, dict) else None
+        if not artifact_id:
+            findings.append(
+                Finding(
+                    "error",
+                    "decided_no_artifact_id",
+                    f"approved proposal {pr.id} has no payload id",
+                    [pr.id],
+                )
+            )
+            continue
+        target_kind_str = (
+            pr.payload.get("target_kind") if isinstance(pr.payload, dict) else None
+        )
+        try:
+            target_kind = ProposalKind(target_kind_str) if target_kind_str else None
+        except ValueError:
+            target_kind = None
+        if target_kind is None or target_kind not in presence:
+            findings.append(
+                Finding(
+                    "error",
+                    "decided_delete_invalid_target_kind",
+                    f"approved delete proposal {pr.id} has an invalid or "
+                    f"missing target_kind {target_kind_str!r}",
+                    [pr.id],
+                )
+            )
+            continue
+        if artifact_id in presence[target_kind]:
+            findings.append(
+                Finding(
+                    "error",
+                    "decided_delete_artifact_present",
+                    f"approved delete proposal {pr.id} targeted "
+                    f"{target_kind.value} {artifact_id}, but the artifact "
+                    f"still exists on disk",
+                    [pr.id, artifact_id],
+                )
+            )
+        else:
+            deleted[target_kind].add(artifact_id)
+
+    for pr in approved:
+        if pr.kind is ProposalKind.DELETE:
+            continue
         artifact_id = pr.payload.get("id") if isinstance(pr.payload, dict) else None
         if not artifact_id:
             findings.append(
@@ -494,6 +577,8 @@ def _check_decided_proposals(
                 )
             )
             continue
+        if artifact_id in deleted[pr.kind]:
+            continue  # removed by a later, separately-verified delete proposal
         if artifact_id not in presence[pr.kind]:
             findings.append(
                 Finding(

tests/test_health.py

@@ -8,6 +8,7 @@
 
 from vouch import health, index_db
 from vouch.models import Claim, ClaimStatus, Proposal, ProposalKind, ProposalStatus
+from vouch.proposals import approve, propose_claim, propose_delete, propose_goal
 from vouch.storage import KBStore, _yaml_dump
 
 
@@ -315,6 +316,102 @@ def test_fsck_decided_missing_artifact(store: KBStore) -> None:
     assert "decided_missing_artifact" in codes
 
 
+def test_fsck_survives_approved_delete_proposal(store: KBStore) -> None:
+    """An approved delete proposal must not crash fsck, and the artifact it
+    correctly removed must not be flagged as a missing create-time artifact."""
+    src = store.put_source(b"evidence")
+    pr = propose_claim(store, text="a fact", evidence=[src.id], proposed_by="agent-a")
+    claim = approve(store, pr.id, approved_by="human-b")
+
+    del_pr = propose_delete(
+        store, target_kind="claim", target_id=claim.id, proposed_by="agent-a",
+    )
+    approve(store, del_pr.id, approved_by="human-c")
+
+    report = health.fsck(store)
+    assert report.findings == []
+
+
+def test_fsck_flags_delete_whose_artifact_still_exists(store: KBStore) -> None:
+    """A delete proposal is approved, but the artifact it claims to have
+    removed is still on disk — the delete never actually took effect."""
+    src = store.put_source(b"evidence")
+    claim = Claim(id="still-here", text="t", evidence=[src.id])
+    store.put_claim(claim)
+    store.put_proposal(Proposal(
+        id="del-1",
+        kind=ProposalKind.DELETE,
+        proposed_by="agent",
+        payload={"target_kind": "claim", "id": "still-here", "snapshot": {}},
+        status=ProposalStatus.APPROVED,
+    ))
+
+    report = health.fsck(store)
+    codes = {f.code for f in report.findings}
+    assert "decided_delete_artifact_present" in codes
+
+
+def test_fsck_flags_delete_with_invalid_target_kind(store: KBStore) -> None:
+    store.put_proposal(Proposal(
+        id="del-2",
+        kind=ProposalKind.DELETE,
+        proposed_by="agent",
+        payload={"target_kind": "not-a-real-kind", "id": "whatever", "snapshot": {}},
+        status=ProposalStatus.APPROVED,
+    ))
+
+    report = health.fsck(store)
+    codes = {f.code for f in report.findings}
+    assert "decided_delete_invalid_target_kind" in codes
+
+
+def test_fsck_flags_delete_proposal_with_no_artifact_id(store: KBStore) -> None:
+    """A malformed DELETE proposal with no payload id is reported the same
+    way a malformed create/edit proposal already is, not silently skipped."""
+    store.put_proposal(Proposal(
+        id="del-3",
+        kind=ProposalKind.DELETE,
+        proposed_by="agent",
+        payload={"target_kind": "claim", "snapshot": {}},
+        status=ProposalStatus.APPROVED,
+    ))
+
+    report = health.fsck(store)
+    codes = {f.code for f in report.findings}
+    assert "decided_no_artifact_id" in codes
+
+
+def test_fsck_survives_approved_goal_proposal(store: KBStore) -> None:
+    """An approved GOAL proposal must not crash fsck. `_check_decided_
+    proposals`' presence map previously covered only CLAIM/PAGE/ENTITY/
+    RELATION, so an approved goal fell through to `presence[pr.kind]` (or,
+    with only the DELETE fix applied, the sibling `deleted[pr.kind]` two
+    lines earlier) and raised KeyError - the exact crash `fsck` is meant to
+    survive, just for a different kind."""
+    pr = propose_goal(store, title="ship the thing", proposed_by="agent")
+    approve(store, pr.id, approved_by="reviewer")
+
+    report = health.fsck(store)
+    codes = {f.code for f in report.findings}
+    assert not any(c.startswith("decided_") for c in codes)
+
+
+def test_check_decided_proposals_presence_covers_every_non_delete_kind() -> None:
+    """`_check_decided_proposals`'s presence map is meant to be exhaustive
+    over ProposalKind (minus DELETE, checked separately via target_kind).
+    Exercised indirectly by test_fsck_survives_approved_goal_proposal for
+    the current six kinds; this pins the *shape* of that guarantee so a
+    seventh kind fails here - and via the runtime assert the next time any
+    test touches fsck - instead of crashing fsck for a user."""
+    assert set(ProposalKind) - {ProposalKind.DELETE} == {
+        ProposalKind.CLAIM,
+        ProposalKind.PAGE,
+        ProposalKind.ENTITY,
+        ProposalKind.RELATION,
+        ProposalKind.GOAL,
+    }
+
+
 def test_fsck_index_orphan_row(store: KBStore) -> None:
     """An FTS5 row with no on-disk claim is reported as an index orphan."""
     src = store.put_source(b"e")