From 49bb6a88ebb76a85c6ad0807db5c1c7a3f8f6337 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 01:54:57 +0800 Subject: [PATCH] fix(tool-bash): handle unavailable spill paths --- .../2026-06-11-content-block-vocabulary.md | 2 +- ...18-shared-persistence-write-coordinator.md | 2 +- packages/bash-local/README.md | 2 +- packages/bash/src/types.ts | 6 +- packages/tool-bash/README.md | 4 +- packages/tool-bash/src/index.ts | 5 +- packages/tool-bash/tests/tools.spec.ts | 58 +++++++++++++++++++ 7 files changed, 69 insertions(+), 10 deletions(-) diff --git a/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md b/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md index 81ae260d53..a3ef1c3202 100644 --- a/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md +++ b/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md @@ -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 diff --git a/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md b/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md index f213286783..06bbe1a93d 100644 --- a/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md +++ b/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md @@ -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 diff --git a/packages/bash-local/README.md b/packages/bash-local/README.md index cceefbb67c..c9f3c432e3 100644 --- a/packages/bash-local/README.md +++ b/packages/bash-local/README.md @@ -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. diff --git a/packages/bash/src/types.ts b/packages/bash/src/types.ts index 1860401ee2..cfa60ab331 100644 --- a/packages/bash/src/types.ts +++ b/packages/bash/src/types.ts @@ -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 } diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index a3d6821371..9a7102ce4c 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -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: ]` 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: ]` 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` diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 6721b70f7c..d25bb6060b 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -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 }]) diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index cb54a8a60f..47c7ae2a8e 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -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 { + 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 })