diff --git a/app/src/main/assets/hook-relay-blocking.js b/app/src/main/assets/hook-relay-blocking.js index 89eb28825..ec134aeeb 100644 --- a/app/src/main/assets/hook-relay-blocking.js +++ b/app/src/main/assets/hook-relay-blocking.js @@ -13,7 +13,10 @@ var net = require('net'); var socket = process.env.CLAUDE_MOBILE_SOCKET; if (!socket) process.exit(0); -var TIMEOUT_MS = parseInt(process.env.CLAUDE_RELAY_TIMEOUT || '120000', 10); +// Tier-2 backstop: 2h30m — above EventBridge's 2h hold, below Bootstrap's 3h +// CC hook timeout. Relay-wins = exit 2 (clean deny); CC-wins = hook killed +// with no decision = AskUserQuestion wedges forever. Do NOT equalize (spec §1). +var TIMEOUT_MS = parseInt(process.env.CLAUDE_RELAY_TIMEOUT || '9000000', 10); var input = ''; process.stdin.setEncoding('utf8'); diff --git a/app/src/main/kotlin/com/youcoded/app/bridge/HookSerializer.kt b/app/src/main/kotlin/com/youcoded/app/bridge/HookSerializer.kt index 4bc670e7b..1145627e4 100644 --- a/app/src/main/kotlin/com/youcoded/app/bridge/HookSerializer.kt +++ b/app/src/main/kotlin/com/youcoded/app/bridge/HookSerializer.kt @@ -30,9 +30,12 @@ object HookSerializer { return envelope("PermissionRequest", sessionId, inner) } - fun permissionExpired(sessionId: String, requestId: String): JSONObject { + fun permissionExpired(sessionId: String, requestId: String, reason: String? = null): JSONObject { val inner = JSONObject().apply { put("_requestId", requestId) + // _reason rides inside the payload — same convention as desktop + // main.ts, so the shared hook-dispatcher parses both transports. + if (reason != null) put("_reason", reason) } return envelope("PermissionExpired", sessionId, inner) } diff --git a/app/src/main/kotlin/com/youcoded/app/parser/EventBridge.kt b/app/src/main/kotlin/com/youcoded/app/parser/EventBridge.kt index b2611337d..906dce85c 100644 --- a/app/src/main/kotlin/com/youcoded/app/parser/EventBridge.kt +++ b/app/src/main/kotlin/com/youcoded/app/parser/EventBridge.kt @@ -19,12 +19,25 @@ import java.util.concurrent.ConcurrentHashMap * structured decision back through it (blocking relay protocol). */ class EventBridge(private val socketName: String) { + companion object { + /** §1 tier-1 hold (2h). Must stay UNDER the relay asset's 2h30m and + * Bootstrap's 3h CC hook timeout — margins are load-bearing (the + * losing order kills the hook with no decision and AskUserQuestion + * wedges forever). Pinned by desktop/tests/permission-timeout-margins. */ + const val PERMISSION_HOLD_MS = 7_200_000L + } + private val _events = MutableSharedFlow(extraBufferCapacity = 1000) val events: SharedFlow = _events /** Sockets held open for blocking PermissionRequest responses. */ private val pendingSockets = ConcurrentHashMap() + /** Tier-1 hold timers, keyed by requestId. Cancelled on every path that + * ends a request (respond, closeSocket, closure monitor, stop) so a + * 2h coroutine never outlives the socket it was guarding. */ + private val holdJobs = ConcurrentHashMap() + /** Maps mobile session IDs to Claude Code session IDs. */ private val sessionIdMap = ConcurrentHashMap() @@ -119,6 +132,43 @@ class EventBridge(private val socketName: String) { // hook-relay-blocking.js times out or Claude Code kills the hook. // Desktop equivalent: hook-relay.ts socket.on('close') handler. monitorSocketClosure(requestId, sessionId, client) + + // §1 tier-1: the app ends the wait with a labeled deny. + // Must emit explicitly — respond() removes the pending + // entry BEFORE closing, so the closure monitor stays + // silent for app-initiated endings (its own comment). + monitorScope?.launch(Dispatchers.IO) { + delay(PERMISSION_HOLD_MS) + holdJobs.remove(requestId) + if (pendingSockets.containsKey(requestId)) { + // Nested decision shape is load-bearing: the relay + // reads appDecision.decision. Message lands in the + // tool result the model reads. + // Fix: derive from PERMISSION_HOLD_MS instead of a + // hardcoded "2 hours" — mirrors desktop's + // hook-relay.ts deriving from APP_HOLD_MS, so a + // future tier change can't silently desync the two + // platforms' copy (permission-timeout-margins.test.ts + // still pins the underlying millisecond values). + val holdHours = PERMISSION_HOLD_MS / 3_600_000L + val hoursLabel = if (holdHours == 1L) "hour" else "hours" + val deny = JSONObject().put("decision", JSONObject() + .put("behavior", "deny") + .put("message", "YouCoded auto-denied this request after $holdHours $hoursLabel with no user response — ask again if still needed.")) + // Only emit "app-timeout" if the deny actually went out. If + // the write failed, respond() already emitted its own + // "delivery-failed" PermissionExpired — emitting again here + // would violate "at most one expiry per request". + if (respond(requestId, deny)) { + _events.tryEmit(HookEvent.PermissionExpired( + sessionId = sessionId, + hookEventName = "PermissionExpired", + requestId = requestId, + reason = "app-timeout", + )) + } + } + }?.also { holdJobs[requestId] = it } } else { pendingSockets.remove(requestId) client.close() @@ -143,7 +193,7 @@ class EventBridge(private val socketName: String) { /** * Monitor a held PermissionRequest socket for remote closure. - * When hook-relay-blocking.js times out (120s) or Claude Code kills the hook + * When hook-relay-blocking.js times out (its 2h30m tier-2 backstop) or Claude Code kills the hook * process, the socket closes. We detect this and emit PermissionExpired so * the React UI can clear the stale approval card. * @@ -165,10 +215,14 @@ class EventBridge(private val socketName: String) { // responded to — emit PermissionExpired to clean up the React UI. if (pendingSockets.remove(requestId) != null) { try { client.close() } catch (_: Exception) {} + // Far-end death (relay backstop or CC killing the hook), not our + // own hold firing — cancel the hold job so it doesn't also emit. + holdJobs.remove(requestId)?.cancel() if (!_events.tryEmit(HookEvent.PermissionExpired( sessionId = sessionId, hookEventName = "PermissionExpired", requestId = requestId, + reason = "hook-closed", ))) { android.util.Log.e("EventBridge", "Event buffer full, dropped PermissionExpired") } @@ -176,18 +230,29 @@ class EventBridge(private val socketName: String) { } } - /** Send a decision back through a held PermissionRequest socket. */ - fun respond(requestId: String, decision: JSONObject) { + /** + * Send a decision back through a held PermissionRequest socket. + * Returns true if the write succeeded, false if it failed (in which case + * this method has ALREADY emitted a "delivery-failed" PermissionExpired + * itself — callers must not emit a second one for the same requestId, or + * "at most one expiry per request" breaks for the hold-timeout path). + */ + fun respond(requestId: String, decision: JSONObject): Boolean { + // A decision is about to be delivered (or attempted) — the tier-1 + // hold is no longer needed. Cancel first so it can never race a + // second emit for the same request. + holdJobs.remove(requestId)?.cancel() val socket = pendingSockets.remove(requestId) if (socket == null) { android.util.Log.e("EventBridge", "No pending socket for requestId=$requestId") - return + return false } try { val payload = decision.toString() + "\n" socket.outputStream.write(payload.toByteArray()) socket.outputStream.flush() socket.close() + return true } catch (e: Exception) { // Response couldn't be delivered — permission effectively expired. // Emit PermissionExpired so React UI clears the stale approval card. @@ -197,12 +262,15 @@ class EventBridge(private val socketName: String) { sessionId = "", // ManagedSession uses its own ID for broadcast hookEventName = "PermissionExpired", requestId = requestId, + reason = "delivery-failed", )) + return false } } /** Close a held socket without sending a response (cross-path cleanup). */ fun closeSocket(requestId: String) { + holdJobs.remove(requestId)?.cancel() val socket = pendingSockets.remove(requestId) ?: return try { socket.close() } catch (_: Exception) {} } @@ -219,6 +287,10 @@ class EventBridge(private val socketName: String) { fun hasPendingPermission(): Boolean = pendingSockets.isNotEmpty() fun stop() { + // Cancel any outstanding tier-1 hold timers so they don't fire (and + // try to write to a socket we're about to close) after teardown. + holdJobs.values.forEach { it.cancel() } + holdJobs.clear() // Close all pending sockets for ((_, socket) in pendingSockets) { try { socket.close() } catch (_: Exception) {} diff --git a/app/src/main/kotlin/com/youcoded/app/parser/HookEvent.kt b/app/src/main/kotlin/com/youcoded/app/parser/HookEvent.kt index 060b46653..659352766 100644 --- a/app/src/main/kotlin/com/youcoded/app/parser/HookEvent.kt +++ b/app/src/main/kotlin/com/youcoded/app/parser/HookEvent.kt @@ -56,14 +56,17 @@ sealed class HookEvent { val requestId: String, ) : HookEvent() - /** Emitted when a held PermissionRequest socket closes before a response - * was sent — e.g., hook-relay-blocking.js timed out (120s) or Claude Code - * killed the hook process. React uses this to clear stale approval cards. - * Desktop equivalent: hook-relay.ts socket.on('close') → 'permission-expired'. */ + /** Emitted when a held PermissionRequest ends without a delivered user + * decision. reason discriminates (2026-07-30 spec §2): "app-timeout" + * (our own 2h hold fired — a deny WAS delivered), "delivery-failed" + * (respond() write threw), "hook-closed" (relay died / CC killed the + * hook — the terminal menu may still be live; React retains the card), + * or null (legacy producers; React resolves). */ data class PermissionExpired( override val sessionId: String, override val hookEventName: String, val requestId: String, + val reason: String? = null, ) : HookEvent() companion object { diff --git a/app/src/main/kotlin/com/youcoded/app/runtime/Bootstrap.kt b/app/src/main/kotlin/com/youcoded/app/runtime/Bootstrap.kt index 3041340ea..617cf4af6 100644 --- a/app/src/main/kotlin/com/youcoded/app/runtime/Bootstrap.kt +++ b/app/src/main/kotlin/com/youcoded/app/runtime/Bootstrap.kt @@ -25,6 +25,53 @@ class Bootstrap(internal val context: Context) { // Last Claude Code release shipping cli.js. See the comment on // isFullySetup and installClaudeCode() for why this is pinned. private const val PINNED_CLAUDE_CODE_VERSION = "2.1.112" + + /** Tier-3 CC hook timeout (3h) — 30m above the relay asset's 2h30m so CC + * never kills the hook first (no decision = AskUserQuestion wedges + * forever, spec §1). Pinned by desktop/tests/permission-timeout-margins. */ + const val PERMISSION_HOOK_TIMEOUT_SECONDS = 10800 + + /** Ensure the PermissionRequest blocking-relay hook entry exists AND + * carries the current command + timeout. WHY: earlier versions only + * appended when missing, so every existing install kept timeout 300 + * forever — and the relay asset DOES redeploy on every launch, so a + * relay-only change would put relay-2h30m against CC-300s: CC kills the + * hook with no decision and AskUserQuestion wedges permanently + * (2026-07-30 spec §Constraints). Mirrors desktop install-hooks.js + * find-and-replace semantics. */ + fun ensurePermissionRequestHook( + hooksObj: org.json.JSONObject, + blockingHookCommand: String, + timeoutSeconds: Int, + ) { + val prEvent = "PermissionRequest" + val prArray = hooksObj.optJSONArray(prEvent) ?: org.json.JSONArray() + var updated = false + for (i in 0 until prArray.length()) { + val hooks = prArray.optJSONObject(i)?.optJSONArray("hooks") ?: continue + for (j in 0 until hooks.length()) { + val h = hooks.optJSONObject(j) + if (h?.optString("command")?.contains("hook-relay-blocking.js") == true) { + h.put("command", blockingHookCommand) + h.put("timeout", timeoutSeconds) + updated = true + } + } + } + if (!updated) { + val hookEntry = org.json.JSONObject() + hookEntry.put("matcher", ".*") + val hooksList = org.json.JSONArray() + val hookDef = org.json.JSONObject() + hookDef.put("type", "command") + hookDef.put("command", blockingHookCommand) + hookDef.put("timeout", timeoutSeconds) + hooksList.put(hookDef) + hookEntry.put("hooks", hooksList) + prArray.put(hookEntry) + } + hooksObj.put(prEvent, prArray) + } } val usrDir: File get() = File(context.filesDir, "usr") @@ -986,37 +1033,9 @@ class Bootstrap(internal val context: Context) { hooksObj.put(event, eventArray) } - // Register PermissionRequest with blocking relay (long timeout for user approval) - val prEvent = "PermissionRequest" - val prArray = hooksObj.optJSONArray(prEvent) ?: org.json.JSONArray() - var prRegistered = false - for (i in 0 until prArray.length()) { - val entry = prArray.optJSONObject(i) - val hooks = entry?.optJSONArray("hooks") - if (hooks != null) { - for (j in 0 until hooks.length()) { - val h = hooks.optJSONObject(j) - if (h?.optString("command")?.contains("hook-relay-blocking.js") == true) { - prRegistered = true - break - } - } - } - if (prRegistered) break - } - if (!prRegistered) { - val hookEntry = org.json.JSONObject() - hookEntry.put("matcher", ".*") - val hooksList = org.json.JSONArray() - val hookDef = org.json.JSONObject() - hookDef.put("type", "command") - hookDef.put("command", blockingHookCommand) - hookDef.put("timeout", 300) - hooksList.put(hookDef) - hookEntry.put("hooks", hooksList) - prArray.put(hookEntry) - } - hooksObj.put(prEvent, prArray) + // Register PermissionRequest with blocking relay (tier-3 CC timeout, see + // PERMISSION_HOOK_TIMEOUT_SECONDS doc comment for the margin rationale) + ensurePermissionRequestHook(hooksObj, blockingHookCommand, PERMISSION_HOOK_TIMEOUT_SECONDS) // Auto-title hook: always deploy the bundled asset. Post-decomposition, // title-update.sh is app-owned (not a toolkit hook) on both platforms — diff --git a/app/src/main/kotlin/com/youcoded/app/runtime/ManagedSession.kt b/app/src/main/kotlin/com/youcoded/app/runtime/ManagedSession.kt index 8c66a933a..e2e597691 100644 --- a/app/src/main/kotlin/com/youcoded/app/runtime/ManagedSession.kt +++ b/app/src/main/kotlin/com/youcoded/app/runtime/ManagedSession.kt @@ -253,13 +253,17 @@ class ManagedSession( )) } is HookEvent.PermissionExpired -> { - // Socket closed before user responded — relay timed out - // or Claude Code killed the hook. Clear the stale approval - // card in React UI. Desktop equivalent: main.ts - // hookRelay.on('permission-expired') handler. + // Ended without a delivered decision (app hold fired, + // respond() write failed, or the relay/hook died). + // reason discriminates which — React uses it to decide + // whether to retain or resolve the stale approval card. + // Routing keys on requestId / this session's own `id`, + // NEVER event.sessionId (EventBridge's write-failure + // path emits that as "" — see EventBridge.kt). server.broadcast(HookSerializer.permissionExpired( sessionId = id, requestId = event.requestId, + reason = event.reason, )) } is HookEvent.Notification -> { diff --git a/app/src/test/kotlin/com/youcoded/app/bridge/HookSerializerTest.kt b/app/src/test/kotlin/com/youcoded/app/bridge/HookSerializerTest.kt index bf21d8c21..a601c5831 100644 --- a/app/src/test/kotlin/com/youcoded/app/bridge/HookSerializerTest.kt +++ b/app/src/test/kotlin/com/youcoded/app/bridge/HookSerializerTest.kt @@ -140,6 +140,17 @@ class HookSerializerTest { assertFalse("Should not have 'requestId'", inner.has("requestId")) } + @Test + fun `permissionExpired carries _reason when present and omits it when null`() { + val with = HookSerializer.permissionExpired("s1", "r1", "hook-closed") + val inner = with.getJSONObject("payload").getJSONObject("payload") + assertEquals("hook-closed", inner.getString("_reason")) + + val without = HookSerializer.permissionExpired("s1", "r1", null) + val innerNone = without.getJSONObject("payload").getJSONObject("payload") + assertFalse(innerNone.has("_reason")) + } + // ── notification ───────────────────────────────────────────────────────── @Test diff --git a/app/src/test/kotlin/com/youcoded/app/runtime/BootstrapHooksTest.kt b/app/src/test/kotlin/com/youcoded/app/runtime/BootstrapHooksTest.kt new file mode 100644 index 000000000..48b899111 --- /dev/null +++ b/app/src/test/kotlin/com/youcoded/app/runtime/BootstrapHooksTest.kt @@ -0,0 +1,48 @@ +package com.youcoded.app.runtime + +import org.json.JSONArray +import org.json.JSONObject +import org.junit.Assert.assertEquals +import org.junit.Test + +/** Guards the 2026-07-30 spec §Constraints inversion: an install that already + * has the hook must still receive a changed timeout on the next launch. */ +class BootstrapHooksTest { + + private fun existingHooks(timeout: Int): JSONObject { + val h = JSONObject().put("type", "command") + .put("command", "node /old/path/hook-relay-blocking.js").put("timeout", timeout) + val entry = JSONObject().put("matcher", ".*") + .put("hooks", JSONArray().put(h)) + return JSONObject().put("PermissionRequest", JSONArray().put(entry)) + } + + @Test + fun `overwrites timeout and command on an existing entry`() { + val hooksObj = existingHooks(300) + Bootstrap.ensurePermissionRequestHook(hooksObj, "node /new/path/hook-relay-blocking.js", 10800) + val h = hooksObj.getJSONArray("PermissionRequest") + .getJSONObject(0).getJSONArray("hooks").getJSONObject(0) + assertEquals(10800, h.getInt("timeout")) + assertEquals("node /new/path/hook-relay-blocking.js", h.getString("command")) + } + + @Test + fun `appends a new entry when none exists`() { + val hooksObj = JSONObject() + Bootstrap.ensurePermissionRequestHook(hooksObj, "node /p/hook-relay-blocking.js", 10800) + val arr = hooksObj.getJSONArray("PermissionRequest") + assertEquals(1, arr.length()) + val h = arr.getJSONObject(0).getJSONArray("hooks").getJSONObject(0) + assertEquals(10800, h.getInt("timeout")) + assertEquals("command", h.getString("type")) + } + + @Test + fun `does not duplicate on repeat runs`() { + val hooksObj = existingHooks(300) + Bootstrap.ensurePermissionRequestHook(hooksObj, "node /p/hook-relay-blocking.js", 10800) + Bootstrap.ensurePermissionRequestHook(hooksObj, "node /p/hook-relay-blocking.js", 10800) + assertEquals(1, hooksObj.getJSONArray("PermissionRequest").length()) + } +} diff --git a/desktop/docs/blocking-relay-handoff.md b/desktop/docs/blocking-relay-handoff.md index 258dcf78c..eac29c4ff 100644 --- a/desktop/docs/blocking-relay-handoff.md +++ b/desktop/docs/blocking-relay-handoff.md @@ -39,28 +39,36 @@ Keep PTY detection as the catch-all (for trust gates, non-hook menus), but add s The relay writes its payload and **waits**. The server decides what happens: - **Fire-and-forget:** Server closes socket without writing → relay sees `end` → exits 0 (backward compatible) -- **Blocking allow:** Server holds socket, writes `{"allow":true}\n` → relay exits 0 -- **Blocking deny:** Server holds socket, writes `{"allow":false}\n` → relay exits 2 -- **Timeout safety:** Relay has a configurable timeout (default 30s, via `CLAUDE_RELAY_TIMEOUT` env var). If server goes silent → relay exits 0 (fail-open) +- **Blocking decision:** Server holds socket, writes a decision in the real nested shape — `{"decision":{"behavior":"allow"}}` or `{"decision":{"behavior":"deny"}}` (see `main.ts`'s `hookRelay.respond()` calls and `relay-blocking.js`, which reads `appDecision.decision` and re-wraps it as `hookSpecificOutput.decision`) → relay wraps it in `hookSpecificOutput` and exits 0 either way; Claude Code reads the `decision` field, not the exit code. The nesting matters: a flat `{"decision":"allow"}` or bare-string decision reaches Claude Code as `decision: undefined`, silently dropping the answer. Exit 2 is the timeout path only (see below) — there is no deny-specific exit code. +- **Timeout safety:** Relay has a configurable timeout (default 2h30m, via `CLAUDE_RELAY_TIMEOUT` env var). If the server goes silent past it → relay exits 2 (fail-closed deny) Key property: **relay doesn't need to know which hooks are blocking.** The server decides. This means adding new blocking hook types only requires server-side changes. -### Spike Test Results (VALIDATED, 4/4 PASS) +### Spike Test Results (re-run 2026-07-31, 4/4 PASS) -We created `hook-scripts/relay-blocking.js` (the new relay) and `scripts/test-blocking-relay.js` (test harness). Results: +We created `hook-scripts/relay-blocking.js` (the new relay) and `docs/test-blocking-relay.js` (test harness). The harness previously asserted a deny-specific exit code (`exit 2`) that never matched shipped behavior — the relay has no such path; a delivered deny exits 0 like any other delivered decision, with the decision riding in stdout's `hookSpecificOutput`. Test 2's handler also previously sent the legacy flat `{"allow":true}` shape instead of the real nested `{"decision":{"behavior":"allow"}}` shape. Both were corrected to match `relay-blocking.js` as written, then re-run for real numbers (`node desktop/docs/test-blocking-relay.js` from the repo root; Linux, not Windows — see note below): ``` === Blocking Relay Protocol Spike Test === -[TEST] Fire-and-forget (server closes immediately)... PASS (exit=0, expected=0, 413ms) -[TEST] Blocking allow (server sends allow=true)... PASS (exit=0, expected=0, 853ms) -[TEST] Blocking deny (server sends allow=false)... PASS (exit=2, expected=2, 853ms) -[TEST] Timeout (server holds, relay fails open)... PASS (exit=0, expected=0, 3365ms) +[TEST] Fire-and-forget (server closes immediately)... PASS (exit=0, expected=0, 333ms) +[TEST] Blocking allow (server sends allow decision)... PASS (exit=0, expected=0, 839ms) +[TEST] Blocking deny (server sends deny decision, exits 0 — decision rides in stdout)... PASS (exit=0, expected=0, 831ms) +[TEST] Timeout (server holds, relay fails CLOSED — exit 2 deny)... PASS (exit=2, expected=2, 3348ms) ``` -The pipe protocol works on Windows. Backward compatibility confirmed. +The pipe protocol works. Backward compatibility confirmed. (This re-run was executed on Linux, where `net.createServer().listen(PIPE_NAME)` binds the Windows-style pipe string as a plain Unix socket file — the harness doesn't branch on platform. Timings are illustrative for this machine/run, not a perf contract.) ## Implementation Plan +> **Historical — this plan shipped, with two deliberate divergences.** Read it as +> the original spike's proposal, not as current API docs. What actually shipped: +> (1) the blocking hook is **`PermissionRequest`**, not `PreToolUse` (Step 5 below +> guessed this might become possible — it did); (2) `respond()` takes the nested +> **decision object** `{ decision: { behavior: 'allow' | 'deny' } }`, not the +> `allow: boolean` / `{"allow": …}` shape sketched in Steps 2, 3 and 5 — see the +> corrected protocol bullet near the top of this file. For current behavior, +> trust `hook-relay.ts` and `relay-blocking.js`, not this section. + ### Step 1: Replace relay.js with relay-blocking.js - `relay-blocking.js` is a drop-in replacement — when server closes without writing back, behavior is identical to current `relay.js` - Rename or replace; update any references in `scripts/install-hooks.js` or hook config @@ -120,6 +128,6 @@ The pipe protocol works on Windows. Backward compatibility confirmed. ## Design Constraints - PTY-based Ink detection (`usePromptDetector`, `PromptCard`, `TrustGate`) must remain as the fallback for non-hook interactive prompts. Do not remove or break it. -- The protocol must fail-open (exit 0) on timeout or error — Claude Code must never deadlock waiting for a response that will never come. +- The protocol fails CLOSED (exit 2) on timeout, and OPEN (exit 0) on connection error — no listener means a terminal session, which gets CC's own prompt. Claude Code must never deadlock waiting for a response that will never come; the staggered timeout tiers (2026-07-30 spec §1) are what guarantee that. - `relay-blocking.js` must remain backward-compatible with the current fire-and-forget server behavior. - The spec is at `~/.claude/specs/claude-desktop-ui-spec.md` — update the "Planned updates" section when done to reflect that blocking approval flow is implemented. diff --git a/desktop/docs/test-blocking-relay.js b/desktop/docs/test-blocking-relay.js index c1eb0ac6e..c75778b17 100644 --- a/desktop/docs/test-blocking-relay.js +++ b/desktop/docs/test-blocking-relay.js @@ -4,9 +4,11 @@ * * Runs four scenarios against relay-blocking.js: * 1. Fire-and-forget: server closes immediately → relay exits 0 - * 2. Blocking allow: server sends {"allow":true} → relay exits 0 - * 3. Blocking deny: server sends {"allow":false} → relay exits 2 - * 4. Timeout: server holds forever → relay exits 0 after timeout + * 2. Blocking allow: server sends {"decision":{"behavior":"allow"}} → relay exits 0 + * 3. Blocking deny: server sends {"decision":{"behavior":"deny"}} → relay exits 0 + * (the decision rides in stdout's hookSpecificOutput, not the exit code — + * there is no deny-specific exit code; see relay-blocking.js) + * 4. Timeout: server holds forever → relay exits 2 (fail-closed) after timeout * * Usage: node scripts/test-blocking-relay.js */ @@ -120,28 +122,39 @@ async function main() { socket.end(); }, { expectedCode: 0 }); - // Test 2: Blocking allow — server sends allow response - await runTest('Blocking allow (server sends allow=true)', (socket, payload) => { + // Test 2: Blocking allow — server sends an allow decision in the real + // nested wire shape ({ decision: { behavior: 'allow' } }). A flat or + // bare-string decision would ship `decision: undefined` to Claude Code — + // see relay-blocking.js, which reads `appDecision.decision` and re-wraps + // it as `hookSpecificOutput.decision`. + await runTest('Blocking allow (server sends allow decision)', (socket, payload) => { // Simulate a brief "thinking" delay, then approve const parsed = JSON.parse(payload); log(` Received: ${parsed.hook_event_name} tool=${parsed.tool_name}`); setTimeout(() => { - socket.end(JSON.stringify({ allow: true }) + '\n'); + socket.end(JSON.stringify({ decision: { behavior: 'allow' } }) + '\n'); }, 500); }, { expectedCode: 0 }); - // Test 3: Blocking deny — server sends deny response - await runTest('Blocking deny (server sends allow=false)', (socket, payload) => { + // Test 3: Blocking deny — server sends a deny decision. The relay does not + // special-case deny: it wraps whatever `decision` the server sent into + // hookSpecificOutput on stdout and exits 0, same as allow. Exit 2 is + // reserved for the timeout path (test 4), never for a delivered decision. + await runTest('Blocking deny (server sends deny decision, exits 0 — decision rides in stdout)', (socket, payload) => { setTimeout(() => { - socket.end(JSON.stringify({ allow: false }) + '\n'); + socket.end(JSON.stringify({ decision: { behavior: 'deny' } }) + '\n'); }, 500); - }, { expectedCode: 2 }); + }, { expectedCode: 0 }); // Test 4: Timeout — server holds socket open, never responds - // Override relay timeout to 3s so this test doesn't take 30s - await runTest('Timeout (server holds, relay fails open)', (socket, _payload) => { + // Override relay timeout to 3s so this test doesn't take 2h30m (shipped + // default). Fails CLOSED (exit 2) — the pre-2026 fail-open (exit 0) + // contract this used to pin was replaced by the 2026-07-30 staggered + // timeout tiers; a silent auto-allow on timeout is the wrong failure mode + // for a permission gate. + await runTest('Timeout (server holds, relay fails CLOSED — exit 2 deny)', (socket, _payload) => { // Deliberately do nothing — hold the socket open - }, { expectedCode: 0, timeout: 10000, env: { CLAUDE_RELAY_TIMEOUT: '3000' } }); + }, { expectedCode: 2, timeout: 10000, env: { CLAUDE_RELAY_TIMEOUT: '3000' } }); // Summary console.log('\n=== Summary ==='); diff --git a/desktop/hook-scripts/relay-blocking.js b/desktop/hook-scripts/relay-blocking.js index d848ac9d3..7be76d033 100644 --- a/desktop/hook-scripts/relay-blocking.js +++ b/desktop/hook-scripts/relay-blocking.js @@ -16,10 +16,16 @@ const net = require('net'); const os = require('os'); const path = require('path'); const PIPE_NAME = process.env.CLAUDE_DESKTOP_PIPE || (process.platform === 'win32' ? '\\\\.\\pipe\\claude-desktop-hooks' : path.join(os.tmpdir(), 'claude-desktop-hooks.sock')); -// Default 300s to match the Claude Code hook timeout in settings.json. -// If the relay times out before Claude Code's hook timeout, it exits with -// code 2 (deny), causing an auto-deny before the user can respond. -const TIMEOUT_MS = parseInt(process.env.CLAUDE_RELAY_TIMEOUT || '300000', 10); +// Tier-2 backstop: 2h30m — deliberately 30m LONGER than the app's own 2h hold +// (hook-relay.ts APP_HOLD_MS: the app must answer first, it's the only party +// that can label the card accurately) and 30m SHORTER than the Claude Code +// hook timeout in settings.json (install-hooks.js, 10800s): if the app hangs, +// this relay must fire before CC does. Relay timeout = exit 2 = clean deny +// that unblocks the turn; a CC hook-kill delivers NO decision, and +// AskUserQuestion then waits forever on CC's default-"never" question +// timeout. Do NOT tidy these back to equal — that restores the silent +// 5-minute wedge this replaced (2026-07-30 spec §1). +const TIMEOUT_MS = parseInt(process.env.CLAUDE_RELAY_TIMEOUT || '9000000', 10); let input = ''; process.stdin.setEncoding('utf8'); diff --git a/desktop/scripts/install-hooks.js b/desktop/scripts/install-hooks.js index 40ff91aee..7217deac3 100644 --- a/desktop/scripts/install-hooks.js +++ b/desktop/scripts/install-hooks.js @@ -111,7 +111,9 @@ function installHooks() { } } - // Register PermissionRequest with blocking relay (longer timeout for user response) + // Register PermissionRequest with blocking relay. Tier-3 backstop: 3h — + // 30m ABOVE the relay's 2h30m so CC never wins (CC winning kills the hook + // with no decision; see relay-blocking.js header). Margins are load-bearing. if (!settings.hooks['PermissionRequest']) { settings.hooks['PermissionRequest'] = []; } @@ -123,7 +125,7 @@ function installHooks() { const blockingEntry = { matcher: '', - hooks: [{ type: 'command', command: expectedBlockingCmd, timeout: 300 }], + hooks: [{ type: 'command', command: expectedBlockingCmd, timeout: 10800 }], }; const existingBlockingIdx = settings.hooks['PermissionRequest'].findIndex((matcher) => diff --git a/desktop/src/main/hook-relay.ts b/desktop/src/main/hook-relay.ts index 9f1bc09e6..cf1853733 100644 --- a/desktop/src/main/hook-relay.ts +++ b/desktop/src/main/hook-relay.ts @@ -11,6 +11,18 @@ const DEFAULT_PIPE_NAME = process.platform === 'win32' ? '\\\\.\\pipe\\claude-desktop-hooks' : path.join(os.tmpdir(), 'claude-desktop-hooks.sock'); +// §1 tier-1 hold (2026-07-30 spec): the APP owns the permission-ask clock — +// 2h here < 2h30m relay backstop (relay-blocking.js) < 3h CC hook entry +// (install-hooks.js). Margins are load-bearing: if CC ever wins it kills the +// hook with NO decision and AskUserQuestion waits forever on CC's +// default-"never" question timeout. Do not equalize. NOTE: setTimeout does +// not advance during system suspend, so the hold can stretch past 2h of +// wall-clock on a laptop that slept — expected, not a bug. +export const APP_HOLD_MS = 7200000; +// §1a dead-man cap: an ask whose sessionId matches no live session will never +// render a card anywhere — a 2h hold would be a 2h invisible hang. +export const UNROUTABLE_HOLD_MS = 60000; + export class HookRelay extends EventEmitter { private server: net.Server | null = null; private running = false; @@ -20,10 +32,30 @@ export class HookRelay extends EventEmitter { // a live permission/AskUserQuestion menu and must not be typed into. private pendingSockets = new Map(); private pipeName: string; + // requestId → the app-owned hold timer for that pending ask. Cleared on + // respond()/closeSocket()/socket-close/stop() — a leaked 2h timer would + // hold a socket reference alive, and firing one after teardown would + // respond() into a dead/reused socket. + private holdTimers = new Map(); + private sessionGate: ((sessionId: string) => boolean) | null = null; + private readonly holdMs: number; + private readonly unroutableHoldMs: number; - constructor(pipeName?: string) { + constructor(pipeName?: string, holdMs: number = APP_HOLD_MS, unroutableHoldMs: number = UNROUTABLE_HOLD_MS) { super(); this.pipeName = pipeName || DEFAULT_PIPE_NAME; + this.holdMs = holdMs; + this.unroutableHoldMs = unroutableHoldMs; + } + + /** main.ts wires this to SessionManager (mirrors setReloadPluginsGate). */ + setSessionGate(gate: (sessionId: string) => boolean): void { + this.sessionGate = gate; + } + + private clearHold(requestId: string): void { + const t = this.holdTimers.get(requestId); + if (t) { clearTimeout(t); this.holdTimers.delete(requestId); } } private parseHookPayload(data: string): HookEvent { @@ -62,13 +94,61 @@ export class HookRelay extends EventEmitter { event.payload._requestId = requestId; this.emit('hook-event', event); + // §1 tier-1: the app ends the wait, with a labeled deny. §1a: an + // unroutable ask (no live session at arrival) gets the short + // dead-man cap instead — restores what the old 300s timeout was + // silently doing for that case. + const routable = this.sessionGate ? this.sessionGate(event.sessionId) : true; + const holdMs = routable ? this.holdMs : this.unroutableHoldMs; + this.holdTimers.set(requestId, setTimeout(() => { + this.holdTimers.delete(requestId); + // Nested { decision: { … } } is load-bearing: relay-blocking.js + // reads appDecision.decision — a flat shape ships undefined. + // The message lands VERBATIM in the denied tool result the model + // reads (verified in the CC 2.1.220 binary), so say what + // happened and invite a re-ask. + // Fix: pluralize naturally (was the literal "hour(s)" landing in + // the model-visible message). Android derives the same way from + // its own PERMISSION_HOLD_MS (EventBridge.kt) — keep both in + // sync if this tier value ever changes; permission-timeout- + // margins.test.ts pins the underlying numbers. + const holdHours = Math.round(this.holdMs / 3600000); + // Fix: only claim "auto-denied" if the deny actually reached the + // socket. respond() returns false when the pending entry is + // already gone or the socket is already destroyed — nothing was + // written in that case, so emitting unconditionally would assert + // a cause that never happened (docs/error-message-standards.md). + // Mirrors Android's `if (respond(requestId, deny)) { emit }` + // guard (EventBridge.kt) — the two platforms deliberately agree. + const delivered = this.respond(requestId, { + decision: { + behavior: 'deny', + message: routable + ? `YouCoded auto-denied this request after ${holdHours} hour${holdHours === 1 ? '' : 's'} with no user response — ask again if still needed.` + : 'YouCoded could not route this request to any open session — auto-denied. Ask again if still needed.', + }, + }); + // respond() deletes the pending entry BEFORE 'close' fires, so + // the close handler's wasOpen guard swallows any emit — + // app-initiated endings must emit explicitly (spec §2). + if (delivered) { + this.emit('permission-expired', event.sessionId, requestId, + routable ? 'app-timeout' : 'unroutable'); + } + }, holdMs)); + // When the socket closes (relay timeout, Claude Code kills hook, // or network error), notify listeners so the UI can clear the // awaiting-approval state instead of leaving dead buttons. socket.on('close', () => { + this.clearHold(requestId); const wasOpen = this.pendingSockets.delete(requestId); if (wasOpen) { - this.emit('permission-expired', event.sessionId, requestId); + // Reachable ONLY when the far end went away first (relay + // timeout/death, CC killing the hook): app-initiated paths + // delete the entry before 'close' fires and emit their own + // reason. That asymmetry IS the §2 discrimination. + this.emit('permission-expired', event.sessionId, requestId, 'hook-closed'); } }); } else { @@ -154,6 +234,7 @@ export class HookRelay extends EventEmitter { } respond(requestId: string, decision: object): boolean { + this.clearHold(requestId); const pending = this.pendingSockets.get(requestId); if (!pending || pending.socket.destroyed) { this.pendingSockets.delete(requestId); @@ -166,6 +247,7 @@ export class HookRelay extends EventEmitter { } closeSocket(requestId: string): void { + this.clearHold(requestId); const pending = this.pendingSockets.get(requestId); if (pending && !pending.socket.destroyed) { pending.socket.end(); @@ -187,6 +269,11 @@ export class HookRelay extends EventEmitter { } stop(): void { + // Clear all app-owned hold timers first — otherwise a still-pending + // timer could fire respond() into a socket we're about to tear down. + for (const t of this.holdTimers.values()) clearTimeout(t); + this.holdTimers.clear(); + // Clean up all pending permission sockets for (const [, pending] of this.pendingSockets) { if (!pending.socket.destroyed) { diff --git a/desktop/src/main/main.ts b/desktop/src/main/main.ts index c03fb38dd..ccfa92fd6 100644 --- a/desktop/src/main/main.ts +++ b/desktop/src/main/main.ts @@ -179,6 +179,8 @@ const hookRelay = new HookRelay(pipeName); // live permission/AskUserQuestion menu — typing into that menu presses Enter // on the highlighted option and silently answers the prompt (stray-Enter fix). sessionManager.setReloadPluginsGate((sessionId) => hookRelay.hasPendingPermission(sessionId)); +// §1a: unroutable asks get the 60s dead-man cap instead of the 2h hold. +hookRelay.setSessionGate((sessionId) => sessionManager.hasSession(sessionId)); const remoteConfig = new RemoteConfig(); const skillProvider = new LocalSkillProvider(); skillProvider.ensureMigrated(); @@ -924,12 +926,16 @@ function createWindow(firstRunManager?: FirstRunManager) { } }); - // Notify renderer when a permission request socket closes (timeout/killed) - hookRelay.on('permission-expired', (sessionId: string, requestId: string) => { + // Notify renderer when a permission request ends — either app-initiated + // (hold timer / unroutable cap, with a labeled reason) or because the far + // end went away first (hook-closed). See hook-relay.ts §1/§2. + hookRelay.on('permission-expired', (sessionId: string, requestId: string, reason?: string) => { const evt = { type: 'PermissionExpired', sessionId, - payload: { _requestId: requestId }, + // _reason rides INSIDE the payload — no IPC channel shape change, so + // ipc-channels.test.ts needs nothing and old remote shims just ignore it. + payload: { _requestId: requestId, _reason: reason }, timestamp: Date.now(), }; const ownerId = windowRegistry.getOwner(sessionId); diff --git a/desktop/src/main/session-manager.ts b/desktop/src/main/session-manager.ts index dba322a8a..8ac73f2a7 100644 --- a/desktop/src/main/session-manager.ts +++ b/desktop/src/main/session-manager.ts @@ -315,6 +315,11 @@ export class SessionManager extends EventEmitter { return this.sessions.get(id)?.info; } + /** §1a routability gate — true when this sessionId belongs to a live session. */ + hasSession(sessionId: string): boolean { + return this.sessions.has(sessionId); + } + destroyAll(): void { for (const [id] of this.sessions) { this.destroySession(id); diff --git a/desktop/src/renderer/App.tsx b/desktop/src/renderer/App.tsx index 8bdfb9ac1..23d3c9702 100644 --- a/desktop/src/renderer/App.tsx +++ b/desktop/src/renderer/App.tsx @@ -38,7 +38,7 @@ import { dispatchSlashCommand, type DispatcherResult } from './state/slash-comma import { runNativeSlashAction, routeSlashResult } from './state/native-slash-actions'; import { GameProvider, useGameState, useGameDispatch } from './state/game-context'; import { hookEventToAction } from './state/hook-dispatcher'; -import { hasPendingInteraction, canPtySend } from './state/pty-input-gate'; +import { hasPendingInteraction, pendingInteractionKind, pendingInteractionRefusalCopy, canPtySend } from './state/pty-input-gate'; import { buildOutgoingMessage } from './components/outgoing-message'; import type { SyncWarning } from '../main/sync-state'; import { usePromptDetector } from './hooks/usePromptDetector'; @@ -526,7 +526,11 @@ function AppInner() { const notifyIfPtyBlocked = useCallback((sid: string): boolean => { const session = chatStateMapRef.current.get(sid); if (session && hasPendingInteraction(session)) { - setToast('Claude is waiting for your response — answer the prompt first.'); + // Name the blocker — under the 2h app-owned hold (Task 9) a generic + // line reads as a mystery lock once it's been sitting there a while. + // An 'approval' card carries its own outs (answer / Dismiss) even + // after it's expired, so point at it instead of the generic copy. + setToast(pendingInteractionRefusalCopy(pendingInteractionKind(session))); return true; } return false; @@ -2935,7 +2939,19 @@ function AppInner() { {/* TerminalToolbar (Esc/Tab/Ctrl/arrows) now renders inside ChatInputBar when minimal={isTerminalTouch}, slotted in the QuickChips position so both modes share one container. */} - setResumeRequested(true)} getUsageSnapshot={getUsageSnapshot} onOpenPreferences={() => setPreferencesOpen(true)} onToast={(msg) => setToast(msg)} onSendBlocked={(retry) => setToast({ message: 'Claude is waiting for your response — answer the prompt first.', durationMs: 8000, action: { label: 'Send anyway', onClick: () => { setToast(null); retry(); } } })} getSessionState={(sid) => chatStateMapRef.current.get(sid)} onOpenModelPicker={() => setModelPickerOpen(true)} initialInput={currentSession?.initialInput} provider={currentSession?.provider} /> + setResumeRequested(true)} getUsageSnapshot={getUsageSnapshot} onOpenPreferences={() => setPreferencesOpen(true)} onToast={(msg) => setToast(msg)} onSendBlocked={(retry) => { + // "Send anyway" writes straight into the live Ink menu (the + // exact stray-input class this gate exists to prevent) — + // name the actual blocker so reaching for it is an informed + // choice, not a reflex against a generic-sounding lock. + const blockedSession = chatStateMapRef.current.get(sessionId ?? ''); + // Route through the shared helper even in the no-session + // fallback — hardcoding its `null` output here let this + // site silently drift from the other two on any future + // copy edit (found in review of 5fb40acd). + const message = pendingInteractionRefusalCopy(blockedSession ? pendingInteractionKind(blockedSession) : null); + setToast({ message, durationMs: 8000, action: { label: 'Send anyway', onClick: () => { setToast(null); retry(); } } }); + }} getSessionState={(sid) => chatStateMapRef.current.get(sid)} onOpenModelPicker={() => setModelPickerOpen(true)} initialInput={currentSession?.initialInput} provider={currentSession?.provider} /> (function InputBar({ sessionId sendRef.current(true); }); } else { - onToast?.('Claude is waiting for your response — answer the prompt first.'); + // Same copy as the onSendBlocked branch above (and App.tsx's + // notifyIfPtyBlocked) — pendingInteractionRefusalCopy is the one + // source both refusal sites read from so they can't drift apart. + onToast?.(pendingInteractionRefusalCopy(pendingInteractionKind(session))); } return false; } diff --git a/desktop/src/renderer/components/ToolCard.test.tsx b/desktop/src/renderer/components/ToolCard.test.tsx index 031a48e04..e7743e1f3 100644 --- a/desktop/src/renderer/components/ToolCard.test.tsx +++ b/desktop/src/renderer/components/ToolCard.test.tsx @@ -1,8 +1,8 @@ // @vitest-environment jsdom import '@testing-library/jest-dom/vitest'; import React from 'react'; -import { describe, it, expect, beforeEach } from 'vitest'; -import { render, screen, cleanup } from '@testing-library/react'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; import { ChatProvider } from '../state/chat-context'; import ToolCard from './ToolCard'; import type { ToolCallState } from '../../shared/types'; @@ -62,3 +62,140 @@ describe('ToolCard — Skill compact variant', () => { expect(screen.queryByTestId('tool-card-chevron')).not.toBeNull(); }); }); + +describe('ToolCard — expired approval card', () => { + const originalClaude = (window as any).claude; + + beforeEach(() => { + cleanup(); + (window as any).claude = { remote: { broadcastAction: () => {} } }; + }); + + afterEach(() => { + (window as any).claude = originalClaude; + }); + + it('renders a normal (non-expired) awaiting-approval card with Yes/No, not header-only', () => { + const tool = makeTool({ + toolName: 'Bash', + input: { command: 'ls' }, + status: 'awaiting-approval', + requestId: 'req-1', + }); + render( + + + + ); + expect(screen.getByText('Yes')).toBeInTheDocument(); + expect(screen.getByText('No')).toBeInTheDocument(); + }); + + // The bug this task fixes: retention (Task 3) clears requestId on a + // retained card, but the old gate (`tool.requestId &&`) required it — so a + // retained card rendered header-only, with no explanation and no way out, + // on a session whose red attention dot stayed lit. + it('widens the gate so an expired card (requestId cleared) still renders UI', () => { + const tool = makeTool({ + toolName: 'Bash', + input: { command: 'rm -rf /tmp/x' }, + status: 'awaiting-approval', + requestId: undefined, + expired: true, + }); + render( + + + + ); + expect( + screen.getByText(/The buttons on this card timed out, but Claude may still be/) + ).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /Dismiss — I answered in the terminal/ })).toBeInTheDocument(); + // The old Yes/No/Always Allow row must NOT render — requestId is gone, + // so those buttons could never deliver a response anyway. + expect(screen.queryByText('Yes')).toBeNull(); + expect(screen.queryByText('No')).toBeNull(); + }); + + it('Dismiss on an expired card dispatches PERMISSION_CARD_RESOLVED and mirrors to remote', () => { + const broadcastAction = vi.fn(); + (window as any).claude = { remote: { broadcastAction } }; + const tool = makeTool({ + toolName: 'Bash', + input: { command: 'ls' }, + status: 'awaiting-approval', + requestId: undefined, + expired: true, + toolUseId: 'toolu_expired_1', + }); + render( + + + + ); + fireEvent.click(screen.getByRole('button', { name: /Dismiss — I answered in the terminal/ })); + expect(broadcastAction).toHaveBeenCalledWith({ + type: 'PERMISSION_CARD_RESOLVED', + sessionId: 's1', + toolUseId: 'toolu_expired_1', + }); + }); + + it('AskUserQuestion also gets the expired branch with Dismiss as its only out', () => { + const tool = makeTool({ + toolName: 'AskUserQuestion', + input: { + questions: [ + { question: 'Pick one', header: 'Choice', multiSelect: false, options: [{ label: 'A' }, { label: 'B' }] }, + ], + }, + status: 'awaiting-approval', + requestId: undefined, + expired: true, + }); + render( + + + + ); + expect(screen.getByRole('button', { name: /Dismiss — I answered in the terminal/ })).toBeInTheDocument(); + // The question options themselves must not render — there's no live + // socket to submit answers through. + expect(screen.queryByText('Pick one')).toBeNull(); + }); + + // A delivered===false response means the socket is provably gone — under + // the new retention rules (Task 3), a bare PERMISSION_EXPIRED with no + // reason would still resolve (reason absent defaults to resolve), but this + // pins the intent explicitly so a future reducer default change can't + // silently start retaining these. + it('tags delivery failure (delivered === false) with reason: delivery-failed', () => { + const broadcastAction = vi.fn(); + (window as any).claude = { + remote: { broadcastAction }, + session: { respondToPermission: vi.fn().mockResolvedValue(false) }, + }; + const tool = makeTool({ + toolName: 'Bash', + input: { command: 'ls' }, + status: 'awaiting-approval', + requestId: 'req-1', + toolUseId: 'toolu_del_fail', + }); + render( + + + + ); + fireEvent.click(screen.getByText('Yes')); + return Promise.resolve().then(() => { + expect(broadcastAction).toHaveBeenCalledWith({ + type: 'PERMISSION_EXPIRED', + sessionId: 's1', + requestId: 'req-1', + reason: 'delivery-failed', + }); + }); + }); +}); diff --git a/desktop/src/renderer/components/ToolCard.tsx b/desktop/src/renderer/components/ToolCard.tsx index 48e04c279..266437508 100644 --- a/desktop/src/renderer/components/ToolCard.tsx +++ b/desktop/src/renderer/components/ToolCard.tsx @@ -9,6 +9,9 @@ import { isAndroid } from '../platform'; import ToolBody from './tool-views/ToolBody'; import { useExpandAllToggle, getInitialExpanded } from '../hooks/useExpandAllToggle'; import { isTypingTarget } from '../utils/is-typing-target'; +import { getVisibleScreenText } from '../hooks/terminal-registry'; +import { parseInkSelect, rebindButtons, type PromptButton } from '../parser/ink-select-parser'; +import type { ChatAction } from '../state/chat-types'; // --- Helpers for friendly display --- @@ -495,6 +498,115 @@ function PlanApprovalButtons({ requestId, sessionId, onResponded }: { ); } +// --- Expired-card rebind UI (2026-07-30 spec §3) --- +// A card lands here only after Task 4's retention path sets `expired`: the +// hook socket died but CC's Ink menu may STILL be live in the terminal. +// Re-parse the visible buffer periodically and, if the parse yields a menu +// whose every option carries a bare digit, render THAT menu's own labels as +// buttons that type the digit straight into the PTY. + +/** How often to re-parse the terminal buffer while an expired card is on + * screen — cheap because this component only ever mounts on expired cards, + * which are rare and short-lived (the menu-absence rule below resolves them + * within a couple of buffer flushes once the digit lands). */ +const REBIND_POLL_MS = 2000; + +/** How long a clicked rebind button stays disabled before re-arming. The + * click does NOT resolve the card — nothing here confirms the write + * landed, since the hook socket is dead and no response can ever arrive on + * it. Resolution comes exclusively from usePromptDetector's menu-absence + * rule (Task 4): once the Ink menu has been gone from the buffer for two + * consecutive flushes, it dispatches PERMISSION_CARD_RESOLVED. So a + * successful digit write makes the menu disappear, which resolves the card + * on its own; a write that didn't land leaves the menu (and this card) up, + * and the buttons must be clickable again — hence re-arming on a timer + * rather than on any signal we'd have to invent. */ +const REBIND_REARM_MS = 2000; + +/** Expired-card actions. Re-parses the live buffer every REBIND_POLL_MS: if + * CC's menu is still up AND every option has a digit (rebindButtons' + * gate — see its own header for why), render the MENU'S OWN options as + * digit-writing buttons; otherwise fall back to copy + Dismiss only (the + * only case in the Workbench, which has no real terminal buffer to parse). + * + * The digit write is a DELIBERATE menu-driving write — like + * PlanApprovalButtons and TrustGate, it must NOT go through + * pty-input-gate.ts (see that module's header: "driving the menu is their + * whole purpose"). Unlike PlanApprovalButtons' `DOWN.repeat(i) + '\r'` (a + * known-broken write shape — arrows in a write that ends with `\r` are + * discarded, see menuToButtons' header — already filed separately, not + * fixed here), this only ever sends a bare digit: select-and-submit in one + * byte, no `\r`, nothing for CC to collapse. */ +function ExpiredApprovalActions({ sessionId, tool, dispatch }: { + sessionId?: string; + tool: ToolCallState; + dispatch: React.Dispatch; +}) { + const [buttons, setButtons] = useState(null); + const [clicked, setClicked] = useState(false); + const rearmTimerRef = useRef | null>(null); + + useEffect(() => { + if (!sessionId) return; + const parse = () => { + const screen = getVisibleScreenText(sessionId); + setButtons(rebindButtons(screen ? parseInkSelect(screen) : null, tool.toolName)); + }; + parse(); + const poll = setInterval(parse, REBIND_POLL_MS); + return () => clearInterval(poll); + }, [sessionId, tool.toolName]); + + // Cleanup: don't let a stale re-arm timer fire setState after this card + // has already been resolved and unmounted. + useEffect(() => () => { + if (rearmTimerRef.current) clearTimeout(rearmTimerRef.current); + }, []); + + const resolveLocally = () => { + if (!sessionId) return; + const action: ChatAction = { type: 'PERMISSION_CARD_RESOLVED', sessionId, toolUseId: tool.toolUseId }; + dispatch(action); + (window as any).claude?.remote?.broadcastAction?.(action); + }; + + const writeDigit = (button: PromptButton) => { + if (!sessionId || clicked) return; + setClicked(true); + window.claude.session.sendInput(sessionId, button.input); + rearmTimerRef.current = setTimeout(() => setClicked(false), REBIND_REARM_MS); + }; + + return ( +
+

