feat(e2e): WIF shim and PR authorization gate (#1604)

[ifireball]

Summary

Incremental step toward secretless e2e (#1604) — authorized runs without per-repo secrets/variables. This PR does not complete that goal; it delivers:

• e2e_shim.yml calling trusted e2e.yml@refs/heads/main via workflow_call on same-repo PRs
• PR authorization gate (member/collaborator or fresh ok-to-test; stale label removal + sticky comment)
• GCP auth via WIF (google-github-actions/auth@v3, job_workflow_ref trust boundary) — no SA JSON keys
• Default GCP project it-gcp-konflux-e2e-fullsend when E2E_GCP_PROJECT_ID secret is unset
• Operator/contributor docs (ADR 0043, e2e-gcp-setup.md, e2e-testing.md)

Still required today: E2E_GITHUB_SESSION, E2E_MINT_URL, E2E_GCP_WIF_PROVIDER, E2E_GCP_SERVICE_ACCOUNT, password/TOTP secrets. Fork PR e2e remains blocked.

Closes #1604

Test plan

• bash scripts/check-e2e-authorization_test.sh passes locally
• After merge: apply GCP WIF provider per e2e-gcp-setup.md and verify workflow_dispatch GCP auth against it-gcp-konflux-e2e-fullsend
• Same-repo member PR: shim → gate passes → e2e runs
• Untrusted same-repo PR: sticky comment; passes after maintainer applies ok-to-test post-push
• Stale ok-to-test after new push: label removed, e2e skipped

[github-actions]

Site preview

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

Commit: 20ecffe2832d081f9d4203aacce99ab7e02be8c8

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/, scripts/ — This PR modifies 5 files under protected paths (.github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, .github/workflows/e2e_shim.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization_test.sh). The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale for CI authorization gate and WIF integration. Human approval is required for protected-path changes regardless of review outcome.

• [pagination-edge-case] scripts/check-e2e-authorization.sh:48 — gh api --paginate without jq -s 'add' outputs separate JSON arrays per page. When stored in a bash variable and piped to jq, each array is processed independently — [.[] | select(...)] | max runs once per page, producing one line per page. The bash string comparison on multi-line last_push_at and ok_to_test_at values can produce incorrect authorization decisions for PRs with enough timeline/event entries to span multiple API pages.
  Remediation: Pipe each paginated gh api call through jq -s 'add' to flatten pages into a single array, e.g.: timeline_json="$(gh api ... --paginate | jq -s 'add')". Same for events_json and the commits_json fallback.

• [scope-claim-mismatch] PR body — PR says "Closes Apply WIF to e2e test workflows via shim/gate pattern #1604" but ADR 0043 and all three new docs explicitly document this as incremental progress, not completion. E2E_GITHUB_SESSION, E2E_MINT_URL, password/TOTP secrets remain required. Using "Closes" will auto-close issue Apply WIF to e2e test workflows via shim/gate pattern #1604 prematurely.
  Remediation: Change "Closes Apply WIF to e2e test workflows via shim/gate pattern #1604" to "Relates to Apply WIF to e2e test workflows via shim/gate pattern #1604" or "Progress on Apply WIF to e2e test workflows via shim/gate pattern #1604".

Low

• [api-field-assumption] scripts/check-e2e-authorization.sh:50 — The jq filter selects timeline events with .event == "committed" and reads .created_at. GitHub's timeline API committed event type returns Git commit objects whose top-level structure may not include created_at (they use committer.date instead). The fallback to the commits API (line 53) handles this gracefully, but test mocks always set created_at on committed events, so the primary timeline path may not produce useful results for non-force-push scenarios.

• [missing-test-coverage] scripts/check-e2e-authorization_test.sh — The test suite does not cover the ERR-trap / reason=error path. All test cases exercise success paths where the gh mock succeeds.

• [secrets-scope] .github/workflows/e2e_shim.yml:42 — secrets: inherit passes all repository secrets to the called workflow. Since e2e.yml runs from @refs/heads/main (trusted) and pull_request_target prevents PR authors from modifying the called workflow, blast radius is limited. Explicitly listing required secrets would be more defensive.

• [naming-convention] .github/workflows/e2e_shim.yml — Uses underscore separator (e2e_shim.yml) while all 16 existing workflows use hyphens (e.g., branch-cleanup.yml, site-build.yml). Consider renaming to e2e-shim.yml for consistency.

• [permissions-change] .github/workflows/e2e.yml — Top-level permissions: changes from contents: read + id-token: write to permissions: {} (empty), with job-level blocks introduced. Net security improvement (least-privilege at job level).

• [permissions-change] .github/workflows/e2e_shim.yml:38 — New workflow grants contents: read, id-token: write, and pull-requests: write. Permissions are appropriately scoped for WIF auth and gate comments.

Info

• [naming-convention] scripts/check-e2e-authorization_test.sh — Uses underscore separator (_test.sh). Existing test scripts use hyphens (e.g., post-triage-test.sh).

• [sensitive-data-defaults] .github/workflows/e2e.yml:51 — GCP project ID, service account, and WIF provider path hardcoded as workflow defaults. Intentional per ADR 0043 and CLAUDE.md exception.

• [fail-closed-validation] scripts/check-e2e-authorization.sh:84 — Authorization script correctly uses fail-closed design: ERR trap sets authorized=false reason=error.

Previous run

Review

Findings

Medium

• [protected-path] .github/, scripts/ — This PR modifies 5 files under protected paths (.github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, .github/workflows/e2e_shim.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization_test.sh). The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale for CI authorization gate and WIF integration. Human approval is required for protected-path changes regardless of review outcome.

• [pagination-edge-case] scripts/check-e2e-authorization.sh:46 — gh api --paginate without --jq or piping through jq -s 'add' outputs separate JSON arrays per page. The jq expression [.[] | .commit.committer.date] | max runs independently per page via jq's default streaming behavior, so last_push_at can contain multiple lines (one max per page). The same issue applies to events_json on line 49. The bash string comparison on a multi-line value produces incorrect results — it compares against the first page's max only, which could allow a stale ok-to-test label to appear fresh on PRs with many commits/events.
  Remediation: Pipe paginated output through jq -s 'add' to flatten pages before extracting the max. For example: commits_json="$(gh api ... --paginate | jq -s 'add')". Alternatively, use --jq with the gh api call which processes all pages as a single stream.

Low

• [error-comment-template] .github/actions/check-e2e-authorization/action.yml:69 — When the authorization script fails (API error, rate limit), reason=error is set via the ERR trap, but the comment template's case statement has no error) branch. It falls through to *) which displays "E2E tests run automatically for org/repo members and collaborators" — misleading when the actual cause is a transient error.
  Remediation: Add an error) case to the comment template that explains a transient error occurred and suggests re-running the workflow.

• [missing-test-coverage] scripts/check-e2e-authorization_test.sh — The test suite does not cover the ERR-trap / reason=error path. Since the error path writes authorized=false reason=error and is a distinct code path with its own output logic, it should have at least one test case verifying that a gh api failure produces the expected error output and exits 0.

• [sensitive-data-defaults] .github/workflows/e2e.yml:51 — GCP project name (it-gcp-konflux-e2e-fullsend), service account email, WIF provider path (including project number 208332380190), and pool/provider IDs are hardcoded as workflow defaults. The CLAUDE.md exception and ADR 0043 document the design decision that WIF attribute conditions are the security boundary, not identifier secrecy. Kept as low since this is an intentional, documented design choice.

• [secrets-scope] .github/workflows/e2e_shim.yml:42 — secrets: inherit passes all repository secrets to the called workflow. Since e2e.yml runs from @refs/heads/main (trusted) and pull_request events don't expose secrets to forks, blast radius is limited. Explicitly listing required secrets would be more defensive and self-documenting.

• [stale-reference] docs/architecture.md:43 — The architecture doc states shim workflow security uses pull_request_target as a general principle (ADR 0009). The new e2e_shim.yml uses pull_request with a different security model (calling trusted workflow on main). A clarifying note distinguishing enrollment shims from the e2e CI shim would prevent confusion.

• [naming-convention] .github/workflows/e2e_shim.yml — Uses underscore separator (e2e_shim.yml) while all other workflows in .github/workflows/ use hyphens. Consider renaming to e2e-shim.yml for consistency.

• [scope-claim-mismatch] PR body — PR says "Closes Apply WIF to e2e test workflows via shim/gate pattern #1604" but ADR 0043 explicitly documents this as incremental progress, not completion. Several secrets (E2E_GITHUB_SESSION, E2E_MINT_URL, password/TOTP) and fork PR support remain required. Using "Closes" will auto-close the issue prematurely. Consider changing to "Partially addresses Apply WIF to e2e test workflows via shim/gate pattern #1604".

Info

• [naming-convention] scripts/check-e2e-authorization_test.sh — Uses underscore separator (_test.sh). Most test scripts in the repo use hyphens, though the convention is not fully uniform (e.g., topissues_test.py).

Previous run (2)

Review

Findings

Medium

• [protected-path] .github/, scripts/ — This PR modifies 6 files under protected paths (.github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, .github/workflows/e2e_shim.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization_test.sh, scripts/resolve-e2e-gcp-auth.sh). The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale. Human approval is required for protected-path changes regardless of review outcome.

• [path-filter-mismatch] .github/workflows/e2e_shim.yml — The e2e_shim.yml paths filter is missing scripts/resolve-e2e-gcp-auth.sh, which IS included in the e2e.yml push-trigger paths. A PR that only modifies resolve-e2e-gcp-auth.sh will not trigger e2e tests on the PR but will trigger them on push to main, so regressions in that script would not be caught before merge.
  Remediation: Add 'scripts/resolve-e2e-gcp-auth.sh' to the paths list in e2e_shim.yml.

• [sensitive-data-defaults] .github/workflows/e2e.yml, e2e/admin/testutil.go — GCP project name (it-gcp-konflux-e2e-fullsend), service account ID (fullsend-e2e), and other infrastructure identifiers are hardcoded as defaults. This PR resolves the prior tension with CLAUDE.md by adding an explicit exception (line 16) documented in ADR 0043, which argues WIF attribute conditions are the security boundary, not name secrecy. The exception's scope should be monitored — see also [policy-exception-scope] below.

• [policy-exception-scope] CLAUDE.md — The new exception for shared e2e infrastructure identifiers establishes a category of allowed defaults. While architecturally coherent with WIF-based security (credentials are scoped by IAM conditions, not obscurity), the exception language could be narrower. It should clarify whether this applies exclusively to the dedicated CI e2e test project or could extend to other WIF-protected resources in the future. See also [sensitive-data-defaults] above.
  Remediation: Consider narrowing the exception to emphasize it is exclusively for the dedicated CI e2e test project, or update the base rule to acknowledge WIF-protected identifiers as a distinct category with explicit criteria.

Low

• [error-comment-template] .github/actions/check-e2e-authorization/action.yml — When the authorization script fails (API error, rate limit), reason=error is set via the ERR trap but the comment template's case statement has no error) branch. It falls through to *) which displays "E2E tests run automatically for org/repo members and collaborators" — misleading when the actual cause is a transient error.
  Remediation: Add an error) case to the comment template that explains a transient error occurred and suggests re-running the workflow.

