fix(tool-bash): handle unavailable spill paths

This commit is contained in:
Tianyi Cui
2026-06-19 01:54:57 +08:00
parent 0334b4ad2e
commit 49bb6a88eb
7 changed files with 69 additions and 10 deletions

View File

@@ -12,7 +12,7 @@ The harness needs one internal language for messages that the loop, session log,
Own it: messages are arrays of typed content blocks (`text`, `reasoning`, `tool-call`, `tool-result`, `image`), with the union derived from the merge-extensible `ContentBlockMap` so plugins add block types via declaration merging. The same merge-extensible-map pattern types every "stringly" field (`MessageSource`, `FinishReason`, `TurnTrigger`, `TurnEndReason`). Streaming is a raw chunk protocol; `BlockAssembler` is the single shared assembly implementation. Adapters translate to provider wire formats — mapping cost lives in adapters, where it belongs.
In-session context injection (`context/message`, `steering/message`) renders as tagged user-role envelopes (the system-reminder pattern) rather than a new role, so adapters carry zero burden. TODO(review): revisit once the DeepSeek V4 adapter exists.
In-session context injection (`context/message`, `steering/message`) renders as tagged user-role envelopes (the system-reminder pattern) rather than a new role, so adapters carry zero burden. TODO(review): verify the tagged-envelope rendering against live model behavior; the real adapters that were the original precondition now exist (see the twin-adapter RFC).
## Consequences

View File

@@ -4,7 +4,7 @@ Status: proposed
## Problem
`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration is now duplicated: per-session state, `session/created` adoption, seed-prefix collision checks, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. That code is correctness-heavy and already receives the same fixes twice.
`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration is now duplicated: per-session state, `session/created` adoption, backend-specific prefix reads, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. The pure seed-prefix collision and serializability guards have already moved into the seam package; the remaining orchestration is still correctness-heavy and already receives the same fixes twice.
## Proposal

View File

@@ -20,7 +20,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. `TODO(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. The model can `grep`/`tail` the spill file with bash itself.
- **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** — `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results.
- **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.

View File

@@ -44,7 +44,7 @@ export interface CollectedOutput {
text: string
/** True when bytes were dropped from `text`. */
truncated: boolean
/** Path to a file holding the COMPLETE stream, when truncated. */
/** Path to a file holding the COMPLETE stream, when truncated and available. */
spillPath?: string
}
@@ -87,9 +87,9 @@ export interface BashTaskRead {
delta: string
/** True when truncation dropped unread bytes the delta cannot include. */
lossy: boolean
/** Full stdout spill file, when stdout truncation occurred. */
/** Full stdout spill file, when stdout truncation occurred and a safe path is available. */
stdoutSpillPath?: string
/** Full stderr spill file, when stderr truncation occurred. */
/** Full stderr spill file, when stderr truncation occurred and a safe path is available. */
stderrSpillPath?: string
}

View File

@@ -18,11 +18,11 @@ Requires a loaded executor implementation (e.g. `@deepseek-ai/dsh-bash-local`);
`command`, `workdir`, and `timeoutMs` are resolved against the executor's config defaults via `ctx.bash.resolve()` before execution, so the executor seam (`BashExecSpec`) receives explicit `workdir`/`timeoutMs` values. The workdir default is applied in the tool layer (from the calling agent's `session.header.cwd`) BEFORE `resolve()` — the per-session cwd must come from `exec.agent`, since N sessions share one executor; only when no session cwd is available does the executor fall back to its own config / `process.cwd()`.
Result text: stdout, then a `[stderr]` section, then status markers — `[timed out after Nms]` whenever the executor's timer fired (reported independently of how the process ended, so a command that traps SIGTERM and exits 0 still shows it), `[killed by signal: …]` for a signal death, `[exit code: N]` for a non-zero exit (reported, **not** `isError`: the model decides how to react), and `[output truncated; full output: <path>]` when the tail was kept. Only infrastructure failures (spawn errors, aborts) surface as `isError` results.
Result text: stdout, then a `[stderr]` section, then status markers — `[timed out after Nms]` whenever the executor's timer fired (reported independently of how the process ended, so a command that traps SIGTERM and exits 0 still shows it), `[killed by signal: …]` for a signal death, `[exit code: N]` for a non-zero exit (reported, **not** `isError`: the model decides how to react), and `[output truncated; full output: <path>]` when the tail was kept and a safe spill file is available. If the executor knows output was dropped but cannot safely advertise a complete spill file, the path is reported as `(unavailable)`. Only infrastructure failures (spawn errors, aborts) surface as `isError` results.
### `bash_output`
`task_id` → output produced **since the previous `bash_output` call** plus a status line (`running` / `completed, exit code: N` / `killed`). Reads that lost data to buffer bounds say so and point at the full-output spill file.
`task_id` → output produced **since the previous `bash_output` call** plus a status line (`running` / `completed, exit code: N` / `killed`). Reads that lost data to buffer bounds say so and point at the full-output spill file when one is safely available, otherwise `(unavailable)`.
### `bash_kill`

