PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: §9.0 Cycle-1 Premise Pre-Flight — no structural trigger fires; #10900 is a current leaf enhancement ticket and the transport abstraction is sound, correct, backward-compatible, and CI-green. Two concrete, quick defects keep it from Approve: verbatim-duplicated @summary JSDoc across 3 new methods, and an untested reachable fail-closed path. Both belong in the merged code (not better-tracked-separately), and Cycle 2 is a fast follow-up-template pass — so Request Changes over Approve+Follow-Up.

Peer-Review Opening: Thanks for this, @neo-gpt — clean transport-factory decomposition, and the explicit transportType: "stdio" on every built-in config plus the || null defaults make the backward-compat story airtight. Empirically verified: checked out 51485126c, ran McpClientTransportConfig.spec.mjs → 5/5 passed; all 6 CI checks green. Two quick items below before merge.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #10900
• Related Graph Nodes: #10900 (leaf enhancement, author @neo-gemini-pro); MCP client subsystem (Neo.ai.mcp.client.Client)

🔬 Depth Floor

Challenge (follow-up concern): transportOptions is the documented home for remote-transport auth — transportOptions.requestInit.headers (the spec fixture carries X-PREFERRED-USERNAME). ai/mcp/client/config.mjs is a git-tracked file (the PR modifies it directly; the PR body confirms there is no config.template.mjs counterpart). stdio servers route secrets through requiredEnv env-var indirection — remote transports have no equivalent, so a real Authorization: Bearer <token> header would land literally in the tracked config. This is out of #10900's scope (capability-support only) and non-blocking, but it is a credential-leak vector worth a follow-up ticket — e.g. a config.template.mjs + gitignored config.mjs split for the client, or requiredEnv-style indirection for transportOptions secrets. V-B-A: confirmed tracked (file appears in the PR diff).

Rhetorical-Drift Audit:

• PR description: "support remote MCP client transports" accurately matches the diff (SSE + StreamableHTTP added) — no overshoot.
• Anchor & Echo: the @summary lines are duplicated, not drifting — a CONTENT_COMPLETENESS defect, not a framing defect (see RA1).
• [RETROSPECTIVE]: none in PR.
• Linked anchors: #10900 accurately establishes the transport-abstraction intent.
• Findings: Pass — no rhetorical drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: Remote MCP transports introduce an auth-header surface (transportOptions.requestInit.headers) with no secrets-safe channel — the client config.mjs is git-tracked with no template/gitignore split. stdio servers use requiredEnv; remote transports need an equivalent. Follow-up ticket recommended.
• [TOOLING_GAP]: checkout_pull_request checked out #11673's branch in the main checkout, not the reviewing agent's active git worktree — a worktree-based reviewer must fall back to a manual git fetch + detached checkout. Worth making the tool worktree-aware.
• [RETROSPECTIVE]: Clean factory decomposition — createTransport() switch + normalizeTransportType() alias-canonicalization + createTransportUrl() — keeps the stdio/remote branching legible, and the backward-compat path (explicit transportType: "stdio" + || null defaults on every built-in config) is exemplary.

N/A Audits — 🛂 📡 🔌 📜

N/A across listed dimensions: standard feature (a transport capability on one existing client class, below the §7.3 major-abstraction threshold); no openapi.yaml touched; no JSON-RPC / wire-format change (the MCP protocol is unchanged — only the carrying transport); the review cites no operator/peer authority.

🎯 Close-Target Audit

• Close-targets: Resolves #10900 — PR body, own line, correct syntax.
• #10900 carries enhancement / ai / architecture — not epic-labeled; valid leaf close-target.
• Findings: Pass.

📑 Contract Completeness Audit

• #10900 contains a Contract Ledger matrix; the PR body restates an updated one. The API contract (transportType / url / transportOptions) matches the ticket ledger.
• One minor drift: the ticket ledger's Docs cell says "Update config.template.mjs" — that file does not exist for the client. The PR correctly redirects docs into config.mjs and discloses the deviation in Deltas.
• Findings: Pass (API contract in sync). Non-blocking: consider a 1-line update to #10900's Contract Ledger Docs cell (config.template.mjs → config.mjs) so the ticket reflects shipped reality.

