ci(functional-tests): use pull_request_target for fork PR support

[ralphbean]

Summary

• Switch functional tests from pull_request to pull_request_target with a gate job so fork PRs receive secrets (same pattern as e2e.yml)
• Add change-relevance filtering so PRs without eval/ or internal/scaffold/ changes skip the tests
• Add a secrets-check step to gracefully skip GCP auth instead of hard-failing when secrets are unavailable
• Pin all actions to full-length commit SHAs, set persist-credentials: false on untrusted checkout

Fixes the failure seen in https://github.com/fullsend-ai/fullsend/actions/runs/27982063986/job/82816098640?pr=2346 where fork PRs hit google-github-actions/auth with empty secrets.

Test plan

• Verify the workflow triggers correctly on a fork PR (gate → authorize → skip or run)
• Verify push-to-main still triggers with path filtering
• Verify PRs that don't touch eval/ or internal/scaffold/ skip gracefully

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

CI: Run functional tests via pull_request_target with fork-safe gating
⚙️ Configuration changes ✨ Enhancement 🕐 20-40 Minutes

Description

• Switch functional tests to pull_request_target with an authorization gate for fork PRs.
• Skip runs when PR changes don’t touch eval/ or internal/scaffold/ (plus workflow file).
• Gracefully skip GCP-auth-dependent steps when secrets are unavailable; pin actions by SHA.

Diagram

graph TD
  A["Workflow triggers"] --> B["Gate job (base checkout)"] --> C{"Authorized?"} --> D["Functional-tests job (PR head checkout)"] --> E{"Relevant changes?"} --> F{"Secrets available?"} --> G["GCP auth + tests + artifacts"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Trigger a separate workflow via workflow_run/workflow_dispatch after gate

• ➕ Harder separation boundary between authorization and secret-using execution
• ➕ Can keep PR_target workflow minimal and reduce blast radius
• ➖ More plumbing (event passing, correlation, concurrency) and slower feedback
• ➖ More complex to debug across multiple workflows

2. Use a dedicated path-filter action (e.g., dorny/paths-filter) instead of gh api

• ➕ Less bespoke shell/grep logic; clearer intent
• ➕ Can centralize path patterns and outputs
• ➖ Still needs careful handling under pull_request_target to avoid unsafe checkouts
• ➖ May require additional checkout/diff setup depending on approach

3. Run fork PR functional tests without secrets (mock/stub GCP) and keep pull_request

• ➕ Avoids PR_target security considerations entirely
• ➕ Simpler permission model
• ➖ Reduced coverage/realism; does not validate GCP/WIF integration
• ➖ May miss failures only visible with real credentials

Recommendation: The chosen pull_request_target + separate authorization gate is the right primary approach for enabling fork PR coverage while controlling secret access (and it aligns with the existing e2e.yml pattern). The additional relevance filtering and explicit secrets-check are pragmatic safeguards that reduce noise and prevent hard failures when secrets are absent.

Files changed (1) +121 / -24

Other (1) +121 / -24

functional-tests.ymlMake functional tests fork-safe with PR_target gating and skip logic +121/-24

Make functional tests fork-safe with PR_target gating and skip logic

• Switches PR execution to pull_request_target and adds a separate gate job that performs authorization on the base checkout only. Adds PR-file relevance filtering (via GitHub API) and a secrets availability check so GCP auth/tests are skipped when secrets are missing. Also pins actions to full commit SHAs, disables persisted credentials for untrusted checkouts, and refines concurrency behavior for PR vs main runs.

.github/workflows/functional-tests.yml

[github-actions]

Site preview

Preview: https://41d0a016-site.fullsend-ai.workers.dev

Commit: 53ab724aa87ab11003eb7f83cea76b53683854fb

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Fork checkout blocked ✓ Resolved 🐞 Bug ≡ Correctness

Description

In functional-tests, actions/checkout@v7 is asked to checkout github.event.pull_request.head.sha on
pull_request_target without allow-unsafe-pr-checkout, which (as documented in your existing e2e
workflow) blocks fork PR head checkouts and will break fork PR functional tests.

Code

.github/workflows/functional-tests.yml[R96-101]

+      - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+        if: steps.changes.outputs.relevant != 'false'
        with:
+          ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || github.sha }}
+          persist-credentials: false
          submodules: true

