ci(e2e): split ok-to-test label trigger into caller workflows

[ifireball]

Summary

• Move labeled / ok-to-test handling out of the main E2E and Functional Tests workflows into thin caller workflows that workflow_call the suites
• Non-ok-to-test labels (e.g. ready-for-review) no longer create skipped checks under E2E Tests / Functional Tests
• Label events never cancel an in-progress suite (authorization only, not a retest)

Follow-up to #5457 / #5455 residual skip noise (seen on #5499). Related: #5479 (label-filter sync mostly superseded).

Changes

• .github/workflows/e2e.yml / functional-tests.yml — add workflow_call, drop labeled from triggers, simplify gate if:, cancel-in-progress: false when action == labeled
• .github/workflows/e2e-ok-to-test.yml / functional-ok-to-test.yml — thin labeled callers (if: label == ok-to-test)
• docs/guides/dev/e2e-testing.md — document the split trigger model

Test plan

• make lint (pre-commit / actionlint) on staged files
• Bot PR + ready-for-review: E2E Tests runs from open; E2E ok-to-test shows skipped run; opened run not cancelled
• Apply ok-to-test while e2e is running: opened run keeps going (not cancelled)
• Fork with no prior run + ok-to-test: suite starts via the thin caller
• Confirm ruleset still accepts nested run / e2e check name on the fork/ok-to-test path (update ruleset only if needed)

Made with Cursor

[qodo-code-review]

PR Summary by Qodo

CI: split ok-to-test label handling into caller workflows for E2E/Functional

⚙️ Configuration changes 📝 Documentation 🕐 20-40 Minutes

AI Description

• Add thin ok-to-test caller workflows that invoke suites via workflow_call.
• Remove labeled triggers from main suites to avoid skipped checks for unrelated labels.
• Ensure label events never cancel in-progress suite runs; only PR code changes do.

Diagram

graph TD
  A(("PR opened/sync/reopened")) --> S["Main suites (reusable)"] --> G["Gate job"] --> T["Test jobs"]
  L(("Label event")) --> C["ok-to-test callers"] --> S

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep labeled trigger in main suites with gate/job-level filtering

• ➕ Fewer workflow files and less indirection
• ➕ All suite entrypoints remain in a single workflow definition
• ➖ GitHub still creates a check/run for every labeled event, producing skipped-check noise under the main suite names
• ➖ Harder to ensure label events don’t participate in suite concurrency/cancellation without more complex conditions

2. Use workflow_run chaining from a labeled-only workflow

• ➕ Main suites could remain untouched and be invoked only after an explicit labeled workflow completes
• ➕ Potentially clearer separation of authorization vs execution
• ➖ More moving parts and additional run state to reason about
• ➖ Passing context/secrets and preserving the same gate semantics can be trickier than workflow_call

Recommendation: The chosen workflow_call split is the most direct way to stop non-ok-to-test label events from creating skipped checks under the main suite workflow names, while keeping the same gate/suite logic. It also cleanly decouples label authorization events from suite concurrency/cancellation behavior.

Files changed (5) +83 / -43

Documentation (1) +21 / -16

e2e-testing.mdDocument split trigger model for ok-to-test authorization +21/-16

Document split trigger model for ok-to-test authorization

• Updates documentation to reflect that ok-to-test label events now run via dedicated caller workflows that invoke the main suites via workflow_call. Clarifies cancellation behavior: only PR open/sync/reopen events cancel; label events are authorization-only and do not cancel running suites.

docs/guides/dev/e2e-testing.md

Other (4) +62 / -27

e2e-ok-to-test.ymlAdd labeled-only E2E ok-to-test caller workflow +20/-0

Add labeled-only E2E ok-to-test caller workflow

• Introduces a thin pull_request_target(labeled) workflow that filters for the ok-to-test label and invokes the main E2E workflow via workflow_call. Avoids sharing the per-PR concurrency group with the reusable workflow to prevent deadlocks.

.github/workflows/e2e-ok-to-test.yml

e2e.ymlMake E2E workflow reusable and drop labeled trigger/cancel behavior +12/-14

Make E2E workflow reusable and drop labeled trigger/cancel behavior

• Adds workflow_call support and removes labeled from pull_request_target triggers, delegating ok-to-test label handling to the new caller workflow. Simplifies the gate job condition and updates concurrency cancellation so label events never cancel an in-progress run; also updates relevant-file filters to include the new workflow.

.github/workflows/e2e.yml

functional-ok-to-test.ymlAdd labeled-only Functional ok-to-test caller workflow +20/-0

Add labeled-only Functional ok-to-test caller workflow

