refactor(setup): run Codex Cloud as root and simplify both setup paths - #3841
Conversation
…up-web Codex Cloud rewrote cargo as a wrapper that dropped to a UID-1000 `ubuntu` user under `tini`, and prepared the environment by chowning the checkout, the rustup home, and three cache directories to match. All of it existed so the two permission tests would not skip — and `setup-web` has always run them as root and skipped them anyway. Running Codex Cloud as root too drops the wrapper, `tini`, `runuser`, and every chown, and makes the two environments agree about what the suite observes. Both setups now install the same archives, so their versions and checksums move to `scripts/tool-pins.sh`, which both source. The weekly bump has one file to edit instead of two that drift. `setup-web` also gains what it was missing next to Codex Cloud: `lsof`, a checksummed Nushell install rather than a check that one is already present, a `uv` bootstrap with pinned pre-commit, and `$HOME/.local/bin` on PATH. Its cargo tools pin to the shared versions, and `wt` installs from the debug build already produced a few lines above rather than through a second full compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
The consolidation itself reads well — one pin file sourced by both setups, and the wrapper removal is clean (no runuser, tini, or CODEX_CARGO_IDENTITY_PROBE references survive anywhere in the tree). Four things on what got dropped:
The skip set is larger than two tests. The header comment and tests/CLAUDE.md both say the two permission tests are what Codex Cloud gives up, but the root-probe pattern (if fs::write(&probe, "").is_ok() { … return; } into an 0o555 directory) appears at five sites, and root satisfies all five. Beyond test_permission_error_prevents_save and test_approval_prompt_permission_error, running as root also silences test_remove_foreground_succeeds_with_stuck_directory and its _detached sibling, plus test_configure_shell_fish_legacy_remove_failure_warns — all three ran as UID 1000 under the wrapper and stop running now. Two of them cover wt remove's stuck-directory path, which is the part I'd want the accounting to name rather than fold into "two permission tests". The comparative claim (setup-web already ran as root) holds regardless, so this is about the accounting, not the decision. Suggestions inline on both.
tini's subreaper went with the wrapper, unremarked. The old README credited it with matching "the suite's permission and child-reaping assumptions"; the new text keeps the permission half and drops the other without saying where reaping now comes from. It matters because NixSignaller::is_alive in src/git/fsmonitor.rs is kill(pid, 0) — Ok for a zombie — so a git fsmonitor--daemon that wt SIGKILLs reads as still alive until something reaps it. tini -s was that something inside the test tree; on Codex Cloud without it, it depends on what the container's PID 1 does. I can't check that from CI, and setup-web has never had a reaper, so this may well be a non-issue — but it's the one removed guarantee the PR body doesn't account for, and the fsmonitor reap tests are where it'd show on the first environment build.
The shared cargo pins don't actually bind in setup-web. command -v cargo-insta &> /dev/null || cargo install … keeps whatever version is already there, so an image (or a previous run at an older pin) carrying cargo-insta 1.40 stays on 1.40 while the script prints "Installed … cargo-insta", and a bump in tool-pins.sh silently doesn't reach the environment. codex.sh installs its pinned archive unconditionally, so the two diverge despite sharing the file. cargo install --version is already a no-op at exit 0 when the pinned version is installed, so dropping the guard costs nothing in the common case — suggestion inline.
The new Nushell fetch skips the retry hardening. codex.sh's download() uses --retry 6 --retry-all-errors --retry-delay 2; the copy in setup-web is a bare curl -fsSL, so a CDN blip fails the whole task where the same fetch retries on the other side. Suggestion inline for the Nushell curl; the PowerShell .deb fetch just above it has the same gap if you want them consistent.
Two smaller things, both outside the diff — happy to push either if useful: export PATH="$HOME/bin:$PATH" inside the gh install branch is now dead given the new export at the top of the task, and cargo build 2>&1 | tail -5 still swallows a build failure under set -e, which the new install -D … target/debug/wt turns into a cannot stat rather than the compile error cargo install --path . used to surface.
Nothing invoked Task on Codex Cloud once the launchers stopped going through it, so it is no longer installed. The archive checksums go the same way as the Taskfile digest did: HTTPS authenticates GitHub, and the container is disposable and holds no secrets, so verifying the downloads earned a helper pair and a 64-char line per tool for very little. Each install is now `curl | tar` and an `install`. The version numbers stay. The gate runs `--all-features`, so nu and pwsh drive PTY snapshots their own versions can move, and CI pins the same ones. Also gone: curl retry flags, a root precondition that `set -e` on `apt-get` already reports, and a `command -v` sweep over tools whose installs would have failed the script already. `codex.sh` is 68 lines, from 155. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
The cut reads well. curl | tar still fails the script on a bad fetch because set -o pipefail is on; install -D covers the install -d the old code did by hand; and the removed command -v sweep really is redundant — every tool it checked is either installed by a step that would have failed first (apt-get, uv tool install, install -D), run explicitly (pwsh), or exercised by maintain (cargo, pre-commit). scripts/tool-pins.sh has no surviving references anywhere in the tree.
One claim doesn't hold: "keep them level with the CI pins" is unfollowable for pwsh. .github/actions/test-setup/action.yaml pins cargo-insta (=1.48.0), cargo-nextest (=0.9.143), and nushell (0.115.0) — all matching codex.sh — but there is no PowerShell pin anywhere under .github/; every hit is a shell: pwsh usage, so CI runs whatever the runner image ships, and that version moves on its own with each image bump. pwsh is the tool the new comment names most emphatically as snapshot-affecting, and it's the one with nothing to be level with. The same claim appears in the PR body ("CI pins the same ones") and in the weekly-maintenance bullet — which is the one that gets executed, so a future maintenance run goes looking for a pwsh pin in test-setup/action.yaml and finds nothing. Suggestions inline on both.
The title now describes the commit before it. "share tool pins with setup-web" was true at 360ae0c; 43df7d5 deletes scripts/tool-pins.sh and puts NU_VERSION=0.115.0 / PWSH_VERSION=7.6.5 inline in both codex.sh and Taskfile.yaml instead. The body is accurate about this, but the title is what lands as the squash subject.
Smaller, take or leave: tests/CLAUDE.md documents task profile-tests and task coverage as the suite's commands, and both stop being runnable on Codex Cloud now that Task isn't installed. coverage was already broken there (cargo-llvm-cov is never installed), but profile-tests needs only nextest and bash and did work. "Nothing invoked Task on Codex Cloud" holds for the setup scripts; it doesn't quite hold for an agent following tests/CLAUDE.md.
CI pins cargo-insta, cargo-nextest, and Nushell in .github/actions/test-setup/action.yaml, and codex.sh matches all three. Nothing under .github/ pins PowerShell — CI runs whatever the runner image ships — so "keep them level with the CI pins" sent a weekly maintenance run looking for a pin that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three taken. pwsh pin — confirmed and fixed in 9afb696. Title — updated to "run Codex Cloud as root and simplify both setup paths", which no longer claims a shared pins file that 43df7d5 deleted.
|
|
9afb696 checks out — the only PowerShell hits under The earlier review went unanswered, and 43df7d5 moots two of its four points on its own — the retry asymmetry closed by dropping The skip set is eight tests, not two — and I undercounted at five. The wrapper ran Two of the eight gate on
Sites, with line links at 9afb696Write-probe gated (unconditional as root — six):
Both set the config file to |
`scripts/` existed only for the Codex Cloud setup, and `dev/` already holds the repo's other development files. The README folds into the script's header — the environment's settings, its two commands, and the validation line — so the setup is one file and `scripts/` goes away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
git grep scripts/ over the merged tree comes back clean — the only hits are scripts/deploy.sh and scripts/pre-start-setup.py, both unrelated config examples — and each line of the deleted README has a home in the new header. cd "$(dirname "${BASH_SOURCE[0]}")/.." is right for the new depth.
One leftover from the fold. The old closing paragraph survived at lines 23–26: it restates lines 4–6 in substance and points at the README.md this same commit deletes. Suggestion inline.
One thing outside the repo. The environment's two settings fields still hold bash scripts/codex-cloud/codex.sh setup|maintain, so the next environment build fails on the missing path until they're re-typed once — the one kind of change to this file that the "short and fixed" property those lines claim doesn't cover.
It restated the opening lines and pointed at the README this branch deletes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both taken. Leftover paragraph — fixed in a177388. The fold added the new opening block but left the old closing one, which restated it and pointed at the README the same commit deletes. The settings fields — correct, and worth stating loudly since it is the one part of this no commit can fix: the environment currently invokes
|
|
a177388 is exactly the three lines and nothing else, and the header now reads straight through — settings block, validate line, image/permission block, pins block — with no restatement and no pointer to a deleted file. Two things from the comment before this one are still open — flagging that they went past rather than re-arguing them, since both are one-commit changes and either may simply be a deliberate leave:
Happy to push either if useful, or to leave them. |
`command -v cargo-insta || cargo install` keeps whatever version the image already carries, where main's unconditional `cargo install` tracked latest. Neither form pins, so the guard bought nothing and silently froze the one tool whose version moves snapshot output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three permission tests skipped on `USER == "root"`, which fails open: a container that runs as root without exporting `USER` runs them and asserts an error root never gets. The other seven root-skipping tests probe the filesystem; these now do too, through one helper. The docs that named the skip set describe it instead of counting it. Two careful passes put the count at five and then eight; it is ten, and the `wt remove` stuck-directory pair is the part worth naming. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both taken, and the count is worth one more correction. The cargo-tool guard — dropped in 4c3bb5b. Restoring main's unconditional The skip set is ten, not eight. Two sites escaped the sweep: The
|
|
Both commits check out. 4c3bb5b is main's line restored verbatim, and after d07c906 Since it merged: root isn't only a skip condition — five tests fail under it, and this merge is what puts them there. They set a restrictive mode and assert the resulting failure, with no probe in front, so root's
That is the sentence in The remedy I'd reach for is the one already in the tree rather than five more probes: One correction to the count while it's in the body: the set is eleven, not ten — Mechanism, the full inventory, and what I couldn't runNot executed as root. This sandbox is uid 1002 with no passwordless sudo, and Per-site trace:
Adjacent, not a failure: The eleven that do skip (all probe-gated as of d07c906): Inventory method, in case it's worth re-running after any change here: |
Consolidates the two remaining Codex Cloud drafts, #3835 and #3838, onto what #3839 landed, then cuts what was left. Main's setup is a 163-line script and a 26-line README under
scripts/codex-cloud/; this is one 79-line file atdev/codex.sh, beside the repo's other development files, with the README folded into its header. That emptiesscripts/— the directory existed only for this.Codex Cloud runs as root (from #3835). Setup used to replace
/root/.cargo/bin/cargowith a wrapper that re-executed cargo as a UID-1000ubuntuuser undertini, and maintenance chowned the checkout, the rustup home, and three cache directories to match. All of it existed to keep the suite's permission tests from skipping, since root can write to a read-only file.Worth stating plainly: ten tests now skip on Codex Cloud. The pair carrying the most weight is
test_remove_foreground_succeeds_with_stuck_directoryand its_detachedtwin, the only automated coverage ofwt removeagainst a directory it cannot delete. It is not a new hole —setup-webcreates no non-root user, so the Claude Code web environment has always skipped them, andtests/integration_tests/approval_pty.rs:157carries a standing TODO about it. Codex Cloud was the one environment buying an exception, and a cargo wrapper,tini,runuser, and four chown passes were the price. Both environments now agree about what the suite observes, and that TODO is the single place to fix it for both.Three of the ten decided that skip by reading
$USERrather than by probing the filesystem, which fails open: a container that runs as root without exportingUSERruns them and asserts an error root never gets. They probe now, through one helper, like the other seven.Task, the checksums, and the retries are gone. Nothing invoked Task on Codex Cloud once #3839 stopped routing the launchers through it, so it is no longer installed. The archive checksums follow the Taskfile digest for the same reason that one went: HTTPS authenticates GitHub and the container is disposable and secret-free, so verifying each download bought a helper pair and a 64-char line per tool for very little. Each install is now
curl | tarand aninstall.The version numbers stay. The gate runs
--all-features, so nu and pwsh drive PTY snapshots their own versions can move, and.github/actions/test-setup/action.yamlpins cargo-insta, cargo-nextest, and nu to the same three versions — unpinning those would make the environment and CI disagree about snapshot output. Nothing under.github/pins PowerShell, so CI runs whatever the runner image ships and that version answers to nothing but these scripts.setup-webcatches up (from #3838). It gainslsof, installs Nushell from its release archive rather than checking that one is already present, bootstrapsuvwith pre-commit, and puts$HOME/.local/binon PATH.wtinstalls from the debug build produced a few lines earlier instead of through a second full compile. Itscargo installof cargo-insta and cargo-nextest stays unconditional, as on main: neither form pins, and acommand -vguard would have frozen whatever versions the image happened to carry.Testing
cargo run -- hook pre-merge --yespasses, shellcheck is clean at warning level, andtask --liststill parses. The.tar.xzand.tar.gzextractions were run against the real release archives to confirm the tar flags and the paths inside them. The three converted tests pass unprivileged; their skip branch rests on the same probe the other seven root-skipping tests already use.Neither setup path is executable from a dev machine — Codex Cloud needs Linux and root on the universal image,
setup-webneeds a web image — so the first real exercise is the next environment build.