From e46eb11a7bb4d35605e823000971b8351e431583 Mon Sep 17 00:00:00 2001 From: kurosawareiji7007-hub Date: Fri, 31 Jul 2026 10:09:05 -0700 Subject: [PATCH 1/3] fix(delete): include goals in referenced_by and cascade_plan Goals cite claims/entities but the delete gate never walked list_goals, so deleting a cited claim left dangling refs that crashed set_goal_status. Block delete when a goal cites the target, plan goal unlinks for cascade, and apply them via goal.cascade_unlink. Fixes #727 --- CHANGELOG.md | 7 +++++ src/vouch/proposals.py | 52 ++++++++++++++++++++++++++++++++---- src/vouch/storage.py | 2 +- tests/test_cascade_delete.py | 21 +++++++++++++++ tests/test_delete.py | 23 ++++++++++++++++ 5 files changed, 99 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b0113b4..a047dc01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,13 @@ All notable changes to vouch are documented here. Format follows artifact the caller could not already retrieve, and it touches no write path. ### Fixed +- **delete gate / cascade now see goals that cite the target** (#727): + `referenced_by` and `cascade_plan` walked pages, claims, and relations + but never goals, even though `Goal.claims` / `Goal.entities` are + validated refs. Deleting a cited claim succeeded (empty gate), then + `set_goal_status` crashed with `ValueError` from `_validate_goal_refs`. + Goals now block delete, appear in the cascade plan, and are unlinked + via `goal.cascade_unlink` on approve. - **`extract` no longer fractures file paths/URLs into auto-approved garbage claims** (#702): the sentence segmenter only skipped a `.` as a boundary when it was flanked by digits on both sides (decimals/versions diff --git a/src/vouch/proposals.py b/src/vouch/proposals.py index a15abcc7..46fbba29 100644 --- a/src/vouch/proposals.py +++ b/src/vouch/proposals.py @@ -1318,6 +1318,9 @@ def referenced_by(store: KBStore, target_kind: str, target_id: str) -> list[str] for page in store.list_pages(): if target_id in page.claims: refs.append(f"page {page.id!r}") + for goal in store.list_goals(): + if target_id in goal.claims: + refs.append(f"goal {goal.id!r}") for rel in store.list_relations(): if _relation_refers_to(store, rel, target_kind, target_id): refs.append(f"relation {rel.id!r}") @@ -1341,6 +1344,9 @@ def referenced_by(store: KBStore, target_kind: str, target_id: str) -> list[str] for page in store.list_pages(): if target_id in page.entities: refs.append(f"page {page.id!r}") + for goal in store.list_goals(): + if target_id in goal.entities: + refs.append(f"goal {goal.id!r}") for rel in store.list_relations(): if _relation_refers_to(store, rel, target_kind, target_id): refs.append(f"relation {rel.id!r}") @@ -1366,11 +1372,11 @@ def cascade_plan( One step per referring artifact: a page citing the target in both its frontmatter and its body is one decision, not two. - Pages and claims lose their pointer; relations are deleted outright, - because an edge whose endpoint is gone has no meaning. Relations carry - no inbound refs of their own (`referenced_by` returns [] for them), so - the walk is one level deep by construction — there is no transitive - cascade to bound. + Pages, claims, and goals lose their pointer; relations are deleted + outright, because an edge whose endpoint is gone has no meaning. + Relations carry no inbound refs of their own (`referenced_by` returns + [] for them), so the walk is one level deep by construction — there is + no transitive cascade to bound. """ if target_kind not in _DELETE_KINDS: raise ProposalError( @@ -1384,6 +1390,11 @@ def cascade_plan( steps.append( {"kind": "page", "id": page.id, "unlink_claims": [target_id]} ) + for goal in store.list_goals(): + if target_id in goal.claims: + steps.append( + {"kind": "goal", "id": goal.id, "unlink_claims": [target_id]} + ) steps.extend(_relation_cascade_steps(store, target_id)) for claim in store.list_claims(): if claim.id == target_id: @@ -1410,6 +1421,11 @@ def cascade_plan( steps.append( {"kind": "page", "id": page.id, "unlink_entities": [target_id]} ) + for goal in store.list_goals(): + if target_id in goal.entities: + steps.append( + {"kind": "goal", "id": goal.id, "unlink_entities": [target_id]} + ) steps.extend(_relation_cascade_steps(store, target_id)) return steps @@ -1487,6 +1503,29 @@ def _apply_cascade_claim( return True +def _apply_cascade_goal( + store: KBStore, step: dict[str, Any], step_id: str, *, actor: str +) -> bool: + try: + goal = store.get_goal(step_id) + except ArtifactNotFoundError: + return False + claims = [c for c in step.get("unlink_claims") or [] if c in goal.claims] + entities = [e for e in step.get("unlink_entities") or [] if e in goal.entities] + if not claims and not entities: + return False + goal.claims = [c for c in goal.claims if c not in claims] + goal.entities = [e for e in goal.entities if e not in entities] + goal.updated_at = datetime.now(UTC) + store.update_goal(goal) + audit.log_event( + store.kb_dir, event="goal.cascade_unlink", actor=actor, + object_ids=[goal.id], data={"claims": claims, "entities": entities}, + reversible=False, + ) + return True + + def _apply_cascade( store: KBStore, steps: list[dict[str, Any]], *, actor: str ) -> list[str]: @@ -1523,6 +1562,9 @@ def _apply_cascade( elif kind == "claim": if not _apply_cascade_claim(store, step, step_id, actor=actor): continue + elif kind == "goal": + if not _apply_cascade_goal(store, step, step_id, actor=actor): + continue else: continue changed.append(step_id) diff --git a/src/vouch/storage.py b/src/vouch/storage.py index 892b726f..23e510f1 100644 --- a/src/vouch/storage.py +++ b/src/vouch/storage.py @@ -1141,7 +1141,7 @@ def put_goal(self, goal: Goal) -> Goal: return goal def update_goal(self, goal: Goal) -> Goal: - """Persist a mutated goal. Only `lifecycle.set_goal_status` calls this.""" + """Persist a mutated goal (status moves and cascade unlinks).""" if not self._goal_path(goal.id).exists(): raise ArtifactNotFoundError(f"goal {goal.id}") # Round-trip so in-place mutation can't skip the model validators diff --git a/tests/test_cascade_delete.py b/tests/test_cascade_delete.py index b24e1817..a6158a26 100644 --- a/tests/test_cascade_delete.py +++ b/tests/test_cascade_delete.py @@ -112,6 +112,27 @@ def test_plan_mirrors_referenced_by_for_a_page_cited_claim(store: KBStore) -> No ] +def test_plan_and_apply_unlink_goal_cited_claims(store: KBStore) -> None: + """Goals are first-class claim referrers after #427 — cascade must free them.""" + from vouch.models import Goal + + _claim(store, "c1") + store.put_goal(Goal(id="keep-c1", title="keep c1", claims=["c1"])) + assert referenced_by(store, "claim", "c1") == ["goal 'keep-c1'"] + assert cascade_plan(store, "claim", "c1") == [ + {"kind": "goal", "id": "keep-c1", "unlink_claims": ["c1"]} + ] + pr = propose_delete( + store, target_kind="claim", target_id="c1", proposed_by="agent", + cascade=True, + ) + approve(store, pr.id, approved_by="reviewer") + assert store.get_goal("keep-c1").claims == [] + with pytest.raises(ArtifactNotFoundError): + store.get_claim("c1") + assert "goal.cascade_unlink" in _events(store) + + def test_plan_deletes_relations_and_unlinks_pages(store: KBStore) -> None: _claim(store, "c1") _claim(store, "c2") diff --git a/tests/test_delete.py b/tests/test_delete.py index 838193b5..c11ff78f 100644 --- a/tests/test_delete.py +++ b/tests/test_delete.py @@ -200,6 +200,29 @@ def test_unreferenced_claim_is_deletable(store: KBStore) -> None: assert referenced_by(store, "claim", "lonely") == [] +def test_claim_referenced_by_goal(store: KBStore) -> None: + """Goals cite claims; the delete gate must see that (#727).""" + from vouch.models import Goal + + _claim(store, "c1") + store.put_goal(Goal(id="keep-c1", title="keep c1 live", claims=["c1"])) + refs = referenced_by(store, "claim", "c1") + assert refs == ["goal 'keep-c1'"] + with pytest.raises(ProposalError, match="referenced"): + propose_delete( + store, target_kind="claim", target_id="c1", proposed_by="a", + ) + + +def test_entity_referenced_by_goal(store: KBStore) -> None: + from vouch.models import Goal + + store.put_entity(Entity(id="e1", name="E", type=EntityType.CONCEPT)) + store.put_goal(Goal(id="track-e1", title="track e1", entities=["e1"])) + refs = referenced_by(store, "entity", "e1") + assert refs == ["goal 'track-e1'"] + + def test_entity_referenced_by_claim(store: KBStore) -> None: store.put_entity(Entity(id="e1", name="E", type=EntityType.CONCEPT)) src = store.put_source(b"s") From 78795f42e83b545ef7a9b8d3b6bec8b432c21f5e Mon Sep 17 00:00:00 2001 From: kurosawareiji7007-hub Date: Fri, 31 Jul 2026 10:34:55 -0700 Subject: [PATCH 2/3] fix(delete): route goal cascade unlink through lifecycle test_only_lifecycle_mutates_a_stored_goal forbids proposals calling update_goal. Move cascade claim/entity unlinks into lifecycle.cascade_unlink_goal_refs so the single-writer + audit invariant holds. Fixes #727 --- CHANGELOG.md | 3 ++- src/vouch/lifecycle.py | 44 ++++++++++++++++++++++++++++++++++++++++-- src/vouch/proposals.py | 29 ++++++++++++---------------- src/vouch/storage.py | 2 +- tests/test_goals.py | 9 +++++---- 5 files changed, 62 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a047dc01..930b7e93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -161,7 +161,8 @@ All notable changes to vouch are documented here. Format follows validated refs. Deleting a cited claim succeeded (empty gate), then `set_goal_status` crashed with `ValueError` from `_validate_goal_refs`. Goals now block delete, appear in the cascade plan, and are unlinked - via `goal.cascade_unlink` on approve. + via `lifecycle.cascade_unlink_goal_refs` (`goal.cascade_unlink` audit) + on approve — the same single-writer path as status moves. - **`extract` no longer fractures file paths/URLs into auto-approved garbage claims** (#702): the sentence segmenter only skipped a `.` as a boundary when it was flanked by digits on both sides (decimals/versions diff --git a/src/vouch/lifecycle.py b/src/vouch/lifecycle.py index d6583997..43dfbb29 100644 --- a/src/vouch/lifecycle.py +++ b/src/vouch/lifecycle.py @@ -176,6 +176,45 @@ def confirm(store: KBStore, *, claim_id: str, actor: str) -> Claim: _CLOSED_GOAL_STATUSES = frozenset({GoalStatus.DONE, GoalStatus.ABANDONED}) +def cascade_unlink_goal_refs( + store: KBStore, + goal_id: str, + *, + unlink_claims: list[str] | None = None, + unlink_entities: list[str] | None = None, + actor: str, +) -> Goal | None: + """Drop claim/entity pointers from a goal during cascade delete. + + Companion to ``set_goal_status``: both are the only callers of + ``store.update_goal``. Pointer edits are not status moves, but they still + mutate durable goal yaml and must append their own audit event + (``goal.cascade_unlink``) rather than calling storage from proposals. + Returns ``None`` when the goal is gone or nothing to unlink (idempotent). + """ + try: + goal = store.get_goal(goal_id) + except ArtifactNotFoundError: + return None + claims = [c for c in (unlink_claims or []) if c in goal.claims] + entities = [e for e in (unlink_entities or []) if e in goal.entities] + if not claims and not entities: + return None + goal.claims = [c for c in goal.claims if c not in claims] + goal.entities = [e for e in goal.entities if e not in entities] + goal.updated_at = datetime.now(UTC) + store.update_goal(goal) + audit.log_event( + store.kb_dir, + event="goal.cascade_unlink", + actor=actor, + object_ids=[goal.id], + data={"claims": claims, "entities": entities}, + reversible=False, + ) + return goal + + def set_goal_status( store: KBStore, *, @@ -184,14 +223,15 @@ def set_goal_status( actor: str, reason: str | None = None, ) -> Goal: - """Move an approved goal to a new status. The only goal write path. + """Move an approved goal to a new status. The only status write path. Same posture as `archive` / `confirm` above: a status move is metadata about already-reviewed knowledge, not a new assertion, so it lands directly — but it lands *here*, in one place, so every transition appends a `goal.status` event to the audit log and a row to the goal's own `history`. Nothing else in the codebase may set `Goal.status`; if a future - change needs to, it belongs in this function. + change needs to, it belongs in this function. Claim/entity pointer edits + during cascade delete go through ``cascade_unlink_goal_refs``. """ try: new_status = GoalStatus(status) diff --git a/src/vouch/proposals.py b/src/vouch/proposals.py index 46fbba29..c9a5ca71 100644 --- a/src/vouch/proposals.py +++ b/src/vouch/proposals.py @@ -1506,24 +1506,19 @@ def _apply_cascade_claim( def _apply_cascade_goal( store: KBStore, step: dict[str, Any], step_id: str, *, actor: str ) -> bool: - try: - goal = store.get_goal(step_id) - except ArtifactNotFoundError: - return False - claims = [c for c in step.get("unlink_claims") or [] if c in goal.claims] - entities = [e for e in step.get("unlink_entities") or [] if e in goal.entities] - if not claims and not entities: - return False - goal.claims = [c for c in goal.claims if c not in claims] - goal.entities = [e for e in goal.entities if e not in entities] - goal.updated_at = datetime.now(UTC) - store.update_goal(goal) - audit.log_event( - store.kb_dir, event="goal.cascade_unlink", actor=actor, - object_ids=[goal.id], data={"claims": claims, "entities": entities}, - reversible=False, + # Lazy import: lifecycle imports strip_claim_markers from this module. + from . import lifecycle as life + + return ( + life.cascade_unlink_goal_refs( + store, + step_id, + unlink_claims=list(step.get("unlink_claims") or []), + unlink_entities=list(step.get("unlink_entities") or []), + actor=actor, + ) + is not None ) - return True def _apply_cascade( diff --git a/src/vouch/storage.py b/src/vouch/storage.py index 23e510f1..d51c673a 100644 --- a/src/vouch/storage.py +++ b/src/vouch/storage.py @@ -1141,7 +1141,7 @@ def put_goal(self, goal: Goal) -> Goal: return goal def update_goal(self, goal: Goal) -> Goal: - """Persist a mutated goal (status moves and cascade unlinks).""" + """Persist a mutated goal. Only ``lifecycle`` may call this.""" if not self._goal_path(goal.id).exists(): raise ArtifactNotFoundError(f"goal {goal.id}") # Round-trip so in-place mutation can't skip the model validators diff --git a/tests/test_goals.py b/tests/test_goals.py index f181d17f..1f917e5f 100644 --- a/tests/test_goals.py +++ b/tests/test_goals.py @@ -170,11 +170,12 @@ def test_unknown_status_and_no_op_transition_are_refused(store: KBStore) -> None def test_only_lifecycle_mutates_a_stored_goal() -> None: """`store.update_goal` is the single mutation path, and only - `lifecycle.set_goal_status` may call it. + `lifecycle` may call it (`set_goal_status` and cascade unlink). - A second caller would be a status move that skipped the audit-log append - — exactly the parallel write path the north star forbids. Storage and the - test suite are excluded: one defines the method, the other exercises it. + A second module calling storage directly would be a write that skipped + the audit-log append — exactly the parallel write path the north star + forbids. Storage and the test suite are excluded: one defines the + method, the other exercises it. """ src_dir = Path(life.__file__).parent callers = sorted( From 16f179fa213ab511a7ca428869eabf3492102cde Mon Sep 17 00:00:00 2001 From: kurosawareiji7007-hub Date: Fri, 31 Jul 2026 10:56:56 -0700 Subject: [PATCH 3/3] test(delete): cover goal cascade plan and idempotent unlink hit entity->goal cascade_plan, ArtifactNotFoundError and no-op unlink paths in lifecycle.cascade_unlink_goal_refs so diff-coverage stays 100%. --- tests/test_cascade_delete.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_cascade_delete.py b/tests/test_cascade_delete.py index a6158a26..6d6a9103 100644 --- a/tests/test_cascade_delete.py +++ b/tests/test_cascade_delete.py @@ -133,6 +133,16 @@ def test_plan_and_apply_unlink_goal_cited_claims(store: KBStore) -> None: assert "goal.cascade_unlink" in _events(store) +def test_plan_unlinks_goal_cited_entities(store: KBStore) -> None: + from vouch.models import Goal + + store.put_entity(Entity(id="e1", name="E", type=EntityType.CONCEPT)) + store.put_goal(Goal(id="track-e1", title="track e1", entities=["e1"])) + assert cascade_plan(store, "entity", "e1") == [ + {"kind": "goal", "id": "track-e1", "unlink_entities": ["e1"]} + ] + + def test_plan_deletes_relations_and_unlinks_pages(store: KBStore) -> None: _claim(store, "c1") _claim(store, "c2") @@ -365,6 +375,24 @@ def test_applier_skips_a_claim_already_unlinked(store: KBStore) -> None: assert "claim.cascade_unlink" not in _events(store) +def test_applier_skips_a_goal_that_vanished(store: KBStore) -> None: + assert _apply_cascade( + store, [{"kind": "goal", "id": "gone", "unlink_claims": ["c1"]}], + actor="reviewer", + ) == [] + + +def test_applier_skips_a_goal_already_unlinked(store: KBStore) -> None: + from vouch.models import Goal + + store.put_goal(Goal(id="g1", title="empty refs")) + assert _apply_cascade( + store, [{"kind": "goal", "id": "g1", "unlink_claims": ["c1"]}], + actor="reviewer", + ) == [] + assert "goal.cascade_unlink" not in _events(store) + + def test_applier_skips_a_relation_that_vanished(store: KBStore) -> None: assert _apply_cascade( store, [{"kind": "relation", "id": "gone", "action": "delete"}],