Preserve review safety across item removal and identity mappings

Author: mchwang

docs/implementation/persistent-review-store.md

@@ -47,3 +47,7 @@ Declined the duplicate-source-mapping finding as a correctness issue: the rewrit
 ## Review round 4
 
 Reproduced a saved assignment throwing in `applyChoices` after its target item was removed by amendment. Revision commits now discard assignments whose targets no longer exist, in the same transaction as the revision and request invalidation. Unrelated assignments and standalone acceptances remain. Removed assignments cannot reactivate if an ID is later reintroduced. No finding declined this round. Final suite: 155 tests.
+
+## Review round 5
+
+Reproduced and fixed three cases: old approvals reviving after item removal/reintroduction; an unknown identity-mapped SHA remaining unrecorded and later claimable; and the linking engine rejecting historical owners absent from the selected plan revision. Revision commits now discard approvals for removed IDs. Missing identity-map sources receive immutable foreign/null-owner ledger entries. `ownership(identity, revision)` produces a conservative view for that revision, mapping absent owners to null without altering the historical ledger; pass the same revision as the plan supplied to the linking engine. `getLedger` remains the raw provenance record. No findings declined this round. Final suite: 157 tests.

runner/store.ts

@@ -80,6 +80,9 @@ export class Store {
     this.#run('INSERT INTO revisions VALUES (?,?,?)', key, plan.revision, encode(plan));
     this.#run("UPDATE requests SET state='invalidated' WHERE key=? AND state IN ('pending','ready')", key);
     const items = new Set(plan.items.map(item => item.id));
+    for (const row of this.#db.prepare('SELECT item FROM approvals WHERE key=?').all(key)) {
+      if (!items.has(row.item as string)) this.#run('DELETE FROM approvals WHERE key=? AND item=?', key, row.item!);
+    }
     for (const row of this.#db.prepare('SELECT choice_key,data FROM choices WHERE key=?').all(key)) {
       const choice = decode<SegmentChoice>(row.data);
       if (choice.action === 'assign' && !items.has(choice.item!))
@@ -193,8 +196,10 @@ export class Store {
     const key = identityKey(identity); this.#current(key);
     return this.#db.prepare('SELECT data FROM ledger WHERE key=? ORDER BY sha').all(key).map(row => decode<LedgerEntry>(row.data));
   }
-  ownership(identity: PlanIdentity): ReadonlyMap<string, string | null> {
-    return new Map(this.getLedger(identity).map(entry => [entry.sha, entry.owner]));
+  /** Link a selected revision conservatively; the raw ledger retains historical owners. */
+  ownership(identity: PlanIdentity, revision?: number): ReadonlyMap<string, string | null> {
+    const items = new Set(this.getPlan(identity, revision).items.map(item => item.id));
+    return new Map(this.getLedger(identity).map(entry => [entry.sha, entry.owner !== null && items.has(entry.owner) ? entry.owner : null]));
   }
   recordRebase(identity: PlanIdentity, expected: ReviewState, base: string, head: string, mappings: readonly { oldSha: string; newSha: string }[]): Snapshot {
     const key = identityKey(identity);
@@ -209,6 +214,7 @@ export class Store {
         destinations.add(newSha);
         const source = ledger.get(oldSha);
         if (oldSha !== newSha) this.#entry(key, { sha: newSha, owner: source?.owner ?? null, origin: source?.origin ?? 'foreign', sourceSha: oldSha });
+        else if (!source) this.#entry(key, { sha: newSha, owner: null, origin: 'foreign', sourceSha: null });
         this.#run('INSERT INTO rewrites VALUES (?,?,?,?)', key, snapshot.id, oldSha, newSha);
       }
       return snapshot;

test/store.test.ts

@@ -100,6 +100,9 @@ it('feeds persisted remapped ownership into the linking engine on real Git histo
   store.recordRebase(identity, state(store), base, head, [{ oldSha: oid(9), newSha: head }]);
   const segments = linkHistory(store.getPlan(identity), readHistory(dir, base, head), store.ownership(identity), p => p);
   expect(segments.length).toBeGreaterThan(0); expect(segments.every(s => s.row === 'P1')).toBe(true);
+  const amended = plan(); amended.items[0]!.id = 'P2'; store.importRevision(JSON.stringify(amended), 'json', context, 1);
+  const afterRemoval = linkHistory(store.getPlan(identity), readHistory(dir, base, head), store.ownership(identity), p => p);
+  expect(afterRemoval.every(s => s.row === 'Unplanned')).toBe(true);
 });
 it('retains typed file-card approvals/choices while fingerprints detect later metadata and plan changes', () => {
   const { store, path } = fixture(); const current = store.getPlan(identity);
@@ -183,7 +186,7 @@ it('allows historical ledger retries after the owning item is removed, but rejec
   expect(() => store.recordHistory(identity, state(store), oid(1), oid(2), [entry])).not.toThrow();
   expect(() => store.recordHistory(identity, state(store), oid(1), oid(3), [{ ...entry, sha: oid(3) }])).toThrow(/Unknown ledger owner/);
   store.recordRebase(identity, state(store), oid(4), oid(5), [{ oldSha: oid(2), newSha: oid(5) }]);
-  expect(store.ownership(identity).get(oid(5))).toBe('P1');
+  expect(store.getLedger(identity).find(entry => entry.sha === oid(5))!.owner).toBe('P1');
 });
 it('allows a later amended revision to receive a fresh continuation approval', () => {
   const { store } = fixture(); const checkpoint = store.recordCheckpoint(identity, state(store), { item: 'P1', completedItems: ['P1'], outOfScopePaths: ['outside'], baseEntries: context.baseEntries });
@@ -210,3 +213,14 @@ it('expires assignments to removed plan items atomically with the amendment', ()
   store.importRevision(JSON.stringify(plan()), 'json', context, 2);
   expect(store.getReview(identity).choices).toEqual([]);
 });
+it('does not revive an old approval when a removed item ID is reintroduced', () => {
+  const { store } = fixture(); store.saveReview(identity, state(store), [approveItem(store.getPlan(identity), [], 'P1', identity, true)], []);
+  const next = plan(); next.items[0]!.id = 'P2'; store.importRevision(JSON.stringify(next), 'json', context, 1);
+  store.importRevision(JSON.stringify(plan()), 'json', context, 2);
+  expect(approvalStates(store.getPlan(identity), [], store.getReview(identity).approvals, identity).P1).toBe('unreviewed');
+});
+it('persists foreign ownership for an unknown SHA mapped to itself', () => {
+  const { store } = fixture(); store.recordRebase(identity, state(store), oid(1), oid(2), [{ oldSha: oid(2), newSha: oid(2) }]);
+  expect(store.getLedger(identity)).toEqual([{ sha: oid(2), owner: null, origin: 'foreign', sourceSha: null }]);
+  expect(() => store.recordHistory(identity, state(store), oid(1), oid(2), [{ sha: oid(2), owner: 'P1', origin: 'owned', sourceSha: null }])).toThrow(/immutable/);
+});