فهرست منبع

fix(dev): make hooks and bash seams safer

Tianyi Cui 3 ماه پیش
والد
کامیت
513e16f9dc

+ 2 - 2
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:

+ 1 - 1
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",

+ 10 - 0
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<Omit<Config, 'cwd'>> & Pick<Config, 'cwd'>
 
+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,

+ 10 - 0
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 }))

+ 1 - 1
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.

+ 2 - 2
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)
 

+ 2 - 2
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.' },
     },

+ 13 - 0
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)