feat(e2e): gate fork PR runs with pull_request_target

[ifireball]

Closes #1604

Summary

Progress on #1604: enable fork PR e2e runs without the shim/workflow_call indirection proposed in earlier designs. This PR uses a simplified single-workflow design (pull_request_target + separate gate job) that provides the same security properties as the shim/gate pattern:

• Base-branch workflow definition (PR authors cannot modify the gate)
• Authorization before checkout of untrusted PR head code
• Trusted author or maintainer-applied ok-to-test after latest push
• Sticky PR comment when blocked; stale label removal on new commits

GCP access via WIF is unchanged — google-github-actions/auth with E2E_GCP_WIF_PROVIDER / E2E_GCP_SERVICE_ACCOUNT was already implemented for e2e tests; this PR does not alter that path.

ADR 0009 does not apply here. ADR 0009 documents pull_request_target for shim workflows in enrolled repos that consume fullsend (static dispatch curl, no PR code checkout). This change is an internal CI workflow for fullsend itself, with a different threat model: it intentionally checks out fork PR code after authorization, documented in e2e-testing.md.

Changes

• Switch PR-triggered e2e from pull_request to pull_request_target so fork PRs receive repository secrets
• Add a separate gate job that authorizes trusted authors (OWNER/MEMBER/COLLABORATOR) or a fresh ok-to-test label before the e2e job checks out PR head code
• Post/update a sticky PR comment when e2e is blocked; remove stale ok-to-test labels after new pushes
• Add authorization script tests and e2e testing guide

Protected-path changes (.github/workflows, .github/actions, scripts/) implement the authorization gate above.

Test plan

• bash scripts/check-e2e-authorization-test.sh passes locally
• Member/collaborator PR: gate passes, e2e runs
• Fork PR without label: sticky comment posted, e2e skipped
• Fork PR with maintainer ok-to-test after latest push: e2e runs on head SHA
• New push after ok-to-test: label removed, e2e skipped until re-labeled
• Non-ok-to-test label event does not bypass gate (fixed in 0343838)
• Create ok-to-test label in repo settings if missing

[github-actions]

Site preview

Preview: https://170a1f81-site.fullsend-ai.workers.dev

Commit: d7648e8da5e4e29cc6af51cb01cfedfa6f4bdad2

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:56 AM UTC · Completed 7:11 AM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [architectural-misalignment] .github/workflows/e2e.yml — ADR 0009 ("Use pull_request_target in shim workflows") is explicitly scoped to shim workflows in enrolled repos with a specific threat model (no checkout, no code execution). The e2e workflow's pull_request_target usage has a fundamentally different security model (authorization gate → checkout of fork code → secret access). While the PR's claim that "ADR 0009 does not apply" is correct, a new ADR documenting the e2e workflow's distinct pull_request_target pattern and its mitigations (gate job, permission separation, persist-credentials: false) would prevent future confusion about which security model applies where. See also: [stale-adr-reference] finding below.

• [scope-misalignment] .github/workflows/e2e.yml — Issue Apply WIF to e2e test workflows via shim/gate pattern #1604 proposed a three-workflow shim/gate pattern with job_workflow_ref validation. The PR implements a simplified single-workflow, two-job design without workflow_call or job_workflow_ref. The PR description acknowledges this divergence and the simplification is architecturally sound for a single-repo CI workflow, but the issue should be updated to reflect the accepted design.

• [stale-adr-reference] docs/ADRs/0009-pull-request-target-in-shim-workflows.md — ADR 0009 exclusively documents pull_request_target for shim workflows. With this PR, the project now has two distinct pull_request_target usage patterns with different security models. A companion ADR documenting the CI workflow authorization gate pattern would help future readers understand the distinction. See also: [architectural-misalignment] finding above.

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale for these changes (implementing the authorization gate). Human approval is always required for protected-path changes, regardless of context.

Low

• [architectural-trajectory-divergence] .github/workflows/e2e.yml — The broader architectural trajectory (ADRs 0029, 0031, 0033, 0041) moves toward workflow_call + OIDC for agent dispatch workflows. The e2e workflow uses pull_request_target instead, which is appropriate since workflow_call cannot solve the fork PR secrets problem — fork PRs cannot trigger workflow_call in the target repo. This is a different problem domain from agent dispatch.

• [missing-workflow-documentation] docs/guides/dev/testing-workflows.md — The existing testing-workflows.md guide could cross-reference the new docs/guides/dev/e2e-testing.md guide for contributors who land there first.

• [edge-case] .github/workflows/e2e.yml:60 — The gate job checks out ref: github.sha (base branch HEAD for pull_request_target). Bug fixes to the authorization script require merging to main before they take effect for fork PRs. This is intentional pull_request_target behavior, documented in e2e-testing.md and workflow comments.

• [edge-case] scripts/check-e2e-authorization.sh:65 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as updated_at is treated as stale. Fails closed by design.

• [secrets-exposure] .github/workflows/e2e.yml:70 — The e2e job checks out untrusted fork code and runs make e2e-test with access to secrets. Mitigated by gate authorization, persist-credentials: false, and permission separation. Accepted design trade-off.

