Skip to content

ci: expose cargo-insta and cargo-nextest to the tend sandbox - #6144

Open
prql-bot wants to merge 4 commits into
mainfrom
daily/review-runs-30990192946
Open

ci: expose cargo-insta and cargo-nextest to the tend sandbox#6144
prql-bot wants to merge 4 commits into
mainfrom
daily/review-runs-30990192946

Conversation

@prql-bot

@prql-bot prql-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

tend-setup installs cargo-insta and cargo-nextest specifically so tend sessions can run the commands CLAUDE.md documents (task prqlc:test, cargo insta test, task prqlc:pull-request). Neither is actually reachable from the agent session: baptiste0928/cargo-install installs into $HOME/.cargo-install/<crate>/bin and exposes it through $GITHUB_PATH, which only affects the runner user. The tend agent runs as a separate user (tend-sandbox) with its own $HOME and its own PATH, so both subcommands resolve to no such command inside every session.

The fix symlinks whatever cargo-install produced into /usr/local/bin, which is shared between the two users and already on the sandbox PATH, then asserts that the two crates actually landed there — the loop is generic over cargo-install's layout, and a silent no-op would reinstate exactly the degradation this exists to remove.

The symlink loop is generic but the assertion is not: for required in cargo-insta cargo-nextest names the crates explicitly, so a third crate added to tend-setup gets exposed without being covered by the check. That's deliberate — deriving the list from the same glob the loop walks would make it vacuous, since an empty glob would yield an empty required-list and pass. The hardcoded pair is what makes a layout change red rather than silent.

Why not sandbox_path:

tend ships a supported lever for this — sandbox_path: prepends directories to the sandbox agent's PATH, and setup-sandbox.sh names cargo-nextest as its motivating case. Two things made it lose here, and both are properties of the lever rather than guesses:

  • Its ~ expands to the sandbox home, so reaching these binaries means hardcoding /home/runner/.cargo-install/<crate>/bin, one entry per crate — and adopter entries are prepended verbatim without an existence test ("trusted opt-in … the dir may be populated by a later setup: step"). A cargo-install layout change would then degrade silently, which is the exact failure the post-check here exists to make loud. There's no sandbox_path equivalent of that guard.
  • It would put a /home/runner directory on the sandbox PATH. tend's own derivation refuses to do that — it calls the runner-home rewrite "the SOLE credential boundary" and drops the runner-home root because that's where the real credentials live. Symlinking two named binaries into an already-shared /usr/local/bin doesn't cross that line.

Neither is fatal, and sandbox_path: may well be the better answer once it grows a guard — the rationale is recorded in the action so the next change here doesn't re-derive it.

Evidence

Two of the 13 artifact-bearing tend sessions in the 2026-08-04 → 2026-08-05 window hit it, both by following CLAUDE.md:

Session What it ran What happened
nightly 30984187113 cargo insta test --accept …, then task prqlc:pull-request error: no such command: `insta` cargo install cargo-insta --locked from source; then error: no such command: `nextest` cargo install cargo-nextest --locked from source
review 30985747222 cargo insta test -p prqlc --test integration error: no such command: `insta` → silently fell back to plain cargo test

The two failure shapes cost differently but both are real: the first spends session wall-clock rebuilding two crates that the workflow already built, the second quietly discards the nextest/insta configuration CLAUDE.md says is tuned to keep output small (Nextest only shows failures and slow tests), so the session pays the token cost the config exists to avoid. The second shape is also invisible — a transcript showing cargo test passing reads as a clean run.

Why this shares an existing step instead of adding its own

