From a46c068507510d0a32e88e86914897142398e7f6 Mon Sep 17 00:00:00 2001 From: michiot05 <281539540+michiot05@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:27:37 +0200 Subject: [PATCH 1/2] fix(health): fsck survives approved delete proposals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the decided-proposals check builds its presence map from the four create kinds only, so `presence[pr.kind]` raised a KeyError the moment a kb held an approved delete proposal — `vouch fsck` tracebacked permanently on any kb that ever approved a delete. delete proposals landed after fsck and never taught it the new kind. an approved delete promises the inverse of the create kinds: the target artifact should be gone. fsck now checks exactly that, reporting a still-present target as `decided_delete_artifact_present` (the mirror of `decided_missing_artifact`) instead of crashing. --- CHANGELOG.md | 7 ++++++ src/vouch/health.py | 17 +++++++++++++ tests/test_health.py | 58 +++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0704c6a4..a64aed97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,13 @@ All notable changes to vouch are documented here. Format follows per-prompt block, the session banner, `vouch status` and the opt-in question all say so rather than calling it "this repo's" knowledge. +### Fixed +- `vouch fsck` no longer crashes with a `KeyError` on a KB that has an + approved delete proposal. the decided-proposals check only knew the + create kinds, so the first approved delete broke fsck permanently. + approved deletes are now checked for the inverse drift — a target + artifact still on disk is reported as `decided_delete_artifact_present`. + ## [1.5.0] — 2026-07-20 ### Added diff --git a/src/vouch/health.py b/src/vouch/health.py index 6c8df7d6..d0f11012 100644 --- a/src/vouch/health.py +++ b/src/vouch/health.py @@ -436,6 +436,8 @@ 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. + Approved delete proposals promise the inverse — the target artifact is + gone — so for those the still-present case is surfaced instead. """ relations = {r.id for r in store.list_relations()} presence: dict[ProposalKind, set[str]] = { @@ -444,6 +446,7 @@ def _check_decided_proposals( ProposalKind.ENTITY: set(entities), ProposalKind.RELATION: relations, } + presence_by_target_kind = {kind.value: ids for kind, ids in presence.items()} for pr in store.list_proposals(ProposalStatus.APPROVED): artifact_id = pr.payload.get("id") if isinstance(pr.payload, dict) else None if not artifact_id: @@ -456,6 +459,20 @@ def _check_decided_proposals( ) ) continue + if pr.kind is ProposalKind.DELETE: + target_kind = pr.payload.get("target_kind", "") + if artifact_id in presence_by_target_kind.get(target_kind, set()): + findings.append( + Finding( + "error", + "decided_delete_artifact_present", + f"approved delete proposal {pr.id} promised " + f"{target_kind} {artifact_id} removed but the " + f"artifact is still on disk", + [pr.id, artifact_id], + ) + ) + continue if artifact_id not in presence[pr.kind]: findings.append( Finding( diff --git a/tests/test_health.py b/tests/test_health.py index 26b15a6b..dedef4fd 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -7,7 +7,15 @@ import pytest from vouch import health, index_db -from vouch.models import Claim, ClaimStatus, Proposal, ProposalKind, ProposalStatus +from vouch.models import ( + Claim, + ClaimStatus, + Entity, + EntityType, + Proposal, + ProposalKind, + ProposalStatus, +) from vouch.storage import KBStore, _yaml_dump @@ -296,6 +304,54 @@ def test_fsck_decided_missing_artifact(store: KBStore) -> None: assert "decided_missing_artifact" in codes +def _decide_proposal(store: KBStore, proposal: Proposal) -> None: + """Land an APPROVED proposal in decided/ so list_proposals finds it.""" + store.put_proposal(proposal) + src_path = store.kb_dir / "proposed" / f"{proposal.id}.yaml" + dst_path = store.kb_dir / "decided" / f"{proposal.id}.yaml" + dst_path.write_text(src_path.read_text()) + src_path.unlink() + + +def test_fsck_approved_delete_with_artifact_gone_is_clean(store: KBStore) -> None: + """An approved delete whose target is gone is the healthy case. + + Regression: the presence map only covered create kinds, so any KB with + an approved delete proposal crashed fsck with a KeyError. + """ + _decide_proposal(store, Proposal( + id="prop-del-1", + kind=ProposalKind.DELETE, + proposed_by="agent", + payload={"target_kind": "entity", "id": "gone-entity", "snapshot": {}}, + status=ProposalStatus.APPROVED, + )) + + report = health.fsck(store) + codes = {f.code for f in report.findings} + assert "decided_missing_artifact" not in codes + assert "decided_delete_artifact_present" not in codes + + +def test_fsck_flags_approved_delete_artifact_still_present(store: KBStore) -> None: + """An approved delete whose target survived on disk is the drift case.""" + store.put_entity( + Entity(id="acme-example", name="acme example", type=EntityType.COMPANY) + ) + _decide_proposal(store, Proposal( + id="prop-del-2", + kind=ProposalKind.DELETE, + proposed_by="agent", + payload={"target_kind": "entity", "id": "acme-example", "snapshot": {}}, + status=ProposalStatus.APPROVED, + )) + + report = health.fsck(store) + codes = {f.code for f in report.findings} + assert "decided_delete_artifact_present" in codes + assert report.ok is False + + 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 8fb2a2b633034019c86612f4b6e0bd0ab7469933 Mon Sep 17 00:00:00 2001 From: michiot05 <281539540+michiot05@users.noreply.github.com> Date: Wed, 22 Jul 2026 05:51:44 +0200 Subject: [PATCH 2/2] fix(health): validate delete target kinds; exempt gate-deleted creates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit address the review round: lowercase the added docstring prose, report a missing or unknown delete target_kind as decided_delete_invalid_target_kind instead of treating it as clean, and rework the regression tests to run the real propose/approve lifecycle rather than hand-landing decided yaml. running the real flow exposed a second gap the manual tests hid: after a gate-approved delete, the target's original create proposal tripped decided_missing_artifact — a kb that created and then deleted an artifact through the gate would report drift forever. approved deletes now explain the absence: create proposals whose artifact was removed by a matching approved delete are exempt from the missing-artifact finding. --- CHANGELOG.md | 5 +++- src/vouch/health.py | 32 +++++++++++++++++---- tests/test_health.py | 68 ++++++++++++++++++++++++++++---------------- 3 files changed, 75 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a64aed97..e6ae8b98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,7 +69,10 @@ All notable changes to vouch are documented here. Format follows approved delete proposal. the decided-proposals check only knew the create kinds, so the first approved delete broke fsck permanently. approved deletes are now checked for the inverse drift — a target - artifact still on disk is reported as `decided_delete_artifact_present`. + artifact still on disk is reported as `decided_delete_artifact_present`, + a missing or unknown target kind as `decided_delete_invalid_target_kind` + — and an artifact removed by an approved delete no longer misreports + its create proposal as `decided_missing_artifact`. ## [1.5.0] — 2026-07-20 diff --git a/src/vouch/health.py b/src/vouch/health.py index d0f11012..ca3207b4 100644 --- a/src/vouch/health.py +++ b/src/vouch/health.py @@ -436,8 +436,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. - Approved delete proposals promise the inverse — the target artifact is - gone — so for those the still-present case is surfaced instead. + approved delete proposals promise the inverse — the target artifact is + gone — so for those the still-present case is surfaced instead, and an + artifact a later approved delete removed is not missing: the delete + explains its absence. """ relations = {r.id for r in store.list_relations()} presence: dict[ProposalKind, set[str]] = { @@ -447,7 +449,13 @@ def _check_decided_proposals( ProposalKind.RELATION: relations, } presence_by_target_kind = {kind.value: ids for kind, ids in presence.items()} - for pr in store.list_proposals(ProposalStatus.APPROVED): + approved = store.list_proposals(ProposalStatus.APPROVED) + deleted_by_gate = { + (pr.payload.get("target_kind"), pr.payload.get("id")) + for pr in approved + if pr.kind is ProposalKind.DELETE and isinstance(pr.payload, dict) + } + for pr in approved: artifact_id = pr.payload.get("id") if isinstance(pr.payload, dict) else None if not artifact_id: findings.append( @@ -461,7 +469,18 @@ def _check_decided_proposals( continue if pr.kind is ProposalKind.DELETE: target_kind = pr.payload.get("target_kind", "") - if artifact_id in presence_by_target_kind.get(target_kind, set()): + target_ids = presence_by_target_kind.get(target_kind) + if target_ids is None: + findings.append( + Finding( + "error", + "decided_delete_invalid_target_kind", + f"approved delete proposal {pr.id} has a missing or " + f"unknown target_kind {target_kind!r}", + [pr.id, artifact_id], + ) + ) + elif artifact_id in target_ids: findings.append( Finding( "error", @@ -473,7 +492,10 @@ def _check_decided_proposals( ) ) continue - if artifact_id not in presence[pr.kind]: + if ( + artifact_id not in presence[pr.kind] + and (pr.kind.value, artifact_id) not in deleted_by_gate + ): findings.append( Finding( "error", diff --git a/tests/test_health.py b/tests/test_health.py index dedef4fd..9840280c 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -6,12 +6,11 @@ import pytest -from vouch import health, index_db +from vouch import health, index_db, proposals from vouch.models import ( Claim, ClaimStatus, Entity, - EntityType, Proposal, ProposalKind, ProposalStatus, @@ -304,13 +303,18 @@ def test_fsck_decided_missing_artifact(store: KBStore) -> None: assert "decided_missing_artifact" in codes -def _decide_proposal(store: KBStore, proposal: Proposal) -> None: - """Land an APPROVED proposal in decided/ so list_proposals finds it.""" - store.put_proposal(proposal) - src_path = store.kb_dir / "proposed" / f"{proposal.id}.yaml" - dst_path = store.kb_dir / "decided" / f"{proposal.id}.yaml" - dst_path.write_text(src_path.read_text()) - src_path.unlink() +def _approve_entity_delete(store: KBStore) -> Entity: + """Create and then delete an entity through the real review gate.""" + created = proposals.propose_entity( + store, name="acme example", entity_type="company", proposed_by="agent", + ) + entity = proposals.approve(store, created.id, approved_by="reviewer") + assert isinstance(entity, Entity) + deletion = proposals.propose_delete( + store, target_kind="entity", target_id=entity.id, proposed_by="agent", + ) + proposals.approve(store, deletion.id, approved_by="reviewer") + return entity def test_fsck_approved_delete_with_artifact_gone_is_clean(store: KBStore) -> None: @@ -319,36 +323,52 @@ def test_fsck_approved_delete_with_artifact_gone_is_clean(store: KBStore) -> Non Regression: the presence map only covered create kinds, so any KB with an approved delete proposal crashed fsck with a KeyError. """ - _decide_proposal(store, Proposal( - id="prop-del-1", - kind=ProposalKind.DELETE, - proposed_by="agent", - payload={"target_kind": "entity", "id": "gone-entity", "snapshot": {}}, - status=ProposalStatus.APPROVED, - )) + _approve_entity_delete(store) report = health.fsck(store) codes = {f.code for f in report.findings} assert "decided_missing_artifact" not in codes assert "decided_delete_artifact_present" not in codes + assert report.ok is True def test_fsck_flags_approved_delete_artifact_still_present(store: KBStore) -> None: - """An approved delete whose target survived on disk is the drift case.""" - store.put_entity( - Entity(id="acme-example", name="acme example", type=EntityType.COMPANY) - ) - _decide_proposal(store, Proposal( - id="prop-del-2", + """An approved delete whose target survived on disk is the drift case — + a crash between the artifact removal and the decided-move leaves the + target in place; simulate it by recreating the entity after approval.""" + entity = _approve_entity_delete(store) + store.put_entity(entity) + + report = health.fsck(store) + codes = {f.code for f in report.findings} + assert "decided_delete_artifact_present" in codes + assert report.ok is False + + +def test_fsck_flags_approved_delete_with_invalid_target_kind( + store: KBStore, +) -> None: + """A decided delete with an unknown target_kind is drift, not clean. + + No current write path produces this (propose_delete validates the + kind), so hand-land the decided YAML the way an older writer or a + hand edit could have left it — fsck is the after-the-fact net. + """ + store.put_proposal(Proposal( + id="prop-del-bad", kind=ProposalKind.DELETE, proposed_by="agent", - payload={"target_kind": "entity", "id": "acme-example", "snapshot": {}}, + payload={"target_kind": "wormhole", "id": "x", "snapshot": {}}, status=ProposalStatus.APPROVED, )) + src_path = store.kb_dir / "proposed" / "prop-del-bad.yaml" + dst_path = store.kb_dir / "decided" / "prop-del-bad.yaml" + dst_path.write_text(src_path.read_text()) + src_path.unlink() report = health.fsck(store) codes = {f.code for f in report.findings} - assert "decided_delete_artifact_present" in codes + assert "decided_delete_invalid_target_kind" in codes assert report.ok is False