• [removed-feature-undocumented] .github/workflows/e2e.yml — The previous changes step (runtime e2e-relevance check via GitHub API) is replaced by paths: filters on the pull_request_target trigger. The push trigger already had these paths: filters. The functional behavior is equivalent but moved from runtime to trigger-level filtering.

Info

• [permission-reduction] .github/workflows/e2e.yml:6 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job. Positive least-privilege change.

• [credential-hardening] .github/workflows/e2e.yml:73 — persist-credentials: false on the e2e job checkout prevents the GITHUB_TOKEN from being persisted in git config when checking out untrusted fork code. Positive hardening.

• [credential-hardening] .github/actions/check-e2e-authorization/action.yml:72 — BOT_LOGIN now hardcoded to github-actions[bot] (fixed since prior review). Prevents the comment upsert logic from searching for comments by the wrong user.

• [fail-closed] .github/workflows/e2e.yml:67 — The e2e job condition needs.gate.outputs.authorized == 'true' combined with !cancelled() and event_name check ensures no fail-open bypass. A skipped gate leaves authorized empty (not 'true'), blocking execution.

• [pull_request_target-safety] .github/workflows/e2e.yml:52 — Gate job checks out ref: github.sha (base branch SHA), ensuring the authorization script runs from trusted code.

• [injection-review] scripts/check-e2e-authorization.sh — All values written to GITHUB_OUTPUT are hardcoded strings (true/false, fixed reason codes). No user-controlled data flows into workflow commands.

• [workflow-permission] .github/workflows/e2e.yml:48 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

Previous run

Review

Findings

High

• [logic-error] .github/actions/check-e2e-authorization/action.yml:68 — BOT_LOGIN is set to ${{ github.actor }} (the human who triggered the event, e.g., the PR author on synchronize, the maintainer on labeled). But comments are posted via github.token, which authenticates as github-actions[bot]. The comment lookup filters by BOT_LOGIN to find existing gate comments, so it will never match comments created by github-actions[bot]. This causes: (1) a new duplicate comment on every unauthorized run instead of updating the existing one, and (2) the "Clear gate comment" step will never find or update the sticky comment when authorization passes.
  Remediation: Replace BOT_LOGIN: ${{ github.actor }} with BOT_LOGIN: github-actions[bot] in both the "Post gate comment" (line 68) and "Clear gate comment" (line 117) steps.

Medium

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale for these changes (implementing the authorization gate). Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] .github/workflows/e2e.yml:60 — The gate job checks out ref: github.sha (base branch HEAD for pull_request_target). Bug fixes to the authorization script require merging to main before they take effect for fork PRs. This is intentional pull_request_target behavior, partially documented in e2e-testing.md and workflow comments.

• [edge-case] scripts/check-e2e-authorization.sh:65 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as updated_at is treated as stale. Fails closed by design.

• [test-adequacy] scripts/check-e2e-authorization-test.sh:169 — The test case "gh api failure on events returns error reason" sets GH_FAIL=true which causes ALL gh calls to fail, including the initial PR fetch. The test name is misleading — it tests total API unavailability rather than events-only failure.

• [secrets-exposure] .github/workflows/e2e.yml:70 — The e2e job checks out untrusted fork code and runs make e2e-test with access to secrets. Mitigated by gate authorization, persist-credentials: false, and permission separation. Accepted design trade-off.

• [error-handling-pattern] scripts/check-e2e-authorization.sh:23 — The ERR trap calls write_error_output (authorized=false, fail-closed) and then exit 0. Deliberate design choice for composite actions but makes debugging harder.

• [scope-divergence] .github/workflows/e2e.yml — Issue Apply WIF to e2e test workflows via shim/gate pattern #1604 proposed a three-workflow shim/gate pattern with job_workflow_ref validation. The PR implements a simplified single-workflow, two-job design. The PR description acknowledges this divergence. The architectural pivot should be documented.

Info

• [permission-reduction] .github/workflows/e2e.yml:4 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job. Positive least-privilege change.

• [credential-hardening] .github/workflows/e2e.yml:73 — persist-credentials: false on the e2e job checkout prevents the GITHUB_TOKEN from being persisted in git config when checking out untrusted fork code. Positive hardening.

• [fail-closed] .github/workflows/e2e.yml:67 — The e2e job condition needs.gate.outputs.authorized == 'true' combined with !cancelled() and event_name check ensures no fail-open bypass. A skipped gate leaves authorized empty (not 'true'), blocking execution.

• [pull_request_target-safety] .github/workflows/e2e.yml:52 — Gate job checks out ref: github.sha (base branch SHA), ensuring the authorization script runs from trusted code.

• [injection-review] scripts/check-e2e-authorization.sh — All values written to GITHUB_OUTPUT are hardcoded strings (true/false, fixed reason codes). No user-controlled data flows into workflow commands.

• [workflow-permission] .github/workflows/e2e.yml:48 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

Previous run (2)

Review

Findings

Medium

