PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Working PR with no blocking defect found under an adversarial walk of the guard-ordering contract. The two Depth-Floor observations below are reasoned-through residuals of the accepted design (each dies at a covered guard), not defects or debt — so neither Request Changes nor Approve+Follow-Up applies.

Peer-Review Opening: Thanks Euclid — this is how a security-ingress PR should arrive: the ticket ledger's ordering contract implemented literally, every guard proven at its real consumed seam (real SDK middleware, real sockets, real bind address), and the deltas from the ticket declared rather than discovered.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15185 in full (Contract Ledger, ten ACs, the #15187 boundary note); the ADR-0019 §3 antipattern catalog (the critical-gate read); current dev source of AuthService.mjs, TransportService.mjs (incl. computeAllowedHosts), and ai/config.template.mjs; the gitlab-pat mode-branch precedent (the #12378 auth-mode contract); Memory Core prior-art sweep — which surfaced that the Origin-validation gap this PR closes was identified empirically in a 2026-06-02 createMcpExpressApp V-B-A (Host validated, Origin not, CORS origin:'*'; the SDK's own transport-level origin options deprecated in favor of external middleware) and parked as future hardening.
• Expected Solution Shape: A third auth.mode early-return branch following the gitlab-pat precedent; an opt-in listener-bind leaf preserving app.listen(port) exactly when absent; a presence-based Origin reject registered before wildcard CORS; a side-effect-free credential helper with no persistence surface. Must NOT hardcode the bind into non-local modes, thread config values through layers (B5), or defensively ?. AiConfig reads (B3). Test isolation: consumed-boundary — the real SDK bearer middleware and a real socket, with config coverage on the canonical template spec (C3).
• Patch Verdict: Matches, and improves the expected shape in two places. (1) Object.hasOwn(req.headers, 'origin') is a presence discriminator, so Origin: "" — which a truthiness check would silently pass — is rejected, and a test pins exactly that case. (2) Startup validates the configured credential as canonical 32-byte unpadded base64url: the ledger's own credential-format row enforced at the fail-closed boundary, stricter than the ticket's "non-empty" floor and declared as a delta in the PR body.
• Premise Coherence: Coheres — verify-before-assert lineage made real: the Origin gap was found empirically with tools, parked with evidence, and is now closed inside a scoped trust contract instead of as speculative hardening. Possession-not-identity is carried honestly through the code (AuthInfo with no userId/username).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15185
• Related Graph Nodes: #15184 (parent epic) · #15187 (BigData journey — the launch contract's consumer) · ADR-0019 (config SSOT authority) · ADR-0020 · the #12378 auth-mode contract (the axis this extends)

🔬 Depth Floor

Challenge (per guide §7.1):

Two reasoned-through residuals worth naming so they sit on the graph — both non-blocking:

1. The no-Origin browser path. The Origin gate keys on header presence, and a top-level browser navigation (GET http://127.0.0.1:<port>/mcp from the address bar) sends no Origin header — it passes the Origin gate by design, then dies at the bearer gate, because navigations cannot carry Authorization. The missing-auth GET → 401 test covers that path precisely. The layering is correct — possession is the constant gate; Origin/Host kill the scriptable browser channels (fetch/XHR/form-POST all send Origin; DNS rebinding additionally trips the Host allowlist). Named so nobody later reads the navigation path as an Origin bypass — it is already accounted for.
2. localhost → ::1 client dialing. The listener binds IPv4 127.0.0.1 only; a consumer dialing http://localhost:<port> on a stack that resolves localhost to ::1 first will not connect. The in-repo surface is safe — createLocalBearerLaunchContract pins NEO_MCP_LISTEN_HOST: '127.0.0.1' — but #15187's journey client should dial the literal 127.0.0.1, never localhost. Downstream note for #15187, not an AC here.

I also actively checked and cleared: the ADR-0019 §3 catalog against every hunk (no module-level env re-derivation; no test-mode ternaries; no config-value exports; plain aiConfig.auth.* access without defensive ?.; no runtime AiConfig mutation — createLocalBearerLaunchContract returns a frozen env object for a child process and never touches process.env; whole-object aiConfig injection is the sanctioned entrypoint-DI shape; the helper imports only node crypto; the config spec extends the canonical template spec); base64url canonicality (43 chars × 6 bits = 258 bits → the two trailing bits are roundtrip-checked, so non-canonical final characters are rejected); and the timingSafeEqual preconditions (both operands decode-gated to exactly 32 bytes before the equal-length primitive runs — the fast-reject regex leaks only presented-token well-formedness, never configured-token content).

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — "pinned to IPv4 loopback, rejects browser-originated requests, preserves the SDK Host allowlist, authenticates one disposable possession credential without resolving an identity" is mechanically true, hunk by hunk.
• Anchor & Echo summaries: precise — "possession, not identity"; "process exit is the revocation boundary"; the Number.MAX_SAFE_INTEGER sentinel documented as SDK-required rather than dressed up as a TTL policy.
• Linked anchors: the #15187 boundary claim matches the ticket's own scope note; the --migrate-config reliance matches repository reality (one tracked SSOT plus the gitignored operator overlay).

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None observed.
• [TOOLING_GAP]: Author-disclosed, not PR-caused: the broad parallel unit run surfaced a cross-file boot race in McpServerListToolsSmoke (green isolated, 31/31) plus sandbox EPERM/shared-state failures. The new beforeEach state reset with its CI single-worker rationale is the right local mitigation; the MCP unit suites' shared-singleton sensitivity is a known, mineable seam.
• [RETROSPECTIVE]: Two reusable patterns worth graph weight. (1) The presence-discriminator Origin gate — rejecting on header presence rather than value is the correct browser-channel kill for a loopback trust contract, and it composes with (not substitutes for) the Host allowlist and possession auth: three independent guards, each with its own falsifier test. (2) The two-layer fail-closed credential contract — declarative requiredFor readiness at the config tier plus a canonical-format throw at the service tier — turns "non-empty at startup" into an enforced format contract with no hidden default. This PR is also friction→gold with a paper trail: the 2026-06-02 V-B-A that found the Origin gap is now closed substrate.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15185 (PR body); single commit 61f1d23ee with subject ending (#15185), no stale magic keywords.
• #15185 confirmed not epic-labeled (labels: enhancement, ai, architecture, security — a leaf of epic #15184); #15184/#15187 correctly non-closing Related:.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #15185 carries the Contract Ledger matrix.
• Diff matches the ledger row-by-row: mcpListenHost opt-in leaf, absent ⇒ the exact prior app.listen(port) arity preserved; local-bearer requires the literal 127.0.0.1 bind (setup-head throw; 'localhost' rejected, test-proven); Origin absent-accepted / present-rejected before wildcard CORS and MCP dispatch (middleware registered before cors.default; connectCalls === 0 proves pre-dispatch); Host allowlist independent (computeAllowedHosts untouched; valid-bearer + bad-Host → 403 test); 32-byte unpadded-base64url credential with constant-time equal-length compare; no logging/SQLite/file/durable-config writes anywhere in the helper or verifier (tests assert token non-observability in logs and error bodies).
• The declared strictness delta (canonical-format startup validation vs "non-empty") implements the ledger's own credential-format row at the fail-closed boundary — coherence, not drift.

Findings: Pass — no drift.

🪜 Evidence Audit

• PR body carries the greppable Evidence: L3 (…) → L3 required (…). No residuals. declaration.
• Achieved ≥ required: real socket bound to 127.0.0.1, authenticated SDK initialize 200 + session id, present-Origin/invalid-Host/invalid-bearer rejections, and the non-loopback interface probe — that is the close target's observable local-ingress surface.
• Two-ceiling distinction: explicitly drawn — the broad sandbox run's 11 failures are disclosed as environment (EPERM on protected paths, parallel shared-state, timeouts), with isolated CI-shape reruns green and exact-head CI green as the routine evidence.
• No evidence-class inflation.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

N/A — no openapi.yaml surface touched.

🔗 Cross-Skill Integration Audit

• mcp-config-template-change-guide.md is scoped to the four per-server config.template.mjs files; this PR changes the root SSOT ai/config.template.mjs, governed by ADR-0019 (gate read done). Applying the guide's principle anyway: changed keys are named in the PR body, --migrate-config follow-up is an explicit post-merge checkbox, and both new leaves are opt-in (null/'' defaults, no mode enabled by default) ⇒ zero live-behavior change for running clones; harness restart unnecessary.
• No existing skill documents a predecessor step that should fire this mode; consumer wiring is #15187's declared scope.
• No new MCP tool surface; no AGENTS_STARTUP.md impact.
• One non-blocking substrate observation: the template-change guide's scope list predates the ADR-0019 root-SSOT consolidation — the one file that now carries every server's config contract is the one file the guide doesn't cover. Worth a one-line scope amendment next time that guide is touched (friction→gold candidate, not a Required Action here).

Findings: No integration gaps blocking merge; one guide-scope gap noted above.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 61f1d23eea (unit, integration-unified, lint ×5, CodeQL, Analyze, lint-pr-body — all pass) + author non-CI receipts (scoped 4-spec run 60/60; isolated MCP smoke 31/31; lint-staged chain; structure-map) — current-head-appropriate.
• Reviewer falsifier: N/A — no named behavioral concern CI leaves open. I actively looked for an uncovered guard-ordering case (empty-string Origin — covered), an uncovered bind case ('localhost' startup rejection — covered), and an uncovered credential-shape case (padded / short / non-canonical — covered at both helper and middleware tiers) and found none.
• Reviewer-side ai:structure-map --files --loc run at the exact head in an isolated worktree: pass — ai/mcp/server/shared/helpers now holds deploymentConfig.mjs (44 LOC) + localBearer.mjs (39 LOC); placement idiomatic.
• Test location: canonical mirrors (test/playwright/unit/ai/..., including the new helpers/ node).

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — actively checked and cleared: mode-branch precedent fit (early-return shape mirrors gitlab-pat); the advertised-host vs listener-bind separation (mcpHttpHost/publicUrl semantics untouched, mcpListenHost a new orthogonal concern — exactly the boundary the ticket drew); SDK-recommended external-middleware placement for the Origin gate (the SDK's own transport-level origin options are deprecated); helper placement beside deploymentConfig.mjs; the full ADR-0019 catalog.
• [CONTENT_COMPLETENESS]: 100 — every new export and method carries Anchor & Echo JSDoc stating the why (revocation boundary, sentinel semantics, presence discriminator); the mode-selector doc and both class summaries updated; the PR body is a complete Fat Ticket whose Evolution section documents a reversed design decision (synthetic TTL → SDK sentinel) instead of hiding it.
• [EXECUTION_QUALITY]: 100 — actively checked and cleared: base64url canonicality roundtrip; timingSafeEqual equal-length preconditions; empty-Origin presence handling; guard independence at a real socket; CI single-worker state inheritance (the beforeEach reset with rationale); the non-loopback reachability probe with an honest environment skip.
• [PRODUCTIVITY]: 100 — all ticket ACs delivered at the required L3 evidence class with zero residuals.
• [IMPACT]: 80 — security-critical shared ingress that unblocks the local-launch epic (#15184) and the #15187 journey; scoped to the MCP shared services rather than engine-wide.
• [COMPLEXITY]: 70 — four production surfaces plus a real-socket harness; the cognitive load sits in the guard-ordering contract and the SDK boundary semantics, both of which the tests make legible.
• [EFFORT_PROFILE]: Heavy Lift — a security-critical multi-surface change held to a consumed-boundary evidence bar.

Clean work. The three-guard ingress — Host allowlist ∥ Origin presence ∥ possession bearer, each independently falsified at a real seam — is the strongest security-shape evidence I've reviewed on this substrate, and the honest broad-run disclosure is exactly the reporting culture we want.

Reviewed by Clio (Claude Fable 5, Claude Code). Session fc11bd7a-b599-46f1-b635-bdd1cd3b7a5e.