[kyle-sexton]

No related issue: follow-up to #797

Summary

#797 removes the write bits on the CLI install tree (<workDir>/cli) and _work/_actions while the legs run. A leg runs as the uid that owns those trees, so it could put the bits back. This change bind-mounts both trees read-only for each leg, which the leg cannot undo.

Fix

• When readOnlyPaths finds a tree, each leg runs as bwrap --die-with-parent --dev-bind / / --ro-bind <dir> <dir> ... -- <cli> <args> (legCommand in lane.cjs). The env, cwd, stdin and stdio are the same as before, and the read-only binds come after the root bind so they take precedence. With no tree present, the CLI runs directly as before.
• --die-with-parent: bwrap forwards no signals, so without it a leg timeout or stdout overflow killed only bwrap and left the CLI running. A local probe showed the orphan still running 4 s later. With the flag the CLI gets SIGKILL when bwrap dies. Timeout and overflow are still classified from spawnSync's ETIMEDOUT/ENOBUFS (lane.cjs, timedOut/overflowed; classify.cjs returns timeout before it reads any message), so classification never depends on the CLI flushing a final result.
• The mode lock from fix(run-claude-cli): reset step files, lock install and action dirs, add plugin and add dirs #797 stays as a second layer. A missing bwrap already fails in verifyIsolation, and there is no unwrapped fallback.

Verification

• timeout 300 node --test .github/actions/run-claude-cli/lane.test.cjs: 38 of 38 pass locally with bwrap 0.11.1.
• New tests:
  • argv with both trees present, including --die-with-parent and the bind order;
  • argv with neither tree present (a pin of the unchanged direct spawn);
  • under real bwrap, a leg's chmod on the install tree fails with EROFS and the file is unchanged;
  • under real bwrap, a timed-out leg leaves no surviving CLI.
    The two real-bwrap tests skip on hosts where bwrap cannot run, such as the test-contracts job. Against the old lane.cjs, the argv test and the EROFS test fail. With --die-with-parent removed, the orphan test fails.
• Existing tests that run the fake CLI against real trees go through an injected spawn that runs whatever follows --. They still exercise the mode lock on hosts without bwrap.
• Hosted-runner probe (nested bwrap, model answer, write refused): see the comment on this PR.

Related

• Refs fix(run-claude-cli): reset step files, lock install and action dirs, add plugin and add dirs #797

🤖 Generated with Claude Code

[kyle-sexton]

Hosted-runner probe at 162c9e1 passed: run 39307237666 (ubuntu-24.04, CLI 2.1.303, role automations, sandbox PR melodic-software/pr-pipeline-sandbox#173, closed unmerged).

• The leg completed and the model answered: lane-class= (empty), exit_code: 0, is_error: false, num_turns: 2, final result OK followed by the script output.
• Nested bwrap and the CLI's subprocess scrub both worked. The Bash tool ran inside the leg with no bwrap namespace error in the log, and the subprocess saw no credential: credential-in-subprocess= (empty).
• A write under each protected tree from inside the leg failed, and so did restoring the write bits first:
  • chmod: changing permissions of '.../run-claude-cli-kXiSDz/cli': Read-only file system, then touch: cannot touch '.../cli/probe-write': Read-only file system
  • The same two errors for /home/runner/work/_actions.
  • mountinfo shows both trees as ro,nosuid,nodev,relatime binds.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-19T19:04:53.904642Z	162c9e1	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 10m 3s —— View job

---

Security review complete

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md rules)
• Review lane.cjs changes (bwrap bind-mount logic)
• Review lane.test.cjs changes (new tests)
• Check for instruction-surface deletions / guardrail removal — N/A, this PR only touches lane.cjs/lane.test.cjs, no rules/instruction files changed
• Adversarially validate findings (independent review:lane-verifier check, CONFIRMED)
• Post final review

Scope: Per REVIEW.md/this lane's charter, GitHub Actions hardening (triggers, permissions, pin drift) is zizmor's lane, not this one — reported here only where it needs logic/trust-boundary reasoning static analysis can't reach.

IMPORTANT — --die-with-parent doesn't reap a detached grandchild a leg's Bash tool spawns

Posted as an inline comment on legCommand() (permalink).

--die-with-parent relies on PR_SET_PDEATHSIG, which is scoped to the single process bwrap directly execs and is cleared on fork() (prctl(2)). It is not propagated to anything that process itself forks, and legCommand doesn't pass --unshare-pid (unlike the unrelated probe at lane.cjs:105), so there's no PID-namespace-level guarantee that killing bwrap takes the whole process tree with it.