• Introduces a thin pull_request_target(labeled) workflow that runs only for ok-to-test and calls the Functional Tests workflow via workflow_call. Keeps concurrency management inside the reusable workflow to avoid cross-workflow deadlocks.

.github/workflows/functional-ok-to-test.yml

functional-tests.ymlMake Functional Tests reusable and remove labeled trigger from suite +10/-13

Make Functional Tests reusable and remove labeled trigger from suite

• Adds workflow_call support and removes labeled from pull_request_target triggers, delegating ok-to-test label events to the new caller workflow. Updates concurrency cancellation and simplifies the gate job condition; expands relevant-file filtering to include the new workflow.

.github/workflows/functional-tests.yml

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:02 AM UTC · Completed 6:14 AM UTC
Commit: cd71e57 · View workflow run →

[github-actions]

Site preview

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

Commit: 15ac2929bc86575887335c6f88b042d45b56fd53

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 54 rules

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

1. E2E caller no permissions ✓ Resolved 🐞 Bug ☼ Reliability

Description

Both .github/workflows/e2e-ok-to-test.yml and .github/workflows/functional-ok-to-test.yml set
permissions: {} and do not grant any job-level permissions, preventing their called reusable
workflows (e2e.yml and functional-tests.yml) from obtaining required GITHUB_TOKEN scopes
(e.g., pull-requests: write, contents: read, id-token: write). As a result, ok-to-test runs
can fail during authorization/commenting and gh api/GitHub API operations that rely on those
permissions.

Code

.github/workflows/e2e-ok-to-test.yml[R10-20]

+permissions: {}
+
+on:
+  pull_request_target:
+    types: [labeled]
+
+jobs:
+  run:
+    if: github.event.label.name == 'ok-to-test'
+    uses: ./.github/workflows/e2e.yml
+    secrets: inherit

Relevance

⭐⭐⭐ High

Likely real CI break: caller permissions {} blocks reusable workflow token scopes; team often fixes
workflow auth/permission issues.

PR-#3903
PR-#1215
PR-#2617

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In each caller workflow the token permissions are explicitly zeroed via permissions: {}, and
reusable workflows cannot request broader permissions than the caller grants. The called workflows
then use github.token with gh api for gate/authorization and PR file change detection, and they
post/update PR (sticky) comments—operations that require PR read/write and repository read scopes—so
with an empty permission set these steps will error; because the composite action is configured with
strict shell settings (set -euo pipefail), such failures will stop the jobs.

.github/workflows/e2e-ok-to-test.yml[10-20]
.github/workflows/e2e.yml[66-93]
.github/workflows/e2e.yml[108-116]
.github/actions/check-e2e-authorization/action.yml[61-121]
.github/workflows/functional-ok-to-test.yml[10-20]
.github/workflows/functional-tests.yml[39-68]
.github/workflows/functional-tests.yml[82-92]

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 ok-to-test caller workflows (`e2e-ok-to-test.yml` and `functional-ok-to-test.yml`) set `permissions: {}` which removes all `GITHUB_TOKEN` permissions. When these workflows `uses` their respective reusable workflows (`e2e.yml` and `functional-tests.yml`), the called workflows cannot elevate token permissions beyond what the caller granted, so steps that rely on `gh api` and PR comment updates (and other GitHub API operations) can fail.

## Issue Context
The called reusable workflows include gate/authorization behavior and change-detection that use `github.token`/`gh api`, and they read/update PR state via PR comments (e.g., sticky comment). The caller workflows must grant at least the union of permissions required by the called workflow jobs; otherwise authorization, commenting, and API calls will fail and terminate the job.

## Fix Focus Areas
- .github/workflows/e2e-ok-to-test.yml[10-20]
- .github/workflows/functional-ok-to-test.yml[10-20]
- .github/workflows/e2e.yml[66-116]
- .github/workflows/functional-tests.yml[39-92]
- .github/actions/check-e2e-authorization/action.yml[61-121]

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

---

2. Guide under docs/guides/dev 📜 Skill insight ⌂ Architecture

Description

docs/guides/dev/e2e-testing.md is a guide file located under docs/guides/dev/, but the guide
directory policy requires guide files to live under either docs/guides/admin/ or
docs/guides/user/. This makes the guide non-compliant with the required guide placement
convention.

Code

docs/guides/dev/e2e-testing.md[R165-173]