• [api-contract-violation] .github/actions/check-e2e-authorization/action.yml:99 — Both the "Post gate comment" and "Clear gate comment" steps call gh api user --jq .login to discover the bot login. The GH_TOKEN is set to github.token, which is a GitHub App installation token, not a user token. The /user endpoint returns 403 for app installation tokens. With set -euo pipefail, this will cause both steps to fail — the gate comment will never be posted or cleared, and the gate job will show as failed. The authorization decision itself is unaffected (outputs are already written by the prior step), but the sticky comment UX feature is broken.
  Remediation: Replace gh api user --jq .login with a hardcoded github-actions[bot] string, or use gh api /app --jq '.slug + "[bot]"'.

• [scope-divergence] .github/workflows/e2e.yml — Issue Apply WIF to e2e test workflows via shim/gate pattern #1604 proposed a three-workflow shim/gate pattern (test_shim.yml, test_gate.yml, test.yaml) with job_workflow_ref validation. The PR implements a simplified single-workflow, two-job design that does not use job_workflow_ref. The PR description acknowledges this divergence and the simplification is architecturally sound, but the architectural pivot from the authorized scope should be documented.
  Remediation: Update issue Apply WIF to e2e test workflows via shim/gate pattern #1604 with a comment explaining the accepted design, or create a brief ADR for the authorization gate pattern.

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale for these changes (implementing the authorization gate). Human approval is always required for protected-path changes, regardless of context.

Low

• [error-handling-pattern] scripts/check-e2e-authorization.sh:23 — The ERR trap calls write_error_output (which sets authorized=false, fail-closed — correct) and then exit 0. This is a deliberate design choice for composite actions (exit non-zero would prevent subsequent steps from running), but it means the gate job shows as successful even on script errors, making debugging harder.

• [architectural-coherence] docs/guides/dev/e2e-testing.md — The new guide references ADR 0009 and documents the gate pattern inline, but no ADR exists for the e2e gate pattern itself. ADR 0009 is explicitly scoped to shim workflows with a different security model (no code checkout). A dedicated ADR would help future readers understand why the e2e pattern differs.

• [edge-case] scripts/check-e2e-authorization.sh:65 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as updated_at is treated as stale. This is documented behavior in e2e-testing.md and fails closed.

• [test-adequacy] scripts/check-e2e-authorization-test.sh:155 — The test case "gh api failure on events returns error reason" sets GH_FAIL=true which causes ALL gh calls to fail, including the initial PR fetch. The ERR trap fires on the PR fetch, not the events API call. The mock supports GH_FAIL=events (selective events failure) but this mode is only tested with a trusted author who never reaches the events code path. Adding a test with GH_FAIL=events for an untrusted author with ok-to-test label would cover this gap.

Info

• [credential-hardening] .github/workflows/e2e.yml:73 — persist-credentials: false added to the e2e job checkout step since the prior review. This prevents the checked-out fork code from accessing GITHUB_TOKEN via the git credential helper. Positive hardening change.

• [permission-reduction] .github/workflows/e2e.yml:3 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job — positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:46 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

• [fail-closed] .github/workflows/e2e.yml:52 — When the gate job is skipped (non-PR events), needs.gate.outputs.authorized is empty (not 'true'), and the e2e job condition short-circuits on github.event_name != 'pull_request_target'. No fail-open bypass exists.

• [pull_request_target-safety] .github/workflows/e2e.yml:39 — Gate job checks out ref: github.sha (base branch SHA), ensuring the authorization script runs from trusted code, not attacker-controlled fork code.

• [injection-review] scripts/check-e2e-authorization.sh — All values written to GITHUB_OUTPUT are hardcoded strings (true/false, fixed reason codes). No user-controlled data flows into workflow commands.

• [secrets-exposure] .github/workflows/e2e.yml:62 — The e2e job checks out untrusted fork code and runs it with secrets (session tokens, GCP WIF). Mitigated by the authorization gate requiring trusted author association or explicit ok-to-test label from a collaborator, plus persist-credentials: false.

Previous run (3)

Review

Findings

Medium

• [api-contract-violation] .github/actions/check-e2e-authorization/action.yml:99 — Both the "Post gate comment" and "Clear gate comment" steps call gh api user --jq .login to discover the bot login. The GH_TOKEN is set to github.token, which is a GitHub App installation token, not a user token. The /user endpoint returns 403 for app installation tokens. With set -euo pipefail, this will cause both steps to fail. The authorization decision is unaffected (outputs are already written by the prior step), but the PR commenting feature (sticky gate explanation) will be broken, and the gate job will always show as failed.
  Remediation: Replace gh api user --jq .login with a hardcoded github-actions[bot] string, or use gh api /app --jq '.slug + "[bot]"'.

• [error-handling-pattern] scripts/check-e2e-authorization.sh:23 — The ERR trap calls write_error_output (which sets authorized=false, fail-closed — correct) and then exit 0. The GitHub Actions step always reports success even on API failures or script bugs. The fail-closed authorization behavior is correct, but CI operators have no visible signal that something went wrong. The script comments document this as intentional (Exits 0 always; callers inspect outputs).
  Remediation: Consider exit 1 instead of exit 0. The authorized=false output is already written to GITHUB_OUTPUT before the exit, so e2e still won't run. The gate failure would provide a visible signal.

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale for these changes (implementing the authorization gate). Human approval is always required for protected-path changes, regardless of context.

Low

