Skip to content

refactor(setup): run Codex Cloud as root and simplify both setup paths - #3841

Merged
max-sixty merged 7 commits into
mainfrom
codex-simp
Aug 17, 2026
Merged

refactor(setup): run Codex Cloud as root and simplify both setup paths#3841
max-sixty merged 7 commits into
mainfrom
codex-simp

Conversation

@max-sixty

@max-sixty max-sixty commented Aug 17, 2026

Copy link
Copy Markdown
Owner

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 at dev/codex.sh, beside the repo's other development files, with the README folded into its header. That empties scripts/ — the directory existed only for this.

Codex Cloud runs as root (from #3835). Setup used to replace /root/.cargo/bin/cargo with a wrapper that re-executed cargo as a UID-1000 ubuntu user under tini, 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_directory and its _detached twin, the only automated coverage of wt remove against a directory it cannot delete. It is not a new hole — setup-web creates no non-root user, so the Claude Code web environment has always skipped them, and tests/integration_tests/approval_pty.rs:157 carries 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 $USER rather than by probing the filesystem, which fails open: a container that runs as root without exporting USER runs 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 | 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 .github/actions/test-setup/action.yaml pins 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-web catches up (from #3838). It gains lsof, installs Nushell from its release archive rather than checking that one is already present, bootstraps uv with pre-commit, and puts $HOME/.local/bin on PATH. wt installs from the debug build produced a few lines earlier instead of through a second full compile. Its cargo install of cargo-insta and cargo-nextest stays unconditional, as on main: neither form pins, and a command -v guard would have frozen whatever versions the image happened to carry.

Testing

cargo run -- hook pre-merge --yes passes, shellcheck is clean at warning level, and task --list still parses. The .tar.xz and .tar.gz extractions 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-web needs a web image — so the first real exercise is the next environment build.

This was written by Claude Code on behalf of max-sixty

…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 worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread scripts/codex-cloud/codex.sh Outdated
Comment thread tests/CLAUDE.md Outdated
Comment thread Taskfile.yaml Outdated
Comment thread Taskfile.yaml Outdated
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 worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread scripts/codex-cloud/codex.sh Outdated
Comment thread .claude/skills/running-tend/SKILL.md Outdated
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>
@max-sixty max-sixty changed the title refactor(setup): run Codex Cloud as root and share tool pins with setup-web refactor(setup): run Codex Cloud as root and simplify both setup paths Aug 17, 2026
@max-sixty

Copy link
Copy Markdown
Owner Author

All three taken.

pwsh pin — confirmed and fixed in 9afb696. .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; nothing under .github/ pins PowerShell. The script comment, the weekly-maintenance bullet, and the PR body now say that CI has no pwsh pin and that version answers only to these scripts.

Title — updated to "run Codex Cloud as root and simplify both setup paths", which no longer claims a shared pins file that 43df7d5 deleted.

task profile-tests — leaving it. Reinstalling Task so one documented profiling command keeps working runs against the whole point of the change, and tests/CLAUDE.md already treats Task as something an environment may need installed rather than something every environment has. Flagging rather than hiding it: task profile-tests did work on Codex Cloud before this and does not now.

This was written by Claude Code on behalf of max-sixty

@worktrunk-bot

Copy link
Copy Markdown
Collaborator

9afb696 checks out — the only PowerShell hits under .github/ are shell: pwsh usages, so there is genuinely nothing for that pin to be level with, and splitting the maintenance bullet into "these three track test-setup/action.yaml, pwsh tracks nothing" is the form a weekly run can actually follow. No argument on task profile-tests either; flagging it in the body is the right resolution.

The earlier review went unanswered, and 43df7d5 moots two of its four points on its own — the retry asymmetry closed by dropping download()'s retries rather than adding them to setup-web, and the tool-pins.sh binding concern went with the file. Two are still live, and one of them I got wrong in a way that cuts against the PR body:

The skip set is eight tests, not two — and I undercounted at five. The wrapper ran cargo itself as UID 1000, so the --lib tests were on the ubuntu side of it too, and three root-gated unit tests stop running alongside the five integration ones. That matters mainly for the accounting: the header comment, tests/CLAUDE.md, and the body all say "the two permission tests", and the two that carry the most weight are the wt remove stuck-directory pair, which is the only automated coverage of that path.

Two of the eight gate on $USER, not on euid. test_reload_from_permission_error and test_save_to_existing_file_with_unreadable_file_returns_read_error skip on std::env::var("USER").as_deref() == Ok("root") rather than on a write probe, so they fail open: in a container that runs as root without exporting USER, they proceed and assert an error that root never gets. The other six probe the filesystem and are unconditional. I can't check what the universal image exports, and setup-web has been root all along without this surfacing — which is decent evidence the web image sets it — but it's the one item in this set whose outcome turns on the container's environment rather than on the uid, so it's worth a glance at the first environment build.

setup-web's new command -v guard freezes the cargo tools. command -v cargo-insta &> /dev/null || cargo install cargo-insta --quiet replaces main's unconditional cargo install cargo-insta cargo-nextest --quiet. Unconditional cargo install tracks latest; the guard keeps whatever is already there forever. Neither form pins, so this is not about matching codex.sh's =1.48.0 — it is that an image carrying an old cargo-insta now stays on it silently, and cargo-insta's version is exactly the kind that moves snapshot output. Dropping the guard restores main's behaviour at no cost, and pinning both to the codex.sh values would make the two setups agree.

tini's subreaper remains the one removed guarantee the body doesn't account for — unverifiable from CI, and quite possibly a non-issue given setup-web has never had a reaper, but the fsmonitor reap path is where it would show.

Sites, with line links at 9afb696

Write-probe gated (unconditional as root — six):

$USER gated (two):

Both set the config file to 0o000 and expect a read error; as root the read succeeds, so reaching the assertion is a failure rather than a skip.

`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 worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread dev/codex.sh Outdated
It restated the opening lines and pointed at the README this branch deletes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@max-sixty

Copy link
Copy Markdown
Owner Author

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 bash scripts/codex-cloud/codex.sh setup|maintain, and that path is gone after merge, so both fields need re-typing once to bash dev/codex.sh setup|maintain. Raised with the repo owner rather than left in a review thread.

This was written by Claude Code on behalf of max-sixty

@worktrunk-bot

Copy link
Copy Markdown
Collaborator

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. git grep for codex-cloud and tool-pins over the tree at that commit comes back empty outside the script itself. Nothing to add on the settings fields; raising it with the owner is the only place that fixes.

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:

  • Taskfile.yaml:147-148 — the command -v guard freezes cargo-insta and cargo-nextest at whatever the image carries, where main's unconditional cargo install tracked latest.
  • dev/codex.sh:16 and tests/CLAUDE.md:17 — both still name two tests; the wt remove stuck-directory pair is the part of the eight worth naming.

Happy to push either if useful, or to leave them.

max-sixty and others added 2 commits August 17, 2026 16:14
`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>
@max-sixty

Copy link
Copy Markdown
Owner Author

Both taken, and the count is worth one more correction.

The cargo-tool guard — dropped in 4c3bb5b. Restoring main's unconditional cargo install cargo-insta cargo-nextest --quiet is a line shorter than the guard as well as correct, which is the giveaway that the guard was never buying anything.

The skip set is ten, not eight. Two sites escaped the sweep: src/config/deprecation.rs:3887 is a seventh probe-gated test, and src/config/user/tests.rs has a third $USER gate in test_with_locked_mutation_propagates_save_error. That a careful pass landed on five, then eight, then ten is the argument against carrying a number at all, so dev/codex.sh and tests/CLAUDE.md now describe the set — tests needing an unprivileged uid — and name the wt remove stuck-directory pair as the part worth naming. The PR body carries the ten, where it can go stale without misleading anyone reading the script.

The $USER gates — converted in d07c906. All three now call one permissions_restrict_reads helper that writes a mode-0 probe file and tries to read it, which is the shape the other seven already use. That takes the outcome off what the container exports and puts it on the uid, so it no longer needs a glance at the first environment build.

tini's subreaper stays unaccounted for; setup-web has never had a reaper, so the fsmonitor reap path has been running without one in at least one environment for as long as that environment has existed. Leaving it rather than reintroducing tini to cover a gap neither environment has shown.

This was written by Claude Code on behalf of max-sixty

@max-sixty
max-sixty merged commit 8faeabf into main Aug 17, 2026
41 checks passed
@max-sixty
max-sixty deleted the codex-simp branch August 17, 2026 23:58
@worktrunk-bot

Copy link
Copy Markdown
Collaborator

Both commits check out. 4c3bb5b is main's line restored verbatim, and after d07c906 git grep 'USER").as_deref()' returns nothing at all — all three gates now go through the same probe the other seven use, and the docs describe the set rather than counting it.

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 CAP_DAC_OVERRIDE makes the operation succeed and the assertion is the thing that breaks. Under the wrapper this PR removes, cargo ran as uid 1000, so Codex Cloud never saw them; as of 8faeabf it does.

That is the sentence in dev/codex.sh:16 — "the tests that need an unprivileged uid skip" — the set that needs an unprivileged uid is larger than the set that skips.

The remedy I'd reach for is the one already in the tree rather than five more probes: shell_integration_prompt.rs:847 makes writes fail by putting a regular file where a parent directory belongs — ENOTDIR, which root can't override — and its comment already calls that out as "root-proof, unlike chmod". Reshaping keeps the coverage in the environment that now runs as root, where a probe guard drops it there permanently; the five above are the only automated coverage of those error branches. Happy to open that PR if useful.

One correction to the count while it's in the body: the set is eleven, not ten — approvals.rs:806 test_save_failure_preserves_existing_file is an eighth probe-gated test. Which is more evidence for the position the commit already took than against it.

Mechanism, the full inventory, and what I couldn't run

Not executed as root. This sandbox is uid 1002 with no passwordless sudo, and unshare -r fails with Operation not permitted, so there is no root to run the suite under here. The claims below come from the code paths plus DAC semantics — a process with CAP_DAC_OVERRIDE bypasses file mode bits for read and write, including directory writes. Confirmable in the environment itself with:

cargo test --lib -- test_write_handles_write_failure test_uninstall_from_file_leaves_rc_intact_when_write_fails
cargo test --test integration copy_ignored_error

Per-site trace:

  • test_write_handles_write_failurecached.write() reaches cache::write_json, a plain fs::write into the 0o444 dir; root succeeds, sweep_lru(dir, 1) keeps the entry, and read finds it. The assertion is is_none().
  • test_uninstall_from_file_leaves_rc_intact_when_write_fails — the rewrite's temp file lands in the 0o500 dir; root creates it, uninstall_from_file returns Ok, and both result.is_err() and the rc-unchanged assert_eq! fail.
  • The three step_copy_ignored tests — the wt child is root too, so the copy into the 0o555 destination succeeds and the command exits 0. The fourth 0o555 site in that file (:1351) asserts success and preserved mode bits, so it is fine as root.

Adjacent, not a failure: configure_shell.rs:1870 test_scan_managed_files_skips_unreadable_file passes as root, but vacuously — its fixture is function wt\nend\n, which contains neither WRAPPER_MARKER nor config shell init, so is_worktrunk_managed_content returns false whether or not the file reads. Root skips the Err(_) => continue line it was written to cover rather than failing on it.

The eleven that do skip (all probe-gated as of d07c906): src/remove_dir.rs:173, src/config/user/tests.rs ×3, src/config/deprecation.rs:3883, src/config/approvals.rs:806, tests/integration_tests/configure_shell.rs:617, tests/integration_tests/approval_pty.rs:186, tests/integration_tests/remove.rs:3741 and :3795, tests/integration_tests/approval_save.rs:543.

Inventory method, in case it's worth re-running after any change here: grep -rn "from_mode(0o\|set_mode(0o" --include=*.rs src tests filtered to the restrictive modes (0o000, 0o444, 0o500, 0o555) gives every site; each is then either probe-gated, asserts success, or is one of the five above.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants