feat(delete): cascade delete — the referrers ride along in the proposal - #719
Merged
Merged
Conversation
vouchdev#686 added coerce_numeric and routed capture.py's two boolean fields through coerce_bool, but left min_observations and dedup_window_seconds on bare int()/float(). a typo'd value raised out of load_config instead of falling back to the default, which is the exact case the helper's own docstring cites (`min_observations: "three"`), and the resulting unused import tripped ruff F401. surfaced by merging test into this branch: the branch-push workflows on test don't run pytest/mypy/ruff, so the gate never ran on the merge that landed it. Co-authored-by: Cursor <cursoragent@cursor.com>
closes vouchdev#600. `referenced_by()` refuses a delete while anything still points at the target. that block is correct, but in a compiled kb it leaves most claims permanently undeletable — pages cite claims in bulk — and a supersede pair is mutually locked at both ends, so no delete ordering can ever remove either half of a chain. the gate is unchanged. what changes is what the reviewer is asked to approve: with cascade=true the required referrer edits are recorded in the proposal payload as a plan, and _approve_delete re-derives that plan at approve time — the same posture as the existing ref re-check — applies it, and only then deletes. the approve-time referenced_by gate still has to come back empty, so the gate is satisfied rather than bypassed. pages and claims lose their pointer, frontmatter and the inline [claim: …] body markers both. relations are deleted outright: an edge whose endpoint is gone has no meaning, and relations carry no inbound refs of their own, so the walk is one level deep by construction and there is no transitive cascade to bound. additive and default-off — omitting cascade reproduces today's behaviour exactly, and the refusal message now names the flag so the dead end is discoverable. Co-authored-by: Cursor <cursoragent@cursor.com>
approve() re-derives the plan, so the applier's artifact-missing and already-unlinked paths cannot be reached through the public flow. they exist for the narrow race where a concurrent writer changes a referrer between derivation and application, and for a crash-retry of approve(). exercised directly against _apply_cascade, which is the only honest way to reach them, and what the 100% diff-coverage gate asks for. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
diff coverage: n/a — this PR changes no python under |
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.
Closes #600
removing an approved claim usually fails with
cannot delete claim <id>: referenced by page '<page>'. the block is right —referenced_by()refuses while anything still points at the target — but in a compiled kb it means most claims are permanently undeletable, because pages cite claims in bulk. and a supersede pair is mutually locked:blistsainsupersedes,a'ssuperseded_bypoints back atb, so no delete ordering removes either half of a chain. the only workaround was hand-editing the page'sclaims:list first, which is exactly the parallel data path the north star forbids.the invariant i most want reviewed
the gate is satisfied, not bypassed. cascade does not lower the bar for approving a delete; it changes what the reviewer is asked to approve.
_apply_cascaderuns before the target is touched, and then the existing approve-timereferenced_byre-check still has to come back empty beforedeleter()is called. if the cascade missed a referrer, the delete refuses exactly as it does today. i deliberately did not add an "ignore refs when cascading" branch — the gate re-check is the thing that makes this safe to review.the one place that genuinely had to change is
_payload_block_reason, which refuses a DELETE proposal whose target is referenced. that runs insideapprove()too, so without it a cascade proposal was unapprovable. it now skips the refusal only when the payload carries a cascade — i.e. only when a reviewer has already seen and approved the referrer edits.the plan is re-derived at approve time, not replayed. same posture as the ref re-check: the kb may have moved since filing. a referrer added after propose is still unlinked (
test_a_referrer_added_after_propose_is_still_unlinked); one removed since is simply absent from the new plan and is not fatal. the payload keeps the plan the reviewer actually saw, as the record of what they approved —test_payload_keeps_the_plan_the_reviewer_sawpins that the two can diverge and that the stored copy does not get rewritten.why relations are deleted and nodes are not
pages and claims lose their pointer; relations are deleted outright. an edge whose endpoint is gone has no meaning, and
referenced_byalready returns[]for a relation — edges carry no inbound refs of their own. that is what bounds this: the walk is one level deep by construction, so there is no transitive cascade and no depth limit to argue about. the far endpoint of a deleted edge is never touched (test_cascade_deletes_the_relations_that_pointed_at_the_targetassertsc2survives).pages lose the frontmatter entry and the inline
[claim: …]body markers, through the samestrip_claim_markershelperwipe_dead_refsuses. dropping only the frontmatter would leave the body rendering a citation whose claim no longer exists, which is the "silently corrupting pages" outcome #600 explicitly rejects.what i did not do
the kind-blind relation match.
referenced_bycompares the claim id against bare relation endpoints, which carry no kind tag, so a same-slug artifact of a different kind can block an unrelated claim. #600 calls this out and says it "could land first" as a smaller separate change. cascade inherits the imprecision — it would delete that same mismatched edge. i left it alone rather than widen this pr into a second behaviour change; it deserves its own diff and its own tests, and it is not made worse here.no console change. #600 also asks for
DeleteArtifactButtonto offer "delete with cascade". that is asrc/vouch/web/change on a surface with its own screenshot gate, and this pr is already the fullkb.*+ cli surface. happy to follow up if you want them in one piece.no new
kb.*method, socapabilities.METHODSis untouched — this is a new optional parameter on an existing method. the four registration sites still applied, andtest_capabilitieswould not have caught a missing cli flag, so the three surfaces are pinned individually.review gate
filing a cascade proposal writes nothing:
test_proposing_a_cascade_writes_nothingpins the page's claim list, its body markers, the target claim, and the audit-event count across apropose_delete(cascade=True). self-approval is still refused on a human-reviewed kb, and the refusal leaves every referrer intact.every cascade edit lands its own irreversible audit event —
page.cascade_unlink,claim.cascade_unlink,relation.delete— and the{kind}.deleteevent names what it touched, so the log answers "what else changed when this was deleted" without a join.tests
32 cases in
tests/test_cascade_delete.py: today's refusal unchanged and the message naming the flag; plan shape mirroringreferenced_byfor every target kind; the supersede pair unlocked from both ends;superseded_bycleared,contradictsunlinked, entity refs dropped from claims and pages; relations deleted for claim/page/entity targets; inline marker stripping; re-derivation both directions; thecheck_approvablesplit (cascade allowed, plain referenced delete still blocked); self-approval; propose-writes-nothing; audit events andreversible=False; and all three surfaces — mcp, jsonl envelope shapes both ways, and the cli in success and clean-error form.verification
one note: this branch also carries
fix(capture): apply coerce_numeric to the numeric config fields, the same one-hunk commit as #693.testis currently red without it — #686 addedcoerce_numeric, routed capture.py's two booleans throughcoerce_bool, and leftmin_observations/dedup_window_secondson bareint()/float(), which failstest_load_config_malformed_numeric_falls_backand trips ruff F401 on the now-unused import. it reachedtestbecause the branch-push workflows there run only the bot jobs — pytest/mypy/ruff arepull_request-only, so the gate never ran on the merge that landed it. whichever of #693 or this lands first makes the other a no-op; the commits are identical so they merge cleanly either way.