Relevance

⭐⭐⭐ High

Team fixed same checkout@v7 PR-head-on-pull_request_target block by opting into
allow-unsafe-pr-checkout in e2e.

PR-#2503

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

functional-tests checks out the PR head SHA on pull_request_target but lacks the
allow-unsafe-pr-checkout opt-in; e2e.yml shows that checkout@v7 requires this for fork PR head
checkouts on pull_request_target.

.github/workflows/functional-tests.yml[96-101]
.github/workflows/e2e.yml[105-113]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`actions/checkout@v7` blocks checking out a fork PR head on `pull_request_target` unless `allow-unsafe-pr-checkout` is explicitly enabled. The functional-tests workflow checks out `github.event.pull_request.head.sha` but does not set this flag, so fork PR runs are likely to fail at checkout (or not actually test PR code).

### Issue Context
The repo’s `e2e.yml` already implements the required opt-in and explains why it is needed.

### Fix Focus Areas
- .github/workflows/functional-tests.yml[96-101]

### Suggested change
Add the same opt-in used by `e2e.yml`:
- `allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }}`

Optionally add the same explanatory comment to avoid future regressions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Secrets check incomplete 🐞 Bug ☼ Reliability

Description

The new “Check for secrets” step only validates E2E_GCP_WIF_PROVIDER, but the subsequent GCP auth
step also requires E2E_GCP_SERVICE_ACCOUNT, so some missing/partial secret configurations will still
hard-fail instead of being skipped.

Code

.github/workflows/functional-tests.yml[R225-244]

+      - name: Check for secrets
+        if: steps.changes.outputs.relevant != 'false'
+        id: secrets-check
+        run: |
+          if [ -z "$WIF_PROVIDER" ]; then
+            echo "::warning::GCP secrets are not configured. Skipping functional tests."
+            echo "available=false" >> "$GITHUB_OUTPUT"
+          else
+            echo "available=true" >> "$GITHUB_OUTPUT"
+          fi
+        env:
+          WIF_PROVIDER: ${{ secrets.E2E_GCP_WIF_PROVIDER }}
+
      - name: Authenticate to GCP
-        uses: google-github-actions/auth@v2
+        if: steps.changes.outputs.relevant != 'false' && steps.secrets-check.outputs.available == 'true'
+        uses: google-github-actions/auth@7c6bc770dae815cd3e89ee6cdf493a5fab2cc093 # v3.0.0
        with:
          workload_identity_provider: ${{ secrets.E2E_GCP_WIF_PROVIDER }}
          service_account: ${{ secrets.E2E_GCP_SERVICE_ACCOUNT }}

Relevance

⭐⭐⭐ High

Repo previously added secrets-check gates to skip workflows when required secrets missing; likely
expects checking all required secrets.

PR-#221

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The gate only checks WIF_PROVIDER, but the auth step immediately consumes both WIF_PROVIDER and
SERVICE_ACCOUNT from secrets, so the guard does not actually prevent auth failures when
SERVICE_ACCOUNT is missing.

.github/workflows/functional-tests.yml[225-244]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow intends to gracefully skip when secrets are unavailable, but the gate only checks `E2E_GCP_WIF_PROVIDER`. If `E2E_GCP_WIF_PROVIDER` is set while `E2E_GCP_SERVICE_ACCOUNT` (or other required secrets) is missing, the workflow will proceed and then fail in `google-github-actions/auth`.

### Issue Context
The auth step consumes both `workload_identity_provider` and `service_account` from secrets.

### Fix Focus Areas
- .github/workflows/functional-tests.yml[225-244]

### Suggested change
Update the secrets-check step to verify *all* secrets required for downstream gated steps, at minimum:
- `E2E_GCP_WIF_PROVIDER`
- `E2E_GCP_SERVICE_ACCOUNT`
(Optionally also check `E2E_GCP_PROJECT_ID` / `EVAL_GH_TOKEN` if those are required for `make functional-tests`.)