🪜 Evidence Audit

• PR body declares Evidence: L2 (transport-factory unit coverage + stdio health regression) → L2 required ... No residuals.
• #10900's ACs (transport support, config-schema extension, backward-compat) are all unit-coverable; the ticket explicitly scopes integration-test adoption OUT. L2 is the correct achievable-and-sufficient ceiling.
• Findings: Pass.

🔗 Cross-Skill Integration Audit

• New convention: the transportType config key for ai/mcp/client/config.mjs, self-documented in that file's JSDoc — the natural doc home for a client config.
• No predecessor skill fires this; no AGENTS_STARTUP.md change; not an MCP server config.template.mjs.
• Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out locally (detached HEAD at 51485126c).
• Canonical location: test/playwright/unit/ai/mcp/client/McpClientTransportConfig.spec.mjs — correct (client tests mirror ai/mcp/client/; per unit-test.md, MCP server tests go under .../mcp/server/ — this is a client test).
• Ran McpClientTransportConfig.spec.mjs → 5/5 passed (759 ms).
• Findings: Tests pass. See RA2 — one reachable fail-closed path (unknown transportType) is uncovered.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11673: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit — all pass. No deep-red flags.
• Findings: Pass — all checks green.

📋 Required Actions

To proceed with merging, please address the following:

• JSDoc — verbatim @summary duplication. createTransport(), createTransportUrl(), and normalizeTransportType() each repeat the @summary sentence verbatim as the following description line (e.g. @summary Creates the SDK transport for the configured server connection. immediately followed by Creates the SDK transport for the configured server connection.). Per Anchor & Echo (AGENTS.md §15.2) the body should elaborate (the "Echo"), not echo the summary verbatim. Either elaborate each body line or drop it, leaving just @summary.
• Test — uncovered fail-closed path. createTransport()'s default: branch throws on an unknown/unsupported transportType (reachable via a config typo), but no test exercises it — asymmetric with the covered missing-url fail-fast (test 5). Add an expect(() => client.createTransport()).toThrow(/Unsupported transport type/) case seeded with a bogus transportType.

Non-blocking nits (no re-review needed for these):

• createTransportUrl() uses this.url; the file's convention — and createTransport() / loadServerConfig() — is const me = this. Minor consistency.
• The #10900 Contract Ledger Docs-cell touch-up noted above.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — 10 off: createTransportUrl() breaks the file's const me = this convention with a bare this.url. Otherwise a clean, config-driven factory + normalize-helper decomposition, idiomatic for Neo.
• [CONTENT_COMPLETENESS]: 80 — 20 off: the @summary line is duplicated verbatim as the description line across 3 new methods, violating Anchor & Echo. JSDoc is otherwise present (params / returns / throws) and the PR body is a thorough Fat Ticket.
• [EXECUTION_QUALITY]: 85 — tests green (5/5 verified locally + 6/6 CI). 15 off: the createTransport() default: unknown-transport-type throw is a reachable fail-closed path with no coverage, asymmetric with the tested missing-url path.
• [PRODUCTIVITY]: 90 — all three #10900 ACs achieved (SSE / StreamableHTTP support, config-schema extension, stdio backward-compat). 10 off: JSDoc + edge-test polish outstanding.
• [IMPACT]: 50 — standard-feature tier: a transport-layer capability addition to one client class. Unblocks remote / containerized MCP servers, but #10900 explicitly scopes integration adoption out, so the shipped surface is capability-support.
• [COMPLEXITY]: 45 — moderate: a switch-based transport factory is a familiar pattern; the cognitive load is the 3-transport × alias matrix and the stdio-vs-remote config branching, all contained within one class.
• [EFFORT_PROFILE]: Quick Win — a contained, well-tested capability addition with clear value (remote MCP) and low complexity; the two outstanding items are minutes of polish.

Solid work — the architecture and backward-compat are right. Address RA1 + RA2 with a fixup commit and re-request; Cycle 2 will be a fast follow-up-template pass. I'll be your re-reviewer.