diff --git a/src/main/approval-bridge-rules.test.ts b/src/main/approval-bridge-rules.test.ts index af3697cb..efb8898b 100644 --- a/src/main/approval-bridge-rules.test.ts +++ b/src/main/approval-bridge-rules.test.ts @@ -1,7 +1,9 @@ -// Unit coverage for the persistent-allowlist short-circuit in -// ApprovalBridge. Talks to the bridge's unix socket directly with -// hand-written request frames instead of spawning `claude`, so it runs -// everywhere (unlike approval-bridge.test.ts, which needs the binary). +// Unit coverage for the ApprovalBridge paths that don't need a model: +// the persistent-allowlist short-circuit, and dropping an approval whose +// tool call Claude already abandoned. Talks to the bridge's unix socket +// directly with hand-written request frames instead of spawning +// `claude`, so it runs everywhere (unlike approval-bridge.test.ts, +// which needs the binary). import { describe, it, expect, afterEach } from 'vitest' import { createConnection, type Socket } from 'node:net' @@ -46,7 +48,11 @@ afterEach(() => { interface Harness { store: TestStore sessionId: string + bridge: ApprovalBridge send: (frame: Record) => Promise | null> + /** Every response frame the bridge has written back, in order — + * including ones that landed after `send` already gave up waiting. */ + responses: Record[] } /** Boot a bridge with `rules` pre-loaded and return a `send` that writes one @@ -65,10 +71,13 @@ async function harness(rules: StoredPermissionRule[]): Promise { openBridges.push(bridge) const sessionId = randomUUID() const socketPath = bridge.startSession(sessionId) + const responses: Record[] = [] return { store, sessionId, + bridge, + responses, send: (frame) => new Promise((resolve, reject) => { const socket = createConnection(socketPath) @@ -81,10 +90,16 @@ async function harness(rules: StoredPermissionRule[]): Promise { }) socket.on('data', (chunk: Buffer) => { buf += chunk.toString('utf8') - const idx = buf.indexOf('\n') - if (idx < 0) return - clearTimeout(timer) - resolve(JSON.parse(buf.slice(0, idx)) as Record) + let idx: number + while ((idx = buf.indexOf('\n')) >= 0) { + const line = buf.slice(0, idx) + buf = buf.slice(idx + 1) + if (!line.trim()) continue + const parsed = JSON.parse(line) as Record + responses.push(parsed) + clearTimeout(timer) + resolve(parsed) + } }) socket.on('connect', () => { socket.write(JSON.stringify({ type: 'request', sessionId, ...frame }) + '\n') @@ -93,6 +108,10 @@ async function harness(rules: StoredPermissionRule[]): Promise { } } +function tick(ms = 50): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + describe('ApprovalBridge — persistent allowlist', () => { it('allows a matching call without surfacing a card', async () => { const h = await harness([rule('Bash', 'git status:*')]) @@ -170,3 +189,77 @@ describe('ApprovalBridge — persistent allowlist', () => { expect(res).toBeNull() }) }) + +describe('ApprovalBridge — abandoned tool calls', () => { + it('drops a pending approval once a result lands for its tool_use', async () => { + // Claude Code's MCP idle timeout aborts our `approve` call and + // reports it as an is_error tool_result on the original tool_use. + // Nothing reaches the socket, so without this the card and its + // needs-approval dot would outlive the turn. + const h = await harness([]) + expect( + await h.send({ + id: 'req-stale', + tool_name: 'AskUserQuestion', + input: { questions: [] }, + tool_use_id: 'tu-stale' + }) + ).toBeNull() + expect( + h.store.getSnapshot().state.jsonClaude.pendingApprovals['req-stale'] + ).toBeDefined() + + expect(h.bridge.cancelPendingForToolUse(h.sessionId, 'tu-stale')).toBe(true) + await tick() + + expect( + h.store.getSnapshot().state.jsonClaude.pendingApprovals['req-stale'] + ).toBeUndefined() + // The MCP subprocess is still parked on the socket — answer it so it + // unwinds instead of leaking until the session dies. + expect(h.responses).toHaveLength(1) + expect(h.responses[0]).toMatchObject({ type: 'response', id: 'req-stale' }) + expect( + (h.responses[0] as { result: { behavior: string } }).result.behavior + ).toBe('deny') + }) + + it('leaves approvals for other tool_use ids and other sessions alone', async () => { + const h = await harness([]) + await h.send({ + id: 'req-keep', + tool_name: 'Bash', + input: { command: 'rm -rf /' }, + tool_use_id: 'tu-keep' + }) + expect(h.bridge.cancelPendingForToolUse(h.sessionId, 'tu-other')).toBe(false) + expect(h.bridge.cancelPendingForToolUse('some-other-session', 'tu-keep')).toBe( + false + ) + await tick() + expect( + h.store.getSnapshot().state.jsonClaude.pendingApprovals['req-keep'] + ).toBeDefined() + expect(h.responses).toHaveLength(0) + }) + + it('is a no-op for an approval the user already answered', async () => { + // The normal ordering: the user clicks Allow, the tool runs, and its + // result arrives. One deny must not be written on top of the allow. + const h = await harness([]) + await h.send({ + id: 'req-done', + tool_name: 'Bash', + input: { command: 'ls' }, + tool_use_id: 'tu-done' + }) + expect(h.bridge.resolveApproval('req-done', { behavior: 'allow' })).toBe(true) + await tick() + expect(h.bridge.cancelPendingForToolUse(h.sessionId, 'tu-done')).toBe(false) + await tick() + expect(h.responses).toHaveLength(1) + expect( + (h.responses[0] as { result: { behavior: string } }).result.behavior + ).toBe('allow') + }) +}) diff --git a/src/main/approval-bridge.ts b/src/main/approval-bridge.ts index 99191b7b..1b3ff867 100644 --- a/src/main/approval-bridge.ts +++ b/src/main/approval-bridge.ts @@ -178,6 +178,44 @@ export class ApprovalBridge { } } + /** Claude Code abandoned the tool call we were asking about, so the + * answer can no longer land anywhere. The usual cause is its MCP + * idle timeout firing on our `approve` call after 1800s of silence + * ("sent no response or progress for 1800s; aborting") — by design, + * so an agent nobody answered works around the question instead of + * blocking forever. It reports that as an is_error tool_result on + * the *original* tool_use; the approval socket itself hears nothing. + * So without this reconciliation the pending entry outlives the + * turn, and the sidebar keeps a needs-approval dot for a tool the + * chat already shows as skipped. + * + * Called from the json-claude stream reader for every tool_result: + * a result means the call was decided, so any approval still pending + * for that tool_use id is by definition stale. Answers the socket so + * the MCP subprocess unwinds, then drops the card. */ + cancelPendingForToolUse( + sessionId: string, + toolUseId: string, + reason = 'tool call abandoned before approval resolved' + ): boolean { + for (const [id, pending] of Array.from(this.pendingResponses.entries())) { + if (pending.sessionId !== sessionId) continue + if (pending.payload.toolUseId !== toolUseId) continue + this.writeResponse(pending.socket, id, { behavior: 'deny', message: reason }) + this.pendingResponses.delete(id) + this.store.dispatch({ + type: 'jsonClaude/approvalResolved', + payload: { requestId: id } + }) + log( + 'approval-bridge', + `stale approval dropped session=${sessionId} id=${id} tool=${pending.payload.toolName} toolUseId=${toolUseId} reason="${reason}"` + ) + return true + } + return false + } + /** Dispatched from the renderer-facing IPC handler. Push the user's * chosen PermissionResult back out over the waiting socket. */ resolveApproval(requestId: string, result: ApprovalResult): boolean { diff --git a/src/main/index.ts b/src/main/index.ts index dbdd0d0c..355efdd5 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -431,6 +431,8 @@ const jsonClaudeManager = new JsonClaudeManager(store, { store.getSnapshot().state.settings.useSystemClaudeForJsonMode === true, getApprovalSocketPath: (sessionId) => approvalBridge.startSession(sessionId), closeApprovalSession: (sessionId) => approvalBridge.stopSession(sessionId), + cancelApprovalForToolUse: (sessionId, toolUseId) => + approvalBridge.cancelPendingForToolUse(sessionId, toolUseId), getClaudeEnvVars: () => store.getSnapshot().state.settings.claudeEnvVars || {}, getControlServer: () => getControlServerInfo(), diff --git a/src/main/json-claude-manager-fork.test.ts b/src/main/json-claude-manager-fork.test.ts index 6e666cc6..5597f760 100644 --- a/src/main/json-claude-manager-fork.test.ts +++ b/src/main/json-claude-manager-fork.test.ts @@ -51,6 +51,7 @@ function makeManager(store: Store): JsonClaudeManager { getUseSystemClaude: () => false, getApprovalSocketPath: (sid) => `/tmp/sock-${sid}`, closeApprovalSession: vi.fn(), + cancelApprovalForToolUse: vi.fn(), getClaudeEnvVars: () => ({}), getControlServer: () => null, getControlBridgeScriptPath: () => '/tmp/bridge.js', diff --git a/src/main/json-claude-manager.test.ts b/src/main/json-claude-manager.test.ts index 306e25f8..edc8d1d4 100644 --- a/src/main/json-claude-manager.test.ts +++ b/src/main/json-claude-manager.test.ts @@ -81,13 +81,15 @@ describe('JsonClaudeManager', () => { function makeManager( store: Store, launchSettings: ClaudeLaunchSettings = { tuiFullscreen: true }, - useSystemClaude = false + useSystemClaude = false, + cancelApprovalForToolUse: (sessionId: string, toolUseId: string) => void = vi.fn() ): JsonClaudeManager { return new JsonClaudeManager(store, { getClaudeCommand: () => 'claude', getUseSystemClaude: () => useSystemClaude, getApprovalSocketPath: (sid) => `/tmp/sock-${sid}`, closeApprovalSession: vi.fn(), + cancelApprovalForToolUse, getClaudeEnvVars: () => ({}), getControlServer: () => null, getControlBridgeScriptPath: () => '/tmp/bridge.js', @@ -293,6 +295,67 @@ describe('JsonClaudeManager', () => { expect(call.args).toContain('r/b') }) + // Load-bearing absence: with no per-server `timeout`, Claude's default + // MCP idle timeout (1800s) aborts an approval nobody answered, and the + // agent works around the question instead of parking forever. Setting + // one here to keep an approval card alive longer would trade a + // self-healing agent for a blocked one — Ness reconciles the abort + // instead (see the stale-approval test below). + it('sets no timeout on the ness-permissions MCP server', () => { + const store = new Store() + const mgr = makeManager(store) + store.dispatch({ + type: 'jsonClaude/sessionStarted', + payload: { sessionId: 'sess-mcp-timeout', worktreePath: '/tmp/wt' } + }) + mgr.create('sess-mcp-timeout', '/tmp/wt') + const call = spawnCalls + .filter((c) => c.args.some((a) => a.includes('--permission-prompt-tool'))) + .pop() + const configIdx = call!.args.indexOf('--mcp-config') + const config = JSON.parse(call!.args[configIdx + 1]) as { + mcpServers: Record + } + expect(config.mcpServers['ness-permissions']).toBeDefined() + expect(config.mcpServers['ness-permissions'].timeout).toBeUndefined() + }) + + it('drops a stale approval when a tool_result lands for its tool_use', () => { + // Claude Code aborting its `approve` MCP call is reported as an + // is_error tool_result on the original tool_use — the only signal we + // get that the approval window closed. + const store = new Store() + const cancel = vi.fn() + const mgr = makeManager(store, { tuiFullscreen: true }, false, cancel) + const sessionId = 'sess-stale-approval' + store.dispatch({ + type: 'jsonClaude/sessionStarted', + payload: { sessionId, worktreePath: '/tmp/wt' } + }) + mgr.create(sessionId, '/tmp/wt') + sessionProcs()[0].stdout.emit( + 'data', + Buffer.from( + JSON.stringify({ + type: 'user', + message: { + role: 'user', + content: [ + { + type: 'tool_result', + tool_use_id: 'toolu_timeout', + is_error: true, + content: + 'Error calling tool (AskUserQuestion): MCP server "ness-permissions" tool "approve" sent no response or progress for 1800s; aborting.' + } + ] + } + }) + '\n' + ) + ) + expect(cancel).toHaveBeenCalledWith(sessionId, 'toolu_timeout') + }) + it('omits --append-system-prompt, --model, --name when launch settings are unset', () => { const store = new Store() const mgr = makeManager(store, { tuiFullscreen: true }) diff --git a/src/main/json-claude-manager.ts b/src/main/json-claude-manager.ts index aec0b142..5e859fe8 100644 --- a/src/main/json-claude-manager.ts +++ b/src/main/json-claude-manager.ts @@ -101,6 +101,11 @@ export interface JsonClaudeManagerOptions { getUseSystemClaude: () => boolean getApprovalSocketPath: (sessionId: string) => string closeApprovalSession: (sessionId: string) => void + /** Called for every tool_result we see so an approval still pending + * for that tool_use id can be dropped — Claude Code aborting its + * `approve` MCP call (idle timeout) is otherwise invisible to us. + * See ApprovalBridge.cancelPendingForToolUse. */ + cancelApprovalForToolUse: (sessionId: string, toolUseId: string) => void getClaudeEnvVars: () => Record /** Looked up from main when building the inline MCP config. Returning * null means the ness-control bridge isn't injected (settings flag @@ -594,6 +599,14 @@ export class JsonClaudeManager { // need a separate Node install in packaged builds. // * ness-permissions: receives the per-tool approval requests // Claude raises via --permission-prompt-tool. Tool is 'approve'. + // Deliberately carries no per-server `timeout`, so Claude's + // default MCP idle timeout (1800s) still aborts an approval + // nobody answered. That's wanted: the agent gets an error + // tool_result and can work around the question or move to + // something else instead of parking forever. Ness reconciles + // the abort via cancelApprovalForToolUse — setting a timeout + // here to "fix" a vanishing approval card would trade a + // self-healing agent for a blocked one. // * ness-control: the same MCP bridge used by xterm-backed // Claude tabs, exposing ness-control tools (worktree mgmt, // browser tabs, shell tabs, etc.). Skipped when settings has @@ -1711,6 +1724,10 @@ export class JsonClaudeManager { if (type === 'user') { const results = extractToolResults(parsed) for (const r of results) { + // A result means this tool call is decided, so an approval still + // pending for it can never be answered — drop it before the + // card and its needs-approval dot outlive the turn. + this.opts.cancelApprovalForToolUse(instance.sessionId, r.toolUseId) this.store.dispatch({ type: 'jsonClaude/toolResultAttached', payload: {