From ff7eccc36c6997b94de769b4ce14f2c154c4b255 Mon Sep 17 00:00:00 2001 From: gnanam1990 Date: Thu, 9 Apr 2026 11:50:42 +0530 Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20feedback=20=E2=80=94?= =?UTF-8?q?=20enforce=20maxRetries,=20wire=20abort=20signal,=20use=20cross?= =?UTF-8?q?-platform=20shell?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Enforce maxRetries: track auto-fix attempts per query chain in toolHooks.ts and stop feeding errors back after the configured limit is reached. 2. Wire abort signal to subprocess: subscribe to AbortController signal in runCommand() and kill the process tree on abort. Uses detached process groups on Unix to ensure child processes are also terminated. 3. Replace hardcoded bash with shell:true: use Node's cross-platform shell resolution instead of spawn('bash', ['-c', ...]) so auto-fix commands work on Windows and non-bash environments. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../autoFix/autoFixIntegration.test.ts | 2 + src/services/autoFix/autoFixRunner.test.ts | 8 ++ src/services/autoFix/autoFixRunner.ts | 38 +++++++++- src/services/tools/toolHooks.ts | 73 +++++++++++++------ 4 files changed, 97 insertions(+), 24 deletions(-) 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) {