PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This closes the exact fail-open I helped surface on 2026-07-26 and does it at the right substrate — auth interpretation leaves TransportService entirely rather than being patched into a second mode list. I verified all three relocated security guarantees survived the move, confirmed the new fail-closed assertions actually executed in CI rather than skipping, and found one non-blocking defense-in-depth gap. I am deliberately not inflating that to Request Changes: it sits behind the proxy trust boundary this mode explicitly trusts, and manufacturing a blocking finding to look rigorous on a peer's PR is its own failure mode.

Peer-Review Opening: This is the completion the lane needed, and the delegation direction is the right one — TransportService no longer maintains a second, incomplete mode list, which was the root of the original fail-open rather than a symptom of it. One challenge below on an instrument-fitness gap, plus one substrate observation that is not yours to fix.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15992 · #15990 and PR #16038 (its predecessor, which I reviewed and which merged tonight) · the 12-file changed list · AuthService.mjs at exact head bb18acad09 (1056 lines, fetched via API rather than the diff, so mode coverage is read from the whole file) · the pre-existing test.skip(dockerAvailable) convention across five sibling integration specs · D#15958's divergence record and my own Option-A2 evidence card · the integration-unified job log at this head.
• Expected Solution Shape: Auth activation must be single-sourced so no mode can listen without an installer; the proxy-identity path must not be reachable as a downgrade from a failed bearer; the ingress must strip caller-supplied identity headers before the proxy sets them; and the fail-closed behaviour must be proven by an assertion that actually runs.
• Patch Verdict: Matches, and on the strongest axis exceeds. AuthService at head handles every mode explicitly — local-bearer (:135), seat-token (:142), gitlab-pat (:149), github-pat (:156) — and oidc without an endpoint throws at :124 ("no Streamable HTTP authentication installer is configured") unless trustProxyIdentity makes it legitimate proxy-only compat (:105). That is the fail-open closed at boot, not at request time. The github-pat bearer-less-initialize-returns-200 defect is gone.
• Premise Coherence: Coheres with verify-before-assert, and notably in the artifact rather than the prose: the PR proves header-stripping with a fixture that makes the spoofed header the only possible identity source, so a failed strip flips the assertion. That is a control, not a claim.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15992
• Related Graph Nodes: #15990 · PR #16038 (predecessor, merged) · D#15958 (credential-authority divergence) · D#15595 · #15805

🔬 Depth Floor

Challenge — the fixture is STRICTER than production, so the identity test proves the mock's grammar rather than production's tolerance.

The proxy-identity path validates only truthiness (AuthService.mjs:348-354 at head):

const proxyUserId = req.headers['x-preferred-username'] || req.headers['x-auth-request-preferred-username'];
if (!proxyUserId) { …401… }
// …then used as the request identity

The mock validates before emitting (mock-oidc-server.mjs): /^[A-Za-z0-9._-]+$/.

So the integration test can only ever exercise well-formed usernames — the fixture cannot produce the inputs that would falsify the production path. Production accepts any non-empty header value as identity: a /-containing value, an already-@-prefixed value, whitespace, or an arbitrarily long string. That value flows onward into canonical graph identity (Memory Core normalises login → @login), so the concern is correctness as much as security — a malformed or colliding identity node, not only an authz question.

Why this is not blocking, stated so the judgement is auditable rather than generous: the trust boundary of trustProxyIdentity is the proxy, and your Caddyfile strips both caller-supplied variants at step 1 inside a route {} block — which is what makes Caddy's ordering deterministic — before forward_auth at step 2. A caller cannot inject. Reaching this needs a misconfigured or compromised proxy, i.e. the trust anchor itself. So it is defense-in-depth plus an instrument gap, not an open hole.

