From 9ff5c16c52fbacfa9972f273f3929c9907236641 Mon Sep 17 00:00:00 2001 From: kurosawareiji7007-hub Date: Fri, 31 Jul 2026 09:58:18 -0700 Subject: [PATCH 1/2] fix(health): fsck survives approved DELETE and GOAL proposals presence was keyed on four create kinds, so any KB that had approved a delete or a goal crashed vouch fsck with KeyError. Handle DELETE against target_kind (and skip legitimately removed ids on the create pass), treat GOAL as a create kind, and pin exhaustiveness to ProposalKind in tests. Fixes #682 --- CHANGELOG.md | 13 ++++++ src/vouch/health.py | 98 +++++++++++++++++++++++++++++++++++++++++++- tests/test_health.py | 88 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b0113b4..6cadc14e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,19 @@ 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 approved DELETE or GOAL proposals** + (#682; revisits #538/#683): `_check_decided_proposals` indexed a + `presence` dict by every approved proposal's own kind, but only the + four create kinds (claim/page/entity/relation) were keys — so any KB + that had approved a delete or a goal crashed `fsck()` with an uncaught + `KeyError`. DELETE proposals are checked in a first pass against + `target_kind` (reporting `decided_delete_invalid_target_kind` / + `decided_delete_artifact_present` as appropriate), and ids they + legitimately removed are excluded from the second pass so the original + creating proposal does not false-positive as `decided_missing_artifact`. + GOAL is a create kind and is checked against `list_goals()`. An + exhaustiveness test ties the artifact-kind set to `ProposalKind` so a + seventh member fails the suite instead of users. - **`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..87f13298 100644 --- a/src/vouch/health.py +++ b/src/vouch/health.py @@ -462,6 +462,21 @@ def _check_claim_graph_refs( ) +# Artifact kinds whose approve path writes a durable file. DELETE is the +# inverse (approve removes the file) and is handled in a dedicated pass. +# Kept exhaustive over ``ProposalKind`` minus DELETE so a seventh kind +# fails the suite instead of crashing ``vouch fsck`` for users (#682/#683). +_ARTIFACT_PROPOSAL_KINDS: frozenset[ProposalKind] = frozenset( + { + ProposalKind.CLAIM, + ProposalKind.PAGE, + ProposalKind.ENTITY, + ProposalKind.RELATION, + ProposalKind.GOAL, + } +) + + def _check_decided_proposals( store: KBStore, claims: dict[str, Claim], @@ -473,7 +488,10 @@ def _check_decided_proposals( 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 +499,22 @@ def _check_decided_proposals( ProposalKind.PAGE: set(pages), ProposalKind.ENTITY: set(entities), ProposalKind.RELATION: relations, + ProposalKind.GOAL: {g.id for g in store.list_goals()}, } - for pr in store.list_proposals(ProposalStatus.APPROVED): + # Exhaustiveness over ProposalKind is enforced by + # test_artifact_proposal_kinds_cover_enum — a new member must land in + # _ARTIFACT_PROPOSAL_KINDS (or DELETE) before it can KeyError fsck. + + 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( @@ -494,6 +526,68 @@ def _check_decided_proposals( ) ) 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( + Finding( + "error", + "decided_no_artifact_id", + f"approved proposal {pr.id} has no payload id", + [pr.id], + ) + ) + continue + if pr.kind not in presence: + # Unreachable while the exhaustiveness assert holds; keep a + # Finding path so a runtime enum drift still reports cleanly + # instead of KeyError'ing the whole fsck. + findings.append( + Finding( + "error", + "decided_unknown_kind", + f"approved proposal {pr.id} has unrecognized kind " + f"{pr.kind.value!r}", + [pr.id], + ) + ) + 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..8ec78a26 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,93 @@ def test_fsck_decided_missing_artifact(store: KBStore) -> None: assert "decided_missing_artifact" in codes +def test_artifact_proposal_kinds_cover_enum() -> None: + """Every ProposalKind is either DELETE or an artifact kind fsck knows. + + Hand-maintained presence maps over a growing enum are what made #682 + (DELETE) and the #683 follow-up (GOAL) crash vouch fsck. A new member + must extend _ARTIFACT_PROPOSAL_KINDS (or be DELETE) or this fails. + """ + assert health._ARTIFACT_PROPOSAL_KINDS | {ProposalKind.DELETE} == set( + ProposalKind + ) + assert ProposalKind.DELETE not in health._ARTIFACT_PROPOSAL_KINDS + + +def test_fsck_survives_approved_delete_proposal(store: KBStore) -> None: + """An approved delete must not crash fsck, and must not false-positive + the original create proposal as decided_missing_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_survives_approved_goal_proposal(store: KBStore) -> None: + """Approved GOAL proposals are live create kinds — must not KeyError.""" + 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 "decided_missing_artifact" not in codes + assert report.ok is True + + +def test_fsck_flags_delete_whose_artifact_still_exists(store: KBStore) -> None: + """Delete approved but target still on disk — the delete never took.""" + 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: + """Malformed DELETE with no payload id is reported, 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_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") From 3ed92977168bd61c09eb661e4a203acbdfe580b2 Mon Sep 17 00:00:00 2001 From: kurosawareiji7007-hub Date: Fri, 31 Jul 2026 10:10:58 -0700 Subject: [PATCH 2/2] test(health): cover create-proposal missing id in fsck drop the unreachable decided_unknown_kind branch (exhaustiveness test guards enum drift) and hit the second-pass decided_no_artifact_id path so diff-coverage stays at 100%. --- src/vouch/health.py | 14 -------------- tests/test_health.py | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/vouch/health.py b/src/vouch/health.py index 87f13298..6800903c 100644 --- a/src/vouch/health.py +++ b/src/vouch/health.py @@ -572,20 +572,6 @@ def _check_decided_proposals( ) ) continue - if pr.kind not in presence: - # Unreachable while the exhaustiveness assert holds; keep a - # Finding path so a runtime enum drift still reports cleanly - # instead of KeyError'ing the whole fsck. - findings.append( - Finding( - "error", - "decided_unknown_kind", - f"approved proposal {pr.id} has unrecognized kind " - f"{pr.kind.value!r}", - [pr.id], - ) - ) - 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]: diff --git a/tests/test_health.py b/tests/test_health.py index 8ec78a26..8a8e67fa 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -403,6 +403,21 @@ def test_fsck_flags_delete_proposal_with_no_artifact_id(store: KBStore) -> None: assert "decided_no_artifact_id" in codes +def test_fsck_flags_create_proposal_with_no_artifact_id(store: KBStore) -> None: + """Second-pass create/edit proposals also report a missing payload id.""" + store.put_proposal(Proposal( + id="claim-no-id", + kind=ProposalKind.CLAIM, + proposed_by="agent", + payload={"text": "t", "evidence": ["e1"]}, + 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_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")