• [pagination-edge-case] scripts/check-e2e-authorization.sh — gh api --paginate without --slurp outputs separate JSON arrays per page. The jq expression processes each page independently, so last_push_at could contain multiple lines (one max per page), with bash string comparison using the multi-line value. Practical impact is limited (PRs with >100 commits are rare), but it is a correctness gap.
  Remediation: Use gh api ... --paginate --slurp | jq '[.[][] | .commit.committer.date] | max' or pipe through jq -s 'add' to flatten pages.

• [secrets-scope] .github/workflows/e2e_shim.yml:43 — secrets: inherit passes all repository secrets to the called workflow. Since e2e.yml runs from main (trusted) and pull_request events already have access to repo secrets, blast radius is limited. Explicitly listing required secrets would be more defensive.

• [naming-convention] .github/workflows/e2e_shim.yml — Uses underscore separator (e2e_shim.yml) while all other workflows in .github/workflows/ use hyphens. Consider renaming to e2e-shim.yml for consistency.

• [naming-convention] scripts/check-e2e-authorization_test.sh — Uses underscore separator (_test.sh) while all other test scripts in scripts/ use hyphens (e.g., post-triage-test.sh, validate-output-schema-test.sh). Consider renaming to check-e2e-authorization-test.sh.

Previous run (3)

