fix(health): fsck survives approved delete proposals - #683
Conversation
_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 vouchdev#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 vouchdev#682
…elete-proposals # Conflicts: # CHANGELOG.md
…-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.
|
the diagnosis is right, and the second-pass exclusion you found while verifying — skipping the ids a delete legitimately removed, so the original creating proposal does not false-positive as but the fix is one kind short, and the same verified against current pr = proposals.propose_goal(store, title="ship the thing", proposed_by="agent")
proposals.approve(store, pr.id, approved_by="reviewer")
health.fsck(store)goal proposals are fully live — which points at the actual root cause: happy to see this merge with |
…elete-proposals # Conflicts: # CHANGELOG.md
a6c6862 (vouchdev#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 (vouchdev#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`.
|
closing this — the fix is one proposal kind short, and the PR's own new line is where the crash now lands.
verified against current pr = proposals.propose_goal(store, title="ship the thing", proposed_by="agent")
proposals.approve(store, pr.id, approved_by="reviewer")
health.fsck(store)goals are fully live — the delete logic itself is good and the second-pass exclusion you found while verifying — skipping ids a delete legitimately removed so the original creating proposal does not false-positive as what i want instead of a please reopen with that and i will merge it. this is a scope objection, not a quality one. |
|
reopening failed via the API for this branch, so opened fresh instead: #735 - same branch, GOAL added to presence per the review above, plus the exhaustiveness assert. |
What changed
_check_decided_proposalsinsrc/vouch/health.pynow handlesProposalKind.DELETEproposals in a dedicated first pass instead ofindexing a
presencedict that has no entry for that kind.Why
presencewas keyed byProposalKindfor the four artifact kinds(
CLAIM/PAGE/ENTITY/RELATION), and every approved proposal waslooked up by
presence[pr.kind]. Butpropose_delete()files proposalswith
kind=ProposalKind.DELETE, andapprove()explicitly allows aDELETEproposal to clear without its target artifact pre-existing(it's being removed, not created). Any approved delete proposal reaching
this loop hit
presence[ProposalKind.DELETE]→ uncaughtKeyError,aborting
fsck()entirely — andvouch fsck's CLI entry has noexception handling around the call, so the command itself crashed.
Confirmed end-to-end: approve a claim, then approve a delete of it, then
call
health.fsck(store)— raises.Delete proposals are now checked against
target_kind(the kind ofwhat was deleted, not
pr.kind, which is alwaysDELETE):decided_delete_invalid_target_kindif it's missing or unrecognized,decided_delete_artifact_presentif the target somehow still exists ondisk. A second pass then checks ordinary create/edit proposals, skipping
any artifact id a delete proposal legitimately removed — without that
exclusion, the original creating proposal (still recorded
APPROVEDin
decided/) false-positives asdecided_missing_artifactthe momentits artifact is correctly deleted, which I hit and fixed while verifying
this change.
Fixes #682
What might break
Nothing for users with an existing
.vouch/directory — no on-diskshape,
kb.*method, or object model change. Behaviorally:vouch fscknow runs to completion instead of crashing on any KB with an approved
delete in its history, and correctly reports zero findings for a
legitimate delete rather than either crashing or false-positiving.
VEP
Not applicable — no object model,
kb.*method, on-disk layout, bundleformat, or audit-log shape change. A health-check logic fix inside
health.py.Prior art
This exact defect and a fix were previously submitted as #538
(
fix(health): fsck survives approved delete proposals,CodeRabbit-reviewed, author addressed all review feedback) but closed
unmerged on 2026-07-29 purely for going stale against
test(aCHANGELOG.mdconflict) — the maintainer's closing comment explicitlyinvited a rebase or fresh PR. Re-verified independently against current
testHEAD: the crash is still live. This PR reintroduces the fix,freshly rebased, with the additional false-positive fix for the
originating creation proposal.
Tests
make check-equivalent: ruff clean (src+tests); mypyclean on
health.py; all 26tests/test_health.pycases pass (23pre-existing + 3 new)
test_fsck_survives_approved_delete_proposal,test_fsck_flags_delete_whose_artifact_still_exists,test_fsck_flags_delete_with_invalid_target_kindCHANGELOG.mdupdated under## [Unreleased]