feat(mint): cross-org authorization and e2e WIF auth (#2155)

[ifireball]

Summary

• Add cross-org mint authorization via target-org FULLSEND_FOREIGN_<role>_REPOS (ADR 0054); merged upstream/main (ADR 0051–0053 functional eval harness)
• Replace Playwright/session e2e auth with OIDC → mint (role=e2e, target_org) in CI; local runs use gh auth login / GH_TOKEN
• Add fullsend admin foreign allow|list|revoke, update pool setup script, and remove legacy PAT cleanup / Playwright Makefile targets

Intentional permission expansion (e2e role)

The new e2e GitHub App role has broad permissions (administration:write, organization_administration:write, members:write, secrets:write, actions_variables:write, organization_actions_variables:write, etc.). This is deliberate for full admin install/uninstall e2e on dedicated pool orgs only.

Scope is constrained by:

1. WIF/OIDC enrollment — only enrolled workflows with valid OIDC claims can call the mint
2. Cross-org FOREIGN allowlist — pool orgs must set FULLSEND_FOREIGN_E2E_REPOS to authorize CI callers (fullsend-ai/fullsend)
3. Pool org isolation — tests run against halfsend-01…06, not production orgs

Installation-wide tokens: repos may be omitted on both same-org and cross-org mint paths (symmetric). Cross-org still requires FOREIGN authorization; same-org relies on WIF trust in the calling workflow. This supports e2e creating repos (e2e-lock, .fullsend) at runtime without predeclaring them in the mint request.

Test plan

• go test ./internal/mintcore/... — same-org + cross-org foreign allowlist, empty-repos installation-wide
• go test ./internal/cli/... — foreign subcommands
• go test ./... — full suite
• CI e2e green (E2E_MINT_URL, pool orgs with e2e app + FOREIGN allow)
• scripts/check-e2e-authorization-test.sh

Operator follow-up

Pool orgs (halfsend-01…06) — one-time if not already done:

fullsend admin foreign allow --org halfsend-NN --role e2e --caller fullsend-ai/fullsend

Ensure fullsend-ai-e2e app installations include Variables write (repo + org) on each pool org.

Remove obsolete repo secrets after stable CI: E2E_GITHUB_SESSION, E2E_GITHUB_PASSWORD, E2E_GITHUB_TOTP_SECRET.

Note: .github/workflows/e2e.yml changes may require protected-path maintainer approval.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:33 PM UTC · Completed 5:50 PM UTC
Commit: 67d5469 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [race condition / error propagation] internal/mintcore/handler.go — loadForeignAllowlist does not cache errors from fetchForeignAllowlist. On success the result is cached with 60s TTL, but on failure no cache entry is written. The inflight sync.WaitGroup deduplication prevents thundering herd for concurrent requests, but sequential requests after an error will each re-fetch from GitHub. ADR 0060 states "Cache entries include empty/missing allowlists (negative cache)" but the implementation only caches on success (if err == nil { h.foreignCache[key] = ... }).
  Remediation: Cache error results with a shorter TTL (e.g., 5–10s) to prevent retry storms during transient GitHub API failures.

• [protected-path] .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml (deleted), AGENTS.md, skills/e2e-health/SKILL.md — These are protected paths that require human approval. The PR links to issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 and the description explains the rationale (mint/OIDC migration replacing Playwright browser sessions).

Low

• [permission-expansion] internal/mintcore/github.go:69 — The e2e role permissions are expanded: actions_variables upgraded from read to write, and organization_actions_variables: write is added. ADR 0060 documents the rationale (pool tests set repo/org variables during install flows). Ensure the e2e App is only installed on pool orgs, not production orgs.

• [authorization-scope] internal/mintcore/handler.go — The repos-required validation is removed, allowing OIDC-authenticated callers to request installation-wide tokens (empty repos) for any enrolled role. ADR 0060 documents this as intentional: same-org elevation relies on WIF/OIDC enrollment, cross-org is gated by the FOREIGN allowlist.

• [logic error] e2e/admin/cleanup.go — In listOrgRepoNames, the HTTP response body is decoded via json.NewDecoder(resp.Body).Decode(&batch) before resp.StatusCode is checked. On non-200 responses, the decode may silently produce an empty batch. Move the status code check before the JSON decode.

• [stale dependency] go.mod — The PR deletes all Go source files that import playwright-go and pquerna/otp/totp, but go.mod and go.sum are not updated. Run go mod tidy to remove orphaned dependencies.

• [cache-revocation-delay] internal/mintcore/handler.go — Foreign allowlist cache uses 60s TTL. A revoked FOREIGN variable may still grant access for up to one TTL window. ADR 0060 documents this as an accepted consequence.

Previous run

Review

Findings

Medium

• [race condition / error propagation] internal/mintcore/handler.go — loadForeignAllowlist does not cache errors from fetchForeignAllowlist. On success the result is cached with 60s TTL, but on failure no cache entry is written. The inflight sync.WaitGroup deduplication prevents thundering herd for concurrent requests, but sequential requests after an error will each re-fetch from GitHub. If the target org's app is misconfigured (e.g., not installed, variable unreadable), every cross-org mint request triggers a fresh GitHub API call. ADR 0059 states "Cache entries include empty/missing allowlists (negative cache)" but the implementation only caches on success (if err == nil { h.foreignCache[key] = ... }).
  Remediation: Cache error results with TTL (negative cache), consistent with ADR 0059's stated design.

• [permission-expansion] internal/mintcore/github.go:67 — The e2e role permissions are expanded: actions_variables upgraded from read to write, and organization_actions_variables: write is added. This grants the e2e App write access to all repo-level and org-level Actions variables on any org where the App is installed. ADR 0059 documents the rationale (pool tests set repo/org variables during install flows). Ensure the e2e App is only installed on pool orgs, not production orgs.

• [protected-path] .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml (deleted), AGENTS.md, skills/e2e-health/SKILL.md — These are protected paths that require human approval. The PR links to issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 and the description explains the rationale (mint/OIDC migration replacing Playwright browser sessions).

Low

• [logic error] e2e/admin/cleanup.go — In listOrgRepoNames, the HTTP response status code is checked after json.NewDecoder(resp.Body).Decode(&batch). On non-200 responses, the decode may fail with a misleading error or silently produce an empty batch. Move the resp.StatusCode check before the JSON decode.

• [scope-alignment] e2e/admin/admin_test.go:639 — Triage test updated to apply ready-for-triage label in a follow-up call after issue creation, referencing issue dispatch: retro-filed proposal issues skip auto-triage under ADR 0054 authorization gate #2636 and ADR 0054 (bot-authored issues skip issues.opened dispatch). This change is orthogonal to the mint migration but bundled in the same PR. Consider filing a follow-on issue to track this workaround separately.

• [code-organization] internal/cli/github_client.go — New file contains only one 17-line helper function newGitHubLiveClient. Single-function files are unusual in this codebase. Consider moving into foreign.go or admin.go.

Previous run (2)

Review

Findings

Medium

• [fail-open] internal/mintcore/handler.go:215 — Cross-org mint requests with empty repos produce installation-wide tokens granting full role permissions across ALL repos in the target org. The empty-repos path is intentionally permitted on both same-org and cross-org paths. For cross-org, FOREIGN allowlist authorization gates access and the target org must have explicitly installed the role App and set FULLSEND_FOREIGN_<role>_REPOS. For same-org, WIF/OIDC enrollment is the only gate. ADR 0054 documents this as intentional.

• [permission-expansion] internal/mintcore/github.go:67 — The e2e role's permissions are expanded: actions_variables upgraded from read to write, and organization_actions_variables: write added (mirrored in internal/forge/github/types.go). ADR 0054 documents the rationale: pool tests set repo/org variables during install flows, scoped to pool orgs that explicitly authorize CI via FULLSEND_FOREIGN_E2E_REPOS.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths: .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml, AGENTS.md, skills/e2e-health/SKILL.md. The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Previous run (3)

Review

Findings

Medium

• [fail-open] internal/mintcore/handler.go:228 — Cross-org mint requests with empty repos produce installation-wide tokens granting full role permissions across ALL repos in the target org. The empty-repos path is intentionally permitted on both same-org and cross-org paths. For cross-org, FOREIGN allowlist authorization gates access and the target org must have explicitly installed the role App and set FULLSEND_FOREIGN_<role>_REPOS. For same-org, WIF/OIDC enrollment is the only gate. ADR 0054 documents this as intentional.

• [permission-expansion] internal/mintcore/github.go:67 — The e2e role's permissions are expanded: actions_variables upgraded from read to write, and organization_actions_variables: write added (mirrored in internal/forge/github/types.go). ADR 0054 documents the rationale: pool tests set repo/org variables during install flows, scoped to pool orgs that explicitly authorize CI via FULLSEND_FOREIGN_E2E_REPOS.

• [stale-doc] docs/ADRs/0040-org-pool-for-parallel-e2e-tests.md:23 — The Context section still describes e2e tests as "using Playwright browser automation" and references ADR 0010 (stored session). The PR supersedes ADR 0010 and removes Playwright but only updates the Consequences section, not the Context section.
  Remediation: Add a short annotation to lines 23–24 noting that Playwright browser automation was superseded by mint/OIDC authentication (ADR 0054).

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths: .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml, AGENTS.md, skills/e2e-health/SKILL.md. The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [stale-cache-authorization-bypass] internal/mintcore/handler.go — loadForeignAllowlist caches ALL results (including nil/empty allowlists and populated ones) with the same 60-second TTL. If a populated allowlist is cached and the operator then deletes or empties the FOREIGN variable (revoking authorization), the stale populated cache entry continues to authorize minting for up to 60 seconds. ADR 0054 documents the caching behavior.
  Remediation: Consider documenting the up-to-60s stale-positive window after FOREIGN variable deletion in ADR 0054's Consequences section so operators understand revocation is not instant.

• [stale-code] e2e/internal/otp/ — The e2e/internal/otp package (page.go, otp.go, otp_test.go) is dead code after this PR — its only consumers (login.go, export-session/main.go) are deleted. The package still pulls in playwright-go.

• [missing-test] internal/mintcore/handler_test.go — No handler-level integration test covers the case where FULLSEND_FOREIGN_*_REPOS exists but is empty/whitespace. ParseForeignAllowlist unit tests cover parsing, but the end-to-end 403 path at the handler level is untested.

• [relaxed-validation] internal/mintcore/handler.go:178 — The mint REST API (POST /v1/token) changes repos from required to conditionally required: same-org still accepts empty repos (producing installation-wide tokens), cross-org allows omitting for installation-wide access. ADR 0054 documents this semantic split.

• [fail-open] internal/cli/admin.go:2656 — checkTokenScopes skips OAuth scope preflight for installation tokens (correct — OAuth scopes don't apply). Installation tokens with insufficient App permissions fail at point of use rather than at preflight.

• [comment-style-consistency] internal/cli/foreign.go — Multiple helper functions (loadForeignAllowlist, parseForeignVariableName, validateForeignCaller, containsForeignCaller, removeForeignCaller) lack godoc comments. All validation and helper functions in the existing admin.go follow a consistent godoc commenting pattern.

Previous run (4)

Review

Findings

Medium

• [fail-open] internal/mintcore/handler.go:228 — Cross-org mint requests with empty repos produce installation-wide tokens granting full role permissions across ALL repos in the target org. The empty-repos path is intentionally permitted on both same-org and cross-org paths. For cross-org, FOREIGN allowlist authorization gates access and the target org must have explicitly installed the role App and set FULLSEND_FOREIGN_<role>_REPOS. For same-org, WIF/OIDC enrollment is the only gate. ADR 0054 documents this as intentional.

• [permission-expansion] internal/mintcore/github.go:67 — The e2e role's permissions are expanded: actions_variables upgraded from read to write, and organization_actions_variables: write added (mirrored in internal/forge/github/types.go). ADR 0054 documents the rationale: pool tests set repo/org variables during install flows, scoped to pool orgs that explicitly authorize CI via FULLSEND_FOREIGN_E2E_REPOS.

• [stale-doc] docs/ADRs/0040-org-pool-for-parallel-e2e-tests.md:23 — The Context section still describes e2e tests as "using Playwright browser automation" and references ADR 0010 (stored session). The PR supersedes ADR 0010 and removes Playwright but only updates the Consequences section, not the Context section.
  Remediation: Add a short annotation to lines 23–24 noting that Playwright browser automation was superseded by mint/OIDC authentication (ADR 0054).

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths: .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml, AGENTS.md, skills/e2e-health/SKILL.md. The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [stale-cache-authorization-bypass] internal/mintcore/handler.go — loadForeignAllowlist caches ALL results (including nil/empty allowlists and populated ones) with the same 60-second TTL. If a populated allowlist is cached and the operator then deletes or empties the FOREIGN variable (revoking authorization), the stale populated cache entry continues to authorize minting for up to 60 seconds. ADR 0054 documents the caching behavior.
  Remediation: Consider documenting the up-to-60s stale-positive window after FOREIGN variable deletion in ADR 0054's Consequences section so operators understand revocation is not instant.

• [stale-code] e2e/internal/otp/ — The e2e/internal/otp package (page.go, otp.go, otp_test.go) is dead code after this PR — its only consumers (login.go, export-session/main.go) are deleted. The package still pulls in playwright-go.

• [missing-test] internal/mintcore/handler_test.go — No handler-level integration test covers the case where FULLSEND_FOREIGN_*_REPOS exists but is empty/whitespace. ParseForeignAllowlist unit tests cover parsing, but the end-to-end 403 path at the handler level is untested.

• [relaxed-validation] internal/mintcore/handler.go:178 — The mint REST API (POST /v1/token) changes repos from required to conditionally required: same-org still accepts empty repos (producing installation-wide tokens), cross-org allows omitting for installation-wide access. ADR 0054 documents this semantic split.

• [fail-open] internal/cli/admin.go:2656 — checkTokenScopes skips OAuth scope preflight for installation tokens (correct — OAuth scopes don't apply). Installation tokens with insufficient App permissions fail at point of use rather than at preflight.

• [comment-style-consistency] internal/cli/foreign.go — Multiple helper functions (loadForeignAllowlist, parseForeignVariableName, validateForeignCaller, containsForeignCaller, removeForeignCaller) lack godoc comments. All validation and helper functions in the existing admin.go follow a consistent godoc commenting pattern.

Previous run (5)

Review

Findings

Medium

• [fail-open] internal/mintcore/handler.go:228 — Cross-org mint requests with empty repos produce installation-wide tokens granting full role permissions across ALL repos in the target org. The empty-repos path is intentionally permitted on both same-org and cross-org paths. For cross-org, FOREIGN allowlist authorization gates access and the target org must have explicitly installed the role App and set FULLSEND_FOREIGN_<role>_REPOS. For same-org, WIF/OIDC enrollment is the only gate. ADR 0054 documents this as intentional.

• [permission-expansion] internal/mintcore/github.go:67 — The e2e role's permissions are expanded: actions_variables upgraded from read to write, and organization_actions_variables: write added (mirrored in internal/forge/github/types.go). ADR 0054 documents the rationale: pool tests set repo/org variables during install flows, scoped to pool orgs that explicitly authorize CI via FULLSEND_FOREIGN_E2E_REPOS.

• [stale-cache-authorization-bypass] internal/mintcore/handler.go — loadForeignAllowlist caches ALL results (including nil/empty allowlists and populated ones) with the same 60-second TTL. If a populated allowlist is cached and the operator then deletes or empties the FOREIGN variable (revoking authorization), the stale populated cache entry continues to authorize minting for up to 60 seconds. This is a bidirectional stale-cache issue affecting both authorization and revocation.
  Remediation: Consider either (1) not caching empty/nil results, or (2) documenting the up-to-60s stale-positive window after FOREIGN variable deletion in ADR 0054's Consequences section so operators understand revocation is not instant.

• [stale-doc] docs/ADRs/0040-org-pool-for-parallel-e2e-tests.md:23 — The Context section still describes e2e tests as "using Playwright browser automation" and references ADR 0010 (stored session). The PR supersedes ADR 0010 and removes Playwright but only updates the Consequences section, not the Context section.
  Remediation: Update lines 23–24 to reflect mint/OIDC authentication.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths: .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml, AGENTS.md, skills/e2e-health/SKILL.md. The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [relaxed-validation] internal/mintcore/handler.go:178 — The mint REST API (POST /v1/token) changes repos from required to conditionally required: same-org still accepts empty repos (producing installation-wide tokens), cross-org allows omitting for installation-wide access. ADR 0054 documents this semantic split.

• [missing-test] internal/mintcore/handler_test.go — No handler-level integration test covers the case where FULLSEND_FOREIGN_*_REPOS exists but is empty/whitespace. ParseForeignAllowlist unit tests cover parsing, but the end-to-end 403 path at the handler level is untested.

• [fail-open] internal/cli/admin.go:2656 — checkTokenScopes skips OAuth scope preflight for installation tokens (correct — OAuth scopes don't apply). Installation tokens with insufficient App permissions fail at point of use rather than at preflight.

• [error-handling-gap] e2e/admin/testutil.go:75 — When tokenForOrg fails for all orgs in acquireOrg, per-org failures are logged but the final error message ("could not acquire any org from pool") suggests lock contention rather than auth failure. Diagnostic info is present in the log stream but not in the returned error.

• [stale-code] e2e/internal/otp/ — The e2e/internal/otp package (page.go, otp.go, otp_test.go) is dead code after this PR — its only consumers (login.go, export-session/main.go) are deleted. The package still pulls in playwright-go.

Previous run (6)

Review

Findings

Medium

• [fail-open] internal/mintcore/handler.go:228 — Cross-org mint requests with empty repos produce installation-wide tokens granting full role permissions across ALL repos in the target org. The path is gated by FOREIGN allowlist authorization and ADR 0054 explicitly documents this behavior ("When repos is omitted, mint an installation-wide token"), but there is no separate opt-in for installation-wide vs. scoped cross-org tokens. A compromised workflow in any allowlisted repo can obtain org-wide access to every repo in the target org.
  Remediation: Consider a separate FOREIGN variable (e.g. FULLSEND_FOREIGN_<role>_ALLOW_INSTALLATION_WIDE) for target orgs to explicitly permit installation-wide tokens. At minimum, ensure the ADR 0054 Consequences section clearly states that the FOREIGN allowlist implicitly permits installation-wide tokens.

• [permission-expansion] internal/mintcore/github.go:67 — The e2e role's permissions are expanded: actions_variables upgraded from read to write, and organization_actions_variables: write is added (mirrored in internal/forge/github/types.go:152). ADR 0054 documents the rationale (pool tests set variables during install flows), but this grants the e2e app write access to repo-level and org-level Actions variables on any installed org.

• [stale-doc] docs/ADRs/0040-org-pool-for-parallel-e2e-tests.md:23 — The Context section still describes e2e tests as "using Playwright browser automation" and references ADR 0010 (stored session). The PR supersedes ADR 0010 and removes Playwright but only updates the Consequences section, not the Context section.
  Remediation: Update lines 23–24 to reflect mint/OIDC authentication.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths: .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml, AGENTS.md, skills/e2e-health/SKILL.md. The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [error-handling-gap] e2e/admin/testutil.go:74 — When tokenForOrg fails for all orgs in acquireOrg, per-org failures are logged but the final error message ("could not acquire any org from pool") suggests lock contention rather than auth failure. Diagnostic info is present in the log stream but not in the returned error.

• [relaxed-validation] internal/mintcore/handler.go — The mint REST API (POST /v1/token) changes repos from required to conditionally required: same-org still requires repos, cross-org allows omitting for installation-wide access. ADR 0054 documents this semantic split.

• [stale-code] e2e/internal/otp/ — The e2e/internal/otp package (page.go, otp.go, otp_test.go) is dead code after this PR — its only consumers (login.go, export-session/main.go) are deleted. The package still pulls in playwright-go.

• [missing-test] internal/mintcore/handler_test.go — No handler-level integration test covers the case where FULLSEND_FOREIGN_*_REPOS exists but is empty/whitespace. ParseForeignAllowlist unit test covers parsing, but the end-to-end 403 path is untested at the handler level.

• [fail-open] internal/cli/admin.go:2656 — checkTokenScopes skips OAuth scope preflight for installation tokens (correct — OAuth scopes don't apply). Installation tokens with insufficient App permissions fail at point of use rather than at preflight.

---

Labels: PR modifies install/CLI admin flow and forge Client interface

Previous run (7)

Review

Reason: stale-head

The review agent reviewed commit 55d2b9b3c5ebb11a1dcd5f2765efeec6a5a024f1 but the PR HEAD is now c97480d7b0b19c24040466e08e0fccb7866009a8. This review was discarded to avoid approving unreviewed code.

Previous run (8)

Review

Findings

High

• [authorization] internal/mintcore/handler.go:190 — The diff removes the repos is required check from the mint handler, allowing callers to omit the repos field entirely. When repos is empty, FindOrgInstallation is called and CreateInstallationToken is invoked without scoping to specific repositories, resulting in an installation-wide token with full role permissions on ALL repositories in the target org. While the cross-org e2e use case legitimately needs this (gated by the FOREIGN allowlist), the same-org mintToken path now also accepts empty repos with no equivalent gate — any OIDC-authenticated workflow from an allowed org can request installation-wide tokens for any enrolled role (e.g., coder with contents:write or fullsend with actions:write). A WARNING log is emitted but enforcement is absent.
  Remediation: Restrict installation-wide (repos-omitted) minting to the cross-org path, or add an explicit allowlist of roles permitted to omit repos. At minimum, re-add the repos-required check for same-org mints.

• [stale-doc] AGENTS.md:37 — Lines 37–48 still document removed Playwright-based e2e env vars (E2E_GITHUB_PASSWORD, E2E_GITHUB_PASSWORD_FILE, E2E_GITHUB_SESSION_FILE, E2E_GITHUB_TOTP_SECRET) and reference Playwright browser automation. The PR migrated e2e auth to cross-org mint but did not update AGENTS.md, which is the primary contributor-facing instructions file.
  Remediation: Replace lines 37–48 with the new auth model: CI uses E2E_MINT_URL for cross-org mint, local runs use gh auth login or GH_TOKEN.

Medium

• [duplicate-api-call] internal/mintcore/handler.go:362 — mintTokenCrossOrg calls loadForeignAllowlist, which generates a JWT via GenerateAppJWT, calls FindOrgInstallation on the target org, and creates a policy installation token via ReadForeignAllowlist. It then delegates to mintToken, which generates a second JWT and calls FindOrgInstallation again on the same target org. This doubles JWT generation and GitHub API calls on the cross-org critical path.
  Remediation: Have loadForeignAllowlist return the installation ID and JWT, then pass them into mintToken to skip the redundant calls.

• [stale-reference] .github/workflows/pat-cleanup.yml — The entire pat-cleanup.yml workflow is dead code. It uses E2E_GITHUB_SESSION secret and runs hack/cleanup-pats.go which depends on Playwright + session files. Since this PR removes PAT-based auth entirely, this workflow will fail on every scheduled run.
  Remediation: Delete .github/workflows/pat-cleanup.yml and hack/cleanup-pats.go, or disable the schedule trigger.

• [permission-change] internal/mintcore/github.go:67 — The e2e role's actions_variables permission is escalated from read to write, and organization_actions_variables: read plus organization_administration: write are added. ADR 0051 mentions actions_variables: read for reading FOREIGN policy; the write escalation is not documented in the ADR.
  Remediation: Confirm that actions_variables: write (not just read) is required for the e2e role. If only org-level variable reading is needed for FOREIGN policy checks, organization_actions_variables: read alone suffices.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths (.github/). The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [incomplete-doc] docs/ADRs/0010-stored-session-for-e2e-browser-auth.md — ADR 0010 status is Accepted but the mechanism it describes (Playwright storageState for e2e auth) has been superseded by cross-org mint (ADR 0051). The e2e-testing.md guide already notes supersession.

• [incomplete-doc] docs/ADRs/0039-totp-automation-for-e2e-2fa.md — ADR 0039 status is Accepted but the TOTP automation mechanism it describes has been removed along with Playwright.

• [negative-cache-denial] internal/mintcore/handler.go — loadForeignAllowlist caches successful results with a 60-second TTL but does not cache errors. During a GitHub API outage, every concurrent request will generate a fresh JWT and hit GitHub again.

• [missing-test] internal/mintcore/handler_test.go — No handler-level test covers the case where FULLSEND_FOREIGN_*_REPOS is absent on the target org (variable returns 404).

• [error-handling] e2e/admin/testutil.go — In acquireOrg, when tokenForOrg fails for all orgs, the function falls through to the polling loop, retrying for the full timeout (10m) rather than failing fast.

• [relaxed-validation] internal/mintclient/mintclient.go:63 — MintRequest.Repos validation relaxed: previously required at least one repo, now optional. Intentional for cross-org e2e use case but affects same-org path too.

• [authorization] internal/mintcore/handler.go — Cross-org mint path does not validate that the target org is enrolled in ALLOWED_ORGS. By design per ADR 0051.

• [authorization] internal/mintcore/handler.go — Foreign allowlist cache uses 60-second TTL. Revocations will not take effect during this window.

• [authorization] internal/cli/admin.go:2680 — checkTokenScopes now calls IsInstallationToken first, which is a network probe.

• [code-duplication] e2e/admin/auth.go:15 — resolveLocalToken duplicates resolveToken from internal/cli/admin.go.

• [function-organization] internal/mintcore/github.go:205 — FindOrgInstallation and FindInstallation share similar structure but duplicate the pattern.

• [stale-doc] skills/e2e-health/SKILL.md:46 — Lists "Session expired" as a common failure category, which is obsolete with cross-org mint.

Previous run (9)

Review

Findings

High

• [authorization] internal/mintcore/handler.go:190 — The diff removes the repos is required check from the mint handler, allowing callers to omit the repos field entirely. When repos is empty, FindOrgInstallation is called and CreateInstallationToken is invoked without scoping to specific repositories, resulting in an installation-wide token with full role permissions on ALL repositories in the target org. The test TestHandler_EmptyRepos_FullOrgToken confirms this is intentional behavior, but it represents a significant privilege escalation surface: any workflow in the org can request an unrestricted token for any allowed role.
  Remediation: Re-add the repos-required validation for the default same-org path. If org-wide tokens are needed for specific use cases (e.g., cross-org enrollment), gate them behind an explicit opt-in field or restrict which roles/callers may request org-wide tokens.

• [stale-doc] AGENTS.md:37 — Lines 37–48 still document removed Playwright-based e2e env vars (E2E_GITHUB_PASSWORD, E2E_GITHUB_PASSWORD_FILE, E2E_GITHUB_SESSION_FILE, E2E_GITHUB_TOTP_SECRET) and reference Playwright browser automation. The PR migrated e2e auth to cross-org mint but did not update AGENTS.md.
  Remediation: Replace lines 37–48 with the new auth model: CI uses E2E_MINT_URL for cross-org mint, local runs use gh auth login or GH_TOKEN.

Medium

• [duplicate-api-call] internal/mintcore/handler.go — mintTokenCrossOrg calls loadForeignAllowlist, which generates a JWT via GenerateAppJWT, calls FindOrgInstallation on the target org, and creates a policy installation token via ReadForeignAllowlist. It then delegates to mintToken, which generates a second JWT and calls FindOrgInstallation again on the same target org. This doubles JWT generation and GitHub API calls on the cross-org critical path.
  Remediation: Have loadForeignAllowlist return the installation ID and JWT, then pass them into mintToken to skip the redundant calls.

• [scope-coherence] internal/mintcore/handler.go:212 — The handler now allows repos to be omitted and issues installation-wide tokens. ADR 0051 justifies this for e2e pool org operations, but the change affects all same-org mint paths, not just cross-org. The architectural shift from "repos required" to "repos optional" is not documented in an ADR amendment or Consequences.
  Remediation: Document the installation-wide token capability in ADR 0051 Consequences or a follow-on ADR. Clarify whether installation-wide tokens are restricted to specific roles or available to all roles.

• [stale-reference] .github/workflows/pat-cleanup.yml — The entire pat-cleanup.yml workflow is dead code. It uses E2E_GITHUB_SESSION secret and runs hack/cleanup-pats.go which depends on Playwright + session files. Since this PR removes PAT-based auth entirely, this workflow will fail on every scheduled run.
  Remediation: Delete .github/workflows/pat-cleanup.yml and hack/cleanup-pats.go, or disable the schedule trigger.

• [incomplete-doc] docs/ADRs/0010-stored-session-for-e2e-browser-auth.md — ADR 0010 status is Accepted but the mechanism it describes (Playwright storageState for e2e auth) has been superseded by cross-org mint (ADR 0051).
  Remediation: Change ADR 0010 status to Superseded by ADR 0051.

• [incomplete-doc] docs/ADRs/0039-totp-automation-for-e2e-2fa.md — ADR 0039 status is Accepted but the TOTP automation mechanism it describes has been removed along with Playwright.
  Remediation: Change ADR 0039 status to Superseded by ADR 0051.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths (.github/). The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [function-organization] internal/mintcore/github.go:205 — FindOrgInstallation and FindInstallation share similar structure but differ in API endpoint, parameters, and error messages. A common helper could reduce duplication.

• [negative-cache-denial] internal/mintcore/handler.go — loadForeignAllowlist caches successful results with a 60-second TTL but does not cache errors. During a GitHub API outage, every concurrent request will generate a fresh JWT and hit GitHub again.

• [missing-test] internal/mintcore/handler.go — No handler-level test covers the case where FULLSEND_FOREIGN_*_REPOS is absent on the target org (variable returns 404).

• [error-handling] e2e/admin/testutil.go — In acquireOrg, when tokenForOrg fails for all orgs, the function falls through to the polling loop, retrying for the full timeout (10m) rather than failing fast.

• [relaxed-validation] internal/mintclient/mintclient.go:63 — MintRequest.Repos validation relaxed: previously required at least one repo, now optional. Intentional for cross-org e2e use case but affects same-org path too.

• [authorization] internal/cli/admin.go:2680 — checkTokenScopes now calls IsInstallationToken first, which is a network probe. Only genuine network failures propagate, but this adds a network dependency to preflight checks.

• [authorization] internal/mintcore/handler.go — Cross-org mint path does not validate that the target org is enrolled in ALLOWED_ORGS. By design per ADR 0051.

• [authorization] internal/mintcore/handler.go — Foreign allowlist cache uses 60-second TTL. Revocations will not take effect during this window. Standard caching trade-off.

• [code-duplication] e2e/admin/auth.go:15 — resolveLocalToken duplicates resolveToken from internal/cli/admin.go. Pragmatic duplication to avoid import cycles in test code.

• [stale-doc] skills/e2e-health/SKILL.md:46 — Lists "Session expired" as a common failure category, which is obsolete with cross-org mint.

Previous run (10)

Review

Findings

High

• [authorization] internal/mintcore/handler.go:178 — The diff removes the repos is required check from the mint handler, allowing callers to omit the repos field entirely. When repos is empty, CreateInstallationToken is called without scoping to specific repositories, resulting in an installation-wide token with full role permissions on ALL repositories in the target org. This applies to both same-org and cross-org paths. The change logs a WARNING but does not restrict which roles may request unscoped tokens.
  Remediation: Restrict installation-wide (unscoped) token minting to an explicit allowlist of roles that require it (e.g., e2e). For all other roles, preserve the existing repos is required validation.

• [stale-doc] AGENTS.md:43 — Lines 37–48 still document removed Playwright-based e2e env vars (E2E_GITHUB_PASSWORD, E2E_GITHUB_PASSWORD_FILE, E2E_GITHUB_SESSION_FILE, E2E_GITHUB_TOTP_SECRET) and reference Playwright browser automation. The PR migrated e2e auth to cross-org mint but did not update AGENTS.md.
  Remediation: Replace lines 37–48 with the new auth model: CI uses E2E_MINT_URL for cross-org mint, local runs use gh auth login or GH_TOKEN.

Medium

• [duplicate-api-call] internal/mintcore/handler.go — mintTokenCrossOrg calls loadForeignAllowlist, which generates a JWT via GenerateAppJWT and calls FindOrgInstallation on the target org, then creates an installation token via ReadForeignAllowlist. It then delegates to mintToken, which generates a second JWT and calls FindOrgInstallation again on the same target org. This doubles JWT generation and GitHub API calls on the cross-org critical path.
  Remediation: Have loadForeignAllowlist return the JWT and installation ID so mintToken can reuse them.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths (.github/). The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [negative-cache-denial] internal/mintcore/handler.go — loadForeignAllowlist caches successful results with a 60-second TTL but does not cache errors. During a GitHub API outage, every concurrent request will generate a fresh JWT and hit GitHub again — potential thundering-herd amplification.

• [missing-test] internal/mintcore/handler.go — No handler-level test covers the case where FULLSEND_FOREIGN_*_REPOS is absent on the target org (variable returns 404). The denial case where the variable exists but the caller is unauthorized is tested, but the missing-variable case is not.

• [authorization] internal/cli/admin.go:2680 — checkTokenScopes now calls IsInstallationToken first, which is a network probe (GET /installation/repositories). If the probe fails due to transient issues, the error propagates and blocks the preflight operation.

• [error-handling] e2e/admin/testutil.go — In acquireOrg, when tokenForOrg fails for all orgs (e.g., mint is down), the function logs and continues to the next org. If all orgs fail token acquisition, it falls through to the polling loop, retrying for the full timeout (default 10m) rather than failing fast.

• [authorization] internal/mintcore/handler.go — Cross-org mint path does not validate that the target org is enrolled in ALLOWED_ORGS. By design per ADR 0051 — target orgs opt in via installing the role App and setting the FOREIGN variable.

• [authorization] internal/mintcore/handler.go — Foreign allowlist cache uses 60-second TTL. Revocations will not take effect during this window. Standard caching trade-off.

• [incomplete-doc] docs/ADRs/0010-stored-session-for-e2e-browser-auth.md — ADR 0010 status is Accepted but the PR removes the mechanism it describes. The e2e guide notes supersession; the ADR itself should be marked Superseded with a reference to ADR 0051.

• [incomplete-doc] docs/ADRs/0039-totp-automation-for-e2e-2fa.md — ADR 0039 status is Accepted but the PR removes TOTP automation entirely. Same as above.

• [stale-doc] .github/workflows/pat-cleanup.yml / hack/cleanup-pats.go — Workflow and script still use removed Playwright infrastructure (E2E_GITHUB_SESSION, hack/cleanup-pats.go). These are orphaned by the Playwright removal.

• [stale-doc] skills/e2e-health/SKILL.md:46 — Lists "Session expired" as a common failure category, which is obsolete with cross-org mint.

• [code-duplication] e2e/admin/auth.go:36 — resolveLocalToken duplicates resolveToken from internal/cli/admin.go. Both implement identical GH_TOKEN/GITHUB_TOKEN/gh auth token resolution.

• [relaxed-validation] internal/mintclient/mintclient.go:63 — MintRequest.Repos validation relaxed: previously required at least one repo, now optional. Backward compatible but changes contract semantics.

• [breaking-change] internal/forge/forge.go:259 — Client interface gains 4 new methods. In internal/ so no external consumers, but all in-repo implementations must be updated (already done in this PR).

Previous run (11)

Review

Findings

High

• [authorization] internal/mintcore/handler.go — The diff removes the repos is required check from the mint handler, allowing callers to omit the repos field entirely. When repos is empty, CreateInstallationToken is called without scoping to specific repositories, resulting in an installation-wide token with full permissions for the role on ALL repositories in the target org. The same-org path has no restriction on omitting repos — any OIDC-authenticated caller from an allowed org can now request an unscoped token for their own org. While the cross-org path has the FOREIGN allowlist gate and the code emits WARNING logs, no denial or role-based gate exists.
  Remediation: Consider either (a) only allowing repos to be omitted on the cross-org path (where the FOREIGN allowlist provides the authorization gate), or (b) adding an explicit allowlist of roles that may request installation-wide tokens.

• [stale-doc] AGENTS.md:43 — Lines 41–48 still document removed Playwright-based e2e env vars (E2E_GITHUB_PASSWORD, E2E_GITHUB_PASSWORD_FILE, E2E_GITHUB_SESSION_FILE, E2E_GITHUB_TOTP_SECRET) that no longer exist after the migration to OIDC/mint auth. The PR updates docs/guides/dev/e2e-testing.md with the new auth model but does not update AGENTS.md, which has nearly identical content that is now materially incorrect.
  Remediation: Update AGENTS.md section "Running e2e tests" to reflect the new auth model: CI uses cross-org mint (E2E_MINT_URL), local runs use gh auth login or GH_TOKEN. Remove Playwright references and document new env vars.

Medium

• [duplicate-api-call] internal/mintcore/handler.go — mintTokenCrossOrg calls loadForeignAllowlist, which generates a JWT via GenerateAppJWT and calls FindOrgInstallation on the target org. It then delegates to mintToken, which generates a second JWT and calls FindOrgInstallation on the same target org again. This doubles JWT generation and GitHub API calls on the cross-org critical path.
  Remediation: Have loadForeignAllowlist return the installation ID and JWT, then pass them into mintToken (or a variant) to skip the redundant calls.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths (.github/). The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [authorization] internal/mintcore/handler.go — Cross-org mint path does not validate that the target org is enrolled in ALLOWED_ORGS. By design per ADR 0051, target orgs opt in via installing the role App and setting the FOREIGN variable. Worth noting but intentional.

• [authorization] internal/mintcore/handler.go — Foreign allowlist cache uses 60-second TTL. Revocations will not take effect during this window. Standard caching trade-off; tokens themselves are short-lived.

• [privilege-escalation] internal/mintcore/github.go — The e2e role gains organization_actions_variables: read. Required by ADR 0051 for reading FOREIGN allowlist. Read-only and proportional given the role already has administration: write.

• [incomplete-doc] docs/ADRs/0010-stored-session-for-e2e-browser-auth.md — ADR 0010 status is Accepted but the PR removes the mechanism it describes. The e2e guide already notes supersession; a short annotation on the ADR itself would be good housekeeping.

• [incomplete-doc] docs/ADRs/0039-totp-automation-for-e2e-2fa.md — ADR 0039 status is Accepted but the PR removes TOTP automation entirely. Same as above — guide notes supersession, ADR itself could use an annotation.

• [missing-test] internal/mintcore/handler.go — No handler-level test covers the case where FULLSEND_FOREIGN_*_REPOS is absent on the target org (variable returns 404). The denial case (unauthorized caller) is tested.

• [authorization] internal/cli/admin.go:2680 — checkTokenScopes now calls IsInstallationToken first, which is a network probe. If the probe fails due to transient issues, the error propagates and blocks the preflight operation.

• [incomplete-doc] .github/workflows/pat-cleanup.yml — Workflow still uses E2E_GITHUB_SESSION_FILE and Playwright to clean up PATs. The PR removes PAT creation, making this workflow obsolete.

• [incomplete-doc] hack/cleanup-pats.go — Script still requires E2E_GITHUB_SESSION_FILE and uses Playwright to delete expired PATs. No longer needed after this migration.

• [code-duplication] e2e/admin/auth.go:36 — resolveLocalToken duplicates resolveToken from internal/cli/admin.go. Both implement identical GH_TOKEN/GITHUB_TOKEN/gh auth token resolution. Acceptable for test/prod isolation.

• [naming-convention] e2e/admin/auth.go — tokenForOrg doesn't follow the codebase's resolve* naming pattern, though the function does more than resolution (conditional mint vs local).

• [function-naming] e2e/admin/testutil.go — acquireOrgWithClient naming is unclear — it's a test-only backward compatibility shim, not a feature variant.

• [error-handling-idiom] e2e/admin/testutil.go — When tokenForOrg fails in acquireOrg, the function logs and continues but cannot attempt stale-lock recovery (no token available). This is correct behavior but differs from acquireOrgFromClient's pattern.

---

Labels: PR adds cross-org mint authorization and migrates e2e auth from Playwright to OIDC/mint, touching mint handler, e2e test infra, and security-sensitive authorization paths.

Previous run

Review of PR #2277 — Cross-org mint authorization & e2e auth migration

Verdict: Request Changes

Prior review SHA: bc07e1c → 0f37c9d
Provenance: app-verified ✓

---

High

#	Category	File	Description
1	stale-doc	AGENTS.md	Lines 37-48 still document removed Playwright-based e2e env vars (E2E_GITHUB_PASSWORD, E2E_GITHUB_SESSION_FILE, E2E_GITHUB_MFA_SECRET, E2E_GITHUB_ORG) that no longer exist after the migration to OIDC/mint auth. These are the primary env vars operators would configure — stale docs here will cause deployment confusion. Remediation: Replace the Playwright env var block with the new OIDC/mint auth configuration (e.g., FULLSEND_FOREIGN_*_REPOS org variables, WIF provider settings).

Medium

#	Category	File	Description
2	missing-test	internal/mintcore/foreign.go	parseForeignAllowlist has no unit tests. This function parses comma-separated repo lists from org variables and is security-critical (controls which repos can mint tokens for foreign orgs). Edge cases like empty strings, whitespace, duplicate entries, and malformed input are untested. Remediation: Add unit tests for parseForeignAllowlist covering empty input, single repo, multiple repos, trailing commas, whitespace, and invalid formats.
3	missing-test	internal/mintcore/handler.go	The new cross-org mint authorization path (handleCrossOrgMint / foreign allowlist check) lacks integration test coverage. The e2e tests exercise the happy path but do not cover rejection scenarios (unauthorized repo, missing org var, malformed allowlist). Remediation: Add test cases for cross-org mint denial and edge cases.
4	protected-path	.github/workflows/e2e.yml	CI workflow file modified — changes to .github/ are governance-sensitive. The modifications look consistent with the auth migration (removing Playwright secrets, adding OIDC permissions), but require maintainer review of the workflow permission changes.

Low

#	Category	File	Description
5	naming-convention	internal/cli/foreign.go	Function runForeignList returns a formatted table but the name doesn't distinguish it from a data-returning function. Consider printForeignList or displayForeignList for clarity.
6	error-handling-idiom	internal/mintcore/foreign.go	lookupForeignAllowlist silently returns an empty list when the GitHub API call to fetch org variables fails. Consider logging the error or returning it so callers can distinguish "no allowlist configured" from "API failure".
7	design-direction	e2e/admin/auth.go	The new OIDC auth flow is well-structured, but the retry logic (3 attempts with 5s sleep) uses a simple loop rather than the retry package used elsewhere in the codebase.
8	scope-creep	internal/cli/foreign.go	The foreign list CLI command is a useful addition but is not mentioned in issue #2155's scope. This is minor — the command is a natural complement to the foreign mint feature — but worth noting for scope tracking.
9	code-organization	e2e/admin/auth.go	The WaitForToken helper bundles token polling with file I/O. Separating the polling logic from the file-reading would improve testability.
10	doc-style	internal/mintcore/foreign.go	The package-level comment references "ADR 0051" but doesn't include the ADR title, making it harder to find without searching. Consider adding the title: "ADR 0051: Cross-org mint authorization".
11	pattern-inconsistency	internal/mintcore/handler.go	The cross-org path uses slog.Warn for authorization failures while the existing same-org path uses slog.Error. Authorization failures should use a consistent log level.
12	stale-reference	e2e/admin/auth.go	Comment on line 28 references "the Playwright session" but this file is the OIDC replacement — the reference is vestigial.
13	naming-convention	internal/mintcore/foreign.go	The ForeignRepoEntry struct uses OrgName field but the variable naming in parseForeignAllowlist uses orgSlug — inconsistent terminology for the same concept.
14	incomplete-doc	docs/cross-org-mint.md	The new cross-org mint documentation doesn't mention the foreign list CLI command added in this PR.

---

1 high finding and 3 medium findings require resolution. The high-severity stale documentation in AGENTS.md will mislead operators configuring the system. The medium-severity missing tests for the security-critical foreign allowlist parsing need coverage before merge.

---

Labels: PR modifies mint service (cross-org authorization), e2e test infrastructure, and CI workflows.

---

Labels: PR modifies mint service (cross-org authorization), e2e test infrastructure, and CI workflows

Previous run (12)

Review

Findings

High

• [authorization] internal/mintcore/handler.go — The diff removes the repos is required check from the mint handler, allowing callers to omit the repos field entirely. When repos is empty, CreateInstallationToken is called without scoping to specific repositories, resulting in an installation-wide token with full permissions for the role on ALL repositories in the target org. The same-org path has no restriction on omitting repos — any OIDC-authenticated caller from an allowed org can now request an unscoped token for their own org. While the cross-org path has the FOREIGN allowlist gate and the code emits WARNING logs, no denial or role-based gate exists.
  Remediation: Consider either (a) only allowing repos to be omitted on the cross-org path (where the FOREIGN allowlist provides the authorization gate), or (b) adding an explicit allowlist of roles that may request installation-wide tokens.

• [stale-doc] AGENTS.md:43 — Lines 41–48 still document removed Playwright-based e2e env vars (E2E_GITHUB_PASSWORD, E2E_GITHUB_PASSWORD_FILE, E2E_GITHUB_SESSION_FILE, E2E_GITHUB_TOTP_SECRET) that no longer exist after the migration to OIDC/mint auth. The PR updates docs/guides/dev/e2e-testing.md with the new auth model but does not update AGENTS.md, which has nearly identical content that is now materially incorrect.
  Remediation: Update AGENTS.md section "Running e2e tests" to reflect the new auth model: CI uses cross-org mint (E2E_MINT_URL), local runs use gh auth login or GH_TOKEN. Remove Playwright references and document new env vars.

Medium

• [duplicate-api-call] internal/mintcore/handler.go — mintTokenCrossOrg calls loadForeignAllowlist, which generates a JWT via GenerateAppJWT and calls FindOrgInstallation on the target org. It then delegates to mintToken, which generates a second JWT and calls FindOrgInstallation on the same target org again. This doubles JWT generation and GitHub API calls on the cross-org critical path.
  Remediation: Have loadForeignAllowlist return the installation ID and JWT, then pass them into mintToken (or a variant) to skip the redundant calls.

• [protected-path] .github/workflows/e2e.yml — This PR modifies files under protected paths (.github/). The changes are justified by issue E2e: secretless GitHub auth via mint OIDC (replace Playwright session + PAT) #2155 (migrating e2e auth from Playwright to OIDC/mint), but human approval is always required for protected-path changes.

Low

• [authorization] internal/mintcore/handler.go — Cross-org mint path does not validate that the target org is enrolled in ALLOWED_ORGS. By design per ADR 0051, target orgs opt in via installing the role App and setting the FOREIGN variable. Worth noting but intentional.

• [authorization] internal/mintcore/handler.go — Foreign allowlist cache uses 60-second TTL. Revocations will not take effect during this window. Standard caching trade-off; tokens themselves are short-lived.

• [privilege-escalation] internal/mintcore/github.go — The e2e role gains organization_actions_variables: read. Required by ADR 0051 for reading FOREIGN allowlist. Read-only and proportional given the role already has administration: write.

• [incomplete-doc] docs/ADRs/0010-stored-session-for-e2e-browser-auth.md — ADR 0010 status is Accepted but the PR removes the mechanism it describes. The e2e guide already notes supersession; a short annotation on the ADR itself would be good housekeeping.

• [incomplete-doc] docs/ADRs/0039-totp-automation-for-e2e-2fa.md — ADR 0039 status is Accepted but the PR removes TOTP automation entirely. Same as above — guide notes supersession, ADR itself could use an annotation.

• [missing-test] internal/mintcore/handler.go — No handler-level test covers the case where FULLSEND_FOREIGN_*_REPOS is absent on the target org (variable returns 404). The denial case (unauthorized caller) is tested.

• [authorization] internal/cli/admin.go:2680 — checkTokenScopes now calls IsInstallationToken first, which is a network probe. If the probe fails due to transient issues, the error propagates and blocks the preflight operation.

• [incomplete-doc] .github/workflows/pat-cleanup.yml — Workflow still uses E2E_GITHUB_SESSION_FILE and Playwright to clean up PATs. The PR removes PAT creation, making this workflow obsolete.

• [incomplete-doc] hack/cleanup-pats.go — Script still requires E2E_GITHUB_SESSION_FILE and uses Playwright to delete expired PATs. No longer needed after this migration.

• [code-duplication] e2e/admin/auth.go:36 — resolveLocalToken duplicates resolveToken from internal/cli/admin.go. Both implement identical GH_TOKEN/GITHUB_TOKEN/gh auth token resolution. Acceptable for test/prod isolation.

• [naming-convention] e2e/admin/auth.go — tokenForOrg doesn't follow the codebase's resolve* naming pattern, though the function does more than resolution (conditional mint vs local).

• [function-naming] e2e/admin/testutil.go — acquireOrgWithClient naming is unclear — it's a test-only backward compatibility shim, not a feature variant.

• [error-handling-idiom] e2e/admin/testutil.go — When tokenForOrg fails in acquireOrg, the function logs and continues but cannot attempt stale-lock recovery (no token available). This is correct behavior but differs from acquireOrgFromClient's pattern.

---

Labels: PR adds cross-org mint authorization and migrates e2e auth from Playwright to OIDC/mint, touching mint handler, e2e test infra, and security-sensitive authorization paths.

Previous run (13)

Review of PR #2277 — Cross-org mint authorization & e2e auth migration

Verdict: Request Changes

Prior review SHA: bc07e1c → 0f37c9d
Provenance: app-verified ✓

---

High

#	Category	File	Description
1	stale-doc	AGENTS.md	Lines 37-48 still document removed Playwright-based e2e env vars (E2E_GITHUB_PASSWORD, E2E_GITHUB_SESSION_FILE, E2E_GITHUB_MFA_SECRET, E2E_GITHUB_ORG) that no longer exist after the migration to OIDC/mint auth. These are the primary env vars operators would configure — stale docs here will cause deployment confusion. Remediation: Replace the Playwright env var block with the new OIDC/mint auth configuration (e.g., FULLSEND_FOREIGN_*_REPOS org variables, WIF provider settings).

Medium

#	Category	File	Description
2	missing-test	internal/mintcore/foreign.go	parseForeignAllowlist has no unit tests. This function parses comma-separated repo lists from org variables and is security-critical (controls which repos can mint tokens for foreign orgs). Edge cases like empty strings, whitespace, duplicate entries, and malformed input are untested. Remediation: Add unit tests for parseForeignAllowlist covering empty input, single repo, multiple repos, trailing commas, whitespace, and invalid formats.
3	missing-test	internal/mintcore/handler.go	The new cross-org mint authorization path (handleCrossOrgMint / foreign allowlist check) lacks integration test coverage. The e2e tests exercise the happy path but do not cover rejection scenarios (unauthorized repo, missing org var, malformed allowlist). Remediation: Add test cases for cross-org mint denial and edge cases.
4	protected-path	.github/workflows/e2e.yml	CI workflow file modified — changes to .github/ are governance-sensitive. The modifications look consistent with the auth migration (removing Playwright secrets, adding OIDC permissions), but require maintainer review of the workflow permission changes.

Low

#	Category	File	Description
5	naming-convention	internal/cli/foreign.go	Function runForeignList returns a formatted table but the name doesn't distinguish it from a data-returning function. Consider printForeignList or displayForeignList for clarity.
6	error-handling-idiom	internal/mintcore/foreign.go	lookupForeignAllowlist silently returns an empty list when the GitHub API call to fetch org variables fails. Consider logging the error or returning it so callers can distinguish "no allowlist configured" from "API failure".
7	design-direction	e2e/admin/auth.go	The new OIDC auth flow is well-structured, but the retry logic (3 attempts with 5s sleep) uses a simple loop rather than the retry package used elsewhere in the codebase.
8	scope-creep	internal/cli/foreign.go	The foreign list CLI command is a useful addition but is not mentioned in issue #2155's scope. This is minor — the command is a natural complement to the foreign mint feature — but worth noting for scope tracking.
9	code-organization	e2e/admin/auth.go	The WaitForToken helper bundles token polling with file I/O. Separating the polling logic from the file-reading would improve testability.
10	doc-style	internal/mintcore/foreign.go	The package-level comment references "ADR 0051" but doesn't include the ADR title, making it harder to find without searching. Consider adding the title: "ADR 0051: Cross-org mint authorization".
11	pattern-inconsistency	internal/mintcore/handler.go	The cross-org path uses slog.Warn for authorization failures while the existing same-org path uses slog.Error. Authorization failures should use a consistent log level.
12	stale-reference	e2e/admin/auth.go	Comment on line 28 references "the Playwright session" but this file is the OIDC replacement — the reference is vestigial.
13	naming-convention	internal/mintcore/foreign.go	The ForeignRepoEntry struct uses OrgName field but the variable naming in parseForeignAllowlist uses orgSlug — inconsistent terminology for the same concept.
14	incomplete-doc	docs/cross-org-mint.md	The new cross-org mint documentation doesn't mention the foreign list CLI command added in this PR.

---

1 high finding and 3 medium findings require resolution. The high-severity stale documentation in AGENTS.md will mislead operators configuring the system. The medium-severity missing tests for the security-critical foreign allowlist parsing need coverage before merge.

---

Labels: PR modifies mint service (cross-org authorization), e2e test infrastructure, and CI workflows.

---

Labels: PR modifies mint service (cross-org authorization), e2e test infrastructure, and CI workflows

[fullsend-ai-review]

See the review comment for full details.

[github-actions]

Site preview

Preview: https://e080f159-site.fullsend-ai.workers.dev

Commit: 3e1fd5f747ef0a02172642429213a7a125ecac8a

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:08 AM UTC · Completed 7:43 AM UTC
Commit: 074deed · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 7:36 AM UTC · Completed 7:56 AM UTC
Commit: eae178a · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 8:25 AM UTC · Ended 8:41 AM UTC
Commit: 4e21a60 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 78.23129% with 96 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/foreign.go	74.19%	29 Missing and 27 partials ⚠️
internal/forge/github/github.go	76.64%	19 Missing and 13 partials ⚠️
internal/cli/github.go	25.00%	3 Missing and 3 partials ⚠️
internal/layers/preflight.go	60.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 8:25 AM UTC · Completed 8:41 AM UTC
Commit: c89dc81 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 8:57 AM UTC · Completed 9:18 AM UTC
Commit: 02c054d · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:19 AM UTC · Completed 10:39 AM UTC
Commit: bc07e1c · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-retro]

🤖 Finished Retro · ❌ Failure · Started 7:09 AM UTC · Completed 7:21 AM UTC
Commit: 3e1fd5f · View workflow run →