Use native Codex Cloud environment and remove Codex-specific launcher test - #3835
Use native Codex Cloud environment and remove Codex-specific launcher test#3835max-sixty wants to merge 1 commit into
Conversation
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The main thing I'd want before this lands is a Codex Cloud run of cargo run -- hook pre-merge --yes on this branch. #3810 established the wrapper as load-bearing rather than incidental ("preserve the proven UID 1000, tini, pinned-tool, and retry behavior") and backed it with a Cloud task showing 4,601/4,601 passing; this PR's Testing section only covers YAML parsing and the digest, neither of which exercises what the wrapper was there for. The two inline notes are the specific things I expect that run to surface.
Two process notes:
- #3836 is byte-identical to this branch (
git diff 0d3ce4b1 a1de40f2is empty) — worth closing one. - The Taskfile rewrite and the test removal are independent: the test passes on this branch as-is, since the README digest was updated to match. Splitting them would let the environment change be judged on its Cloud run and the test removal on its own merits, rather than the removal riding along.
| preconditions: | ||
| - sh: test "$(id -u)" = 0 && test "$(id -u ubuntu 2>/dev/null)" = 1000 | ||
| msg: Codex Cloud setup requires root and the universal image's ubuntu user | ||
| - sh: test "$(id -u)" = 0 |
There was a problem hiding this comment.
Dropping the ubuntu UID-1000 requirement (and the wrapper below) means the suite runs as root, and three permission tests in tests/integration_tests/step_copy_ignored.rs should fail there. test_copy_ignored_error_includes_path_directory, test_copy_ignored_error_includes_path_file, and test_copy_ignored_error_nested_file_parent_creation each fs::set_permissions(..., fs::Permissions::from_mode(0o555)) on the destination and then assert !output.status.success() — root's CAP_DAC_OVERRIDE bypasses the mode bits, so wt step copy-ignored succeeds and the assertion inverts.
The repo already treats root as defeating this class of assertion; those three just never needed a guard because the environment guaranteed non-root. Their guarded siblings say so explicitly — configure_shell.rs ("Skip when running as root — permissions don't restrict."), remove.rs ("Check if permissions actually restrict us (skip if running as root)"), approval_pty.rs ("TODO: Find a way to test permission errors without skipping when running as root.") — and tests/CLAUDE.md documents the same for test_permission_error_prevents_save / test_approval_prompt_permission_error.
Worth noting the second-order cost too: the guarded ones don't fail under root, they skip, so Codex Cloud stops covering the permission-error paths entirely rather than reporting that it does.
I haven't run this — a Cloud task on this branch would settle it either way, and it's the same evidence #3810 shipped.
| add-apt-repository -y ppa:git-core/ppa | ||
| apt-get update -qq | ||
| apt-get install -y -qq --no-install-recommends git zsh fish xz-utils lsof tini | ||
| apt-get install -y -qq --no-install-recommends git zsh fish xz-utils lsof |
There was a problem hiding this comment.
tini goes away here (and from the command -v loop below) along with the wrapper that used it. The README this PR rewrites said it was there to match "the suite's child-reaping assumptions", and #3810 listed it among the behavior it was deliberately preserving — so the removal is worth a sentence on what makes reaping fine now, rather than dropping it as wrapper scaffolding.
Lower confidence than the permission point: it depends on whether the setup process ends up as PID 1 in the Cloud container, which I can't check from here. The same Cloud run would cover it — the shape to watch for is a PTY or shell-integration test hanging rather than failing.
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
This is the only thing checking that the README's TASKFILE_SHA matches the file it gates, and both the README and the weekly-maintenance step still assume something does. The README (unchanged by this PR) says "After a reviewed Taskfile change reaches the default branch, update both environment commands", and .claude/skills/running-tend/SKILL.md instructs "copy the Taskfile's new digest into both README launchers" — two hand-maintained steps that now have no verification behind them.
The failure isn't dangerous (the digest lives in the environment settings; a stale README just means a pasted command fails sha256sum -c), but it's silent until someone sets up the environment, and this file is otherwise where exactly this kind of doc-sync check lives.
The PR's reasoning is that the test is "repository-specific and unnecessary for usual CI runs" — that's true of most of readme_sync.rs, so I don't think it separates this test from its neighbours. If the objection is the coupling to the launcher's exact string, asserting just the digest and the task version would keep the guarantee with less brittleness.
|
The run-as-root simplification from this draft is consolidated into #3841 — cargo wrapper,
|
#3841) 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_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Motivation
tinilogic.Description
tests/integration_tests/readme_sync.rsso README/Taskfile launcher checks no longer assert a repo-specific launcher string.scripts/codex-cloud/Taskfile.yamlto stop creating a UID-switchingcargowrapper, droptiniusage, and avoid enforcing theubuntuUID 1000 precondition; the Taskfile now prepares and runs Rustup,pre-commit, and Cargo directly using the native environment.ubuntuuser and replaced those with straightforward directory creation and nativerustup/pre-commitinvocations inprepare-codex.scripts/codex-cloud/README.mdto reflect the new Taskfile behavior and refreshed the documentedTASKFILE_SHAto match the updated Taskfile.Testing
cargo test --test integration test_taskfile_llm_commands_match_config_exampleand it passed (1 passed).task --taskfile scripts/codex-cloud/Taskfile.yaml --listand it succeeded.TASKFILE_SHAlines inscripts/codex-cloud/README.mdmatchsha256sum scripts/codex-cloud/Taskfile.yamland the checks passed.git diff --checkand basic repo hygiene checks and they passed; note thatpython3 -c 'import yaml'failed due toPyYAMLnot being installed in the environment, buttaskitself successfully parsed the YAML, so Taskfile syntax was validated.Codex Task