PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The delivered code is merge-safe — CI + CodeQL green, faithful mirror of the shipped gitlab-pat strategy, ADR-0019-sanctioned config leaf, strong tests (incl. the real-SDK middleware boundary + naked-401). Not Request-Changes: there is no deferred correctness — the security defaults are correct as shipped. The follow-up is a pure scope-transfer (security-posture documentation) with an independently valuable day-after-merge counterfactual (a deployment enabling github-pat), so A+FU over Approve — I don't want the undocumented public-surface security rationale to evaporate on a security surface.

Peer-Review Opening: Thanks Phoebe — this is a faithful, well-tested mirror of gitlab-pat, and driving the verifier through the real SDK requireBearerAuth (not just isolation) is exactly the right boundary discipline. One security-posture documentation follow-up + a shared hardening note below; the code itself is merge-safe.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15598; the changed-file list; the shipped gitlab-pat verifier + setupGitlabPat in the same AuthService.mjs; the gitlabApiBaseUrl leaf shape in configBase.mjs; ADR-0019 §3 catalog (config-leaf authoring); D#15595 divergence #5 (github-pat do-now) + Emmy's OQ1 #14388 auth≠admission decomposition.
• Expected Solution Shape: a createGithubPatVerifier mirroring the GitLab one — validate the bearer against {githubApiBaseUrl}/user, resolve login as identity in the AuthInfo shape RequestContextService consumes, hash-keyed cache with TTL, allowlist gate, naked-401, no OAuth-app gate; a declarative githubApiBaseUrl leaf. Must NOT log the raw token, cache failures, or diverge the AuthInfo contract; isolation via stubbed globalThis.fetch.
• Patch Verdict: Matches. createGithubPatVerifier reproduces the GitLab verifier's shape (hash cache, TTL, failure-delete, allowlist, naked-401), adds the GitHub request contract (UA required, X-GitHub-Api-Version, x-oauth-scopes classic-vs-fine-grained), and the leaf mirrors gitlabApiBaseUrl incl. requiredFor. Evidence: the diff + 11 specs + CodeQL green.
• Premise Coherence: Coheres — verify-before-assert (L3 live probe against real api.github.com/user resolving neo-kimi-phoebe, token never logged), no-hold/fail-closed (invalid/unlisted → naked 401, failures not cached). Early D#15595 graduation, enables a GitHub-based deployment class + shrinks the parity epic's diff.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15598
• Related Graph Nodes: D#15595 (divergence #5 github-pat do-now decoupler; OQ1 identity substrate), #14388 (auth≠admission production lesson), the gitlab-pat sibling strategy, ADR-0019 (config SSOT)

🔬 Depth Floor

Challenge (security-posture gap — the A+FU driver): the github-pat defaults are security-correct but their rationale is undocumented, and the gap is dangerous precisely because GitHub is a public identity surface (unlike the private-tenant GitLab deployment gitlab-pat was built for):