• [incomplete-doc] docs/ADRs/0009-pull-request-target-in-shim-workflows.md — ADR 0009 is scoped exclusively for shim workflows and states the shim's safety model: no checkout, no build, no script execution from the PR. The e2e workflow now uses pull_request_target with a different security model (checks out PR code, gates execution via authorization). The new e2e-testing.md guide references ADR 0009 and explains the distinction, partially closing this gap. A dedicated ADR or cross-reference in ADR 0009 would help future readers.

• [edge-case] scripts/check-e2e-authorization.sh:65 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as updated_at is treated as stale. This is documented behavior in e2e-testing.md ("strictly after") and fails closed.

• [test-adequacy] scripts/check-e2e-authorization-test.sh:155 — The test case "gh api failure on events returns error reason" sets GH_FAIL=true which causes ALL gh calls to fail, including the initial PR fetch. The ERR trap fires on the PR fetch, not the events API call. The mock supports GH_FAIL=events (selective events failure) but this mode is only tested with a trusted author who never reaches the events code path. Adding a test with GH_FAIL=events for an untrusted author with ok-to-test label would cover this gap.

• [stale-reference] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md:162 — The Security sub-agent skill document warns against pull_request_target workflows running fork code with access to secrets as a blanket anti-pattern. The e2e workflow now implements exactly this pattern with an authorization gate mitigation. The guidance does not distinguish between ungated (unsafe) and gated (safe) pull_request_target usage, which could cause false positives in future reviews.

Info

• [permission-reduction] .github/workflows/e2e.yml:3 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job — positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:46 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

• [fail-closed] .github/workflows/e2e.yml:52 — When the gate job is skipped (non-PR events), needs.gate.outputs.authorized is empty (not 'true'), and the e2e job condition short-circuits on github.event_name != 'pull_request_target'. For PR events, gate must explicitly output authorized=true. No fail-open bypass exists.

• [pull_request_target-safety] .github/workflows/e2e.yml:39 — Gate job checks out ref: github.sha (base branch SHA), ensuring the authorization script runs from trusted code, not attacker-controlled fork code.

• [injection-review] scripts/check-e2e-authorization.sh — All values written to GITHUB_OUTPUT are hardcoded strings (true/false, fixed reason codes). No user-controlled data flows into workflow commands.

• [secrets-exposure] .github/workflows/e2e.yml:62 — The e2e job checks out untrusted fork code and runs it with secrets (session tokens, GCP WIF). Mitigated by the authorization gate requiring trusted author association or explicit ok-to-test label from a collaborator.

Previous run (4)

Review

Findings

Medium

• [authorization-bypass] scripts/check-e2e-authorization.sh:63 — The committer.date fallback in last_push_at computation is attacker-controlled for fork PRs. Scenario: (1) maintainer applies ok-to-test after reviewing a fork PR, (2) attacker pushes additional non-force-push commits with GIT_COMMITTER_DATE backdated to before the ok-to-test label time, (3) no head_ref_force_pushed event is created for normal pushes, so max() of committed.committer.date values resolves to the backdated time, (4) the stale ok-to-test label appears fresh and the e2e job runs the attacker's new code with secrets. The primary defense (trusted author check via author_association) is unaffected; this applies only to the ok-to-test label freshness path.
  Remediation: Use the synchronize event from the issue events API (which has a server-side created_at) to determine the latest push time, or compare the PR head SHA at ok-to-test time against the current head SHA instead of using timestamps.

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR links to issue Apply WIF to e2e test workflows via shim/gate pattern #1604 and the description explains the rationale for these changes (implementing the authorization gate). Human approval is always required for protected-path changes, regardless of context.

• [error-handling-pattern] scripts/check-e2e-authorization.sh:23 — The ERR trap sets authorized=false (fail-closed) and exits 0. Fail-closed for authorization is correct, but exit 0 means the GitHub Actions step reports success even on API failures or script bugs, making debugging harder. This is also inconsistent with established scripts (post-triage.sh, post-code.sh, validate-output-schema.sh) which exit non-zero on errors.
  Remediation: Consider exiting non-zero from the ERR trap so the step is visibly failed, while still writing authorized=false to GITHUB_OUTPUT. The e2e job condition already handles this — gate failure means authorized output is not 'true'.

Low

• [edge-case] scripts/check-e2e-authorization.sh:81 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as the last push is treated as stale. This is documented behavior ("strictly after") and fails closed, but could cause occasional maintainer confusion.

• [architectural-coherence] .github/workflows/e2e.yml — The PR introduces a pull_request_target security pattern (single-workflow, two-job gate) that differs from the shim workflow pattern documented in ADR 0009. ADR 0009 is explicitly scoped to shim workflows in enrolled repos and does not prohibit other uses. The new e2e-testing.md guide references ADR 0009 and explains the distinction. A formal ADR for the gate pattern would close the documentation gap.

• [scope-alignment] docs/guides/dev/e2e-testing.md — Issue Apply WIF to e2e test workflows via shim/gate pattern #1604 proposed a multi-workflow architecture (test_shim.yml, test_gate.yml, test.yaml). The PR delivers a simplified single-workflow, two-job design. The PR description explains the simplification rationale.