Review

Findings

Medium

• [protected-path] .github/, scripts/ — This PR modifies 6 files under protected paths (.github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, .github/workflows/e2e_shim.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization_test.sh, scripts/resolve-e2e-gcp-auth.sh). The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale. Human approval is required for protected-path changes regardless of review outcome.

• [path-filter-mismatch] .github/workflows/e2e_shim.yml — The e2e_shim.yml paths filter is missing scripts/resolve-e2e-gcp-auth.sh, which IS included in the e2e.yml push-trigger paths (line 39). A PR that only modifies resolve-e2e-gcp-auth.sh will not trigger e2e tests on the PR but will trigger them on push to main, so regressions in that script would not be caught before merge.
  Remediation: Add 'scripts/resolve-e2e-gcp-auth.sh' to the paths list in e2e_shim.yml.

• [sensitive-data-defaults] .github/workflows/e2e.yml, e2e/admin/testutil.go — GCP project name (it-gcp-konflux-e2e-fullsend), service account ID (fullsend-e2e), and other infrastructure identifiers are hardcoded as defaults. CLAUDE.md line 15 prohibits committing "GCP project names, service account identifiers" and says "Use environment variables with no defaults for sensitive values." ADR 0043 documents this as an intentional design choice for reduced configuration overhead, and the WIF attribute condition prevents unauthorized access even if names are known. This tension between the project instruction and the ADR's design should be resolved.
  Remediation: Either (1) update CLAUDE.md to exempt shared test infrastructure identifiers that are protected by WIF attribute conditions, or (2) require these values as secrets/variables with no fallback defaults.

