Skip to content

ci: refuse to re-dispatch a focused test that already failed at that commit - #13680

Merged
teamleaderleo merged 2 commits into
mainfrom
fix-focused-dispatch-repeat-guard
Sep 22, 2026
Merged

teamleaderleo merged 2 commits into
mainfrom
fix-focused-dispatch-repeat-guard

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

test-e2e.yml took 959 dispatches in 7 days, 531 of them red, for 6,730 runner-minutes. The expensive failures are not flaky tests. Sampled logs show Swift compile errors on the dispatched ref:

SplitPaneBackgroundUITests.swift:89:13: error: invalid redeclaration of 'terminal'
DeviceSurfaceProviderRegistry.swift:56:46: error: call to main actor-isolated initializer ...
Cannot find 'pullRequestWorkspacePathBelongsToRepository' in scope

A focused run compiles the whole tree before it runs anything, so a 10-20 minute macOS run existed only to discover that the branch does not build. scripts/ci/require_selected_test_execution.sh then correctly reports that no selected test executed — the guard works, it is being fed refs that do not compile.

The single worst line item: cmuxUITests/SplitPaneBackgroundUITests was dispatched 24 times in 12 hours for 397 runner-minutes and passed zero times. Two of those failures were the same assertion, two were the same compile error. test-depot.yml shows the same pattern (348 dispatches, 213 red, 5,335 runner-minutes) against branches that do not compile.

Two changes

1. The launcher refuses a repeat that cannot say anything new.

A focused run's result is a property of the commit. Before dispatching, the launcher now looks for a completed run of the same selector at the same commit — the run name already carries both halves, <selector> on <runner> @ <commit> [<dispatch id>], so no local state is needed — and refuses when one failed and none succeeded, naming the earlier run:

cmuxTests/Foo already failed at <sha> (3 time(s)); the newest is <url>.
A focused run compiles the tree first, so the most common red result is a
compile error in the branch, not a flaky test -- and re-running the same
selector at the same commit returns the same answer. Read that run, fix the
branch, push, and dispatch the new commit. Pass --force to dispatch anyway.

--force keeps the escape hatch for a failure you know was infrastructure. The lookup is an economy measure and never a gate: if the run history cannot be read for any reason, it dispatches exactly as before.

2. The instruction that produced most of this could not work.

skills/cmux-testing/references/local-vs-ci-validation.md said:

Run through GitHub Actions or the VM: gh workflow run test-e2e.yml.

test_filter is required: true, so that command is rejected by the API. Anyone following it literally gets an error and improvises, which is where filters matching zero tests come from. It also omits --ref, so a dispatch that does succeed lands on whatever ref is current rather than the commit under test — while docs/contributor-verification.md has the correct form via scripts/run-e2e.sh, which the skill never mentions.

The section now points at the wrapper with --ref, says to compile the test target locally first (the same file already gives that exact cmux-unit command eight lines earlier), and says not to repeat a selector at a commit that already failed.

Validation

Two commits, per the repository's regression policy:

  1. d2039ef adds five cases to tests/test_run_e2e.py only — two fail against the current launcher.
  2. ce45716 adds the guard and the skill fix — all 19 pass.

The new cases cover: a prior failure at the same commit refuses and does not dispatch; --force dispatches anyway; a prior success does not block; a failure of a different selector does not block; a failure at a different commit does not block. The fake gh harness distinguishes the pre-dispatch lookup from the post-dispatch correlation by whether the caller asked for conclusion, rather than by call ordering.

The 14 pre-existing cases still pass unchanged, including with the guard's extra lookup in place.

Scope and what I did not do

This targets the repeat-dispatch class specifically. It does not stop the first dispatch against a non-compiling ref — that is what the local cmux-unit build in the skill is for, and enforcing it mechanically would mean a cheap pre-gate on the workflow, which is a real cost decision and a separate change.

I also have not touched test-depot.yml, which shows the same pattern from the same source. It has no in-repo dispatcher to add a guard to, and its workflow_call trigger is dead (no callers anywhere) while its workflow_dispatch half is actively used ~50 times a day — so it needs its own change, not deletion.