+Applying the label triggers the **E2E ok-to-test** / **Functional ok-to-test**
+caller workflows, which `workflow_call` into the main suites.
+
+The main **E2E Tests** and **Functional Tests** workflows do **not** subscribe to
+`labeled` events. Only `opened` / `synchronize` / `reopened` cancel in-progress
+work in the per-PR concurrency group (code changed). Label events are
+authorization only: they **never** cancel an in-progress suite. If a suite is
+already running when `ok-to-test` is applied, GitHub may queue a second run
+behind it (no expression-only “skip if busy”); that is accepted.

Relevance

⭐ Low

Exact precedent: team previously rejected moving docs/guides/dev/e2e-testing.md to admin/user
taxonomy.

PR-#5457
PR-#5454
PR-#4901

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The guide placement rule requires guides under docs/guides/ to live in either admin/ or user/.
The PR modifies docs/guides/dev/e2e-testing.md, which is located in the disallowed dev/
subdirectory (as shown by the file path and the guide index entry).

docs/guides/dev/e2e-testing.md[1-3]
docs/guides/README.md[44-52]
Skill: writing-user-docs

Agent prompt

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

## Issue description
A modified guide file (`docs/guides/dev/e2e-testing.md`) is stored under `docs/guides/dev/`, but guides must be placed under either `docs/guides/admin/` or `docs/guides/user/`.

## Issue Context
This PR edits `docs/guides/dev/e2e-testing.md`, so the file must comply with the guide placement policy.

## Fix Focus Areas
- docs/guides/dev/e2e-testing.md[1-3]
- docs/guides/dev/e2e-testing.md[165-199]
- docs/guides/README.md[44-52]

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

---

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/workflows/e2e-ok-to-test.yml, .github/workflows/e2e.yml, .github/workflows/functional-ok-to-test.yml, .github/workflows/functional-tests.yml — All four changed workflow files are under .github/, a protected path requiring human approval. The PR links to e2e: non-ok-to-test label events cancel in-progress PR e2e runs #5455 and ci(#5455): stop non-ok-to-test labels cancelling e2e runs #5457 and explains the rationale for splitting the ok-to-test label trigger into caller workflows. Human approval is required for protected-path changes regardless of context.

• [workflow-architecture-staleness] docs/testing/functional-tests.md:201 — Documentation states "The workflow is defined in .github/workflows/functional-tests.yml" but the workflow has been split into two files: functional-tests.yml (main reusable workflow) and functional-ok-to-test.yml (thin caller for labeled events). The singular reference is now incomplete.
  Remediation: Update lines 201–203 to mention both workflow files.

Low

• [edge-case] .github/workflows/e2e-ok-to-test.yml:19 — When the thin caller invokes e2e.yml via workflow_call, GitHub reports check runs under the caller workflow name (E2E ok-to-test / run / gate, E2E ok-to-test / run / e2e) rather than the reusable workflow name (E2E Tests / gate, E2E Tests / e2e). If branch protection rules list E2E Tests / e2e as a required status check, the ok-to-test path will not satisfy that requirement directly. This is likely acceptable because the initial opened/synchronize run of e2e.yml produces E2E Tests / e2e in a skipped state (GitHub treats skipped required checks as passing by default), but operators should verify their branch protection / ruleset configuration handles this correctly. The same applies to Functional ok-to-test.

Previous run

Review

Findings

Medium

• [protected-path] .github/workflows/e2e-ok-to-test.yml, .github/workflows/e2e.yml, .github/workflows/functional-ok-to-test.yml, .github/workflows/functional-tests.yml — All four changed workflow files are under .github/, a protected path requiring human approval. The PR links to e2e: non-ok-to-test label events cancel in-progress PR e2e runs #5455 and ci(#5455): stop non-ok-to-test labels cancelling e2e runs #5457 and explains the rationale for splitting the ok-to-test label trigger into caller workflows. Human approval is required for protected-path changes regardless of context.

Low

• [edge-case] .github/workflows/e2e-ok-to-test.yml:19 — When the thin caller invokes e2e.yml via workflow_call, GitHub reports check runs under the caller workflow name (E2E ok-to-test / run / gate, E2E ok-to-test / run / e2e) rather than the reusable workflow name (E2E Tests / gate, E2E Tests / e2e). If branch protection rules list E2E Tests / e2e as a required status check, the ok-to-test path will not satisfy that requirement directly. This is likely acceptable because the initial opened/synchronize run of e2e.yml produces E2E Tests / e2e in a skipped state (GitHub treats skipped required checks as passing by default), but operators should verify their branch protection / ruleset configuration handles this correctly. The same applies to Functional ok-to-test.

---