+ The buttons on this card timed out, but Claude may still be waiting + in the terminal.{buttons + ? ' These options come straight from the terminal menu:' + : ' Answer it there — or dismiss this if you already did.'} +

+ {buttons && ( +
+ {buttons.map((b) => ( + + ))} +
+ )} + +
+ ); +} + // --- AskUserQuestion UI --- // Claude Code's AskUserQuestion tool sends 1-4 multiple-choice questions. // Unlike regular tools (allow/deny), we must collect the user's selections @@ -782,7 +894,28 @@ export default React.memo(function ToolCard({ tool, sessionId, inGroup = false } {/* Permission / AskUserQuestion / ExitPlanMode UI */} - {tool.status === 'awaiting-approval' && tool.requestId && (() => { + {tool.status === 'awaiting-approval' && (tool.requestId || tool.expired) && (() => { + // Expired: the hook socket is dead (requestId cleared) but CC's Ink + // menu may still be live in the terminal. ExpiredApprovalActions + // (Task 9) re-parses the buffer and offers digit-rebind buttons when + // safe; Dismiss is the universal out (only out for AskUserQuestion, + // which never rebinds — spec §3/§1b). Copy says what is known + // (buttons stopped working), not what is assumed (the ask failed) — + // the user may have already answered in the terminal. + // + // WHY "answer it in the terminal" is safe copy: `expired` is only + // ever set by the hook/PTY permission path ('hook-closed' reason, + // src/shared/types.ts). The native permission broker never emits + // that reason, and native sessions have no terminal at all — so an + // expired card always implies a live PTY the user can act in. If a + // future change makes native cards expire too, this copy (and the + // rebind attempt below, which needs a real screen buffer) goes wrong + // silently — check that invariant still holds before touching it. + if (tool.expired || !tool.requestId) { + return ( + + ); + } // AskUserQuestion needs its own UI with option selection instead of Yes/No const isAskUser = tool.toolName === 'AskUserQuestion' && isValidQuestions(tool.input); // ExitPlanMode has a 4-option Ink menu in the CLI (bypass/manual/refine/feedback), @@ -797,7 +930,13 @@ export default React.memo(function ToolCard({ tool, sessionId, inGroup = false } }; const onFailedCb = () => { if (sessionId && tool.requestId) { - const action = { type: 'PERMISSION_EXPIRED' as const, sessionId, requestId: tool.requestId }; + // 'delivery-failed': the respond() write returned false or threw — + // the socket is provably gone, so this must RESOLVE the card, never + // retain it (retention would pin a card whose buttons cannot work). + const action = { + type: 'PERMISSION_EXPIRED' as const, sessionId, + requestId: tool.requestId, reason: 'delivery-failed' as const, + }; dispatch(action); (window as any).claude?.remote?.broadcastAction(action); } diff --git a/desktop/src/renderer/components/buddy/CompactToolStrip.test.tsx b/desktop/src/renderer/components/buddy/CompactToolStrip.test.tsx new file mode 100644 index 000000000..d3d7ee7a6 --- /dev/null +++ b/desktop/src/renderer/components/buddy/CompactToolStrip.test.tsx @@ -0,0 +1,131 @@ +// @vitest-environment jsdom +import '@testing-library/jest-dom/vitest'; +import React from 'react'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; +import { ChatProvider } from '../../state/chat-context'; +import { CompactToolStrip } from './CompactToolStrip'; +import type { ToolCallState } from '../../../shared/types'; + +function makeTool(overrides: Partial): ToolCallState { + return { + toolUseId: 'toolu_test', + toolName: 'Bash', + input: { command: 'ls' }, + status: 'complete', + ...overrides, + }; +} + +describe('CompactToolStrip — expired approval card', () => { + const originalClaude = (window as any).claude; + + beforeEach(() => { + cleanup(); + (window as any).claude = { remote: { broadcastAction: () => {} } }; + }); + + afterEach(() => { + (window as any).claude = originalClaude; + }); + + it('renders Allow/Deny/Always for a normal (non-expired) awaiting-approval tool', () => { + const tool = makeTool({ status: 'awaiting-approval', requestId: 'req-1' }); + render( + + + + ); + expect(screen.getByText('✓ Allow')).toBeInTheDocument(); + expect(screen.getByText('✕ Deny')).toBeInTheDocument(); + expect(screen.getByText('∞ Always')).toBeInTheDocument(); + }); + + // Same bug as ToolCard: the old gate (`tool.requestId &&`) required a live + // requestId, but a retained card clears it — so a retained card in the + // buddy strip showed an amber dot with no Allow/Deny/Always row AND no + // Dismiss, leaving the user with no in-app way to act on it. + it('widens the gate so an expired card (requestId cleared) shows a Dismiss button instead of nothing', () => { + const tool = makeTool({ status: 'awaiting-approval', requestId: undefined, expired: true }); + render( + + + + ); + expect(screen.getByText('Dismiss — I answered in the terminal')).toBeInTheDocument(); + expect(screen.queryByText('✓ Allow')).toBeNull(); + expect(screen.queryByText('✕ Deny')).toBeNull(); + expect(screen.queryByText('∞ Always')).toBeNull(); + }); + + it('Dismiss on an expired card dispatches PERMISSION_CARD_RESOLVED and mirrors to remote', () => { + const broadcastAction = vi.fn(); + (window as any).claude = { remote: { broadcastAction } }; + const tool = makeTool({ + status: 'awaiting-approval', + requestId: undefined, + expired: true, + toolUseId: 'toolu_expired_1', + }); + render( + + + + ); + fireEvent.click(screen.getByText('Dismiss — I answered in the terminal')); + expect(broadcastAction).toHaveBeenCalledWith({ + type: 'PERMISSION_CARD_RESOLVED', + sessionId: 's1', + toolUseId: 'toolu_expired_1', + }); + }); + + // Same rationale as ToolCard's delivery-failed test: delivered===false + // means the socket is provably gone, so this dispatch must RESOLVE (never + // retain) — tag it explicitly rather than relying on the reducer's default. + it('tags delivery failure (delivered === false) with reason: delivery-failed', async () => { + const broadcastAction = vi.fn(); + (window as any).claude = { + remote: { broadcastAction }, + session: { respondToPermission: vi.fn().mockResolvedValue(false) }, + }; + const tool = makeTool({ status: 'awaiting-approval', requestId: 'req-1' }); + render( + + + + ); + fireEvent.click(screen.getByText('✓ Allow')); + await Promise.resolve(); + await Promise.resolve(); + expect(broadcastAction).toHaveBeenCalledWith({ + type: 'PERMISSION_EXPIRED', + sessionId: 's1', + requestId: 'req-1', + reason: 'delivery-failed', + }); + }); + + it('tags a thrown respond() error with reason: delivery-failed', async () => { + const broadcastAction = vi.fn(); + (window as any).claude = { + remote: { broadcastAction }, + session: { respondToPermission: vi.fn().mockRejectedValue(new Error('socket closed')) }, + }; + const tool = makeTool({ status: 'awaiting-approval', requestId: 'req-1' }); + render( + + + + ); + fireEvent.click(screen.getByText('✓ Allow')); + await Promise.resolve(); + await Promise.resolve(); + expect(broadcastAction).toHaveBeenCalledWith({ + type: 'PERMISSION_EXPIRED', + sessionId: 's1', + requestId: 'req-1', + reason: 'delivery-failed', + }); + }); +}); diff --git a/desktop/src/renderer/components/buddy/CompactToolStrip.tsx b/desktop/src/renderer/components/buddy/CompactToolStrip.tsx index d26cfc5a6..7f57b65ec 100644 --- a/desktop/src/renderer/components/buddy/CompactToolStrip.tsx +++ b/desktop/src/renderer/components/buddy/CompactToolStrip.tsx @@ -168,13 +168,17 @@ function ToolRow({ decision, ); if (delivered === false) { - // Fix: Socket already closed — mark expired so the UI unsticks. + // Fix: Socket already closed — resolve the card so the UI unsticks. + // reason: 'delivery-failed' — the write returned false, so the + // socket is provably gone; this must RESOLVE, never retain (spec + // §2c/§2d — retention would pin a card whose buttons cannot work). // Reset responding so user can retry if needed. setResponding(false); const action = { type: 'PERMISSION_EXPIRED' as const, sessionId, requestId: tool.requestId, + reason: 'delivery-failed' as const, }; dispatch(action); (window as any).claude?.remote?.broadcastAction(action); @@ -190,12 +194,15 @@ function ToolRow({ (window as any).claude?.remote?.broadcastAction(action); } catch (err) { console.error('CompactToolStrip: failed to respond to permission:', err); - // Treat as expired so the card doesn't get stuck + // Treat as expired so the card doesn't get stuck. reason: + // 'delivery-failed' — same rationale as the delivered===false branch + // above: the throw means the socket is gone, so this must RESOLVE. if (tool.requestId) { const action = { type: 'PERMISSION_EXPIRED' as const, sessionId, requestId: tool.requestId, + reason: 'delivery-failed' as const, }; dispatch(action); (window as any).claude?.remote?.broadcastAction(action); @@ -248,38 +255,70 @@ function ToolRow({ > {target} - {/* Inline Allow / Deny / Always buttons only for awaiting-approval tools */} - {tool.status === 'awaiting-approval' && tool.requestId ? ( - - - - - + {/* Inline Allow / Deny / Always buttons only for awaiting-approval tools. + An expired card (requestId cleared, socket dead) gets a single + Dismiss instead — same quiet local resolve as ToolCard's, since + there's nothing left to respond to over the dead socket. */} + {tool.status === 'awaiting-approval' && (tool.requestId || tool.expired) ? ( + tool.expired || !tool.requestId ? ( + + + + ) : ( + + + + + + ) ) : null} ); diff --git a/desktop/src/renderer/dev/workbench/fixture-loader.test.ts b/desktop/src/renderer/dev/workbench/fixture-loader.test.ts index c1d94d39e..84c6534fd 100644 --- a/desktop/src/renderer/dev/workbench/fixture-loader.test.ts +++ b/desktop/src/renderer/dev/workbench/fixture-loader.test.ts @@ -79,6 +79,31 @@ describe('loadFixture', () => { } }); + it('retains an awaiting-approval card with expired:true on a permission_expired line', () => { + const raw = [ + '{"type":"tool_use","id":"toolu_01BashExpired","name":"Bash","input":{"command":"rm -rf node_modules && npm ci"}}', + '{"type":"permission_request","tool_use_id":"toolu_01BashExpired","requestId":"wb-expired-1","denyListed":false}', + '{"type":"permission_expired","tool_use_id":"toolu_01BashExpired","requestId":"wb-expired-1","reason":"hook-closed"}', + ].join('\n'); + + const result = loadFixture('bash-awaiting-approval-expired', raw); + + // permission_expired swaps the block pushed by permission_request in + // place — it must not add a second block for the same tool. + expect(result.blocks).toHaveLength(1); + expect(result.blocks[0].kind).toBe('tool'); + if (result.blocks[0].kind === 'tool') { + expect(result.blocks[0].tool).toMatchObject({ + toolUseId: 'toolu_01BashExpired', + toolName: 'Bash', + status: 'awaiting-approval', + expired: true, + requestId: undefined, + }); + } + expect(result.error).toBeUndefined(); + }); + it('returns an error field when the fixture is malformed', () => { const result = loadFixture('broken', 'not valid json\n'); diff --git a/desktop/src/renderer/dev/workbench/fixture-loader.ts b/desktop/src/renderer/dev/workbench/fixture-loader.ts index 40dca65fd..f63985356 100644 --- a/desktop/src/renderer/dev/workbench/fixture-loader.ts +++ b/desktop/src/renderer/dev/workbench/fixture-loader.ts @@ -157,6 +157,35 @@ export function loadFixture(name: string, raw: string, sessionId: string = SANDB const after = state.get(sessionId); const tool = after?.toolCalls.get(parsed.tool_use_id); if (tool) blocks.push({ kind: 'tool', tool }); + } else if (parsed.type === 'permission_expired') { + // WHY: reproduces the RETAINED card state from PERMISSION_EXPIRED + // ('hook-closed') — spec 2026-07-30 §2a. The hook socket died but + // Claude's menu may still be live in the terminal, so the card must + // NOT resolve to failed: it stays awaiting-approval, gains + // `expired: true`, loses its requestId, and swaps Yes/No for a + // "Dismiss — I answered in the terminal" control. This is the one + // card state Tasks 3-5 added that nothing else in the workbench can + // produce, so a fixture line must exist to review it (task-6-brief). + // Always follows a permission_request line for the same tool_use_id. + const action: ChatAction = { + type: 'PERMISSION_EXPIRED', + sessionId, + requestId: parsed.requestId, + reason: parsed.reason ?? 'hook-closed', + }; + state = chatReducer(state, action); + actions.push(action); + // The permission_request line above already pushed a block for this + // tool — but that block is a snapshot object, and PERMISSION_EXPIRED + // produces a NEW tool object (reducer spreads, never mutates). Swap + // the existing block in place so the gallery renders the post-expiry + // state instead of the stale awaiting-approval-with-buttons one. + const after = state.get(sessionId); + const tool = after?.toolCalls.get(parsed.tool_use_id); + if (tool) { + const idx = blocks.findIndex((b) => b.kind === 'tool' && b.tool.toolUseId === parsed.tool_use_id); + if (idx !== -1) blocks[idx] = { kind: 'tool', tool }; + } } // Unknown types are silently skipped (same policy as before). } diff --git a/desktop/src/renderer/dev/workbench/fixtures/tools/bash-awaiting-approval-expired.jsonl b/desktop/src/renderer/dev/workbench/fixtures/tools/bash-awaiting-approval-expired.jsonl new file mode 100644 index 000000000..3a37b4b2b --- /dev/null +++ b/desktop/src/renderer/dev/workbench/fixtures/tools/bash-awaiting-approval-expired.jsonl @@ -0,0 +1,3 @@ +{"type":"tool_use","id":"toolu_01BashExpired","name":"Bash","input":{"command":"rm -rf node_modules && npm ci","description":"Reinstall dependencies"}} +{"type":"permission_request","tool_use_id":"toolu_01BashExpired","requestId":"wb-expired-1","denyListed":false} +{"type":"permission_expired","tool_use_id":"toolu_01BashExpired","requestId":"wb-expired-1","reason":"hook-closed"} diff --git a/desktop/src/renderer/hooks/usePromptDetector.ts b/desktop/src/renderer/hooks/usePromptDetector.ts index 8fd9f17f7..4d78faecd 100644 --- a/desktop/src/renderer/hooks/usePromptDetector.ts +++ b/desktop/src/renderer/hooks/usePromptDetector.ts @@ -2,6 +2,7 @@ import { useEffect, useRef } from 'react'; import { parseInkSelect, menuToButtons } from '../parser/ink-select-parser'; import { useChatDispatch, useChatStore } from '../state/chat-context'; import { getVisibleScreenText, onBufferReady } from './terminal-registry'; +import { expiredToolIds, nextAbsentCount } from '../state/expired-card-resolver'; // How long to wait before showing a parser-detected prompt, giving the hook // system time to deliver a PermissionRequest via the named pipe relay. @@ -67,6 +68,10 @@ export function usePromptDetector() { const lastPermissionClearedRef = useRef>(new Map()); const prevAwaitingRef = useRef>(new Map()); + // §2 standing rule: per-session count of consecutive buffer flushes with no + // Ink menu while an expired card is retained. At 2, resolve the card(s). + const expiredAbsentRef = useRef>(new Map()); + // Perf: detect awaiting-approval transitions in an effect (off the render // path) and iterate activeTurnToolIds (current-turn only, per chat-reducer // rule #2) rather than the session-lifetime toolCalls Map. With many @@ -100,8 +105,44 @@ export function usePromptDetector() { // (the hook-based UI is handling the permission flow) const sessionState = store.getState().get(sid); if (sessionState) { + // §2 resolver runs BEFORE the awaiting-approval bail below — and that + // bail must ignore expired cards, or retention switches this whole + // detector off for the session, taking the resolver itself, the + // digit-rebind feature, and every unrelated setup PromptCard (trust + // gate, usage limit, resume) down with it (spec §2b). A retained card + // stays awaiting-approval indefinitely, so without this exemption the + // bail below would never fire again for this session. + const expired = expiredToolIds(sessionState); + if (expired.length > 0) { + // Menu-absence standing rule (spec §2): resolve only after the Ink + // menu has been absent for two consecutive buffer flushes — see + // expired-card-resolver.ts for why a one-shot check races both ways. + const expiredScreen = getVisibleScreenText(sid); + const menuPresent = !!(expiredScreen && parseInkSelect(expiredScreen)); + const prev = expiredAbsentRef.current.get(sid) ?? 0; + const { count, resolve } = nextAbsentCount(menuPresent, prev); + expiredAbsentRef.current.set(sid, count); + if (resolve) { + expiredAbsentRef.current.delete(sid); + for (const toolUseId of expired) { + const action = { type: 'PERMISSION_CARD_RESOLVED' as const, sessionId: sid, toolUseId }; + dispatch(action); + (window as any).claude?.remote?.broadcastAction?.(action); + } + } + } else { + // Nothing expired for this session — drop any stale counter so a + // FUTURE expiry starts its own count from zero instead of leaking + // state across unrelated permission asks. + expiredAbsentRef.current.delete(sid); + } for (const [, tool] of sessionState.toolCalls) { - if (tool.status === 'awaiting-approval') return; + // Live asks silence the parser below — the hook-based ToolCard + // owns that menu and a PromptCard must not double-render it. + // EXPIRED asks must NOT silence it — their menu has no live socket + // behind it, and the resolver above needs this function to keep + // running every flush to ever see two consecutive absences. + if (tool.status === 'awaiting-approval' && !tool.expired) return; } } @@ -161,11 +202,14 @@ export function usePromptDetector() { pendingTimerRef.current.delete(sid); // Re-check: if a PermissionRequest arrived during the debounce, - // a tool will be in awaiting-approval — don't show the prompt + // a tool will be in awaiting-approval — don't show the prompt. + // Same expired exemption as the top-of-flush bail above: an + // expired card must not block this setup-prompt PromptCard from + // showing (spec §2b). const currentSession = store.getState().get(sid); if (currentSession) { for (const [, tool] of currentSession.toolCalls) { - if (tool.status === 'awaiting-approval') return; + if (tool.status === 'awaiting-approval' && !tool.expired) return; } } diff --git a/desktop/src/renderer/parser/ink-select-parser.ts b/desktop/src/renderer/parser/ink-select-parser.ts index c7804bf96..c9125a963 100644 --- a/desktop/src/renderer/parser/ink-select-parser.ts +++ b/desktop/src/renderer/parser/ink-select-parser.ts @@ -384,3 +384,25 @@ export function menuToButtons(menu: ParsedMenu): PromptButton[] { return { label, input: DOWN.repeat(steps), submitInput: '\r' }; }); } + +/** §3 rebind gate (2026-07-30 spec): expired-card buttons may drive the + * still-live menu ONLY when every option carries a bare digit. A digit + * selects-and-submits in one byte with no cursor dependency (header above). + * Any arrow-fallback option disqualifies the whole menu — its DOWN math + * depends on selectedIndex freshness, which goes stale the moment the card + * is expired (nothing is left maintaining it), and a mixed row would also + * mix write shapes on one render. AskUserQuestion NEVER rebinds: CC's TUI + * for it is sequential (Q1 then Q2), multi-select, with a Skip and + * free-text box our card doesn't model — replaying that blind has a + * wrong-answer failure mode strictly worse than no feature. Its outs stay + * Dismiss + terminal view. Labels always come from THIS parse (menu.options), + * never from the card's original buttons — matching by index or guess is + * the exact misfire class ("click No, land on don't-ask-again") this gate + * exists to rule out. */ +export function rebindButtons(menu: ParsedMenu | null, toolName: string): PromptButton[] | null { + if (!menu) return null; + if (toolName === 'AskUserQuestion') return null; + const buttons = menuToButtons(menu); + if (buttons.some((b) => b.submitInput !== undefined)) return null; + return buttons; +} diff --git a/desktop/src/renderer/state/__tests__/chat-reducer.test.ts b/desktop/src/renderer/state/__tests__/chat-reducer.test.ts index f18e66a29..ea78d1507 100644 --- a/desktop/src/renderer/state/__tests__/chat-reducer.test.ts +++ b/desktop/src/renderer/state/__tests__/chat-reducer.test.ts @@ -3,6 +3,7 @@ import { chatReducer } from '../chat-reducer'; import { createSessionChatState, serializeChatState } from '../chat-types'; import type { ChatState } from '../chat-types'; import type { ToolCallState } from '../../../shared/types'; +import { hasPendingInteraction, canRetrySubmit, pendingInteractionKind } from '../pty-input-gate'; function stateWithInFlightTurn(sessionId = 'sess-1', turnId = 'turn-1'): ChatState { const session = createSessionChatState(); @@ -652,3 +653,290 @@ describe('chatReducer COMPACTION_COMPLETE — native auto-compaction (I2b)', () expect(next).toBe(state); // untouched — no spurious marker }); }); + +describe('PERMISSION_EXPIRED reasons (2026-07-30 spec §2/§2a/§2c/§2d)', () => { + // Fresh two-session-map state, mirroring initState() in the tool-card- + // duplication block above. A named helper (rather than inlining) makes the + // "fresh state, no pending ask" case in the last test read clearly. + function emptySession(sessionId = 's1'): ChatState { + return new Map([[sessionId, createSessionChatState()]]); + } + + // Arrange: PERMISSION_REQUEST with no matching running tool takes the + // synthetic-card path in chat-reducer.ts (no TRANSCRIPT_TOOL_USE has landed + // yet), minting toolCalls.get('perm-r1') with status 'awaiting-approval' + // and requestId 'r1' — the exact shape a live permission ask has in prod + // right before it expires. + function withPendingAsk(sessionId = 's1'): ChatState { + return chatReducer(emptySession(sessionId), { + type: 'PERMISSION_REQUEST', + sessionId, + toolName: 'Bash', + input: {}, + requestId: 'r1', + } as any); + } + + function expire( + state: ChatState, + opts: { reason?: 'app-timeout' | 'unroutable' | 'delivery-failed' | 'hook-closed' }, + sessionId = 's1', + ): ChatState { + return chatReducer(state, { + type: 'PERMISSION_EXPIRED', + sessionId, + requestId: 'r1', + ...opts, + } as any); + } + + it("'hook-closed' retains: awaiting-approval + expired, requestId cleared, no error", () => { + const state = expire(withPendingAsk(), { reason: 'hook-closed' }); + const tool = state.get('s1')!.toolCalls.get('perm-r1')!; + expect(tool.status).toBe('awaiting-approval'); // red dot + input gates keep holding + expect(tool.expired).toBe(true); + expect(tool.requestId).toBeUndefined(); + expect(tool.error).toBeUndefined(); + }); + + it("'hook-closed' still counts as pending for both pty gates", () => { + const state = expire(withPendingAsk(), { reason: 'hook-closed' }); + const session = state.get('s1')!; + expect(hasPendingInteraction(session)).toBe(true); + expect(canRetrySubmit(session)).toBe(false); + }); + + it("'app-timeout' resolves as failed with accurate copy, never retains", () => { + const state = expire(withPendingAsk(), { reason: 'app-timeout' }); + const tool = state.get('s1')!.toolCalls.get('perm-r1')!; + expect(tool.status).toBe('failed'); + expect(tool.expired).toBeUndefined(); + expect(tool.error).toContain('auto-denied'); + }); + + it('absent reason resolves — the native-broker / old-shim default', () => { + const state = expire(withPendingAsk(), {}); + expect(state.get('s1')!.toolCalls.get('perm-r1')!.status).toBe('failed'); + }); + + it("'delivery-failed' resolves", () => { + const state = expire(withPendingAsk(), { reason: 'delivery-failed' }); + expect(state.get('s1')!.toolCalls.get('perm-r1')!.status).toBe('failed'); + }); + + it('PERMISSION_CARD_RESOLVED quietly completes an expired card only', () => { + let state = expire(withPendingAsk(), { reason: 'hook-closed' }); + state = chatReducer(state, { type: 'PERMISSION_CARD_RESOLVED', sessionId: 's1', toolUseId: 'perm-r1' }); + const tool = state.get('s1')!.toolCalls.get('perm-r1')!; + expect(tool.status).toBe('complete'); + expect(tool.error).toBeUndefined(); + expect(tool.expired).toBeUndefined(); + // a NON-expired awaiting card must be untouched (only the §2 resolver and + // Dismiss use this action, and both only ever see expired cards) + const fresh = withPendingAsk(); + const untouched = chatReducer(fresh, { type: 'PERMISSION_CARD_RESOLVED', sessionId: 's1', toolUseId: 'perm-r1' }); + expect(untouched.get('s1')!.toolCalls.get('perm-r1')!.status).toBe('awaiting-approval'); + }); + + it('endTurn force-failing a retained hook-closed card clears `expired`, and a later PERMISSION_CARD_RESOLVED cannot erase the failure', () => { + // Retain via 'hook-closed': awaiting-approval + expired: true. + let state = expire(withPendingAsk(), { reason: 'hook-closed' }); + + // The session then actually dies (endTurn is spread by + // SESSION_PROCESS_EXITED). This force-fails the still-awaiting tool. + state = chatReducer(state, { + type: 'SESSION_PROCESS_EXITED', + sessionId: 's1', + exitCode: 1, + } as any); + + const failed = state.get('s1')!.toolCalls.get('perm-r1')!; + expect(failed.status).toBe('failed'); + expect(failed.error).toBe('Turn ended'); + // Regression guard: endTurn() must clear the stale `expired` marker once + // it force-fails the card, or a later quiet PERMISSION_CARD_RESOLVED can + // still pass the (buggy) `!tool.expired`-only guard and erase this + // failure with no error text. + expect(failed.expired).toBeUndefined(); + + // A stray PERMISSION_CARD_RESOLVED (Dismiss button, stale-detector + // callback) must NOT resurrect the failed card as a quiet 'complete'. + state = chatReducer(state, { + type: 'PERMISSION_CARD_RESOLVED', + sessionId: 's1', + toolUseId: 'perm-r1', + }); + const stillFailed = state.get('s1')!.toolCalls.get('perm-r1')!; + expect(stillFailed.status).toBe('failed'); + expect(stillFailed.error).toBe('Turn ended'); + }); + + it('TRANSCRIPT_TOOL_USE merging a retained hook-closed synthetic card preserves `expired`', () => { + // Retain via 'hook-closed': synthetic perm-r1 stays awaiting-approval, + // expired: true, requestId cleared (the exact shape the merge in + // TRANSCRIPT_TOOL_USE hand-picks fields off of). + let state = expire(withPendingAsk(), { reason: 'hook-closed' }); + + // The transcript watcher's real tool_use event arrives after the hook + // already expired — this is the merge branch that promotes perm-r1 to + // the real toolUseId. + state = chatReducer(state, { + type: 'TRANSCRIPT_TOOL_USE', + sessionId: 's1', + uuid: 'line-uuid-1', + toolUseId: 'toolu_real_1', + toolName: 'Bash', + toolInput: {}, + } as any); + + const synthetic = [...state.get('s1')!.toolCalls.keys()].filter((k) => k.startsWith('perm-')); + expect(synthetic).toHaveLength(0); // synthetic replaced, not duplicated + + const merged = state.get('s1')!.toolCalls.get('toolu_real_1')!; + // Regression guard: the merge builds a NEW object off a hand-picked field + // list. `expired` was missing from that list, so a merged retained card + // used to end up awaiting-approval with no requestId (unanswerable) and + // no `expired` flag (unresolvable — PERMISSION_CARD_RESOLVED's guard + // requires it) — an orphan that keeps the red dot and pty gates stuck. + expect(merged.status).toBe('awaiting-approval'); + expect(merged.expired).toBe(true); + expect(merged.requestId).toBeUndefined(); + + // And it must now actually be resolvable via PERMISSION_CARD_RESOLVED. + state = chatReducer(state, { + type: 'PERMISSION_CARD_RESOLVED', + sessionId: 's1', + toolUseId: 'toolu_real_1', + }); + const resolved = state.get('s1')!.toolCalls.get('toolu_real_1')!; + expect(resolved.status).toBe('complete'); + expect(resolved.expired).toBeUndefined(); + }); + + it('TRANSCRIPT_TOOL_RESULT settling a retained hook-closed card clears `expired` (success path: user answers the live terminal menu, tool runs, result lands on the same toolUseId)', () => { + // Retain via 'hook-closed', then merge the real toolUseId in via + // TRANSCRIPT_TOOL_USE (mirrors the prior regression test) so the card + // under test has the exact shape a merged retained card has in prod. + let state = expire(withPendingAsk(), { reason: 'hook-closed' }); + state = chatReducer(state, { + type: 'TRANSCRIPT_TOOL_USE', + sessionId: 's1', + uuid: 'line-uuid-1', + toolUseId: 'toolu_real_1', + toolName: 'Bash', + toolInput: {}, + } as any); + expect(state.get('s1')!.toolCalls.get('toolu_real_1')!.expired).toBe(true); + + // The user answered the still-live Ink menu directly in the terminal; + // Claude Code ran the tool and the transcript watcher's tool_result lands + // on the same toolUseId. This is the PRIMARY success path for a retained + // card — PERMISSION_CARD_RESOLVED never fires here. + const complete = chatReducer(state, { + type: 'TRANSCRIPT_TOOL_RESULT', + sessionId: 's1', + uuid: 'result-uuid-1', + toolUseId: 'toolu_real_1', + result: 'ok', + isError: false, + } as any); + const completeTool = complete.get('s1')!.toolCalls.get('toolu_real_1')!; + // Regression guard: settling this path used to spread `...existing` over + // status/response only, leaving a stale `expired: true` on an ordinary + // successful tool — UI that renders on `expired` would wrongly show it + // as expired forever. + expect(completeTool.status).toBe('complete'); + expect(completeTool.expired).toBeUndefined(); + + // Same bug, the failed branch. + const failed = chatReducer(state, { + type: 'TRANSCRIPT_TOOL_RESULT', + sessionId: 's1', + uuid: 'result-uuid-2', + toolUseId: 'toolu_real_1', + result: 'boom', + isError: true, + } as any); + const failedTool = failed.get('s1')!.toolCalls.get('toolu_real_1')!; + expect(failedTool.status).toBe('failed'); + expect(failedTool.expired).toBeUndefined(); + }); + + it('TRANSCRIPT_TOOL_USE re-emit for an already-progressed real toolUseId is idempotent — does not erase retention or flip status back to running', () => { + // Retain via 'hook-closed', then merge the real toolUseId in — the card is + // now { status: 'awaiting-approval', expired: true, requestId: undefined }. + let state = expire(withPendingAsk(), { reason: 'hook-closed' }); + state = chatReducer(state, { + type: 'TRANSCRIPT_TOOL_USE', + sessionId: 's1', + uuid: 'line-uuid-1', + toolUseId: 'toolu_real_1', + toolName: 'Bash', + toolInput: {}, + } as any); + const merged = state.get('s1')!.toolCalls.get('toolu_real_1')!; + expect(merged.status).toBe('awaiting-approval'); + expect(merged.expired).toBe(true); + + // CC rewrites the same JSONL line again (assistant message still growing) + // and the watcher re-emits tool-use for the SAME real toolUseId — by + // design, per transcript-watcher.ts's dedup-by-uuid comment. No perm-* + // synthetic entry exists anymore (already merged above), so this used to + // fall through to the unconditional toolCalls.set(...) that stamps a + // bare { status: 'running' } object, erasing retention entirely and + // flipping the pty gates open while the terminal menu may still be live. + const reEmitted = chatReducer(state, { + type: 'TRANSCRIPT_TOOL_USE', + sessionId: 's1', + uuid: 'line-uuid-1', + toolUseId: 'toolu_real_1', + toolName: 'Bash', + toolInput: {}, + } as any); + const card = reEmitted.get('s1')!.toolCalls.get('toolu_real_1')!; + expect(card.status).toBe('awaiting-approval'); + expect(card.expired).toBe(true); + expect(card.requestId).toBeUndefined(); + expect(hasPendingInteraction(reEmitted.get('s1')!)).toBe(true); + expect(canRetrySubmit(reEmitted.get('s1')!)).toBe(false); + }); + + it('PERMISSION_CARD_RESOLVED guard independently requires status === awaiting-approval, not just `expired`', () => { + // Discrimination test: a reviewer confirmed that reverting ONLY the + // guard's `status !== 'awaiting-approval'` clause still left the older + // regression test (above) passing, because endTurn()'s `expired`- + // stripping already blocks that particular path. This test builds a + // settled card that STILL carries `expired: true` (bypassing endTurn + // entirely) so it pins the guard clause on its own. + const state = withPendingAsk(); + const toolCalls = new Map(state.get('s1')!.toolCalls); + toolCalls.set('perm-r1', { + ...toolCalls.get('perm-r1')!, + status: 'failed', + error: 'Some real failure', + expired: true, // settled, but the stale marker was never cleared + }); + const settled = new Map(state); + settled.set('s1', { ...state.get('s1')!, toolCalls }); + + const next = chatReducer(settled, { + type: 'PERMISSION_CARD_RESOLVED', + sessionId: 's1', + toolUseId: 'perm-r1', + }); + const tool = next.get('s1')!.toolCalls.get('perm-r1')!; + expect(tool.status).toBe('failed'); + expect(tool.error).toBe('Some real failure'); + }); + + // Task 10 (2026-07-30 spec §4): the send-refusal copy needs to distinguish + // an awaiting-approval card (has a Dismiss out, names itself accurately + // even once expired) from a scraped terminal prompt (no card, no Dismiss). + it('pendingInteractionKind distinguishes approval cards from scraped prompts', () => { + const withCard = withPendingAsk(); // Task 3 helper — awaiting-approval tool + expect(pendingInteractionKind(withCard.get('s1')!)).toBe('approval'); + const expired = expire(withCard, { reason: 'hook-closed' }); + expect(pendingInteractionKind(expired.get('s1')!)).toBe('approval'); // expired still blocks + expect(pendingInteractionKind(emptySession().get('s1')!)).toBeNull(); + }); +}); diff --git a/desktop/src/renderer/state/__tests__/expired-card-resolver.test.ts b/desktop/src/renderer/state/__tests__/expired-card-resolver.test.ts new file mode 100644 index 000000000..d33b0adde --- /dev/null +++ b/desktop/src/renderer/state/__tests__/expired-card-resolver.test.ts @@ -0,0 +1,43 @@ +import { describe, it, expect } from 'vitest'; +import { nextAbsentCount, MENU_ABSENT_FLUSHES_TO_RESOLVE, expiredToolIds } from '../expired-card-resolver'; +import { createSessionChatState } from '../chat-types'; +import type { ToolCallState } from '../../../shared/types'; + +describe('expired-card menu-absence rule (spec §2)', () => { + it('menu present resets the counter — false retain self-heals later', () => { + expect(nextAbsentCount(true, 1)).toEqual({ count: 0, resolve: false }); + }); + it('one absent flush is NOT enough — socket close races the buffer flush', () => { + expect(nextAbsentCount(false, 0)).toEqual({ count: 1, resolve: false }); + }); + it('two consecutive absent flushes resolve', () => { + expect(nextAbsentCount(false, 1)).toEqual({ count: 2, resolve: true }); + expect(MENU_ABSENT_FLUSHES_TO_RESOLVE).toBe(2); + }); +}); + +describe('expiredToolIds', () => { + function makeTool(toolUseId: string, overrides: Partial): ToolCallState { + return { + toolUseId, + toolName: 'Bash', + status: 'awaiting-approval', + input: {}, + ...overrides, + } as ToolCallState; + } + + it('returns ids of tools that are both awaiting-approval AND expired', () => { + const session = createSessionChatState(); + session.toolCalls.set('t1', makeTool('t1', { expired: true })); + session.toolCalls.set('t2', makeTool('t2', {})); // live ask, not expired + session.toolCalls.set('t3', makeTool('t3', { status: 'complete', expired: true })); // resolved already + expect(expiredToolIds(session)).toEqual(['t1']); + }); + + it('returns an empty array when no cards are expired', () => { + const session = createSessionChatState(); + session.toolCalls.set('t1', makeTool('t1', {})); + expect(expiredToolIds(session)).toEqual([]); + }); +}); diff --git a/desktop/src/renderer/state/chat-reducer.ts b/desktop/src/renderer/state/chat-reducer.ts index 3d09d3897..83ede2ac1 100644 --- a/desktop/src/renderer/state/chat-reducer.ts +++ b/desktop/src/renderer/state/chat-reducer.ts @@ -176,7 +176,15 @@ function endTurn( for (const id of session.activeTurnToolIds) { const tool = toolCalls.get(id); if (tool && (tool.status === 'running' || tool.status === 'awaiting-approval')) { - toolCalls.set(id, { ...tool, status: 'failed', error: errorMessage }); + // Fix: also clear `expired` when force-failing. A retained hook-closed + // card (PERMISSION_EXPIRED reason 'hook-closed') carries `expired: true` + // while still 'awaiting-approval'. If the session then dies here, the + // card is now genuinely settled ('failed') — leaving `expired` set is + // stale state that lets a later quiet PERMISSION_CARD_RESOLVED (Dismiss + // button, stale-detector callback) pass its `!tool.expired` guard and + // silently overwrite this real failure with 'complete' and no error. + const { expired: _expired, ...rest } = tool; + toolCalls.set(id, { ...rest, status: 'failed', error: errorMessage }); } } return { @@ -824,7 +832,16 @@ export function chatReducer(state: ChatState, action: ChatAction): ChatState { for (const [synId, synTool] of toolCalls) { if (synId.startsWith('perm-') && synTool.toolName === action.toolName && synTool.status === 'awaiting-approval') { - // Replace synthetic with real tool, preserving permission state + // Replace synthetic with real tool, preserving permission state. + // Fix: this field list is HAND-PICKED, not a spread — any ToolCallState + // field a synthetic card can carry that isn't listed here is silently + // dropped on merge. `expired` was missing: a hook-closed retained card + // (status stays 'awaiting-approval', expired: true, requestId cleared) + // that merges with its real TRANSCRIPT_TOOL_USE used to lose `expired`, + // leaving a card with no requestId (unanswerable) and no expired flag + // (unresolvable — PERMISSION_CARD_RESOLVED's guard requires it) — an + // orphan stuck awaiting-approval forever. If you add a new field to + // ToolCallState that a synthetic card can carry, add it here too. toolCalls.delete(synId); toolCalls.set(action.toolUseId, { toolUseId: action.toolUseId, @@ -834,6 +851,7 @@ export function chatReducer(state: ChatState, action: ChatAction): ChatState { requestId: synTool.requestId, permissionSuggestions: synTool.permissionSuggestions, denyListed: synTool.denyListed, + expired: synTool.expired, }); // Update the tool group to reference the real ID const toolGroups = new Map(session.toolGroups); @@ -881,12 +899,28 @@ export function chatReducer(state: ChatState, action: ChatAction): ChatState { } if (mergedSynthetic) return next; - toolCalls.set(action.toolUseId, { - toolUseId: action.toolUseId, - toolName: action.toolName, - input: action.toolInput, - status: 'running', - }); + // Fix: this write must be IDEMPOTENT for a toolUseId that already has a + // progressed entry. transcript-watcher.ts's readNewLines re-emits + // tool-use on a repeated line uuid by design (CC rewrites the same + // JSONL line as the assistant message grows, and the earlier tool_use + // block is still present in the rewrite) — see its dedup-by-uuid + // comment. If the hook relay already carried this same toolUseId past + // 'running' (awaiting-approval, or retained via PERMISSION_EXPIRED + // 'hook-closed': awaiting-approval + expired: true), a later re-emit + // used to unconditionally stamp a bare { status: 'running' } object + // here, erasing that progress — and since the pty-input gates key off + // `status`, a retained card's gates would flip back open while the + // terminal's Ink menu may still be live. Only a genuinely new + // toolUseId gets a fresh 'running' card. + const existingCall = toolCalls.get(action.toolUseId); + toolCalls.set(action.toolUseId, existingCall + ? { ...existingCall, toolName: action.toolName, input: action.toolInput } + : { + toolUseId: action.toolUseId, + toolName: action.toolName, + input: action.toolInput, + status: 'running', + }); let { assistantTurns, timeline, currentTurnId } = getOrCreateTurn(session); const toolGroups = new Map(session.toolGroups); @@ -966,14 +1000,23 @@ export function chatReducer(state: ChatState, action: ChatAction): ChatState { // Carry structuredPatch onto the tool state so DiffView can render // with absolute file line numbers (Claude Code ships it pre-computed). const patch = action.structuredPatch; + // Fix: a tool_result settling a card must clear `expired`, or a card + // retained through PERMISSION_EXPIRED('hook-closed') stays flagged + // expired forever even though it just settled normally. This IS the + // retention feature's primary success path (not an edge case): the + // user answers the still-live terminal menu, Claude Code runs the + // tool, and this tool_result lands on the same toolUseId — + // PERMISSION_CARD_RESOLVED never fires for it. A card must always end + // up either retained (awaiting-approval + expired) or settled + // (failed/complete, expired cleared) — never both. if (action.isError) { toolCalls.set(action.toolUseId, { - ...existing, status: 'failed', error: action.result, + ...existing, status: 'failed', error: action.result, expired: undefined, ...(patch ? { structuredPatch: patch } : {}), }); } else { toolCalls.set(action.toolUseId, { - ...existing, status: 'complete', response: action.result, + ...existing, status: 'complete', response: action.result, expired: undefined, ...(patch ? { structuredPatch: patch } : {}), }); } @@ -1238,12 +1281,37 @@ export function chatReducer(state: ChatState, action: ChatAction): ChatState { const toolCalls = new Map(session.toolCalls); for (const [id, tool] of toolCalls) { if (tool.status === 'awaiting-approval' && tool.requestId === action.requestId) { - toolCalls.set(id, { - ...tool, - status: 'failed', - requestId: undefined, - error: 'Permission request expired — socket closed before a response was sent', - }); + if (action.reason === 'hook-closed') { + // Far end died (relay timeout/death, CC killed the hook) but the + // Ink menu may STILL be on screen. Retain provisionally: keep + // awaiting-approval so useSessionAttention stays red and the + // pty-input gates keep blocking sends into the live menu. + // usePromptDetector's menu-absence rule or the Dismiss button + // resolves it (spec §2/§2a). The reducer never reads the buffer. + toolCalls.set(id, { ...tool, requestId: undefined, expired: true }); + } else if (action.reason === 'app-timeout' || action.reason === 'unroutable') { + // The app itself delivered a deny — accurate copy, no retention. + toolCalls.set(id, { + ...tool, + status: 'failed', + requestId: undefined, + error: action.reason === 'unroutable' + ? 'No open session could show this request — YouCoded auto-denied it' + : 'No response in time — YouCoded auto-denied this request so Claude could continue', + }); + } else { + // 'delivery-failed' or absent (native PermissionBroker cancel, + // renderer delivery-failure recovery, older remote shim). DEFAULT + // IS RESOLVE, never retain: native sessions have no PTY (nothing + // to rebind), and delivery-failure means the socket is provably + // gone. Do not flip this default (spec §2c/§2d). + toolCalls.set(id, { + ...tool, + status: 'failed', + requestId: undefined, + error: 'Permission request expired — socket closed before a response was sent', + }); + } break; } } @@ -1252,6 +1320,30 @@ export function chatReducer(state: ChatState, action: ChatAction): ChatState { return next; } + case 'PERMISSION_CARD_RESOLVED': { + const session = next.get(action.sessionId); + if (!session) return state; + const tool = session.toolCalls.get(action.toolUseId); + // Only a still-awaiting expired card resolves this way — a live ask + // must go through its buttons (which deliver a real decision through + // the socket). Fix: also require status === 'awaiting-approval', not + // just `expired`. endTurn() force-fails a retained hook-closed card + // ('failed' + real error) if the session dies before it's resolved, + // but historically left `expired` set — without this status check a + // late PERMISSION_CARD_RESOLVED (Dismiss button, stale-detector + // callback) would pass on that stale marker and silently flip a real + // failure to 'complete' with no error, erasing it. + if (!tool || !tool.expired || tool.status !== 'awaiting-approval') return state; + const toolCalls = new Map(session.toolCalls); + // 'complete' with no error: nothing failed — the ask was answered in + // the terminal or dismissed. If the tool really runs, the transcript's + // TOOL_RESULT overwrites this with the true outcome. + const { expired: _resolved, ...rest } = tool; + toolCalls.set(action.toolUseId, { ...rest, status: 'complete' }); + next.set(action.sessionId, { ...session, toolCalls }); + return next; + } + case 'HISTORY_LOADED': { const session = next.get(action.sessionId); if (!session) return state; diff --git a/desktop/src/renderer/state/chat-types.ts b/desktop/src/renderer/state/chat-types.ts index db4424b0d..bcec28407 100644 --- a/desktop/src/renderer/state/chat-types.ts +++ b/desktop/src/renderer/state/chat-types.ts @@ -460,6 +460,20 @@ export type ChatAction = type: 'PERMISSION_EXPIRED'; sessionId: string; requestId: string; + /** Why the ask ended. ONLY 'hook-closed' (far end died, menu may still + * be live) retains the card. Absent = resolve: retention is the + * riskier behavior, and the native PermissionBroker + older remote + * shims never send a reason — defaulting to retain would wedge them + * (spec §2c/§2d). Optional so older serialized actions deserialize. */ + reason?: 'app-timeout' | 'unroutable' | 'delivery-failed' | 'hook-closed'; + } + | { + /** Quiet local resolve of an EXPIRED card: the user answered in the + * terminal (menu left the buffer) or clicked Dismiss. No error text — + * nothing failed. */ + type: 'PERMISSION_CARD_RESOLVED'; + sessionId: string; + toolUseId: string; } | { type: 'PERMISSION_RESPONDED'; diff --git a/desktop/src/renderer/state/expired-card-resolver.ts b/desktop/src/renderer/state/expired-card-resolver.ts new file mode 100644 index 000000000..39e3c72cd --- /dev/null +++ b/desktop/src/renderer/state/expired-card-resolver.ts @@ -0,0 +1,38 @@ +import type { SessionChatState } from './chat-types'; + +/** §2 standing rule (2026-07-30 spec): a card retained by a 'hook-closed' + * expiry resolves only after the Ink menu has been ABSENT from the visible + * buffer for TWO consecutive flushes. One flush is a race in both + * directions: a terminal answer's socket-close often lands BEFORE the flush + * that removes the menu (would false-retain, self-heals here), and CC's own + * fallback menu renders a beat AFTER a hook kill (a one-shot parse would + * false-RESOLVE — clearing the red dot while the session is still blocked + * on a menu chat view never renders; that is the original reported bug). */ +export const MENU_ABSENT_FLUSHES_TO_RESOLVE = 2; + +/** Ids of tool calls that are a still-live retained card: awaiting-approval + * AND expired. Scans the session-lifetime `toolCalls` map (not + * `activeTurnToolIds`) because a retained card can outlive the turn it was + * asked in — the whole point of retention is that it survives past the + * point a normal ask would have resolved. */ +export function expiredToolIds(session: SessionChatState): string[] { + const ids: string[] = []; + for (const [id, tool] of session.toolCalls) { + if (tool.status === 'awaiting-approval' && tool.expired) ids.push(id); + } + return ids; +} + +/** Pure transition for the per-session consecutive-absence counter. Menu + * present resets to 0 (self-heals a false retain); menu absent increments, + * and resolves once it reaches MENU_ABSENT_FLUSHES_TO_RESOLVE consecutive + * absent flushes (guards against a one-shot false resolve — see the + * module-level comment above). */ +export function nextAbsentCount( + menuPresent: boolean, + prevCount: number, +): { count: number; resolve: boolean } { + if (menuPresent) return { count: 0, resolve: false }; + const count = prevCount + 1; + return { count, resolve: count >= MENU_ABSENT_FLUSHES_TO_RESOLVE }; +} diff --git a/desktop/src/renderer/state/hook-dispatcher.ts b/desktop/src/renderer/state/hook-dispatcher.ts index 83ed37708..40cdde178 100644 --- a/desktop/src/renderer/state/hook-dispatcher.ts +++ b/desktop/src/renderer/state/hook-dispatcher.ts @@ -35,7 +35,9 @@ export function hookEventToAction(event: HookEvent): ChatAction | null { case 'PermissionExpired': { const requestId = payload._requestId as string; if (!requestId) return null; - return { type: 'PERMISSION_EXPIRED', sessionId, requestId }; + const reason = payload._reason as + | 'app-timeout' | 'unroutable' | 'delivery-failed' | 'hook-closed' | undefined; + return { type: 'PERMISSION_EXPIRED', sessionId, requestId, reason }; } default: diff --git a/desktop/src/renderer/state/pty-input-gate.ts b/desktop/src/renderer/state/pty-input-gate.ts index ec061bd6e..f579e4af4 100644 --- a/desktop/src/renderer/state/pty-input-gate.ts +++ b/desktop/src/renderer/state/pty-input-gate.ts @@ -46,6 +46,57 @@ export function hasPendingInteraction(session: SessionChatState): boolean { return false; } +/** + * Which kind of interaction is blocking sends — drives the send-refusal + * copy (§4, 2026-07-30 permission-ask-timeout spec) so a 2h app-owned hold + * never reads as a generic mystery lock: an 'approval' card points the user + * at "the card in the chat" even once expired (still blocking, no longer + * "waiting" in the live sense); a scraped 'prompt' has no card, so it keeps + * the plainer "answer the prompt" phrasing. Same scan order and + * same source fields as hasPendingInteraction — kept as a parallel function + * (not derived from its boolean) so the two can never disagree about + * whether something is blocking. + */ +export function pendingInteractionKind(session: SessionChatState): 'approval' | 'prompt' | null { + for (const id of session.activeTurnToolIds) { + if (session.toolCalls.get(id)?.status === 'awaiting-approval') return 'approval'; + } + for (const entry of session.timeline) { + if (entry.kind === 'prompt' + && entry.prompt.promptId !== HISTORY_EXPAND_PROMPT_ID + && !entry.prompt.completed) { + return 'prompt'; + } + } + return null; +} + +/** + * Send-refusal copy for a given pendingInteractionKind — the single source + * both refusal sites (App.tsx's command/skill send guard and InputBar's + * typed-message guard, including its "Send anyway" force-path toast) pull + * from, so they cannot drift apart (§4, 2026-07-30 spec). An 'approval' card + * points at "the card in the chat" rather than naming a specific control — + * of the four card shapes this kind collapses (permission triad, ExitPlanMode, + * AskUserQuestion, expired/retained), only two render a Dismiss button, so + * "resolve" is the one phrasing that stays true of all four, including once + * the app-owned hold has expired the card (still blocking, no longer + * "waiting" in the live sense) — see docs/error-message-standards.md: + * specific+accurate over a generic guess. `null` falls back to the 'prompt' + * copy defensively; callers only invoke this when a kind was found. + */ +export function pendingInteractionRefusalCopy(kind: 'approval' | 'prompt' | null): string { + // 'approval' covers four card shapes and only two of them have a Dismiss + // button (AskUserQuestion, ExpiredApprovalActions) — the plain permission + // triad and ExitPlanMode do not. "Answer or dismiss" named a control that + // isn't always on screen (review finding on 5fb40acd); "resolve" is honest + // for all four (click Yes/No/Always Allow, pick a plan option, answer the + // question, or dismiss an already-expired card) without enumerating them. + return kind === 'approval' + ? 'Claude is waiting for your response — resolve the card in the chat before sending.' + : 'Claude is waiting for your response — answer the prompt first.'; +} + /** * True only when the session is observably idle enough that a recovery `\r` * (useSubmitConfirmation) cannot land on anything but CC's empty input bar: diff --git a/desktop/src/shared/types.ts b/desktop/src/shared/types.ts index 7c0af59ac..5d94ef504 100644 --- a/desktop/src/shared/types.ts +++ b/desktop/src/shared/types.ts @@ -288,6 +288,13 @@ export interface ToolCallState { /** Native broker only: winning rule came from the destructive deny-list → * the "Always allow" button shows a consequence-gated confirm. Task 13. */ denyListed?: boolean; + /** Permission ask whose hook socket died with the terminal menu possibly + * still live ('hook-closed' expiry). The card STAYS awaiting-approval so + * the red strip dot and pty-input gates keep holding (the 2026-07-30 bug + * was this card flipping 'failed' — session looked fine while CC stayed + * blocked). requestId is cleared: the socket is gone, respond() can never + * work. Resolved by usePromptDetector's menu-absence rule or Dismiss. */ + expired?: true; response?: string; error?: string; /** Set when the tool result carries a structuredPatch (Edit/MultiEdit). */ diff --git a/desktop/tests/hook-relay.test.ts b/desktop/tests/hook-relay.test.ts index e37e03ff5..2b75995f4 100644 --- a/desktop/tests/hook-relay.test.ts +++ b/desktop/tests/hook-relay.test.ts @@ -111,4 +111,77 @@ describe('HookRelay', () => { client.destroy(); }); }); + + describe('tier-1 hold (2026-07-30 spec §1)', () => { + it('auto-denies with nested decision shape + app-timeout reason when the hold fires', async () => { + const short = new HookRelay((relay as any).pipeName + '-hold', 60 /* holdMs */); + await short.start(); + const expired = new Promise<[string, string, string?]>((resolve) => { + short.once('permission-expired', (sid, rid, reason) => resolve([sid, rid, reason])); + }); + const net = await import('net'); + const client = net.createConnection((short as any).pipeName); + await new Promise((res, rej) => { client.on('connect', res); client.on('error', rej); }); + const received = new Promise((resolve) => { + let buf = ''; + client.on('data', (c) => { buf += c; if (buf.includes('\n')) resolve(buf); }); + }); + client.write(JSON.stringify({ hook_event_name: 'PermissionRequest', _desktop_session_id: 'sess-h' }) + '\n'); + + const [sid, , reason] = await expired; + expect(sid).toBe('sess-h'); + expect(reason).toBe('app-timeout'); + // Assert against the RELAY'S OWN parse path: relay-blocking.js reads + // appDecision.decision — a flat shape would make this undefined. + const decision = JSON.parse((await received).trim()); + expect(decision.decision.behavior).toBe('deny'); + expect(decision.decision.message).toContain('auto-denied'); + short.stop(); + client.destroy(); + }); + + it('caps the hold at 60s-tier when the session gate says unroutable', async () => { + const short = new HookRelay((relay as any).pipeName + '-unroutable', 60_000, 40 /* unroutableHoldMs */); + short.setSessionGate(() => false); + await short.start(); + const expired = new Promise((resolve) => { + short.once('permission-expired', (_s, _r, reason) => resolve(reason)); + }); + const net = await import('net'); + const client = net.createConnection((short as any).pipeName); + await new Promise((res, rej) => { client.on('connect', res); client.on('error', rej); }); + client.write(JSON.stringify({ hook_event_name: 'PermissionRequest', _desktop_session_id: 'ghost' }) + '\n'); + expect(await expired).toBe('unroutable'); + short.stop(); + client.destroy(); + }); + + it("far-end death emits 'hook-closed'; respond() cancels the hold and emits nothing", async () => { + const short = new HookRelay((relay as any).pipeName + '-closed', 100); + await short.start(); + const reasons: (string | undefined)[] = []; + short.on('permission-expired', (_s, _r, reason) => reasons.push(reason)); + + const net = await import('net'); + const c1 = net.createConnection((short as any).pipeName); + await new Promise((res, rej) => { c1.on('connect', res); c1.on('error', rej); }); + const evt = new Promise((resolve) => short.once('hook-event', resolve)); + c1.write(JSON.stringify({ hook_event_name: 'PermissionRequest', _desktop_session_id: 'sess-c' }) + '\n'); + const e1 = await evt; + c1.destroy(); // far end dies + await new Promise((r) => setTimeout(r, 30)); + expect(reasons).toEqual(['hook-closed']); + + const c2 = net.createConnection((short as any).pipeName); + await new Promise((res, rej) => { c2.on('connect', res); c2.on('error', rej); }); + const evt2 = new Promise((resolve) => short.once('hook-event', resolve)); + c2.write(JSON.stringify({ hook_event_name: 'PermissionRequest', _desktop_session_id: 'sess-d' }) + '\n'); + const e2 = await evt2; + short.respond(e2.payload._requestId, { decision: { behavior: 'deny' } }); + await new Promise((r) => setTimeout(r, 150)); // past holdMs — timer must be dead + expect(reasons).toEqual(['hook-closed']); // respond() itself emits nothing (caller's job) + short.stop(); + c2.destroy(); + }); + }); }); diff --git a/desktop/tests/ink-select-parser.test.ts b/desktop/tests/ink-select-parser.test.ts index f4ea83940..9d4229398 100644 --- a/desktop/tests/ink-select-parser.test.ts +++ b/desktop/tests/ink-select-parser.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; -import { parseInkSelect, menuToButtons } from '../src/renderer/parser/ink-select-parser'; +import { parseInkSelect, menuToButtons, rebindButtons } from '../src/renderer/parser/ink-select-parser'; +import type { ParsedMenu } from '../src/renderer/parser/ink-select-parser'; describe('ink-select-parser', () => { describe('parseInkSelect', () => { @@ -423,4 +424,38 @@ Pick a follow-up: expect(menu.options).toEqual(['as is', 'from summary']); }); }); + + describe('rebindButtons (spec §3 gates)', () => { + const digitMenu: ParsedMenu = { + id: 'm1', title: 'Bash command', options: ['Yes', "Yes, and don't ask again", 'No'], + optionNumbers: [1, 2, 3], selectedIndex: 0, + }; + + it('returns digit-writing buttons for a fully-numbered single-select menu', () => { + const btns = rebindButtons(digitMenu, 'Bash')!; + expect(btns.map((b) => b.input)).toEqual(['1', '2', '3']); + expect(btns.every((b) => b.submitInput === undefined)).toBe(true); + // Labels come from the fresh parse, not any guessed/matched original — + // this is what rules out the "click No, land on don't-ask-again" misfire. + expect(btns.map((b) => b.label)).toEqual(digitMenu.options); + }); + + it('rejects a menu with ANY arrow-fallback option — never mix write shapes', () => { + const partial: ParsedMenu = { ...digitMenu, optionNumbers: [1, null, 3] }; + expect(rebindButtons(partial, 'Bash')).toBeNull(); + }); + + it('rejects a menu with NO digits at all (pure arrow-fallback)', () => { + const noDigits: ParsedMenu = { ...digitMenu, optionNumbers: undefined }; + expect(rebindButtons(noDigits, 'Bash')).toBeNull(); + }); + + it('never rebinds AskUserQuestion — sequential multi-question TUI with Skip/free-text', () => { + expect(rebindButtons(digitMenu, 'AskUserQuestion')).toBeNull(); + }); + + it('null menu → null', () => { + expect(rebindButtons(null, 'Bash')).toBeNull(); + }); + }); }); diff --git a/desktop/tests/permission-timeout-margins.test.ts b/desktop/tests/permission-timeout-margins.test.ts new file mode 100644 index 000000000..2d37962dc --- /dev/null +++ b/desktop/tests/permission-timeout-margins.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect } from 'vitest'; +import fs from 'fs'; +import path from 'path'; + +// Pins the §1 tier margins by reading the LITERALS, not process.env-resolved +// values (the env override would make an env-based test pass vacuously — +// spec §Constraints). All six sites live in this one repo. +// +// Structure note (for tasks 7/8, which EXTEND this file): the file-reading +// helpers below (repoRoot, read, literal) are module-scope and generic — +// Task 7 adds an APP_HOLD_MS assertion reading desktop/src/main/hook-relay.ts, +// Task 8 adds a PERMISSION_HOLD_MS assertion reading EventBridge.kt. Both can +// reuse `literal()` as-is. +const repoRoot = path.resolve(__dirname, '..', '..'); +const read = (p: string) => fs.readFileSync(path.join(repoRoot, p), 'utf8'); + +function literal(file: string, re: RegExp): number { + const m = read(file).match(re); + if (!m) throw new Error(`pattern ${re} not found in ${file}`); + return parseInt(m[1].replace(/_/g, ''), 10); +} + +const RELAY_RE = /CLAUDE_RELAY_TIMEOUT \|\| '(\d+)'/; + +describe('permission timeout tier margins (2026-07-30 spec §1)', () => { + const desktopRelay = () => literal('desktop/hook-scripts/relay-blocking.js', RELAY_RE); + const androidRelay = () => literal('app/src/main/assets/hook-relay-blocking.js', RELAY_RE); + const desktopCcSeconds = () => + literal('desktop/scripts/install-hooks.js', /command: expectedBlockingCmd, timeout: (\d+)/); + const androidCcSeconds = () => literal( + 'app/src/main/kotlin/com/youcoded/app/runtime/Bootstrap.kt', + /PERMISSION_HOOK_TIMEOUT_SECONDS = ([\d_]+)/); + + it('relay backstop is 2h30m on both platforms', () => { + expect(desktopRelay()).toBe(9000000); + expect(androidRelay()).toBe(9000000); + }); + + it('CC hook entry is 3h on both platforms', () => { + expect(desktopCcSeconds()).toBe(10800); + expect(androidCcSeconds()).toBe(10800); + }); + + it('relay fires strictly BEFORE CC, with a real margin', () => { + // CC winning is the bad outcome: hook killed with no decision → + // AskUserQuestion waits forever on its default-"never" question timeout. + expect(desktopRelay()).toBeLessThanOrEqual(desktopCcSeconds() * 1000 - 15 * 60 * 1000); + expect(androidRelay()).toBeLessThanOrEqual(androidCcSeconds() * 1000 - 15 * 60 * 1000); + }); + + it('every value is under the 32-bit setTimeout ceiling', () => { + for (const v of [desktopRelay(), androidRelay(), desktopCcSeconds() * 1000, androidCcSeconds() * 1000]) { + expect(v).toBeLessThan(2147483647); // overflow fires IMMEDIATELY — the bug, disguised + } + }); + + const appHold = () => literal('desktop/src/main/hook-relay.ts', /APP_HOLD_MS = ([\d_]+)/); + it('app hold is 2h and strictly under the relay backstop', () => { + expect(appHold()).toBe(7200000); + expect(appHold()).toBeLessThanOrEqual(desktopRelay() - 15 * 60 * 1000); + }); + + const androidHold = () => literal( + 'app/src/main/kotlin/com/youcoded/app/parser/EventBridge.kt', + /PERMISSION_HOLD_MS = ([\d_]+)L/); + it('android hold is 2h and strictly under the relay backstop', () => { + expect(androidHold()).toBe(7200000); + expect(androidHold()).toBeLessThanOrEqual(androidRelay() - 15 * 60 * 1000); + }); + + // Unlike the other five tiers, this one has no "loses the race to CC" cap + // to check against — its whole purpose is to cap an ask whose sessionId + // matches no live session, which will never render a card anywhere. A + // large value here silently turns that into an invisible hang, so pin the + // literal (not an env-resolved value) and require it stay far below the + // routable app hold. + const unroutableHold = () => literal('desktop/src/main/hook-relay.ts', /UNROUTABLE_HOLD_MS = (\d+)/); + it('unroutable dead-man cap is 60s and far below the routable app hold', () => { + expect(unroutableHold()).toBe(60000); + expect(unroutableHold()).toBeLessThanOrEqual(appHold() / 10); + }); +}); diff --git a/desktop/tests/use-prompt-detector.test.tsx b/desktop/tests/use-prompt-detector.test.tsx index 2882a0169..f30a43b3f 100644 --- a/desktop/tests/use-prompt-detector.test.tsx +++ b/desktop/tests/use-prompt-detector.test.tsx @@ -16,12 +16,15 @@ const mocks = vi.hoisted(() => ({ callbacks: [] as Array<(sid: string) => void>, screen: { text: '' }, // Minimal ChatStore stand-in (tranche 1: the detector reads the store - // directly instead of subscribing to the whole chat map). These tests drive - // the detector purely through terminal buffer events, so chat state stays - // empty — no awaiting-approval tools — and nothing ever notifies. Identity - // is stable across renders, matching the real store's per-provider lifetime. + // directly instead of subscribing to the whole chat map). Most tests in + // this file drive the detector purely through terminal buffer events with + // an empty `sessions` map (no awaiting-approval tools), so nothing ever + // notifies. The §2 resolver tests below populate `sessions` per-test. + // Identity is stable across renders, matching the real store's + // per-provider lifetime. + sessions: new Map(), store: { - getState: () => new Map(), + getState: () => mocks.sessions, subscribeAll: () => () => {}, }, })); @@ -70,6 +73,7 @@ describe('usePromptDetector prompt lifecycle', () => { mocks.dispatch.mockClear(); mocks.callbacks.length = 0; mocks.screen.text = ''; + mocks.sessions.clear(); }); afterEach(() => { @@ -192,3 +196,114 @@ describe('usePromptDetector prompt lifecycle', () => { expect(dismiss![0].promptId).toBe(shownId); }); }); + +// Coverage for the §2 standing rule (2026-07-30 spec): usePromptDetector is +// the ONLY thing that auto-resolves a 'hook-closed'-retained card. Before +// this block, only the pure helpers in expired-card-resolver.ts had tests — +// deleting the resolver block inside usePromptDetector (the effect body that +// calls expiredToolIds/nextAbsentCount and dispatches PERMISSION_CARD_RESOLVED) +// left the whole suite green while every retained card stayed stuck until the +// user clicked Dismiss by hand. +describe('usePromptDetector — expired card resolver (2026-07-30 spec §2)', () => { + beforeEach(() => { + vi.useFakeTimers(); + mocks.dispatch.mockClear(); + mocks.callbacks.length = 0; + mocks.screen.text = ''; + mocks.sessions.clear(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + // A neutral, unrecognized Ink menu — parses as "a menu is present" without + // matching any SETUP_PROMPT_TITLES entry, so it can't also trigger a + // SHOW_PROMPT dispatch and muddy the PERMISSION_CARD_RESOLVED assertions. + const NEUTRAL_MENU = `Pick a flavor + + ❯ 1. Vanilla + 2. Chocolate`; + const NO_MENU = 'plain output, no menu here'; + + function makeSession(tools: Array>) { + const toolCalls = new Map(tools.map((t) => [t.toolUseId, { input: {}, ...t }])); + return { + toolCalls, + activeTurnToolIds: new Set(tools.map((t) => t.toolUseId)), + }; + } + + function setExpiredCard() { + mocks.sessions.set('s1', makeSession([ + { toolUseId: 'toolu_expired', toolName: 'Bash', status: 'awaiting-approval', expired: true }, + ])); + } + + it('menu present: does not resolve, and resets the absence counter', () => { + setExpiredCard(); + renderHook(() => usePromptDetector()); + + // Build up one absent flush first... + mocks.screen.text = NO_MENU; + fireBuffer('s1'); + // ...then the menu reappears, which must reset the counter to 0. + mocks.screen.text = NEUTRAL_MENU; + fireBuffer('s1'); + expect( + mocks.dispatch.mock.calls.find((c) => c[0].type === 'PERMISSION_CARD_RESOLVED'), + ).toBeUndefined(); + + // If the reset above didn't happen, this single absent flush would be + // the "2nd" and would wrongly resolve. + mocks.screen.text = NO_MENU; + fireBuffer('s1'); + expect( + mocks.dispatch.mock.calls.find((c) => c[0].type === 'PERMISSION_CARD_RESOLVED'), + ).toBeUndefined(); + }); + + it('menu absent for ONE flush: still does not resolve', () => { + setExpiredCard(); + renderHook(() => usePromptDetector()); + + mocks.screen.text = NO_MENU; + fireBuffer('s1'); + + expect( + mocks.dispatch.mock.calls.find((c) => c[0].type === 'PERMISSION_CARD_RESOLVED'), + ).toBeUndefined(); + }); + + it('menu absent for TWO consecutive flushes: dispatches PERMISSION_CARD_RESOLVED for the expired card', () => { + setExpiredCard(); + renderHook(() => usePromptDetector()); + + mocks.screen.text = NO_MENU; + fireBuffer('s1'); + fireBuffer('s1'); + + const resolved = mocks.dispatch.mock.calls.find((c) => c[0].type === 'PERMISSION_CARD_RESOLVED'); + expect(resolved).toBeTruthy(); + expect(resolved![0]).toMatchObject({ + type: 'PERMISSION_CARD_RESOLVED', + sessionId: 's1', + toolUseId: 'toolu_expired', + }); + }); + + it('a LIVE (non-expired) awaiting-approval card still bails prompt detection, unaffected by the resolver', () => { + mocks.sessions.set('s1', makeSession([ + { toolUseId: 'toolu_live', toolName: 'Bash', status: 'awaiting-approval' }, + ])); + renderHook(() => usePromptDetector()); + + // A recognized setup-prompt menu would normally SHOW_PROMPT after the + // debounce — the live awaiting-approval bail must suppress that too. + mocks.screen.text = RESUME_MENU; + fireBuffer('s1'); + act(() => { vi.advanceTimersByTime(400); }); + + expect(mocks.dispatch).not.toHaveBeenCalled(); + }); +}); diff --git a/desktop/tests/workbench-fixture-actions.test.ts b/desktop/tests/workbench-fixture-actions.test.ts index a884df487..bb8778deb 100644 --- a/desktop/tests/workbench-fixture-actions.test.ts +++ b/desktop/tests/workbench-fixture-actions.test.ts @@ -12,7 +12,7 @@ const FIXTURE_ROOT = join(__dirname, '../src/renderer/dev/workbench/fixtures'); * timeline with no error — which is exactly the failure this guards. */ const KNOWN_KINDS = new Set([ 'text', 'user_message', 'assistant_text', 'tool_use', 'tool_result', - 'permission_request', + 'permission_request', 'permission_expired', ]); function fixtureFiles(dir: string): Array<{ name: string; raw: string }> {