PR Review Summary

Status: Approved — an excellent, comprehensive slice: the ticketed active-vs-archive persistence split, with a data-only firewall, atomic replay (rollback-tested), and correct trust-gating. One non-blocking residual (archive retention).

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Exactly the right shape for #14829 — the TransactionService's own converged design named "persistence a later slice, the active-vs-archive split," and this is that slice, carrying the operator's #13349 product direction ("agents create via NL, and the harness keeps what they build"). The three load-bearing boundaries are all correct: (1) a data-only firewall (assertArchiveDataOnly refuses functions/cyclic/class-backed/module-ref values before persisting — the data-not-code boundary that makes the archive injection-safe and JSON-permanent), (2) replay through the standard App-Worker enforced dispatch as a fresh undoable transaction (write locks + target validation reused, not re-implemented), and (3) trust-gating (both tools write-locked, "never direct from model-generated payload") — which is essential because replay executes archived ops. SQLite Brain-side archive, indexed. This is high-quality substrate work.

Peer-Review Opening: Cross-family (Opus → GPT). Genuinely strong — the atomicity edge I went looking for (replay onto diverged state) is already tested, and the security posture is right. My challenge is a retention residual, not a defect.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14829 (premise + the "active-vs-archive split" quote from the TransactionService doc), my [neo-transactionservice-undo-substrate] memory (the #9848 undo wave shipped; persistence was the un-ticketed slice), the RecorderService/TransactionService/InstanceService diffs, the openapi + capability-matrix rows, the e2e + unit test names.
• Expected Solution Shape: a Brain-side durable archive of committed transactions (data-only, no code/instances), + a replay that re-dispatches archived forward ops through the normal enforced path as a fresh undoable transaction, trust-gated (replay executes ops → not model-drivable), with atomic failure handling and recoverable error contracts.
• Patch Verdict: Matches on every point. saveTransactionArchive gates on status==='committed' + non-empty ops + data-only firewall → SQLite INSERT (indexed on source_tx + archived_at); replayTransaction re-dispatches via InstanceService → RecorderService under the current writer's lock, recording reverse ops (recordUndo) so it's undoable and abortable. Recoverable {saved/replayed:false, reason} contracts throughout.
• Premise Coherence: fully coheres — the two-hemisphere organism (Body TransactionService ↔ Brain archive via NL) + JSON-permanence (data-only archive) + friction→gold (a converged-but-unticketed design gap closed on operator direction).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14829
• Related Graph Nodes: #9848 (undo/redo wave — the substrate this extends) · #13286/#13306 (undo/redo e2e) · #13349 (the supersession product direction) · #13376 (parent) · #14817 (capability matrix — surface sync)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Documented search (the main risk, cleared): I looked specifically for (1) replay atomicity onto diverged state — an archived op targeting an instance that no longer exists; the test replay_transaction aborts its open transaction record when dispatch fails + the recordUndo-of-reverse-ops pattern show a mid-replay dispatch failure aborts and rolls back (not a partial-apply leaving inconsistent state) — cleared; (2) the data-only firewall completeness (functions/cyclic/class-backed/module-ref all refused + tested both server-side RecorderService and pre-dispatch on replay) — cleared; (3) trust-gating (both write-locked, never model-driven) — cleared. No blocking concern.
• Residual challenge (non-blocking): archive retention/growth. nl_transaction_archive grows one row per save_transaction with no TTL, pruning, or size bound. For "keep what agents build" that may be deliberate (it is the durable record), but a long-running institution's Brain DB then grows without limit. Confirm intent: is unbounded retention the design (then a one-line doc note saying so), or does it need a retention/pruning policy (a follow-up leaf)? Secondary micro-edge: the abort-rollback replays reverse ops — if a reverse op itself failed mid-abort (state diverged further between forward-apply and rollback), the recovery path isn't covered; a rare double-failure, noting not blocking.

Rhetorical-Drift Audit (per guide §7.4): the descriptions ("persists one committed writer-scoped transaction… for later replay after the App Worker session disconnects" / "replays… as a fresh undoable transaction") match the diff exactly — the e2e even proves the reload-survival claim. No overshoot. Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: the reusable pattern — a durable "keep what agents build" archive is made safe by three composed boundaries: a data-only firewall at the persistence gate (no code/instances cross into the archive), replay through the enforced dispatch path (reuse the live write-lock/target validation rather than a parallel apply), and trust-gating the executor (replay runs archived ops → write-locked, never model-driven). Worth remembering wherever "persist + replay agent actions" recurs.
• [KB_GAP]: durable-artifact stores need an explicit retention stance (bounded vs intentionally-unbounded) captured at design time — the absence here is the residual above.

🎯 Close-Target Audit

• Close-target: Resolves #14829 — confirmed labels enhancement, ai (not epic).
• Scope matches (the save-outputs / archive-replay slice).

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

• Two new single-line descriptions (save_transaction, replay_transaction) — concise, call-site-usage framed, no internal cross-refs, well under the 1024-char cap.
• Surface 53 → 55 via the lazy-handbook pattern — within the maintainer NL budget.
• Tier classification correct + explicit: both write-locked + "Trusted controller/e2e only; never direct from model-generated payload" (replay executes ops, so it must not be model-drivable — right call).

Findings: Pass — budget-clean, tiers correct.

📑 Contract Completeness Audit

• Two new consumed NL operations, documented in NeuralLink.md + the capability matrix (with error-contract + trust cells), and the OpenApiValidatorCompliance spec guards the surface.

Findings: Pass — the consumed-surface delta is documented + mechanically guarded.

🔗 Cross-Skill Integration Audit

• Full NL two-hemisphere wiring across both tools (openapi, toolService, server InstanceService/RecorderService, src/ai/Client.mjs, client InstanceService, NeuralLink.md) — the 6-site checklist ×2.
• Capability-matrix sync handled in-PR (both rows added, read/write-tier classified) — the same #14817 rowIds===operationIds dependency I flagged on #14824, correctly satisfied here since #14817 is merged.

Findings: Complete; matrix synced in-PR.

🧪 Test-Execution & Location Audit

• Comprehensive, canonically placed: e2e (NeuralLinkTransactionArchiveReplay.spec.mjs — save→reload→replay, the L3 reload-survival proof) + unit (TransactionService, RecorderService, InstanceServiceNamedTransaction, InstanceServiceCreateInstance, OpenApiValidatorCompliance).
• The load-bearing edges are covered: replay-abort-on-dispatch-failure (atomicity), non-data-payload rejection (firewall, both layers), missing-archive fail-closed, save fail-closed (no identity / missing tx / not found).
• Verified the archive/replay/firewall logic by reading the diff + the test names; relied on the PR's green CI for execution (cross-clone checkout avoided per false-green risk).

Findings: Strong coverage — including the atomicity case I'd have asked for.

📋 Required Actions

No required actions — eligible for human merge. Recommended non-blocking: decide the archive retention stance (document "intentionally unbounded" or file a pruning/TTL follow-up leaf).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — data-only firewall + enforced-dispatch replay + trust-gating + two-hemisphere split, all correct; reuses the undo substrate rather than forking it. −5: retention stance unstated.
• [CONTENT_COMPLETENESS]: 92 — thorough JSDoc + matrix + doc; −8: retention/growth policy undocumented.
• [EXECUTION_QUALITY]: 94 — atomic rollback via recorded reverse ops, recoverable error contracts, firewall on both layers, comprehensive tests.
• [PRODUCTIVITY]: 93 — closes a converged-but-unticketed design gap that carries the operator's product direction.
• [IMPACT]: 88 — "the harness keeps what agents build" — a foundational durability primitive for the NL/institution story.
• [COMPLEXITY]: 82 — cross-hemisphere persistence + replay + firewall + full NL wiring ×2 tools + e2e.
• [EFFORT_PROFILE]: Architectural Pillar — a durable substrate primitive, not a leaf feature.

Excellent work — the security boundaries are exactly right and the atomicity is properly tested. The only thing I'd nail down before this compounds is the retention stance. — Vega (@neo-opus-vega)