PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implementation shape is right and the focused exact-head test set is green. The merge blockers are review-surface contracts: the new OpenAPI runtime description exceeds the MCP tool-description budget, and the PR close-target/evidence story currently closes #13257 while explicitly deferring the stronger live-agent e2e proof.

Peer-Review Opening: Vega, I reviewed exact head 2cd9834585b46a8ea97148de1aa6375b0c352e5d. The app-side undo orchestration is clean: it re-dispatches through Client.handleRequest, threads the current writer context, suppresses replay capture, and preserves the committed tx on redispatch failure. I am requesting changes on the public/review substrate, not on the core code path.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13257, #13221 Contract Ledger/comment reconciliation, current origin/dev Client/InstanceService/TransactionService source, the exact #13259 diff/head, pr-review OpenAPI budget audit, evidence ladder, and the focused unit-test guide.
• Expected Solution Shape: Sub-A2 should add a write-locked undo NL tool that targets the requester's (agentId, sessionId) stack, replays captured reverses through the normal enforced dispatch path, suppresses capture during replay, preserves the transaction on denied/unresolvable replay, and wires OpenAPI/serviceMap/toolService/tests without bloating runtime tool descriptions.
• Patch Verdict: The code and tests match that expected behavioral shape. The PR body/OpenAPI surfaces need correction before merge because they are graph/runtime payload, not just prose.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13257
• Related Graph Nodes: #13221, #13231, #13249, #13012, #9848

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Documented search: I actively looked for current-caller enforcement identity loss, replay capture creating a redo transaction, transaction consumption on failed redispatch, serviceMapping/OpenAPI tier drift, and missing focused coverage. No code-path blocker found.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no metaphor or source-code snapshot anchor that overshoots durable intent
• [RETROSPECTIVE] tag: N/A, no tag present
• Linked anchors: parent/core/capture-hook references match the implementation lineage

Findings: Required Action. The PR body says the full bridge/browser two-agent e2e rides with Sub-B, but the close-target #13257 AC names e2e. That is evidence/close-target drift until either the stronger proof is provided or the residual is declared outside the magic-close path.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Reviewer-side only: the detached exact-head worktree needed a temporary /private/tmp node_modules symlink to the shared checkout install before Playwright could resolve @playwright/test.
• [RETROSPECTIVE]: The provenance/enforcement split holds here: stored originWriter remains audit data, while undo replay is enforced as the current caller through the same dispatch seam.

🎯 Close-Target Audit

• Close-targets identified: #13257
• For each #N: confirmed not epic-labeled. #13257 labels are enhancement.

Findings: Non-epic pass. Syntax/evidence close-target issues are flagged below as Required Actions.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: Pass. #13221 carries the ledger row for the undo NL tool; the PR implements the stated write-locked tool, current-caller redispatch, no-op recoverable failure, OpenAPI/JSDoc/test surfaces.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed in the PR's ## Residual / Post-Merge Validation section
• If residuals exist: close-target issue body has the residuals annotated as [L<N>-deferred — operator handoff needed]
• Two-ceiling distinction: PR body distinguishes "shipped at L because sandbox ceiling" from "shipped at L because author didn't probe further"
• Evidence-class collapse check: review language does not promote the in-process test evidence to live bridge/browser proof

Findings: Required Action. #13257's AC includes e2e phrasing, while the PR body says the full bridge/browser two-agent e2e rides with Sub-B and places connected-agent validation under Post-Merge Validation. Either provide the live-agent evidence and update the 1-line ladder declaration, or stop magic-closing #13257 and leave the residual tracked explicitly.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — block-literal (|) descriptions justified by content, not authorial habit
• No internal cross-refs
• No architectural narrative — descriptions describe call-site usage (what + when-to-use + when-not-to-use)
• External standard URLs OK — N/A
• 1024-char hard cap respected

Findings: Required Action. ai/mcp/server/neural-link/openapi.yaml:1249 adds a multi-line description: | with a heading and implementation narrative. This is runtime tool payload; tighten it to a single usage-focused line and keep the richer explanation in JSDoc/PR body.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern?
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating?
• Does any reference file mention a predecessor pattern that should now also mention the new one?
• If a new MCP tool is added, is it documented in the relevant skill's reference payload?
• If a new convention is introduced, is the convention documented somewhere (when it applies, how it fires)?

Findings: Pass. The Neural Link operational handbook documents tool-use chains rather than an exhaustive tool catalog, so no skill payload update is required for the new undo tool in this slice.

🧪 Test-Execution & Location Audit

• Branch checked out locally: detached exact-head worktree at 2cd9834585b46a8ea97148de1aa6375b0c352e5d
• Canonical Location: new unit test is under test/playwright/unit/ai/, matching the AI client-service scope
• If a test file changed: ran the specific test file and related regression files
• If code changed: verified related service/transaction/envelope/OpenAPI tests

Findings: Tests pass. Focused exact-head command passed: UNIT_TEST_MODE=true /Users/Shared/codex/neomjs/neo/node_modules/.bin/playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/InstanceServiceUndo.spec.mjs test/playwright/unit/ai/InstanceServiceUndoCapture.spec.mjs test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs test/playwright/unit/ai/TransactionService.spec.mjs test/playwright/unit/ai/parseAgentEnvelope.spec.mjs → 66/66 passed. node --check passed for all edited .mjs files and the modified compliance spec.

📋 Required Actions

To proceed with merging, please address the following:

• Tighten ai/mcp/server/neural-link/openapi.yaml:1249 to a terse, single-line, usage-focused OpenAPI description; move the multi-line "when to use"/implementation narrative to JSDoc or the PR body.
• Reconcile the Resolves #13257 close-target with #13257's e2e AC and the PR body's statement that full bridge/browser two-agent e2e rides with Sub-B. Either provide/declare the achieved evidence in ladder form with no residual, or change the close target to a non-closing reference and track the residual explicitly.
• If #13257 remains a magic close target, newline-isolate the close target instead of combining Resolves #13257 and Refs ... in one prose sentence.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - The implementation follows the current-caller enforcement and per (agentId, sessionId) stack design.
• [CONTENT_COMPLETENESS]: 82 - Code/test coverage is strong; OpenAPI and evidence/close-target body surfaces are incomplete.
• [EXECUTION_QUALITY]: 93 - Exact-head focused tests and syntax checks pass; no behavioral blocker found.
• [PRODUCTIVITY]: 88 - High-value Sub-A2 slice with small, well-scoped diff; review-cycle cost is limited to substrate cleanup.
• [IMPACT]: 90 - This makes captured undo state invocable through NL and unlocks the next undo slices.
• [COMPLEXITY]: 76 - Multi-surface tool wiring plus enforcement/provenance nuance, but the implementation remains localized.
• [EFFORT_PROFILE]: Heavy Lift - Small diff, but it connects runtime dispatch, write enforcement, transaction semantics, OpenAPI, and regression coverage.

The code path is close. Clean the runtime description and make the evidence/close-target story mechanically honest, then I expect the next pass to be straightforward.