diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b0113b4..930b7e93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,14 @@ 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 `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 a15abcc7..c9a5ca71 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,24 @@ def _apply_cascade_claim( return True +def _apply_cascade_goal( + store: KBStore, step: dict[str, Any], step_id: str, *, actor: str +) -> bool: + # 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 + ) + + def _apply_cascade( store: KBStore, steps: list[dict[str, Any]], *, actor: str ) -> list[str]: @@ -1523,6 +1557,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..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. Only `lifecycle.set_goal_status` calls this.""" + """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_cascade_delete.py b/tests/test_cascade_delete.py index b24e1817..6d6a9103 100644 --- a/tests/test_cascade_delete.py +++ b/tests/test_cascade_delete.py @@ -112,6 +112,37 @@ 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_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") @@ -344,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"}], 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") 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(