Set `available=true` only when all required values are non-empty; otherwise emit the warning and set `available=false`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:08 PM UTC · Ended 9:18 PM UTC
Commit: 4e21a60 · View workflow run →

[ralphbean]

I'm dealing with this over at #2535

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:21 PM UTC · Completed 9:33 PM UTC
Commit: 53106e3 · View workflow run →

[fullsend-ai-review]

Review

Findings

High

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The PR has no linked issue providing justification for modifying governance/infrastructure files. Protected-path changes always require human review regardless of context.
  Remediation: File an issue documenting the fork PR secret access problem (reference the failing Actions run in the PR body), link it from the PR, and obtain human maintainer approval.

Medium

• [naming-abstraction-misalignment] .github/workflows/functional-tests.yml:52 — The workflow reuses .github/actions/check-e2e-authorization for functional test authorization, but the action's name, description, and default docs_url are all e2e-specific. This creates a naming mismatch and will direct users to the wrong documentation (e2e-testing.md instead of functional testing docs) when authorization fails.
  Remediation: Either: (1) rename the action to something generic like check-pr-authorization and update its description and docs_url parameter, or (2) pass a custom docs_url parameter pointing to functional test documentation when invoking the action from this workflow.

Low

• [missing-authorization] .github/workflows/functional-tests.yml — Non-trivial change (123 additions, 20 deletions) lacks a linked issue. The PR body describes clear motivation (fork PRs failing on google-github-actions/auth with empty secrets) and the implementation follows the established pull_request_target pattern from e2e.yml. A missing issue link is a process gap, not a code defect.

• [edge-case] .github/workflows/functional-tests.yml — The change-relevance grep regex and the push paths filter are declared as a SYNC-WITH pair, but the sync is manual and comment-driven. If a future contributor adds a path to one but not the other, pull_request_target events will silently skip tests for files that push events would test. This is a pre-existing design limitation inherited from e2e.yml.

• [secret-exposure] .github/workflows/functional-tests.yml — The functional-tests job exposes EVAL_GH_TOKEN and GCP WIF credentials to fork PR code after gate authorization. The gate mitigates unauthorized access, but the blast radius of a compromised ok-to-test label includes these secrets. This is inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile.

• [action-version-pinning] .github/workflows/functional-tests.yml — Actions are SHA-pinned with version comments (e.g., actions/checkout@9c091bb...# v7.0.0), which is a security best practice. However, the reference workflow (e2e.yml) uses semver tags (@v7). The inconsistency is minor — SHA pinning is strictly more secure.

Previous run

Review

Findings

High

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The PR has no linked issue providing justification for modifying governance/infrastructure files. Protected-path changes always require human review regardless of context.
  Remediation: File an issue documenting the fork PR secret access problem (reference the failing Actions run in the PR body), link it from the PR, and obtain human maintainer approval.

Low

• [missing-authorization] .github/workflows/functional-tests.yml — Non-trivial change (125 additions, 24 deletions) lacks a linked issue. The PR body describes the motivation (fork PRs failing on google-github-actions/auth with empty secrets) and the implementation follows the established pull_request_target pattern from ADR 0009 and e2e.yml. A missing issue link is a process gap, not a code defect.

• [edge case] .github/workflows/functional-tests.yml — The change-relevance grep regex and the push paths filter are declared as a SYNC-WITH pair, but the sync is manual and comment-driven. If a future contributor adds a path to one but not the other, pull_request_target events will silently skip tests for files that push events would test. This is a pre-existing design limitation inherited from e2e.yml.

• [secrets exposure] .github/workflows/functional-tests.yml — The functional-tests job exposes EVAL_GH_TOKEN and GCP WIF credentials to fork PR code after gate authorization. The gate mitigates unauthorized access, but the blast radius of a compromised ok-to-test label includes these secrets. This is inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile.

Previous run

Review

Findings

High

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The PR has no linked issue providing justification for modifying governance/infrastructure files. Protected-path changes always require human review regardless of context.
  Remediation: File an issue documenting the fork PR secret access problem (reference the failing Actions run in the PR body), link it from the PR, and obtain human maintainer approval.

Low

• [missing-authorization] .github/workflows/functional-tests.yml — Non-trivial change (125 additions, 24 deletions) lacks a linked issue. The PR body describes the motivation (fork PRs failing on google-github-actions/auth with empty secrets) and the implementation follows the established pull_request_target pattern from ADR 0009 and e2e.yml. A missing issue link is a process gap, not a code defect.

• [edge case] .github/workflows/functional-tests.yml — The change-relevance grep regex and the push paths filter are declared as a SYNC-WITH pair, but the sync is manual and comment-driven. If a future contributor adds a path to one but not the other, pull_request_target events will silently skip tests for files that push events would test. This is a pre-existing design limitation inherited from e2e.yml.

• [secrets exposure] .github/workflows/functional-tests.yml — The functional-tests job exposes EVAL_GH_TOKEN and GCP WIF credentials to fork PR code after gate authorization. The gate mitigates unauthorized access, but the blast radius of a compromised ok-to-test label includes these secrets. This is inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile.

---

Labels: PR modifies CI workflow infrastructure (.github/workflows/functional-tests.yml) to add pull_request_target fork support.

Previous run

Review

Findings

High

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The PR has no linked issue providing justification for modifying governance/infrastructure files. Protected-path changes always require human review regardless of context.
  Remediation: File an issue documenting the fork PR secret access problem (reference the failing Actions run in the PR body), link it from the PR, and obtain human maintainer approval.

Low

• [missing-authorization] .github/workflows/functional-tests.yml — Non-trivial change (125 additions, 24 deletions) lacks a linked issue. The PR body describes clear motivation (fork PRs failing on google-github-actions/auth with empty secrets) and the implementation follows the established pull_request_target pattern from ADR 0009 and e2e.yml. A missing issue link is a process gap, not a code defect.

• [edge case] .github/workflows/functional-tests.yml — The change-relevance grep regex and the push paths filter are declared as a SYNC-WITH pair, but the sync is manual and comment-driven. If a future contributor adds a path to one but not the other, pull_request_target events will silently skip tests for files that push events would test. This is a pre-existing design limitation inherited from e2e.yml.

• [secrets exposure] .github/workflows/functional-tests.yml — The functional-tests job exposes EVAL_GH_TOKEN and GCP WIF credentials to fork PR code after gate authorization. The gate mitigates unauthorized access, but the blast radius of a compromised ok-to-test label includes these secrets. This is inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile.

Previous run (2)

Review

Findings

High

• [protected-path] .github/workflows/functional-tests.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The PR has no linked issue providing justification for modifying governance/infrastructure files. Protected-path changes always require human review regardless of context.
  Remediation: File an issue documenting the fork PR secret access problem (reference the failing Actions run in the PR body), link it from the PR, and obtain human maintainer approval.

Low

• [missing-authorization] .github/workflows/functional-tests.yml — Non-trivial change (125 additions, 24 deletions) lacks a linked issue. The PR body describes the motivation (fork PRs failing on google-github-actions/auth with empty secrets) and the implementation follows the established pull_request_target pattern from ADR 0009 and e2e.yml. A missing issue link is a process gap, not a code defect.

• [edge case] .github/workflows/functional-tests.yml — The change-relevance grep regex and the push paths filter are declared as a SYNC-WITH pair, but the sync is manual and comment-driven. If a future contributor adds a path to one but not the other, pull_request_target events will silently skip tests for files that push events would test. This is a pre-existing design limitation inherited from e2e.yml.

• [secrets exposure] .github/workflows/functional-tests.yml — The functional-tests job exposes EVAL_GH_TOKEN and GCP WIF credentials to fork PR code after gate authorization. The gate mitigates unauthorized access, but the blast radius of a compromised ok-to-test label includes these secrets. This is inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile.

---

Labels: PR modifies CI workflow infrastructure (.github/workflows/functional-tests.yml) to add pull_request_target fork support.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[low] secrets exposure

The functional-tests job exposes EVAL_GH_TOKEN and GCP WIF credentials to fork PR code after gate authorization. The gate mitigates unauthorized access, but the blast radius of a compromised ok-to-test label includes these secrets. Inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:06 PM UTC · Completed 11:17 PM UTC
Commit: 470372d · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:02 PM UTC · Completed 2:14 PM UTC
Commit: 53ab724 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] naming-abstraction-misalignment

