diff --git a/src/services/autoFix/autoFixIntegration.test.ts b/src/services/autoFix/autoFixIntegration.test.ts index a8d8fe44..1a1347f1 100644 --- a/src/services/autoFix/autoFixIntegration.test.ts +++ b/src/services/autoFix/autoFixIntegration.test.ts @@ -18,6 +18,7 @@ describe('autoFix end-to-end flow', () => { lint: config!.lint, test: config!.test, timeout: config!.timeout, + cwd: '/tmp', }) expect(result.hasErrors).toBe(true) @@ -37,6 +38,7 @@ describe('autoFix end-to-end flow', () => { const result = await runAutoFixCheck({ lint: config!.lint, timeout: config!.timeout, + cwd: '/tmp', }) expect(result.hasErrors).toBe(false) diff --git a/src/services/autoFix/autoFixRunner.test.ts b/src/services/autoFix/autoFixRunner.test.ts index 13d177e8..616d6170 100644 --- a/src/services/autoFix/autoFixRunner.test.ts +++ b/src/services/autoFix/autoFixRunner.test.ts @@ -10,6 +10,7 @@ describe('runAutoFixCheck', () => { const result = await runAutoFixCheck({ lint: 'echo "all clean"', timeout: 5000, + cwd: '/tmp', }) expect(result.hasErrors).toBe(false) @@ -21,6 +22,7 @@ describe('runAutoFixCheck', () => { const result = await runAutoFixCheck({ lint: 'echo "error: unused var" && exit 1', timeout: 5000, + cwd: '/tmp', }) expect(result.hasErrors).toBe(true) @@ -32,6 +34,7 @@ describe('runAutoFixCheck', () => { const result = await runAutoFixCheck({ test: 'echo "FAIL test_foo" && exit 1', timeout: 5000, + cwd: '/tmp', }) expect(result.hasErrors).toBe(true) @@ -44,6 +47,7 @@ describe('runAutoFixCheck', () => { lint: 'echo "lint ok"', test: 'echo "test ok"', timeout: 5000, + cwd: '/tmp', }) expect(result.hasErrors).toBe(false) @@ -56,6 +60,7 @@ describe('runAutoFixCheck', () => { lint: 'echo "lint error" && exit 1', test: 'echo "should not run"', timeout: 5000, + cwd: '/tmp', }) expect(result.hasErrors).toBe(true) @@ -67,6 +72,7 @@ describe('runAutoFixCheck', () => { const result = await runAutoFixCheck({ lint: 'sleep 10', timeout: 100, + cwd: '/tmp', }) expect(result.hasErrors).toBe(true) @@ -76,6 +82,7 @@ describe('runAutoFixCheck', () => { test('returns success with no commands configured', async () => { const result = await runAutoFixCheck({ timeout: 5000, + cwd: '/tmp', }) expect(result.hasErrors).toBe(false) @@ -85,6 +92,7 @@ describe('runAutoFixCheck', () => { const result = await runAutoFixCheck({ lint: 'echo "src/foo.ts:10:5 error no-unused-vars" && exit 1', timeout: 5000, + cwd: '/tmp', }) expect(result.hasErrors).toBe(true) diff --git a/src/services/autoFix/autoFixRunner.ts b/src/services/autoFix/autoFixRunner.ts index 0d45359d..a24268a7 100644 --- a/src/services/autoFix/autoFixRunner.ts +++ b/src/services/autoFix/autoFixRunner.ts @@ -25,15 +25,43 @@ async function runCommand( signal?: AbortSignal, ): Promise<{ stdout: string; stderr: string; exitCode: number; timedOut: boolean }> { return new Promise((resolve) => { + if (signal?.aborted) { + resolve({ stdout: '', stderr: 'Aborted', exitCode: 1, timedOut: false }) + return + } + let timedOut = false let stdout = '' let stderr = '' - const proc = spawn('bash', ['-c', command], { + const isWindows = process.platform === 'win32' + const proc = spawn(command, [], { cwd, env: { ...process.env }, + shell: true, + windowsHide: true, + // On Unix, create a process group so we can kill child processes on timeout/abort + detached: !isWindows, }) + const killTree = () => { + try { + if (!isWindows && proc.pid) { + // Kill the entire process group + process.kill(-proc.pid, 'SIGTERM') + } else { + proc.kill('SIGTERM') + } + } catch { + // Process may have already exited + } + } + + const onAbort = () => { + killTree() + } + signal?.addEventListener('abort', onAbort, { once: true }) + proc.stdout?.on('data', (data: Buffer) => { stdout += data.toString() }) @@ -43,11 +71,12 @@ async function runCommand( const timer = setTimeout(() => { timedOut = true - proc.kill('SIGTERM') + killTree() }, timeout) proc.on('close', (code) => { clearTimeout(timer) + signal?.removeEventListener('abort', onAbort) resolve({ stdout: stdout.slice(0, 10000), stderr: stderr.slice(0, 10000), @@ -58,6 +87,7 @@ async function runCommand( proc.on('error', () => { clearTimeout(timer) + signal?.removeEventListener('abort', onAbort) resolve({ stdout, stderr: stderr || 'Command failed to start', @@ -94,6 +124,10 @@ export async function runAutoFixCheck( return { hasErrors: false } } + if (signal?.aborted) { + return { hasErrors: false } + } + const result: AutoFixResult = { hasErrors: false } // Run lint first diff --git a/src/services/tools/toolHooks.ts b/src/services/tools/toolHooks.ts index 98f4e0e2..94596f93 100644 --- a/src/services/tools/toolHooks.ts +++ b/src/services/tools/toolHooks.ts @@ -32,6 +32,10 @@ import { formatError } from '../../utils/toolErrors.js' import { getAutoFixConfig } from '../autoFix/autoFixConfig.js' import { shouldRunAutoFix, buildAutoFixContext } from '../autoFix/autoFixHook.js' import { runAutoFixCheck } from '../autoFix/autoFixRunner.js' + +// Track auto-fix retry count per query chain to enforce maxRetries cap. +// Key: queryChainId (or 'default'), Value: number of auto-fix attempts used. +const autoFixRetryCount = new Map() import { isMcpTool } from '../mcp/utils.js' import type { McpServerType, MessageUpdateLazy } from './toolExecution.js' @@ -197,29 +201,54 @@ export async function* runPostToolUseHooks( : undefined, ) if (shouldRunAutoFix(tool.name, autoFixConfig) && autoFixConfig) { - try { - const cwd = toolUseContext.options?.cwd ?? process.cwd() - const autoFixResult = await runAutoFixCheck({ - lint: autoFixConfig.lint, - test: autoFixConfig.test, - timeout: autoFixConfig.timeout, - cwd, - signal: toolUseContext.abortController.signal, - }) - const autoFixContext = buildAutoFixContext(autoFixResult) - if (autoFixContext) { - yield { - message: createAttachmentMessage({ - type: 'hook_additional_context', - content: [autoFixContext], - hookName: `AutoFix:${tool.name}`, - toolUseID, - hookEvent: 'PostToolUse', - }), - } + // Enforce maxRetries cap to prevent unbounded auto-fix loops. + // Uses queryChainId to scope the counter to the current conversation turn. + const chainKey = (toolUseContext.queryTracking?.chainId as string) ?? 'default' + const currentRetries = autoFixRetryCount.get(chainKey) ?? 0 + + if (currentRetries >= autoFixConfig.maxRetries) { + // Max retries reached — skip auto-fix and let the user know + yield { + message: createAttachmentMessage({ + type: 'hook_additional_context', + content: [ + `\nAUTO-FIX: Maximum retry limit (${autoFixConfig.maxRetries}) reached. ` + + `Skipping further auto-fix attempts. Please review the errors manually.\n`, + ], + hookName: `AutoFix:${tool.name}`, + toolUseID, + hookEvent: 'PostToolUse', + }), + } + } else { + try { + const cwd = toolUseContext.options?.cwd ?? process.cwd() + const autoFixResult = await runAutoFixCheck({ + lint: autoFixConfig.lint, + test: autoFixConfig.test, + timeout: autoFixConfig.timeout, + cwd, + signal: toolUseContext.abortController.signal, + }) + const autoFixContext = buildAutoFixContext(autoFixResult) + if (autoFixContext) { + autoFixRetryCount.set(chainKey, currentRetries + 1) + yield { + message: createAttachmentMessage({ + type: 'hook_additional_context', + content: [autoFixContext], + hookName: `AutoFix:${tool.name}`, + toolUseID, + hookEvent: 'PostToolUse', + }), + } + } else { + // Lint/test passed — reset the retry counter for this chain + autoFixRetryCount.delete(chainKey) + } + } catch (autoFixError) { + logError(autoFixError) } - } catch (autoFixError) { - logError(autoFixError) } } } catch (error) {