From b062b50069424693da9e1191793c69a98890caab Mon Sep 17 00:00:00 2001 From: EmersonBraun Date: Tue, 25 Aug 2026 22:32:25 -0300 Subject: [PATCH] feat: harden local CLI worker --- CHANGELOG.md | 1 + README.md | 1 + docs/OPERATIONS.md | 2 + llms-full.txt | 4 + readme-standard-v1.json | 2 +- src/claude-code-adapter.ts | 19 ++-- src/cli.ts | 5 +- src/codex-adapter.ts | 21 +++-- src/local-cli-process.ts | 160 ++++++++++++++++++++++---------- src/review-config.ts | 8 ++ test/local-cli-process.test.mjs | 63 +++++++++++++ test/review-config.test.mjs | 1 + 12 files changed, 217 insertions(+), 70 deletions(-) create mode 100644 test/local-cli-process.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 30a5ebb..0d96423 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes will be documented here. This project follows Semantic Versi ### Changed - Added strict versioned `.agentskit-review.json` policy with lens coverage, budgets, thresholds, context, and safe CI precedence; incomplete profiles require explicit local opt-in. +- Hardened the shared local CLI worker with cancellation, process-tree cleanup, isolated temporary environments, bounded output, and redacted diagnostics. - Made reviews fail closed when any reviewable file has no successful primary lens or cannot be ingested; advisory mode now suppresses finding-based failures only, never source/provider/execution failures. - Added primary-lens execution coverage to review summaries so partial provider degradation is visible. - Repositioned the CLI and GitHub Action as provider-neutral. diff --git a/README.md b/README.md index e972547..83ccc65 100644 --- a/README.md +++ b/README.md @@ -286,6 +286,7 @@ never accepted in CI. }, "votes": 3, "budget": { "maxFiles": 20, "maxCalls": 200, "concurrency": 1 }, + "worker": { "timeoutMs": 120000, "maxOutputBytes": 20971520 }, "thresholds": { "minSeverity": "med", "minConfidence": 0.7 }, "context": { "mode": "prompt", "patterns": ["src/**"] } } diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index b418fe9..2036bc4 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -43,6 +43,8 @@ It requires `configVersion: 1` and supports lens policy (`enabled` and `required` per built-in lens), votes, retries, thresholds, file/byte/call and concurrency budgets, conventions, and context selection. All built-in lenses are enabled by default; correctness, security, and tests are required. +The shared local worker also accepts bounded `timeoutMs` and `maxOutputBytes` +settings; absolute ceilings are always enforced. Flags override file values. The file cannot contain credentials or executable plugins. Provider, model, transport, trust mode, redaction, permissions, and diff --git a/llms-full.txt b/llms-full.txt index 67ceb83..8fc81ba 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -304,6 +304,7 @@ never accepted in CI. }, "votes": 3, "budget": { "maxFiles": 20, "maxCalls": 200, "concurrency": 1 }, + "worker": { "timeoutMs": 120000, "maxOutputBytes": 20971520 }, "thresholds": { "minSeverity": "med", "minConfidence": 0.7 }, "context": { "mode": "prompt", "patterns": ["src/**"] } } @@ -432,6 +433,8 @@ It requires `configVersion: 1` and supports lens policy (`enabled` and `required` per built-in lens), votes, retries, thresholds, file/byte/call and concurrency budgets, conventions, and context selection. All built-in lenses are enabled by default; correctness, security, and tests are required. +The shared local worker also accepts bounded `timeoutMs` and `maxOutputBytes` +settings; absolute ceilings are always enforced. Flags override file values. The file cannot contain credentials or executable plugins. Provider, model, transport, trust mode, redaction, permissions, and @@ -795,6 +798,7 @@ All notable changes will be documented here. This project follows Semantic Versi ### Changed - Added strict versioned `.agentskit-review.json` policy with lens coverage, budgets, thresholds, context, and safe CI precedence; incomplete profiles require explicit local opt-in. +- Hardened the shared local CLI worker with cancellation, process-tree cleanup, isolated temporary environments, bounded output, and redacted diagnostics. - Made reviews fail closed when any reviewable file has no successful primary lens or cannot be ingested; advisory mode now suppresses finding-based failures only, never source/provider/execution failures. - Added primary-lens execution coverage to review summaries so partial provider degradation is visible. - Repositioned the CLI and GitHub Action as provider-neutral. diff --git a/readme-standard-v1.json b/readme-standard-v1.json index dd9316d..0c40267 100644 --- a/readme-standard-v1.json +++ b/readme-standard-v1.json @@ -221,7 +221,7 @@ "docs/OPERATIONS.md", "test/cli-smoke.test.mjs" ], - "sourceHash": "sha256:1a2be3167c85699211c0b9ba45b870d8d88d0841a5d8d99b2c4bfe211a2b22fc" + "sourceHash": "sha256:23a5ee0d6e4a39e12a8fa1e0b05df4f470b2dd2904968ce96b45894e0263f7e9" }, "exceptions": [] } diff --git a/src/claude-code-adapter.ts b/src/claude-code-adapter.ts index 8c94694..9cae1be 100644 --- a/src/claude-code-adapter.ts +++ b/src/claude-code-adapter.ts @@ -6,7 +6,7 @@ * args and synthesize the `tool_call` stream chunk the runtime expects. */ import type { AdapterFactory, AdapterRequest, StreamChunk, StreamSource } from "@agentskit/core"; -import { runLocalCli } from "./local-cli-process.js"; +import { runLocalCli, type LocalCliMode } from "./local-cli-process.js"; /** * Run `claude` and capture stdout. Crucially we CLOSE the child's stdin: in a @@ -14,11 +14,11 @@ import { runLocalCli } from "./local-cli-process.js"; * stdin ("no stdin data received in 3s …") and fails. Locally stdin is a TTY so it * never showed. stderr is attached to the error for diagnosis. */ -async function runClaude(args: string[]): Promise { +async function runClaude(args: string[], signal?: AbortSignal, mode?: LocalCliMode, worker?: { timeoutMs?: number; maxOutputBytes?: number }): Promise { // Run from HOME (a trusted dir): the runner's checkout dir is untrusted and can // make claude exit without output (folder-trust). The file under review is in the // prompt, not read from cwd, so cwd is irrelevant to the result. - const { stdout } = await runLocalCli("claude", args, { cwd: process.env.HOME }); + const { stdout } = await runLocalCli("claude", args, { signal, mode, ...worker }); return stdout; } @@ -36,10 +36,12 @@ function extractJson(text: string): string { return text.slice(start, end + 1); } -export function claudeCode(opts: { model?: string } = {}): AdapterFactory { +export function claudeCode(opts: { model?: string; mode?: LocalCliMode; worker?: { timeoutMs?: number; maxOutputBytes?: number } } = {}): AdapterFactory { return { capabilities: { streaming: false, tools: true, structuredOutput: true }, - createSource: (request: AdapterRequest): StreamSource => ({ + createSource: (request: AdapterRequest): StreamSource => { + const controller = new AbortController(); + return { stream: async function* (): AsyncIterableIterator { try { const system = request.messages.find((m) => m.role === "system")?.content ?? ""; @@ -57,7 +59,7 @@ export function claudeCode(opts: { model?: string } = {}): AdapterFactory { const args = ["-p", prompt]; if (opts.model) args.push("--model", opts.model); - const out = (await runClaude(args)).trim(); + const out = (await runClaude(args, controller.signal, opts.mode, opts.worker)).trim(); if (tools.length === 1) { yield { type: "tool_call", toolCall: { id: `tc-${Date.now()}`, name: tools[0]!.name, args: extractJson(out) } }; @@ -73,7 +75,8 @@ export function claudeCode(opts: { model?: string } = {}): AdapterFactory { yield { type: "error", content: `claude -p failed${detail ? `: ${detail.slice(0, 400)}` : ` (no output): ${(e.message ?? "").split("\n")[0]}`}` }; } }, - abort: () => {}, - }), + abort: () => controller.abort(), + }; + }, }; } diff --git a/src/cli.ts b/src/cli.ts index 8818bf7..ad86c4d 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -184,8 +184,9 @@ function buildAdapter(reviewConfig: ResolvedReviewConfig): AdapterFactory { const provider = requestedProvider && resolveProviderId(requestedProvider) if (!provider) throw new Error(requestedProvider ? `unknown --provider "${requestedProvider}" (run --list-providers for common options)` : 'choose a provider with --provider (run --list-providers for common options)') const model = reviewConfig.model ?? (has('api') ? 'claude-opus-4-8' : undefined) - if (provider === 'claude-cli') return claudeCode({ model }) - if (provider === 'codex-cli') return codexCli({ model }) + const mode = flag('mode') === 'trusted-local' ? 'trusted-local' : 'isolated' + if (provider === 'claude-cli') return claudeCode({ model, mode, worker: reviewConfig.worker }) + if (provider === 'codex-cli') return codexCli({ model, mode, worker: reviewConfig.worker }) if (provider === 'ollama') { if (!model) throw new Error('--model is required for provider "ollama"') return ollamaReview({ model, ...(flag('base-url') ? { baseUrl: flag('base-url') } : {}) }) diff --git a/src/codex-adapter.ts b/src/codex-adapter.ts index 6fe376a..6e7b48e 100644 --- a/src/codex-adapter.ts +++ b/src/codex-adapter.ts @@ -9,7 +9,7 @@ import { mkdtempSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { AdapterFactory, AdapterRequest, StreamChunk, StreamSource } from "@agentskit/core"; -import { runLocalCli } from "./local-cli-process.js"; +import { runLocalCli, type LocalCliMode } from "./local-cli-process.js"; /** Pull the JSON object out of a reply: first `{` to last `}` over the whole output. */ function extractJson(text: string): string { @@ -22,7 +22,7 @@ function extractJson(text: string): string { } /** Run `codex exec` and return its final message (captured via -o). */ -async function runCodex(prompt: string, model?: string): Promise { +async function runCodex(prompt: string, model?: string, signal?: AbortSignal, mode?: LocalCliMode, worker?: { timeoutMs?: number; maxOutputBytes?: number }): Promise { const dir = mkdtempSync(join(tmpdir(), "cr-codex-")); const outFile = join(dir, "out.txt"); const args = [ @@ -30,8 +30,6 @@ async function runCodex(prompt: string, model?: string): Promise { "--skip-git-repo-check", "-s", "read-only", - "-C", - process.env.HOME ?? process.cwd(), "-o", outFile, ]; @@ -39,17 +37,19 @@ async function runCodex(prompt: string, model?: string): Promise { args.push(prompt); try { - await runLocalCli("codex", args); + await runLocalCli("codex", args, { signal, mode, ...worker }); return readFileSync(outFile, "utf8"); } finally { rmSync(dir, { recursive: true, force: true }); } } -export function codexCli(opts: { model?: string } = {}): AdapterFactory { +export function codexCli(opts: { model?: string; mode?: LocalCliMode; worker?: { timeoutMs?: number; maxOutputBytes?: number } } = {}): AdapterFactory { return { capabilities: { streaming: false, tools: true, structuredOutput: true }, - createSource: (request: AdapterRequest): StreamSource => ({ + createSource: (request: AdapterRequest): StreamSource => { + const controller = new AbortController(); + return { stream: async function* (): AsyncIterableIterator { try { const system = request.messages.find((m) => m.role === "system")?.content ?? ""; @@ -65,7 +65,7 @@ export function codexCli(opts: { model?: string } = {}): AdapterFactory { prompt += `\n\nReturn ONLY a JSON object that is the argument to the "${t.name}" tool, matching this JSON Schema exactly. No prose, no code fences:\n${JSON.stringify(t.schema)}`; } - const out = (await runCodex(prompt, opts.model)).trim(); + const out = (await runCodex(prompt, opts.model, controller.signal, opts.mode, opts.worker)).trim(); if (tools.length === 1) { yield { type: "tool_call", toolCall: { id: `tc-${Date.now()}`, name: tools[0]!.name, args: extractJson(out) } }; @@ -79,7 +79,8 @@ export function codexCli(opts: { model?: string } = {}): AdapterFactory { yield { type: "error", content: `codex exec failed${detail ? `: ${detail.slice(0, 400)}` : ` (no output): ${(e.message ?? "").split("\n")[0]}`}` }; } }, - abort: () => {}, - }), + abort: () => controller.abort(), + }; + }, }; } diff --git a/src/local-cli-process.ts b/src/local-cli-process.ts index 09cf3fb..40bce08 100644 --- a/src/local-cli-process.ts +++ b/src/local-cli-process.ts @@ -1,94 +1,156 @@ +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs' import { spawn, type ChildProcess } from 'node:child_process' -import { localCliTimeoutMs } from './local-cli-timeout.js' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { DEFAULT_LOCAL_CLI_TIMEOUT_MS, localCliTimeoutMs } from './local-cli-timeout.js' -const MAX_BUFFER = 20 * 1024 * 1024 +export { DEFAULT_LOCAL_CLI_TIMEOUT_MS } + +export const DEFAULT_LOCAL_CLI_OUTPUT_BYTES = 20 * 1024 * 1024 +export const ABSOLUTE_LOCAL_CLI_OUTPUT_BYTES = 25 * 1024 * 1024 +export const ABSOLUTE_LOCAL_CLI_TIMEOUT_MS = 10 * 60 * 1000 type LocalCliError = Error & { code?: string; stderr?: string; stdout?: string } +export type LocalCliMode = 'isolated' | 'trusted-local' + +export interface LocalCliOptions { + readonly cwd?: string + readonly timeoutMs?: number + readonly maxOutputBytes?: number + readonly signal?: AbortSignal + readonly mode?: LocalCliMode + /** Explicitly selected provider credential; arbitrary project env is never copied in isolated mode. */ + readonly providerCredential?: { readonly name: string; readonly value: string } +} + +const SECRET_PATTERNS = [ + /(?:sk|pk)-[A-Za-z0-9_-]{16,}/g, + /(?:ghp|gho|ghs|ghr|github_pat)_[A-Za-z0-9_]{16,}/g, + /xox[baprs]-[A-Za-z0-9-]{12,}/g, + /\bAKIA[0-9A-Z]{16}\b/g, + /\bBearer\s+[A-Za-z0-9._~+/=-]{12,}/gi, + /-----BEGIN [^-]*PRIVATE KEY-----[\s\S]*?-----END [^-]*PRIVATE KEY-----/g, +] + +export function redactDiagnostic(value: string, secrets: readonly string[] = []): string { + let redacted = value + for (const secret of secrets.filter(Boolean)) redacted = redacted.split(secret).join('[REDACTED]') + for (const pattern of SECRET_PATTERNS) redacted = redacted.replace(pattern, '[REDACTED]') + return redacted.slice(0, 4000) +} function terminateProcessTree(child: ChildProcess): void { if (!child.pid) return - if (process.platform === 'win32') { spawn('taskkill', ['/pid', String(child.pid), '/t', '/f'], { stdio: 'ignore' }) return } + try { process.kill(-child.pid, 'SIGKILL') } catch { child.kill('SIGKILL') } +} - try { - process.kill(-child.pid, 'SIGKILL') - } catch { - child.kill('SIGKILL') +function createEnvironment(mode: LocalCliMode, credential?: LocalCliOptions['providerCredential']): { env: NodeJS.ProcessEnv; tempRoot?: string } { + if (mode === 'trusted-local') return { env: { ...process.env } } + const tempRoot = mkdtempSync(join(tmpdir(), 'agentskit-review-worker-')) + const home = join(tempRoot, 'home') + const temp = join(tempRoot, 'tmp') + mkdirSync(home) + mkdirSync(temp) + const env: NodeJS.ProcessEnv = { + PATH: process.env.PATH ?? '', LANG: process.env.LANG, LC_ALL: process.env.LC_ALL, CI: process.env.CI, + HOME: home, TMPDIR: temp, TEMP: temp, TMP: temp, SystemRoot: process.env.SystemRoot, } + // Offline fixtures use non-secret CODEX_FIXTURE_* switches to exercise failure paths. + // No general project or user environment is inherited by isolated workers. + for (const [name, value] of Object.entries(process.env)) if (name.startsWith('CODEX_FIXTURE_')) env[name] = value + if (credential) env[credential.name] = credential.value + return { env, tempRoot } +} + +function boundedAppend(current: string, chunk: string, limit: number): { value: string; overflow: boolean } { + const next = current + chunk + if (Buffer.byteLength(next, 'utf8') <= limit) return { value: next, overflow: false } + return { value: Buffer.from(next, 'utf8').subarray(0, limit).toString('utf8'), overflow: true } } -export function runLocalCli( - command: string, - args: string[], - options: { readonly cwd?: string; readonly timeoutMs?: number } = {}, -): Promise<{ readonly stdout: string; readonly stderr: string }> { +export function runLocalCli(command: string, args: string[], options: LocalCliOptions = {}): Promise<{ readonly stdout: string; readonly stderr: string }> { const timeoutMs = options.timeoutMs ?? localCliTimeoutMs() + const maxOutputBytes = options.maxOutputBytes ?? DEFAULT_LOCAL_CLI_OUTPUT_BYTES + const mode = options.mode ?? 'isolated' + if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > ABSOLUTE_LOCAL_CLI_TIMEOUT_MS) return Promise.reject(new Error(`timeout must be between 1 and ${ABSOLUTE_LOCAL_CLI_TIMEOUT_MS}ms`)) + if (!Number.isSafeInteger(maxOutputBytes) || maxOutputBytes < 1 || maxOutputBytes > ABSOLUTE_LOCAL_CLI_OUTPUT_BYTES) return Promise.reject(new Error(`maxOutputBytes must be between 1 and ${ABSOLUTE_LOCAL_CLI_OUTPUT_BYTES}`)) + if (options.signal?.aborted) return Promise.reject(Object.assign(new Error(`${command} aborted before start`), { code: 'ABORT_ERR' })) return new Promise((resolve, reject) => { + const { env, tempRoot } = createEnvironment(mode, options.providerCredential) const child = spawn(command, args, { - cwd: options.cwd, - detached: process.platform !== 'win32', - stdio: ['pipe', 'pipe', 'pipe'], + cwd: options.cwd ?? (mode === 'trusted-local' ? process.env.HOME : join(tempRoot!, 'home')), env, + detached: process.platform !== 'win32', stdio: ['pipe', 'pipe', 'pipe'], }) let stdout = '' let stderr = '' let timedOut = false + let aborted = false + let parentShutdown = false let failureReason: Error | undefined let timeout: NodeJS.Timeout | undefined let settled = false + const secrets = options.providerCredential ? [options.providerCredential.value] : [] + const cleanup = () => { + if (timeout) clearTimeout(timeout) + options.signal?.removeEventListener('abort', onAbort) + process.removeListener('SIGINT', onParentShutdown) + process.removeListener('SIGTERM', onParentShutdown) + if (tempRoot) rmSync(tempRoot, { recursive: true, force: true }) + } const finishError = (error: Error): void => { if (settled) return settled = true - if (timeout) clearTimeout(timeout) + cleanup() const failure = error as LocalCliError - if (timedOut) { - failure.code = 'ETIMEDOUT' - failure.message = `${command} timed out after ${timeoutMs}ms` - } - failure.stdout = stdout - failure.stderr = stderr + if (timedOut) { failure.code = 'ETIMEDOUT'; failure.message = `${command} timed out after ${timeoutMs}ms` } + else if (aborted) { failure.code = 'ABORT_ERR'; failure.message = `${command} aborted` } + else if (parentShutdown) { failure.code = 'PARENT_SHUTDOWN'; failure.message = `${command} stopped because the parent process is shutting down` } + failure.stdout = redactDiagnostic(stdout, secrets) + failure.stderr = redactDiagnostic(stderr, secrets) reject(failure) } + const stop = (reason: Error) => { + if (settled || failureReason) return + failureReason = reason + terminateProcessTree(child) + } + const onAbort = () => { aborted = true; stop(new Error(`${command} aborted`)) } + const onParentShutdown = (signal: NodeJS.Signals) => { + parentShutdown = true + stop(new Error(`${command} stopped because the parent process received ${signal}`)) + setImmediate(() => process.kill(process.pid, signal)) + } child.stdout.setEncoding('utf8') child.stderr.setEncoding('utf8') child.stdout.on('data', (chunk: string) => { - stdout += chunk - if (stdout.length > MAX_BUFFER && !failureReason) { - failureReason = new Error(`${command} stdout exceeded ${MAX_BUFFER} bytes`) - terminateProcessTree(child) - } + const next = boundedAppend(stdout, chunk, maxOutputBytes) + stdout = next.value + if (next.overflow) stop(new Error(`${command} stdout exceeded ${maxOutputBytes} bytes`)) }) child.stderr.on('data', (chunk: string) => { - stderr += chunk - if (stderr.length > MAX_BUFFER && !failureReason) { - failureReason = new Error(`${command} stderr exceeded ${MAX_BUFFER} bytes`) - terminateProcessTree(child) - } + const next = boundedAppend(stderr, chunk, maxOutputBytes) + stderr = next.value + if (next.overflow) stop(new Error(`${command} stderr exceeded ${maxOutputBytes} bytes`)) }) child.once('error', (error) => finishError(error)) child.once('close', (code, signal) => { - if (failureReason) { - finishError(failureReason) - } else if (timedOut) { - finishError(new Error(`${command} terminated with signal ${signal ?? 'unknown'}`)) - } else if (code === 0) { - settled = true - if (timeout) clearTimeout(timeout) - resolve({ stdout, stderr }) - } else { - finishError(new Error(`${command} exited with code ${code ?? 'unknown'}${signal ? ` (${signal})` : ''}`)) - } + if (failureReason) finishError(failureReason) + else if (timedOut) finishError(new Error(`${command} terminated with signal ${signal ?? 'unknown'}`)) + else if (aborted) finishError(new Error(`${command} aborted`)) + else if (parentShutdown) finishError(new Error(`${command} stopped because the parent process is shutting down`)) + else if (code === 0) { settled = true; cleanup(); resolve({ stdout, stderr }) } + else finishError(new Error(`${command} exited with code ${code ?? 'unknown'}${signal ? ` (${signal})` : ''}`)) }) + options.signal?.addEventListener('abort', onAbort, { once: true }) + process.once('SIGINT', onParentShutdown) + process.once('SIGTERM', onParentShutdown) child.stdin.end() - - timeout = setTimeout(() => { - if (settled) return - timedOut = true - terminateProcessTree(child) - }, timeoutMs) + timeout = setTimeout(() => { if (!settled) { timedOut = true; stop(new Error(`${command} timed out after ${timeoutMs}ms`)) } }, timeoutMs) }) } diff --git a/src/review-config.ts b/src/review-config.ts index 382eb90..f60f723 100644 --- a/src/review-config.ts +++ b/src/review-config.ts @@ -2,6 +2,8 @@ import { existsSync, readFileSync } from 'node:fs' import { join } from 'node:path' import { z } from 'zod' import type { Category, Severity } from '../agents/code-review/agent.js' +import { ABSOLUTE_LOCAL_CLI_OUTPUT_BYTES, ABSOLUTE_LOCAL_CLI_TIMEOUT_MS, DEFAULT_LOCAL_CLI_OUTPUT_BYTES } from './local-cli-process.js' +import { localCliTimeoutMs } from './local-cli-timeout.js' export const BUILTIN_LENS_KEYS = [ 'correctness', 'security', 'performance', 'maintainability', 'design', 'tests', 'conventions', @@ -39,6 +41,10 @@ const ReviewConfigSchema = z.object({ maxFiles: positiveInt.max(500).optional(), maxBytes: positiveInt.max(25 * 1024 * 1024).optional(), maxCalls: positiveInt.max(1000).optional(), concurrency: positiveInt.max(32).optional(), }).strict().optional(), + worker: z.object({ + timeoutMs: positiveInt.max(ABSOLUTE_LOCAL_CLI_TIMEOUT_MS).optional(), + maxOutputBytes: positiveInt.max(ABSOLUTE_LOCAL_CLI_OUTPUT_BYTES).optional(), + }).strict().optional(), conventions: relativePattern.optional(), context: z.object({ mode: z.enum(['prompt', 'isolated-snapshot']), patterns: z.array(relativePattern).max(100).optional() }).strict().optional(), provider: z.string().min(1).max(100).optional(), model: z.string().min(1).max(200).optional(), @@ -63,6 +69,7 @@ export interface ResolvedReviewConfig { retries: number thresholds: { minSeverity?: Severity; minConfidence?: number; maxPerFile?: number; suppressNits?: boolean } budget: { maxFiles?: number; maxBytes?: number; maxCalls?: number; concurrency: number } + worker: { timeoutMs: number; maxOutputBytes: number } conventions?: string context: { mode: 'prompt' | 'isolated-snapshot'; patterns: string[] } provider?: string; model?: string; transport?: 'api' | 'acp' | 'headless' | 'http' @@ -121,6 +128,7 @@ export function resolveReviewConfig( configVersion: 1 as const, lenses, incompleteProfile, votes: overrides.votes ?? file?.votes ?? 3, retries: overrides.retries ?? file?.retries ?? 1, thresholds, budget: { ...budget, concurrency: budget.concurrency ?? 4 }, + worker: { timeoutMs: file?.worker?.timeoutMs ?? localCliTimeoutMs(), maxOutputBytes: file?.worker?.maxOutputBytes ?? DEFAULT_LOCAL_CLI_OUTPUT_BYTES }, conventions: overrides.conventions ?? file?.conventions, context: { mode: file?.context?.mode ?? 'prompt', patterns: file?.context?.patterns ?? [] }, provider: overrides.provider ?? file?.provider, model: overrides.model ?? file?.model, diff --git a/test/local-cli-process.test.mjs b/test/local-cli-process.test.mjs new file mode 100644 index 0000000..49723e0 --- /dev/null +++ b/test/local-cli-process.test.mjs @@ -0,0 +1,63 @@ +import assert from 'node:assert/strict' +import test from 'node:test' +import { runLocalCli } from '../dist/src/local-cli-process.js' + +const node = process.execPath + +test('isolated workers receive temporary HOME/TMPDIR and only the selected credential', async () => { + process.env.AGENTSKIT_TEST_INHERITED = 'must-not-pass' + try { + const result = await runLocalCli(node, ['-e', 'process.stdout.write(JSON.stringify({ home: process.env.HOME, tmp: process.env.TMPDIR, cwd: process.cwd(), inherited: process.env.AGENTSKIT_TEST_INHERITED, key: process.env.TEST_PROVIDER_KEY }))'], { + providerCredential: { name: 'TEST_PROVIDER_KEY', value: 'selected-secret' }, + }) + const env = JSON.parse(result.stdout) + assert.notEqual(env.home, process.env.HOME) + assert.notEqual(env.tmp, process.env.TMPDIR) + assert.match(env.cwd, /agentskit-review-worker-[^/]+\/home$/) + assert.equal(env.inherited, undefined) + assert.equal(env.key, 'selected-secret') + } finally { delete process.env.AGENTSKIT_TEST_INHERITED } +}) + +test('trusted-local mode explicitly inherits the caller environment', async () => { + process.env.AGENTSKIT_TEST_INHERITED = 'trusted-value' + try { + const result = await runLocalCli(node, ['-e', 'process.stdout.write(process.env.AGENTSKIT_TEST_INHERITED ?? "")'], { mode: 'trusted-local' }) + assert.equal(result.stdout, 'trusted-value') + } finally { delete process.env.AGENTSKIT_TEST_INHERITED } +}) + +test('abort terminates a local worker and returns a stable error code', async () => { + const controller = new AbortController() + const pending = runLocalCli(node, ['-e', 'setInterval(() => {}, 1000)'], { signal: controller.signal, timeoutMs: 5000 }) + setTimeout(() => controller.abort(), 25) + await assert.rejects(pending, (error) => error?.code === 'ABORT_ERR' && error.message === `${node} aborted`) +}) + +test('output overflow stops the worker and never returns unbounded output', async () => { + const pending = runLocalCli(node, ['-e', "process.stdout.write('x'.repeat(100))"], { maxOutputBytes: 10 }) + await assert.rejects(pending, (error) => error?.message.includes('stdout exceeded 10 bytes') && error.stdout.length <= 10) +}) + +test('failed worker diagnostics redact explicit credentials and known token formats', async () => { + const secret = 'provider-secret-123456' + const pending = runLocalCli(node, ['-e', `process.stderr.write(${JSON.stringify(`key=${secret} token=sk-1234567890123456`)}); process.exit(1)`], { + providerCredential: { name: 'TEST_PROVIDER_KEY', value: secret }, + }) + await assert.rejects(pending, (error) => { + assert.match(error.message, /exited with code 1/) + assert.doesNotMatch(error.stderr, new RegExp(secret)) + assert.doesNotMatch(error.stderr, /sk-1234567890123456/) + assert.match(error.stderr, /\[REDACTED\]/) + return true + }) +}) + +test('rejects unsafe worker limits before spawning', async () => { + await assert.rejects(runLocalCli(node, ['-e', 'process.exit(0)'], { maxOutputBytes: 25 * 1024 * 1024 + 1 }), /maxOutputBytes/) + await assert.rejects(runLocalCli(node, ['-e', 'process.exit(0)'], { timeoutMs: 10 * 60 * 1000 + 1 }), /timeout/) +}) + +test('missing executable remains a typed spawn failure without raw output', async () => { + await assert.rejects(runLocalCli('agentskit-command-does-not-exist', []), (error) => error?.code === 'ENOENT' && error.stdout === '' && error.stderr === '') +}) diff --git a/test/review-config.test.mjs b/test/review-config.test.mjs index 8743a50..c8928ad 100644 --- a/test/review-config.test.mjs +++ b/test/review-config.test.mjs @@ -34,6 +34,7 @@ test('merges independent lens policy and flags override file values', () => { assert.equal(config.lenses.security.enabled, true) assert.equal(config.votes, 1) assert.equal(config.thresholds.minSeverity, 'med') + assert.equal(config.worker.timeoutMs, 120000) }) test('rejects unknown fields, unsupported versions, and impossible required lenses', () => {