Numbers are from the 7-day totals in docs/ci/workflow-inventory.md; the root-cause sampling is from a 17-hour window of runs, so the per-class proportions are a sample rather than a weekly figure.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Stops the focused-test launcher from re-dispatching a selector at a commit where it already failed, cutting wasted macOS runner minutes that mostly reprint Swift compile errors rather than flaky failures. The launcher now looks up completed runs by selector and commit (the run name already encodes both) and refuses with a link to the earlier run unless --force is passed; if history can't be read, it dispatches as before. The testing skill now points to scripts/run-e2e.sh --ref <sha>, which is the only working form (test_filter is a required input), and tells readers to compile the test target locally first and never repeat a failed selector at the same commit.

Written for commit ce45716. Summary will update on new commits.

Review in cubic

teamleaderleo and others added 2 commits September 22, 2026 07:53
These tests fail on the current launcher. They are committed before the guard so
CI records that they detect its absence.

A focused run compiles the tree before it runs anything, so a red result belongs
to the commit. Re-dispatching the same selector at the same commit spends
another 10-20 macOS runner-minutes to reprint the same failure.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…commit

test-e2e.yml took 959 dispatches in 7 days, 531 of them red, for 6,730 runner
minutes. The expensive failures are not flaky tests: sampled logs show Swift
compile errors on the dispatched ref, so a 10-20 minute macOS run existed only
to discover the branch does not build. One selector,
cmuxUITests/SplitPaneBackgroundUITests, was dispatched 24 times in 12 hours for
397 runner-minutes and never once passed.

A focused run compiles the whole tree before running anything, so a red result
is a property of the commit. The launcher now looks for a completed run of the
same selector at the same commit, and refuses to dispatch when one failed and
none succeeded, naming the earlier run so it can be read instead of repeated.
--force keeps the escape hatch. The lookup is an economy measure, never a gate:
if the history cannot be read it dispatches as before.

The instruction that produced most of this could not work. The testing skill
said to run `gh workflow run test-e2e.yml`, but test_filter is a required input,
so that command is rejected by the API and the reader improvises -- which is
where filters that match zero tests come from. It now points at
scripts/run-e2e.sh with --ref, says to compile the test target locally first,
and says not to repeat a selector at a commit that already failed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 0a9647c2-291f-4f90-87f4-f9f83f66aaf0

📥 Commits

Reviewing files that changed from the base of the PR and between 6b2ba6d and ce45716.

📒 Files selected for processing (3)
  • scripts/ci/dispatch-focused-test.py
  • skills/cmux-testing/references/local-vs-ci-validation.md
  • tests/test_run_e2e.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo
teamleaderleo merged commit 9a3de8f into main Sep 22, 2026
53 of 54 checks passed
@teamleaderleo
teamleaderleo deleted the fix-focused-dispatch-repeat-guard branch September 22, 2026 15:47
teamleaderleo added a commit that referenced this pull request Sep 22, 2026
#13680 landed after this branch started. Rebasing kept its refusal but left
it reading a list where it expects one selector, and a batched run would
have slipped past it in two ways.

Refuse per entry, so one already-red selector stops the whole dispatch: the
batch shares a single compile, so it would only reprint a failure we have.

Match selector membership in the run title instead of a prefix. A batched
run names several selectors before " on ", so prefix matching would have
made every batch invisible to the guard, including for its own entries on a
later dispatch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
teamleaderleo added a commit that referenced this pull request Sep 22, 2026
* ci: run a batch of e2e filters against one compile

test-e2e.yml compiles the app from scratch on every dispatch: it runs
`xcodebuild ... test`, and -only-testing: narrows execution, not
compilation. Chasing one flake therefore costs a full cold Debug build per
filter, and dispatchers routinely fire several filters at the same commit
seconds apart:

  21:37:54  cmuxUITests/SplitPaneBackgroundUITests            @ 9d2a8c0
  21:37:51  cmuxTests/SplitPaneGeometryProjectionRenderParity @ 9d2a8c0
  21:37:49  cmuxTests/SplitPaneGeometryProjectionTests        @ 9d2a8c0
  21:37:46  cmuxTests/TerminalWindowPortalProvisionalGeometry @ 9d2a8c0
  21:37:44  cmuxTests/WorkspaceSplitProvisionalGeometryTests  @ 9d2a8c0