• [incomplete-doc] docs/ADRs/0009-pull-request-target-in-shim-workflows.md — ADR 0009 discusses pull_request_target exclusively for shim workflows. The e2e workflow uses pull_request_target with a different security model (authorization gate with code checkout). A cross-reference or new ADR would help future readers.

• [edge-case] scripts/check-e2e-authorization.sh:62 — The jq -s 'add' pattern after gh api --paginate produces null on empty input. Subsequent jq queries degrade to empty strings via // empty, and the ERR trap provides a fail-closed safety net.

Info

• [permission-reduction] .github/workflows/e2e.yml:4 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job — positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:46 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

• [fail-open] .github/workflows/e2e.yml:52 — When the gate job is skipped (e.g., labeled events for non-ok-to-test labels), needs.gate.outputs.authorized is empty (not 'true'), so the e2e job does NOT run. No bypass exists.

Previous run (5)

Review

Findings

Medium

• [logic-error] .github/workflows/e2e.yml — cancel-in-progress uses github.ref != 'refs/heads/main' but for pull_request_target events, github.ref is always the base branch (typically refs/heads/main). This means cancel-in-progress evaluates to false for all fork PRs targeting main, so rapid pushes to a fork PR will queue up multiple e2e runs instead of canceling the previous one. The concurrency group is correctly scoped per-PR (e2e-{number}), but the cancel condition defeats its purpose.
  Remediation: Change to cancel-in-progress: ${{ github.event_name == 'pull_request_target' || github.ref != 'refs/heads/main' }}

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

Low

• [architectural-coherence] .github/workflows/e2e.yml — The PR introduces a new pull_request_target security pattern (single-workflow, two-job gate) that differs from the documented shim/gate multi-workflow pattern in ADR 0009. The e2e-testing.md guide explains the approach and acknowledges the difference, but a formal ADR would close the architectural documentation gap.

• [missing-cross-reference] docs/ADRs/0009-pull-request-target-in-shim-workflows.md — ADR 0009 discusses pull_request_target security for shim workflows but does not mention or reference the e2e workflow which also uses pull_request_target with a different authorization pattern. The new e2e-testing.md guide references ADR 0009, but ADR 0009 does not reciprocate this cross-reference.

• [edge-case] scripts/check-e2e-authorization.sh:81 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as the last push is treated as stale. This is documented behavior ("strictly after") and fails closed, but could cause occasional maintainer confusion.

