From dd60a3b4caf3bcb078d8aafd458d537700e69f27 Mon Sep 17 00:00:00 2001 From: Mike Lyons Date: Sun, 18 Oct 2026 11:07:24 -0400 Subject: [PATCH 1/2] fix(chat): an abandoned approval must not leave a needs-approval dot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude Code applies its MCP idle timeout to our `approve` call and aborts the tool after 1800s of silence, reporting it as an is_error tool_result on the original tool_use: Error calling tool (AskUserQuestion): MCP server "ness-permissions" tool "approve" sent no response or progress for 1800s; aborting. Nothing reaches the approval socket when that happens — the MCP subprocess is still parked on it — so the pending entry survived the turn and the status deriver kept publishing needs-approval. The sidebar showed a red dot forever for a tool the chat already showed as skipped, and the only cure was killing the tab. Two halves: * Reconcile on tool_result. A result means the call was decided, so an approval still pending for that tool_use id can never be answered. The stream reader hands every result to the bridge, which denies the waiting socket (so the subprocess unwinds instead of leaking until the session dies) and drops the card. * Don't let it fire in the first place. The `approve` call is parked on a human, and the needs-approval dot exists precisely so you can answer the tab whenever you get to it — half an hour is not a meaningful deadline. Give the ness-permissions MCP entry its own one-week timeout; verified against the bundled binary that a per-server `timeout` overrides the global idle timeout rather than being min'd with it. Co-Authored-By: Claude Opus 5 --- src/main/approval-bridge-rules.test.ts | 109 ++++++++++++++++++++-- src/main/approval-bridge.ts | 36 +++++++ src/main/index.ts | 2 + src/main/json-claude-manager-fork.test.ts | 1 + src/main/json-claude-manager.test.ts | 64 ++++++++++++- src/main/json-claude-manager.ts | 33 ++++++- 6 files changed, 234 insertions(+), 11 deletions(-) diff --git a/src/main/approval-bridge-rules.test.ts b/src/main/approval-bridge-rules.test.ts index af3697cbd..efb8898b0 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 99191b7b7..8efbc1956 100644 --- a/src/main/approval-bridge.ts +++ b/src/main/approval-bridge.ts @@ -178,6 +178,42 @@ export class ApprovalBridge { } } + /** Claude Code abandoned the tool call we were asking about, so the + * answer can no longer land anywhere. The common cause is its MCP + * idle timeout firing on our `approve` call ("sent no response or + * progress for Ns; aborting"), which it reports as an is_error + * tool_result on the *original* tool_use — the approval socket stays + * open and we never hear about it. Without this reconciliation the + * pending entry outlives the turn, so 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 dbdd0d0ca..355efdd5f 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 6e666cc63..5597f7601 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 306e25f83..8d7f6aca8 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,66 @@ describe('JsonClaudeManager', () => { expect(call.args).toContain('r/b') }) + // An approval is parked on a human, so Claude Code's default MCP idle + // timeout (1800s) would abandon the tool call while the sidebar dot is + // still waiting to be clicked. Scoped to ness-permissions only. + it('gives the ness-permissions MCP server a timeout long enough for a human', () => { + 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 + } + const timeout = config.mcpServers['ness-permissions'].timeout + expect(timeout).toBeGreaterThan(60 * 60 * 1000) + // A larger delay than a Node timer can hold overflows to ~instant. + expect(timeout).toBeLessThan(2 ** 31 - 1) + }) + + 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 aec0b1427..bd3b319f5 100644 --- a/src/main/json-claude-manager.ts +++ b/src/main/json-claude-manager.ts @@ -81,6 +81,20 @@ const PARTIAL_TEXT_FLUSH_MS = 30 * see. 4Hz still feels live if the card is expanded. */ const PARTIAL_THINKING_FLUSH_MS = 250 +/** Per-server timeout (ms) for the ness-permissions MCP entry. Claude + * Code otherwise applies its default MCP idle timeout (1800s) to our + * `approve` call and aborts the tool with + * `sent no response or progress for 1800s`. That call is parked on a + * *human*, and the whole point of the sidebar's needs-approval dot is + * that you answer the tab when you get to it — half an hour is not a + * meaningful deadline, so give it a week. Must stay under + * 2**31-1 (~24.8 days), the largest delay a Node timer can hold + * without overflowing and firing immediately. Scoped to this server + * only, so a genuinely hung ness-control tool still times out + * normally. The abort is still handled if it does fire — see + * cancelApprovalForToolUse. */ +const APPROVAL_MCP_TIMEOUT_MS = 7 * 24 * 60 * 60 * 1000 + export interface JsonClaudeControlServerInfo { port: number token: string @@ -101,6 +115,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 @@ -600,7 +619,12 @@ export class JsonClaudeManager { // harnessMcpEnabled=false or the control server isn't up. const mcpServers: Record< string, - { command: string; args: string[]; env: Record } + { + command: string + args: string[] + env: Record + timeout?: number + } > = { 'ness-permissions': { command: process.execPath, @@ -609,7 +633,8 @@ export class JsonClaudeManager { ELECTRON_RUN_AS_NODE: '1', HARNESS_APPROVAL_SOCKET: socketPath, HARNESS_JSON_CLAUDE_SESSION_ID: sessionId - } + }, + timeout: APPROVAL_MCP_TIMEOUT_MS } } if (this.opts.isHarnessMcpEnabled()) { @@ -1711,6 +1736,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: { From b97658aef94ea0d7dd4a31bebf588213edf37f35 Mon Sep 17 00:00:00 2001 From: Mike Lyons Date: Sun, 18 Oct 2026 11:33:39 -0400 Subject: [PATCH 2/2] revert(chat): keep Claude's 1800s idle timeout on unanswered approvals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit gave the ness-permissions MCP entry a one-week per-server timeout on the theory that an approval parked on a human has no meaningful deadline. It does: the abort is what lets an agent nobody answered work around the question — or go do something else — instead of blocking on a tab until someone opens it. The transcript that started this even shows the good outcome ("The question tool timed out, so I'll proceed on my own defaults and flag both assumptions at the end"). So the default stays, and its absence is now load-bearing — asserted in a test and noted at the config site, because "the approval card vanished after 30 minutes" reads like a bug worth fixing with a timeout. It isn't. The stale-dot half of the fix stands on its own: Ness reconciles the abort via cancelApprovalForToolUse. Co-Authored-By: Claude Opus 5 --- src/main/approval-bridge.ts | 16 ++++++++------ src/main/json-claude-manager.test.ts | 17 ++++++++------- src/main/json-claude-manager.ts | 32 +++++++++------------------- 3 files changed, 28 insertions(+), 37 deletions(-) diff --git a/src/main/approval-bridge.ts b/src/main/approval-bridge.ts index 8efbc1956..1b3ff8676 100644 --- a/src/main/approval-bridge.ts +++ b/src/main/approval-bridge.ts @@ -179,13 +179,15 @@ export class ApprovalBridge { } /** Claude Code abandoned the tool call we were asking about, so the - * answer can no longer land anywhere. The common cause is its MCP - * idle timeout firing on our `approve` call ("sent no response or - * progress for Ns; aborting"), which it reports as an is_error - * tool_result on the *original* tool_use — the approval socket stays - * open and we never hear about it. Without this reconciliation the - * pending entry outlives the turn, so the sidebar keeps a - * needs-approval dot for a tool the chat already shows as skipped. + * 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 diff --git a/src/main/json-claude-manager.test.ts b/src/main/json-claude-manager.test.ts index 8d7f6aca8..edc8d1d44 100644 --- a/src/main/json-claude-manager.test.ts +++ b/src/main/json-claude-manager.test.ts @@ -295,10 +295,13 @@ describe('JsonClaudeManager', () => { expect(call.args).toContain('r/b') }) - // An approval is parked on a human, so Claude Code's default MCP idle - // timeout (1800s) would abandon the tool call while the sidebar dot is - // still waiting to be clicked. Scoped to ness-permissions only. - it('gives the ness-permissions MCP server a timeout long enough for a human', () => { + // 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({ @@ -313,10 +316,8 @@ describe('JsonClaudeManager', () => { const config = JSON.parse(call!.args[configIdx + 1]) as { mcpServers: Record } - const timeout = config.mcpServers['ness-permissions'].timeout - expect(timeout).toBeGreaterThan(60 * 60 * 1000) - // A larger delay than a Node timer can hold overflows to ~instant. - expect(timeout).toBeLessThan(2 ** 31 - 1) + 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', () => { diff --git a/src/main/json-claude-manager.ts b/src/main/json-claude-manager.ts index bd3b319f5..5e859fe85 100644 --- a/src/main/json-claude-manager.ts +++ b/src/main/json-claude-manager.ts @@ -81,20 +81,6 @@ const PARTIAL_TEXT_FLUSH_MS = 30 * see. 4Hz still feels live if the card is expanded. */ const PARTIAL_THINKING_FLUSH_MS = 250 -/** Per-server timeout (ms) for the ness-permissions MCP entry. Claude - * Code otherwise applies its default MCP idle timeout (1800s) to our - * `approve` call and aborts the tool with - * `sent no response or progress for 1800s`. That call is parked on a - * *human*, and the whole point of the sidebar's needs-approval dot is - * that you answer the tab when you get to it — half an hour is not a - * meaningful deadline, so give it a week. Must stay under - * 2**31-1 (~24.8 days), the largest delay a Node timer can hold - * without overflowing and firing immediately. Scoped to this server - * only, so a genuinely hung ness-control tool still times out - * normally. The abort is still handled if it does fire — see - * cancelApprovalForToolUse. */ -const APPROVAL_MCP_TIMEOUT_MS = 7 * 24 * 60 * 60 * 1000 - export interface JsonClaudeControlServerInfo { port: number token: string @@ -613,18 +599,21 @@ 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 // harnessMcpEnabled=false or the control server isn't up. const mcpServers: Record< string, - { - command: string - args: string[] - env: Record - timeout?: number - } + { command: string; args: string[]; env: Record } > = { 'ness-permissions': { command: process.execPath, @@ -633,8 +622,7 @@ export class JsonClaudeManager { ELECTRON_RUN_AS_NODE: '1', HARNESS_APPROVAL_SOCKET: socketPath, HARNESS_JSON_CLAUDE_SESSION_ID: sessionId - }, - timeout: APPROVAL_MCP_TIMEOUT_MS + } } } if (this.opts.isHarnessMcpEnabled()) {