fix(health): fsck survives approved delete and goal proposals - #735
Open
galuis116 wants to merge 8 commits into
Open
fix(health): fsck survives approved delete and goal proposals#735galuis116 wants to merge 8 commits into
galuis116 wants to merge 8 commits into
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.
…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`.
…elete-proposals # Conflicts: # CHANGELOG.md
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 vouchdev#682
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #683, closed for review feedback — same branch, extended per
the reviewer's request.
What changed
_check_decided_proposalsinsrc/vouch/health.pyhandlesProposalKind.DELETEproposals in a dedicated first pass instead ofindexing a
presencedict that has no entry for that kind, andpresenceis now exhaustive over every
ProposalKindthat owns an artifact of itsown kind — including
GOAL, added per review on #683.Why
presencewas keyed onCLAIM/PAGE/ENTITY/RELATIONonly, so_check_decided_proposalsraisedKeyErroron any approvedDELETEproposal (
presence[pr.kind], sinceDELETEisn't inpresenceeither).Delete proposals now get their own first pass checked against
target_kind(reportingdecided_delete_invalid_target_kindfor amissing/unrecognized one, or
decided_delete_artifact_presentif thetarget wasn't actually removed), with the ids they legitimately removed
excluded from the second pass so the original creating proposal doesn't
false-positive as
decided_missing_artifactonce its artifact iscorrectly gone.
Review on #683 caught that this was one kind short: an approved
GOALproposal hits the identical
KeyErrortwo lines earlier, atdeleted[pr.kind], sincedeletedsharespresence's same four keys.Verified against
testHEAD:Goals are fully live (
proposals.pyfiles and approves them,lifecyclemaps them to
get_goal), sovouch fsckcrashes today on any KB with anapproved goal, exactly as it did on an approved delete.
Rather than adding a
GOALline next toDELETEand leaving the same gapopen for a seventh kind,
presenceis now tied toProposalKinditself:an assert derives the expected key set as
frozenset(ProposalKind) - {DELETE}(DELETEis checked separately againsttarget_kind, not itsown presence entry) rather than hand-copying members, so a future kind
fails the next test that touches
fsckinstead of crashing for a user —the same exhaustiveness shape
test_capabilities_matches_jsonl_handlersalready uses for method/handler parity.
Fixes #682
What might break
Nothing for users with an existing
.vouch/directory — no on-disk shape,kb.*method, or object model change._check_decided_proposalsgains agoalsparameter (internal, not a public signature) threaded fromfsck's existingstore.list_goals()call.VEP
Not applicable — no object model,
kb.*method, on-disk layout, bundleformat, or audit-log shape change. A crash fix + exhaustiveness guard
inside
fsck's internals.Tests
make check-equivalent: ruff clean (src+tests); mypyclean on
health.py; alltests/test_health.pycases pass (29total: 25 pre-existing + 1 for the delete-proposal fix + 2 new for
the goal fix + exhaustiveness pin, plus the earlier
decided_no_artifact_idDELETE-payload test)test_fsck_survives_approved_goal_proposal(confirmed it raisesKeyErroron the pre-fix code, passes with the fix) andtest_check_decided_proposals_presence_covers_every_non_delete_kind(pins the exhaustiveness shape)
CHANGELOG.mdupdated under## [Unreleased]