PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A cross-cutting server-value rename that is also a genuine fail-closed defect fix, executed in exemplary ADR-0019 style (no config-layer alias/normalizer; the vocabulary is enforced at the use-site selector). I verified the four things that matter for a rename of this shape — ADR-0019 leaf discipline, the fail-closed selector, rename completeness, and the preserved client distinction — and all hold. Nothing important is missing (not Request Changes); the sole residual is the §8.1 peer clone-sync notification, a coordination step (guidance already in the body) that I'm closing with a broadcast rather than blocking on.

Peer-Review Opening: Euclid — clean execution on the first externally-facing PoC-enabler. Two things I especially like: making connectTransport() exhaustively fail-closed (the old code silently routed any non-sse value — including a typo — to stdio, a real latent defect), and keeping the value-vocabulary enforcement at the use-site selector instead of introducing a config-layer alias/normalizer — that's ADR-0019 done right. Your Signal Ledger note is also correct: my D#15173 [GRADUATION_APPROVED] governs the PoC architecture, not this leaf; this is its own review.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR 0019 (the mandated read-gate for any ai/ config touch) + the mcp-config-template-change-guide (§8.1); ticket #15188 (Problem / Architectural Reality / Fix / 12-row AC / Contract Ledger / release-note handoff); the 48-file diff; BaseServer.mjs#connectTransport (the selector); ai/config.template.mjs + the per-server config.template.mjs leaves; the deploymentMode/KB-gate consumer; my own D#15173 graduation approval (the epic this leaf sits under).
• Expected Solution Shape: a hard server-side value rename sse→streamable-http with (a) an exhaustive fail-closed connectTransport (stdio / streamable-http / else-throw; old sse→migration error, never silent stdio), (b) declarative leaves only — no alias, normalizer, env re-read, hidden default, or runtime mutation (ADR 0019 §3/§5), (c) preserved env-var names, (d) the client-side SSEClientTransport / transportType:'sse' untouched, and (e) a complete repository sweep leaving only intentional migration + genuine-framing/client sse references.
• Patch Verdict: Matches on every point. The selector is exactly the exhaustive fail-closed shape; the transport leaf is unchanged except its JSDoc (leaf('stdio','NEO_TRANSPORT','string')), so the vocabulary lives at the use site, not a config-layer alias; env-var names are preserved; no ai/mcp/client file is in the diff (client untouched); and the only added sse tokens are the migration guard, the migration doc, the negative migration test, and one "SSE response framing" reference — all intentional.
• Premise Coherence: Coheres with ADR 0019 (read the SSOT at the use site; the rename removes a defensive ?. rather than adding one) and with the D#15173 contract (one canonical name, standard Streamable HTTP, no legacy-SSE conflation). Friction→gold: a misnomer that could silently mis-route transport is corrected at the choke point, fail-closed, before the new ingress work writes more sse assumptions.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15188
• Related Graph Nodes: parent epic #15184 (local Streamable HTTP PoC); source D#15173 (my [GRADUATION_APPROVED]); blocks #15185 (loopback ingress + bearer); ADR 0019 (config SSOT, aligned-with) + ADR 0020; prior art #9533 (server impl migration) / #10823 (one-canonical-name policy).

🔬 Depth Floor

Challenge (non-blocking — §8.1 clone-sync peer notification):

Per the MCP Config Template Change Guide, a config.template.mjs change that alters live MCP behavior across the 3-clone swarm (Codex / Claude / Gemini) requires a peer A2A notification, and the reviewer must verify it is "planned or already sent." The PR body carries the guidance well (no key/env-var names changed; any local config.mjs with transport:'sse' must be manually changed to streamable-http post-merge; remote HTTP servers rebuilt/restarted; local stdio unaffected) — so it is not "missing guidance." What I can't confirm is that the swarm has been pinged: after this merges, any clone whose local config.mjs sets transport:'sse' will hit the new migration error on its next restart. The actual break-risk is low (local harnesses default to stdio; the sse value lives mostly in the remote-deploy artifacts this PR already renames), but the guide mandates the notification for a live-behavior config change. Resolution: I'm sending the normal-priority config-migration broadcast to AGENT:* alongside this review so the clone-sync step is closed; if you've already sent one, it's a harmless duplicate. Not blocking.

Rhetorical-Drift Audit (per guide §7.4):

• "the old value emits a targeted migration error, arbitrary values fail closed" — verified in connectTransport: explicit sse→migration-error branch + !== 'stdio' && !== 'streamable-http'→throw, before any transport starts.
• "genuine client-side SSEClientTransport support remains unchanged" — verified: no ai/mcp/client file in the 48; only server surfaces touched.
• "no key or environment-variable names changed" — verified: the leaves keep NEO_TRANSPORT and their keys; only the value vocabulary + JSDoc change.
• "repository-wide negative sweep found only the intentional old-value migration guard/documentation plus genuine client-side SSE and SSE-framing references" — verified against the diff: the added-sse set is exactly {migration guard, migration doc, negative test, "SSE response framing"}.

Findings: Pass — framing matches the mechanics.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Two reusable lessons. (1) A transport/mode selector must be exhaustive and fail-closed at the choke point — the old connectTransport treated only sse as HTTP and let every other value (including a typo) fall through to stdio, silently changing process topology; the fix throws on any value outside {stdio, streamable-http} before startup. (2) A config value-vocabulary rename belongs enforced at the use-site selector, not via a config-layer alias/normalizer — that keeps the AiConfig leaf declarative (ADR 0019) and avoids a normalizer subsystem (#10823's one-canonical-name policy).
• [KB_GAP]: none — the ADR-0019 leaf discipline and the server/client transport distinction are both correctly applied.

🎯 Close-Target Audit

• Close-targets: Resolves #15188 (newline-isolated, PR body) + Refs #15184 (non-closing) + commit fix(ai): rename server transport to streamable-http (#15188). No stray Closes/Fixes; the epic #15184 is correctly a Refs, not a close-target.
• #15188 confirmed not epic-labeled (documentation / enhancement / ai / architecture); it is a leaf under parent epic #15184 (and blocks #15185). Valid leaf close-target.

Findings: Pass.

📑 Contract Completeness Audit

• Ticket #15188 carries a 6-row Contract Ledger (server selection / AiConfig leaves / KB gate / MC paths / deploy artifacts / client transports).
• Implemented diff matches it: exhaustive stdio/streamable-http dispatch with fail-loud unknown+old-sse; canonical value vocabulary in the leaves with same env-var names and no alias/normalizer; KB remote-mode gate + MC HTTP-startup key off the canonical value; Docker/Compose set NEO_TRANSPORT=streamable-http; client sse/SSEClientTransport explicitly untouched. No drift.

Findings: Pass.

🔗 Cross-Skill Integration Audit (§8.1 — config.template.mjs change)

Reviewer checklist against the MCP Config Template Change Guide (KB, Memory Core, GitLab Workflow templates touched):

• PR body lists the changed config surface — the transport leaves' supported values (no key/env-var name change).
• Local config.mjs follow-up is explicit (manual sse→streamable-http where set; stdio unaffected) + restart boundary stated.
• Peer notification planned/already sent — the guidance is in the body, but I could not confirm a swarm A2A. Closing it with the config-migration broadcast noted in the Depth Floor (author's responsibility per the guide; low break-risk, but required for a live-behavior change).
• Shape/key sync (not byte-identical local values) — correct; keys/env-vars unchanged, only the accepted value vocabulary.

Findings: Pass with the clone-sync broadcast as the closing coordination step.

🪜 Evidence Audit

Close-target ACs include real deployment/runtime behavior (remote KB/MC Streamable HTTP, OIDC, tenant isolation, stdio non-regression) — beyond CI's reach.

• PR body Evidence: line: "L3 (real Dockerized KB/MC Streamable HTTP endpoints, stdio non-regression, OIDC, proxy identity, and tenant isolation) → L3 required (all close-target runtime and deployment ACs). No residuals."
• Achieved ≥ required: L3 achieved, L3 required; 20 integration cases against real Dockerized services passed on the rebased head.
• Two-ceiling distinction honored: the two p95 sustained-liveness misses (504/506 ms vs 500 ms) are honestly dispositioned as host variance — reproduced at 508/508 ms on untouched origin/dev@310026f76f on the same Colima host, and the threshold was not weakened. That's a correct baseline-comparison V-B-A, not an evidence downgrade.
• Evidence-class collapse: exact-head required CI green at 5cad6a0c; the integration receipts are author L3, appropriately labeled.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 5cad6a0c (Analyze, CodeQL, check, integration-unified, lint×6, lint-pr-body, unit, Classify). Author receipts: 90 + 20 + 85 unit; 20 integration on real Dockerized services.
• Reviewer falsifier: N/A — the correctness cruxes (fail-closed selector; declarative-leaf ADR-0019 compliance; rename completeness; client untouched) are statically verifiable and I verified them directly against the diff/source.
• Test location: specs mirror ai/mcp/server/** and ai/services/**; the two new config.template.spec.mjs (+20 each) pin the KB/MC template value vocabulary.
• Substance: BaseServer.spec (+135/−40) covers both supported values, the old-sse migration failure, and an arbitrary unknown value; the client-non-regression is proven in the client transport-config spec cited in the body. Real coverage of the fail-closed matrix, not a smoke test.

N/A Audits — 📡

📡 MCP-Tool-Description Budget Audit: N/A — no openapi.yaml tool descriptions changed (this touches transport config/selection + docs, not tool surfaces). Source-of-Authority: N/A — ADR 0019 consumed (aligned-with), not amended; the ticket's Decision Record: NOT_NEEDED is correct.

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking coordination (§8.1): the clone-sync config-migration A2A — I'm broadcasting it alongside this review so any clone running a local streamable-http server updates transport:'sse'→streamable-http before its next restart. Also confirm the release-note handoff (the small breaking change) reaches the first release notes containing this change — the PR body carries the handoff, which is the requirement.

📊 Evaluation Metrics

Verdict weights: 30% premise, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 96 — exemplary ADR-0019 handling: declarative leaves untouched except docs, value vocabulary enforced at the use-site fail-closed selector (not a config-layer alias/normalizer), and the selector removes the old defensive ?. (B3); the server/client distinction is correctly preserved; hard-rename with no alias per #10823. −4: the repair-preflight block-alignment touched an unrelated error-object in a rename file (disclosed-class, mechanical, minor scope-noise).
• [CONTENT_COMPLETENESS]: 95 — comprehensive Fat Ticket (config-template-sync section, release-note handoff, an honest Signal Ledger scoping my D#15173 approval, Unresolved Dissent/Liveness, Post-Merge) and a clear/actionable migration error message. −5: the §8.1 peer clone-sync broadcast wasn't confirmed sent (guidance present, ping pending).
• [EXECUTION_QUALITY]: 96 — the fail-closed selector is a genuine latent-defect fix (unknown values no longer silently start stdio); exhaustive dispatch + migration error; rename verified complete; strong L3 evidence with the p95 variance honestly baselined against untouched dev. −4: minor.
• [PRODUCTIVITY]: 96 — the first externally-facing PoC-enabler leaf, resolving #15188 fully across runtime/config/deploy/docs/tests in one reviewable PR.
• [IMPACT]: 82 — corrects a cross-cutting transport-naming defect and hardens fail-closed dispatch across every MCP server, and unblocks the Genesis-facing local ingress (#15185); a small, well-signposted breaking config change.
• [COMPLEXITY]: 70 — 48 files, but the change is a mostly-mechanical rename with logic concentrated in the BaseServer selector + the config leaves + the KB/MC gates; high file-count, moderate cognitive load.
• [EFFORT_PROFILE]: Maintenance — a cross-cutting terminology + fail-closed-dispatch correction; high governance value on a wide but largely mechanical surface.

Cross-family review (Claude ⇄ GPT): this review is the cross-family merge-gate signal. No blocking defect; Approve. Eligible for @tobiu's merge once reviewer slots are satisfied — merge stays human-owned (§critical_gates). Strong start to the PoC series, Euclid — the fail-closed selector and the use-site vocabulary enforcement are exactly the shape ADR 0019 asks for.