ci(#5455): stop non-ok-to-test labels cancelling e2e runs

[ifireball]

Summary

• Align cancel-in-progress in E2E and Functional Tests workflows with the existing gate filter so non-ok-to-test labeled events (e.g. ready-for-review) no longer cancel an in-progress opened/synchronize run.
• Document the concurrency behavior in the e2e testing guide.

Related Issue

Closes #5455

Changes

• .github/workflows/e2e.yml — only cancel for actionable PR events
• .github/workflows/functional-tests.yml — same change
• docs/guides/dev/e2e-testing.md — note that other labels neither authorize nor cancel

Testing

• make lint (pre-commit) passed on staged files
• Manual verification: bot PR open + immediate ready-for-review should leave the initial e2e run running

Checklist

• Gate if: and cancel-in-progress use the same actionable filter
• Both e2e and functional-tests workflows updated
• Docs updated

Made with Cursor

[qodo-code-review]

PR Summary by Qodo

CI: prevent non-ok-to-test labels from canceling E2E runs

⚙️ Configuration changes 📝 Documentation 🕐 20-40 Minutes

AI Description

• Prevent non-ok-to-test labeled events from canceling in-progress PR E2E runs.
• Apply the same concurrency cancellation rule to Functional Tests.
• Document which PR events/labels can cancel per-PR CI concurrency runs.

Diagram

graph TD
  E(["PR event"]) --> D{"Actionable?"} --> W1["e2e.yml concurrency"] --> G["gate job if:"]
  D --> W2["functional-tests.yml concurrency"] --> G
  D --> N["Do not cancel run"]
  W1 --> Doc["e2e-testing.md"]

  subgraph Legend
    direction LR
    _evt(["Event"]) ~~~ _dec{"Decision"} ~~~ _cfg["Workflow/Doc"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Factor the actionable filter into a shared reusable workflow

• ➕ Avoids duplicating the same cancel-in-progress expression across multiple workflows
• ➕ Keeps behavior consistent as more PR-test workflows are added
• ➖ Requires restructuring workflows (reusable workflow calls), which may be more change/risk than warranted for a small fix
• ➖ Harder to read/debug than inline expressions for simple cases

2. Use YAML anchors to reuse the expression within each workflow file

• ➕ Reduces duplication while keeping workflows as standalone YAML
• ➕ Minimal behavioral risk compared to refactoring into reusable workflows
• ➖ Cross-file reuse still isn’t possible (duplication remains across e2e vs functional)
• ➖ Anchors can reduce clarity for reviewers unfamiliar with YAML merge/anchor syntax

Recommendation: The PR’s approach is the right minimal fix: align cancel-in-progress with the existing gate’s actionable-event logic so incidental labeled events don’t disrupt authorized runs. Consider reusable workflows or YAML anchors only if this pattern needs to be replicated across many additional CI workflows.

Files changed (3) +16 / -2

Documentation (1) +6 / -0

e2e-testing.mdDocument which labels/events can cancel per-PR E2E/functional concurrency +6/-0

Document which labels/events can cancel per-PR E2E/functional concurrency

• Clarifies that labels like 'ready-for-review' and 'component/*' neither authorize E2E nor cancel in-progress E2E/functional-test runs. Documents that only opened/synchronize/reopened and 'labeled' with 'ok-to-test' are cancellation triggers.

docs/guides/dev/e2e-testing.md

Other (2) +10 / -2

e2e.ymlAlign E2E concurrency cancellation with actionable PR events +5/-1

Align E2E concurrency cancellation with actionable PR events

• Updates 'concurrency.cancel-in-progress' to only cancel per-PR runs on actionable 'pull_request_target' events, including 'labeled' only when the label is 'ok-to-test'. Adds an explanatory comment clarifying why non-ok-to-test label events must not cancel in-progress runs.

.github/workflows/e2e.yml

functional-tests.ymlPrevent non-ok-to-test label events from canceling Functional Tests runs +5/-1

Prevent non-ok-to-test label events from canceling Functional Tests runs

• Applies the same 'cancel-in-progress' condition as E2E so only actionable PR events (or 'ok-to-test' labeling) can cancel an in-progress run. Adds a matching comment to document the rationale.

.github/workflows/functional-tests.yml

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:17 AM UTC · Completed 10:28 AM UTC
Commit: 402f931 · View workflow run →

[github-actions]

Site preview

Preview: https://40a8b9d3-site.fullsend-ai.workers.dev

Commit: 0882ce7a8fd73f464b3eadc87dc331a105d6ffe3

[rh-hemartin]

I have been hit by this, thanks for the fix.

[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. Guide under docs/guides/dev/ 📜 Skill insight ⌂ Architecture

Description

docs/guides/dev/e2e-testing.md is a documentation guide that lives outside the allowed
docs/guides/admin/ or docs/guides/user/ directories. This violates the guide placement
requirement and can confuse the intended audience and indexing.

Code

docs/guides/dev/e2e-testing.md[R160-164]

+Other labels (for example `ready-for-review`, `requires-manual-review`, or
+`component/*`) do **not** authorize e2e and do **not** cancel an in-progress
+e2e or functional-test run for that PR. Only `opened` / `synchronize` /
+`reopened`, and `labeled` when the label is `ok-to-test`, cancel in-progress
+work in the per-PR concurrency group.

Relevance

⭐ Low

Same relocation request was definitely rejected in PR #4901; team keeps dev guides under
docs/guides/dev/.

PR-#4901

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062077 requires all guide files under docs/guides/ to be placed under either
admin/ or user/. The PR modifies docs/guides/dev/e2e-testing.md, demonstrating the guide
remains under docs/guides/dev/ in this change.

docs/guides/dev/e2e-testing.md[160-164]
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
The modified guide file is located at `docs/guides/dev/e2e-testing.md`, but guides under `docs/guides/` must be placed in either `admin/` or `user/` and target a single audience.

## Issue Context
This PR updates `docs/guides/dev/e2e-testing.md`, which keeps the guide in a non-compliant directory (`dev/`).

## Fix Focus Areas
- docs/guides/dev/e2e-testing.md[1-5]
- docs/guides/dev/e2e-testing.md[160-164]

ⓘ 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.yml, .github/workflows/functional-tests.yml — These files are under the .github/ protected path. The PR links to issue e2e: non-ok-to-test label events cancel in-progress PR e2e runs #5455 and explains the rationale (aligning cancel-in-progress with the gate filter to prevent non-ok-to-test labels from cancelling in-progress e2e runs). Human approval is always required for protected-path changes, regardless of context.

Previous run

Review

Findings

Medium

• [protected-path] .github/workflows/e2e.yml, .github/workflows/functional-tests.yml — These files are under the .github/ protected path. The PR links to issue e2e: non-ok-to-test label events cancel in-progress PR e2e runs #5455 and explains the rationale (aligning cancel-in-progress with the gate filter to prevent non-ok-to-test labels from cancelling in-progress e2e runs). Human approval is always required for protected-path changes, regardless of context.

Previous run (2)

Review

Findings

Medium

• [protected-path] .github/workflows/e2e.yml, .github/workflows/functional-tests.yml — These files are under the .github/ protected path. The PR links to issue e2e: non-ok-to-test label events cancel in-progress PR e2e runs #5455 and explains the rationale (aligning cancel-in-progress with the gate filter to prevent non-ok-to-test labels from cancelling in-progress e2e runs). Human approval is always required for protected-path changes, regardless of context.

---

Labels: PR modifies CI workflow files for e2e and functional tests

[waynesun09]

Overall this is a small, well-scoped, and logically correct fix. I traced the full event x action matrix (push, merge_group, workflow_dispatch, and pull_request_target x {opened, synchronize, reopened, labeled+ok-to-test, labeled+other}) and confirmed the new cancel-in-progress expression behaves identically to the old one except for the intended case, and matches the gate: job's if: filter it's meant to mirror. e2e.yml and functional-tests.yml received identical changes.

One process note worth flagging: because pull_request_target always executes the workflow file from the base branch, this PR's own CI checks can never exercise the new logic before merge -- only the old, unfixed expression on main. The "Manual verification" checkbox in the description is unchecked accordingly, which tracks. This isn't a defect in the PR, just worth confirming after merge: while this was under review, three non-ok-to-test labels landed on this PR one second apart (10:28:37-39 UTC), and the resulting E2E runs (#6925, #6926) cancelled each other in quick succession under the current main logic (10:28:40-42 UTC) -- a live reproduction of #5455's exact failure mode. Worth deliberately re-running that scenario once this lands on main to confirm the fix holds in practice.

Left two inline suggestions on the cancel-in-progress expressions (defensive parens) and one on the docs update (mention the queued/skipped run and the sticky-comment gap) -- none blocking, all follow-ups/polish.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:58 PM UTC · Completed 1:10 PM UTC
Commit: 35371c5 · View workflow run →

[waynesun09]

Approving — the fix is logically correct (re-verified this round with a fresh independent pass, including a second model), and both issues from the first review round are genuinely fixed in 35371c5 (defensive parens on cancel-in-progress, and the docs clarification distinguishing gate-skipped from gate-rejected).

One item from the first round remains open and can't really be resolved before merge: because pull_request_target always executes the base branch's workflow file, this PR's own CI never exercises the new logic — the "Manual verification" checkbox is still unchecked. Not a blocker (it's inherent to this class of change), but worth deliberately re-running the #5455 scenario once this lands on main.

Left two more inline notes, both non-blocking:

• A doc wording precision issue — the new "finishes quickly as skipped" text undersells what happens when the concurrency group isn't idle.
• A note that the duplicated actionable-filter logic across both workflows has no safeguard against future drift beyond a comment — fine for now, worth a lint check if a third workflow ever adopts this pattern.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:12 PM UTC · Completed 2:25 PM UTC
Commit: 0882ce7 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:41 PM UTC · Completed 6:53 PM UTC
Commit: 0882ce7 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5457 — stop non-ok-to-test labels cancelling e2e runs

Timeline

1. 09:59 UTC — Human (ifireball) filed issue #5455: non-ok-to-test labels cancel in-progress e2e runs.
2. 10:00 UTC — Triage agent ran in ~5 min. Correctly diagnosed root cause (concurrency cancel-in-progress not filtering by label name), proposed the right fix, applied priority/high + ready-to-code.
3. 10:06 UTC — /fs-code dispatched code agent (run 29910621227). Agent produced the fix but push was rejected — GitHub App token lacks workflows write permission for .github/workflows/ files.
4. 10:16 UTC — Human created PR #5457 manually (3 files, +35/−7).
5. 10:17–10:28 UTC — Review agent first run: dispatched 5 sub-agents (correctness, security, intent-coherence, style-conventions, docs-currency). All returned clean except one [protected-path] finding for .github/ files.
6. 10:19 UTC — Human (rh-hemartin) approved.
7. 11:56 UTC — Human (waynesun09) posted detailed review with 5 actionable findings: missing defensive parentheses in boolean expressions (2 files), documentation completeness gaps (2 items), and code duplication drift risk.
8. 12:56 UTC — Author addressed all feedback in commit 35371c5.
9. 12:58 UTC — Review agent second run: same [protected-path] finding, no new technical findings.
10. 14:08 UTC — Human (waynesun09) approved with 2 non-blocking notes.
11. 14:12 UTC — Review agent third run: same [protected-path] finding.
12. 18:38 UTC — PR merged.

Review quality gap

The review agent ran 3 times (15 sub-agent invocations total) and produced zero actionable technical findings beyond the protected-path policy flag. The human reviewer found 5 actionable items spanning code quality (operator precedence / defensive parentheses), documentation accuracy (oversimplified description of run states), and maintainability (duplicated filter logic with no automated sync enforcement).

The correctness sub-agent did extensive work — searched for cancel-in-progress, ok-to-test, workflow events, read ADRs 0054/0063/0009 — and correctly concluded the logic is functionally correct. However, it missed that the cancel-in-progress expression A && B || C && D relies on implicit &&-over-|| precedence without explicit parentheses, making it fragile to future edits. This is a classic code review find that any experienced reviewer would flag.

What worked well

• Triage agent performed excellently: correct root cause, correct fix suggestion, proper labels, all within 5 minutes.
• Protected-path detection worked correctly — the review agent never approved, correctly deferring to human judgment for .github/ changes.
• Label application was accurate (component/ci, component/e2e, requires-manual-review).

Evidence for existing issues (not proposed as new issues)

• Code agent should treat .github/workflows/ as unpushable when coder token lacks workflows permission #3627 / Triage agent should avoid recommending .github/workflows/ changes when coder token lacks workflows permission #3814 (code agent should treat .github/workflows/ as unpushable): This PR provides another data point — the code agent produced the correct fix but couldn't push, forcing a 3-minute human handoff. The triage agent also recommended the .github/workflows/ change despite the permission limitation.
• Review agent should fact-check technical claims in documentation PRs #2199 (review agent should fact-check technical claims in docs): The docs-currency sub-agent missed that the new documentation oversimplified run behavior — claiming runs "finish quickly as skipped" when they can actually sit pending or show as cancelled in burst scenarios.
• ci: enforce e2e/functional actionable-label filter stays in sync #5479 (enforce actionable-label filter sync): Filed as a direct follow-up from this PR's review — the duplicated filter logic across 2 workflows × 2 locations was flagged by the human reviewer as a drift risk.
• Correctness sub-agent should verify symmetric application of defensive patterns across all diff call sites agents#380 (correctness sub-agent defensive pattern checks): Related but distinct — that issue covers symmetric application of defensive patterns across call sites, whereas the gap here is detecting the need for defensive patterns (explicit parentheses) in the first place.

Proposals filed

• Correctness sub-agent should flag implicit operator precedence in complex boolean expressions (in fullsend-ai/agents)