fix(bash-local): keep /dev/null stdin when no bytes supplied

Address review on the bash stdin/env seam PR: the seam spawned stdin as a
`'pipe'` for EVERY call, closing it empty when no stdin was supplied. That is
NOT observationally equivalent to the pre-seam `'ignore'` default — node's
spawn pipe is an AF_UNIX socket, so `test -c /dev/stdin` (and any fd-0 type
probe) flipped for every model-driven bash call, even though the code claimed
the no-stdin path was unchanged.

Spawn stdin as `'pipe'` only when the caller supplies bytes; otherwise
`'ignore'` (fd 0 → /dev/null), the exact prior default. A literal `stdio`
tuple per branch preserves the typed `spawn` overload's non-null stdout/stderr.
Regression test asserts fd 0 is a char device with no stdin and a socket when
supplied — proven red on the always-pipe code.
This commit is contained in:
Tianyi Cui
2026-07-02 16:39:29 +08:00
parent 5533bb783a
commit 40488e29e5
4 changed files with 43 additions and 22 deletions

View File

@@ -22,7 +22,7 @@ Three deliberate choices:
3. **`stdin`/`env` are required-absent-OK (plain optional) on the resolved spec, NOT required-but-nullable like `owner`.** `owner` is required-but-nullable because a *silently* missing owner yields an unowned, cross-session-readable task — a security footgun that a visible `undefined` guards against. `stdin`/`env` have no such hazard: a missing one means "no stdin / no extra env", which is the safe, ordinary case (every model-driven call). So they stay plain optionals, matching `signal`.
`dsh-bash-local` now ALWAYS spawns stdin as a `'pipe'` and closes it immediately — with the supplied bytes when a caller set `stdin`, empty otherwise. A closed empty pipe gives a reading child EOF exactly as the previous `'ignore'` (`/dev/null`) did, so the no-stdin path is behavior-equivalent; keeping the `stdio` tuple a literal `['pipe','pipe','pipe']` also preserves the typed `spawn` overload that guarantees non-null `stdout`/`stderr`. A child that exits without reading makes the stdin write fail EPIPE; that error is swallowed (the command's outcome rides on its exit code/output, not the write) so it never crashes the host or rejects `done`.
`dsh-bash-local` spawns stdin as a `'pipe'` (writing the supplied bytes, then closing) ONLY when a caller set `stdin`; with none supplied it uses `'ignore'` — fd 0 → `/dev/null` — the exact pre-seam default. This distinction is observable and deliberate: a closed empty pipe and `/dev/null` are NOT the same file type (node's spawn pipe is an `AF_UNIX` socket, so `test -c /dev/stdin` holds for `/dev/null` but not for an empty pipe), so the no-stdin path — every model-driven call — must keep `/dev/null` rather than regress to an always-open pipe. Each branch's `stdio` tuple is a literal, which preserves the typed `spawn` overload that guarantees non-null `stdout`/`stderr`. When stdin IS written, a child that exits without reading makes the write fail EPIPE; that error is swallowed (the command's outcome rides on its exit code/output, not the write) so it never crashes the host or rejects `done`.
## Scope: configurable scrub pattern is NOT included

View File

@@ -21,7 +21,7 @@ Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi;
- **Spawn per call, no shell state** — every call is a fresh non-login `bash -c` (deterministic; no rc files). All four surveyed tools spawn per call. `XXX(stateful-shell)` in `src/run.ts` records the two proven stateful designs (Claude Code's cwd-only persistence; Codex's PTY exec sessions) for when real workflows demand them.
- **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after a 3s grace (OpenCode's escalation; pipelines and subshells die with the parent). ESRCH is tolerated; daemons that re-parent away from the group can still survive — same caveat as the surveyed tools.
- **Tail-keep truncation + spill files** — output beyond `maxOutputBytes` keeps the in-memory TAIL (errors/results cluster at the end — pi/OpenCode rationale) while the FULL stream is appended to a temp file whose path is reported when available. If the final spill close reports a delayed writeback failure, the executor still returns the tail but withholds the path rather than advertising a possibly incomplete file.
- **Model-friendly env + credential scrub** — `process.env` minus credential-shaped vars (`*KEY*`/`*SECRET*`/`*TOKEN*`), then `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. This scrub is the security control that keeps the harness's *ambient* credentials out of a spawned command. A spec's `env` is merged LAST (after the scrub), so a caller's explicit entry — a value it already holds — wins even on a credential-shaped name. The spec's `stdin` is written to the child and closed; with none supplied, stdin is an immediately-closed empty pipe (EOF, as before). Both `env`/`stdin` are set by in-process plugins (the hooks bridges); the model-facing tool doesn't expose them. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md).
- **Model-friendly env + credential scrub** — `process.env` minus credential-shaped vars (`*KEY*`/`*SECRET*`/`*TOKEN*`), then `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. This scrub is the security control that keeps the harness's *ambient* credentials out of a spawned command. A spec's `env` is merged LAST (after the scrub), so a caller's explicit entry — a value it already holds — wins even on a credential-shaped name. The spec's `stdin`, when supplied, is written to the child and closed; with none supplied, fd 0 is `/dev/null` — the exact pre-seam default, so a command that probes stdin's file type is unaffected. Both `env`/`stdin` are set by in-process plugins (the hooks bridges); the model-facing tool doesn't expose them. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md).
- **Background tasks** — `start()` returns immediately, no timeout applies (Claude Code detaches timeouts when backgrounding), `readOutput()` is incremental with whole-stream byte offsets, and disposal kills everything. The spec's opaque `owner` token is stored on the tracked task and returned by `ownerOf(id)` — the executor never interprets it (the consumer's access policy does), and because it lives with the task here it survives a `tool-bash` HMR reload.
## Sandboxing

View File

@@ -15,7 +15,8 @@
* @module dsh-bash-local/run
*/
import { spawn } from 'node:child_process'
import { type ChildProcessByStdio, spawn } from 'node:child_process'
import type { Readable, Writable } from 'node:stream'
import { randomBytes } from 'node:crypto'
import { closeSync, mkdtempSync, openSync, writeSync } from 'node:fs'
import { tmpdir } from 'node:os'
@@ -298,17 +299,20 @@ export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningB
throw new Error(`aborted before spawn: ${String(spec.signal.reason ?? 'aborted')}`)
}
// stdin is ALWAYS a pipe (kept literal so the typed spawn overload guarantees
// non-null stdout/stderr) and is closed immediately: with bytes when a caller
// supplied stdin, empty otherwise. A closed empty pipe gives a reading child
// EOF exactly as `/dev/null` would, so the no-stdin path (every model-driven
// call) is unchanged.
const child = spawn('bash', ['-c', spec.command], {
cwd: spec.cwd,
env: childEnv(spec.env),
stdio: ['pipe', 'pipe', 'pipe'],
detached: true,
})
// stdin is a pipe ONLY when the caller supplied bytes; with none it is `ignore`
// (fd 0 → /dev/null) — the exact pre-seam default. This matters: a spawn pipe
// and /dev/null are NOT observationally identical (node's pipe is an AF_UNIX
// socket, so a command that probes stdin's type — `test -c /dev/stdin`, `stat
// /proc/self/fd/0` — sees a char device vs a socket), so the no-stdin path
// (every model-driven call) must keep /dev/null rather than regress to a socket.
// Two LITERAL `stdio` tuples (not one variable tuple): only a literal lets the
// typed `spawn` overload infer non-null stdout/stderr, which the
// `ChildProcessByStdio` annotation captures (stdin `Writable | null`; stdout/
// stderr the non-null `Readable` the collectors attach to without a cast).
const env = childEnv(spec.env)
const child: ChildProcessByStdio<Writable | null, Readable, Readable> = spec.stdin !== undefined
? spawn('bash', ['-c', spec.command], { cwd: spec.cwd, env, stdio: ['pipe', 'pipe', 'pipe'], detached: true })
: spawn('bash', ['-c', spec.command], { cwd: spec.cwd, env, stdio: ['ignore', 'pipe', 'pipe'], detached: true })
const stdout = new OutputCollector(spec.maxOutputBytes, 'stdout', spillDir)
const stderr = new OutputCollector(spec.maxOutputBytes, 'stderr', spillDir)
@@ -343,10 +347,12 @@ export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningB
}
spec.signal?.addEventListener('abort', onAbort, { once: true })
// Write stdin and close it. This handler must exist: an unhandled 'error' on
// the stream would throw and crash the host. We swallow the error rather than
// reject `done`, and that is correct for ANY stdin-write error, not just the
// common one — the stdin write is BEST-EFFORT, while the command's authoritative
// Write stdin and close it, but ONLY when the caller supplied bytes — with no
// stdin, fd 0 is `ignore` (/dev/null) and `child.stdin` is null. The error
// handler must exist whenever we write: an unhandled 'error' on the stream
// would throw and crash the host. We swallow the error rather than reject
// `done`, and that is correct for ANY stdin-write error, not just the common
// one — the stdin write is BEST-EFFORT, while the command's authoritative
// outcome is its exit code + captured output, which the `close` handler reports
// regardless of whether the write landed. The expected case is EPIPE (the child
// exited without reading, so closing our end of a still-full pipe fails); a rare
@@ -354,8 +360,10 @@ export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningB
// surfaces that itself through its own exit/output (e.g. a hook that gets
// truncated JSON errors out) — rejecting here would instead discard that real
// output and turn it into an opaque infrastructure error, which is worse.
child.stdin.on('error', () => { /* stdin write is best-effort; outcome rides on exit/output. */ })
child.stdin.end(spec.stdin ?? '')
if (child.stdin !== null) {
child.stdin.on('error', () => { /* stdin write is best-effort; outcome rides on exit/output. */ })
child.stdin.end(spec.stdin)
}
const done = new Promise<SpawnOutcome>((resolve, reject) => {
child.on('error', (error) => {

View File

@@ -166,13 +166,26 @@ describe('stdin and extra env (set by in-process plugins)', () => {
})
it('a command that reads stdin sees EOF when none is supplied', async () => {
// No stdin → the always-piped-but-empty stdin closes immediately, so `cat`
// reads EOF and exits 0 with no output (it does NOT block).
// No stdin → fd 0 is /dev/null, so `cat` reads EOF and exits 0 with no
// output (it does NOT block).
const result = await runBash(spec('cat')).done
expect(result.exitCode).toBe(0)
expect(result.stdout.text).toBe('')
})
it('gives fd 0 the exact pre-seam type: /dev/null when no stdin, a pipe when supplied', async () => {
// The no-stdin path must stay observationally identical to the pre-seam
// `ignore` default: a command that probes stdin's file type sees a char
// device (/dev/null). Regressing to an always-open pipe would make fd 0 a
// socket (node's spawn pipe is an AF_UNIX socket, not a FIFO), flipping
// `test -c /dev/stdin` for every model-driven call. When bytes ARE supplied,
// fd 0 is that pipe (a socket), as it must be to carry them.
const none = await runBash(spec('test -c /dev/stdin && echo char || echo other')).done
expect(none.stdout.text).toBe('char\n')
const piped = await runBash(spec('test -S /dev/stdin && echo socket || echo other', { stdin: 'x' })).done
expect(piped.stdout.text).toBe('socket\n')
})
it('merges extra env entries onto the scrubbed environment', async () => {
const result = await runBash(spec('echo "$DSH_EXTRA_ONE/$DSH_EXTRA_TWO"', {
env: { DSH_EXTRA_ONE: 'alpha', DSH_EXTRA_TWO: 'beta' },