Skip to content

ci: install cargo-insta and cargo-nextest into the tend sandbox - #6251

Open
prql-bot wants to merge 2 commits into
mainfrom
daily/review-runs-33241784791
Open

ci: install cargo-insta and cargo-nextest into the tend sandbox#6251
prql-bot wants to merge 2 commits into
mainfrom
daily/review-runs-33241784791

Conversation

@prql-bot

@prql-botprql-bot commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

.github/actions/tend-setup installed cargo-insta and cargo-nextest via baptiste0928/cargo-install, which puts them under the runner user's home — which tend deliberately keeps off the sandbox PATH, since that home can hold credentials. Since tend 0.1.20 the agent gets a stub that exits 127 and names the remedy: reinstall under the sandbox user's home via sandbox_setup:. This adds that block and drops the two now-dead cargo-install steps, so the tools move rather than being installed twice; both crates ship prebuilt binaries, so the pair downloads in ~2s rather than building.

This restores task prqlc:test, the inner loop CLAUDE.md documents. It does not restore task prqlc:pull-request: that reaches cargo llvm-cov (third command of prqlc:test-all) and pre-commit (first command of :lint), and unlike the two crates here neither is a blocked stub — both are absent from the runner's PATH too.

Verified from inside a tend session on this runner: starting from the blocked-stub state, the exact command block resolves cargo-nextest 0.9.143 and cargo-insta 1.48.0 in ~2s, both to ~/.local/bin rather than the ~/.tend-blocked/bin stub, with no dependence on the runner-home copies. task prqlc:test then runs to completion — cargo insta test --accept --test-runner=nextest over 704 tests, all passed, "no snapshots to review", doctests and clippy --fix clean, exit 0, working tree unchanged.

Two earlier attempts at this were closed, so please close this one too if you'd rather own it.#6144 symlinked into /usr/local/bin from the composite action — that reached past tend's supported lever. #6235 documented an export PATH workaround in the running-tend skill and was closed with "we need to fix the underlying problem, am working on it". This is neither: it is the residual repo-side config that the underlying fix expects. 0.1.20 landed here in #6242 and did fix the underlying problem — cargo, rustc and task all reach the sandbox now, where none of them did before. What is left is the class tend refuses on purpose: tools installed under the runner's home. sandbox_setup: is the first-class knob for exactly that case, and it is what the stub asks for by name.

Why the `cargo-install` steps go away

tend-setup is used only by the eight tend-*.yaml workflows, and no runner-side step in any of them invokes cargo or task — the agent is the only consumer of both crates. Keeping the cargo-install steps after this change would install them into a location nothing reads, and rebuild both from source on a cache miss in every tend job (Swatinem/rust-cache runs with cache-bin: "false"). The set_version.sh and cache steps around them are still doing real work and stay. baptiste0928/cargo-install remains in use across tests.yaml, release.yaml and others, so Dependabot still tracks it.

What the gap cost in last night's runs

Two hits in the nightly session (33238864253):

  1. Regenerating a book snapshot for #6248, cargo insta test --accept -p mdbook-prql --test documentation was unavailable, so the session fell back to INSTA_UPDATE=always INSTA_FORCE_UPDATE=1 cargo test. That is broader than --accept: it also rewrote documentation__book__project__target__version__0.snap and one other snapshot the change did not touch, which the session had to git checkout -- back by hand. It caught it; a session that did not would have shipped unrelated snapshot churn in a docs PR.
  2. The session then declined to file anything about the gap, correctly, on the strength of docs: record how to run the documented inner loop from a tend session #6235's close.

task prqlc:pull-request also aborted in that session, so #6248 and #6250 were opened without the pre-return gate having run — but as noted above, this PR does not fix that one.

This has been recorded in #6123 and its predecessors 15+ times since 2026-08-05, across roughly twelve review-runs windows.

Why ~/.local/bin and not the cargo default

The sandbox PATH is, in order: ~/.local/bin, ~/.tend-blocked/bin, /opt/hostedtoolcache/task/3.53.1/x64/bin, /opt/pipx_bin, ~/.cargo/bin, then system paths. ~/.local/bin is first, ahead of both ~/.tend-blocked/bin (where the 127 stubs live) and ~/.cargo/bin. Hence CARGO_HOME=~/.local on the cargo-dist installer, which places the binary at $CARGO_HOME/bin. The rationale is in the config comment at .config/tend.yaml so it is not re-derived.

Each command ends by running the tool, per tend's own guidance, so a broken install fails before the agent starts rather than silently reinstating today's degradation. Versions are unpinned; see the review thread for the one caveat there (a transient blip at either download URL fails the job for all eight workflows, and a bare curl --retry is the wrong remedy on a pipe).

`.github/actions/tend-setup` installs both under the runner user's home,
which tend keeps off the sandbox PATH; the agent gets a stub that exits
127. Reinstall them under the sandbox user's home with `sandbox_setup:`,
which is the remedy that stub names.
Both ship prebuilt binaries, so the pair downloads in ~1.2s. Verified
from inside a tend session: from the blocked-stub state the block
resolves both tools, and `task prqlc:test` then runs to completion —
704 tests passed, no snapshots to review, exit 0.
@prql-botprql-bot added the review-runs Findings from tend-review-runs daily analysis label Aug 29, 2026

@prql-botprql-bot left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Self-review — I authored this, so treating it with extra skepticism per the review skill. I re-ran the exact sandbox_setup: block from inside a live tend sandbox on this runner: it resolves cargo-nextest 0.9.143 and cargo-insta 1.48.0 in 1.4s, and both then resolve to ~/.local/bin rather than the ~/.tend-blocked/bin stub. I also confirmed the rendered block in all eight tend-*.yaml files is byte-identical to the joined .config/tend.yaml list, so regeneration is faithful. Three things I'd still change or flag.

The pull-request claim doesn't hold — inline suggestion below. task prqlc:pull-request runs prqlc:test-all, whose third command is cargo llvm-cov, then :lint, whose first command is pre-commit run --all-files. Neither cargo-llvm-cov nor pre-commit resolves in the sandbox — and unlike the two crates here, they aren't blocked stubs, they're absent from the runner's PATH too, so sandbox_setup: alone can't recover them. So the pre-return gate this PR's own evidence section says #6248/#6250 shipped without will still abort, just later and at a different command. That sentence is a durable comment a future session reads as ground truth, so it's worth narrowing rather than leaving it to be re-derived.

tend-setup now installs both crates for nobody..github/actions/tend-setup/action.yaml still runs baptiste0928/cargo-install for cargo-insta and cargo-nextest, into the runner home. That action is used only by the eight tend workflows, and no runner-side step in any of them invokes cargo or task — the agent is the only consumer, and the runner home is exactly what the sandbox PATH excludes by design. After this PR those two steps install into a location nothing reads, and on a cache miss they build both crates from source in every tend job. Dropping them would make this change net-neutral in machinery instead of additive; the Swatinem/rust-cache and set_version.sh steps around them are still doing real work and should stay.

Minor: both installers are unpinned latest fetches behind bash -eo pipefail, so a transient blip at get.nexte.st or an upstream asset rename fails the whole job before the agent starts — on all eight workflows, including tend-notifications and tend-review-runs, which never touch Rust. Failing loudly is the right call over silently reinstating today's degradation, and unpinned does keep this in step with tend-setup's unpinned cargo-install; I mention it only because the blast radius is every tend session rather than just the ones that run tests. curl --retry 3 --retry-all-errors would cover the transient case without adding a version to maintain.

Comment thread.config/tend.yaml Outdated
Addresses self-review on #6251.
`tend-setup` installed cargo-insta and cargo-nextest with `cargo-install`
into the runner user's home. That action is used only by the eight tend
workflows, and no runner-side step in any of them invokes `cargo` or
`task` — the agent is the only consumer, and the runner home is exactly
what the sandbox PATH excludes by design. Now that `sandbox_setup:`
installs both under the sandbox home, those two steps install into a
location nothing reads, and build both crates from source on a cache
miss in every tend job.
Also narrows the config comment: `task prqlc:pull-request` reaches
`cargo llvm-cov` (via `prqlc:test-all`) and `pre-commit` (via `:lint`),
neither of which is on the sandbox PATH at all, so this change does not
restore that gate — only `task prqlc:test`.
@prql-bot

Copy link
Copy Markdown
CollaboratorAuthor

Actioned the review as author. Two of the three are in 0369e91; the third I'm reporting rather than fixing, because the suggested remedy turns out to be unsafe.

1. pull-request claim — applied verbatim, replied inline. Verified against both Taskfiles and the live sandbox: cargo-llvm-cov and pre-commit are absent from the sandbox entirely, not stubbed.

2. tend-setup installing for nobody — agreed, and dropped both cargo-install steps. Verified the premise before removing: grep -rl tend-setup .github/ returns only the eight tend-*.yaml workflows (plus dependabot.yaml), and the only cargo/task invocations anywhere in those eight are inside the sandbox_setup: block this PR adds. I then re-ran that block in this session and both tools resolved to ~/.local/bin in 2.2s with the runner-home copies unused. baptiste0928/cargo-install is still used by tests.yaml, release.yaml, test-rust.yaml and others, so Dependabot keeps tracking it. This does make the PR net-neutral in machinery — the tools move rather than being installed in two places.

3. Unpinned installers — not changed, and curl --retry 3 --retry-all-errors would be the wrong fix here. curl restarts a retried transfer from byte 0, but bytes already written to a pipe can't be rewound, so a mid-stream failure feeds the consumer the partial body followed by the whole body. Both commands pipe straight into a consumer (tar zxf -, sh), so the retry converts a clean failure into corrupt input.

Repro for the retry hazard

Server returns Content-Length: 20, writes 5 bytes, closes; second request serves the full body:

$ curl -LsSf --retry 3 --retry-all-errors http://127.0.0.1:8731/x | od -c
curl: (18) transfer closed with 15 bytes remaining to read
0000000 A A A A A B B B B B B B B B B B
0000020 B B B B B B B B B

25 bytes out of a 20-byte body. curl exits 0 — it did succeed, on the retry. tar would almost certainly reject that, but the | sh case is a shell script with a truncated prefix glued to a complete copy of itself.

The safe form is a download-to-tempfile then extract/run, which is four lines rather than one flag. Given no observed failure at either URL and a blast radius that is "job fails loudly before the agent starts" rather than a wrong outward action, I left it — the note is here so the next session doesn't reach for the one-flag version.

Nothing else from the review is left open. I'm polling the checks on 0369e91 and will follow up here only if they go red.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-runsFindings from tend-review-runs daily analysis

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@prql-bot