• [error-handling] scripts/check-e2e-authorization.sh:62 — The jq -s 'add' pattern after gh api --paginate produces null on empty input. Subsequent jq queries degrade gracefully (returning empty strings via // empty), but this is an implicit contract.

• [defense-in-depth] scripts/check-e2e-authorization.sh:63 — The committer.date fallback path (lines 63-65) uses git-authored timestamps that an attacker controls. If the timeline API returns no committed/force-push events, an attacker could backdate committer.date to make a stale ok-to-test label appear fresh. The primary timeline path uses server-side timestamps and is safe; this is a defense-in-depth concern for the fallback only.

• [error-visibility] scripts/check-e2e-authorization.sh:10 — The ERR trap sets authorized=false (fail-closed) and exits 0. While authorization is correctly denied, exit 0 means the GitHub Actions step reports success even on API failures. The gate comment does post a reason=error message for visibility.

• [secret-exposure] .github/workflows/e2e.yml:64 — The e2e job checks out fork PR head code with access to secrets (session tokens, GCP credentials). The authorization gate logic is sound, but future modifications to the auth script could widen the trust boundary.

Info

• [scope-alignment] — Issue Apply WIF to e2e test workflows via shim/gate pattern #1604 proposed a multi-workflow architecture (test_shim.yml, test_gate.yml, test.yaml). The PR delivers a different architecture (single workflow, two jobs). The PR description explains the simplification rationale.

• [test-adequacy] scripts/check-e2e-authorization-test.sh:175 — The new test case "commits api fallback with fresh ok-to-test label" addresses the prior finding about missing coverage for the commits-API fallback path with label freshness comparison.

• [fail-open] .github/workflows/e2e.yml:52 — When the gate job is skipped (e.g., labeled events for non-ok-to-test labels), needs.gate.outputs.authorized is empty (not 'true'), so the e2e job does NOT run. No bypass exists.

• [permission-reduction] .github/workflows/e2e.yml:3 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job — positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:44 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

Previous run (6)

Review

Findings

Medium

• [logic-error] .github/workflows/e2e.yml — cancel-in-progress uses github.ref != 'refs/heads/main' but for pull_request_target events, github.ref is always the base branch (typically refs/heads/main). This means cancel-in-progress evaluates to false for all fork PRs targeting main, so rapid pushes to a fork PR will queue up multiple e2e runs instead of canceling the previous one. The concurrency group is correctly scoped per-PR (e2e-{number}), but the cancel condition defeats its purpose.
  Remediation: Change to cancel-in-progress: ${{ github.event_name == 'pull_request_target' || github.ref != 'refs/heads/main' }}

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

Low

• [architectural-coherence] .github/workflows/e2e.yml — The PR introduces a new pull_request_target security pattern (single-workflow, two-job gate) that differs from the documented shim/gate multi-workflow pattern in ADR 0009. The e2e-testing.md guide explains the approach and acknowledges the difference, but a formal ADR would close the architectural documentation gap.

• [missing-cross-reference] docs/ADRs/0009-pull-request-target-in-shim-workflows.md — ADR 0009 discusses pull_request_target security for shim workflows but does not mention or reference the e2e workflow which also uses pull_request_target with a different authorization pattern. The new e2e-testing.md guide references ADR 0009, but ADR 0009 does not reciprocate this cross-reference.

• [edge-case] scripts/check-e2e-authorization.sh:81 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as the last push is treated as stale. This is documented behavior ("strictly after") and fails closed, but could cause occasional maintainer confusion.

• [error-handling] scripts/check-e2e-authorization.sh:62 — The jq -s 'add' pattern after gh api --paginate produces null on empty input. Subsequent jq queries degrade gracefully (returning empty strings via // empty), but this is an implicit contract.

• [defense-in-depth] scripts/check-e2e-authorization.sh:63 — The committer.date fallback path (lines 63-65) uses git-authored timestamps that an attacker controls. If the timeline API returns no committed/force-push events, an attacker could backdate committer.date to make a stale ok-to-test label appear fresh. The primary timeline path uses server-side timestamps and is safe; this is a defense-in-depth concern for the fallback only.

• [error-visibility] scripts/check-e2e-authorization.sh:10 — The ERR trap sets authorized=false (fail-closed) and exits 0. While authorization is correctly denied, exit 0 means the GitHub Actions step reports success even on API failures. The gate comment does post a reason=error message for visibility.

• [secret-exposure] .github/workflows/e2e.yml:64 — The e2e job checks out fork PR head code with access to secrets (session tokens, GCP credentials). The authorization gate logic is sound, but future modifications to the auth script could widen the trust boundary.

Info

• [scope-alignment] — Issue Apply WIF to e2e test workflows via shim/gate pattern #1604 proposed a multi-workflow architecture (test_shim.yml, test_gate.yml, test.yaml). The PR delivers a different architecture (single workflow, two jobs). The PR description explains the simplification rationale.

• [test-adequacy] scripts/check-e2e-authorization-test.sh:175 — The new test case "commits api fallback with fresh ok-to-test label" addresses the prior finding about missing coverage for the commits-API fallback path with label freshness comparison.

• [fail-open] .github/workflows/e2e.yml:52 — When the gate job is skipped (e.g., labeled events for non-ok-to-test labels), needs.gate.outputs.authorized is empty (not 'true'), so the e2e job does NOT run. No bypass exists.

• [permission-reduction] .github/workflows/e2e.yml:3 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job — positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:44 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

Previous run (7)

Review

Findings

High

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR has no linked issue justifying changes to governance/infrastructure files. Human approval is required for all protected-path changes regardless of context, and the absence of a linked issue means the change lacks formal authorization.
  Remediation: Link an issue that authorizes these infrastructure changes.

Low

• [edge-case] scripts/check-e2e-authorization.sh:81 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as the last push is treated as stale. This is documented behavior ("strictly after") and fails closed, but could cause occasional maintainer confusion.

• [test-inadequate] scripts/check-e2e-authorization-test.sh:161 — The "commits api fallback when timeline has no push events" test case has no ok-to-test label, so it only exercises the "unauthorized" path. The interaction between the commits-API-derived last_push_at and the label freshness comparison is untested. Adding a test case with an untrusted author, ok-to-test label, and empty timeline would cover this gap.

• [architectural-coherence] .github/workflows/e2e.yml — The e2e workflow uses pull_request_target with a gate-based authorization model that differs from the shim workflow pattern documented in ADR 0009. The new e2e-testing.md guide explains the approach and cross-references ADR 0009, but a formal ADR would close the architectural documentation gap.

• [error-handling] scripts/check-e2e-authorization.sh:62 — The jq -s 'add' pattern after gh api --paginate produces null on empty input. Subsequent jq queries degrade gracefully (returning empty strings via // empty), but this is an implicit contract.

• [fail-open] scripts/check-e2e-authorization.sh:10 — The ERR trap sets authorized=false and exits 0 (fail-closed), but exit 0 means API failures or jq errors are swallowed rather than surfacing as step failures. The gate comment does post a reason=error message for visibility.

• [secret-exposure] .github/workflows/e2e.yml:64 — The e2e job checks out fork PR head code with access to secrets. The authorization gate logic is sound, but future modifications to the auth script could widen the trust boundary.

Info

• [naming-convention] scripts/check-e2e-authorization.sh — Creates a new top-level scripts/ directory. Existing scripts live under internal/scaffold/fullsend-repo/scripts/ (scaffold templates). The separation is correct — CI infrastructure vs. scaffold scripts.

• [fail-open] .github/workflows/e2e.yml:52 — Prior critical finding (gate bypass via non-ok-to-test labels) re-evaluated: when the gate job is skipped, needs.gate.outputs.authorized is empty (not 'true'), so the e2e job does NOT run. No bypass exists.

• [permission-reduction] .github/workflows/e2e.yml:3 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job — positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:44 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

Previous run (8)

Review

Findings

High

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR has no linked issue justifying changes to governance/infrastructure files. Human approval is required for all protected-path changes regardless of context, and the absence of a linked issue means the change lacks formal authorization.
  Remediation: Link an issue that authorizes these infrastructure changes.

Low

• [edge-case] scripts/check-e2e-authorization.sh:81 — The freshness check uses strict greater-than (>) on ISO 8601 timestamps, so a label applied at the exact same second as the last push is treated as stale. This is documented behavior ("strictly after") and fails closed, but could cause occasional maintainer confusion.

• [test-inadequate] scripts/check-e2e-authorization-test.sh:161 — The "commits api fallback when timeline has no push events" test case has no ok-to-test label, so it only exercises the "unauthorized" path. The interaction between the commits-API-derived last_push_at and the label freshness comparison is untested. Adding a test case with an untrusted author, ok-to-test label, and empty timeline would cover this gap.

• [architectural-coherence] .github/workflows/e2e.yml — The e2e workflow uses pull_request_target with a gate-based authorization model that differs from the shim workflow pattern documented in ADR 0009. The new e2e-testing.md guide explains the approach and cross-references ADR 0009, but a formal ADR would close the architectural documentation gap.

• [error-handling] scripts/check-e2e-authorization.sh:62 — The jq -s 'add' pattern after gh api --paginate produces null on empty input. Subsequent jq queries degrade gracefully (returning empty strings via // empty), but this is an implicit contract.

• [fail-open] scripts/check-e2e-authorization.sh:10 — The ERR trap sets authorized=false and exits 0 (fail-closed), but exit 0 means API failures or jq errors are swallowed rather than surfacing as step failures. The gate comment does post a reason=error message for visibility.

• [secret-exposure] .github/workflows/e2e.yml:64 — The e2e job checks out fork PR head code with access to secrets. The authorization gate logic is sound, but future modifications to the auth script could widen the trust boundary.

Info

• [naming-convention] scripts/check-e2e-authorization.sh — Creates a new top-level scripts/ directory. Existing scripts live under internal/scaffold/fullsend-repo/scripts/ (scaffold templates). The separation is correct — CI infrastructure vs. scaffold scripts.

• [fail-open] .github/workflows/e2e.yml:52 — Prior critical finding (gate bypass via non-ok-to-test labels) re-evaluated: when the gate job is skipped, needs.gate.outputs.authorized is empty (not 'true'), so the e2e job does NOT run. No bypass exists.

• [permission-reduction] .github/workflows/e2e.yml:3 — Top-level permissions changed from contents: read + id-token: write to permissions: {}. Permissions now scoped per-job — positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:44 — Gate job requests pull-requests: write, justified for PR comments and label removal. The e2e job correctly does NOT have this permission.

Previous run (9)

Review

Findings

Critical

• [fail-open] .github/workflows/e2e.yml:52 — Authorization gate bypass via non-ok-to-test label events. The gate job's if condition evaluates to false when github.event.action == 'labeled' and the label is not ok-to-test, causing the gate job to be skipped. The e2e job treats a skipped gate as authorized (needs.gate.result == 'skipped'), so adding ANY non-ok-to-test label to a fork PR (when the workflow is already triggered by path-matching changes) causes the e2e job to check out and execute fork code with full access to repository secrets (E2E_GITHUB_SESSION, GCP WIF credentials, etc.).
  Remediation: Change the e2e job's if condition to distinguish between gate-skipped-because-not-pull_request_target (legitimate) and gate-skipped-because-label-filtered (bypass). For example: if: !cancelled() && (github.event_name != 'pull_request_target' || needs.gate.outputs.authorized == 'true')

High

• [protected-path] .github/actions/check-e2e-authorization/action.yml, .github/workflows/e2e.yml, scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Four files in protected paths (.github/, scripts/) are modified or added. The PR has no linked issue justifying changes to governance/infrastructure files. Human approval is required for all protected-path changes.
  Remediation: Link an issue that authorizes these infrastructure changes, or provide justification in the PR description.

Medium

• [architectural-coherence] .github/workflows/e2e.yml — The e2e workflow uses pull_request_target and checks out PR code, which is the "pwn request" pattern ADR 0009 warns against for shim workflows. While ADR 0009 is scoped to shim workflows (not e2e), the e2e workflow uses a fundamentally different security model (authorization gate) that is not documented in any ADR. The new e2e-testing.md guide explains the approach, but a formal ADR would close the architectural documentation gap.

• [edge-case] scripts/check-e2e-authorization.sh:81 — The freshness check uses strict greater-than (>), meaning a label applied at the exact same second as the last push event is treated as stale. Consistent with the documented "after" semantics and fails closed (stale = denied), but could cause maintainer confusion in rare edge cases.

Low

• [missing-authorization] This PR has no linked issue. It introduces a non-trivial new authorization pattern for fork PR e2e runs (547 lines, 7 files). Non-trivial changes benefit from explicit authorization via a linked issue.

• [tier-mismatch] The PR title uses feat(e2e): but ci(e2e): may be more accurate since it modifies CI infrastructure for existing tests rather than adding new test capabilities.

• [missing-cross-reference] docs/ADRs/0009-pull-request-target-in-shim-workflows.md — ADR 0009 discusses pull_request_target usage for shim workflows. The e2e workflow now also uses pull_request_target with a different security model. A cross-reference could help future readers.

• [test-inadequate] scripts/check-e2e-authorization-test.sh — The test suite does not exercise the commits API fallback path (when timeline API returns no committed/force_pushed events). All test cases write timeline data containing committed events, so the branch at line 63-65 of the auth script is never exercised.

• [edge-case] scripts/check-e2e-authorization.sh:62 — The jq -s 'add' pattern after gh api --paginate could produce unexpected results if the API returns a non-array response. Low risk in practice.

• [naming-convention] scripts/check-e2e-authorization.sh — This PR creates a new top-level scripts/ directory. Existing scripts live under internal/scaffold/fullsend-repo/scripts/. The new location is justified (CI infrastructure vs scaffold scripts) but worth acknowledging as intentional.

Info

• [permission-reduction] .github/workflows/e2e.yml:4 — Top-level permissions changed from contents: read + id-token: write to permissions: {} (empty). Permissions now scoped per-job. Positive least-privilege change.

• [workflow-permission] .github/workflows/e2e.yml:44 — Gate job requests pull-requests: write. Justified for posting/updating PR comments and removing labels. The e2e job correctly does NOT have this permission.

• [architectural-documentation] The PR introduces a labeled-authorization pattern (ok-to-test + stale label removal + sticky comments) that could be reusable for other fork PR scenarios.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:14 AM UTC · Completed 7:29 AM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:18 AM UTC · Completed 8:32 AM UTC
Commit: 4ed6da4 · View workflow run →

[ralphbean]

LGTM. Two minor notes inline.

[waynesun09]

6-agent review squad — new findings (deduplicated against existing reviews)

Ran 6 parallel review agents (2x claude-coder, 2x claude-researcher, gemini-code-review, cursor-code-review). After deduplication against fullsend-ai-review bot (3 runs), @ralphbean's approval, and @ifireball's responses, 5 new findings remain that were not previously raised.

New findings

Severity	Finding	File
high	Timeline committed event lacks created_at — stale-label check uses spoofable timestamps by default, not just as fallback	check-e2e-authorization.sh:52
high	Test mocks use incorrect API schema, masking the above	check-e2e-authorization-test.sh
medium	Gate checkout should pin explicit base ref for defense-in-depth	e2e.yml:65
medium	Gate job missing timeout-minutes	e2e.yml:58
medium	Comment marker <!-- e2e-gate --> can be hijacked by PR participants	action.yml:93

Already covered by existing reviews (not re-raised)

• cancel-in-progress always false — @ralphbean, non-blocking
• jq -s 'add' null guard — fullsend-ai-review, author accepted as fail-closed
• !cancelled() + skipped gate — covered by critical bypass fix (0343838)
• Commits fallback test gap — fixed in 7ac778a

Key takeaway

The most significant new finding is H1: the GitHub Timeline API's committed event has no top-level created_at field (verified against API docs). This means for regular pushes, the timeline path returns nothing useful, and the script always falls back to attacker-controllable committer.date. The test mocks mask this by using an incorrect schema.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:40 PM UTC · Completed 9:57 PM UTC
Commit: a81599f · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:31 AM UTC · Completed 7:42 AM UTC
Commit: d7648e8 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2106 — feat(e2e): gate fork PR runs with pull_request_target

Timeline: PR opened 2026-06-10 06:54 UTC by ifireball, merged 2026-06-11 07:28 UTC (~24.5 hours). 13 commits, 9 review bot runs, 3 CHANGES_REQUESTED verdicts from the bot, 2 human approvals (ralphbean, waynesun09).

What went well:

• The review bot's first run caught a potential fail-open risk on non-ok-to-test label events, prompting a defensive improvement (commit 0343838).
• waynesun09's multi-agent review squad caught a HIGH-severity timeline API schema mismatch that the bot missed across 3+ runs — .committed events use .committer.date, not .created_at.
• The author was responsive, addressing findings promptly with clear commit-level attribution.
• Security-sensitive changes received thorough multi-layer review (bot + 2 human reviewers + 6-agent squad).

What could improve:

• 9 review bot runs is excessive. 3 ran on the same commit (4ed6da4). Covered by existing issues: #1452 (dedup same-SHA dispatches), #1014 (debounce rapid pushes), #981 (concurrency groups).
• Bot self-contradicted across runs — first run flagged a critical fail-open, second run on the same commit downgraded to info. Partially covered by #1319 (align approval with severity) and #2141 (validate recommendations).
• Bot missed the most important finding — the timeline API schema mismatch was a real security gap (attacker-controllable committer.date was the default code path). This was caught only by the human-led multi-agent review squad. One proposal filed below.
• Late CHANGES_REQUESTED after human approvals — the bot submitted CHANGES_REQUESTED at 07:00 UTC after both humans had approved. Partially covered by #2029 (COMMENT verdict for low-severity re-reviews).

Most improvement areas are well-covered by existing open issues. One new proposal filed for the API schema validation gap.

Proposals filed

• Review agent should validate external API schema assumptions in shell scripts (in fullsend-ai/fullsend)