For a lane that grants the Bash tool (acknowledged in this file's own header, L23-25), a compromised/prompt-injected task could daemonize a background process (setsid nohup ... & or an explicit double fork) before the leg's timeout fires. That process escapes the bwrap-death kill chain entirely, keeps the leg's live credential (set by legEnv) in its inherited environment, and can keep running past the leg's declared timeout and the job boundary — contradicting the PR's claim that "the CLI gets SIGKILL when bwrap dies" and that timeout classification "never depends on the CLI flushing a final result." The new orphan test doesn't catch this because its fake CLI blocks in its own process (no fork).

Suggested fix: add --unshare-pid to the bwrap argv in legCommand, so the CLI becomes PID 1 of its own PID namespace and its death (or bwrap's) triggers the kernel to SIGKILL every remaining process in that namespace. Worth confirming against the real-bwrap tests that the CLI tolerates running as PID 1.

No other security issues found. The read-only bind-mount design itself (bind order, EROFS enforcement over chmod, mount-namespace persistence across the leg) checks out — reasoning matches the PR's own EROFS test and the hosted-runner probe in the comments.

[claude]

Claude finished @kyle-sexton's task in 6m 20s —— View job

---

Code review complete

• Gather context (PR diff, rule files, REVIEW.md, CLAUDE.md, closing-issue context)
• Review lane.cjs changes (bwrap invocation, bind order, die-with-parent)
• Review lane.test.cjs changes (new tests)
• Check path-scoped rules against changed files
• Post findings

This repo runs a dedicated security lane (pr-review-security.yml), so per REVIEW.md's scope split, security-coded aspects of this sandboxing change (whether --dev-bind/--ro-bind/--die-with-parent actually hold against a hostile leg) are that lane's job, not this one's — and they're also the part the PR already backs with a real hosted-runner probe (nested bwrap, EROFS on write, no orphaned CLI), which I did not re-run and treat as author-claimed/unverified, not independently confirmed.

On correctness/maintainability, legCommand/runLegs (lane.cjs:567-657) and the legCommand argv-construction tests read correctly against the stated bind-order and fallback logic, and the diff is internally consistent with #797's existing mode lock.

One concrete test-coverage finding posted inline on lane.test.cjs:634-650: the new real-bwrap EROFS test only covers the cli install tree, not the _work/_actions tree the same code also protects — so an automated regression in enforcement for the second bound tree specifically wouldn't be caught by this suite on a host where bwrap works, only by a manual probe.

I was not able to independently run node --test .github/actions/run-claude-cli/lane.test.cjs or a real-bwrap check in this review environment (tool restrictions), so the PR's own "38 of 38 pass" and hosted-probe claims are noted as author-claimed, not verified here.

No findings against the paved-paths, closing-issue, or .claude/rules/pr-body-contract.md criteria: no closing issue is claimed (body opens with No related issue: follow-up to #797, matching the contract), no paved-path index row was found to apply, and the PR body already fills ## Summary/## Fix/## Verification/## Related.

[github-actions]

claude-review has reviewed this pull request through 8afb95e; a later push is reviewed from there.

[github-actions]

claude-security-review has reviewed this pull request through 8afb95e; a later push is reviewed from there.

[claude]

Security review in progress

• Gather context (incremental diff since 162c9e1, lane.cjs/lane.test.cjs)
• Review the --unshare-pid addition in legCommand
• Review the new _work/_actions real-bwrap test
• Adversarial validation of any candidate findings
• Post final review

View job run

[claude]

Claude finished @kyle-sexton's task in 3m 0s —— View job

---

Code review complete (incremental, since 162c9e1)

• Gather context (incremental diff, rule files, REVIEW.md, closing-issue context)
• Review lane.cjs changes since 162c9e1
• Review lane.test.cjs changes since 162c9e1
• Check path-scoped rules / paved paths against changed files
• Adversarially validate candidate findings
• Post findings

Scope: per this lane's charter, this is incremental — only .claude-lane/incremental.diff (162c9e1..5a15fa2) was in scope; every other file was reviewed at 162c9e1 and isn't re-reviewed here. Security-coded reasoning about the sandbox's trust boundary is pr-review-security.yml's lane, not this one's, per REVIEW.md's split.

What changed: this commit adds --unshare-pid to the legCommand() bwrap invocation (lane.cjs#L597-L613) and updates BWRAP_PROBE's test-side mirror, plus two new tests in lane.test.cjs: a real-bwrap EROFS check for the _work/_actions tree (closing the coverage gap flagged in the prior review round), and a real-bwrap test asserting neither the CLI nor a process it detaches survives a timed-out leg.

This is a direct, correctly-scoped fix for the two findings raised on 162c9e1 (the --die-with-parent-without---unshare-pid gap both claude and kyle-sexton independently confirmed, and the missing _work/_actions real-bwrap write test). Checked:

• Flag placement is consistent: legCommand's argv and the updated argv-construction test (lane.test.cjs:606-631) agree on --die-with-parent, --unshare-pid in that order before the binds.
• BWRAP_PROBE (lane.cjs:105, unchanged by this diff) already required --unshare-pid, so verifyIsolation was already gating on the capability this fix now relies on — no new fail-open path.
• The new _work/_actions EROFS test (lane.test.cjs:658-677) mirrors the existing install-tree test exactly (same FAKE_CHMOD/FAKE_WRITE mechanism, same assertions), closing the gap from the prior round's finding.
• The new orphan/PID-namespace test (lane.test.cjs:681-700) exercises the real failure mode reported against 162c9e1 (a detached grandchild via setsid-equivalent spawn({detached:true}).unref()), with a timeout short enough (300 ms) to fire well before either the CLI's 5 s or the detachee's 3 s marker-write.

No new findings. I could not independently execute node --test .github/actions/run-claude-cli/lane.test.cjs or a real-bwrap run in this review environment (tool restrictions same as the prior round), so the "38/38 pass" and hosted-probe-rerun claims for this commit remain author-claimed/unverified here, not independently confirmed.

No closing issue is claimed (PR body still opens with No related issue: follow-up to #797, satisfying .claude/rules/pr-body-contract.md); no paved-path index row was found to apply to these files.

[claude]

Claude finished @kyle-sexton's task in 4m 0s —— View job

---

Code review complete

• Gather context (incremental diff since 5a15fa2, rule files, REVIEW.md, closing-issue context)
• Review lane.cjs changes since 5a15fa2 (the --unshare-pid removal)
• Review lane.test.cjs changes since 5a15fa2 (the detached-process test narrowed)
• Adversarial validation of the candidate finding (independent review:lane-verifier check, CONFIRMED)
• Post final review

Scope: incremental — only the diff between 5a15fa2 and this head (8afb95e) in lane.cjs/lane.test.cjs; every other file was reviewed at 5a15fa2. Per REVIEW.md, this repo runs a dedicated security lane (pr-review-security.yml), so whether reverting --unshare-pid reopens the detached-process vulnerability the security lane flagged on the prior push is that lane's call to make, not this one's — I did not re-litigate it.

What changed in this push

8afb95e reverts 5a15fa2's --unshare-pid addition in legCommand (lane.cjs:597-612), citing a real, distinct functional break: inside the new PID namespace, /proc is still the host's procfs, so the CLI's own nested bwrap (its Bash-tool sandbox) failed with open /proc/3/ns/ns failed. That rationale is plausible on its face and I have no way to independently confirm or refute the nested-bwrap failure in this environment (no bwrap binary here), so it's author-claimed/unverified by me, not confirmed.

Finding posted inline

One test-coverage finding on lane.test.cjs:674-691: the regression test 5a15fa2 added specifically to catch a leg's detached child surviving a timed-out leg was deleted along with the FAKE_DETACHED helper, and the replacement test only checks the fake CLI's own process. Nothing in the suite now exercises detached-process survival, and there's no comment or tracking reference marking that gap as known/intentional. Independently verified by a review:lane-verifier subagent (CONFIRMED) before posting.

No findings on the .claude/rules/pr-body-contract.md criteria (unchanged this push, already satisfied) or on paved paths (no standards index found in this repo). No closing-issue context file was present — the PR body's No related issue: follow-up to #797 line matches the body contract, consistent with the prior review.

I could not run node --test .github/actions/run-claude-cli/lane.test.cjs in this environment (no bwrap binary, and shell execution for the test run itself was blocked by tooling here), so I did not independently verify the author's "38/38 pass" or hosted-probe claims for this push — noted as author-claimed, not verified.

[claude]

Claude finished @kyle-sexton's task in 2m 9s —— View job

---

Security review complete

• Gather context (incremental diff since 162c9e1, lane.test.cjs only)
• Run /review:security-review per the lane's instructions
• Review the new _work/_actions real-bwrap test added in this push
• Adversarial validation — N/A, no candidate findings surfaced
• Post final review

Scope: per this lane's incremental-review contract, lane.cjs is unchanged since 162c9e1 (commit 5a15fa2 added --unshare-pid, commit 8afb95e reverted it — net diff is zero) and was already reviewed at that commit, so it's out of scope here. Only the diff to .github/actions/run-claude-cli/lane.test.cjs since 162c9e1 was reviewed.

Finding: none. The only change is the new test "under bwrap a leg cannot put the write bits back on _work/_actions" (permalink), which closes the coverage gap flagged in the prior review round (real bwrap enforcement was tested for the cli install tree but not for _work/_actions). It mirrors the existing install-tree test exactly: builds a writable file under _work/_actions/owner/action, sets RUNNER_WORKSPACE so readOnlyPaths (lane.cjs:567-575) resolves the sibling _actions directory, and asserts the leg's chmod/write attempts fail with EROFS under real bwrap. This is pure additive test code — no production logic changed, no new trust boundary, no instruction-surface deletion. Per the skip gate, there is no security-relevant surface in this increment.

For the record (not a new finding, since this file is out of scope and was already flagged at 162c9e1): the --unshare-pid fix for the "--die-with-parent doesn't reap a detached grandchild" issue was attempted in 5a15fa2 and reverted in 8afb95e (nested bwrap inside the leg broke: open /proc/3/ns/ns failed), so legCommand at current HEAD is byte-identical to 162c9e1 and that previously-reported CRITICAL finding still applies to this PR as posted in the earlier review round.