fix(ci): treat renovate bot as trusted for e2e/functional test gates

[ralphbean]

Summary

• Adds TRUSTED_BOT_LOGINS list to check-e2e-authorization.sh with renovate-fullsend[bot] as the first entry
• The bot's author_association is CONTRIBUTOR, which meant its PRs were silently skipping e2e and functional tests unless someone manually added ok-to-test
• This is how the openshell 0.0.63→0.0.71 upgrade (chore(deps): update dependency nvidia/openshell to v0.0.71 #2714) merged without test coverage and caused sandbox creation failures downstream
• Adds test cases for both the trusted bot path and an unknown bot being denied

Companion to #2788 (adds .github/scripts/ to e2e path triggers).

Test plan

• bash scripts/check-e2e-authorization-test.sh — all passing

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

CI: allowlist Renovate bot for e2e/functional test authorization

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Allowlist Renovate PRs to pass e2e/functional test authorization without manual labels.
• Add a trusted-bot check ahead of collaborator-permission and ok-to-test label fallbacks.
• Extend authorization tests to cover trusted and untrusted bot behaviors.

Diagram

graph TD
  E["GitHub PR event"] --> S["check-e2e-authorization.sh"] --> D1{Trusted assoc?}
  D1 -- "yes" --> O1["authorized: trusted_author"]
  D1 -- "no" --> D2{Trusted bot?}
  D2 -- "yes" --> O2["authorized: trusted_bot"]
  D2 -- "no" --> API[("Collaborator API")] --> D3{Write+?}
  D3 -- "yes" --> O1
  D3 -- "no" --> L[("PR/Issue APIs")] --> D4{Fresh ok-to-test?}
  D4 -- "yes" --> O3["authorized: ok_to_test"]
  D4 -- "no" --> O4["unauthorized / stale"]
  subgraph Legend
    direction LR
    _svc["Script"] ~~~ _dec{"Decision"} ~~~ _api[("GitHub API")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Trust by GitHub App identity (app_id) instead of login string

• ➕ More robust than matching a mutable login string
• ➕ Can support multiple bot identities (Renovate, Dependabot, internal apps) without username drift
• ➖ Requires wiring app_id (or similar) from event payload and/or extra API calls
• ➖ More implementation complexity for a small immediate fix

2. Grant the bot write permissions so collaborator API authorizes it

• ➕ Avoids maintaining an allowlist in the script
• ➕ Uses existing permission-check path
• ➖ Broader permissions than necessary for dependency update PRs
• ➖ May not be feasible depending on org security policy and how the bot is configured

Recommendation: The allowlist approach is the best near-term fix: it prevents trusted Renovate PRs from silently skipping e2e/functional tests while keeping the authorization logic simple and testable. If additional bots need similar treatment or the allowlist grows, consider moving to an App-identity-based trust check to reduce brittleness.

Files changed (2) +30 / -3

Bug fix (1) +16 / -3

check-e2e-authorization.shAdd trusted-bot allowlist to e2e authorization gate +16/-3

Add trusted-bot allowlist to e2e authorization gate

• Introduces a TRUSTED_BOT_LOGINS allowlist and an is_trusted_bot helper. The main authorization logic now grants access to allowlisted bot PRs (e.g., renovate-fullsend[bot]) before falling back to collaborator-permission checks and ok-to-test label freshness.

scripts/check-e2e-authorization.sh

Tests (1) +14 / -0

check-e2e-authorization-test.shAdd regression tests for trusted and untrusted bots +14/-0

Add regression tests for trusted and untrusted bots

• Adds test cases asserting that renovate-fullsend[bot] is authorized even when author_association is CONTRIBUTOR/NONE, and that an unknown bot login is denied. Ensures the new trusted-bot path behaves deterministically without requiring collaborator permissions.

scripts/check-e2e-authorization-test.sh

[github-actions]

Site preview

Preview: https://9d77919c-site.fullsend-ai.workers.dev

Commit: 330a31b99703e32ca3a9b045df71aaa6d0daadc9

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 4:53 PM UTC · Ended 5:00 PM UTC
Commit: 104508d · View workflow run →

[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. Bot trust lacks login fallback 🐞 Bug ≡ Correctness

Description

The new trusted-bot authorization path only checks PR_AUTHOR_LOGIN, so callers that don’t set this
env var cannot recognize trusted bots even when PR data is fetched via the GitHub API. This can
incorrectly deny trusted bot PRs in ad-hoc/local invocations or future workflows that omit
PR_AUTHOR_LOGIN.

Code

scripts/check-e2e-authorization.sh[R102-104]

+elif is_trusted_bot "${PR_AUTHOR_LOGIN:-}"; then
+  authorized=true
+  reason="trusted_bot"

Relevance

⭐⭐⭐ High

Team frequently fixes auth misclassification/fallback issues in this script (accepted in #2158,
#2673, #2106).

PR-#2158
PR-#2673
PR-#2106

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR_AUTHOR_LOGIN is described as optional, but the trusted-bot check consults only PR_AUTHOR_LOGIN
and the code that fetches PR JSON does not extract .user.login, so the script cannot recognize
trusted bots unless the caller provides PR_AUTHOR_LOGIN.

scripts/check-e2e-authorization.sh[20-23]
scripts/check-e2e-authorization.sh[92-104]
scripts/check-e2e-authorization.sh[95-97]
.github/actions/check-e2e-authorization/action.yml[45-59]

Agent prompt

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

### Issue description
`is_trusted_bot` is only evaluated against `PR_AUTHOR_LOGIN`. When `PR_AUTHOR_LOGIN` is unset, the script never derives the author login from the PR API response (even when it already fetches `pr_json`), so trusted bots can be incorrectly treated as unauthorized depending on how the script is invoked.

### Issue Context
- The script documents `PR_AUTHOR_LOGIN` as optional.
- The composite action/workflows currently pass `PR_AUTHOR_LOGIN`, so CI isn’t broken today; this is a robustness/contract mismatch for other invocation paths.

### Fix Focus Areas
- scripts/check-e2e-authorization.sh[60-66]
- scripts/check-e2e-authorization.sh[89-113]

### Suggested implementation
- Introduce a local `author_login` variable.
- Populate it as:
 - `author_login="${PR_AUTHOR_LOGIN:-}"`
 - If empty and `pr_json` is available (or needs to be fetched for other reasons), set `author_login="$(jq -r '.user.login // empty' <<<"${pr_json}")"`.
- Use `author_login` for both `is_trusted_bot` and `has_write_permission`.
- Optional hardening: have `is_trusted_bot` return false when `login` is empty to avoid any future edge cases if the trusted list ever becomes empty/malformed.

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

---

[qodo-code-review]

1. Bot trust lacks login fallback 🐞 Bug ≡ Correctness

The new trusted-bot authorization path only checks PR_AUTHOR_LOGIN, so callers that don’t set this
env var cannot recognize trusted bots even when PR data is fetched via the GitHub API. This can
incorrectly deny trusted bot PRs in ad-hoc/local invocations or future workflows that omit
PR_AUTHOR_LOGIN.

Agent Prompt

### Issue description
`is_trusted_bot` is only evaluated against `PR_AUTHOR_LOGIN`. When `PR_AUTHOR_LOGIN` is unset, the script never derives the author login from the PR API response (even when it already fetches `pr_json`), so trusted bots can be incorrectly treated as unauthorized depending on how the script is invoked.

### Issue Context
- The script documents `PR_AUTHOR_LOGIN` as optional.
- The composite action/workflows currently pass `PR_AUTHOR_LOGIN`, so CI isn’t broken today; this is a robustness/contract mismatch for other invocation paths.

### Fix Focus Areas
- scripts/check-e2e-authorization.sh[60-66]
- scripts/check-e2e-authorization.sh[89-113]

### Suggested implementation
- Introduce a local `author_login` variable.
- Populate it as:
  - `author_login="${PR_AUTHOR_LOGIN:-}"`
  - If empty and `pr_json` is available (or needs to be fetched for other reasons), set `author_login="$(jq -r '.user.login // empty' <<<"${pr_json}")"`.
- Use `author_login` for both `is_trusted_bot` and `has_write_permission`.
- Optional hardening: have `is_trusted_bot` return false when `login` is empty to avoid any future edge cases if the trusted list ever becomes empty/malformed.

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 4:53 PM UTC · Completed 5:00 PM UTC
Commit: dfc0f47 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 5:19 PM UTC · Completed 5:26 PM UTC
Commit: b13f51b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:40 PM UTC · Completed 7:50 PM UTC
Commit: 330a31b · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] scripts/check-e2e-authorization.sh, scripts/check-e2e-authorization-test.sh — Both modified files are under scripts/, a protected path requiring human approval. The PR provides clear context: renovate-fullsend[bot]'s PRs were silently skipping e2e tests due to its CONTRIBUTOR author_association, which allowed the openshell 0.0.63→0.0.71 upgrade (chore(deps): update dependency nvidia/openshell to v0.0.71 #2714) to merge without test coverage and cause downstream sandbox creation failures. Human approval is required for all protected-path changes regardless of context.

• [missing-authorization] scripts/check-e2e-authorization.sh — This PR introduces a new authorization path (trusted bot logins) for security-critical test gates without a formal linked issue. The PR body references chore(deps): update dependency nvidia/openshell to v0.0.71 #2714 (root cause) and fix(ci): trigger e2e tests on .github/scripts/ and action.yml changes #2788 (companion), but neither constitutes a linked issue authorizing this specific change. Consider filing a tracking issue documenting the problem, chosen solution (hardcoded allowlist), and alternatives considered.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:10 AM UTC · Completed 8:19 AM UTC
Commit: 330a31b · View workflow run →

[fullsend-ai-retro]

Retro: PR #2789 — fix(ci): treat renovate bot as trusted for e2e/functional test gates

Timeline

1. 16:50 UTC Jun 30 — Human (ralphbean) opens PR with a single commit adding TRUSTED_BOT_LOGINS to check-e2e-authorization.sh and corresponding tests.
2. 16:50–17:00 — Review agent run 1 (run 28461220601) fails: openshell sandbox container crashes on startup due to network namespace deletion failure (EINVAL).
3. 17:16–17:26 — Review agent run 2 (run 28462769599) fails with identical sandbox crash. Same openshell version (0.0.63) as the eventual successful run — failure was runner-specific, not version-specific.
4. 19:38–19:50 — Review agent run 3 (run 28470956675) succeeds. Posts 2 medium findings: [protected-path] (human approval required for scripts/ changes) and [missing-authorization] (no formal linked issue).
5. 07:57 UTC Jul 1 — Human (rh-hemartin) approves.
6. 08:06 UTC Jul 1 — PR merged.

Assessment

Review quality: Good. The review agent's two findings were appropriate — protected-path correctly flagged security-sensitive CI scripts, and missing-authorization reasonably noted the lack of a formal linked issue. Qodo's finding about a missing login fallback was arguably a false positive (the script always runs in CI where PR_AUTHOR_LOGIN is set, and ${PR_AUTHOR_LOGIN:-} safely handles the unset case).

Infrastructure reliability: Poor. Two out of three review runs failed due to transient openshell sandbox crashes (network namespace EINVAL errors), adding ~3 hours of delay. Each retry required a manual force-push to re-trigger the review dispatch.

Rework rate: Zero. Single commit, no fix agent involvement, no code changes requested by review.

Existing issue coverage

All identified improvement opportunities are already tracked by open issues:

• Auto-retry after sandbox failures: #2711 — Auto re-dispatch review after consecutive infrastructure failures with backoff. Would have eliminated the need for manual force-pushes to retry.
• Stale-head race handling: #1331, #1043 — The first run's status comment updated its commit SHA mid-run when a force-push occurred.
• Protected-path noise reduction: #2794, #1392, #1551, #2588 — Extensive coverage of protected-path finding calibration.

No new proposals filed — existing issues adequately cover the improvement opportunities identified in this workflow.