PR Review Summary

Status: Request Changes

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The pure-core split is the right shape and the local/CI evidence is strong, but two contract-level gaps block merge: malformed op descriptors can enter or throw inside the stack instead of failing closed, and the implemented API drifts from the parent Contract Ledger that governs Slice-1.

Peer-Review Opening: This is the correct unblocked slice: a pure in-heap undo-stack core before the InstanceService capture hook and undo tool wiring. The implementation follows the WriteGuard pattern well, but the contract surface is too foundational to let malformed record descriptors or ledger drift through.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13230, parent #13221 Contract Ledger, epic #13012 ledger shape, changed-file list, current dev WriteGuard.mjs / LockRegistry sibling pattern, current RecorderService role, ArchitectureOverview, KB grounding for Neural Link / RecorderService, and team memory for Discussion #13216 OQ1/OQ5 convergence.
• Expected Solution Shape: A correct core should be pure and in-heap, with a per-session stack keyed by Neural Link session + requester identity; it should store complete JSON data descriptors for forward and reverse ops, fail closed without throwing on malformed input, keep provenance separate from enforcement identity, and leave live-tree mutation / lock enforcement to #13221 wiring. It must not become the forward audit ledger or bypass WriteGuard.
• Patch Verdict: Partial match. The shape, isolation, caps, single-level undo semantics, and tests align with the intended slice. The patch contradicts the fail-closed descriptor contract in record(), and the shipped contract does not match the inherited Contract Ledger exactly.

Context & Graph Linking

• Target Epic / Issue ID: Resolves #13230
• Related Graph Nodes: #13221, #13216, #9848, #13012, #13167, #13134, #13208, #13226

Depth Floor

Challenge: The core claims "fail-closed throughout" for transaction ops, but record() only validates originWriter, targetSubtreePath, and reverse. It accepts missing sequenceId and label, and it can throw while cloning a missing or cyclic forward descriptor. A stack authority that future write-path wiring calls after a grant should never expose malformed input as an uncaught exception.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift. It says all fail-closed branches are covered, but the malformed-op branch does not cover missing sequenceId, missing forward, missing label, or non-serializable forward.
• Anchor & Echo summaries: drift. TransactionService.mjs line 54 names timedOut as a state-machine terminal, but there is no timeout state or test in this PR.
• [RETROSPECTIVE] tag: none.
• Linked anchors: drift against parent #13221 Contract Ledger; details below.

Findings: Drift flagged in Required Actions.

Graph Ingestion Notes

• [KB_GAP]: KB correctly shows no existing TransactionService; this PR creates the first active undo-stack core. It also confirms RecorderService is forward audit, not undo authority.
• [TOOLING_GAP]: The MCP review helper previously produced unreliable GitHub author attribution, so this review is posted via gh pr review under the verified neo-gpt CLI identity.
• [RETROSPECTIVE]: The in-heap active-stack vs forward-audit-ledger split is the right foundation for Slice-1, but pure cores still need strict data-contract gates because the later live wiring will trust this boundary.

Close-Target Audit

• Close-targets identified: #13230 (Resolves #13230, newline-isolated in the PR body).
• #13230 is labeled enhancement, ai, architecture; it is not epic-labeled.

Findings: Pass.

Contract Completeness Audit

• Parent #13221 contains a Contract Ledger matrix for TransactionService and reverse-record schema.
• Contract drift detected:
  • Parent ledger places TransactionService at ai/services/neural-link/, while this PR ships src/ai/TransactionService.mjs.
  • Parent ledger says the stack is keyed by {neuralLinkSessionId, requesterAgentId, requesterSessionId, txId}, while implementation keys the stack by {neuralLinkSessionId, requesterAgentId, requesterSessionId} and treats txId as the open/committed transaction id.
  • Parent ledger includes timedOut; implementation and tests do not.
  • Parent reverse-record row requires sequenceId, inverse-op descriptor, forward descriptor, targetSubtreePath, and bounded label; record() does not validate all required fields.

Findings: Contract drift flagged in Required Actions.

Evidence Audit

• PR body contains an Evidence: declaration line.
• L2 achieved evidence is appropriate for this pure in-heap core; live capture + undo tool wiring is explicitly left to #13221.
• Evidence-class collapse is accurate: no L3/L4 runtime effect is claimed for this PR.

Findings: Pass for the pure-core scope.

N/A Audits - MCP Tool Descriptions / Turn-Memory Substrate

N/A across listed dimensions: no OpenAPI tool descriptions, MCP tool signatures, skill files, AGENTS substrate, or turn-loaded instruction files are modified.

Provenance Audit

• Conceptual origin: Internal Discussion #13216, with a Signal Ledger in the PR body and parent #13221.
• Reviewer memory check: Team memory confirms the OQ1/OQ5 convergence: origin writer is provenance only, undo enforcement re-enters as the current caller, and enforcement path is re-derived at undo time.
• Findings: Pass. This is internally derived, not an external framework port.

Cross-Skill Integration Audit

• Existing predecessor/follow-up surfaces are named: #13221 owns the capture hook and undo tool wiring.
• No skill or workflow convention needs updating for this pure core.
• Contract Ledger integration needs correction as noted above.

Findings: Integration is otherwise clean; ledger drift is tracked under Required Actions.

Test-Execution & Location Audit

• Exact head checked in detached worktree: 224f1147aea01f4b13dba6c952535925e4ec383d.
• Canonical Location: test/playwright/unit/ai/TransactionService.spec.mjs is correct for a right-hemisphere src/ai core.
• Ran node --check src/ai/TransactionService.mjs — passed.
• Ran npm run test-unit -- test/playwright/unit/ai/TransactionService.spec.mjs — 16 passed.
• Ran targeted malformed-op probes. Results: missing sequenceId returned {ok:true}; missing label returned {ok:true}; cyclic forward threw TypeError: Converting circular structure to JSON.

Findings: Focused tests pass, but coverage misses the malformed-op gap flagged in Required Actions.

Required Actions

To proceed with merging, please address the following:

• Tighten record() malformed-op validation so every required reverse-record field is enforced before cloneOp(): sequenceId, originWriter.agentId, originWriter.sessionId, non-empty targetSubtreePath as appropriate, forward, reverse, and bounded/non-empty label if that is part of the contract. Missing/invalid fields must return a fail-closed result, not store undefined and not throw.
• Add unit coverage for the missing malformed-op cases: missing sequenceId, missing forward, missing label, and non-serializable/cyclic forward. The invariant is that malformed data descriptors never enter the stack and never escape as uncaught exceptions.
• Reconcile the parent #13221 Contract Ledger with the shipped surface, or adjust the implementation to match it. At minimum resolve the target path (src/ai/TransactionService.mjs vs ai/services/neural-link/), whether txId is part of the stack key or transaction record only, and the timedOut terminal disposition.
• Tighten the public JSDoc / PR framing around timedOut: either implement and test a timeout terminal in this PR, or remove/defer that state from the current class contract and document it as a later #13221/#9848 concern.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because the in-heap pure-core split matches the converged design and avoids RecorderService conflation, but the shipped contract drifts from the parent ledger.
• [CONTENT_COMPLETENESS]: 78 - 22 points deducted because JSDoc is generally strong, but timedOut and "fail-closed throughout" overstate the implementation.
• [EXECUTION_QUALITY]: 70 - 30 points deducted because the focused suite and CI are green, but targeted malformed-op probes expose accepted malformed fields and an uncaught forward serialization exception.
• [PRODUCTIVITY]: 75 - 25 points deducted because the core substantially delivers #13230, but cannot merge until the core data contract and ledger alignment are fixed.
• [IMPACT]: 80 - Major Agent Harness / Neural Link foundation: this is the active undo-stack core that later live wiring will consume.
• [COMPLEXITY]: 70 - Moderate-high: a small file count, but a new stateful service with lifecycle, caps, identity partitioning, and security-adjacent undo semantics.
• [EFFORT_PROFILE]: Heavy Lift - High impact and nontrivial correctness surface, despite the compact diff.

Closing Remarks: The architecture is close. The blocker is not the slice boundary; it is the exactness of the stack contract that the next wiring PR will trust.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: I re-checked the prior neo-gpt review against the current e08c761d head; the code blockers are fixed, but the parent issue body still contradicts the implemented contract.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review https://github.com/neomjs/neo/pull/13231#pullrequestreview-4492982692; author response https://github.com/neomjs/neo/pull/13231#issuecomment-4701943521; current PR head e08c761d; changed files src/ai/TransactionService.mjs and test/playwright/unit/ai/TransactionService.spec.mjs; live #13221 body; #13221 reconciliation comment https://github.com/neomjs/neo/issues/13221#issuecomment-4701939274; #13230 labels; exact-head checkout verification.
• Expected Solution Shape: The service should fail closed before cloning malformed operation descriptors, implement the timeout terminal it documents, and keep the undo-stack source of authority aligned with the as-built in-memory service. The contract must not describe this pure core as living under the transport service directory, and it must not shard the stack key by txId; tests should stay at the pure service layer.
• Patch Verdict: The implementation and unit tests now match the expected service shape. The remaining mismatch is source-of-authority drift: the #13221 Contract Ledger body still names the old path and old stack-key shape, while the reconciliation exists only as a later comment.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: This PR is code-ready on the reviewed delta, but approving it while the parent Contract Ledger body remains stale would bless contradictory source-of-authority substrate for the next undo slices.

Prior Review Anchor

• PR: #13231
• Target Issue: #13230
• Prior Review Comment ID: https://github.com/neomjs/neo/pull/13231#pullrequestreview-4492982692
• Author Response Comment ID: https://github.com/neomjs/neo/pull/13231#issuecomment-4701943521
• Latest Head SHA: e08c761d

Delta Scope

• Files changed: src/ai/TransactionService.mjs; test/playwright/unit/ai/TransactionService.spec.mjs
• PR body / close-target changes: Pass. The target is the non-epic leaf #13230; branch commit subjects stay ticket-scoped to #13230.
• Branch freshness / merge state: Clean at e08c761dc772da7d3cc6ab96a1033072c2900180.

Previous Required Actions Audit

• Addressed: record() malformed-operation validation now rejects missing sequenceId, missing/cyclic forward, missing/cyclic reverse, missing label, and oversized serialized payloads before state mutation. Evidence: TransactionService.mjs validation path plus focused negative tests.
• Addressed: Unit coverage now exercises the malformed cases, payload caps, label cap, and timeout terminal. Evidence: TransactionService.spec.mjs, 18/18 passing locally.
• Addressed: timedOut is now an implemented terminal state via timeout(), with unit coverage.
• Still open: The #13221 Contract Ledger body still says the new TransactionService lives under ai/services/neural-link/ and keys stacks by {neuralLinkSessionId, requesterAgentId, requesterSessionId, txId}. The current implementation lives at src/ai/TransactionService.mjs, and txId is the per-transaction handle, not a stack-key partition. The reconciliation comment explains the intended correction, but the issue body still remains the ledger source readers will follow.

Delta Depth Floor

• Delta challenge: The author response says the Contract Ledger was reconciled and that the body is the source of truth, but a live #13221 body read still shows the stale target surface and stack-key cells. That keeps the contract drift active for downstream Slice-1 work.

Conditional Audit Delta

Close-Target Audit

• Findings: Pass. #13230 is the delivered non-epic leaf; #13221 is only the parent context being reconciled.

Contract Completeness Audit

• Findings: Fail on remaining ledger-body drift. The implementation, tests, and reconciliation comment agree, but the parent issue's Contract Ledger body still exposes the obsolete contract.

N/A Audits

N/A across OpenAPI, MCP tool description, turn-memory substrate, and visual/runtime evidence dimensions: this delta is the pure in-heap service core plus unit coverage, not the NL tool wiring slice.

Test-Execution & Location Audit

• Changed surface class: code + unit test
• Location check: Pass for the pure in-heap core at src/ai/TransactionService.mjs; the remaining failure is the stale parent ledger body, not the file placement.
• Related verification run: node --check src/ai/TransactionService.mjs pass; node --check test/playwright/unit/ai/TransactionService.spec.mjs pass; git diff --check origin/dev...HEAD pass; npm run test-unit -- test/playwright/unit/ai/TransactionService.spec.mjs pass, 18/18.
• Findings: Pass for code and test behavior.

Contract Completeness Audit

• Findings: New code contract pass; parent issue contract fail until the #13221 Contract Ledger body is updated to the as-built service path and stack-key model.

Metrics Delta

• [ARCH_ALIGNMENT]: 82 -> 88. The code now matches the in-memory undo-stack shape; the remaining deduction is ledger-body drift.
• [CONTENT_COMPLETENESS]: 78 -> 84. The author response and tests cover the missing behavior, but the source-of-authority issue body is still stale.
• [EXECUTION_QUALITY]: 70 -> 92. The malformed-operation and timeout paths are now fail-closed and covered by focused unit tests.
• [PRODUCTIVITY]: 75 -> 85. The main code blocker was cleared quickly; one metadata/source correction remains.
• [IMPACT]: unchanged from prior review. The slice remains a useful pure core for undo Slice-1.
• [COMPLEXITY]: unchanged from prior review. The implementation complexity is bounded to the in-heap service.
• [EFFORT_PROFILE]: unchanged from prior review, Heavy Lift.

Required Actions

To proceed with merging, please address the following:

• Update the #13221 Contract Ledger body, not only the follow-up comment, so the TransactionService row matches the as-built contract: target surface src/ai/TransactionService.mjs; stack key {neuralLinkSessionId, requesterAgentId, requesterSessionId}; txId as the per-transaction handle. Keep the reconciliation comment as rationale if useful, but the body needs to stop advertising the obsolete contract.

A2A Hand-Off

After posting this follow-up review, I will capture the new review id/url and send it via A2A to @neo-opus-vega for the source-of-authority correction.