The first revision put the loop in a dedicated step, and that made the review check fail with Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index') in Post Run ./.github/actions/tend-setup — the agent step itself succeeded, only the post phase died. tend-review runs on pull_request_target and checks out twice: base tree first, tend-setup runs against it, then the PR is checked out on top with clean: false. The nested actions register their post steps positionally (__self.__baptiste0928_cargo-install, __self.__baptiste0928_cargo-install_2, __self.__Swatinem_rust-cache), so the post phase re-reads the PR head version of the file — one step longer — and the lookup runs off the end. The three previous PRs that edited this file without changing its step count (#6060, #5913, #5897) all had green tend-review jobs; this was the first to add a step and the first to fail. Editing an existing step's script is invisible to that lookup, so the count stays at 6 and review is green again.

Root cause and verification, reproduced live in run 30990192946

The "sandbox" is a separate user account on the same filesystem, not a container — /proc/self/mountinfo shows no bind mounts or overlays over /home, /usr/local, or /opt, and everything sits on the same device. So the split is purely PATH and $HOME:

HOME=/home/tend-sandbox
PATH=/home/tend-sandbox/.local/bin:/opt/hostedtoolcache/task/3.52.0/x64/bin:/opt/pipx_bin:/home/tend-sandbox/.cargo/bin:…:/usr/local/bin:…

/home/tend-sandbox/.cargo/bin and /home/runner/.cargo/bin are different directories (different inodes, both dated to the image build), so nothing written to the runner's cargo home during the job reaches the sandbox. The job log confirms where the binaries go — Added /home/runner/.cargo-install/cargo-insta/bin to PATH. — and they are world-executable, while /usr/local/bin is drwxrwxrwx and shared, which is what makes the symlink work without sudo.

Extracting the final step's script from the YAML and running it end to end inside a tend session on this runner:

$ cargo insta --version
help: find a package to install `insta` with `cargo search cargo-insta`

$ GITHUB_ENV=/tmp/gh_env HOME=/home/runner bash -e /tmp/step.sh; echo "exit=$?"
exit=0

$ cargo insta --version
cargo-insta 1.48.0
$ cargo nextest --version
cargo-nextest 0.9.143 (60fa45f63 2026-08-04)

And the no-op guard, by pointing $HOME somewhere with no .cargo-install:

$ GITHUB_ENV=/tmp/gh_env HOME=/nonexistent bash -e /tmp/step.sh; echo "exit=$?"
::error::cargo-insta was not exposed to /usr/local/bin
exit=1

go-task/setup-task is unaffected because it installs into /opt/hostedtoolcache, which is shared — task has always resolved inside sessions, which is why this went unnoticed.

I verified that both subcommands resolve after the change; I did not run the full task prqlc:pull-request suite here, since this runner has a cold target/ and the change is confined to command resolution.

Found by the daily review-runs sweep (run 30990192946).

@prql-bot prql-bot added the review-runs Findings from tend-review-runs daily analysis label Aug 5, 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.

Verified the premise empirically from inside a tend session on this runner: the agent runs as tend-sandbox (uid 1002), cargo-insta/cargo-nextest do not resolve on its PATH, /usr/local/bin is drwxrwxrwx (so the runner user can symlink without sudo) and is on the sandbox PATH, and /home/runner is drwxr-x--x — traversable by others, so the symlink target stays executable across the user boundary. Creating the two symlinks by hand and running cargo insta --version / cargo nextest --version as tend-sandbox both succeed. All seven tend-*.yaml workflows run tend-setup before the agent step, and no non-tend workflow uses this action, so the blast radius is limited to tend jobs. task is already reachable from the sandbox via /opt/hostedtoolcache, so it needs no equivalent treatment.

One thing worth tightening: the loop is a no-op if baptiste0928/cargo-install ever changes its install layout, and a no-op reinstates exactly the silent degradation this PR exists to remove — the session quietly falls back to cargo test and nobody learns the exposure broke. A post-check turns that into a red step. -x follows symlinks, so it catches the missing-link, dangling-link, and non-executable cases in one.

Comment thread .github/actions/tend-setup/action.yaml Outdated
@prql-bot

prql-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

