From 513e16f9dced1d5590463a68302cd59fd5427b91 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:26:44 +0800 Subject: [PATCH] fix(dev): make hooks and bash seams safer --- docs/development.md | 4 ++-- package.json | 2 +- packages/bash-local/src/index.ts | 10 ++++++++++ packages/bash-local/tests/executor.spec.ts | 10 ++++++++++ packages/bash/README.md | 2 +- packages/tool-bash/README.md | 4 ++-- packages/tool-bash/src/index.ts | 4 ++-- scripts/install-lefthook.mjs | 13 +++++++++++++ 8 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 scripts/install-lefthook.mjs diff --git a/docs/development.md b/docs/development.md index 144860a4ff..45d23912ca 100644 --- a/docs/development.md +++ b/docs/development.md @@ -17,12 +17,12 @@ Install dependencies from the repo root: pnpm install ``` -The install also runs the root `postinstall` script, which installs lefthook from the repo dev dependency. +The install also runs the root `postinstall` script, which installs lefthook from the repo dev dependency through `scripts/install-lefthook.mjs`; the wrapper uses lefthook's reviewed `--force` mode so linked worktrees with an existing `core.hooksPath` do not fail normal `pnpm run …` commands. If hooks are missing because dependencies were restored from cache or `postinstall` was skipped, install them manually: ```sh -pnpm exec lefthook install +pnpm exec lefthook install --force ``` Run typecheck once after a fresh clone: diff --git a/package.json b/package.json index 8f3c5fc946..227c69c684 100644 --- a/package.json +++ b/package.json @@ -32,7 +32,7 @@ "demo:echo": "node --expose-internals --import tsx examples/echo-agent/start.ts", "demo:coding": "node --expose-internals --import tsx examples/coding-agent/start.ts", "demo:acp": "node --expose-internals --import tsx examples/acp-agent/start.ts", - "postinstall": "lefthook install" + "postinstall": "node scripts/install-lefthook.mjs" }, "devDependencies": { "@agentclientprotocol/sdk": "0.25.1", diff --git a/packages/bash-local/src/index.ts b/packages/bash-local/src/index.ts index 1169160614..7320276a1a 100644 --- a/packages/bash-local/src/index.ts +++ b/packages/bash-local/src/index.ts @@ -38,6 +38,12 @@ export interface Config { /** The shape after schemastery applied the defaults (cwd has none). */ type ResolvedConfig = Required> & Pick +function assertPositiveFinite(name: string, value: number): void { + if (!Number.isFinite(value) || value <= 0) { + throw new Error(`bash-local: ${name} must be a positive finite number`) + } +} + interface TrackedTask extends BashTask { running: RunningBash /** Whole-stream byte offsets already delivered via {@link LocalBashExecutor.readOutput}. */ @@ -72,6 +78,9 @@ export class LocalBashExecutor extends BashExecutor { // schemastery (static Config) has already filled the defaulted fields; // the cast records that runtime fact for exactOptionalPropertyTypes. this.config = config as ResolvedConfig + assertPositiveFinite('timeoutMs', this.config.timeoutMs) + assertPositiveFinite('maxTimeoutMs', this.config.maxTimeoutMs) + assertPositiveFinite('maxOutputBytes', this.config.maxOutputBytes) 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 @@ -98,6 +107,7 @@ export class LocalBashExecutor extends BashExecutor { * values and never re-default. */ resolve(request: BashExecRequest): BashExecSpec { + if (request.timeoutMs !== undefined) assertPositiveFinite('request.timeoutMs', request.timeoutMs) const timeoutMs = Math.min(request.timeoutMs ?? this.config.timeoutMs, this.config.maxTimeoutMs) return { command: request.command, diff --git a/packages/bash-local/tests/executor.spec.ts b/packages/bash-local/tests/executor.spec.ts index 72383d0427..3dd7f7983a 100644 --- a/packages/bash-local/tests/executor.spec.ts +++ b/packages/bash-local/tests/executor.spec.ts @@ -59,6 +59,16 @@ describe('LocalBashExecutor.run', () => { expect(result.timeoutMs).toBe(2_000) }) + it('rejects invalid numeric config and timeout overrides', async () => { + 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/) + + 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('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 })) diff --git a/packages/bash/README.md b/packages/bash/README.md index 12976d551b..8de529fee1 100644 --- a/packages/bash/README.md +++ b/packages/bash/README.md @@ -27,4 +27,4 @@ Implementations subclass `BashExecutor`, implement the abstract methods, and cal ## Vocabulary -`BashExecSpec` (command, workdir?, timeoutMs?, signal?) → `BashRunResult` (exitCode, signal, timedOut, aborted, timeoutMs, stdout/stderr as `CollectedOutput`) and `BashTask`/`BashTaskRead` for the background side. See `src/types.ts` for the full contracts. +`BashExecRequest` (command, workdir?, timeoutMs?, signal?) resolves to `BashExecSpec` (command, workdir, timeoutMs, signal?) before execution; `run()` returns `BashRunResult` (exitCode, signal, timedOut, aborted, timeoutMs, stdout/stderr as `CollectedOutput`) and `start()`/`readOutput()` use `BashTask`/`BashTaskRead` for the background side. See `src/types.ts` for the full contracts. diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 83f84e967d..e60e52542b 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -12,7 +12,7 @@ Requires a loaded executor implementation (e.g. `@deepseek-ai/dsh-bash-local`); |---|---|---| | `command` | string (required) | Run via `bash -c`. No state persists between calls — use `workdir`, not `cd`. | | `description` | string (required) | One-line, active-voice summary of the command (5-10 words), for UI/log display only — no effect on execution. | -| `timeoutMs` | number | Default/max from executor config (120s/600s for bash-local). | +| `timeoutMs` | number | Timeout override in milliseconds. The executor applies its configured default and cap. | | `workdir` | string | Working directory for this call. Defaults to the calling agent's session cwd (`session.header.cwd`) so each session runs in its own workspace; a relative `workdir` is resolved against that session cwd. | | `run_in_background` | boolean | Return a task id immediately; no timeout applies. | @@ -26,7 +26,7 @@ Result text: stdout, then a `[stderr]` section, then status markers — `[timed ### `bash_kill` -`task_id` → SIGTERM→SIGKILL on the task's process group. Killing an already-finished task is a reported no-op; unknown ids are errors. +`task_id` → ask the executor to kill the background task. The concrete executor decides how to signal or stop the process; killing an already-finished task is a reported no-op, and unknown ids are errors. ### Task ownership (cross-session isolation) diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index f4b0559a0b..33a97ddf93 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -213,7 +213,7 @@ export function apply(ctx: Context): void { + '5-10 words (shown in the UI). Examples: "ls" → "List files in current directory"; ' + '"git status" → "Show working tree status"; "npm install" → "Install package dependencies".', }, - timeoutMs: { type: 'number', description: 'Timeout in milliseconds (default 120000, max 600000). The command is killed on expiry.' }, + timeoutMs: { type: 'number', description: 'Timeout in milliseconds. The executor applies its configured default and cap, and kills the command on expiry.' }, workdir: { type: 'string', description: 'Working directory for this command. Defaults to the session workspace; a relative path is resolved against it.' }, run_in_background: { type: 'boolean', description: 'Run in the background and return a task id immediately. No timeout applies.' }, }, @@ -269,7 +269,7 @@ export function apply(ctx: Context): void { ctx.tools.register(defineTool({ name: 'bash_kill', - description: 'Kill a running background bash task (SIGTERM, then SIGKILL) by task id.', + description: 'Ask the executor to kill a running background bash task by task id.', parameters: { task_id: { type: 'string', required: true, description: 'Task id returned by the bash tool.' }, }, diff --git a/scripts/install-lefthook.mjs b/scripts/install-lefthook.mjs new file mode 100644 index 0000000000..be81daa153 --- /dev/null +++ b/scripts/install-lefthook.mjs @@ -0,0 +1,13 @@ +#!/usr/bin/env node +import { existsSync } from 'node:fs' +import { spawnSync } from 'node:child_process' +import { join } from 'node:path' + +const git = spawnSync('git', ['rev-parse', '--git-dir'], { stdio: 'ignore' }) +if (git.status !== 0) process.exit(0) + +const lefthook = join(process.cwd(), 'node_modules', '.bin', process.platform === 'win32' ? 'lefthook.cmd' : 'lefthook') +if (!existsSync(lefthook)) process.exit(0) + +const result = spawnSync(lefthook, ['install', '--force'], { stdio: 'inherit' }) +process.exit(result.status ?? 1)