Labels: PR modifies CI workflow files for e2e and functional test triggers

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:04 AM UTC · Completed 10:18 AM UTC
Commit: 15ac292 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 10:24 AM UTC · Completed 10:35 AM UTC
Commit: 15ac292 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5502 — ci(e2e): split ok-to-test label trigger into caller workflows

Workflow shape: Human-authored CI PR → 2 review agent runs → human approval → merge. No code/fix/triage agents involved.

Timeline

1. 06:00 UTC — PR opened by @ifireball. Splits ok-to-test label handling into thin caller workflows (e2e-ok-to-test.yml, functional-ok-to-test.yml) that invoke main test suites via workflow_call. 5 files changed (+95/−43).
2. 06:01 — Review agent dispatched (run 29983758696). 6 sub-agents (Correctness, Security, Intent-Coherence, Style-Conventions, Docs-Currency, Challenger) run in parallel.
3. 06:07 — qodo-code-review catches a real bug: both caller workflows set permissions: {}, preventing required GITHUB_TOKEN scopes (pull-requests: write, contents: read, id-token: write). Ok-to-test runs would fail.
4. 06:14 — fullsend-ai-review posts findings: protected-path (medium, procedural), check-run naming edge-case (low, actionable). Does NOT flag the permissions bug.
5. 07:25 — Human reviewer @rh-hemartin agrees with the check-run naming edge-case, asks about status sync.
6. 08:05 — @rh-hemartin approves.
7. 10:03 — @ifireball pushes fix for permissions (commit 15ac292), granting required scopes on caller jobs.
8. 10:04 — Second review agent run (run 29997831043). Adds a new finding: workflow-architecture-staleness in docs/testing/functional-tests.md:201 (still references only functional-tests.yml, now incomplete after the split).
9. 10:21 — PR merged. Staleness finding unaddressed.

Review quality analysis

What the review agent missed: The permissions: {} bug is a functional correctness issue — caller workflows with empty permissions blocks cannot pass required scopes to reusable workflows called via workflow_call. The Security sub-agent specifically examined GITHUB_TOKEN scope inheritance and the Correctness sub-agent traced workflow_call event propagation, yet neither flagged the empty permissions block. qodo caught it.

This is a recurring pattern. This is at least the 3rd instance in 16 days:

• PR Cognitive Debt: how to avoid loss of understanding when agents are driving change #32 (Jul 7) — missed issues: write for github.rest.issues.createComment() → agents#46
• PR feat(dispatch): synchronous workflow_call event dispatch (ADR 41) #1611 (Jul 13) — missed permissions: {} blocking OIDC in workflow_call jobs → agents#142
• PR ci(#5481): add mint-cf-worker-test workflow and stub Make target #5482 (Jul 23) — missed merge_group trigger, redundant concurrency, missing timeout-minutes → agents#394
• PR ci(e2e): split ok-to-test label trigger into caller workflows #5502 (Jul 23, this PR) — missed permissions: {} on caller workflows preventing required scopes

Existing open issues that address this gap:

• agents#142 — workflow_call permission inheritance checks (directly covers this case)
• agents#394 — CI workflow correctness checks for sub-agents (filed today from a sibling PR)
• agents#46 — bidirectional permission verification
• fullsend#2658 — verify CI workflow permission scopes match tool requirements
• fullsend#5521 — add CI workflow conventions doc to close review knowledge gap

No new proposals filed for the permissions gap — the existing issues comprehensively cover this improvement area. This retro provides same-day corroborating evidence that these issues remain high-priority.

What the review agent got right:

• The check-run naming edge-case (low severity) was valid and prompted useful human discussion. @rh-hemartin agreed it was a real concern. This demonstrates the review agent catching subtle operational implications that humans engaged with.
• The docs-staleness finding on the second run correctly identified that docs/testing/functional-tests.md:201 still references only functional-tests.yml after the workflow was split into two files. Valid but unaddressed before merge (posted 17 min before merge).
• The Challenger sub-agent effectively removed at least one false positive, demonstrating the adversarial quality-control step working as designed.
• Label recommendations (component/ci, component/e2e) and requires-manual-review were appropriate.

Cost and efficiency

• Review cost: ~$4.87 per run (6 sub-agents, Opus 4.6 orchestrator). Reasonable for a CI workflow change touching protected paths.
• Re-review cost: Similar (~$5). Justified — the push included substantive permission changes, and the re-review found a new staleness issue.
• Token efficiency: Sub-agents attempted to read new files from disk (unavailable on base branch checkout), gracefully fell back to diff analysis. Minor wasted I/O but no impact on quality.

Agents repo

Discovered from run logs: fullsend-ai/agents at commit ca518d9353f5a8363c6ba499d7a2070a1b0e7c5d.