From 7bef73438d631ab429872c14487e88e93049a74d Mon Sep 17 00:00:00 2001 From: galuis116 Date: Thu, 30 Jul 2026 13:44:52 -0700 Subject: [PATCH 1/4] fix(health): fsck survives approved delete proposals _check_decided_proposals built a presence dict keyed by every approved proposal's own kind, but propose_delete() files proposals with kind=ProposalKind.DELETE and approve() explicitly allows a DELETE proposal to clear without its target pre-existing (it's being removed, not created). presence had no DELETE entry, so any KB that had ever had a delete approved crashed fsck() with an uncaught KeyError - vouch fsck's CLI entry has no exception handling around the call, so the command itself crashed. check delete proposals in a first pass against their target_kind (the kind of what was deleted, not pr.kind which is always DELETE): report decided_delete_invalid_target_kind for a missing or unrecognized target_kind, decided_delete_artifact_present if the target somehow still exists on disk, and otherwise record the id as legitimately deleted. the second pass (ordinary create/edit proposals) skips any artifact id a delete proposal legitimately removed - without that, the *original* creating proposal, still recorded APPROVED in decided/, false-positives as decided_missing_artifact the moment its artifact is correctly deleted. this exact defect and fix were previously submitted as #538 (CodeRabbit-reviewed, author addressed feedback), but that PR was closed unmerged for going stale against a fast-moving test branch, not for anything wrong with the change; the maintainer's closing comment explicitly invited a fresh PR. Fixes #682 --- CHANGELOG.md | 12 ++++++++++ src/vouch/health.py | 56 ++++++++++++++++++++++++++++++++++++++++++-- tests/test_health.py | 50 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed440d72..6fb8a325 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,18 @@ 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. - **`hub_client` ETag lookup is now case-insensitive** (#662): `_request` flattened `resp.headers` (case-insensitive by design) into a plain `dict`, so `pull()`'s `resp_headers.get("ETag")` silently returned diff --git a/src/vouch/health.py b/src/vouch/health.py index ce0269a2..7997017e 100644 --- a/src/vouch/health.py +++ b/src/vouch/health.py @@ -473,7 +473,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]] = { @@ -482,7 +485,54 @@ def _check_decided_proposals( ProposalKind.ENTITY: set(entities), ProposalKind.RELATION: relations, } - for pr in store.list_proposals(ProposalStatus.APPROVED): + 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: + 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 +544,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..92f99978 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 from vouch.storage import KBStore, _yaml_dump @@ -315,6 +316,55 @@ 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_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 b74056a34492836402182e6b6d841cc563ecdd66 Mon Sep 17 00:00:00 2001 From: galuis116 Date: Thu, 30 Jul 2026 20:44:24 -0700 Subject: [PATCH 2/4] fix(health): flag DELETE proposals with no payload id, closing a diff-coverage gap the same missing-artifact-id check the second pass already applies to create/edit proposals was silently skipped for DELETE proposals - report decided_no_artifact_id for a malformed delete proposal too, instead of passing it through with no finding. new test covers the branch the repo's diff-coverage gate flagged as untested. --- src/vouch/health.py | 8 ++++++++ tests/test_health.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/vouch/health.py b/src/vouch/health.py index 7997017e..2b977a01 100644 --- a/src/vouch/health.py +++ b/src/vouch/health.py @@ -497,6 +497,14 @@ def _check_decided_proposals( 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 diff --git a/tests/test_health.py b/tests/test_health.py index 92f99978..d734e579 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -365,6 +365,22 @@ def test_fsck_flags_delete_with_invalid_target_kind(store: KBStore) -> None: 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_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 aefb561902f42bff3d97e17ebec6c0c33abb1b2c Mon Sep 17 00:00:00 2001 From: galuis116 Date: Fri, 31 Jul 2026 00:17:25 -0700 Subject: [PATCH 3/4] fix(capture): restore coerce_numeric on min_observations/dedup_window a6c6862 (#686) fixed capture.load_config's min_observations and dedup_window_seconds to fall back to their defaults on a malformed config value via the new coerce_numeric() helper, instead of raising ValueError straight out of load_config. 47eaf56 (#645, realtime opt-in) branched off the pre-fix capture.py and reintroduced the bare int()/float() calls when it merged into test - the coerce_numeric import survived (nothing else referenced it), but the two call sites it fed didn't, silently reverting the fix and leaving test_load_config_malformed_numeric_falls_back red on `test` HEAD itself, currently failing this PR's CI via ruff's unused-import gate. restore the coerce_numeric() calls, matching recall.load_config's still-intact equivalent. unrelated to this PR's own change (fsck delete-proposal handling); needed only to get CI green on top of a currently-broken `test`. --- src/vouch/capture.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/vouch/capture.py b/src/vouch/capture.py index 9aeb98b8..ec99b654 100644 --- a/src/vouch/capture.py +++ b/src/vouch/capture.py @@ -75,9 +75,15 @@ def load_config(store: KBStore) -> CaptureConfig: return CaptureConfig( enabled=coerce_bool(raw.get("enabled", DEFAULT_ENABLED), DEFAULT_ENABLED), realtime=coerce_bool(raw.get("realtime", DEFAULT_REALTIME), DEFAULT_REALTIME), - min_observations=int(raw.get("min_observations", DEFAULT_MIN_OBSERVATIONS)), - dedup_window_seconds=float( - raw.get("dedup_window_seconds", DEFAULT_DEDUP_WINDOW_SECONDS) + min_observations=coerce_numeric( + raw.get("min_observations", DEFAULT_MIN_OBSERVATIONS), + DEFAULT_MIN_OBSERVATIONS, + int, + ), + dedup_window_seconds=coerce_numeric( + raw.get("dedup_window_seconds", DEFAULT_DEDUP_WINDOW_SECONDS), + DEFAULT_DEDUP_WINDOW_SECONDS, + float, ), answer_mode=answer_mode, ) From 2b2e74f1420cbe02670880855c7097c32c34d0fe Mon Sep 17 00:00:00 2001 From: galuis116 Date: Fri, 31 Jul 2026 10:20:27 -0700 Subject: [PATCH 4/4] fix(health): make _check_decided_proposals presence exhaustive review caught that the delete-proposal fix was one kind short: an approved GOAL proposal hits the identical KeyError two lines earlier, at deleted[pr.kind], since presence only covered CLAIM/PAGE/ENTITY/ RELATION. goals are fully live (proposals.py files and approves them, lifecycle maps them to get_goal), so vouch fsck crashes today on any kb with an approved goal, exactly as it did on an approved delete. add ProposalKind.GOAL to presence, threaded through from store. list_goals() in fsck's caller. tie the map to the enum itself rather than hand-copying members: an assert derives the expected key set as frozenset(ProposalKind) - {DELETE} (DELETE is checked separately against target_kind, not its own presence entry), so a future seventh kind fails the next test that touches fsck instead of crashing for a user the day it lands - the same exhaustiveness shape test_capabilities_matches_jsonl_handlers already uses for method/ handler parity. new regression test confirms an approved goal proposal survives fsck without crashing, and a pinning test confirms presence's expected key set still matches ProposalKind minus DELETE. Fixes #682 --- src/vouch/health.py | 29 +++++++++++++++++++++++++++-- tests/test_health.py | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/vouch/health.py b/src/vouch/health.py index 2b977a01..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,6 +477,7 @@ 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. @@ -484,7 +495,21 @@ def _check_decided_proposals( ProposalKind.PAGE: set(pages), ProposalKind.ENTITY: set(entities), ProposalKind.RELATION: relations, + ProposalKind.GOAL: set(goals), } + # `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 diff --git a/tests/test_health.py b/tests/test_health.py index d734e579..5ae52a83 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -8,7 +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 +from vouch.proposals import approve, propose_claim, propose_delete, propose_goal from vouch.storage import KBStore, _yaml_dump @@ -381,6 +381,37 @@ def test_fsck_flags_delete_proposal_with_no_artifact_id(store: KBStore) -> None: 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")