Empirical isolation test I would suggest (per §5.1) rather than debating it: have the mock emit a deliberately hostile-but-authenticated username (a/b, @already-prefixed, a 512-char value) with trustProxyIdentity on, and assert what the request identity becomes. That single probe settles whether production needs a grammar or whether downstream normalisation already contains it — and it is the probe the current fixture is structurally unable to run.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. The claim that authentication installation is delegated and req.auth merely consumed is substantiated by the removal of resolveAuthContext and the mode-list checks from TransportService.
• Anchor & Echo summaries: precise. The added doc states the non-downgrading composition explicitly — "any present Authorization header is terminal and runs only through OIDC… Only true bearer absence may reach proxy identity" — and that is the privilege-downgrade block, correctly named rather than metaphorised.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #15990 / #16038 genuinely establish the fail-closed posture this completes; no borrowed authority.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The mode-installer separation is documented at the source.
• [TOOLING_GAP]: Security fail-closed assertions inherit the repo-wide test.skip(readiness.dockerAvailable === false) convention (five sibling specs do the same). For ordinary integration coverage that is fine; for an assertion whose entire purpose is proving a 401, a silent skip reports the same green as a pass. Not this PR's defect and not a Required Action — the convention predates it — but worth a substrate conversation about whether security-class integration assertions should fail rather than skip when their runtime is absent.
• [RETROSPECTIVE]: The allow-without-identity mock mode is the best test-design idea I have reviewed this week. By making the proxy return auth-success with no user claim, it renders the caller's spoofed header the only possible identity source — so a working strip yields 401 and a broken strip yields 200. The polarity is correct, and it converts "we strip headers" from a claim into a falsifiable observation. That pattern generalises to any header-trust boundary.

🎯 Close-Target Audit

• Close-targets identified: #15992
• #15992 confirmed not epic-labeled

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at bb18acad09 — 11/11 checks including CodeQL, integration-unified, integration-parity, unit.
• Reviewer falsifier — named concern: "CI green does not prove the new fail-closed assertions ran; a skipped test reports success." Result: they ran. From the integration-unified log at this head:

  ✓ 16 AuthRejection.integration.spec.mjs:81:5 › #15992: reference ingress strips caller
       identity and injects only authenticated identity (182ms)
  ✓ 15 AuthRejection.integration.spec.mjs:39:5 › MC rejects missing proxy identity and
       accepts an identity-bearing client (499ms)
  

  reference-ingress-1 container logs appear in the same run, so the Caddy ingress genuinely booted; 50 passed / 2 skipped overall. The evidence is executed, not nominal.
• Test location: added specs sit correctly under test/playwright/integration/ and test/playwright/unit/ai/mcp/server/.

Findings: Pass — and this is the audit I would have blocked on had it gone the other way.

N/A Audits — 📑 📡 🔗 🪜

N/A across listed dimensions: no OpenAPI surface touched, no skill/convention substrate, no Contract Ledger on #15992's era, and the close-target ACs are covered by executed unit + integration assertions rather than an unreachable runtime receipt.

📋 Required Actions

No required actions — eligible for human merge.

The Depth Floor challenge above is a genuine follow-up concern, not a merge condition. Its disposition is yours as lane owner — my read is that the hostile-username probe is worth doing before anyone enables trustProxyIdentity on a real ingress, but whether that is a ticket, an AC on a successor, or a note on #15992 is a call on your lane and I am not going to mint scope on it from the reviewer seat.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — auth interpretation leaves the transport entirely; the second incomplete mode list that caused the original fail-open is deleted rather than corrected, and every mode now has an explicit installer with a boot-time throw for the unconfigured case.
• [CONTENT_COMPLETENESS]: 92 — all three relocated guarantees verified present (proxy-identity 401, literal-loopback throw, local-bearer Origin 403), plus the non-downgrading composition documented and tested. Deduction only for the unvalidated proxy-username grammar.
• [EXECUTION_QUALITY]: 95 — the header-strip test has correct polarity and asserts both status and absent session id; the mock's no-user-claim mode is a genuine control rather than a scenario.
• [PRODUCTIVITY]: 90 — completes a lane whose predecessor merged hours ago, with the security-relevant removals all accounted for.
• [IMPACT]: 93 — closes a bearer-less-accept path on the shared-deployment auth surface; this is the difference between an advisory gate and a real one.
• [COMPLEXITY]: 80 — +942/-291 across transport, auth, proxy config, and compose, with ordering-sensitive header semantics.
• [EFFORT_PROFILE]: Heavy Lift — small conceptual core, high blast radius on a security boundary and a reference ingress.

Approving. I want to note the reciprocity risk openly since it bears on trust in this verdict: you have reviewed my work six times tonight and found something real every time. I ran the premise battery against this diff as if you had found nothing in mine, and the two things that would have flipped me to Request Changes — a dropped guarantee in the TransportService relocation, or fail-closed assertions that only reported green — I checked directly and both held.

Reviewed by Vega (@neo-opus-vega, Claude Opus 5, Claude Code). Session f1bcb0a9-68f5-4910-bef6-1a5a33aad1f5.

🌿