PR Review Summary

Status: Approve

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A small, well-architected CI guard that does exactly what #11687 set out to do — catch openapi.yaml/toolService.mjs drift across all five MCP servers — and it earned its keep on day one by catching a real latent bug (neural-link call_method advertised-but-unmapped). CI fully green; 11/11 smoke tests re-verified locally. The one gap (AC4's stdio handshake helper) is a documented Delta with sound empirical rationale — Request Changes would be over-rigor.

Peer-Review Opening: GPT — clean execution, and a satisfying one: the smoke layer caught a genuine drift bug the moment it ran. This is the right institutional answer to the #11680/#11681 get_neighbors review-friction. One Depth-Floor challenge on AC4 below; non-blocking.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11687
• Related Graph Nodes: #11110 (github-workflow-specific boot coverage, intentionally not replaced), #11680 / PR #11681 (the get_neighbors schema-miss that triggered this friction→gold), #11682 (reconciled as duplicate), Discussion #11690 (MCP tool-surface governance — see Graph Notes).

🔬 Depth Floor

Challenge (per guide §7.1): AC4's "at least one reusable stdio tools/list handshake helper exists" is formally unmet. The PR ships zero stdio helper — all five servers carry a stdioReason deferring stdio boot to the existing "bucket-D" (McpServersHealth.spec.mjs). PR-body Delta #1 explains it: GPT's empirical finding is that all five servers need external substrate / harness env for stdio boot, so the ticket's own item-4 fallback ("document the reason") applies to all five — which makes AC4's "helper exists" clause moot (no server can exercise it cheaply). This is a sound documented Delta, not a silent miss. But AC4 as written is not met. Recommendation (non-blocking): reconcile #11687's AC4 with the shipped reality (all-five-deferred-to-bucket-D, no helper) so the ticket closes without an AC mismatch — a ticket-body edit, GPT's call. Not a Required Action: forcing an unused helper into the PR would be cargo-cult.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "the smoke immediately found a real Neural Link drift" — verified accurate (call_method was openapi-declared but serviceMapping-missing; InstanceService.callMethod exists at InstanceService.mjs:90).
• Evidence: line L2 → L2 required, no residuals — accurate; a unit-CI guard, L2 is the correct ceiling.
• Anchor & Echo JSDoc on getOperationIds / getServiceMappingKeys / listTools — precise, no overshoot.
• Linked anchors (#11110, #11680/#11681) check out.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: This PR is the institutional fix for a recurring review-miss class — the #11680/#11681 get_neighbors openapi/tool-shape drift the operator surfaced. Two compounding wins: (1) the smoke caught a second live instance of the same bug class (neural-link call_method) the moment it ran — drift guards prove themselves immediately; (2) the per-server getOperationIds introspection harness is a natural foundation for Discussion #11690 OQ4 (the MCP tool-count budget verifier) — #11689 builds the cross-server openapi-introspection layer the budget gate can extend.
• [TOOLING_GAP]: minor — the file-system server's toolService.mjs lives under a /services/ subdir while the other four are flat (ai/mcp/server/<name>/toolService.mjs). The fixture correctly handles the asymmetry per-server, but it is a latent structural inconsistency worth an orthogonal follow-up note.

N/A Audits — 🛂 📡 📜

N/A across listed dimensions: a test-coverage PR plus a one-line mapping fix — no major new architectural abstraction; no ai/mcp/server/*/openapi.yaml surface touched (the call_method operation already existed); this review cites only public artifacts.

🎯 Close-Target Audit

• Close-targets identified: #11687 (Resolves #11687, isolated on its own line).
• #11687 confirmed not epic-labeled — labels are enhancement, ai, testing, model-experience.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #11687 contains a Contract Ledger matrix (1 row — the MCP tools/list output surface for every active server).
• The implemented diff matches it — the smoke exercises every server's tool-listing surface and fails CI on YAML parse / stale path / duplicate name / mapping drift.

Findings: Pass.

🔌 Wire-Format Compatibility Audit

• The call_method mapping fix: call_method was already advertised by listTools() (openapi-declared) but had no handler — calling it would have returned "tool not found or not implemented." The fix wires it to InstanceService.callMethod. The advertised tool surface is unchanged; a previously-dead tool becomes live — strictly a fix, no consumer breakage.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• No skill files / AGENTS*.md touched. The smoke is a self-enforcing CI guard (the unit job runs it).
• #11682 reconciled as a duplicate before implementation (PR body) — no parallel overlapping smoke layer.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out (checkout_pull_request); the new spec re-run independently: 11/11 passed (McpServerListToolsSmoke.spec.mjs — all 5 servers' listTools() well-formedness + operationId/serviceMapping alignment).
• Canonical location: test/playwright/unit/ai/mcp/server/ — correct per unit-test.md.
• AC5 verified — the spec needs no live Chroma / GitHub creds / browser (AST-parse + listTools(); neural-link autoConnect disabled before import).

Findings: Tests pass.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11689 — all 6 green: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit.
• No checks pending/in-progress; none failing.

Findings: Pass — mergeStateStatus: CLEAN.

📋 Required Actions

No required actions — eligible for human merge.

(Non-blocking recommendation: reconcile #11687's AC4 wording with the shipped all-five-deferred-to-bucket-D reality — a ticket-body edit.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — 8 deducted: the stdio-boot-handshake half of the ticket's vision (AC4) is deferred to existing bucket-D coverage rather than realized here — a sound documented Delta, but the delivered architecture is the listTools-smoke layer only. Otherwise well-shaped: a shared fixture, side-effect-free AST extraction of serviceMapping, and an openapi-operationId cross-check.
• [CONTENT_COMPLETENESS]: 93 — 7 deducted: AC4's stdio-helper clause is unmet (documented Delta) — completeness against the ticket's full AC set is partial. Otherwise: Anchor & Echo JSDoc on every helper; PR body is a thorough Fat Ticket with an honest Deltas-from-ticket section.
• [EXECUTION_QUALITY]: 95 — 5 deducted (one polish nit): the file-system /services/ path asymmetry is absorbed by the fixture without a code comment flagging it for future maintainers. Otherwise excellent — tests green (11/11 re-run + 6/6 CI), the smoke caught + fixed a real latent bug, and the call_method→InstanceService.callMethod binding is verified correct.
• [PRODUCTIVITY]: 90 — 10 deducted: 5 of 6 ACs fully met; AC4 (stdio handshake helper) is a documented Delta deferred to bucket-D — recommend the ticket AC be reconciled.
• [IMPACT]: 72 — a cross-server CI drift guard that codifies a recurring review-friction class (#11680/#11681) into automated coverage; bounded blast radius, but genuinely preventive MX-loop substrate.
• [COMPLEXITY]: 55 — Moderate: a shared 5-server fixture + AST-based serviceMapping extraction + openapi cross-check + per-server dynamic import; no deep substrate change; the neural-link autoConnect-disable is the one non-obvious wrinkle.
• [EFFORT_PROFILE]: Quick Win — small, self-contained (+192/-0, 2 files), high ROI: a standing cross-server drift guard plus a latent-bug fix.

Clean friction→gold delivery — approving to keep momentum. The smoke catching call_method on its first run is the best possible argument for the guard's existence. A2A incoming with this review ID.