Low

• [error-comment-template] .github/actions/check-e2e-authorization/action.yml — When the authorization script fails (API error, rate limit), reason=error is set but the comment template's case statement has no error) branch. It falls through to *) which displays "E2E tests run automatically for org/repo members and collaborators" — misleading when the actual cause is a transient error.
  Remediation: Add an error) case to the comment template that explains a transient error occurred and suggests re-running the workflow.

• [pagination-edge-case] scripts/check-e2e-authorization.sh — gh api --paginate outputs separate JSON arrays per page (without --slurp). The jq expression [.[] | .commit.committer.date] | max processes each page independently, so last_push_at could reflect only one page's max rather than the global max. Same issue applies to events_json. Practical impact is limited (PRs with >30 commits are rare, and chronological ordering makes the last page likely to contain the latest date), but it's a correctness gap.
  Remediation: Use gh api ... --paginate --slurp | jq '[.[][] | .commit.committer.date] | max' or pipe through jq -s 'add' to flatten pages before extracting timestamps.

• [secrets-scope] .github/workflows/e2e_shim.yml:43 — secrets: inherit passes all repository secrets to the called workflow. While e2e.yml runs from main (trusted), explicitly listing only required secrets would limit blast radius if the workflow is later extended carelessly.

• [inline-script-complexity] .github/actions/check-e2e-authorization/action.yml:47 — The "Post gate comment" step contains a ~40-line inline shell script with case statements, string building, and API calls. Existing actions in the repo keep inline scripts under ~15 lines, with complex logic in scripts/. Consider extracting to scripts/post-e2e-gate-comment.sh.

• [naming-convention] .github/workflows/e2e_shim.yml — Uses underscore separator (e2e_shim.yml) while all other workflows in .github/workflows/ use hyphens (branch-cleanup.yml, site-build.yml, notify-adr-slack.yml, etc.). Consider renaming to e2e-shim.yml for consistency.

Previous run (4)

Review

Findings

Critical

• [api-contract] .github/workflows/e2e_shim.yml:35 — The call-e2e job declares permissions: { contents: read, pull-requests: write } but omits id-token: write. For workflow_call, the callee's effective permissions are the intersection of the caller's and callee's declared permissions. Since the shim (caller) does not grant id-token: write, the e2e job's id-token: write in e2e.yml will be ineffective, and the google-github-actions/auth OIDC token request will be denied. Every PR-triggered e2e run will fail at GCP authentication.
  Remediation: Add id-token: write to the call-e2e job's permissions block in e2e_shim.yml.