Five dispatches, ten seconds apart, same SHA, five independent ~16 minute
compiles of identical source. Measured over the last 90 parsed dispatches
(46 distinct refs), 65 runs sit inside a same-ref burst under two minutes
wide, and 43 of those are redundant compiles.

test_filter now accepts a comma-separated list. Each entry becomes its own
-only-testing: flag, which xcodebuild unions, so one compile serves the
whole batch. scripts/ci/dispatch-focused-test.py takes several positional
filters and joins them into one dispatch.

Entries must share a target, because one invocation runs one scheme;
mixing cmuxTests and cmuxUITests is rejected rather than silently running
half the request. Duplicates and empty entries are rejected too. A single
filter behaves exactly as before, including bare class names targeting UI
tests and the DisplayResolutionRegressionUITests harness, which stays
scoped to a one-selector dispatch.

require_selected_test_execution.sh proves that some tests ran, never which
ones, so a batch could otherwise let a healthy count from one selector
cover a sibling that never started. Each requested suite is now required
by name in the xcodebuild log.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* ci: keep the re-dispatch guard working for batches

#13680 landed after this branch started. Rebasing kept its refusal but left
it reading a list where it expects one selector, and a batched run would
have slipped past it in two ways.

Refuse per entry, so one already-red selector stops the whole dispatch: the
batch shares a single compile, so it would only reprint a failure we have.

Match selector membership in the run title instead of a prefix. A batched
run names several selectors before " on ", so prefix matching would have
made every batch invisible to the guard, including for its own entries on a
later dispatch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* ci: do not fail a batch on swift-testing suite naming

The per-suite accounting I added greps "Test Suite 'Name'", which only
XCTest prints. cmuxTests also runs swift-testing, which prints
Suite "Display Name" -- double quotes, and a display name that can differ
from the identifier passed to -only-testing:. Batching a swift-testing
suite would therefore have failed a run that executed correctly.

Accept either form, report an unseen suite as a warning, and fail only
when nothing requested was observed at all. That still catches the case
this check exists for -- a batch that silently ran none of what was asked
for -- without inventing failures from output-format differences.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* ci: reject a leading or trailing comma in test_filter

The split loop never visits a trailing empty field, so
"cmuxTests/AlphaTests," passed as a single selector instead of failing the
empty-entry check. Reject both edges before splitting.

Reported by CodeRabbit on #13695.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* ci: stop the suite accounting failing green swift-testing runs

The check I added matched "Test Suite 'Name'" and 'Suite "Name"' and hard
failed when it found neither. swift-testing prints a bare @suite unquoted --
"Suite RemoteTmuxMirrorPaneInputMappingTests started." -- so neither pattern
matched it, and there was no single-selector exemption. Of the 821 suites in
cmuxTests, 528 are bare and 293 carry a @suite("...") display name that is
not the identifier -only-testing: takes. Only the 341 XCTestCase classes
matched, so most focused cmuxTests dispatches would have gone red on a test
run that passed, including the example in the dispatcher's own help text.

Match the unquoted form too, and stop failing on a miss. A display name can
never be matched by identifier, so absence is not evidence a suite did not
run, and require_selected_test_execution.sh already owns pass/fail. The
accounting now reports and nothing more.

That leaves the batch gap open: a batch can pass with only one of its
selectors executed, because that guard counts tests rather than naming them.
Closing it needs the typed xcresult that run-app-host-xcodebuild.sh already
writes, which is worth doing separately. Batching is no worse than today's
single dispatch in the meantime.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* ci: brace the suite expansion so shellcheck can parse it

SC1087: "$suite[ .]" reads as an array expansion, which failed the
Testbox broker trust boundary guard. Braces, same behaviour -- verified
against all four log shapes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* ci: drop the comma pattern shellcheck proved unreachable

SC2221/SC2222: in ",*|*,|*, )" the "*," arm always wins over "*, ", so
the spaced arm never matches. It was redundant anyway -- a trailing comma
followed by whitespace still fails the empty-entry check inside the loop,
which the behaviour matrix confirms.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant