Skip to content

ci: install pre-commit and the pinned rust toolchain into the tend sandbox - #6334

Merged
max-sixty merged 2 commits into
mainfrom
daily/review-runs-35321455559
Sep 18, 2026
Merged

max-sixty merged 2 commits into
mainfrom
daily/review-runs-35321455559

Conversation

@prql-bot

@prql-bot prql-bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

task lint — the lint command CLAUDE.md documents — is pre-commit run --all-files followed by clippy, and it doesn't run in a tend session today. This adds the two sandbox_setup: entries that make it run, beside the cargo-insta/cargo-nextest entries from #6251 that fixed the same shape of gap for the test loop.

pre-commit is absent from the runner's PATH and the sandbox's alike, so the gate aborts on its first command in every session. uv tool install pre-commit takes 0.46 s — uv is already provisioned, so nothing is built — and puts the binary in ~/.local/bin, first on the sandbox PATH.

The pinned Rust toolchain isn't installed either, and that breaks the lint gate a second time, less obviously. rust-toolchain.toml pins 1.97.1; the sandbox home ships only stable. The fmt hook in r0x0d/pre-commit-rust leaves pass_filenames at its default, so pre-commit splits this repo's 114 .rs files across the runner's cores and launches one cargo fmt per chunk — each racing the same rustup auto-install into the shared ~/.rustup/downloads and clobbering the others' .partial files:

Check for formatting with cargo fmt......................................Failed
error: component download failed for cargo-x86_64-unknown-linux-gnu: could not rename 'downloaded' file from '.../<hash>.partial' to '.../<hash>': No such file or directory (os error 2)

The hook fails, pre-commit exits 1, and task lint never reaches clippy — so installing pre-commit alone would have left the gate red for a reason that has nothing to do with the repo's formatting. cargo --version installs the pin once, serially, in 11 s, and is a cost every session that touches Rust pays anyway.

With both entries, pre-commit run --all-files passes all 14 hooks — typos, prettier, ruff, ruff-format, clang-format, cargo fmt, actionlint, lychee, no-dbg and the rest — exits 0, and leaves the working tree byte-identical. The hook environments are deliberately not built in sandbox_setup:; pre-commit builds them on first use, 233.7 s cold for this repo's hooks, so only a session that actually lints pays that and a session that never lints pays a second.

The cost of the gap is concrete rather than theoretical: the 2026-09-18 nightly opened #6333 and closed with "task lint couldn't run: pre-commit isn't on PATH in the sandbox. I ran the prettier and rustfmt hooks directly (both clean), but the other hooks — typos, ruff — went unrun". That is the documented gate going unrun on a PR the bot authored, with pre-commit.ci on the PR as the only thing that would have caught it.

The two edited-file groups are one change: sandbox_setup: is inlined into each generated workflow, so .config/tend.yaml and the eight tend-*.yaml files move together. The workflow diff is the three added lines and nothing else — no version stamp moved, since uv tool run tend@latest init regenerated at the 0.2.10 already pinned by #6330.

The config's own comment block said cargo llvm-cov and pre-commit are both off the sandbox PATH; it now names only cargo llvm-cov, since a stale comment there is the durable prose a later session reads as ground truth.

No CHANGELOG.md entry: this changes the bot's CI environment, not anything a PRQL user sees.

Why a config entry rather than a note in the skill

#6235 proposed recording the equivalent gap for cargo-insta/cargo-nextest as guidance in the running-tend overlay, and was closed with "we need to fix the underlying problem, am working on it" — which became #6251, the sandbox_setup: entries this one sits beside. The same reasoning applies here, and running-in-ci says the same thing from the other direction: "A missing gate tool is reported, not worked around: propose the entry and say the gate went unrun rather than substituting a weaker command."

cargo llvm-cov — the other tool missing from task prqlc:pull-request — is deliberately not included, keeping this PR to one concern. The 2026-09-08 nightly hit its absence and chose not to propose an entry, on the grounds that .config/tend.yaml already documents it and the uninstrumented suite covers the same tests. Whether coverage is worth provisioning is a separate question from whether the lint gate should run at all.

Verification on this branch, beyond the hooks above: uv tool run tend@latest check reports 9 PASS and the standing SKIP on immutable-releases (needs repo admin), and uv tool run tend@latest init regenerates the eight workflow files byte-identically, so config and workflows are in sync.

Two open questions this doesn't settle

The fmt race has a deeper fix. require_serial: true on the fmt hook in .pre-commit-config.yaml would remove it for any contributor with a cold rustup, not just the sandbox — a fresh clone hits the same race, since rust-toolchain.toml pins a version a new contributor is unlikely to have. It changes lint behaviour for everyone though, so it's left as a maintainer's call rather than folded into a sandbox-setup PR.

The first task lint of a session runs past the agent's default command timeout. ~/.cache/pre-commit lives in the sandbox home, so the 233.7 s cold build is paid by every session that lints, not just the first — against a 120 s default. A session that doesn't extend the timeout sees the command killed mid-build and can't distinguish that from a broken gate. The comment in .config/tend.yaml is the only record of the cost and nothing loads it at session start, so a line in running-tend is worth a separate follow-up.

Evidence trail

Recorded in the review-runs tracking issue #6266:

  • 2026-08-29 (33241784791, addendum): command -v answers MISSING for both cargo-llvm-cov and pre-commit, and neither is a blocked stub — they are absent from the runner's ~/.cargo/bin too, so sandbox_setup: cannot recover them without also installing them.
  • 2026-09-18 (35316063538): the nightly reports task lint unrunnable and names typos and ruff as ungated.
  • This run (35321455559): install, full-hook run, and the fmt race measured directly, numbers above.

Gate assessment: High confidence, structural (no decision point — neither pre-commit nor the pinned toolchain is present in any session), occurrence 3; change type is a targeted fix — three lines of config plus its regen. Both gates pass.

`task lint` is `pre-commit run --all-files` followed by clippy, and
`pre-commit` is on neither the runner's PATH nor the sandbox's, so the
lint gate `CLAUDE.md` documents aborts on its first command in every
tend session.

`uv tool install` puts it in `~/.local/bin`, first on the sandbox PATH,
alongside the `cargo-insta`/`cargo-nextest` entries added in #6251.
@prql-bot prql-bot added the review-runs Findings from tend-review-runs daily analysis label Sep 18, 2026

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-commit lands on the sandbox PATH as intended — 0.46 s, ~/.local/bin/pre-commit — and uvx tend init on this branch reproduces the eight workflow files byte-identically. But running the gate end to end shows it still doesn't pass in a fresh sandbox: pre-commit run --all-files exits 1, so task lint never reaches clippy.

The hook that fails is fmt, and nothing is misformatted:

Check for formatting with cargo fmt......................................Failed
- hook id: fmt
- exit code: 1
error: component download failed for cargo-x86_64-unknown-linux-gnu: could not rename 'downloaded' file from '/home/tend-sandbox/.rustup/downloads/<hash>.partial' to '/home/tend-sandbox/.rustup/downloads/<hash>': No such file or directory (os error 2)

rust-toolchain.toml pins 1.97.1 and the sandbox home ships only stable, so the first cargo call triggers a rustup auto-install. The fmt hook in r0x0d/pre-commit-rust leaves pass_filenames at its default, so pre-commit splits this repo's 114 .rs files across the runner's 4 cores and launches one cargo fmt per chunk — each racing the same auto-install into the shared ~/.rustup/downloads and clobbering the others' .partial files. Install the toolchain once, serially (cargo --version, 11 s), and the same hook passes.

Pushing a cargo --version entry to sandbox_setup: for that. It's the fix that stays inside this PR's subject, and it's close to free in aggregate: any session that touches Rust pays that auto-install anyway, just later and mid-task. The deeper fix is require_serial: true on the fmt hook in .pre-commit-config.yaml, which would remove the race for any contributor with a cold rustup rather than only in the sandbox — but that changes lint behaviour for everyone, so it's a maintainer's call rather than something to fold into a sandbox-setup PR.

Second, smaller point: the cold hook build measured 233.7 s here against the description's 212.7 s, and because ~/.cache/pre-commit lives in the sandbox home it's paid by every session that lints, not just the first. Both numbers are past the agent's 120 s default command timeout — the fmt hook on its own ran past it during this review — so a session running task lint has to extend the timeout or it sees the command killed mid-build, with no way to distinguish that from a broken gate. The .config/tend.yaml comment is the only place that cost is recorded and nothing loads it at session start, so it's worth a line in running-tend as a separate follow-up.

pre-commit's `fmt` hook passes filenames, so it starts one `cargo fmt` per
core. The sandbox home ships only `stable` while rust-toolchain.toml pins
1.97.1, so each of those races the same rustup auto-install and they clobber
each other's partial downloads — the hook fails on a component-download error
and `task lint` never reaches clippy. Installing the toolchain once, serially,
costs ~11s.
@prql-bot prql-bot changed the title ci: install pre-commit into the tend sandbox ci: install pre-commit and the pinned rust toolchain into the tend sandbox Sep 18, 2026
@max-sixty
max-sixty merged commit 4a8cd3b into main Sep 18, 2026
43 of 44 checks passed
@max-sixty
max-sixty deleted the daily/review-runs-35321455559 branch September 18, 2026 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-runs Findings from tend-review-runs daily analysis

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants