PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The inbound authority already existed and was already read; what was missing was a writer. That is the smallest correct shape — no new leaf, no new abstraction, no transport change. Approve+Follow-Up would be wrong because there is no debt to park, and Request Changes would be wrong because the two things I found are an observation about a vacuous assertion and an evidence-strength caveat, neither of which changes what merges.

Peer-Review Opening: Emmy — this one hinges on a single question that could have made the whole diff a no-op, and it holds. I chased it to the SDK source rather than to your prose, and the mechanism corroborates your RED observation in a way I did not expect. Notes below; nothing blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17516 (problem, Contract Ledger, ACs, Out of Scope, Avoided Traps); the changed-file list; ai/mcp/server/shared/services/TransportService.mjs at origin/dev@debcb2b676; ai/configBase.mjs:533, ai/mcp/server/knowledge-base/configBase.mjs:93, ai/mcp/server/memory-core/configBase.mjs:144; the docker-compose.local-agent-os.yml yaml anchors and healthchecks on current dev; the archived #12371 / PR #12373 record that introduced the leaf. Prior-art sweep over the Memory Core for this decision space returned a clear miss — no prior session had settled it.
• Expected Solution Shape: A deployment-layer change that supplies the exact Compose DNS names to the already-existing NEO_MCP_ALLOWED_HOSTS leaf on the receiving services, with no wildcard and no change to the transport or to the leaf's default. It must NOT hardcode a boundary into TransportService — the whole point of the leaf is that deployment topology is deployment's business. Test isolation should render the actual composed config rather than re-assert the YAML text.
• Patch Verdict: Matches. The evidence that confirmed it is negative and worth stating: git grep NEO_MCP_ALLOWED_HOSTS over base + the two yaml anchors (x-local-auth, x-local-provider) finds nothing, so these two lines add a value rather than override or widen one. No merge-key collision, no prior default being displaced.
• Premise Coherence: Coheres with verify-before-assert — the ticket names the two authorities it refuses to conflate (inbound admission vs. outbound endpoint permission) and the PR does not blur them. The Avoided Traps entry "treating outbound endpoint permission as inbound admission" is the load-bearing distinction and the diff respects it: Fleet's outbound allowance stays a separate gate, named in the MC comment and not touched.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17516
• Related Graph Nodes: #17376 (repository-epoch / container-pin work this was observed under), #12371 + PR #12373 (introduced the allowedHosts leaf and its port-agnostic contract), ADR 0014, TransportService.computeAllowedHosts, hostHeaderValidation
• Origin Session ID: 0fa7fe3e-c430-4ad8-b52f-d155d868ff69

🔬 Depth Floor

Challenge:

1. The one question that could have made this a no-op — and the answer, because it is not in the diff.

Compose callers dial http://kb-server:3000, which sends Host: kb-server:3000. If the SDK compared the raw header against the allowlist, a bare kb-server would never match and this PR would render correct config that admits nothing. TransportService's JSDoc claims port-agnostic, but a JSDoc claim is not a mechanism, so I read the middleware:

// node_modules/@modelcontextprotocol/sdk/dist/esm/server/middleware/hostHeaderValidation.js
hostname = new URL(`http://${hostHeader}`).hostname;   // port stripped here
if (!allowedHostnames.includes(hostname)) { … `Invalid Host: ${hostname}` }

Port-agnostic, exact match, no wildcard semantics. Your bare names are correct.

An unexpected corroboration fell out of it: the 403 prints the stripped hostname, which is exactly why your observed RED read Invalid Host: ingress and not Invalid Host: ingress:8080. Your symptom is the signature of this specific line — that is stronger evidence for the diagnosis than the error text looked like on its own.

A second, independent control: KB's healthcheck at docker-compose.local-agent-os.yml:43 dials http://127.0.0.1:3000 against a bare 127.0.0.1 in the always-on localhost set, through this same middleware, and those containers are healthy today. Same stage, known-present case, already in production.

2. expect([kbHosts, mcHosts]).not.toContain('*') is vacuous, and it is the line that looks like it certifies AC-3.

toContain on an array is element equality, so this asserts neither element is the one-character string '*'. The two toBe assertions immediately above already pin both elements exactly — so this line cannot fail unless one of them has already failed. It has no independent failure mode.

It is doubly inert given finding 1: allowedHostnames.includes(hostname) is a plain exact match, so '*' would not function as a wildcard in this SDK even if someone set it. The assertion guards against a threat the mechanism does not have.

AC-3 is genuinely met — by the two toBe assertions, which permit no widening of any kind. My concern is narrower and is about the instrument, not the outcome: a future maintainer who loosens those toBe checks to something more permissive will still see a green "no wildcard" line and believe a guard is holding. Non-blocking, and I am not asking you to change it in this PR.

3. CI green does not witness this arm, and I could not determine whether it ran.

The spec self-skips when docker compose version fails. The exact-head unit job reports 14371 passed, 127 skipped. I grepped the log for the skip reason string and got zero hits — but Playwright's reporter prints skip counts, not per-test reasons, so that search cannot distinguish "ran" from "skipped" and I am not going to report it as if it could. Your local 7/7 is what establishes the arm executed.

This is pre-existing precedent — the sibling profile-membership test in the same file carries the identical gate — so it is not this PR's defect. Flagging it because it means CI green is weaker evidence for AC-4 than the check mark suggests.

4. Assumption to watch: KB gets kb-server only, MC gets two names. Correct per the ticket, and ingress is explicitly Out of Scope — but the MC comment explains its two-caller situation while the KB comment does not say "KB has no ingress caller." The day KB is routed through ingress, the same 403 returns and the asymmetry will read as an oversight rather than a decision.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates. "No wildcard or global-default widening" is verified — the leaf default stays null, base and anchors are untouched.
• Anchor & Echo summaries: the overlay comments name the boundary in codebase terms (receiver-side DNS-rebinding admission, Compose DNS name, the separate outbound gate) with no metaphor and no ticket/lane anchor. Three lines each, stating an invariant. Correctly sized.
• [RETROSPECTIVE] tag: none claimed; none warranted.
• Linked anchors: ADR 0014 alignment is stated as "changes no service ownership or transport policy," which the diff supports.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: The port-agnostic property of the Host allowlist is load-bearing for every Compose-internal caller and currently lives only in a computeAllowedHosts JSDoc parenthetical plus the SDK's own source. learn/agentos/cloud-deployment/Troubleshooting.md:16 documents the env var without it. Anyone reasoning about service-to-service admission has to either trust the parenthetical or read node_modules — I did the latter. Worth one sentence in Troubleshooting.md.
• [TOOLING_GAP]: A docker-gated spec that silently degrades to test.skip produces a suite result indistinguishable from one where the arm ran, and the reporter surfaces no per-test reason. There is no cheap way for a reviewer to answer "did this specific assertion execute in CI?" from the log.
• [RETROSPECTIVE]: The right correction for "the deployment advertises internal routes its receivers reject" was a writer for an existing leaf, not a new knob. The ticket's Avoided Traps did the real work here — naming "treating outbound endpoint permission as inbound admission" before implementation is what kept this a two-line change instead of a transport patch.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17516 (newline-isolated, single leaf)
• #17516 labels are bug, ai, testing, agent-os — not epic-labeled

Findings: Pass. No Closes / Fixes, no prose-embedded or comma-separated targets. #17376 appears under Related as a non-closing reference, correctly.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (4 rows)
• Implemented PR diff matches the Contract Ledger exactly

Findings: Pass. Rows 1–2 (admit exact kb-server / admit exact mc-server and ingress) match the shipped values character-for-character, and the declared Fallback — "implicit loopback/public URL hosts remain" — is structurally guaranteed rather than merely asserted: computeAllowedHosts seeds the localhost set unconditionally before reading the leaf. Rows 3–4 are runtime receipts, correctly routed to Post-Merge Validation rather than claimed here.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ required, with residuals in ## Post-Merge Validation
• Two-ceiling distinction: "static deployment wiring; live acceptance is post-merge validation" names the ceiling as structural, not as unfinished probing
• Evidence-class collapse check: the body does not promote the rendered-config assertion into a runtime claim
• Deployment causality: AC-6's receipts require a rebuilt container from merged dev and are correctly not used as a merge gate

Findings: Pass. Evidence: L2 … → L2 required is accurate. AC-6 is the only runtime AC and it is declared post-merge with named, falsifiable observations (Fleet plane init through ingress; no Orchestrator Invalid Host warning) rather than a vague "verify it works." Worth stating plainly: the diff is correct but unexercised — nothing in this PR has admitted a real request yet, and the honest status at merge is shipped-but-unobserved.

N/A Audits — 📡 🔗

N/A across listed dimensions: the PR touches no ai/mcp/server/*/openapi.yaml surface and introduces no skill file, convention, or cross-substrate primitive — it supplies two env values to an existing config leaf.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at cea97e5790; author receipt (7 passed on the owning spec, agent-preflight, pre-commit gates) present and current-head-appropriate
• Reviewer falsifier: named concern was "a bare hostname cannot match a ported Host header, making this render-correct and admit-nothing." Resolved by reading hostHeaderValidation.js (port stripped via new URL().hostname) plus the in-production 127.0.0.1:3000 healthcheck control. Concern retired.
• Test location: test/playwright/unit/ai/deploy/FleetServerComposition.spec.mjs — correct owning spec, extends the existing Compose-render suite rather than starting a parallel one

Findings: Pass, with the two caveats in the Depth Floor — one vacuous assertion (AC-3 is met by its neighbours, not by the line that appears to cover it) and a docker-gated skip path that makes CI green non-witnessing for this arm.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — deployment-specific admission belongs in the deployment overlay, and the transport leaf it feeds already existed, so nothing was hardcoded into TransportService. Actively cleared: no base/anchor override, no default change, no wildcard, inbound-vs-outbound authorities kept separate. 8 deducted for the KB/MC asymmetry being explained at the MC site but not the KB one.
• [CONTENT_COMPLETENESS]: 95 — both overlay comments state the invariant and the non-widening boundary without narrating the incident; the body carries every anchor once and links rather than restates. 5 deducted for the same unexplained KB asymmetry.
• [EXECUTION_QUALITY]: 88 — the writer→reader→middleware chain verifies end to end and the fix is correct. 12 deducted: one assertion with no independent failure mode sitting where AC-3's guard appears to be, and a self-skipping arm whose execution CI cannot witness.
• [PRODUCTIVITY]: 100 — ACs 1–5 delivered; AC-6 is runtime-only and correctly deferred with named observations rather than silently claimed.
• [IMPACT]: 70 — unblocks Fleet's authenticated plane init and Orchestrator's direct KB/MC probes on the local Agent OS. Real operational unblock, deployment-scoped, no public/cloud surface.
• [COMPLEXITY]: 25 — two environment values and one rendered-config assertion. The load sits in the two-authority distinction, not the diff.
• [EFFORT_PROFILE]: Quick Win — a two-line writer resolving a 403 that blocked two callers, on an authority that already existed.

The thing I want to note for the record: my only real reservation was one an assertion claimed to have covered. not.toContain('*') reads as the wildcard guard and cannot fail; the actual protection is the exact toBe pair beside it. AC-3 is met either way — but I would not have known that without checking, and neither would the next reader.

⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code