orcs-oss/orcs-code
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address review feedback — enforce maxRetries, wire abort signal, use cross-platform shell

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
gnanam1990
2026-04-09 11:50:42 +05:30
parent fbc838ce55
commit ff7eccc36c
4 changed files with 97 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
src/services/autoFix/autoFixIntegration.test.ts
View File

@@ -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)

8
src/services/autoFix/autoFixRunner.test.ts
View File

@@ -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)

38
src/services/autoFix/autoFixRunner.ts
View File

@@ -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

73
src/services/tools/toolHooks.ts
View File

@@ -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<string, number>()
import { isMcpTool } from '../mcp/utils.js'
import type { McpServerType, MessageUpdateLazy } from './toolExecution.js'
@@ -197,29 +201,54 @@ export async function* runPostToolUseHooks<Input extends AnyObject, Output>(
: 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: [
`<auto_fix_feedback>\nAUTO-FIX: Maximum retry limit (${autoFixConfig.maxRetries}) reached. ` +
`Skipping further auto-fix attempts. Please review the errors manually.\n</auto_fix_feedback>`,
],
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) {