test(ci): guard the Rust toolchain pin as an exact contract — Refs #503 - #506
Conversation
Refs #503 PR #490 fixed the two workflow jobs that bypassed cli/rust-toolchain.toml, but its guard was withdrawn before merge: it modelled workflow semantics with affix tests over shell text, absorbed twenty-two review findings across fourteen rounds, and was still simultaneously too loose (a reassignment walked through it) and too tight (an idiomatic printf publish was rejected). Nothing has watched the pin since. This restores a guard of the shape that finally held, and closes the one bypass still open when the branch was narrowed. The pin reader is an exact contract, compared verbatim against one canonical constant. There is a single such step per Cargo job and it is six lines, so no shell is parsed and nothing can be smuggled past a pattern; changing the step means changing PIN_RUN in the same commit. Around it, for every job that reaches Cargo: * exactly one unconditional dtolnay/rust-toolchain@master installer, consuming the reader's output; * read, then install, then Cargo — in that order; * no rustup command or `cargo +toolchain` selector in the job; * no RUSTUP_TOOLCHAIN at workflow, job, container.env or step scope, matched case-insensitively, because Windows resolves environment variables that way; * cli/rust-toolchain.toml is the only *tracked* toolchain file — a nested one overrides the installed pin for anything built from its directory, and two jobs build from exactly such a directory. Tracked via `git ls-files`, so a git-ignored .claude/worktrees checkout is not policed. Cargo is not only `run: cargo …`. Swatinem/rust-cache shells out to `cargo metadata` for its cache key, which ci.yml's own comment records, so above the installer it builds a cache key on the runner's default compiler with no script for a guard to read. That was the last finding against the withdrawn version and it is closed here: known Cargo-running actions count as Cargo invocations for the ordering check. Naming those actions is only worth something if the list cannot go quietly out of date, so every `uses:` in every workflow must be classified as the installer, Cargo-running, or Cargo-inert. An unclassified action fails by name and asks for the decision rather than being assumed harmless. ci.yml carries two changes the contract requires: `shell: bash` on the two pin steps that lacked it (the other two already had it), and the POSIX-sed note moved from inside the script to a YAML comment above the step, where it does not have to be repeated verbatim in four places.
|
@codex review Generated by Claude Code |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64bd5ea9d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let normalized = lower.split_whitespace().collect::<Vec<_>>().join(" "); | ||
| lower.contains("rustup") || normalized.contains("cargo +") |
There was a problem hiding this comment.
When a workflow formats a selector as cargo \ +nightly build, Bash removes the backslash-newline and executes cargo +nightly build (cargo --help explicitly lists cargo [+toolchain]), but this normalization retains the backslash and produces cargo \ +nightly, so the contains("cargo +") check misses it. Such a job still satisfies the canonical reader/installer checks while compiling with a different toolchain, defeating the guard's stated contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed at the root in bc2f60cc. I reproduced it before changing anything rather than reasoning about it.
Added as a case to later_toolchain_overrides_are_rejected_without_parsing_shell, "cargo \\\n +nightly build" failed the suite on 64bd5ea exactly as you describe — normalisation kept the backslash, so cargo \ +nightly never matched cargo +.
has_toolchain_override now drops \<newline> before normalising whitespace. Planted into the real ci.yml, in the cargo fmt --check step:
ci.yml: job `gates` contains a rustup command or explicit `cargo +toolchain` selector
And the other direction, which is where a tightening of this kind usually goes wrong: an innocently wrapped cargo fmt \ + newline + --all -- --check in the same step still passes, and two negative controls pin that. Removing the continuation must not invent a selector where the script has none.
It is two characters removed lexically, not a step toward interpreting shell — the check still only asks whether those substrings appear at all, which is what keeps it free of the model that sank the previous guard.
Gates on the pinned 1.95.0: fmt clean, clippy clean, full cargo test --locked green, tree restored after each planted mutation.
Generated by Claude Code
Codex review, PR #506. The shell removes a backslash-newline before it splits words, so cargo \ +nightly build runs as `cargo +nightly build`. `has_toolchain_override` normalised whitespace but kept the backslash, producing `cargo \ +nightly`, so the `cargo +` test missed it — a job could satisfy the canonical reader and installer checks and still compile on another toolchain. Reproduced first: the new case fails the suite before this change and passes after it, and planted into `ci.yml`'s `cargo fmt --check` step it is rejected by job and by reason. The opposite direction is covered too — an innocently wrapped `cargo fmt \` + newline + `--all` is still accepted, because removing the continuation must not invent a selector where the script has none. Two characters, removed lexically. The guard still asks only whether these substrings appear at all, so it remains free of any model of shell syntax.
|
Round 1 —
CI was green on all 14 checks on
No lint weakened, no @codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc2f60cc2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn has_toolchain_override(run: &str) -> bool { | ||
| let lower = run.to_ascii_lowercase().replace("\\\n", ""); | ||
| let normalized = lower.split_whitespace().collect::<Vec<_>>().join(" "); | ||
| lower.contains("rustup") || normalized.contains("cargo +") |
There was a problem hiding this comment.
On Windows, a step such as cargo.exe +nightly build bypasses this check because the normalized text contains cargo.exe +, not cargo +, even though cargo --help documents the syntax as Usage: cargo [+toolchain] [OPTIONS] [COMMAND]. Fresh evidence beyond the closed continuation case is that invokes_cargo already explicitly recognizes cargo.exe as Cargo, so this job is guarded but its toolchain override is accepted; normalize the executable form or check both names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed at the root in 53238b30. Your reasoning about why it was there is the part I acted on: invokes_cargo already accepted cargo.exe, so the two checks held different notions of what Cargo is called, and the gap between them is where both findings on this file have landed.
Reproduced before changing anything. With ci.yml's cargo fmt --check step rewritten to cargo.exe +nightly fmt --all -- --check:
bc2f60cc (before): 10 passed, 0 failed ← bypass
53238b30 (after): ci.yml: job `gates` contains a rustup command or explicit `cargo +toolchain` selector
Tree restored after.
is_cargo_executable is now the single definition both checks use, and the selector is read as two adjacent words — a Cargo executable followed by a +… — instead of the substring cargo . So every spelling the invocation check accepts is covered, not just the two you named: cargo.exe, C:\Users\runner\.cargo\bin\cargo.exe +stable build, and a quoted sh -c "cargo +nightly build", all now cases in the suite.
Two negative controls pin the direction a token rule can get wrong on its own: cargo-nextest run +extra is not Cargo, and echo 1 + 2 && cargo build is not a selector.
Gates on the pinned 1.95.0: fmt clean, clippy clean, cargo test --locked 55 suites / 1,651 tests / 0 failed.
Generated by Claude Code
Codex review, PR #506. `has_toolchain_override` searched for the literal `cargo ` followed by `+`, while `invokes_cargo` accepted `cargo.exe`, a path prefix and any case. A step running cargo.exe +nightly build was therefore counted as a Cargo invocation — so the job was guarded — and its selector was read straight past. Both findings on this file have been that gap: two notions of "Cargo" that did not agree. They agree now. `is_cargo_executable` is the single definition, and the selector is read as two adjacent words — a Cargo executable followed by a `+…` — rather than as a substring, so every spelling the invocation check already accepts is covered: `cargo.exe`, an absolute Windows path, a quoted `sh -c "cargo +nightly build"`. Reproduced before fixing: with `ci.yml`'s `cargo fmt --check` step changed to `cargo.exe +nightly fmt`, the suite at bc2f60c reports 10 passed, and with this change it fails naming the job and the reason. Tree restored. Two negative controls pin the other direction, which a token rule can get wrong in its own way: `cargo-nextest run +extra` is not Cargo, and a `+` somewhere else in the script is not a selector.
|
Round 2 — Worth naming the shape, because both findings on this branch are one shape rather than two: the guard held two notions of what Cargo is called. They agree now. Both closed at the root, and both reproduced against the real
No lint weakened, no @codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53238b301a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn has_toolchain_override(run: &str) -> bool { | ||
| let lower = run.to_ascii_lowercase().replace("\\\n", ""); |
There was a problem hiding this comment.
When a Cargo step in the checked ci.yml bridge-windows-packaged job is wrapped using PowerShell's native backtick continuation (for example, cargo.exe followed by a newline and +nightly build), PowerShell removes the backtick-newline and Cargo receives the selector, but this normalization removes only Bash's \-newline; split_whitespace therefore leaves the backtick as an intervening token and returns false. Fresh evidence beyond the closed Bash-continuation thread is that this job runs on windows-latest and its Cargo step does not select Bash, so pwsh syntax is in the guard's actual workflow scope; such a selector would override the installed pin while this test remains green.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed at the root in c7649415. Your scope argument is the load-bearing part and it checks out: bridge-windows-packaged runs on windows-latest, its Build source AWARE host step sets no shell:, so its default is pwsh and the backtick form is in this guard's actual workflow scope rather than hypothetical.
Reproduced in that exact job before changing anything, cargo.exe + backtick + newline + +nightly build:
53238b30 (before): 10 passed, 0 failed ← bypass
c7649415 (after): ci.yml: job `bridge-windows-packaged` contains a rustup command or explicit `cargo +toolchain` selector
Tree restored after.
I did not add a second special case. Three shells spell a continuation three ways — bash \, pwsh `, cmd ^ — so instead of learning them, the pairing now drops any token that is nothing but punctuation once trimmed, which is exactly what a lone backtick or caret is. One rule, and less shell knowledge than the special case it replaces. The backslash keeps its rewrite for a stated reason: \ is a real character inside a Windows path, so it cannot be treated as punctuation.
cargo ^ + newline + +stable build is a case too, and two more negative controls pin the other direction: a wrapped cargo build + backtick + --locked and an echo "+done" on the line after a Cargo command are both still accepted.
Gates on the pinned 1.95.0: fmt clean, clippy clean, cargo test --locked 55 suites / 1,651 tests / 0 failed.
Generated by Claude Code
Codex review, PR #506. Only bash's `\`-newline was removed, so a selector wrapped with PowerShell's backtick left a `` ` `` token between `cargo.exe` and `+nightly` and the pair went unread. The finding is in scope, not theoretical: `bridge-windows-packaged` runs on windows-latest and its Cargo step selects no shell, so pwsh is its default. Rather than learn three shells, the pairing now ignores anything that is nothing but punctuation once trimmed — which is what a lone backtick or a cmd caret is. One rule covers all three continuations, and it is less shell knowledge than the special case it replaces, not more. The backslash keeps its rewrite because `\` is a real character inside a Windows path and cannot be treated as punctuation. Reproduced before fixing, in that exact job: `cargo.exe ` + backtick + newline + `+nightly build` leaves the suite at 10 passed on 53238b3 and fails here naming `bridge-windows-packaged`. Tree restored. Both directions covered: the caret form is a case too, and two more negative controls pin that a wrapped `cargo build ` + backtick + `--locked`, and an `echo "+done"` on the line after a cargo command, are still accepted.
|
Round 3 — All three findings on this branch have been one thing: two of the guard's own notions disagreeing. Round 1 and 2 were two notions of what Cargo is called; this one is two notions of what "adjacent" means. Each fix collapses the pair into a single definition rather than patching the instance, which is the property the previous guard never had. Every case verified by executing the suite against a planted mutation of the real
Six rejections and four acceptances, because a tightening of this kind fails in both directions and the previous guard's record was half false positives.
No lint weakened, no @codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7649415d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .split_whitespace() | ||
| .map(bare_token) | ||
| .filter(|word| !word.is_empty()) |
There was a problem hiding this comment.
In the checked ci.yml bridge-windows-packaged job, which uses the default PowerShell shell, car + backtick-newline + go.exe +nightly build is executed as cargo.exe +nightly build; Microsoft documents that a terminal backtick allows input to continue on the next line. This code instead splits first, yielding car, go.exe, and +nightly, and dropping punctuation-only words never reconstructs the Cargo executable, so the override remains accepted. Normalize backtick-newline and caret-newline sequences before tokenization, as is already done for Bash's backslash-newline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and fixed at the root in 770dbeba — with one correction to the reproduction worth having on the record.
joined_script now removes all three continuations — bash \, pwsh backtick, cmd ^ — once, before anything reads the script, and both the Cargo-invocation check and the selector check read its result. You are right that splitting first can never reconstruct the word; the fix is to stop splitting first.
The correction. Planted into an existing job, your mutation was already rejected on c7649415 — but not for the reason either of us gives: the step stopped looking like Cargo at all, so bridge-windows-packaged dropped out of the found set and the explicit coverage list failed. To show the real escape I had to plant a new job, where nothing is added to that list and so nothing notices:
c7649415 (before): new windows job, `car` + backtick-newline + `go.exe +nightly build` -> 10 passed, 0 failed
770dbeba (after): same job -> the known Cargo-job set changed
Tree restored. So the finding is real and its consequence is worse than "the override is accepted": on the old code such a job was not a Cargo job at all, and the guard skipped it entirely. That is why the fix had to move into invokes_cargo too rather than only into the selector check.
Two negative controls pin the other direction, since joining words is exactly how this could go wrong: my-car + backtick-newline + go-tool build must not become Cargo, at either check.
I have also written the boundary into the file's doc, because each round has probed one step further along this same axis and it deserves a stated answer rather than another lap: this check sees a selector a contributor wrote, in any layout — the accident it exists to catch. It does not see one the shell assembles at run time (c=cargo; $c +nightly build, a value arriving in an environment variable, a script file the step calls). Nothing that reads YAML can, and a version that tried would be back to modelling shell, which is what sank the guard this one replaced.
Gates on the pinned 1.95.0: fmt clean, clippy clean, cargo test --locked 55 suites / 1,651 tests / 0 failed.
Generated by Claude Code
Codex review, PR #506. A continuation is not only *between* words: pwsh runs `car` + backtick-newline + `go.exe +nightly build` as `cargo.exe +nightly build`. Splitting first and then ignoring punctuation-only words can never put that word back together, so the executable was unreadable and with it the selector. `joined_script` now removes all three shells' continuations — bash `\`, pwsh backtick, cmd `^` — once, and both the Cargo-invocation check and the selector check read its result. That is the same correction as the previous two commits, a third time: two of this file's own notions disagreed, here about which text is being read, so there is now one. Reading the same text matters more than it looks. On the previous commit a NEW job whose only Cargo call was wrapped this way was not seen as a Cargo job at all, so the whole guard skipped it and the explicit coverage list stayed satisfied — nothing was added to it. Demonstrated exactly that way: such a job leaves c764941 green and fails here. (Planted into an EXISTING job the same mutation was already caught, by the coverage list noticing the job had dropped out of it. The new-job form is the one that escaped.) Two negative controls pin the other direction: joining must not manufacture Cargo out of `my-car` + backtick-newline + `go-tool build`. Also records where this check stops, since each round has probed one step further along the same axis: it reads a selector a contributor WROTE, in any layout, which is the accident this file exists to catch. One the shell assembles at run time — `c=cargo; $c +nightly` — is not visible to anything that reads YAML, and chasing it is how the previous guard died.
|
Round 4 — Four findings, one shape each time: two of the guard's own notions disagreeing. What Cargo is called (rounds 1–2), what "adjacent" means (round 3), which text is read (round 4). Each fix collapses the pair into one shared definition instead of patching the instance, which is why each has stayed a few lines rather than growing a model. This round also found something neither of us predicted. Planted into an existing job the reported mutation was already rejected — the step stopped looking like Cargo, the job dropped out of the found set, and the explicit coverage list failed. The real escape needs a new job, where nothing is added to that list: on CI is green on I have also written the boundary into the file's doc rather than leave it to be rediscovered: this check sees a selector a contributor wrote, in any layout — the accident the file exists to catch — and not one the shell assembles at run time (
No lint weakened, no @codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
cli/rust-toolchain.toml— nothing has watched the pin since PR ci: pin every Rust build from one reviewed setup #490's guard was withdrawn — in the shape that finally held there: the pin reader is an exact contract, compared verbatim, with no shell parsed anywhere.Swatinem/rust-cacherunscargo metadatawith norun:script, so above the installer it built a cache key on the runner's default compiler, invisibly.uses:in every workflow must be classified as the installer, Cargo-running, or Cargo-inert, and an unclassified one fails by name asking for the decision.Refs #503.
Type of change
cli/tests/toolchain_pin_gate.rs) + the two workflow lines its contract requiresDecalog check
Why this shape
The withdrawn guard modelled workflow semantics with affix tests over shell text and then a hand-rolled tokeniser. It absorbed 22 findings across 14 rounds and was still, at the end, both too loose and too tight —
channel=stableafter a valid assignment walked through it while an idiomaticprintf 'channel=%s\n'publish was rejected. Both are one defect: re-implementing POSIX shell inside a test file. So this guard does not try. The reader is six lines and appears once per Cargo job; changing it means changingPIN_RUNin the same commit, in a diff a reviewer can read.Against #503's acceptance list, each verified by planting the mutation into the real workflows, running the suite, and restoring the tree:
ci.yml gates:@master→@stableci.yml gates:toolchain: 1.88.0channel=stablein the readerbridge-windows-packaged: install moved belowcargo buildSwatinem/rust-cachemoved aboveInstall Rustuses:RUSTUP_TOOLCHAINoverridesrustup_toolchain,container.envrustup/cargo +toolchainselectorsrustup override set stablein a steprust-toolchainadded andgit added understeel-detailer-lookup, agreeing with the pin.claude/worktrees/probe/cli/rust-toolchain.toml(confirmed git-ignored)codecov/codecov-action@v4added togatesThe last row is the part that keeps the second row from the bottom honest. Naming
Swatinem/rust-cachecloses today's instance; requiring everyuses:to be classified closes the class, because the next Cargo-running action cannot be silently assumed inert. The repo uses nine distinct actions, so the three lists are 9 lines.Bounded on purpose — what it does not do
It proves a job installs the pin, never that the compiler can build the crate. Its reach stops at
.github/workflows/, so Cargo invoked by a script a workflow calls is outside it. It knowsdtolnay/rust-toolchainspecifically. All three limits are written into the file's own doc comment.Notes for reviewers
ci.ymlcarries two changes the contract requires, and they are the whole workflow diff:shell: bashon the two pin steps that lacked it (release.yml's andbridge-windows-packaged's already had it), and the POSIX-sed note moved from inside the script to a YAML comment above the step — otherwise that note would have to be repeated verbatim in four places to keep the four steps identical.shell: bashaddspipefail; the same script already runs under it in two jobs across all three runners, andsedreads a 15-line file, sohead -1cannot close the pipe early.actions/checkout,dtolnay/rust-toolchainandSwatinem/rust-cache— an empty finding list otherwise looks identical to a scan that read no files.cargo fmt --all -- --checkclean,cargo clippy --all-targets --locked -- -D warningsclean,cargo test --lockedgreen,scripts/no-claude-coauthor-trailers.pyclean over the branch range. No lint weakened, no#[allow], no test skipped.🤖 Generated with Claude Code
https://claude.ai/code/session_01AFUKh3c4ye7aCkbuq1EeUT
Generated by Claude Code