docs: record how to run the documented inner loop from a tend session - #6235
docs: record how to run the documented inner loop from a tend session#6235prql-bot wants to merge 6 commits into
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Self-authored, so this is a COMMENT rather than an approval.
The diagnosis checks out — I reproduced every mechanical claim from inside this session: cargo insta and cargo nextest both answer no such command, whoami is tend-sandbox, .github/actions/tend-setup installs both via baptiste0928/cargo-install, and prqlc:test and prqlc:pull-request (through test-all) both route through cargo insta test --accept --dnd … --test-runner=nextest.
The remedy is where this comes apart. The load-bearing sentence — "the agent runs as a separate tend-sandbox user whose PATH cannot carry a runner-home path" — isn't true. tend's derived PATH omits runner-home, but a session can prepend one, and the binaries are world-executable. I ran the documented inner loop that way in this session and it worked end to end:
$ export PATH="/home/runner/.cargo-install/cargo-insta/bin:/home/runner/.cargo-install/cargo-nextest/bin:$PATH"
$ cargo insta --version
cargo-insta 1.48.0
$ cargo insta test --test-runner=nextest -p prqlc-parser --lib
Summary [ 0.841s] 101 tests run: 101 passed, 0 skipped
info: no snapshots to review
This doesn't reach into the credential-bearing part of the runner home either: /home/runner is drwxr-x--x, so tend-sandbox can traverse it but not enumerate it, and the two bin directories are 755. task is already on the sandbox PATH at /opt/hostedtoolcache/task/3.53.1/x64/bin/task, and every tend workflow runs tend-setup, so this holds in all of them — not just review sessions.
That inverts three of the section's conclusions: plain cargo test becomes the fallback rather than the recommendation, "inline snapshots can't be auto-accepted here" is false, and the INSTA_UPDATE=always warning — accurate on its own terms, and worth keeping — stops being the thing a session has to lean on. As written, the section would steer future sessions into hand-transcribing snapshots and reporting a weaker verification than was one export away, which is the opposite of what it's for.
One thing worth leaving open rather than closing off: the description rejects the tend sandbox_setup: lever because it would mean cargo install on every sandbox boot. But sandbox_path: on its own needs no install — the binaries are already on disk from tend-setup, so it's just the permanent form of the export above, with no build time and no proxy-allowlist exposure. That's a materially different trade from the one the description weighs, so I'd put it in front of the maintainer as an option rather than under "don't re-litigate this from a session". (I haven't verified whether tend's sandbox_path: accepts a runner-home path — that's the one thing that would need checking before proposing it.)
Caveat on the suggestion below: shell state doesn't persist between tool calls, so the export has to be in the same invocation as the command rather than run once per session.
prql-bot
left a comment
There was a problem hiding this comment.
Self-authored, so this is a COMMENT rather than an approval.
The rewrite lands the main correction, and I re-verified the mechanics from this session rather than taking them on trust: /home/runner is drwxr-x--x, both bin directories are 755, and with the prepend cargo insta --version returns 1.48.0. The sandbox_path: finding is exact too — proxy/setup-sandbox.sh matches "${runner_home}" | "${runner_home}"/*) and exits 1 with the error string quoted in the section, with the comment above it giving the credentials reason. Two things I'd still change before this becomes standing guidance.
task prqlc:pull-request still doesn't run, and the section reads as if it does. It's named in the opening sentence alongside prqlc:test, and "the inner loop CLAUDE.md documents then runs unchanged" covers both. But prqlc:pull-request routes through test-all, whose third command is cargo llvm-cov --lcov …, and then through the root :lint, whose first command is pre-commit run --all-files. Neither cargo-llvm-cov nor pre-commit is on disk in the sandbox — tend-setup installs only cargo-insta and cargo-nextest, and /home/runner/.cargo-install/ contains exactly those two directories. So the export buys prqlc:test and the first two test-all commands, then stops. This is the section's own closing bullet turned on the section: the run that was verified was prqlc:test, so the claim should be too.
--accept is the same false-green shape the section warns about, unremarked. The INSTA_UPDATE=always bullet is correct, but prqlc:test runs cargo insta test --accept and then cargo clippy --fix --allow-dirty --allow-staged. A green task prqlc:test can therefore mean "snapshots and source were rewritten to match what the code now produces", not "the assertions held". That's the intended behaviour for authoring, but this section is written for sessions that use the command as verification, and it warns about the env-var route to in-place rewriting while recommending the flag route without a word.
One judgment call I'd leave with a maintainer rather than file as a change request: the section quotes tend's refusal of runner-home sandbox_path: entries — that home "can contain credentials unrelated to the selected tool" — and then leads with the manual, per-command form of the same prepend. It's much narrower in scope and I don't think it's unsafe, but it is standing guidance that works around a deliberate control, and it depends on /home/runner keeping its drwxr-x--x bit. If that ever tightens, the documented loop breaks with exactly the no such command this section exists to explain. That seems like an argument for actually pursuing the sandbox_setup:-plus-cp option the last paragraph records, rather than parking it.
prql-bot
left a comment
There was a problem hiding this comment.
Self-authored, so this is a COMMENT rather than an approval.
Both findings from the last round are addressed, and I re-derived them from this session rather than reading the commit message: prqlc/Taskfile.yaml has test-all's third command as cargo llvm-cov --lcov … and root Taskfile.yaml's lint starting with pre-commit run --all-files; command -v pre-commit cargo-llvm-cov finds neither and /home/runner/.cargo-install/ holds exactly cargo-insta and cargo-nextest, matching tend-setup's two baptiste0928/cargo-install steps. The new --accept bullet matches prqlc:test's actual command list (cargo insta test --accept --dnd …, cargo test --doc, cargo clippy --fix --allow-dirty --allow-staged). The export still works here — cargo insta --version returns 1.48.0, cargo nextest --version 0.9.143 — and CI is non-empty in the sandbox, so the "insta's default auto writes nothing when CI is set" clause is operative rather than hypothetical. #6144's dates check out too (opened 2026-08-05, closed unmerged 2026-08-25T16:09Z), and /home/tend-sandbox/.cargo/bin is drwxr-xr-x tend-sandbox tend-sandbox, so the cp option the last paragraph records is real.
One gap the scoping opened up, which is why this isn't silence. CLAUDE.md names task prqlc:pull-request as the thing to run before returning to the user — "Comprehensive prqlc tests - sufficient for most changes". The section now correctly says that command doesn't run here, but stops there, so a session reaching that gate has nothing to run and will improvise the substitute every time — the rediscovery cost this section exists to remove. The natural fallback is prqlc:test, and that one is a false green for exactly the case pull-request exists to catch: test covers -p prqlc-parser -p prqlc, while test-all adds packages_addon and packages_bindings, so a prqlc change that breaks prqlc-js or prqlc-c passes prqlc:test and gets reported as verified. Only two of pull-request's steps are actually unavailable, so naming the runnable remainder costs a sentence. Suggestion inline.
I haven't run that remainder end to end — a whole-workspace --all-targets --all-features clippy on a cold cache is the same cost the rust-toolchain.toml section already warns about, so it's outside this session's budget. What I verified is which two binaries are missing, not the wall-clock of what's left.
prql-bot
left a comment
There was a problem hiding this comment.
Self-authored, so this is a COMMENT rather than an approval — but it's a sign-off, not a finding: the last round's one open point is closed and I have nothing further.
I re-derived the new paragraph against the Taskfiles rather than trusting the commit message. prqlc/Taskfile.yaml's test-all has exactly the two commands the text names — cargo insta test --accept --dnd --features=default,test-dbs --test-runner=nextest --unreferenced=auto over packages_core packages_addon packages_bindings, then the matching cargo test --doc --features=default,test-dbs over the same three — with cargo llvm-cov as the third and only missing one; root Taskfile.yaml's lint is pre-commit run --all-files then cargo clippy --all-targets --all-features, so naming the clippy line as the runnable half is right. packages_core is -p prqlc-parser -p prqlc and packages_bindings includes -p prqlc-js -p prqlc-c, which is what makes the "don't substitute prqlc:test" warning load-bearing rather than decorative. The cross-reference resolves in the right direction too — the rust-toolchain.toml section is below this one. The follow-up commit is pre-commit.ci reflow only.
One thing I'd flag for the maintainer as a judgment call rather than a change request, since it's the same tension the last two rounds circled: the paragraph tells a session to run cargo clippy --all-targets --all-features "budgeting for the cold-cache cost", while the section it points at states flatly that a whole-workspace --all-targets clippy on a cold cache exceeds the session budget and prescribes scoping to one compilation unit instead. A session that follows the pointer gets the scoping recipe and the closing "scope the claim to the command that actually ran" bullet, so it isn't wrong — but the pre-return gate ends up with no affordable full form, and what a session should report when the clippy doesn't fit is left to be re-derived. Whether that's worth another sentence or is better left to judgment is your call; I don't think it blocks merging.
|
we need to fix the underlying problem, am working on it |
cargo-instaandcargo-nextestare installed by.github/actions/tend-setupbut land under the runner user's home, which tend leaves off the sandbox PATH — sotask prqlc:testandtask prqlc:pull-request, the inner loopCLAUDE.mddocuments, answerno such commandout of the box in every tend session. That has been recorded 15+ times in the review-runs tracking issue since 2026-08-05. This adds a section to therunning-tendoverlay giving the one-lineexportthat makestask prqlc:testrun unchanged, whytask prqlc:pull-requeststill doesn't, the fallbacks for when the export isn't used, and what the durable fix would actually be.Verified live from inside a session rather than inferred: with the two
bindirectories prepended,task prqlc:testexits 0 andcargo insta test --acceptrewrites anassert_snapshot!(…, @"")literal in place. TheINSTA_UPDATE=alwayswarning is checked against insta 1.48.0's own source.Evidence, and what changed from the first draft of this PR
The gap.
baptiste0928/cargo-installinstalls into$HOME/.cargo-install/<crate>/binand exposes it via$GITHUB_PATH, which only affects therunneruser. The agent runs astend-sandboxwith its own home and PATH, and tend's PATH derivation deliberately omits runner-home directories.prqlc/Taskfile.yaml:31runscargo insta test --accept --dnd {{.packages_core}} --test-runner=nextest, soprqlc:testneeds both binaries andprqlc:pull-requestinherits the same dependency throughtest-all.prqlc:pull-requestneeds more besides:test-all's coverage step iscargo llvm-covand root:lintstarts withpre-commit run --all-files, andtend-setupinstalls neither — so the export unblocksprqlc:testonly, which is what the section now claims.But the tools are reachable. The first draft of this PR said the sandbox PATH "cannot carry a runner-home path" and built a replacement workflow on top of that. It's wrong:
/home/runnerisdrwxr-x--xand bothbindirectories are755, sotend-sandboxcan traverse to the binaries without being able to enumerate the home. Prepending them in the same shell invocation as the command works end to end —task prqlc:testexits 0 (~3 min on a warmtarget/), and a deliberately emptied inline snapshot atprqlc/prqlc-parser/src/parser/pr/ident.rs:216was refilled by--acceptand the source restored afterwards. So the section now leads with the export, and plaincargo testplus hand-transcribed inline snapshots are the fallback rather than the recommendation.INSTA_UPDATE=alwaysis still a false green. Ininsta-1.48.0/src/env.rs:420-440,SnapshotUpdate::Alwaysmaps toSnapshotUpdateBehavior::InPlace;runtime.rs:580then callsnew_snapshot.save(snapshot_file)— the.snapfile is overwritten with whatever the code just produced, so the assertion cannot fail. The same table shows plaincargo testis a real check: the defaultautoresolves toNoUpdatewhenis_ci()(utils.rs:10, readingCI). The mechanism the first draft described for inline snapshots is also right (runtime.rs:568-573downgradesInPlacetoNewFileand writes aPendingInlineSnapshot) — it just doesn't imply "can't, here", becausecargo-instais the thing that applies it andcargo-instais oneexportaway.What the durable fix is. #6144 proposed symlinking both binaries into
/usr/local/bin; it was green from 2026-08-05 and closed unmerged on 2026-08-25T16:09Z, so the section says not to re-propose it.sandbox_path:isn't the alternative: tend'sproxy/setup-sandbox.shrefuses any entry resolving under the runner's home outside the checkout and fails the job with "Install the tool into the sandbox withsandbox_setup:instead", deliberately, since that home can hold unrelated credentials.sandbox_setup:is the supported lever, and — contrary to what the first draft assumed — it needn't meancargo install:tend-setupruns before the tend action (.github/workflows/tend-mention.yaml:392vs:423), so acpof the two existing binaries into/home/tend-sandbox/.cargo/bin(already on the sandbox PATH, owned and writable by the sandbox user) would do it at zero build cost. That's a materially cheaper trade than the one this PR first described, so it's recorded as an open option for a maintainer rather than ruled out.