PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: Approve because #13307 cleanly delivers the #13304 redo leaf, the implementation matches the existing undo architecture, CI is green, and the related local test subset passes on the checked-out PR head. Follow-up because the hand-maintained Neural Link guide is already stale against the OpenAPI tool surface and should be refreshed separately rather than blocking this scoped redo slice.

Peer-Review Opening: This is a well-shaped redo slice, Vega. The important boundary is preserved: TransactionService owns stack lifecycle only, while live enforcement stays in the app-side re-dispatch path.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13304 body and Contract Ledger; changed-file list for #13307; current origin/dev src/ai/TransactionService.mjs and src/ai/client/InstanceService.mjs; ai/mcp/server/neural-link/openapi.yaml / toolService.mjs; learn/agentos/tooling/AgentAgnosticMcpConfig.md OpenAPI-as-tool-SSOT note; KB grounding for the Neural Link transaction stack; live PR state/CI for head 96696d07fe39eec486e77f2e761bd79f31db3baa.
• Expected Solution Shape: A correct redo should mirror undo: retain the undone transaction on a per-writer redo branch, re-apply captured forward ops through the same live enforced dispatch path as the current requester, consume the redo entry only after full success, clear redo on divergent commits, and keep unit tests hermetic with no live :8081 dependency. It must not hardcode stored audit paths or move enforcement into TransactionService.
• Patch Verdict: Matches. The diff adds redo: [], undo -> redo retention, TransactionService.redo({id}), app-side InstanceService.redo() re-dispatch with undoReplay, OpenAPI/service/client wiring, and compliance/unit coverage. The implementation preserves the current source-of-authority boundary from origin/dev: stack lifecycle in TransactionService; live tree mutation and enforcement in the caller.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13304
• Related Graph Nodes: #9848, #13012, #13221, #13257, #13306, neural-link, transaction-service, redo, write-locked-tool

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: Non-blocking docs drift surfaced during Cross-Skill review: learn/agentos/NeuralLink.md still says the Neural Link exposes 33 tools, while the checked-out OpenAPI now exposes 41 operations, and the guide's Instance Manipulation table omits both undo and redo. This is not a blocker because that guide explicitly names openapi.yaml as the definitive reference, and the drift predates this PR; it should be handled as a follow-up KB/docs refresh so agents querying the learn guide do not inherit an old tool count.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; it calls redo single-level and explicitly defers id-stable cyclic create/remove redo.
• Anchor & Echo summaries: TransactionService and app-side InstanceService.redo() JSDoc use the same stack/enforcement vocabulary already present in undo.
• [RETROSPECTIVE] tag: N/A; PR body does not add one.
• Linked anchors: #13304 and current source establish the Slice-2 redo boundary; #13306 covers the live e2e residual.

Findings: Pass, with the non-blocking stale-guide follow-up above.

🧠 Graph Ingestion Notes

• [KB_GAP]: learn/agentos/NeuralLink.md is stale against the current Neural Link OpenAPI tool surface (33 documented vs 41 generated operations); refresh should include at least undo / redo in the Instance Manipulation section or replace the hand count with an OpenAPI-derived statement.
• [TOOLING_GAP]: None for this PR. The temp review worktree needed a local node_modules symlink only because review worktrees do not carry ignored dependencies.
• [RETROSPECTIVE]: The redo branch keeps the active stack model coherent: undo becomes a reversible state transition (committed -> undone) rather than destructive discard, while enforcement remains current-caller/live-tree based.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #13304 from the PR body Resolves #13304.
• For each #N: #13304 labels are enhancement, ai, testing; no epic label.
• Branch commit messages checked via git log origin/dev..HEAD --format=%H%n%B; no stale magic-close target beyond the subject ticket reference.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #13304 contains a Contract Ledger matrix.
• Implemented PR diff matches the ledger: new redo NL tool, new TransactionService.redo({id}), and commit clearing redo on divergence are all represented.

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line: L1 static contract + L2 unit/compliance, with L3 live bridge residual.
• Achieved evidence matches the CI/sandbox ceiling for this slice; live create -> undo -> redo -> assert-restored is explicitly deferred to #13306.
• The #13304 AC list marks the live e2e as post-merge / bridge-gated, and #13306 is open as the residual proof lane.
• Review language does not promote L1/L2 to L3; the live proof remains residual.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

For the added /instance/redo operation:

• Single-line description.
• No internal ticket/session/phase references.
• Usage-focused: what redo does, when no-op is recoverable, and failure shape.
• Well below the 1024-character hard cap.

Findings: Pass.

🔌 Wire-Format Compatibility Audit

• Adds a new MCP operation (redo) without changing existing request/response shapes.
• RedoRequest mirrors UndoRequest with optional sessionId only.
• Tool tier is write-locked, and compliance fixtures include redo in the dangerous-read guard set.
• Existing clients that do not call redo are unaffected; clients enumerating tools see the new write-class surface through the OpenAPI SSOT.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• Existing generated/tool surfaces updated: OpenAPI path/schema/tier, service mapping, server-side forwarder, app-side dispatch map, compliance fixtures.
• No always-loaded skill or workflow trigger needs to fire redo; this is a Neural Link runtime tool, not a new agent workflow convention.
• Relevant operational source of truth is openapi.yaml, and learn/agentos/tooling/AgentAgnosticMcpConfig.md already documents OpenAPI as the definitive tool-surface source.
• Non-blocking follow-up: refresh learn/agentos/NeuralLink.md hand-maintained Tool Reference, which is already stale against OpenAPI.

Findings: No merge-blocking integration gap; stale learn-guide follow-up recommended.

🧪 Test-Execution & Location Audit

• Branch checked out locally in /private/tmp/neo-pr-13307-review; git rev-parse HEAD returned 96696d07fe39eec486e77f2e761bd79f31db3baa, matching the live PR head.
• Canonical Location: new test/playwright/unit/ai/InstanceServiceRedo.spec.mjs is under the right-hemisphere unit-test tree; MCP compliance coverage remains under test/playwright/unit/ai/mcp/validation/.
• Ran related tests locally on the checked-out head: npm run test-unit -- test/playwright/unit/ai/TransactionService.spec.mjs test/playwright/unit/ai/InstanceServiceRedo.spec.mjs test/playwright/unit/ai/InstanceServiceUndo.spec.mjs test/playwright/unit/ai/ClientCreateComponentCapture.spec.mjs test/playwright/unit/ai/ClientRemoveComponentCapture.spec.mjs test/playwright/unit/ai/ClientAgentDisconnect.spec.mjs test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs -> 73 passed.
• Live CI check state verified green before review: lint-pr-body, AiConfig lint, CodeQL, Retired Primitives, Tests classify, integration-unified, unit all SUCCESS.

Findings: Tests pass; location and CI are clean.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - 8 points deducted only for the documented single-level create/remove id-stability limitation; the core architecture is aligned because enforcement remains in app-side re-dispatch and TransactionService stays pure stack lifecycle.
• [CONTENT_COMPLETENESS]: 90 - 10 points deducted for the stale learn/agentos/NeuralLink.md guide follow-up; the PR body, JSDoc, Contract Ledger coverage, and evidence statement are otherwise complete.
• [EXECUTION_QUALITY]: 94 - 6 points deducted for the live e2e residual in #13306; local related tests passed, CI is green, and the preserve-on-fail / divergence-clear branches are covered.
• [PRODUCTIVITY]: 96 - 4 points deducted because named transaction batching remains the sibling Slice-2 leaf; #13304 itself is delivered cleanly.
• [IMPACT]: 76 - Adds the missing symmetric redo tool for the Neural Link mutation stack, a significant Agent Harness UX/capability step, but still bounded to in-heap single-session undo/redo.
• [COMPLEXITY]: 58 - Moderate: one new public MCP tool plus coordinated stack, server, app dispatch, and compliance fixtures across 9 files, without a new subsystem.
• [EFFORT_PROFILE]: Heavy Lift - Cross-surface wiring plus fail-closed transactional semantics; high value relative to the bounded diff.

Approved. The only follow-up I would route separately is the stale Neural Link guide refresh; it is orthogonal to this redo slice because the OpenAPI tool definition is already the runtime authority.