Fix ledger retry equality and checkpoint prefix validation

Author: mchwang

docs/implementation/persistent-review-store.md

@@ -29,3 +29,7 @@ Baseline: 136 tests. Final counts and CI evidence are recorded in the PR.
 The store does not read Git or execute agents. The runner remains responsible for obtaining immutable typed tree entries and actual filesystem identity; keeping metadata stable; auditing paths, links, and occupancy; reconciling execution prefixes; and phase/container enforcement (#6). Checkpoint persistence is not an implemented execution state machine. This slice adds neither UI nor an HTTP endpoint. No migration against a shared environment is performed.
 
 Build the review screen under #3 next. The real-issue assignment, experiment protocol, planted-change script, and paired go/no-go experiment remain prerequisites to proceeding beyond that screen.
+
+## Review round 1
+
+Copilot's summary identified two findings (no inline threads). Both were reproduced with failing regressions, then fixed: immutable ledger comparisons now compare the typed fields rather than JSON property order, and a checkpoint's item must be the last item in its completed prefix. No findings were declined. Final suite: 151 tests.

runner/store.ts

@@ -162,7 +162,9 @@ export class Store {
     if ((entry.origin === 'foreign' && entry.owner !== null) || (entry.origin === 'owned' && !entry.owner) || !['foreign', 'owned'].includes(entry.origin)) throw new Error('Invalid ledger ownership.');
     const existing = this.#get('SELECT data FROM ledger WHERE key=? AND sha=?', key, entry.sha);
     if (existing) {
-      if (existing.data !== encode(entry)) throw new Error('Cannot overwrite immutable ledger ownership.');
+      const prior = decode<LedgerEntry>(existing.data);
+      if (prior.sha !== entry.sha || prior.owner !== entry.owner || prior.origin !== entry.origin || prior.sourceSha !== entry.sourceSha)
+        throw new Error('Cannot overwrite immutable ledger ownership.');
       return;
     }
     this.#run('INSERT INTO ledger VALUES (?,?,?)', key, entry.sha, encode(entry));
@@ -237,7 +239,7 @@ export class Store {
     return this.#transaction(() => {
       this.#expect(key, expected);
       const plan = this.getPlan(identity), ids = plan.items.map(item => item.id);
-      if (!ids.includes(evidence.item) || !evidence.completedItems.includes(evidence.item) || new Set(evidence.completedItems).size !== evidence.completedItems.length || evidence.completedItems.some((item, i) => item !== ids[i])) throw new Error('Checkpoint must describe the executed plan prefix.');
+      if (!ids.includes(evidence.item) || evidence.completedItems.at(-1) !== evidence.item || new Set(evidence.completedItems).size !== evidence.completedItems.length || evidence.completedItems.some((item, i) => item !== ids[i])) throw new Error('Checkpoint must describe the executed plan prefix.');
       const checkpoint = { ...evidence, ...expected, id: randomUUID() };
       this.#run('INSERT INTO checkpoints VALUES (?,?,?)', key, checkpoint.id, encode(checkpoint)); return checkpoint;
     });

test/store.test.ts

@@ -164,3 +164,15 @@ it('keeps identical commit SHAs and local item IDs isolated across plan identiti
   expect(store.ownership(other).has(oid(2))).toBe(false);
   expect(() => store.getSnapshot(other, store.getSnapshot(identity).id)).toThrow(/Unknown/);
 });
+it('accepts idempotent ledger retries regardless of object property order', () => {
+  const { store } = fixture();
+  store.recordHistory(identity, state(store), oid(1), oid(2), [{ sha: oid(2), owner: 'P1', origin: 'owned', sourceSha: null }]);
+  expect(() => store.recordHistory(identity, state(store), oid(1), oid(2), [{ sourceSha: null, origin: 'owned', owner: 'P1', sha: oid(2) }])).not.toThrow();
+  expect(store.getLedger(identity)).toHaveLength(1);
+});
+it('requires the checkpoint item to be the last item in the completed prefix', () => {
+  const { store } = fixture(); const next = plan(); next.items.push({ ...structuredClone(next.items[0]!), id: 'P2' });
+  store.importRevision(JSON.stringify(next), 'json', context, 1);
+  expect(() => store.recordCheckpoint(identity, state(store), { item: 'P1', completedItems: ['P1', 'P2'], outOfScopePaths: ['outside'], baseEntries: context.baseEntries })).toThrow(/prefix/);
+  expect(store.recordCheckpoint(identity, state(store), { item: 'P2', completedItems: ['P1', 'P2'], outOfScopePaths: ['outside'], baseEntries: context.baseEntries }).item).toBe('P2');
+});