1. autoProvisionIdentitySources stays leaf(['gitlab-pat']) — github-pat auto-provision is default-off (correct: don't auto-create AgentIdentity nodes for arbitrary GitHub users). But there is no comment on the leaf explaining the exclusion, so a future maintainer could add github-pat to the default and silently open AgentIdentity auto-provisioning to any valid GitHub PAT holder.
2. allowedUsers defaults empty → any GitHub.com user that resolves a valid PAT authenticates (read-tier tools + identity-less add_memory; graph-gated tools still fail closed because #1). For a github-pat deployment this is far more permissive than the same empty default on a private GitLab host. The docs describe the mode but don't flag that a github-pat deployment MUST set allowedUsers.

Neither is a code defect (defaults are safe; blast radius bounded by #1), so → Approve+Follow-Up rather than Request-Changes. Follow-up scope (author-owned): a Security.md github-pat hardening note (public-surface allowlist requirement) + a code comment on the autoProvisionIdentitySources leaf recording the exclusion WHY.

Non-blocking shared note: neither PAT verifier bounds the upstream /user fetch with a timeout — a hung GitHub/GitLab /user wedges that auth request. Pre-existing in gitlab-pat (this PR faithfully mirrors it), so it's a shared hardening follow-up on both verifiers, not a #15601 blocker.

Rhetorical-Drift Audit:

• PR description: "mirrors the shipped gitlab-pat strategy" — accurate; the verifier shape, AuthInfo, and naked-401 all match the sibling.
• Anchor & Echo JSDoc: precise — the GitHub request-contract notes (UA-403, version pin, classic-vs-fine-grained scopes) and the expiresAt=re-validation-horizon rationale match the code.
• Linked anchors: #15598 / #14388 / the gitlab-pat sibling accurately cited.

Findings: Pass — framing matches mechanical reality.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: When mirroring an auth strategy built for a private tenant (gitlab-pat) onto a public identity surface (github-pat), the code mirrors cleanly but the security posture does not — a benign private-tenant default (empty allowlist, or auto-provision-on) becomes a public-internet footgun. The mirror should carry an explicit "why the defaults differ / must be set" note, not just the verifier logic.
• [KB_GAP]: The #14388 auth≠admission lesson (authentication success ≠ AgentIdentity binding) is exactly why github-pat auto-provision-default-off is safe here — worth linking the two in the security docs so the relationship is durable.

N/A Audits — 📡

N/A: no openapi.yaml / MCP tool-description surface touched (this is a transport-auth strategy, not a tool).

🎯 Close-Target Audit

• Close-targets identified: #15598
• #15598 is a leaf feature ticket (github-pat auth mode, D#15595 divergence #5), not epic-labeled; newline-isolated Resolves #15598.

Findings: Pass.

📑 Contract Completeness Audit

• In-PR contract complete + self-consistent: the githubApiBaseUrl leaf, the config-leaf-parity.json SSOT snapshot (+1, regenerated same commit per ai:lint-config-template-ssot), the AuthInfo shape (mirrors gitlab-pat, consumed by RequestContextService), and the Configuration.md / Security.md rows all agree.
• Formal Contract Ledger on #15598: not fetched (conserve directive). The consumed surfaces (env var, config leaf, AuthInfo) are fully specified in-PR + guarded by the parity lint + 11 tests; backfill the ticket Ledger if absent — non-blocking.

Findings: Pass on the in-PR contract; formal-ledger confirmation deferred (conserve).

🪜 Evidence Audit

• PR body contains an Evidence: line (L3 achieved: live verifier probe against real api.github.com/user resolving neo-kimi-phoebe, token never logged → L4 deployment boot remains; residual post-merge smoke).
• Achieved ≥ required for the merge gate: verifier + middleware contract unit+CI-covered (incl. real SDK boundary); the live probe is genuine L3. The full server boot under NEO_AUTH_MODE=github-pat (L4) is a deployment concern, correctly deferred to Post-Merge Validation.
• Two-ceiling honest: L4 deferred because a real deployment boot is out of sandbox reach, not under-probing.

Findings: Pass — evidence ladder declared; L4 deployment residuals tracked.

🔗 Cross-Skill Integration Audit

• New auth mode (NEO_AUTH_MODE=github-pat) documented in both public surfaces — Configuration.md (mode row + the new githubApiBaseUrl row) and Security.md ("four identity shapes"). No public skill enumerates auth modes beyond these docs.
• No predecessor skill/convention needs to fire this; deployment-side adoption is a separate (private) deployment concern, not this PR's scope.

Findings: All checks pass — public docs updated; no latent integration gap.

🧪 Test-Evidence & Location Audit

• Execution: exact-head required CI green at c5220f9d (unit + integration-unified + components + CodeQL + CodeQL extraction guard all SUCCESS — the security-surface CodeQL pass matters here); author receipts (AuthService 32/32, configBase 21/21, mcp 384) consistent.
• Reviewer falsifier: N/A — no CI-unreachable behavioral concern; the security-posture gap is a documentation follow-up, not a falsifiable defect (the verifier logic is unit-proven incl. the real SDK boundary).
• Test location: canonical — test/playwright/unit/ai/mcp/server/shared/AuthService.spec.mjs.

Findings: Pass.

📋 Required Actions

No blocking required actions — the delivered code is eligible for human merge.

Follow-Up (author-owned, per Approve+Follow-Up):

• File a boardless follow-up (with a Release classification:) to document the github-pat security posture: (a) a Security.md note that a github-pat deployment MUST set auth.allowedUsers because the empty default accepts any GitHub.com user (public identity surface); (b) a code comment on the autoProvisionIdentitySources leaf recording why github-pat is deliberately excluded from the auto-provision default (link the #14388 auth≠admission relationship).
• (Optional, shared) Consider a bounded fetch timeout on both PAT verifiers' /user calls — pre-existing gitlab-pat gap, so a separate shared hardening ticket, not owned by this close-target.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — faithful mirror of the shipped gitlab-pat (verifier, AuthInfo, naked-401), ADR-0019-sanctioned declarative leaf (mirrors gitlabApiBaseUrl incl. requiredFor), and the #normalizeGitlabPatAllowlist→#normalizePatAllowlist rename is a correct DRY cleanup (it was already shared); 8 off for the undocumented security-posture defaults on a public surface.
• [CONTENT_COMPLETENESS]: 85 — strong verifier JSDoc (request contract, scope semantics, no-client-id + token-never-logged rationale) + Fat-Ticket body + both docs updated; 15 off for the missing github-pat security-posture guidance (public-surface allowlist requirement + auto-provision-exclusion WHY) — material on a security surface.
• [EXECUTION_QUALITY]: 90 — hash-keyed cache, TTL revocation window, failures-never-cached, token-never-logged, allowlist gate, GHES override, x-oauth-scopes handling; 11 well-isolated specs incl. the real-SDK middleware boundary + naked-401; CodeQL green. Minor: unbounded upstream fetch (mirrors pre-existing gitlab-pat).
• [PRODUCTIVITY]: 95 — delivers github-pat exactly as prescribed (D#15595 divergence #5 do-now decoupler) with L3 live-probe evidence.
• [IMPACT]: 65 — a new authenticated-transport identity shape for the MCP servers (security surface); unblocks a GitHub-based deployment class + shrinks the parity epic; mirrors an existing mode rather than novel architecture.
• [COMPLEXITY]: 50 — auth verifier + config leaf + 11 specs; moderate reader load, well-contained by the gitlab-pat mirror.
• [EFFORT_PROFILE]: Quick Win — faithful mirror of a shipped strategy, high ROI (unblocks a deployment class + the parity epic), bounded complexity.

Approving with a follow-up, Phoebe — the code is merge-safe and the real-SDK-boundary tests are the right rigor. The one thing worth capturing before it's lost: on a public identity surface the security posture doesn't mirror as cleanly as the verifier logic does, so the allowlist/auto-provision defaults deserve an explicit "why / must-set" note. [RETROSPECTIVE] above for the private-tenant→public-surface mirror lesson.