mirror of https://github.com/deepseek-ai/deepseek-harness synced 2026-08-15 21:04:50 +00:00

Files

Tianyi Cui 40488e29e5 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.

2026-07-02 16:39:29 +08:00

7.2 KiB

Raw Blame History

RFC: stdin + extra env on the bash seam

Status: implemented (accepted 2026-06-30)

Context

The hooks subsystem runs external hook commands the way Claude Code and Codex do: a hook is a shell command that receives its event payload as JSON on stdin and reads context from a handful of environment variables (CLAUDE_PROJECT_DIR, CLAUDE_PLUGIN_ROOT, PLUGIN_ROOT, …). The harness already has a perfectly good command runner behind the ctx.bash capability seam (dsh-bash → dsh-bash-local), with process-group kills, output truncation/spill, and a credential scrub. Reusing it for hook execution means a hook bridge does not re-implement subprocess plumbing — but the seam had no way to write stdin or set extra env. This RFC adds those two inputs.

These fields are NOT a new security boundary. It is tempting to frame arbitrary-stdin / arbitrary-env as "dangerous, so gate who may use them" — but that framing is wrong, because a model driving the bash tool already has equivalent power through ordinary shell syntax: FOO=bar cmd sets an env var, a heredoc or printf … | cmd feeds arbitrary stdin. Adding env/stdin as seam fields grants the model no capability it lacks. In particular they cannot exfiltrate the harness's ambient credentials: the real control for that is the credential scrub in dsh-bash-local's childEnv(), which strips *KEY*/*SECRET*/*TOKEN* from process.env before the child sees it (see AGENTS.md § Defensive patterns, "Never hand untrusted/model output the ambient environment or predictable paths"). The scrub works regardless of these fields — a model cannot read a value that is not in the environment, and tool-call arguments are static JSON, never shell-evaluated, so a model cannot write env: {LEAK: $DEEPSEEK_API_KEY} and have it expand. So the security question is already answered by the scrub; this RFC is only about giving trusted in-process callers a clean way to pass a JSON payload + CLAUDE_* vars without routing them through model-visible shell text.

Decision

Add stdin?: string and env?: Record<string, string> to both BashExecRequest (the model-/plugin-facing request) and BashExecSpec (the resolved spec run/start act on), and thread them through dsh-bash-local: resolve() carries them verbatim, run()/start() pass them to runBash, which writes the bytes to the child's stdin and merges the extra env.

Three deliberate choices:

The model-facing bash tool simply does NOT expose stdin/env as parameters — not as a security wall, but because bash syntax already covers the model's needs, so duplicating them as tool params would be redundant surface. dsh-tool-bash's bash tool builds its BashExecRequest from command/workdir/timeoutMs/signal/owner only; a model that includes env/stdin keys in its tool-call arguments simply has them ignored. A regression guard (tool-bash "does not forward env/stdin" tests) drives the real tool with those extra args and asserts the recorded request carries neither field — its purpose is to catch a future refactor that blindly spreads ...args into the request and silently starts forwarding model input into the post-scrub env merge, NOT to defend a trust boundary. In-process plugins (the hooks bridges, native plugins) that construct a BashExecRequest directly set the fields; the seam imposes no access policy (consistent with how owner works — the executor stores but never interprets it).
env merges AFTER the credential scrub, so an explicit caller entry always wins — even a credential-shaped name. This is correct because the scrub's job is narrow: stop the harness's ambient process.env credentials from leaking into a spawned command. A caller that explicitly sets a var has named a value it already holds (not the ambient secret), so the scrub is not a constraint on it. childEnv(extra?) layers scrub(process.env) → ENV_OVERRIDES (the model-friendly TERM=dumb etc.) → extra, last-wins.
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 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

An earlier sketch of this work also proposed making SENSITIVE_ENV_PATTERN configurable. Validating against the code, that is speculative and already subsumed: run.ts documents a configurable whitelist as future work, and the new explicit env field — merged after the scrub — already gives a caller full control, including over credential-shaped vars. There is no current caller that needs to broaden the ambient scrub (the hazard runs the other way). Adding a config knob now would be a feature with no consumer, against AGENTS.md § "Don't add features beyond what the task requires". If a real workflow ever needs to forward a specific ambient credential, the explicit env field is the supported path; a configurable scrub can be reconsidered then.

Consequences

A hook bridge builds a BashExecRequest with the hook's JSON payload as stdin and its CLAUDE_*/PLUGIN_ROOT vars as env, and runs it through the same ctx.bash everything else uses — no bespoke subprocess code, and the full process-group-kill / truncation / spill machinery for free. The model-facing attack surface is unchanged (the credential scrub, not these fields, is what bounds it), and the bash tool's request-building stays the single place that decides which fields a model call carries — guarded by a test that fails if a refactor starts forwarding model input. The vocabulary addition is documented in docs/core-data-structures/bash.md (the type-equiv request/spec blocks) and the three bash-package READMEs.

7.2 KiB Raw Blame History