fix(health): fsck survives approved delete proposals

[michiot05]

what changed

health._check_decided_proposals builds its presence map from the four create kinds (CLAIM / PAGE / ENTITY / RELATION) and then indexes it with presence[pr.kind] for every approved proposal. ProposalKind.DELETE isn't in the map, so the first approved delete proposal makes vouch fsck traceback with KeyError: <ProposalKind.DELETE: 'delete'> — permanently, since the decided entry never goes away:

  File ".../src/vouch/health.py", line 459, in _check_decided_proposals
    if artifact_id not in presence[pr.kind]:
                          ~~~~~~~~^^^^^^^^^
KeyError: <ProposalKind.DELETE: 'delete'>

repro on a clean kb: propose an entity → approve → propose_delete it → approve → vouch fsck.

the fix teaches the check what an approved delete actually promises — the inverse of the create kinds. for DELETE proposals the target artifact should be gone, so a target still on disk (a crash between the artifact removal and the decided-move) is now reported as a new decided_delete_artifact_present error finding, the mirror of the existing decided_missing_artifact. targets that are gone pass clean.

why

fsck landed before delete proposals did, and the delete feature updated proposals, storage, index, and all three surfaces but never taught fsck the new kind (git log -S "ProposalKind.DELETE" -- src/vouch/health.py is empty). the presence check's docstring — "every approved proposal should have its artifact on disk" — is exactly backwards for a delete, so the fix encodes the inverse rather than just skipping the kind.

what would break for an existing .vouch/

nothing — no on-disk shape, object model, or method surface changes. read-only check; the only behavior change is vouch fsck completing (and reporting honest findings) on kbs where it previously crashed.

test plan

two regression tests in tests/test_health.py, both failing on the previous code with the exact KeyError above:

• test_fsck_approved_delete_with_artifact_gone_is_clean — the healthy case: approved delete, target absent, fsck completes with no decided_* finding
• test_fsck_flags_approved_delete_artifact_still_present — the drift case: target survived on disk, fsck reports decided_delete_artifact_present and ok is False

validation run locally:

python -m pytest tests/ -q --ignore=tests/embeddings   # all passed
python -m mypy src                                     # no issues in 102 files
python -m ruff check src tests                         # all checks passed

Summary by CodeRabbit

• Bug Fixes
  • Fixed a crash in vouch fsck when a knowledge base contains approved delete proposals.
  • Approved deletions are now validated correctly:
    • If the target artifact is gone, fsck reports it as clean.
    • If the target artifact still exists, fsck flags decided_delete_artifact_present.
    • If the decided delete has an unsupported/invalid target kind, fsck flags decided_delete_invalid_target_kind.
• Tests
  • Added regression coverage for approved delete scenarios to prevent future fsck inconsistencies.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d877d335-672a-4d8f-881e-2a9156004037

📥 Commits

Reviewing files that changed from the base of the PR and between a46c068 and 8fb2a2b.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/vouch/health.py
• tests/test_health.py

🚧 Files skipped from review as they are similar to previous changes (2)

• CHANGELOG.md
• src/vouch/health.py

---

Walkthrough

Changes

Approved delete proposal consistency

Layer / File(s)	Summary
Validate approved delete artifact presence src/vouch/health.py, CHANGELOG.md	_check_decided_proposals validates approved deletes, reports invalid or still-present targets, excludes deletes from generic missing-artifact findings, and documents the fix.
Cover approved delete fsck cases tests/test_health.py	Regression tests cover approved deletes with absent targets, present targets, and invalid target kinds.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• vouchdev/vouch#112: Introduces the decided-proposal consistency checks refined by this fix.

Suggested reviewers: galuis116, johnb1109

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main fix: fsck now handles approved delete proposals without crashing.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/vouch/health.py`:
- Around line 439-440: Update the added docstring prose in the approved delete
proposal path to begin with lowercase text, changing “Approved delete
proposals...” to “approved delete proposals...”; preserve the rest of the
wording and behavior unchanged.
- Around line 462-475: Update the DELETE handling in the proposal health-check
flow to validate `target_kind` before querying `presence_by_target_kind`. Emit a
consistency `Finding` when the value is missing or unsupported, and only perform
the artifact membership check for valid target kinds; do not let invalid deletes
pass as clean.

In `@tests/test_health.py`:
- Around line 307-313: The test helper _decide_proposal must use the real
proposal lifecycle instead of manually relocating YAML: create the proposal
through the normal propose flow and call proposals.approve() to produce the
decided state. Update the drift regression to recreate the target artifact after
approval, while preserving the review gate and avoiding direct filesystem
promotion of an APPROVED proposal.
- Around line 330-333: Update the clean-case test around the health.fsck call to
assert report.ok is True in addition to checking the finding codes, ensuring
unrelated findings cannot make the regression test pass.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 72784f2b-4fa2-490f-9260-bbe92c703100

📥 Commits

Reviewing files that changed from the base of the PR and between 1c9ca7c and a46c068.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/vouch/health.py
• tests/test_health.py

[michiot05]

all four findings addressed in the new commit:

• docstring prose lowercased.
• a missing or unknown target_kind on an approved delete is now reported as a new decided_delete_invalid_target_kind error instead of passing as clean, with a regression test covering it (that one hand-lands the decided yaml deliberately — propose_delete validates the kind, so corruption is the only way the state can exist, and fsck is the after-the-fact net for exactly that).
• both main regressions now run the real lifecycle: propose_entity → approve → propose_delete → approve, with the drift case recreating the entity after approval. no filesystem promotion of approved proposals.
• the clean case asserts report.ok is True.

worth flagging: switching to the real flow surfaced a second gap the manual tests had hidden. after a gate-approved delete, the target's original create proposal tripped decided_missing_artifact — so a kb that created and then deleted an artifact through the gate would report drift forever. the new commit exempts create proposals whose artifact was removed by a matching approved delete, and the real-lifecycle clean-case test now locks that in (report.ok is True fails without it). thanks for pushing on the test fidelity — it found a real bug.

validation rerun: pytest tests/ -q --ignore=tests/embeddings, mypy src, ruff check src tests — all green.