fix(sandbox): a command run by exec no longer inherits this process's stdin - #550
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: make the stdin regression test deterministic so it fails against the pre-fix implementation
The production change is appropriately narrow and closes the inherited descriptor on both agent exec and the RPC shell fallback. I found one blocking test issue inline.
I covered the repository rules in AGENTS.md/CLAUDE.md and CONTEXT-MAP.md, the full diff, DirectExecutor's ExecTool and RPC callers, the existing background-exec precedent, relevant history, backward compatibility, test effectiveness, and the stated architecture boundaries. I found no separate runtime or architecture defect.
Verification:
uv run pytest tests/test_sandbox_unit.py -x: 99 passeduv run pytest tests/test_rpc_shell.py -x: 6 passeduv run python scripts/check_source_language.py github/main...HEAD: passeduv run ruff check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passeduv run ruff format --check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passedgit diff --check github/main...HEAD: passed
make check-source-language itself could not run because make is unavailable in this environment; I ran the target's Python checker directly with the review range instead.
4131342 to
bc70c7a
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new revision fixes the prior test gap by supplying known data on fd 0 and checking that the child neither receives nor consumes it. I independently suppressed the production stdin argument and confirmed that this condition detects the pre-fix behavior. The production change remains appropriately narrow.
I covered AGENTS.md/CLAUDE.md and CONTEXT-MAP.md, the full diff and revision delta, DirectExecutor's agent and RPC callers, the background-exec precedent, relevant history, backward compatibility, test effectiveness, architecture boundaries, and interaction with the latest main. No tests were weakened, and the merge-tree check against current main is clean.
Verification:
uv run pytest tests/test_sandbox_unit.py tests/test_rpc_shell.py -x: 105 passed- pre-fix simulation with the production
stdinargument suppressed: child consumedframe, parent read EOF uv run python scripts/check_source_language.py github/main...HEAD: passeduv run ruff check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passeduv run ruff format --check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passedgit diff --check github/main...HEAD: passedgit merge-tree --write-tree HEAD github/main: clean
bc70c7a to
fa962d5
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision's only substantive delta moves the controlled-stdin probe into a child interpreter instead of replacing the pytest worker's fd 0. That retains the deterministic regression check while avoiding interference with pytest capture. I found no new issue.
I covered the repository rules in AGENTS.md/CLAUDE.md and CONTEXT-MAP.md, the full diff and revision delta, DirectExecutor's callers and relevant history, backward compatibility, test effectiveness, architecture boundaries, and interaction with current main. No tests were weakened.
Verification:
uv run pytest tests/test_sandbox_unit.py tests/test_rpc_shell.py -x: 105 passeduv run python scripts/check_source_language.py github/main...HEAD: passeduv run ruff check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passeduv run ruff format --check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passedgit diff --check github/main...HEAD: passedgit merge-tree --write-tree HEAD github/main: clean
|
Not a blocker -- an A stands beside this. Two sentences of prose say something the code does not DEVNULL is an open read-only fd on /dev/null, not a closed one, and a child sees a different world
The cross-reference is exact about parity and loose about the word: background_exec.py:127 passes Two words in two places: "pointed at /dev/null", or "reads EOF at once". Nothing else in the change Everything I checked on the change itself holds, and the regression test in particular is sound. I So the fix is protected in CI, not only on a terminal. |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: resolve the CHANGELOG.md conflict against the current main branch
The code revision is unchanged, but the target branch advanced to 0cb8b7996. git merge-tree --write-tree HEAD github/main now reports a content conflict in CHANGELOG.md, where both sides inserted a new first item under ### Fixed; GitHub likewise reports this PR as dirty and unmergeable. The branch must be refreshed and that changelog conflict resolved before it can merge.
The new /dev/null wording note is explicitly nonblocking and does not change the code assessment. The previously settled test thread remains resolved.
Verification on the unchanged head: uv run pytest tests/test_sandbox_unit.py tests/test_rpc_shell.py -x passed all 105 tests.
… stdin DirectExecutor spawned the command with stdout and stderr piped and stdin left to inherit. Run as an ACP sub-agent, this process's stdin is the pipe the client answers permission requests on, so a command that reads its input (ssh without -n, cat, python3 -) consumed the frames arriving while it ran. Every other session sharing the process then waited out the 300 s approval deadline on an answer the client had written within a millisecond of the request, and reported it as "approval lapsed". Measured 2026-09-15 with four Raven-Code sessions in one process: 18 of 183 approvals lost, each inside another session's ssh; the same DAG with commands that do not read stdin lost none of 493. lsof showed the ssh child's fd 0 and the sub-agent's fd 0 as one pipe. The command's stdin is now DEVNULL, as background_exec already does. The test runs the check in a child interpreter fed a byte on its stdin. Only running "cat" here would prove nothing -- under pytest fd 0 is already at EOF, so the inheriting spawn reads nothing either and the assertion holds against the implementation it is meant to catch, measured both ways -- and swapping this process's fd 0 instead would reach into the capture pytest installed on it. With a readable fd 0 the two diverge: inherited, the child prints the byte and leaves its parent's stdin drained; pointed at /dev/null, the byte is still there for the parent afterwards, which is the property that matters. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
fa962d5 to
cc92c56
Compare
|
Revision cc92c56 rebases onto main at cf3c95f and resolves the CHANGELOG.md The nonblocking wording note is taken too, since leaving it risks someone Verification on this head:
|
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The rebase preserves the reviewed production change and deterministic regression test, keeps current main's changelog entries, and resolves the previously reported conflict. The accepted wording correction now describes the null-device behavior accurately in the production comment and changelog. GitHub reports this head mergeable, and the merge-tree check against current main is clean.
I covered the repository rules in AGENTS.md/CLAUDE.md and CONTEXT-MAP.md, the full diff and range-diff from the prior revision, callers and relevant history, backward compatibility, test effectiveness, architecture boundaries, current comments, and interaction with the target tip. No tests were weakened.
Verification:
uv run pytest tests/test_sandbox_unit.py tests/test_rpc_shell.py -x: 105 passeduv run python scripts/check_source_language.py github/main...HEAD: passeduv run ruff check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passeduv run ruff format --check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passeduv run ty check raven/sandbox/direct_executor.py: passedgit diff --check github/main...HEAD: passedgit merge-tree --write-tree HEAD github/main: clean
|
Not a blocker -- the code change is right and I am accepting it. One observation about the CHANGELOG entry's reach, and one cosmetic note. (1) The bullet's first sentence promises more than this PR delivers, because CHANGELOG.md:124 opens "A command run by I reproduced the local half through the production wrapper, with a control: Same wrapper (machine_exec.py:175's TWO SCOPE LIMITS I want on the record rather than glossed. First, the ssh half is argued from the argv, not run: I read transport.py:150-164 and observed The transport.py behaviour PREDATES this PR -- transport.py is bit-identical at base 2d21c7d and at this head, and has never carried One more irony worth seeing: the bullet's own worked example is "( (2) Cosmetic: the comment block above WHAT I VERIFIED AND FOUND SOUND, so you know where I looked. The executable text of direct_executor.py is unchanged across the rewrite between the two heads -- comment lines stripped, both hash cb8cb1fbe25f77b1a27ec42677b5e7e7430b094d08ec3563938f17e60bfa57b3, and a tokenize comparison shows the only difference is three extra NL tokens; tests/test_sandbox_unit.py is the same blob at both heads (1f7a3312b088f6a9722ab0f84015ed591babb444). The comment's parity claim is true: background_exec.py:127 already passed MUTATIONS, because passing tests on a fix are the weakest evidence there is. Removing GATES on head cc92c56 / tip 2d21c7d: |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The live head changed after this review turn began: d223191e2d15 merges current main into the previously reviewed cc92c5642311 patch without changing the PR diff. The code remains sound and merge-clean.
Nonblocking follow-ups on the record: the verifier's existing changelog-scope and cosmetic observations, and this head's merge-from-main history rather than the rebase flow prescribed by AGENTS.md. The latter is history-only, is excluded by the repository's commit-message gates, and disappears under the configured squash merge, so it does not meet the late-round blocker bar.
I covered the repository rules, full live diff and range-diff, callers and history, backward compatibility, test effectiveness, architecture boundaries, current comments, and interaction with current main. No tests were weakened.
Verification on the live head in an isolated worktree:
uv run pytest tests/test_sandbox_unit.py tests/test_rpc_shell.py -x: 105 passeduv run python scripts/check_source_language.py github/main...HEAD: passeduv run ruff check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passeduv run ruff format --check raven/sandbox/direct_executor.py tests/test_sandbox_unit.py: passeduv run ty check raven/sandbox/direct_executor.py: passedgit diff --check github/main...HEAD: passedgit merge-tree --write-tree HEAD github/main: clean
DirectExecutor spawned the command with stdout and stderr piped and stdin left
to inherit. Run as an ACP sub-agent, this process's stdin is the pipe the
client answers permission requests on, so a command that reads its input (ssh
without -n, cat, python3 -) consumed the frames that arrived while it ran.
Every other session sharing the process then waited out the 300 s approval
deadline on an answer the client had written within a millisecond of the
request, and reported it as "approval lapsed".
The command's stdin is now DEVNULL, which is what background_exec already
does; only the foreground path was left inheriting.
Type: fix
Verification:
approvals lost, each one inside another session's ssh. The same DAG with
commands that do not read stdin lost none of 493. lsof showed the ssh child's
fd 0 and the sub-agent's fd 0 as the same pipe.
old code.
(TestAgentLoopExecutorLifecycle) fail on 0464aa1 without this change, so
this adds one passing test and no new failures.
Risk: low. One keyword argument on one subprocess call. A command that
genuinely wants piped input already has to ask for it; nothing in the tree
relies on inheriting the agent's own stdin.