diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b0113b4..af07f388 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/vouch/health.py b/src/vouch/health.py index ce0269a2..aa4da3ea 100644 --- a/src/vouch/health.py +++ b/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,13 +477,17 @@ 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_()` 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]] = { @@ -481,8 +495,77 @@ def _check_decided_proposals( 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( diff --git a/tests/test_health.py b/tests/test_health.py index f81374c1..5ae52a83 100644 --- a/tests/test_health.py +++ b/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")