CI backstop: cargo test --workspace - #505
Conversation
The rust job names its test suites one step at a time. That buys a per-suite timeout and a step title that names a wedged binary even when the runner dies before uploading logs, and it costs the thing this fixes: the list, rather than the workspace, decides what runs. Two dl suites were off it. picker (7 tests) and terminal (1) are built by the Build step and run by nothing, and terminal is the test written to prove the terminal-restore fix in this same release. devlaunch-test-support had been off it once before and went back with a note about it, which is the pattern. So: one `cargo test --workspace --locked` step after the named ones, same single-threaded run and same log-pipe fence, ~2 minutes in a job that takes 2:15 and is allowed 30. And timeout-minutes on the three jobs that had none: ci, gate, and review, which polls a remote API in a sleep loop. test/test_ci_workflow.py holds all of it: the backstop, the named steps it is a net under, and a timeout on every job rather than on the three that were missing one.
Reviewer's GuideThe PR closes CI coverage gaps by retaining the diagnostically useful per-suite Rust test steps and adding a locked, single-threaded workspace-wide backstop, while bounding every CI job and adding regression tests to prevent either safeguard from being removed. Flow diagram for the Rust CI test backstopflowchart LR
Build[Build Rust workspace] --> Named[Run named per-suite tests]
Named --> Backstop[Run cargo test --workspace --locked -- --test-threads=1]
Backstop --> Clippy[Run Clippy]
Named -.-> Triage[Per-suite diagnostics and timeouts]
Backstop -.-> Coverage[Catch test binaries missing from the list]
Flow diagram for CI workflow regression guardsflowchart TD
Workflow[.github/workflows/ci.yml] --> Strip[Strip comments]
Strip --> Workspace[Assert rust runs --workspace]
Strip --> Suites[Assert named suite steps remain]
Strip --> Timeouts[Assert every job has one timeout]
Workspace --> Tests[test_ci_workflow.py]
Suites --> Tests
Timeouts --> Tests
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test/test_ci_workflow.py" line_range="156" />
<code_context>
+ Per job rather than per named job: the three that were missing one are
+ fixable once, and the next job somebody adds is what this is for.
+ """
+ unbounded = [name for name in job_names() if "timeout-minutes:" not in settings(ci_job(name))]
+ assert not unbounded, (
+ f"these ci.yml jobs run unbounded: {unbounded}. GitHub's own default is "
+ "six hours, which is long enough that the run is abandoned rather than read"
</code_context>
<issue_to_address>
**issue (testing):** The timeout guard searches the entire job text for the substring `timeout-minutes:` rather than requiring a job-level key, so a job with no job timeout passes if a nested action input or other step content happens to contain `timeout-minutes:`.
**Triggers:** When a future job passes `timeout-minutes` as a step or action input without setting the GitHub job-level timeout.
**Suggested fix:** Match `timeout-minutes` at the job's exact indentation or parse the job mapping and check its top-level key.
```suggestion
unbounded = [name for name in job_names() if not re.search(r"^ timeout-minutes:", settings(ci_job(name)), re.MULTILINE)]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed with fresh eyes against two independent axes. The change does what it says: I confirmed on this PR's own run (33263851999, job 99130273744) that rust/dl/tests/picker.rs (7 tests, 4.18s) and rust/dl/tests/terminal.rs really did run, green, inside the backstop step and nowhere else in the job. cargo runs test binaries serially, so --test-threads=1 on top of that gives a fully serial run and both pty suites came out clean. The step measured 16:48:06 to 16:49:21, so the 72s in the comment is the real number and the ~2 minutes estimate was correctly replaced in d34b409. CHANGELOG lands under ## [Unreleased] -> ### Fixed.
Two real defects, both small, both inline. The rest is context.
Standards
Material -- test_every_job_carries_a_timeout passes on a job that is still unbounded. Inline at test/test_ci_workflow.py:156. Proven by mutation.
Material -- the backstop prints nothing when it fails. Inline at .github/workflows/ci.yml:282. Proven by running the step's exact two lines under the exact shell GitHub used.
Minor -- a step name can satisfy the backstop assertion. run_lines() (line 94) keeps every non-comment line in the job slice, - name: lines included, so the assertions in test_the_rust_job_runs_the_whole_workspace_somewhere match against titles as readily as against commands. Mutation: replace the backstop's run: with echo skipping and title the step - name: 'disabled: cargo test --workspace --locked', and all three tests stay green. Same root cause as the finding at line 156 -- the helper does not distinguish run: content from other YAML keys. Lower realism than that one, but the module docstring specifically claims a comment mentioning --workspace cannot satisfy an assertion; the name line is the same hole one key over.
Minor -- flake surface on the gating job. interrupt and lock_wait now run twice per rust job. rust-coverage's own comment in this file calls them "the most flake risk in the suite" and deliberately leaves them out; rust is in gate's needs. The PR states the trade openly and it is the right one, but it is a doubling on the job that blocks merges.
Checked and not findings: the no-em-dash rule globs the README plus docs/*.md only (test_docs_prose.py), so the CHANGELOG and the new test are out of its scope. pyproject.toml:242 ("--workspace where CI names the suites one at a time") reads as stale but is not -- its referent is the rust-coverage job, which still names its suites and still excludes interrupt/lock_wait. The 25-line comment block is proportionate to what ci.yml already carries above rust, review and gate. Markers, file location and naming match the sibling guards.
Spec
Held against #305. All of the PR body's factual claims check out at the merge-base: 15 named per-suite steps, rust/dl/tests/ holds ten files, picker.rs and terminal.rs are the two with no step, and no other binary of substance was unrun (-p devlaunch-runner, -p devlaunch-core and -p devlaunch-test-support carry no target flags, so they sweep their whole package).
Minor -- scope beyond the ticket. Spec: "the ci job and gate carry no timeout-minutes". The PR sets ci: 20, gate: 5 and review: 10, and test_every_job_carries_a_timeout binds every future job. Defensible -- review is the sleep 10 poll loop -- and stated openly, but it is more than was asked.
Minor -- never is slightly overclaimed. Spec: "so a new suite can never silently not run". rust/Cargo.toml has an explicit members = [...], so a new crate left out of it is still invisible to --workspace. A new suite inside an existing crate -- the case the ticket was written about -- is fully covered.
Not a gap: deferring rust-coverage's list is permitted. The ask is "Add one cargo test --workspace --locked backstop step", singular, and rust-coverage's list caps a number rather than what is tested.
Timeout values against history
198 ci.yml runs, 2026-08-22 to 2026-08-29, per-job wall time from the jobs API:
| job | median | max | bound | headroom |
|---|---|---|---|---|
ci |
98s | 115s (1.9m) | 20m | ~10x |
review |
6s | 133s (2.2m) | 10m | ~4.5x |
gate |
3s | 5s | 5m | ~60x |
rust (pre-existing 30m) |
120s | 209s + 74s backstop | 30m | ~6x |
All three new bounds are safe. review is the tightest ratio and it is still fine: its poll loop's own structural worst case is 12 x sleep 10 plus checkout, roughly two and a half minutes, so 10 minutes sits above what the job can do rather than merely above what it has done. Nothing here converts a legitimately slow run into a red main.
Verdict
Request changes, blocking on the two inline findings:
.github/workflows/ci.yml:282-- a failing backstop produces no log at all, which defeats the step's own reason for buffering.test/test_ci_workflow.py:156-- the timeout guard is satisfied by a step-level key that does not bound the job.
Both are one-line fixes. Everything else above is minor and can be taken or left.
| timeout -k 10 900 cargo test --workspace --locked -- --test-threads=1 > workspace.log 2>&1; ec=$? | ||
| cat workspace.log; exit $ec |
There was a problem hiding this comment.
Material: when this step fails it prints nothing.
The run log for this step on 33263851999 says shell: /usr/bin/bash -e {0} -- GitHub's default for Linux is errexit on, pipefail off, and this workflow sets no defaults.run.shell and no per-step shell:. Under -e the shell exits at the failing timeout command itself: ec=$? never runs, and neither does cat workspace.log.
Reproduced with these exact two lines under the exact invocation:
$ cat t.sh
timeout -k 10 5 bash -c 'echo hello-from-test; exit 7' > workspace.log 2>&1; ec=$?
cat workspace.log; exit $ec
$ bash -e t.sh; echo "exit=$?"
exit=7
No hello-from-test. The exit code propagates correctly, so the step does go red -- what is lost is the entire log, and a run that timed out (124) loses it too.
That matters more here than on the four steps this fence was copied from. Those name one suite each, so a red tick already tells you which binary died. This step is now the only place picker.rs and terminal.rs run, so the first time the backstop catches the thing it was added to catch, the operator gets Process completed with exit code 101 and nothing else -- and a 1944-test run is the worst one to have to reproduce locally to find out what failed.
The || form is not tripped by errexit:
ec=0
timeout -k 10 900 cargo test --workspace --locked -- --test-threads=1 > workspace.log 2>&1 || ec=$?
cat workspace.log; exit $ecThe four pre-existing fenced steps (aid interrupt, aid interactive, dl lock_wait, dl interrupt) have the same bug and have been hiding their failure output all along; none of them happened to fail in the last 200 runs, which is why nobody has seen it. Worth the same fix, though that is beyond this PR.
There was a problem hiding this comment.
Fixed in 0bef3af, and you are right that this was the worst place for it: this step is the only place picker and terminal run at all, so a failure in the eight tests the PR exists to start running would have arrived as a red tick with an empty log.
Reproduced before fixing, under bash -e as the runner does it:
$ bash -e fence.sh # `; ec=$?`
exit=101 # nothing printed
$ bash -e fixed.sh # `ec=0` + `|| ec=$?`
THE LOG EVERYBODY SEES
exit=101
|| is a tested condition, which errexit does not fire on, so the cat always runs and $ec still carries the code, 124 included.
The four steps this was copied from still have it. Left alone to keep this PR about one thing, as you suggested, and written up on #305 (#305 (comment)) with the reproduction so it can be picked up as its own change.
| Per job rather than per named job: the three that were missing one are | ||
| fixable once, and the next job somebody adds is what this is for. | ||
| """ | ||
| unbounded = [name for name in job_names() if "timeout-minutes:" not in settings(ci_job(name))] |
There was a problem hiding this comment.
Material: this passes on a job that is still unbounded.
"timeout-minutes:" not in settings(ci_job(name)) is a substring search over the whole job slice, steps included. timeout-minutes is a valid step-level key in GitHub Actions, and a step-level one does not bound the job -- the job keeps the six-hour default.
Mutation, on a scratch copy of this branch's two files: take gate's job-level timeout-minutes: 5, delete it, and re-indent it onto its single step.
gate:
...
runs-on: ubuntu-latest
steps:
- name: Every job this gate covers must have succeeded
timeout-minutes: 5
if: always()gate now runs unbounded and all three tests stay green. Every other mutation I tried fails correctly -- deleting the timeout outright, deleting the backstop, commenting the backstop out, dropping --locked, stripping the per-suite -p steps -- so this is the one hole.
It is worth closing rather than shrugging at, because the property this file argues for is that a check nobody ran reads exactly like a check that passed, and the docstring two lines up claims a property the assertion does not actually check.
Anchoring to the jobs' own settings indentation on the comment-stripped slice closes it:
unbounded = [name for name in job_names() if "\n timeout-minutes:" not in settings(ci_job(name))]There was a problem hiding this comment.
Fixed in 0bef3af. Confirmed with your mutation before touching it: moving gate's timeout-minutes from the job onto its single step left all three tests green while gate ran unbounded.
The guard now walks the job once and returns two things: keys at the job's own indentation (4 spaces, which a step key at 8 cannot reach and a step list item cannot match at all, since the pattern has no leading -), and run: script content. Anything inside a block scalar is skipped rather than searched, so a shell line shaped like a key cannot be mistaken for one either.
After the fix, the same mutation:
E AssertionError: these ci.yml jobs run unbounded: [gate]
Same walker fixes the step-title hole you raised alongside it: a step named disabled: cargo test --workspace --locked with a neutered run: satisfied the backstop assertion before, and now fails it.
Three defects from review, all the same shape as the bug this PR is about: something that looks like it is checking, and is not. 1. The fence loses the log. GitHub runs a `run:` block under `bash -e`, so on a non-zero `timeout` the shell exits at that line and neither `ec=$?` nor the `cat` happens. The step still goes red, with an empty log, and the only copy of cargo's output is a file on a runner about to be discarded. That is worst here of anywhere: this step is the only place picker and terminal run. `ec=0` plus `|| ec=$?` -- a tested condition, which errexit ignores. Reproduced and fixed at exit 101; 124 on timeout propagates the same way. 2. The timeout guard substring-searched the job slice, and `timeout-minutes` is a legal key on a *step*, where it bounds the step and leaves the job unbounded. Demoting gate's timeout onto its one step left all three tests green. Now read at the job's own indentation. 3. `run_lines()` kept `- name:` lines, so a step *titled* `disabled: cargo test --workspace --locked` with a neutered `run:` satisfied the backstop assertion. Now only `run:` content is searched. Both mutations were confirmed green before and red after. The fence defect is pre-existing on the four steps this one copied; left alone here deliberately, noted on #305.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
1 participant
The
rustjob names its test suites one step at a time. That is worth keeping: each step carries its owntimeout, so a wedged binary fails a short step instead of holding the runner until it loses contact, and the step title names the culprit even when the runner dies before it can upload logs. What it costs is that the list, rather than the workspace, decides what runs. A test binary nobody wrote a step for is compiled byBuildand run by nothing, and no tick anywhere goes red to say so.The gap, measured on this tree
Not hypothetical, and worse than the ticket knew. Two
dlsuites are off the list right now:rust/dl/tests/picker.rsrust/dl/tests/terminal.rscargo test --workspace -- --test-threads=1runs 28 test binaries, 1944 tests. The fifteen named steps run 26 of them. Neither missing suite is#[ignore]d and both pass, so this is eight green tests that no CI run has ever executed.terminal.rsis the sharp one: it arrived in 151be70 ("The repair is tested where the user meets it: the shipped binary"), the guard on the terminal-restore fix sitting in[Unreleased]right now. The single test standing behind that repair had never run in CI.devlaunch-test-supportcame off the list once before and went back with a comment about it. That comment is still in the file. This is the same failure a third and fourth time.What changed
One backstop step in the
rustjob, after the named steps and before Clippy:Same
--test-threads=1(the concurrency tests must not interleave) and the same log-pipe fence aslock_waitandinterrupt, because this run includes them. The named steps stay: the list is what triage reads, the workspace is what decides.Cost, measured on this PR's own run (33263666172) rather than estimated: the backstop step takes 72s, and the
rustjob goes from 2:13 to 3:19 against a bound of 30 minutes. Nothing else needed resizing.timeout-minuteson every job that lacked one. The ticket namedciandgate; there were three.reviewpolls the GitHub reviews API in asleep 10loop up to twelve times, which is the shape that actually hangs, and it was the one running unbounded in a workflow that argues for timeouts three separate times.ci: 20,review: 10,gate: 5.The guard
test/test_ci_workflow.py, three tests, in the shape the repo's other workflow guards use (string assertions on a job slice, no YAML parser). Comments are stripped before every assertion, so a comment mentioning--workspacecannot satisfy one.Red before the change, and it named the problem itself:
The third test passed from the start and is the regression guard on the other direction:
--workspacemust not be allowed to replace the per-suite steps, because one step for 1944 tests is a wedge that eats the whole budget and names no suite. It also re-assertsdevlaunch-test-supportby name, since that is the one already known to fall off.The timeout check is per job rather than per named job on purpose. Fixing three jobs is a one-off; what the assertion is for is the next job somebody adds.
What the backstop does not promise
Two limits, said out loud rather than left to be discovered:
--workspaceis bounded byrust/Cargo.toml'smemberslist, which is explicit and has no globs. A new crate nobody adds to that list is invisible to the backstop exactly as a new suite was invisible to the step list. Same class of hole, one level up. Not closed here.interruptandlock_waitnow run twice perrustjob, once as their named step and once inside the backstop. They are the two suites that spawn and kill real process trees, so if flake ever turns up there, that is why.Deliberately not in scope
rust-coveragekeeps its own hand-written suite list, andpicker/terminalare missing from that too. Left alone: that list is a cap on a number rather than on what runs, the job's comment already declares caps out loud, and both suites drive a pty, which is a flake risk this change should not take on. Worth a follow-up ticket.Review fixes (second round)
Three defects from fresh-eyes review, all the same shape as the bug this PR is about: something that looks like it is checking and is not.
run:block runs underbash -e, so on a non-zerotimeoutthe shell exited at that line and neitherec=$?nor thecatran. Red tick, empty log, and the only copy of cargo's output a file on a runner about to be thrown away. Worst here of anywhere, because this step is the only placepickerandterminalrun at all. Nowec=0plus|| ec=$?, a tested condition that errexit does not fire on. Reproduced at exit 101 before and after the fix; a timeout's 124 propagates identically. The same defect is pre-existing on the four fenced steps this one was copied from, left alone here on purpose and noted on CI backstop: cargo test --workspace #305.timeout-minutesis a legal key on a step, where it bounds the step and leaves the job unbounded. Demotinggate's timeout onto its single step left all three tests green. The assertion now reads keys at the job's own indentation, and skipsrun:scripts rather than searching them.run_lines()kept- name:lines, so a step titleddisabled: cargo test --workspace --lockedwith a neuteredrun:satisfied the backstop assertion. Onlyrun:content is searched now.Both mutations were run against the guard: green before the fix, red after.
Gates
cargo test --workspace --locked -- --test-threads=1: 1944 passed, 118scargo clippy --locked --all-targets -- -D warnings: cleancargo fmt --check: cleanpixi run test: 430 passed, 6 skippedpixi run ruff-lint/pylint(10.00/10) /ty/prek: cleancheck yaml(prek) parses the edited workflow; all 8 jobs slice correctly and carry exactly one timeout eachCloses #305
🤖 Generated with Claude Code
Summary by Sourcery
Strengthen CI coverage and reliability by adding a workspace-wide Rust test backstop, enforcing timeouts across all jobs, and guarding these requirements with workflow tests.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: