PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The core shape is correct and the L3 happy path is locally verified, but the boundary contract says module class-reference configs are rejected. The implementation rejects only top-level config.module, leaving nested item configs to fail inside Neo factory code instead of at the Neural Link boundary.

Self-Review Opening: Same-family/self-attribution review for #13387: the PR body attributes Euclid (GPT/Codex) and the current head includes a neo-gpt docs commit, so this is a blocking correctness review, not a substitute for the requested cross-family review after the fix.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #13373 contract ledger and ACs; PR #13387 body/conversation/diff; live head 68596c0c; gh pr checks 13387 --watch=false; ai/mcp/server/neural-link/openapi.yaml; ai/services/neural-link/InstanceService.mjs; src/ai/client/InstanceService.mjs; src/container/Base.mjs; src/Neo.mjs; new unit/E2E tests; Neural Link operational handbook; evidence ladder.
• Expected Solution Shape: create_instance should be an additive write-locked MCP operation that accepts JSON-addressable className/ntype configs, keeps create_component as sugar, records undo through an internal destroy replay, and rejects class-reference/function-bearing config before App Worker mutation. Validation needs to match the public contract recursively, because nested Neo configs are common and are consumed by container/item creation.
• Patch Verdict: Mostly matches the intended shape: service mapping, app-worker dispatch, write guard, undo, docs, and tests align. The patch contradicts the stated data-only boundary for nested module configs.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13373
• Related Graph Nodes: Neural Link write-locked tool surface, create_component sugar, WriteGuard, undo/replay, MCP projection tiers

🔬 Depth Floor

Challenge: Both validation layers recurse only for function-bearing values. A nested module key such as {className: 'Neo.container.Base', config: {items: [{module: 'Neo.button.Base'}]}} is accepted by ai/services/neural-link/InstanceService.mjs and src/ai/client/InstanceService.mjs, then reaches src/container/Base.mjs where module.prototype.className is dereferenced. My one-off exact-head probe produced TypeError: Cannot read properties of undefined (reading 'className') instead of the promised module cannot cross boundary rejection.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift flagged below
• Anchor & Echo summaries: precise enough except for recursive boundary implication
• [RETROSPECTIVE] tag: N/A
• Linked anchors: no borrowed-authority issue found

Findings: The PR description and OpenAPI say module class references are rejected, but the implementation only rejects top-level config.module.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. KB does not know create_instance yet because this PR introduces it; existing Neural Link/write-locked projection context matched the expected substrate.
• [TOOLING_GAP]: Local E2E first failed under sandbox with listen EPERM 0.0.0.0:8080 and uv_uptime EPERM; rerunning the exact command with approved escalation passed.
• [RETROSPECTIVE]: The general create_instance primitive is the right successor layer below create_component, but class-reference validation should be one recursive data-boundary helper shared by server/app-side create paths.

🎯 Close-Target Audit

• Close-targets identified: #13373
• For each #N: confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly

Findings: Contract drift flagged: the ledger/body require rejecting module class-reference payloads, but nested module keys currently pass boundary validation.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence is L3 for the covered happy paths
• If residuals exist: N/A in PR body
• Two-ceiling distinction: acceptable for this PR because the live whitebox E2E is within the reviewer sandbox when escalated
• Evidence-class collapse check: no L1/L2 overclaim found for the tested paths

Findings: The declared L3 evidence is valid for standalone Store creation, parent-attached component creation, undo, projection tier, and top-level invalid payloads. It does not cover recursive module rejection; add that coverage with the fix.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — block-literal justified by new tool contract scope
• No internal cross-refs
• No architectural narrative beyond call-site usage/projection boundary
• External standard URLs OK — N/A
• 1024-char hard cap respected (create_instance operation description measured at 841 chars)

Findings: Pass. The description is close enough to the cap that future additions should move to docs/JSDoc rather than growing OpenAPI payload text.

🛂 Provenance Audit

Findings: The PR's GitHub author, PR-body author attribution, and commit authors are mixed (neo-opus-ada, Euclid/GPT-Codex, tobiu, neo-gpt). I did not treat this review as cross-family approval. After the required fix, the requested neo-claude-opus review still matters for merge eligibility.

📜 Source-of-Authority Audit

Findings: The required action is grounded in the issue contract, PR body/OpenAPI claim, and exact-head source behavior, not in operator preference.

🔌 Wire-Format Compatibility Audit

Findings: Additive MCP operation only. operationId is wired to toolService, x-neo-tool-tier: write-locked is present, and the listTools/projection smoke coverage passed. No existing tool signature break found.

🔗 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? N/A for this tool addition.
• 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?

Findings: Pass. learn/agentos/NeuralLink.md is the human-readable inventory surface and now lists create_instance; the Neural Link skill handbook is procedural and does not carry a tool inventory.

🧪 Test-Execution & Location Audit

• Branch checked out locally: /Users/Shared/codex/neomjs/neo/tmp/fix-13387, HEAD 68596c0c
• Canonical Location: new unit tests are under test/playwright/unit/ai/...; whitebox E2E under test/playwright/e2e/...
• If a test file changed: ran the specific test files
• If code changed: related focused tests exist and were run

Findings: Tests pass for the covered paths.

Verification run:

• gh pr checks 13387 --watch=false — all checks pass (unit, integration-unified, CodeQL, lint, PR-body lint, scope classification, retired primitive check).
• npm run test-unit -- test/playwright/unit/ai/services/neural-link/InstanceServiceCreateInstance.spec.mjs test/playwright/unit/ai/InstanceServiceCreateInstance.spec.mjs test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs test/playwright/unit/ai/mcp/server/McpServerListToolsSmoke.spec.mjs — 60 passed.
• npm run test-e2e -- test/playwright/e2e/NeuralLinkCreateInstance.spec.mjs — sandbox run blocked before test by EPERM; approved escalated rerun passed, 1 passed.

📋 Required Actions

To proceed with merging, please address the following:

• Make module rejection recursive across nested config objects/arrays on both the server-side MCP boundary and the app-side Bridge boundary, and add focused coverage for a nested module payload. The current implementation only checks Object.hasOwn(config, 'module') at the top level (ai/services/neural-link/InstanceService.mjs and src/ai/client/InstanceService.mjs), while Neo container item creation consumes nested item.module as a class-reference shape (src/container/Base.mjs).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 84 - Correct primitive, tiering, and undo architecture; recursive class-reference boundary must be fixed before merge.
• [CONTENT_COMPLETENESS]: 82 - Ticket ACs are mostly covered, but module rejection is narrower than the written contract.
• [EXECUTION_QUALITY]: 88 - Green CI plus local focused unit/E2E evidence; missing negative case for nested module configs.
• [PRODUCTIVITY]: 86 - High-value capability with contained follow-up cost.
• [IMPACT]: 91 - General instance creation unlocks the next Neural Link construction path.
• [COMPLEXITY]: 72 - Crosses MCP schema, server validation, App Worker mutation, undo, write guard, and live E2E.
• [EFFORT_PROFILE]: Architectural Pillar - New write-locked primitive under the agent-facing runtime control plane.

Once recursive module rejection is in place with tests, I expect this to move to approval quickly; the rest of the shape is solid.