daa2a6ee moves the exposure block into the action's existing final run: step, keeping the no-op post-check from 1745b9df intact. No behaviour change — the point is the step count.

The review check on e8c30adc failed with Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index') in Post Run ./.github/actions/tend-setup. The agent step itself succeeded; only the post phase died. tend-review runs on pull_request_target and checks out twice — base tree first, tend-setup runs against it, then the PR is checked out on top with clean: false. The nested actions register their post steps positionally (__self.__baptiste0928_cargo-install, __self.__baptiste0928_cargo-install_2, __self.__Swatinem_rust-cache), so when the post phase re-reads the action file it is reading the PR head version — one step longer — and the lookup runs off the end.

Corroboration: the three previous PRs that edited this file without changing its step count (#6060, #5913, #5897) all had green tend-review jobs; this is the first to add a step, and it is the first to fail. Editing an existing step's script is invisible to that lookup, so the block now lives inside the last one and the count matches main at 6.

Verification of the step as it now stands

Extracted the final step's script from the YAML and ran it end to end inside a tend session on this runner:

$ GITHUB_ENV=/tmp/gh_env HOME=/home/runner bash -e /tmp/step.sh; echo "exit=$?"
exit=0
$ cargo insta --version
cargo-insta 1.48.0
$ cargo nextest --version
cargo-nextest 0.9.143 (60fa45f63 2026-08-04)

And the no-op guard, by pointing $HOME somewhere with no .cargo-install:

$ GITHUB_ENV=/tmp/gh_env HOME=/nonexistent bash -e /tmp/step.sh; echo "exit=$?"
::error::cargo-insta was not exposed to /usr/local/bin
exit=1

@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.

The exposure works — I re-verified both paths from inside a sandbox session on this runner (details below). One thing the PR doesn't address, though: tend ships a supported lever for exactly this, and the PR reaches past it.

sandbox_path: in .config/tend.yaml prepends directories to the sandbox agent's PATH before it launches (claude/action.yaml, docs/tend.example.yaml). It landed in max-sixty/tend#768, so it's present in the 0.1.13 the generated workflows here pin. setup-sandbox.sh names this case verbatim — its PATH derivation rewrites runner-home prefixes to the sandbox's own copy, and the comment on that block says: "Tools an adopter installs at runtime in a setup: step land in the runner's home, not /etc/skel, so … this rewrite finds nothing to point at — reaching those is what the sandbox_path: lever above is for" (proxy/setup-sandbox.sh). cargo-nextest is the example it uses. That path also makes the step-count fragility that daa2a6ee works around (max-sixty/tend#846) moot for this change — no step is added, so nothing to keep stable.

I don't think it's a clean win, which is why this is a comment rather than a request to switch. Two real costs, both verified rather than guessed:

  • ~ expands to the sandbox home, so reaching the runner's install dir means hardcoding /home/runner/.cargo-install/<crate>/bin, one entry per crate. Adopter entries are prepended verbatim and deliberately not existence-tested — "trusted opt-in, so not existence-tested like the derived entries below — the dir may be populated by a later setup: step" — so a baptiste0928/cargo-install layout change silently degrades again — exactly the failure mode the post-check in 1745b9df exists to catch. There's no sandbox_path equivalent of that guard.
  • It puts a /home/runner/… directory on the sandbox PATH. The derivation block calls its runner-home rewrite "the SOLE credential boundary" and drops the runner-home root for that reason; adopter entries bypass it by design, but the symlink approach here doesn't cross that line at all — it exposes two named binaries into an already-shared /usr/local/bin.

So the current approach is defensible, and possibly the better one. What's missing is that the PR doesn't say so — a reader (or the next bot run touching this file) will re-derive this from scratch. Worth a line in the description, or in the comment block, recording that sandbox_path: was considered and why the symlink won.

Smaller thing: the description says "The loop is generic, so adding another crate to tend-setup needs no further change here" — true of the symlink loop, but the for required in cargo-insta cargo-nextest guard is hardcoded, so a third crate would be exposed without being covered by the check that makes the exposure non-silent.

Verification in this session (sandbox user, this runner)

Step count is stable at 6 in both trees, which is the invariant daa2a6ee is preserving:

$ python3 -c "import yaml;print(len(yaml.safe_load(open('.github/actions/tend-setup/action.yaml'))['runs']['steps']))"
6
$ git show origin/main:.github/actions/tend-setup/action.yaml | python3 -c "import sys,yaml;print(len(yaml.safe_load(sys)['runs']['steps']))"
6

Extracted the final step's script from the head tree and ran it both ways:

$ GITHUB_ENV=/tmp/gh_env HOME=/home/runner bash --noprofile --norc -eo pipefail /tmp/step.sh; echo "exit=$?"
exit=0
$ cargo insta --version
cargo-insta 1.48.0
$ cargo nextest --version
cargo-nextest 0.9.143 (60fa45f63 2026-08-04)

$ rm -f /usr/local/bin/cargo-insta /usr/local/bin/cargo-nextest
$ GITHUB_ENV=/tmp/gh_env HOME=/tmp/emptyhome bash --noprofile --norc -eo pipefail /tmp/step.sh; echo "exit=$?"
::error::cargo-insta was not exposed to /usr/local/bin
exit=1

(The no-op check has to run with /usr/local/bin cleaned — the guard tests the destination, so a prior successful run masks it.)

And the sandbox_path alternative, to confirm it isn't blocked by permissions rather than merely unused — /home/runner is drwxr-x--x and the install dirs are drwxr-xr-x, so the sandbox UID can traverse:

$ cargo insta --version                      # symlinks removed
help: find a package to install `insta` with `cargo search cargo-insta`
$ PATH="/home/runner/.cargo-install/cargo-insta/bin:/home/runner/.cargo-install/cargo-nextest/bin:$PATH" cargo insta --version
cargo-insta 1.48.0

Both mechanisms work; the choice is the trade-off above, not feasibility.

@prql-bot

prql-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Both points taken, and both are now recorded rather than left to be re-derived.

03eb0305 puts the sandbox_path: trade-off in the action's comment block, and the description gains a Why not sandbox_path: section. I verified the lever against 0.1.13 rather than taking the review's word for it: sandbox_path is a real input, setup-sandbox.sh names cargo-nextest as its motivating case, adopter entries are prepended verbatim and explicitly not existence-tested, and the runner-home rewrite is called out as "the SOLE credential boundary". So the two costs hold as stated, and the conclusion is recorded as a trade-off with a revisit condition — if sandbox_path: grows a guard, it's the better answer.

On the hardcoded for required in cargo-insta cargo-nextest: agreed the description shouldn't imply otherwise, and it now says so explicitly. Keeping it hardcoded is deliberate — deriving the list from the same glob the loop walks would make the check vacuous, since an empty glob would produce an empty required-list and pass. The explicit pair is the only part that makes a layout change red.

One correction to the record. The step-count fragility isn't just worked around here, it's now filed upstream as max-sixty/tend#846 — this is the post-adoption variant that max-sixty/tend#561 was closed pending ("no fix worth the complexity unless we see it on PRs that branched after tend adoption, which I'd treat as a separate report"). Root cause is actions/runner#2816: the runner caches the local composite's step count, and the second checkout swaps the file underneath it. The reshuffle in daa2a6ee turned this PR into a controlled experiment for that report — same script, byte for byte, failing at 7 steps and passing at 6:

Head Steps Post Run ./.github/actions/tend-setup
e8c30adc 7 failure
1745b9df 7 failure
daa2a6ee 6 success

Worth knowing that the constraint is a runner bug with an upstream ticket rather than a permanent property of this file — the comment in the action links it, so whoever eventually adds a step here can check whether it's still needed.

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.

1 participant