codex engine: unprivileged-user safety strategy (drop-sudo kills the runner) - #40
Merged
Conversation
…runner) Both first codex runs (inspect_ai#389) died at ~62 minutes with 'the hosted runner lost communication' — openai/codex-action#160: the default drop-sudo strategy chmods root-owned service sockets under /run, breaking D-Bus, crashing systemd-resolved, and killing DNS; the runner agent survives on established connections for 52-65 minutes and then dies, taking the always() backstops with it. Every codex step now creates a dedicated codex system user (per the action's examples/unprivileged-user.yml, with the group plumbing so the runner user can land codex's edits afterward) and passes safety-strategy: unprivileged-user + codex-user — the README's other blessed strategy for permission profiles. The API key stays out of reach (codex has no sudo); the host is never mutated. Revisit when the upstream fixes land. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ransomr
added
the
auto
Member
Author
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
Review — codex unprivileged-user safety strategyThe diagnosis and direction are right: I verified the setup step is a faithful copy of the action's Blocking (inline on claude-review.yml:652, applies to all four workflows): with Likely blocking (inline on claude-review.yml:642): the checkout stays runner-owned, so every git command the codex user runs hits git's "dubious ownership" refusal, and neither profile sandbox lets the agent add Non-blocking nits (inline): the comment's claim that the group plumbing is what lets the runner land codex's edits is inaccurate — Regression risk: low for everything that currently works. The new step and inputs are gated on Also fine as-is: the 4× duplication of the comment+step follows this repo's existing pattern of inlining codex steps per workflow (a shared composite action would add an |
Contributor
|
🔎 Review complete. |
Contributor
|
🤖 auto review rounds: 3 (cap 10). |
Contributor
|
@ransomr — auto implemented all four findings from review round 1, but could not push them: the machine account's fine-grained PAT lacks the "Workflows" permission, so any change touching What the patch contains (all four workflows get the identical setup-step change):
Plus a design-doc paragraph recording the two grants and why the upstream example never needs them. Nothing was declined; the four inline threads are deliberately left unresolved because the fixes are not on the branch yet — please resolve them when the patch lands. To apply (from the PR branch): copy the block below into round1.b64 — base64 git patch (commit c11e9f5, applies on 68c6fd6)Status: mergeable per GitHub (merge state UNSTABLE due to the failing 4s |
Two fixes the upstream example never needs, per review: - codex (running via sudo -u) must create the explicit output-file under RUNNER_TEMP, which is 755 runner:runner on the hosted image — grant group write (codex is in group runner). - the checkout stays runner-owned, so git run as codex hits the dubious-ownership refusal and no profile sandbox lets the agent add the exemption — add safe.directory to the codex user's global config. Plus the two nits: reword the setup comment (the runner->codex group grant is inert within the job; landing works via runner's ownership and world-readable files) and re-attach the allow-bot-users comment to the Run Codex review step it documents. Design doc records the two grants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@review — round 2: the round-1 hand-off patch is applied and pushed (RUNNER_TEMP group-write for the codex output file, safe.directory for the codex user, corrected setup comments); round-1 threads resolved. |
Contributor
Review — codex unprivileged-user safety strategyOverall: correct and well-reasoned; three non-blocking suggestions, no blocking issues. I verified the load-bearing claims against the upstream action rather than taking the comments on faith: Verified correct:
Findings (all non-blocking, inline comments for the first two):
Regression assessment beyond the above: for short codex runs that did succeed under drop-sudo, the user switch means codex can no longer write runner-owned locations outside the workspace (e.g. tool caches under Also worth noting (no action needed): the setup block is now copy-pasted four times, and the round-1 fix already demonstrated the sync cost — a local composite action (the |
Contributor
|
🔎 Review complete. |
Contributor
|
@ransomr — auto implemented all three non-blocking findings from review round 2 (nothing declined), but hit the same push limitation as round 1: the machine account's fine-grained PAT lacks the "Workflows" permission, so the push was rejected ( What the patch contains:
Plus design/codex-engine.md updated to record the subdir rationale and both divergences. The three inline threads are deliberately left unresolved because the fixes are not on the branch yet — please resolve them when the patch lands. Status: mergeable per GitHub (merge state CLEAN); the only checks on the head are the two skipping stub jobs — no real CI in this repo; the four workflows were YAML-validated after the edits. To apply (from the PR branch): copy the block below into round2.b64 — base64 git patch (commit 99704e0, applies on 8990b93) |
…setup trim - All four codex setup steps: replace the RUNNER_TEMP group-write with a codex-owned $RUNNER_TEMP/codex subdir (755) and point output-file there — g+w on the temp root would expose the runner's step scripts and per-step GITHUB_ENV/OUTPUT/PATH files to a sandbox-escaped codex. - Write-path land steps (claude, auto, auto-review): chown -R runner .git before committing — codex-created objects/ fan-out dirs are codex-owned and runner's codex-group membership is inert in-job, so runner-side object writes could fail intermittently. - claude-review.yml: drop the workspace-write grants (chown/g+rwX/g+s and the inert runner->codex group add) — the profile is :read-only, reading needs no grant, and the recursive sweep is pure latency on a full-history checkout. Divergence documented in the step comment. - design/codex-engine.md updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@review — round 3: the round-2 hand-off patch is applied and pushed (codex-owned RUNNER_TEMP subdir instead of group-writing the temp root, .git chown at land time, reviewer setup trimmed to read-only needs); round-2 threads resolved. |
Contributor
Review — round 3 (codex unprivileged-user safety strategy)The round-2 patch landed faithfully — I verified the codex-owned One new blocking finding (inline on claude-review.yml:679): the reviewer workflow passes an inline One non-blocking nit (inline on claude-review.yml:643, applies to all four): the new "Create codex user" step has no Regression assessment otherwise: Claude-engine runs (the default on every caller) are untouched — every new step and input is gated on |
Contributor
|
🔎 Review complete. |
Contributor
|
@ransomr — auto implemented both round-3 findings (nothing declined), but hit the same push limitation as rounds 1 and 2: the machine account's fine-grained PAT lacks the "Workflows" permission, so the push was rejected ( What the patch contains:
Plus design/codex-engine.md records the schema workaround and adds dropping it to the revisit condition. The two inline threads are deliberately left unresolved because the fixes are not on the branch yet — please resolve them when the patch lands. Status: MERGEABLE, merge state CLEAN; the only checks on the head are the two skipping stub jobs — no real CI in this repo. Verification: all four workflows YAML-parse after the edits, and the heredoc in the rendered setup script was extracted and checked (terminator at column 0, body parses as the same JSON the inline schema carried). To apply (from the PR branch): copy the block below into round3.b64 — base64 git patch (commit ca0e73e, applies on a55fe12) |
…rfacing Round-3 blocking finding: codex-action@v1's inline output-schema is broken with unprivileged-user (openai/codex-action#103 - schema temp dir mktemp'd as codex mode 700, then written as runner -> EACCES before codex starts; fix #147 unmerged), so every engine:codex review would die deterministically. The reviewer setup step now writes the schema runner-owned and the codex step passes output-schema-file - the explicit-path branch creates no temp dir and skips the action's cleanup. Round-3 nit (all four workflows): the Create codex user step gets id: codexuser, a check in each Surface step (its failure SKIPS the codex step, so no existing outcome check fired - a silent red job), and inclusion in both auto-loop refund conditions so a setup crash does not burn a round/attempt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@review — round 4: the round-3 hand-off patch is applied and pushed (review schema via output-schema-file to dodge the upstream unprivileged-user temp-dir bug, codexuser failure surfacing + round refunds); round-3 threads resolved. |
Contributor
|
Usually transient (model API capacity/overload, or a GitHub-side outage). Re-trigger with |
Member
Author
|
@review — round 4 retry: the previous run died installing the Claude Code CLI (transient connection reset); the round-3 patch is already on the branch. |
Contributor
Review — round 4 (final state after the round-3 patch)No blocking issues and no new suggestions — the branch looks ready to merge. This round I re-verified the whole final state (not just the round-3 delta) against the upstream Verified against upstream source:
Verified in the workflows:
Regression notes (all regression-accepted, informational only):
Two things I considered and deliberately did not raise as suggestions: the schema file's 644 relies on the runner's umask (022 on hosted images) rather than an explicit chmod — but the whole design already leans on umask-022 world-readability (codex's own files must be world-readable for the land step), so pinning one file would be inconsistent noise; and the 4× duplication of the setup step could fold into a composite action, but this is a temporary workaround slated for removal when the upstream fixes land, and the repo's style is deliberate per-workflow inlining with narrative comments. Nice work across the rounds — the design-doc section accurately reflects the final implementation, including both deliberate divergences. |
Contributor
|
🔎 Review complete — no outstanding suggestions. |
Contributor
|
🤖 auto: the reviewer has no outstanding feedback — this PR has converged. auto never takes the final step (merge, or upstream promotion on the fork), so it's yours now; see the checks above for CI status. The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-cause fix for both codex test runs on inspect_ai#389 dying at ~62 minutes with "the hosted runner lost communication with the server": openai/codex-action#160. The action's default⚠️ either time.
safety-strategy: drop-sudochmods root-owned service sockets under/run, breaking D-Bus → crashing systemd-resolved → killing machine-wide DNS; the runner agent coasts on established connections for 52–65 minutes and then dies — taking thealways()error-surfacing backstops with it, which is why the issue got noFix per the action's own docs: every codex step now creates a dedicated
codexsystem user (setup mirrors the action'sexamples/unprivileged-user.yml, including the group plumbing so the runner user can commit/push codex's edits afterward) and runs withsafety-strategy: unprivileged-user+codex-user: codex— the README's other blessed strategy when permission profiles are in use. Containment: user boundary + profile sandbox; the API key stays unreadable (codex has no sudo —unsafewas rejected for exactly that exposure). No host mutation, so nothing outlives the job. Design doc records the incident and the revisit condition (the upstream fix PRs openai/codex-action#161 and openai/codex-action#166 landing).Workflow-editing PR: review requested via comment.
🤖 Generated with Claude Code