• [logic-error] docs/guides/infrastructure/e2e-gcp-setup.md — The documented WIF attribute condition includes assertion.ref == 'refs/heads/main'. For PR-triggered runs via the shim's pull_request event, the GitHub OIDC token's ref claim is refs/pull/N/merge, not refs/heads/main. This condition will reject every PR-triggered WIF token exchange, making the PR e2e path non-functional at the GCP layer. The same incorrect condition appears in ADR 0043 and docs/guides/infrastructure/infrastructure-reference.md. The job_workflow_ref check alone is sufficient to bind credentials to the trusted workflow on main.
  Remediation: Remove assertion.ref == 'refs/heads/main' && from the attribute condition in all locations (e2e-gcp-setup.md sections 3 create/update/expected, ADR 0043, infrastructure-reference.md). The condition should be: assertion.repository == 'fullsend-ai/fullsend' && assertion.job_workflow_ref.startsWith('fullsend-ai/fullsend/.github/workflows/e2e.yml@').

Medium

• [protected-path] .github/, scripts/ — This PR modifies files under protected paths (.github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, .github/workflows/e2e_shim.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization_test.sh). The PR links to Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale. Human approval is always required for protected-path changes regardless of context.

• [scope-creep] .github/workflows/e2e.yml:193, e2e/admin/testutil.go:963 — The GCP project name it-gcp-konflux-e2e-fullsend is hardcoded as a default in both the workflow and Go test code. CLAUDE.md explicitly prohibits committing "GCP project names" as sensitive data. Either amend CLAUDE.md to note this test project name is intentionally public, or remove the hardcoded defaults and require E2E_GCP_PROJECT_ID as a secret.

• [test-inadequate] scripts/check-e2e-authorization_test.sh — Test gaps: (1) CHECK_E2E_AUTH_DRY_RUN=true is set globally, so the gh api -X DELETE label-removal code path is never exercised; (2) no test case for OWNER author_association; (3) no test for the edge case where ok_to_test_at equals last_push_at (the > operator treats equal timestamps as stale — matches the spec but deserves an explicit test).

• [error-handling] scripts/check-e2e-authorization.sh:27 — Under set -euo pipefail, any gh api failure (rate limit, network error, 404) exits the script non-zero. The composite action's comment-posting step only runs when authorized != 'true', so if the check step itself fails, subsequent steps are skipped. The PR will show a red X on the gate job with no explanatory comment. Consider trapping errors to set authorized=false reason=error before exiting.

• [intent-scope-verification] docs/ADRs/0043-e2e-wif-shim-and-pr-authorization-gate.md — The PR body says "Closes Apply WIF to e2e test workflows via shim/gate pattern #1604" but ADR 0043 explicitly states this is an increment, not completion. Session, mint URL, and WIF config secrets remain required. Using "Closes" will auto-close an issue that is not fully addressed. Change to "Partially addresses Apply WIF to e2e test workflows via shim/gate pattern #1604" or verify the issue scope matches this increment.

Low

• [injection] .github/actions/check-e2e-authorization/action.yml:43 — ${{ inputs.pr_number }} and ${{ inputs.repository }} are interpolated directly into a bash run: command. Current callers pass safe GitHub-controlled values, but the pattern is fragile. The "Post gate comment" step correctly uses env: for the same inputs — apply the same pattern to the "Check authorization" step.

• [edge-case] scripts/check-e2e-authorization.sh:49 — Timestamp comparison uses bash string ordering, which is correct for ISO 8601 with identical timezone formats. GitHub API currently returns Z-suffixed UTC timestamps consistently, but the pattern is fragile if format changes.

• [naming-convention] .github/workflows/e2e_shim.yml — Workflow file uses underscore separator (e2e_shim.yml) while existing workflows predominantly use hyphens (reusable-dispatch.yml, branch-cleanup.yml, etc.). Consider renaming to e2e-shim.yml for consistency.

• [missing-doc-update] docs/architecture.md — ADR 0043 introduces a third WIF use case (e2e CI, alongside token mint and inference), but docs/architecture.md was not updated to reflect this.

Info

• [internal-consistency] docs/ADRs/0043-e2e-wif-shim-and-pr-authorization-gate.md — The Mermaid diagram shows "gate job fails, tests skipped" for the unauthorized path, but the gate job actually succeeds (exits 0) with authorized=false output. The e2e job skips itself via its if condition.

• [privilege-scope] .github/workflows/e2e.yml — Prior concern about the removed top-level permissions: block is resolved. Both jobs (gate and e2e) now declare explicit job-level permissions: blocks, enforcing least-privilege per job.

Previous run (5)

Review

Findings

Critical

• [api-contract] e2e_shim.yml:35 — The call-e2e job declares permissions: { contents: read, pull-requests: write } but omits id-token: write. For workflow_call, the callee's effective permissions are the intersection of the caller's and callee's declared permissions. Since the shim (caller) does not grant id-token: write, the e2e job's id-token: write in e2e.yml will be ineffective, and the google-github-actions/auth OIDC token request will be denied. Every PR-triggered e2e run will fail at GCP authentication.
  Remediation: Add id-token: write to the call-e2e job's permissions block in e2e_shim.yml.

• [logic-error] docs/guides/infrastructure/e2e-gcp-setup.md — The documented WIF attribute condition includes assertion.ref == 'refs/heads/main'. For PR-triggered runs via the shim's pull_request event, the GitHub OIDC token's ref claim is refs/pull/N/merge, not refs/heads/main. This condition will reject every PR-triggered WIF token exchange, making the PR e2e path non-functional at the GCP layer. The same incorrect condition appears in ADR 0043, the infrastructure reference, and the gcloud create/update commands in the setup guide. The job_workflow_ref check alone is sufficient to ensure only the trusted e2e.yml@main definition can authenticate.
  Remediation: Remove assertion.ref == 'refs/heads/main' && from the attribute condition in all locations (e2e-gcp-setup.md sections 3 create/update/expected, ADR 0043 lines 378-380, infrastructure-reference.md). The condition should be: assertion.repository == 'fullsend-ai/fullsend' && assertion.job_workflow_ref.startsWith('fullsend-ai/fullsend/.github/workflows/e2e.yml@').

Medium

• [protected-path] .github/, scripts/ — This PR modifies files under protected paths (.github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, .github/workflows/e2e_shim.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization_test.sh). The PR links to Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale. Human approval is always required for protected-path changes regardless of context.

• [test-inadequate] scripts/check-e2e-authorization_test.sh — Test gaps: (1) CHECK_E2E_AUTH_DRY_RUN=true is set globally, so the gh api -X DELETE label-removal code path is never exercised; (2) no test case for OWNER author_association; (3) no test for the edge case where ok_to_test_at equals last_push_at (the > operator treats equal timestamps as stale — matches the spec but deserves an explicit test).

• [error-handling] scripts/check-e2e-authorization.sh:27 — Under set -euo pipefail, any gh api failure (rate limit, network error, 404) exits the script non-zero. The composite action's comment-posting step only runs when authorized != 'true', so if the check step itself fails, subsequent steps are skipped. The PR will show a red X on the gate job with no explanatory comment. Consider trapping errors to set authorized=false reason=error before exiting.

Low

• [injection] .github/actions/check-e2e-authorization/action.yml:43 — ${{ inputs.pr_number }} and ${{ inputs.repository }} are interpolated directly into a bash run: command. Current callers pass safe GitHub-controlled values, but the pattern is fragile if a future caller passes attacker-controlled strings. Prefer passing inputs via env: variables.

• [privilege-scope] .github/workflows/e2e.yml — The top-level permissions: block is removed. For push and workflow_dispatch triggers, the absence of a top-level permissions block means the repository's default token permissions apply. Adding permissions: {} at the top level would enforce least-privilege for all trigger paths.

• [edge-case] scripts/check-e2e-authorization.sh:49 — Timestamp comparison uses bash string ordering, which is correct for ISO 8601 with identical timezone formats. GitHub API currently returns Z-suffixed UTC timestamps consistently, but the pattern is fragile if format changes.

• [missing-doc-update] docs/architecture.md — ADR 0043 introduces a third WIF use case (e2e CI, alongside token mint and inference), but docs/architecture.md was not updated to reflect this.

Info

• [internal-consistency] docs/ADRs/0043-e2e-wif-shim-and-pr-authorization-gate.md — The Mermaid diagram shows "gate job fails, tests skipped" for the unauthorized path, but the gate job actually succeeds (exits 0) with authorized=false output. The e2e job skips itself via its if condition.

[fullsend-ai-review]

See the review comment for full details.

[ralphbean]

The bot's two critical findings are right — I +1'd those and added a docs link confirming the ref claim behavior. One more note inline.

[fullsend-ai-review]

See the review comment for full details.

[waynesun09]

Review Squad Report — 6-agent parallel review

Agents: 2 claude-coder, 2 claude-researcher, 1 gemini, 1 cursor
Verified findings: 2 HIGH, 6 MEDIUM (inline), 7 LOW, 3 INFO
False positives removed: 5 (pagination, labeled+paths, ADR edits, DOCS_URL injection, concurrency collision)

Summary

The security architecture is well-designed — shim→trusted workflow via workflow_call@refs/heads/main, WIF bound by job_workflow_ref, two-layer authorization gate. ADR 0043 and docs are thorough.

HIGH items: (1) Backdated committer.date bypasses ok-to-test freshness — use server-side timestamps instead of git metadata. (2) gcloud projects describe fails without credentials on fresh runners — hardcode the WIF provider resource name to eliminate runtime dependency.

MEDIUM items: ERR trap needs diagnostics, shim missing path filter + top-level permissions: {}, test coverage gaps (CONTRIBUTOR assoc, trusted+label, resolve script), fragile two-pass resolve pattern, always() → !cancelled().

Not flagged (LOW): comment whitespace, action SHA pinning, timestamp string comparison, unused pr_author input, TOCTOU race, trusted author label cleanup, fork PR filtering.

[ralphbean]

Previous findings (561961d) all addressed -- thanks. One thing still needs sorting out before we can merge. Two non-blocking notes inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:37 PM UTC · Completed 10:50 PM UTC
Commit: d0ac11b · View workflow run →

[fullsend-ai-review]

🤖 Review · Started 11:02 PM UTC
Commit: d0ac11b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:13 PM UTC · Completed 11:25 PM UTC
Commit: d0ac11b · View workflow run →

[ralphbean]

Previous findings addressed. One thing to sort out — see inline.

[ralphbean]

This branch predates 6aef818, which added merge_group: and a Check for e2e-relevant changes step. That step lets us require e2e in branch protection — the workflow succeeds on PRs that don't touch e2e files instead of skipping.

After rebasing, it'd be worth thinking through how that fits with the shim. The shim's paths: filter prevents the workflow from firing on non-matching PRs, so we can't make e2e a required check. If we want that option, the shim could fire unconditionally and let the relevance check inside e2e.yml decide.

[ifireball]

I started working on this with the wrong assumptions and ended up with a solution that is far more comlex and cumbersome then what we actually needed. Opened PR #2106 to redo this with fresh approach

[ifireball]

Superseded by #2106

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:25 AM UTC · Completed 8:30 AM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-retro]

Retro: PR #1983 — feat(e2e): WIF shim and PR authorization gate

Outcome: Closed without merge after 3 human review rounds and 6 review bot runs. Superseded by PR #2106 with a simpler approach.

Timeline

• Jun 7 12:51 — PR opened by ifireball (+1113/-22, 16 files)
• Jun 7 13:13 — Review bot round 1: CHANGES_REQUESTED (2 critical, 2 medium, 2 low)
• Jun 7 14:05 — Human review 1 (ralphbean): CHANGES_REQUESTED, confirmed bot's critical findings
• Jun 7 21:26 — Review bot round 2: same findings re-raised on same code
• Jun 7 22:08 — Author pushed fixes, resolved all findings
• Jun 8 15:03 — 6-agent review squad: found 2 HIGH (committer date spoofing, gcloud circular dependency)
• Jun 8 18:25 — Human review 2: CHANGES_REQUESTED (gcloud circular dependency blocking)
• Jun 8 23:10 — Author pushed second fix round
• Jun 9 21:19 — Human review 3: APPROVED
• Jun 10 08:22 — Author concluded approach was too complex, closed PR
• Jun 10 08:23 — Superseded by feat(e2e): gate fork PR runs with pull_request_target #2106

Key observations

Redundant review bot runs: 6 runs produced near-identical findings. The same 2 critical issues were flagged in 4 of 6 runs with near-verbatim wording. This is already well-covered by 12+ open issues (#893, #963, #1271, #1331, #1357, #1370, #1372, #1418, #1422, #1452, #1557, #1362).

Re-raised findings: The bot re-posted findings that the author had already addressed. Covered by issues #1013, #1155, #1285, #1583.

Wasted effort on superseded PR: All review work was discarded. Partially covered by issues #1398, #1414, #1439.

One new proposal below addresses the gap: no mechanism exists to detect that a PR is not converging toward merge and may benefit from an approach reassessment before further review investment.

Proposals filed

• Review agent: detect review-cycle churn and suggest approach reassessment (in fullsend-ai/fullsend)