Expose audited hardcoded tunables as plugin config
The audit swept every packages/*/* plugin for the new AGENTS.md
convention (no hardcoded tunables in plugins) and exposes each finding
as a defaulted, validated Config field. Defaults are the previously
hardcoded values throughout, so no deployment or golden changes.
- tool-fs (had NO Config): readLimit, readMaxLineLength, readMaxBytes,
readStreamMinSize. The caps thread through ReadToolCaps/ReadWindow —
read-render already documented that the consumer applies the caps, so
they become explicit per-request fields.
- tool-web: searchMaxResults (WEB_SEARCH_MAX_RESULTS stays as the
schemastery default). Also fixes the stale GREP_LIMIT references in
search.ts and the web-capability-seam RFC (no such constant exists).
- bash-local: graceMs (SIGTERM->SIGKILL escalation grace). The
RunInternals.graceMs test seam is gone: graceMs is now a required
SpawnSpec field filled from config, so tests exercise the real
config path and the defaults live in exactly one place.
- subagent-acp: disposeEofGraceMs / disposeGraceMs. The AcpRunSpec
fields become required for the same one-defaulting-layer reason.
- session-persistence-sqlite: journalMode ('wal' default; the
rollback-journal modes serve filesystems where WAL's shared-memory
files do not work, e.g. network mounts).
- hooks-claude + hooks-codex: stderrSummaryMaxChars for the persisted
hook/result stderr summary. The duplicated summarize() helpers merge
into hook-protocol's summarizeStderr(stderr, maxChars), beside the
HookResultRecord field it feeds, with the bound parameterized the
same way runHook's defaultTimeoutMs already is.
- compact-basic: charsPerToken for the token estimator (default 4, the
English-text heuristic; CJK-heavy deployments need ~1-2 or compaction
fires far too late). Also corrects the BasicCompactService class doc,
which claimed defaults the required-field config never had.
- fs-local: deletes the dead STREAM_MIN_SIZE constant and the dead
FsIoInternals.streamMinSize seam — the read-routing bound lives in
the consumer (tool-fs), where it is now config. This is item 1 of
the proposed prune-write-only-fs-surface RFC, annotated accordingly.
Every new field gets range validation (following the existing
assertPositiveFinite pattern), a README row, and tests covering the
configured behavior, the schema default, and load-time rejection.
This commit is contained in:
@@ -12,6 +12,7 @@ Local-subprocess implementation of the `@deepseek-ai/dsh-bash` executor seam: `L
|
||||
timeoutMs: 120000 # default foreground timeout
|
||||
maxTimeoutMs: 600000 # cap for per-call overrides
|
||||
maxOutputBytes: 64000 # per-stream in-memory cap; overflow spills to disk
|
||||
graceMs: 3000 # SIGTERM→SIGKILL escalation grace on kills
|
||||
```
|
||||
|
||||
## Behavior (and where it came from)
|
||||
@@ -19,7 +20,7 @@ Local-subprocess implementation of the `@deepseek-ai/dsh-bash` executor seam: `L
|
||||
Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi; the notable choices:
|
||||
|
||||
- **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.
|
||||
- **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after the `graceMs` grace (default 3s — 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`, 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.
|
||||
|
||||
@@ -17,7 +17,7 @@ import { Context } from 'cordis'
|
||||
import z from 'schemastery'
|
||||
import { BashExecutor, BashTaskId } from '@deepseek-ai/dsh-bash'
|
||||
import type { BashExecRequest, BashExecSpec, BashRunResult, BashTask, BashTaskRead, OwnerToken } from '@deepseek-ai/dsh-bash'
|
||||
import { runBash } from './run.ts'
|
||||
import { DEFAULT_GRACE_MS, runBash } from './run.ts'
|
||||
import type { RunInternals, RunningBash } from './run.ts'
|
||||
|
||||
export { DEFAULT_GRACE_MS, ENV_OVERRIDES, killGroup, OutputCollector, runBash } from './run.ts'
|
||||
@@ -33,6 +33,8 @@ export interface Config {
|
||||
maxTimeoutMs?: number
|
||||
/** Per-stream in-memory output cap; overflow spills to a temp file. */
|
||||
maxOutputBytes?: number
|
||||
/** Grace period between the SIGTERM and the SIGKILL escalation on a kill. */
|
||||
graceMs?: number
|
||||
}
|
||||
|
||||
/** The shape after schemastery applied the defaults (cwd has none). */
|
||||
@@ -57,7 +59,7 @@ interface TrackedTask extends BashTask {
|
||||
* Local-subprocess bash executor. Defaults follow the agent-tool survey
|
||||
* consensus: 120s default / 600s max timeout (Claude Code, OpenCode), 64KB
|
||||
* in-memory output with full-stream spill files (pi, OpenCode),
|
||||
* process-group SIGTERM→SIGKILL kills (OpenCode).
|
||||
* process-group SIGTERM→SIGKILL kills with a 3s grace (OpenCode).
|
||||
*/
|
||||
export class LocalBashExecutor extends BashExecutor {
|
||||
static Config: z<Config> = z.object({
|
||||
@@ -65,11 +67,12 @@ export class LocalBashExecutor extends BashExecutor {
|
||||
timeoutMs: z.number().default(120_000),
|
||||
maxTimeoutMs: z.number().default(600_000),
|
||||
maxOutputBytes: z.number().default(64_000),
|
||||
graceMs: z.number().default(DEFAULT_GRACE_MS),
|
||||
})
|
||||
|
||||
private tasks = new Map<BashTaskId, TrackedTask>()
|
||||
private nextTaskId = 1
|
||||
/** Test seam: timer/spill knobs forwarded to runBash. */
|
||||
/** Test seam: spill knobs forwarded to runBash. */
|
||||
internals: RunInternals = {}
|
||||
|
||||
/** Validated config (schemastery applied the defaults before construction). */
|
||||
@@ -83,6 +86,7 @@ export class LocalBashExecutor extends BashExecutor {
|
||||
assertPositiveFinite('timeoutMs', this.config.timeoutMs)
|
||||
assertPositiveFinite('maxTimeoutMs', this.config.maxTimeoutMs)
|
||||
assertPositiveFinite('maxOutputBytes', this.config.maxOutputBytes)
|
||||
assertPositiveFinite('graceMs', this.config.graceMs)
|
||||
ctx.effect(() => async () => {
|
||||
// Kill every live process group and WAIT for the processes to close so
|
||||
// nothing outlives the fiber (HMR safety) — a TERM-trapping child is
|
||||
@@ -132,6 +136,7 @@ export class LocalBashExecutor extends BashExecutor {
|
||||
cwd: spec.workdir,
|
||||
timeoutMs: spec.timeoutMs,
|
||||
maxOutputBytes: this.config.maxOutputBytes,
|
||||
graceMs: this.config.graceMs,
|
||||
signal: spec.signal,
|
||||
stdin: spec.stdin,
|
||||
env: spec.env,
|
||||
@@ -150,6 +155,7 @@ export class LocalBashExecutor extends BashExecutor {
|
||||
cwd: spec.workdir,
|
||||
timeoutMs: 0,
|
||||
maxOutputBytes: this.config.maxOutputBytes,
|
||||
graceMs: this.config.graceMs,
|
||||
signal: spec.signal,
|
||||
stdin: spec.stdin,
|
||||
env: spec.env,
|
||||
|
||||
@@ -73,6 +73,8 @@ export interface SpawnSpec {
|
||||
timeoutMs: number
|
||||
/** Per-stream in-memory cap; overflow spills to disk (tail kept in memory). */
|
||||
maxOutputBytes: number
|
||||
/** Grace period between the SIGTERM and the SIGKILL escalation on a kill. */
|
||||
graceMs: number
|
||||
/** Abort signal — kills the process group when fired. */
|
||||
signal?: AbortSignal | undefined
|
||||
/**
|
||||
@@ -100,15 +102,13 @@ export interface SpawnOutcome {
|
||||
stderr: CollectedOutput
|
||||
}
|
||||
|
||||
/** Injectable knobs so tests can exercise escalation/spill without long waits. */
|
||||
/** Injectable knobs so tests can exercise spill behavior without the OS tmpdir. */
|
||||
export interface RunInternals {
|
||||
/** Grace period between SIGTERM and SIGKILL on the process group. */
|
||||
graceMs?: number
|
||||
/** Directory for spill files (defaults to the OS temp dir). */
|
||||
spillDir?: string
|
||||
}
|
||||
|
||||
/** Default SIGTERM→SIGKILL grace period (matches OpenCode's 3s). */
|
||||
/** Default SIGTERM→SIGKILL grace period (the `graceMs` config; matches OpenCode's 3s). */
|
||||
export const DEFAULT_GRACE_MS = 3_000
|
||||
|
||||
let spillCounter = 0
|
||||
@@ -292,7 +292,6 @@ export interface RunningBash {
|
||||
* no inherited shell state); revisit when real workflows demand it.
|
||||
*/
|
||||
export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningBash {
|
||||
const graceMs = internals.graceMs ?? DEFAULT_GRACE_MS
|
||||
const spillDir = internals.spillDir ?? privateSpillDir()
|
||||
|
||||
if (spec.signal?.aborted) {
|
||||
@@ -331,7 +330,7 @@ export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningB
|
||||
const kill = (): void => {
|
||||
if (graceTimer !== undefined) return // escalation already in flight
|
||||
killGroup(pid, 'SIGTERM')
|
||||
graceTimer = setTimeout(() => { killGroup(pid, 'SIGKILL') }, graceMs)
|
||||
graceTimer = setTimeout(() => { killGroup(pid, 'SIGKILL') }, spec.graceMs)
|
||||
}
|
||||
|
||||
if (spec.timeoutMs > 0) {
|
||||
|
||||
@@ -11,9 +11,10 @@ const spillDir = mkdtempSync(join(tmpdir(), 'dsh-bash-exec-spec-'))
|
||||
|
||||
async function setup(config: ConstructorParameters<typeof LocalBashExecutor>[1] = {}) {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(LocalBashExecutor, config)
|
||||
// A short kill grace via the REAL config path, so escalation tests stay fast.
|
||||
await ctx.plugin(LocalBashExecutor, { graceMs: 200, ...config })
|
||||
const bash = ctx.bash as LocalBashExecutor
|
||||
bash.internals = { spillDir, graceMs: 200 }
|
||||
bash.internals = { spillDir }
|
||||
return { ctx, bash }
|
||||
}
|
||||
|
||||
@@ -80,12 +81,22 @@ describe('LocalBashExecutor.run', () => {
|
||||
await expect(setup({ timeoutMs: Number.NaN })).rejects.toThrow(/timeoutMs/)
|
||||
await expect(setup({ maxTimeoutMs: 0 })).rejects.toThrow(/maxTimeoutMs/)
|
||||
await expect(setup({ maxOutputBytes: -1 })).rejects.toThrow(/maxOutputBytes/)
|
||||
await expect(setup({ graceMs: 0 })).rejects.toThrow(/graceMs/)
|
||||
|
||||
const { bash } = await setup()
|
||||
expect(() => bash.resolve({ command: 'true', timeoutMs: Number.NaN })).toThrow(/request\.timeoutMs/)
|
||||
expect(() => bash.resolve({ command: 'true', timeoutMs: -1 })).toThrow(/request\.timeoutMs/)
|
||||
})
|
||||
|
||||
it('kill escalation uses the configured graceMs (a TERM-trapping task dies by SIGKILL)', async () => {
|
||||
const { bash } = await setup() // setup pins graceMs: 200 via config
|
||||
const task = bash.start(bash.resolve({ command: 'trap \'\' TERM; sleep 60' }))
|
||||
await new Promise(resolve => setTimeout(resolve, 100))
|
||||
bash.kill(task.id)
|
||||
await task.done
|
||||
expect(task.signal).toBe('SIGKILL')
|
||||
})
|
||||
|
||||
it('per-call timeout takes precedence under the cap and kills on expiry', async () => {
|
||||
const { bash } = await setup({ timeoutMs: 60_000 })
|
||||
const result = await bash.run(bash.resolve({ command: 'sleep 60', timeoutMs: 100 }))
|
||||
@@ -271,9 +282,9 @@ describe('LocalBashExecutor background tasks', () => {
|
||||
|
||||
it('disposing with already-finished tasks only kills the running ones', async () => {
|
||||
const ctx = new Context()
|
||||
const fiber = await ctx.plugin(LocalBashExecutor, {})
|
||||
const fiber = await ctx.plugin(LocalBashExecutor, { graceMs: 200 })
|
||||
const bash = ctx.bash as LocalBashExecutor
|
||||
bash.internals = { spillDir, graceMs: 200 }
|
||||
bash.internals = { spillDir }
|
||||
|
||||
const finished = bash.start(bash.resolve({ command: 'true' }))
|
||||
await finished.done
|
||||
@@ -288,9 +299,9 @@ describe('LocalBashExecutor background tasks', () => {
|
||||
|
||||
it('disposing the executor fiber kills running tasks (no orphans)', async () => {
|
||||
const ctx = new Context()
|
||||
const fiber = await ctx.plugin(LocalBashExecutor, {})
|
||||
const fiber = await ctx.plugin(LocalBashExecutor, { graceMs: 200 })
|
||||
const bash = ctx.bash as LocalBashExecutor
|
||||
bash.internals = { spillDir, graceMs: 200 }
|
||||
bash.internals = { spillDir }
|
||||
const listener = vi.fn()
|
||||
bash.onTaskDone(listener)
|
||||
|
||||
@@ -337,9 +348,9 @@ describe('review fixes: lifecycle hardening', () => {
|
||||
|
||||
it('dispose AWAITS a TERM-trapping process (SIGKILL escalation included)', async () => {
|
||||
const ctx = new Context()
|
||||
const fiber = await ctx.plugin(LocalBashExecutor, {})
|
||||
const fiber = await ctx.plugin(LocalBashExecutor, { graceMs: 200 })
|
||||
const bash = ctx.bash as LocalBashExecutor
|
||||
bash.internals = { spillDir, graceMs: 200 }
|
||||
bash.internals = { spillDir }
|
||||
|
||||
const task = bash.start(bash.resolve({ command: 'trap \'\' TERM; sleep 60' }))
|
||||
await new Promise(resolve => setTimeout(resolve, 100))
|
||||
|
||||
@@ -28,6 +28,7 @@ function spec(command: string, overrides: Partial<Parameters<typeof runBash>[0]>
|
||||
cwd: process.cwd(),
|
||||
timeoutMs: 0,
|
||||
maxOutputBytes: 64_000,
|
||||
graceMs: 3_000,
|
||||
...overrides,
|
||||
}
|
||||
}
|
||||
@@ -106,7 +107,7 @@ describe('runBash', () => {
|
||||
})
|
||||
|
||||
it('escalates to SIGKILL when SIGTERM is trapped', async () => {
|
||||
const running = runBash(spec('trap \'\' TERM; echo ready; sleep 60'), { graceMs: 200 })
|
||||
const running = runBash(spec('trap \'\' TERM; echo ready; sleep 60', { graceMs: 200 }))
|
||||
await waitForStdout(running, 'ready\n')
|
||||
running.kill()
|
||||
const result = await running.done
|
||||
|
||||
@@ -21,8 +21,8 @@ async function setup() {
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(AgentRegistry)
|
||||
await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 }
|
||||
await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000, graceMs: 200 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir }
|
||||
await ctx.plugin(ToolBash)
|
||||
return ctx
|
||||
}
|
||||
@@ -184,8 +184,8 @@ describe('bash tool', () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 }
|
||||
await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100, graceMs: 200 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir }
|
||||
await ctx.plugin(ToolBash)
|
||||
const result = await call(ctx, 'bash', { command: 'for i in $(seq 1 100); do printf "line-%04d\\n" $i; done', description: 'test command' })
|
||||
expect(text(result)).toContain('[output truncated; full output: ')
|
||||
@@ -316,8 +316,8 @@ describe('background tools', () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 }
|
||||
await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100, graceMs: 200 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir }
|
||||
await ctx.plugin(ToolBash)
|
||||
|
||||
const started = await call(ctx, 'bash', { command: 'for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', description: 'test command', run_in_background: true })
|
||||
@@ -568,8 +568,8 @@ describe('background task ownership (cross-session isolation)', () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 }
|
||||
await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000, graceMs: 200 })
|
||||
;(ctx.bash as LocalBashExecutor).internals = { spillDir }
|
||||
const fiber = await ctx.plugin(ToolBash)
|
||||
|
||||
const a = fakeAgent('sess-a')
|
||||
|
||||
Reference in New Issue
Block a user