feat(kb): strip dead claim references at approve time and kb-wide#557
Conversation
a page proposal citing claims that were archived, redacted, or bulk- cleared could neither be approved (put_page refuses unknown claim ids) nor repaired without hand-editing. approval now raises a typed DeadClaimRefsError (jsonl code dead_claim_refs) and accepts drop_missing_claims to strip the dead frontmatter refs and inline [claim: ...] markers, recording the dropped ids in the audit event. new kb.wipe_dead_refs (mcp tool, jsonl handler, cli wipe-dead-refs, capabilities entry) sweeps durable pages and pending page proposals in one audited bulk event. the console offers both paths: a strip-and- approve decision card on the dead_claim_refs error, a retry bar for batch approvals, and a dry-run-then-confirm wipe button on the review queue.
github-actions
Bot
added
cli
WalkthroughDead claim references are now detected during page approval, optionally stripped with audit records, and removable across durable pages and pending proposals. CLI, JSONL, MCP, and PendingView expose these flows with dry-run previews and retry actions. ChangesDead claim reference lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant User
participant PendingView
participant kb_wipe_dead_refs
participant wipe_dead_claim_refs
participant KBStore
User->>PendingView: request dead-reference preview
PendingView->>kb_wipe_dead_refs: dry_run=true
kb_wipe_dead_refs->>wipe_dead_claim_refs: scan KB
wipe_dead_claim_refs->>KBStore: inspect pages and proposals
KBStore-->>wipe_dead_claim_refs: dead references and counts
wipe_dead_claim_refs-->>PendingView: preview report
User->>PendingView: confirm wipe
PendingView->>kb_wipe_dead_refs: dry_run=false
kb_wipe_dead_refs->>wipe_dead_claim_refs: apply cleanup
wipe_dead_claim_refs->>KBStore: persist stripped references
wipe_dead_claim_refs-->>PendingView: dropped count and details
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/vouch/lifecycle.py (1)
262-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winthe "dead claim id" predicate is duplicated three times across two files.
proposals.missing_claim_refsalready definesnot store._claim_path(cid).exists();lifecycle.wipe_dead_claim_refsreimplements it inline twice instead of reusing it.
src/vouch/lifecycle.py#L262-L267: reuse a shared "dead ids" helper instead of inlining[c for c in page.claims if not store._claim_path(c).exists()].src/vouch/lifecycle.py#L278-L284: same — reuse the same helper for the pending-proposalrefslist instead of a third inline copy.src/vouch/proposals.py#L69-L74: extract thenot store._claim_path(cid).exists()predicate out ofmissing_claim_refsinto a small shared function (e.g.dead_claim_ids(store, ids)) that bothmissing_claim_refsandlifecycle.wipe_dead_claim_refscall.🤖 Prompt for 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. In `@src/vouch/lifecycle.py` around lines 262 - 267, Extract the dead-claim filtering predicate from proposals.missing_claim_refs into a shared dead_claim_ids helper in src/vouch/proposals.py, then update missing_claim_refs and both inline dead-claim list comprehensions in src/vouch/lifecycle.py (lines 262-267 and 278-284) to call it instead; preserve the existing returned IDs and dry-run behavior.
🤖 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/cli.py`:
- Around line 2643-2687: Rename the `wipe_dead_refs` confirmation-bypass option
so its CLI name clearly indicates that it skips confirmation, while preserving
the existing `skip_confirm` behavior and help text semantics. Update the
`@click.option` declaration and any related command-facing references
consistently; do not make the flag require confirmation.
In `@src/vouch/lifecycle.py`:
- Around line 262-277: Before persisting each modified page in the dead-claim
cleanup loop, revalidate it using the same validate_page flow and
citation-presence condition as proposals.approve()'s drop_missing_claims path.
Apply this after stripping dead claims and before store.update_page(page),
preserving dry-run behavior and preventing citation-requiring pages from being
saved without claims or sources.
In `@src/vouch/server.py`:
- Around line 759-776: The kb_approve exception handling currently converts
DeadClaimRefsError into a generic ValueError. Update kb_approve to catch and
propagate DeadClaimRefsError separately, while preserving the existing
ValueError conversion for ArtifactNotFoundError, ValueError, and other
ProposalError cases, so clients can receive its structured error code and retry
guidance.
In `@webapp/src/views/PendingView.tsx`:
- Around line 304-327: Give the KB-wide wipeDeadRefs mutation its own wipeError
state instead of using decisionFailed and the shared decisionError; set it from
the mutation’s onError while preserving the existing toast behavior. Render
wipeError within the wipeBar action area, and keep proposal-specific
decisionError rendering confined to the selected proposal detail pane.
---
Nitpick comments:
In `@src/vouch/lifecycle.py`:
- Around line 262-267: Extract the dead-claim filtering predicate from
proposals.missing_claim_refs into a shared dead_claim_ids helper in
src/vouch/proposals.py, then update missing_claim_refs and both inline
dead-claim list comprehensions in src/vouch/lifecycle.py (lines 262-267 and
278-284) to call it instead; preserve the existing returned IDs and dry-run
behavior.
🪄 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: 2e1ea3ea-f3cf-45ec-ba01-42fe870b6f4a
📒 Files selected for processing (10)
src/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/hot_memory.pysrc/vouch/jsonl_server.pysrc/vouch/lifecycle.pysrc/vouch/proposals.pysrc/vouch/server.pytests/test_dead_claim_refs.pywebapp/src/views/PendingView.deadrefs.test.tsxwebapp/src/views/PendingView.tsx
| @cli.command(name="wipe-dead-refs") | ||
| @click.option("--dry-run", is_flag=True, default=False, | ||
| help="Preview what would be stripped without making changes") | ||
| @click.option("--confirm", "skip_confirm", is_flag=True, default=False, | ||
| help="Skip confirmation prompt") | ||
| def wipe_dead_refs(dry_run: bool, skip_confirm: bool) -> None: | ||
| """Strip references to claims that no longer exist, KB-wide. | ||
|
|
||
| Scans durable pages and pending page proposals for claim ids that | ||
| resolve to no claim file (archived claims still resolve) and removes | ||
| them — frontmatter list and inline [claim: …] markers both. One | ||
| audited bulk event records what was removed. Run after claims were | ||
| redacted or bulk-cleared and lint reports orphan_page_ref. | ||
| """ | ||
| store = _load_store() | ||
| with _cli_errors(): | ||
| preview = life.wipe_dead_claim_refs(store, actor=_whoami(), dry_run=True) | ||
|
|
||
| if not preview.pages and not preview.proposals: | ||
| click.echo("no dead claim references found") | ||
| return | ||
|
|
||
| click.echo(f"found {preview.dropped} dead claim reference(s):") | ||
| for page_id, dead in preview.pages.items(): | ||
| click.echo(f" page {page_id}: {', '.join(dead)}") | ||
| for pid, dead in preview.proposals.items(): | ||
| click.echo(f" proposal {pid}: {', '.join(dead)}") | ||
|
|
||
| if dry_run: | ||
| click.echo("(dry-run mode: no changes made)") | ||
| return | ||
|
|
||
| if not skip_confirm and not click.confirm( | ||
| f"\nStrip {preview.dropped} dead reference(s)?" | ||
| ): | ||
| click.echo("cancelled") | ||
| return | ||
|
|
||
| with _cli_errors(): | ||
| result = life.wipe_dead_claim_refs(store, actor=_whoami(), dry_run=False) | ||
| click.echo( | ||
| f"stripped {result.dropped} dead reference(s) from " | ||
| f"{len(result.pages)} page(s) and {len(result.proposals)} " | ||
| "pending proposal(s)" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
--confirm flag name inverts its own semantics — skips confirmation instead of requiring it.
the option is named --confirm but is bound to skip_confirm and its help text says "skip confirmation prompt." a user who passes --confirm expecting extra safety on a KB-wide, bulk-mutating command instead bypasses the prompt outright. rename to something unambiguous (--yes / --no-confirm / --force) so the flag name matches its effect.
🛡️ proposed rename
-@click.option("--confirm", "skip_confirm", is_flag=True, default=False,
- help="Skip confirmation prompt")
+@click.option("--yes", "skip_confirm", is_flag=True, default=False,
+ help="Skip confirmation prompt")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @cli.command(name="wipe-dead-refs") | |
| @click.option("--dry-run", is_flag=True, default=False, | |
| help="Preview what would be stripped without making changes") | |
| @click.option("--confirm", "skip_confirm", is_flag=True, default=False, | |
| help="Skip confirmation prompt") | |
| def wipe_dead_refs(dry_run: bool, skip_confirm: bool) -> None: | |
| """Strip references to claims that no longer exist, KB-wide. | |
| Scans durable pages and pending page proposals for claim ids that | |
| resolve to no claim file (archived claims still resolve) and removes | |
| them — frontmatter list and inline [claim: …] markers both. One | |
| audited bulk event records what was removed. Run after claims were | |
| redacted or bulk-cleared and lint reports orphan_page_ref. | |
| """ | |
| store = _load_store() | |
| with _cli_errors(): | |
| preview = life.wipe_dead_claim_refs(store, actor=_whoami(), dry_run=True) | |
| if not preview.pages and not preview.proposals: | |
| click.echo("no dead claim references found") | |
| return | |
| click.echo(f"found {preview.dropped} dead claim reference(s):") | |
| for page_id, dead in preview.pages.items(): | |
| click.echo(f" page {page_id}: {', '.join(dead)}") | |
| for pid, dead in preview.proposals.items(): | |
| click.echo(f" proposal {pid}: {', '.join(dead)}") | |
| if dry_run: | |
| click.echo("(dry-run mode: no changes made)") | |
| return | |
| if not skip_confirm and not click.confirm( | |
| f"\nStrip {preview.dropped} dead reference(s)?" | |
| ): | |
| click.echo("cancelled") | |
| return | |
| with _cli_errors(): | |
| result = life.wipe_dead_claim_refs(store, actor=_whoami(), dry_run=False) | |
| click.echo( | |
| f"stripped {result.dropped} dead reference(s) from " | |
| f"{len(result.pages)} page(s) and {len(result.proposals)} " | |
| "pending proposal(s)" | |
| ) | |
| `@cli.command`(name="wipe-dead-refs") | |
| `@click.option`("--dry-run", is_flag=True, default=False, | |
| help="Preview what would be stripped without making changes") | |
| `@click.option`("--yes", "skip_confirm", is_flag=True, default=False, | |
| help="Skip confirmation prompt") | |
| def wipe_dead_refs(dry_run: bool, skip_confirm: bool) -> None: | |
| """Strip references to claims that no longer exist, KB-wide. | |
| Scans durable pages and pending page proposals for claim ids that | |
| resolve to no claim file (archived claims still resolve) and removes | |
| them — frontmatter list and inline [claim: …] markers both. One | |
| audited bulk event records what was removed. Run after claims were | |
| redacted or bulk-cleared and lint reports orphan_page_ref. | |
| """ | |
| store = _load_store() | |
| with _cli_errors(): | |
| preview = life.wipe_dead_claim_refs(store, actor=_whoami(), dry_run=True) | |
| if not preview.pages and not preview.proposals: | |
| click.echo("no dead claim references found") | |
| return | |
| click.echo(f"found {preview.dropped} dead claim reference(s):") | |
| for page_id, dead in preview.pages.items(): | |
| click.echo(f" page {page_id}: {', '.join(dead)}") | |
| for pid, dead in preview.proposals.items(): | |
| click.echo(f" proposal {pid}: {', '.join(dead)}") | |
| if dry_run: | |
| click.echo("(dry-run mode: no changes made)") | |
| return | |
| if not skip_confirm and not click.confirm( | |
| f"\nStrip {preview.dropped} dead reference(s)?" | |
| ): | |
| click.echo("cancelled") | |
| return | |
| with _cli_errors(): | |
| result = life.wipe_dead_claim_refs(store, actor=_whoami(), dry_run=False) | |
| click.echo( | |
| f"stripped {result.dropped} dead reference(s) from " | |
| f"{len(result.pages)} page(s) and {len(result.proposals)} " | |
| "pending proposal(s)" | |
| ) |
🤖 Prompt for 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.
In `@src/vouch/cli.py` around lines 2643 - 2687, Rename the `wipe_dead_refs`
confirmation-bypass option so its CLI name clearly indicates that it skips
confirmation, while preserving the existing `skip_confirm` behavior and help
text semantics. Update the `@click.option` declaration and any related
command-facing references consistently; do not make the flag require
confirmation.
| for page in store.list_pages(): | ||
| dead = [c for c in page.claims if not store._claim_path(c).exists()] | ||
| if not dead: | ||
| continue | ||
| result.pages[page.id] = dead | ||
| if dry_run: | ||
| continue | ||
| page.claims = [c for c in page.claims if c not in dead] | ||
| page.body = strip_claim_markers(page.body, dead) | ||
| page.updated_at = datetime.now(UTC) | ||
| store.update_page(page) | ||
| with index_db.open_db(store.kb_dir) as conn: | ||
| index_db.index_page( | ||
| conn, id=page.id, title=page.title, body=page.body, | ||
| type=page.type, tags=page.tags, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
durable pages aren't revalidated after stripping — citation-requirement invariant can be silently violated.
proposals.approve()'s drop_missing_claims path rebuilds the Page and calls validate_page(..., has_citations=bool(page.claims or page.sources)) after stripping dead ids (see src/vouch/proposals.py lines 808-820). this function strips the same kind of dead ids from an already-durable page and calls store.update_page(page) directly, with no equivalent revalidation. a page whose kind requires citations, with claims=[dead_id] and sources=[], would end up persisted with zero citations after a bulk wipe — an invariant the approve-time path explicitly protects but this one does not.
🛡️ proposed fix — revalidate before persisting
+from .page_kinds import PageKindError, validate_page
+
def wipe_dead_claim_refs(
store: KBStore,
*,
actor: str,
dry_run: bool = False,
) -> DeadRefsWipeResult:
result = DeadRefsWipeResult(dry_run=dry_run)
for page in store.list_pages():
dead = [c for c in page.claims if not store._claim_path(c).exists()]
if not dead:
continue
result.pages[page.id] = dead
if dry_run:
continue
- page.claims = [c for c in page.claims if c not in dead]
- page.body = strip_claim_markers(page.body, dead)
+ new_claims = [c for c in page.claims if c not in dead]
+ try:
+ validate_page(
+ store, page.type, page.metadata,
+ has_citations=bool(new_claims or page.sources),
+ )
+ except PageKindError:
+ # stripping would violate this kind's citation requirement —
+ # skip this page rather than silently persisting an invalid one.
+ result.pages.pop(page.id, None)
+ continue
+ page.claims = new_claims
+ page.body = strip_claim_markers(page.body, dead)
page.updated_at = datetime.now(UTC)
store.update_page(page)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for page in store.list_pages(): | |
| dead = [c for c in page.claims if not store._claim_path(c).exists()] | |
| if not dead: | |
| continue | |
| result.pages[page.id] = dead | |
| if dry_run: | |
| continue | |
| page.claims = [c for c in page.claims if c not in dead] | |
| page.body = strip_claim_markers(page.body, dead) | |
| page.updated_at = datetime.now(UTC) | |
| store.update_page(page) | |
| with index_db.open_db(store.kb_dir) as conn: | |
| index_db.index_page( | |
| conn, id=page.id, title=page.title, body=page.body, | |
| type=page.type, tags=page.tags, | |
| ) | |
| for page in store.list_pages(): | |
| dead = [c for c in page.claims if not store._claim_path(c).exists()] | |
| if not dead: | |
| continue | |
| result.pages[page.id] = dead | |
| if dry_run: | |
| continue | |
| new_claims = [c for c in page.claims if c not in dead] | |
| try: | |
| validate_page( | |
| store, page.type, page.metadata, | |
| has_citations=bool(new_claims or page.sources), | |
| ) | |
| except PageKindError: | |
| # stripping would violate this kind's citation requirement — | |
| # skip this page rather than silently persisting an invalid one. | |
| result.pages.pop(page.id, None) | |
| continue | |
| page.claims = new_claims | |
| page.body = strip_claim_markers(page.body, dead) | |
| page.updated_at = datetime.now(UTC) | |
| store.update_page(page) | |
| with index_db.open_db(store.kb_dir) as conn: | |
| index_db.index_page( | |
| conn, id=page.id, title=page.title, body=page.body, | |
| type=page.type, tags=page.tags, | |
| ) |
🤖 Prompt for 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.
In `@src/vouch/lifecycle.py` around lines 262 - 277, Before persisting each
modified page in the dead-claim cleanup loop, revalidate it using the same
validate_page flow and citation-presence condition as proposals.approve()'s
drop_missing_claims path. Apply this after stripping dead claims and before
store.update_page(page), preserving dry-run behavior and preventing
citation-requiring pages from being saved without claims or sources.
Source: Path instructions
| def kb_approve( | ||
| proposal_id: str, | ||
| reason: str | None = None, | ||
| drop_missing_claims: bool = False, | ||
| ) -> dict[str, Any]: | ||
| """Approve a proposal → durable artifact. Use carefully. | ||
|
|
||
| A page proposal citing claims that no longer exist is refused; pass | ||
| drop_missing_claims=True to strip the dead references and approve what | ||
| remains (the dropped ids are recorded in the audit event). | ||
| """ | ||
| try: | ||
| artifact = approve(_store(), proposal_id, approved_by=_agent(), | ||
| reason=reason) | ||
| reason=reason, | ||
| drop_missing_claims=drop_missing_claims) | ||
| except (ArtifactNotFoundError, ValueError, ProposalError) as e: | ||
| raise ValueError(str(e)) from e | ||
| return {"kind": type(artifact).__name__.lower(), "id": artifact.id} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Can an MCP python SDK tool function raise a structured/typed error that clients can programmatically distinguish, or only a plain exception message?
💡 Result:
Yes, an MCP Python SDK tool function can raise structured, typed errors that clients can programmatically distinguish [1]. There are two primary mechanisms to achieve this, depending on whether you want to return a tool-level error (for LLM handling) or a protocol-level error (for host/client handling) [1][2]. 1. Structured Protocol-Level Errors (JSON-RPC Errors): By raising an MCPError (from mcp.shared.exceptions), you can propagate a structured error directly to the MCP host, bypassing the default conversion into a tool-result error [1][3]. - This error includes a code (integer), message (string), and an optional data payload [1][4]. - Because MCPError is not caught by the tool wrapper, it propagates as a formal JSON-RPC error response, allowing clients to distinguish specific error conditions programmatically via the error code and associated data [1][3]. - For example, specialized subclasses like UrlElicitationRequiredError use this mechanism to communicate specific flow-control requirements to the client [5][6]. 2. Structured Tool-Level Errors (CallToolResult): If you want the error to be seen and potentially handled by the LLM (rather than the client's transport layer), you should return a CallToolResult object with the isError flag set to True [2][7]. - While this is technically a successful JSON-RPC response (with isError=True), it allows you to pass a structured error message or data within the content field [2][7]. - This approach is typically used when the error is something a "smarter model" might be able to recover from by retrying or correcting its input [1]. Summary of Behavior: - Ordinary exceptions raised in a tool function are automatically caught by the SDK and converted into an error response where isError=True and the exception message is forwarded to the client [1][2]. - To send a strictly typed, programmatic error to the client, use MCPError [1]. - To send an error that the LLM should process as part of the tool-calling flow, return CallToolResult(isError=True, ...) [1][2]. Note: The SDK is under active development (specifically regarding v2). While the high-level API typically propagates MCPError correctly, ensure your implementation aligns with the specific version of the SDK you are using, as older versions or specific layers may require different handling to ensure MCPError is not prematurely swallowed [3][8].
Citations:
- 1: https://py.sdk.modelcontextprotocol.io/v2/servers/handling-errors/
- 2: https://py.sdk.modelcontextprotocol.io/server/
- 3: MCPServer handlers should raise exceptions, not return error objects modelcontextprotocol/python-sdk#2153
- 4: https://py.sdk.modelcontextprotocol.io/v2/api/mcp/shared/exceptions/
- 5: https://github.com/modelcontextprotocol/python-sdk/blob/7ba41dcf/src/mcp/shared/exceptions.py
- 6: Consider extensible pattern for protocol flow-control exceptions modelcontextprotocol/python-sdk#1788
- 7: https://modelcontextprotocol.info/docs/concepts/tools/
- 8: McpError raised in tool handlers is swallowed as isError:true instead of propagating as JSON-RPC error modelcontextprotocol/python-sdk#2770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files and symbols =="
git ls-files | rg '(^|/)server\.py$|(^|/)jsonl_server\.py$|(^|/)capabilities\.py$|(^|/)cli\.py$|proposals\.py$|storage.*\.py$' || true
echo
echo "== server.py kb_approve section =="
if [ -f src/vouch/server.py ]; then
nl -ba src/vouch/server.py | sed -n '730,800p'
fi
echo
echo "== jsonl_server.py similar kb_approve/error handling =="
if [ -f src/vouch/jsonl_server.py ]; then
nl -ba src/vouch/jsonl_server.py | sed -n '1,260p'
echo "--- kb_approve references ---"
rg -n "kb_approve|DeadClaimRefsError|dead_claim_refs|ProposalError|ArtifactNotFoundError" src/vouch/jsonl_server.py
fi
echo
echo "== proposals/storage exception definitions =="
for f in src/vouch/proposals.py src/vouch/storage.py src/vouch/lifecycle.py; do
if [ -f "$f" ]; then
echo "--- $f exception definitions ---"
rg -n "class .*Error|DeadClaimRefsError|drop_missing_claims|audit|approve" "$f" -C 3
fi
done
echo
echo "== imports of DeadClaimRefsError =="
rg -n "DeadClaimRefsError|from .*DeadClaimRefsError|import .*DeadClaimRefsError" src tests || trueRepository: vouchdev/vouch
Length of output: 505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server.py kb_approve section =="
sed -n '730,800p' src/vouch/server.py
echo
echo "== jsonl_server.py kb_approve/error handling section =="
sed -n '1,280p' src/vouch/jsonl_server.py
echo "--- kb_approve references --"
rg -n "kb_approve|DeadClaimRefsError|dead_claim_refs|ProposalError|ArtifactNotFoundError" src/vouch/jsonl_server.py || true
echo
echo "== exception definitions and approve wiring =="
for f in src/vouch/proposals.py src/vouch/storage.py src/vouch/lifecycle.py; do
echo "--- $f exception definitions/wiring ---"
rg -n "class .*Error|DeadClaimRefsError|drop_missing_claims|def approve|audit|approve" "$f" -C 3
done
echo
echo "== all DeadClaimRefsError references =="
rg -n "DeadClaimRefsError|drop_missing_claims|dead_claim_refs" src tests || trueRepository: vouchdev/vouch
Length of output: 42644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server imports and kb_approve surrounding exception handling =="
sed -n '1,80p' src/vouch/server.py
echo "--- kb_approve section with imports in context ---"
sed -n '748,780p' src/vouch/server.py
echo
echo "== jsonl kb_approve exact handler =="
nl -ba src/vouch/jsonl_server.py | sed -n '945,972p'
echo
echo "== tests checking dead_claim_refs envelopes for each surface =="
sed -n '1,180p' tests/test_dead_claim_refs.pyRepository: vouchdev/vouch
Length of output: 3924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== jsonl kb_approve exact handler =="
sed -n '945,972p' src/vouch/jsonl_server.py
echo
echo "== tests checking dead_claim_refs envelopes for each surface =="
sed -n '1,180p' tests/test_dead_claim_refs.py
echo
echo "== exact imports of DeadClaimRefsError in server.py =="
python3 - <<'PY'
from pathlib import Path
p = Path('src/vouch/server.py')
text = p.read_text()
start = text.index('from .proposals import')
end = text.index(':=', start)
print(text[start:end])
print('DeadClaimRefsError in imports:', 'DeadClaimRefsError' in text)
PYRepository: vouchdev/vouch
Length of output: 7516
🌐 Web query:
MCP Python SDK FastMCP tool raise MCPError structured data client can inspect error code
💡 Result:
To raise a structured error that a client can inspect via an error code in the MCP Python SDK and FastMCP, you should use the MCPError class [1][2]. MCPError is designed to propagate as a formal JSON-RPC error response rather than a standard tool execution result [1][3]. When an MCPError is raised within a tool handler, the protocol layer catches it and sends the structured code, message, and optional data payload verbatim to the client [1][4]. 1. Raising MCPError To use this, import MCPError and the desired error codes (from mcp.types or similar constants) [1][4]: from mcp import MCPError from mcp.types import INVALID_PARAMS @mcp.tool def my_tool(arg: str): if not arg: # Client receives this code, message, and data raise MCPError( code=INVALID_PARAMS, message="Argument is missing", data={"details": "arg cannot be empty"}) 2. Behavior in FastMCP FastMCP provides an additional ToolError class [2][5]. While FastMCP wraps generic exceptions into a standard MCP error response (often with isError=True to allow LLMs to read the message) [2][3], raising an MCPError allows you to bypass this wrapping and return a strict JSON-RPC error [1][4]. This is specifically useful when you want to reject a request as a protocol-level error (such as invalid parameters or missing capabilities) rather than a tool execution failure [1][4]. 3. Client Inspection Because MCPError propagates as a JSON-RPC error, the client receiving the response will see a structured error object [1][6]. If using the MCP Python SDK client, this error can be inspected through the code and data fields provided in the JSON-RPC error response [1][7]. If using FastMCP clients, note that call_tool defaults to raising a ToolError for failures; you may need to handle these exceptions or inspect the raw response if you are working with non-FastMCP servers or custom error handling logic [8]. Summary of Error Types: - MCPError: Use for protocol-level errors. It bypasses result wrapping and surfaces as a structured JSON-RPC error [1][4]. - ToolError (FastMCP): Use for tool execution failures. It is caught by the server and returned as a result with isError=True, making the message readable by the LLM [2][5].
Citations:
- 1: https://py.sdk.modelcontextprotocol.io/v2/servers/handling-errors/
- 2: https://fastmcp.wiki/en/servers/tools
- 3: MCPServer handlers should raise exceptions, not return error objects modelcontextprotocol/python-sdk#2153
- 4: https://github.com/modelcontextprotocol/python-sdk/blob/main/docs/migration.md
- 5: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/server/server.py
- 6: McpError raised in tool handlers is swallowed as isError:true instead of propagating as JSON-RPC error modelcontextprotocol/python-sdk#2770
- 7: https://py.sdk.modelcontextprotocol.io/v2/api/mcp/shared/exceptions/
- 8: https://gofastmcp.com/clients/tools
Keep DeadClaimRefsError from collapsing to a generic ValueError in kb_approve.
jsonl_server.py catches DeadClaimRefsError and returns error.code = "dead_claim_refs" so clients can offer the drop_missing_claims=True retry path. Here it falls under ProposalError, gets converted to ValueError(str(e)), and MCP clients only see an error message without an analogous structured code/field.
🤖 Prompt for 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.
In `@src/vouch/server.py` around lines 759 - 776, The kb_approve exception
handling currently converts DeadClaimRefsError into a generic ValueError. Update
kb_approve to catch and propagate DeadClaimRefsError separately, while
preserving the existing ValueError conversion for ArtifactNotFoundError,
ValueError, and other ProposalError cases, so clients can receive its structured
error code and retry guidance.
Source: Path instructions
| // Wipe dead refs is KB-wide (pages + pending page proposals), so it is | ||
| // scoped to a single project like compile. Two-step: a dry-run previews | ||
| // what would be stripped, the confirm click applies it. | ||
| const canWipe = !aggregated && !!conn && hasMethod('kb.wipe_dead_refs') | ||
| const wipeDeadRefs = useMutation({ | ||
| mutationFn: (vars: { dryRun: boolean }) => | ||
| rpc<DeadRefsReport>(conn!, 'kb.wipe_dead_refs', { dry_run: vars.dryRun }), | ||
| onError: decisionFailed, | ||
| onSuccess: (res) => { | ||
| if (res.dry_run) { | ||
| if (res.dropped === 0) toast('info', 'No dead claim references found') | ||
| else setWipePreview(res) | ||
| return | ||
| } | ||
| setWipePreview(null) | ||
| toast( | ||
| 'success', | ||
| `Stripped ${res.dropped} dead reference(s) from ` + | ||
| `${Object.keys(res.pages).length} page(s) and ` + | ||
| `${Object.keys(res.proposals).length} pending proposal(s)`, | ||
| ) | ||
| afterDecision() | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
wipeDeadRefs errors are misattributed to whatever proposal happens to be selected.
onError: decisionFailed sets the shared decisionError state, which is rendered exclusively inside the selected proposal's detail pane (lines 695-728). kb.wipe_dead_refs is a KB-wide action, unrelated to any single selected proposal. If a reviewer has a proposal open and the wipe call fails (network blip, RPC error, etc.), the detail pane for that unrelated proposal will show an error card, implying it has a problem it doesn't have.
The toast still fires for non-dead_claim_refs codes, so this isn't a silent failure — but the misattributed card is confusing and incorrect. Give this mutation its own error state instead of reusing decisionError.
🐛 Proposed fix: separate error state for the wipe action
const [deadRefRows, setDeadRefRows] = useState<Row[]>([])
const [wipePreview, setWipePreview] = useState<DeadRefsReport | null>(null)
+ const [wipeError, setWipeError] = useState<{ code?: string; message: string } | null>(null) const wipeDeadRefs = useMutation({
mutationFn: (vars: { dryRun: boolean }) =>
rpc<DeadRefsReport>(conn!, 'kb.wipe_dead_refs', { dry_run: vars.dryRun }),
- onError: decisionFailed,
+ onError: (err) => {
+ const code = err instanceof VouchRpcError ? err.code : undefined
+ const message = err instanceof Error ? err.message : String(err)
+ toast('error', code ? `${code}: ${message}` : message)
+ setWipeError({ code, message })
+ },
onSuccess: (res) => {
+ setWipeError(null)
if (res.dry_run) {Render wipeError inside wipeBar itself rather than the detail pane.
🤖 Prompt for 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.
In `@webapp/src/views/PendingView.tsx` around lines 304 - 327, Give the KB-wide
wipeDeadRefs mutation its own wipeError state instead of using decisionFailed
and the shared decisionError; set it from the mutation’s onError while
preserving the existing toast behavior. Render wipeError within the wipeBar
action area, and keep proposal-specific decisionError rendering confined to the
selected proposal detail pane.
1 participant
a page proposal citing claims that were archived, redacted, or bulk-cleared could neither be approved (
put_pagerefuses unknown claim ids) nor repaired without hand-editing the payload. the reviewer hit a bareinvalid_requestin the console and had no next step.approval now treats a dead claim reference as a reviewer decision instead of a dead end.
proposals.approve()raises a typedDeadClaimRefsError— surfaced over jsonl as error codedead_claim_refs— and acceptsdrop_missing_claimsto strip the dead frontmatter refs and inline[claim: …]markers, approving what remains. the dropped ids are recorded in the approve audit event, so the trail shows exactly what was removed and by whom.a new
kb.wipe_dead_refsmethod does the same sweep kb-wide: durable pages and pending page proposals both lose refs to claim ids that resolve to nothing, in one audited bulk event. archived claims still count as alive — their files exist — so only truly unresolvable ids are stripped. registered in all four surfaces: mcp tool, jsonl handler, capabilities entry, and avouch wipe-dead-refscli command (dry-run preview + confirm prompt, mirroringclaims-clear). the hot-memory coverage list marks it excluded as a lifecycle write.the console wires both paths in: the
dead_claim_refserror renders as a strip-and-approve decision card instead of an error toast, batch approve collects dead-ref refusals into a one-click retry bar, and the review queue gets a "wipe dead claim refs" button that dry-runs first and only writes on the confirm click. the clivouch approvegains the same interactive prompt plus a--drop-missing-claimsflag for scripting.tests:
tests/test_dead_claim_refs.pycovers the refusal, the strip-and-audit path, archived-claims-are-alive, the kb-wide wipe (dry-run, apply, idempotence), and the jsonl error code;PendingView.deadrefs.test.tsxcovers the decision card retry and the two-step wipe button. full pytest, mypy, ruff, and the webapp vitest suite are green.Summary by CodeRabbit
New Features
Bug Fixes