View File

@@ -316,7 +316,7 @@ export function apply(ctx: Context): void {
description: 'Execute a bash command (`bash -c`) and return its stdout/stderr. '
+ 'Each call runs in a fresh shell: no state (cwd, variables, functions) persists between calls — '
+ 'pass `workdir` instead of using `cd`. Non-zero exits are reported as `[exit code: N]`. '
+ 'Long output is truncated to its tail; the full output is saved to a file whose path is reported. '
+ 'Long output is truncated to its tail; the full output is saved to a file whose path is reported when available. '
+ 'Set `run_in_background: true` for long-running commands: the call returns a task id immediately; '
+ 'poll it with `bash_output` and stop it with `bash_kill`.',
parameters: {
@@ -377,7 +377,8 @@ export function apply(ctx: Context): void {
let text = read.delta.length > 0 ? read.delta : '(no new output)'
if (read.lossy) {
const paths = [read.stdoutSpillPath, read.stderrSpillPath].filter((p): p is string => p !== undefined)
text += `\n[some output was dropped from memory; full output: ${paths.join(', ')}]`
const fullOutput = paths.length > 0 ? paths.join(', ') : '(unavailable)'
text += `\n[some output was dropped from memory; full output: ${fullOutput}]`
}
text += `\n${statusLine(read.task)}`
return Promise.resolve([{ type: 'text', text }])

View File

@@ -4,6 +4,8 @@ import { join } from 'node:path'
import { describe, expect, it, vi } from 'vitest'
import { Context } from 'cordis'
import { CallId } from '@deepseek-ai/dsh-llm'
import { BashExecutor } from '@deepseek-ai/dsh-bash'
import type { BashExecRequest, BashExecSpec, BashRunResult, BashTask, BashTaskRead } from '@deepseek-ai/dsh-bash'
import SystemPrompt from '@deepseek-ai/dsh-system-prompt'
import ToolRegistry from '@deepseek-ai/dsh-tools'
import { LocalBashExecutor } from '@deepseek-ai/dsh-bash-local'
@@ -31,6 +33,51 @@ function text(result: { content: { type: string; text?: string }[] }): string {
return result.content.filter(block => block.type === 'text').map(block => block.text).join('')
}
class LossyReadBashExecutor extends BashExecutor {
private readonly task: BashTask = {
id: 'bash-lossy',
command: 'fake',
status: 'running',
exitCode: null,
signal: null,
done: Promise.resolve(),
}
resolve(request: BashExecRequest): BashExecSpec {
return {
command: request.command,
workdir: request.workdir ?? process.cwd(),
timeoutMs: request.timeoutMs ?? 0,
...request.signal ? { signal: request.signal } : {},
}
}
run(): Promise<BashRunResult> {
return Promise.reject(new Error('not used'))
}
start(): BashTask {
return this.task
}
get(id: string): BashTask | undefined {
return id === this.task.id ? this.task : undefined
}
list(): BashTask[] {
return [this.task]
}
readOutput(id: string): BashTaskRead {
if (id !== this.task.id) throw new Error(`unknown bash task "${id}"`)
return { task: this.task, delta: 'tail', lossy: true }
}
kill(): boolean {
return false
}
}
describe('bash tool', () => {
it('returns stdout for a successful command', async () => {
const ctx = await setup()
@@ -226,6 +273,17 @@ describe('background tools', () => {
expect(text(read)).toContain('[some output was dropped from memory; full output: ')
})
it('bash_output reports unavailable when a lossy read has no safe spill path', async () => {
const ctx = new Context()
await ctx.plugin(SystemPrompt)
await ctx.plugin(ToolRegistry)
await ctx.plugin(LossyReadBashExecutor)
await ctx.plugin(ToolBash)
const read = await call(ctx, 'bash_output', { task_id: 'bash-lossy' })
expect(text(read)).toBe('tail\n[some output was dropped from memory; full output: (unavailable)]\n[status: running]')
})
it('bash_kill stops a running task; repeat reports already-finished', async () => {
const ctx = await setup()
const started = await call(ctx, 'bash', { command: 'sleep 60', description: 'test command', run_in_background: true })