The workflow reuses .github/actions/check-e2e-authorization for functional test authorization, but the action's name, description, and default docs_url are all e2e-specific. This creates a naming mismatch and will direct users to the wrong documentation (e2e-testing.md instead of functional testing docs) when authorization fails.

Suggested fix: Either: (1) rename the action to something generic like check-pr-authorization and update its description and docs_url parameter, or (2) pass a custom docs_url parameter pointing to functional test documentation when invoking the action from this workflow.

[fullsend-ai-review]

[low] secret-exposure

The functional-tests job exposes EVAL_GH_TOKEN and GCP WIF credentials to fork PR code after gate authorization. The gate mitigates unauthorized access, but the blast radius of a compromised ok-to-test label includes these secrets. This is inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile.

[fullsend-ai-review]

[low] action-version-pinning

Actions are SHA-pinned with version comments (e.g., actions/checkout@9c091bb...# v7.0.0), which is a security best practice. However, the reference workflow (e2e.yml) uses semver tags (@v7). The inconsistency is minor — SHA pinning is strictly more secure.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:23 PM UTC · Completed 2:32 PM UTC
Commit: 53ab724 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2534 — ci(functional-tests): use pull_request_target for fork PR support

Timeline

This was a human-authored PR by ralphbean that modified .github/workflows/functional-tests.yml to switch from pull_request to pull_request_target for fork PR secret access.

1. Jun 22 21:04 — PR opened with initial commit
2. Jun 22 21:07 — Qodo bot found a real bug: missing allow-unsafe-pr-checkout for checkout@v7 on pull_request_target
3. Jun 22 21:08 — First fullsend review started, later cancelled (superseded by new commit)
4. Jun 22 21:17 — Second commit addressing Qodo feedback + adding .github/scripts/ to paths filter
5. Jun 22 21:22–21:33 — Second fullsend review → CHANGES_REQUESTED (secrets exposure [low])
6. Jun 22 23:06–23:17 — Third fullsend review on same commit 470372d → CHANGES_REQUESTED again
7. Jun 23 09:21 — Human approval from rh-hemartin
8. Jun 23 13:58 — Merge commit from main (no new PR changes)
9. Jun 23 14:02–14:14 — Fourth fullsend review on merge commit → CHANGES_REQUESTED (naming-abstraction-misalignment [medium], secrets [low], pinning [low])
10. Jun 23 14:19 — PR merged with human approval overriding bot's CHANGES_REQUESTED

Observations

4 review runs (3 completed) for a 1-file CI config change. The review agent requested changes 3 times, all overridden by the human. Key friction points:

1. Duplicate review on same SHA — Run docs: Add codebase context problem document and trim CLAUDE.md #3 reviewed commit 470372d which run Add problem areas: Tekton pipeline review, migration path, multi-tenancy #2 had already reviewed. Covered by #1452.

2. Review triggered on merge-from-main commit — Run Use AI to help formalise intent after rapid local prototyping #4 reviewed merge commit 53ab724 which contained no new PR-authored changes. Covered by #1282.

3. CHANGES_REQUESTED for low/medium findings on a human PR — The verdict logic says low-only → approve and one-medium → comment, yet the bot issued CHANGES_REQUESTED three times. Covered by #2115 and #2029.

4. Self-contradicting findings — The secrets-exposure finding explicitly acknowledges "inherent to the pull_request_target pattern and matches the accepted e2e.yml risk profile" yet still flags it. Covered by #1881.

5. Duplicate inline comments across re-reviews — The secrets-exposure comment was posted twice on the same line. Covered by #1285 and #1013.

Assessment

All issues observed on this PR are already tracked in the existing issue backlog. The most impactful improvements would be resolving #1452 (deduplicate same-SHA reviews), #1282 (skip merge-only re-reviews), and #2115 (COMMENT verdict for human PRs with medium/low findings). Together these would have reduced this PR from 4 review runs to 1, saving significant tokens and eliminating the noise of 3 false CHANGES_REQUESTED verdicts.

No new proposals filed — all findings are covered by existing issues.