fix(permissions): permission asks no longer expire silently — 2h app-owned hold, honest card states - #278
Open
itsdestin wants to merge 20 commits into
Open
fix(permissions): permission asks no longer expire silently — 2h app-owned hold, honest card states#278itsdestin wants to merge 20 commits into
itsdestin wants to merge 20 commits into
Conversation
…installs Bootstrap only appended the PermissionRequest blocking-relay hook entry when none existed, so every pre-existing install kept timeout: 300 forever. Extracted the logic into Bootstrap.ensurePermissionRequestHook, a find-and-replace helper that overwrites command + timeout on a matching existing entry (mirrors desktop install-hooks.js semantics). Task 1 of the permission-ask timeout fix: prerequisite for Task 2's relay-asset + timeout-value bump, which would otherwise regress existing Android installs from a 120s auto-deny into a permanent wedge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s + stale-site sweep Sets the new timeout tier values across all six sites (desktop relay, desktop CC hook entry, Android relay asset, Android CC hook entry via new Bootstrap.PERMISSION_HOOK_TIMEOUT_SECONDS constant) and rewrites every comment/doc/test-harness assertion that still asserted the old design (300s/120s/fail-open-on-timeout). New pinning test desktop/tests/permission-timeout-margins.test.ts reads the literal values out of source (not process.env-resolved) so it can't pass vacuously, and asserts relay < CC with a real margin plus the 32-bit setTimeout ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… missing overflow check Two code-review findings on Task 2 (12e46b7): - blocking-relay-handoff.md's validated spike-test transcript still showed the OLD fail-open timeout result, contradicting the corrected design bullet above it and the relabeled test-blocking-relay.js Test 4. Now reads fail-closed, exit=2. - permission-timeout-margins.test.ts's 32-bit setTimeout ceiling check omitted androidCcSeconds() * 1000, leaving the Android CC-hook tier's overflow case unverified. No timeout values changed.
…handoff doc
test-blocking-relay.js asserted a protocol that never shipped: sending
{"allow": false} and expecting exit 2. relay-blocking.js has no such
path — it doesn't read .allow, and only ever exits 2 on relay timeout.
A delivered decision (allow or deny) always exits 0, with the decision
riding in stdout's hookSpecificOutput. Fixed the test to send a real
deny decision ({"decision":{"behavior":"deny"}}) and assert exit 0,
and relabeled it to say what it verifies.
blocking-relay-handoff.md's "Spike Test Results" block still quoted
the old fabricated 4/4 run (deny at exit=2), contradicting the
"there is no deny-specific exit code" line a previous fix pass had
already corrected 13 lines above it. Replaced with a real re-run of
the fixed harness (measured today, not carried forward).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…doff doc
Test 2 of test-blocking-relay.js still sent the legacy flat {allow:true}
response while its docstring claimed the nested {decision:{behavior:allow}}
shape - fixed the handler and relabeled the test to match. The handoff
doc's "Blocking decision" bullet documented flat-string decisions
({"decision":"allow"}), which is wrong: main.ts's hookRelay.respond() calls
and relay-blocking.js both use the nested shape, and a flat or bare-string
decision would ship decision:undefined to Claude Code. Re-ran the harness
for real numbers since Test 2's label changed; timings are noise-level
unchanged from the prior run.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Steps 2/3/5 sketch an allow:boolean API and PreToolUse blocking; what shipped is PermissionRequest with a nested decision object. Annotate rather than rewrite — it is a record of the spike, not current API docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… PERMISSION_CARD_RESOLVED An expired permission ask used to always flip to 'failed', which turned the session's attention dot green even when Claude Code's terminal menu was still live and blocked waiting for input. Add ToolCallState.expired and a `reason` on PERMISSION_EXPIRED: only 'hook-closed' (far end died, menu may still be on screen) retains the card as awaiting-approval + expired so the red dot and pty-input gates keep holding; every other reason (or an absent one, covering the native broker and older remote shims) resolves as before. New PERMISSION_CARD_RESOLVED action quietly completes an expired card once the menu is confirmed gone or the user dismisses it — it only ever touches cards already marked expired, so a live ask still has to go through its buttons. Task 3 of the permission-ask-timeout plan.
…ired marker
endTurn() force-fails any still-awaiting-approval tool but never cleared
`expired`, so a retained hook-closed card whose session then died became
{status: 'failed', expired: true}. A later quiet PERMISSION_CARD_RESOLVED
(Dismiss button or stale-detector callback) would pass the expired-only
guard and silently flip a real failure to 'complete' with no error text —
the exact "session looks fine but isn't" bug this branch exists to fix.
Fix both sides: endTurn() clears `expired` when it force-fails a tool, and
PERMISSION_CARD_RESOLVED's guard now also requires status ===
'awaiting-approval'. Adds a regression test that ends a retained hook-closed
card's turn via a real SESSION_PROCESS_EXITED dispatch and confirms a
follow-up PERMISSION_CARD_RESOLVED cannot resurrect it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TRANSCRIPT_TOOL_USE's synthetic-merge branch rebuilds the tool card from a hand-picked field list that omitted `expired`. A hook-closed retained card (awaiting-approval + expired: true + no requestId) merging with its real tool_use event lost `expired`, leaving an orphan that was neither answerable (no requestId) nor resolvable (PERMISSION_CARD_RESOLVED requires `expired`) — stuck holding the red attention dot and pty-input gates forever. Also adds a test pinning PERMISSION_CARD_RESOLVED's `status === 'awaiting-approval'` guard clause independently: the existing regression test didn't discriminate it, since endTurn()'s expired-stripping already blocked that path before the guard mattered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…it idempotent
Two more instances of the retained-vs-settled incoherence bug class:
- TRANSCRIPT_TOOL_RESULT settled a card (`failed`/`complete`) without
clearing `expired`, so the primary success path for a hook-closed
retained card (user answers the live terminal menu, tool runs, the
result lands on the same toolUseId) left an ordinary successful tool
permanently flagged expired.
- TRANSCRIPT_TOOL_USE's non-merge branch unconditionally stamped a
bare `{ status: 'running' }` object for any toolUseId with no
matching perm-* synthetic, with no check for an already-progressed
entry. transcript-watcher.ts re-emits tool-use on a repeated line
uuid by design (CC rewrites the same JSONL line as the assistant
message grows), so a real production race — hook progresses a
toolUseId past 'running', then the watcher re-emits tool-use for
that same id — erased retention and flipped the pty-input gates back
open while the terminal's Ink menu could still be live.
Both fixes keep every card in one of the two coherent end states:
retained (awaiting-approval + expired, no requestId) or settled
(failed/complete, expired cleared).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uffer
Task 4 of the permission-ask-timeout plan. Adds the §2 standing rule:
a card retained by PERMISSION_EXPIRED('hook-closed') only resolves once
the Ink menu has been absent from the visible terminal buffer for two
consecutive flushes (expired-card-resolver.ts). A one-shot check races
in both directions — CC's fallback menu can render a beat after the hook
dies (false resolve, the original bug), and a terminal answer's
socket-close often lands before the buffer flush removes the menu
(false retain) — requiring two consecutive absent flushes self-corrects
both.
Also fixes usePromptDetector's awaiting-approval bail, which previously
returned early on ANY awaiting-approval tool including retained (expired)
ones. Since a retained card never leaves awaiting-approval on its own,
that bail would have switched prompt detection off for the session
permanently — silencing this resolver, the later digit-rebind feature,
and every unrelated setup PromptCard (trust gate, usage limit, resume).
The bail and the debounced re-check both now exempt expired cards.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tagged delivery-failed ToolCard and CompactToolStrip gated their approval UI on `tool.requestId`, but retained expired cards (Task 3) deliberately clear it — so a retained card rendered header-only, with no explanation and no way out, on a session whose red attention dot stayed lit. Widen both gates to `tool.requestId || tool.expired` and add an expired branch: explanatory copy that says what's known (buttons stopped working) rather than assumed (the ask failed), plus a Dismiss button that quietly dispatches PERMISSION_CARD_RESOLVED. AskUserQuestion gets the same branch — Dismiss is its only recovery path, since replaying its multi-select TUI blind risks a wrong answer. Also tag the three renderer sites that dispatch PERMISSION_EXPIRED as a generic "unstick this card" action (ToolCard's onFailedCb, and CompactToolStrip's delivered===false and catch branches) with reason: 'delivery-failed'. Under the new retention rules an untagged dispatch would PIN these cards forever instead of resolving them — the socket is provably gone in all three cases, so retention would leave dead buttons on screen with no recovery.
Tasks 3-5 added the retained awaiting-approval card (expired:true, Dismiss
control) that PERMISSION_EXPIRED('hook-closed') produces, but nothing in the
workbench could reproduce it — Destin had no way to review it visually.
Adds a `permission_expired` JSONL line type to fixture-loader.ts, mirroring
the existing `permission_request` branch: dispatches PERMISSION_EXPIRED
through the real reducer, then swaps the block pushed by the prior
permission_request line (blocks are snapshots, so without this the gallery
would keep showing the stale pre-expiry Yes/No card). New fixture
bash-awaiting-approval-expired.jsonl exercises it in the Tool Gallery,
alongside the existing awaiting-approval and denylisted variants.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d expiry emits
The app now ends the permission-ask wait itself instead of relying on the far
end (relay/Claude Code) to close the socket. HookRelay starts a per-request
hold timer on every PermissionRequest: 2h (APP_HOLD_MS) for asks routable to
a live session, 60s (UNROUTABLE_HOLD_MS) dead-man cap for asks whose session
no longer exists. On fire it auto-denies with the nested
{ decision: { behavior: 'deny', message } } shape relay-blocking.js expects
and emits 'permission-expired' with an explicit reason ('app-timeout' /
'unroutable') BEFORE the socket's 'close' handler can run — respond()/
closeSocket() delete the pending entry synchronously, so 'close' never fires
for app-initiated endings. The 'close' handler now emits 'hook-closed' only
when it still finds the entry, i.e. only when the far end went away first
(relay timeout, Claude Code killing the hook). That asymmetry is exactly the
discrimination the renderer's retain-vs-resolve logic (Task 3) depends on.
- HookRelay: holdTimers map, setSessionGate(), clearHold() called from
respond()/closeSocket()/socket-close/stop() so a leaked 2h timer never
holds a socket reference or fires into a torn-down connection.
- SessionManager.hasSession() exposes the same `sessions` map the class
already uses for lookup, wired as the app's routability gate in main.ts.
- main.ts forwards `reason` inside the PermissionExpired payload as
`_reason` (no IPC channel shape change) to hook-dispatcher.ts, which
already threads it into the PERMISSION_EXPIRED action (Task 3 contract).
- Extended tests/hook-relay.test.ts with the tier-1 hold/cap/hook-closed
cases and tests/permission-timeout-margins.test.ts with the APP_HOLD_MS
literal check (2h, strictly under the 2h30m relay backstop).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 6 added the permission_expired line kind to the fixture loader but not to this guard's mirror list, so the guard correctly failed. Unregistered kinds are SILENTLY SKIPPED by the loader — which is exactly what this test protects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pired
Task 8 of the permission-ask-timeout plan — Android half of Task 7's desktop
hold timer. EventBridge now runs its own tier-1 hold (PERMISSION_HOLD_MS =
7_200_000L, mirrors desktop's APP_HOLD_MS) per PermissionRequest socket: if
2h pass with no user response, the app itself sends a labeled deny (nested
{decision:{behavior:"deny"}} shape — flat would ship decision:undefined
through hook-relay-blocking.js) and emits PermissionExpired(reason =
"app-timeout"). This must stay strictly under the relay asset's 2h30m
backstop and Claude Code's 3h hook-entry timeout, or CC wins and kills the
hook with no decision, wedging AskUserQuestion forever (its own timeout
defaults to never) — margins are pinned by the extended
permission-timeout-margins.test.ts.
PermissionExpired now carries an optional `reason` ("app-timeout",
"delivery-failed", "hook-closed", or null) threaded through HookEvent,
HookSerializer (as `_reason` inside the payload, same convention as
desktop's main.ts so the shared hook-dispatcher parses both transports),
and ManagedSession's broadcast — keyed on requestId/session id, never the
raw event's sessionId (EventBridge's write-failure path emits that as "").
Every path that can end a pending request (respond, closeSocket, the
closure monitor, stop) cancels the hold job so a live 2h coroutine never
outlives its socket. Fixed a double-emit the brief's sample code would have
had: respond() now returns Boolean so the hold-timeout path only emits
"app-timeout" when the deny actually went out, since respond() already
emits "delivery-failed" itself on a write failure.
No routability gate added — EventBridge is per-session (one per PtyBridge),
so an ask on a session's own socket is routable by construction; desktop's
unroutable cap has no Android analogue.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…enu-labeled) Task 9 of the permission-ask-timeout plan. Expired cards (hook socket dead, Ink menu possibly still live) can now drive that live menu directly: a new rebindButtons() gate in ink-select-parser.ts refuses the whole rebind unless every option in the fresh parse carries a bare digit (never mixes the digit and arrow+\r write shapes) and never rebinds AskUserQuestion (its TUI is sequential/multi-select with a Skip and free-text box this card doesn't model). ToolCard's Task 5 expired branch is replaced by ExpiredApprovalActions, which polls the terminal buffer every 2s, renders the MENU'S OWN option labels (never the card's original buttons, ruling out a "click No, land on don't-ask-again" misfire), and writes the digit as a deliberate menu-driving PTY passthrough that deliberately bypasses pty-input-gate.ts (same precedent as PlanApprovalButtons — but NOT its known-broken arrow+\r-in-one-write shape). The click never resolves the card itself; resolution stays owned by usePromptDetector's menu-absence rule, so a write that didn't land leaves the card up and the buttons re-arm. Also documents the invariant that makes the branch's "answer it in the terminal" copy safe: `expired` is only ever set on the hook/PTY permission path today, never by the native broker, so a future change that broke that exclusivity needs to be caught here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 10 of the permission-ask-timeout plan (§4). The gate now refuses
sends for up to 2h (Task 9's app-owned hold) instead of the prior 5-min
expiry, and an expired card keeps blocking rather than clearing — a
generic "Claude is waiting for your response" reads as a mystery lock
at that length, and pushes a stuck-feeling user toward InputBar's
"Send anyway" force path, which writes straight into the live Ink menu.
Adds pendingInteractionKind (pty-input-gate.ts), a pure predicate with
the same scan order as hasPendingInteraction so the two can never
disagree about whether something is blocking, plus
pendingInteractionRefusalCopy as the single copy source both refusal
sites read from. An 'approval' card now names itself and its Dismiss
out ("Claude asked a question — answer or dismiss the card in the
chat before sending") — copy that stays accurate even once the card
has expired. Scraped terminal prompts (trust/resume dialogs, no chat
card) keep the plainer "answer the prompt first" phrasing.
Updated all three sites that show this copy: App.tsx's
notifyIfPtyBlocked (command/skill/slash sends), the onSendBlocked
toast wired to ChatInputBar (the actual "Send anyway" force-path
toast typed messages hit), and InputBar's toast fallback for callers
that don't wire onSendBlocked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review findings on the send-refusal message shown while a permission card blocks PTY writes: - App.tsx's onSendBlocked hardcoded pendingInteractionRefusalCopy(null)'s exact return string in its no-session fallback branch instead of calling the helper, defeating the single-source guarantee the helper exists for. Route the fallback through the helper too. - The 'approval' copy said "answer or dismiss the card in the chat" but only 2 of the 4 card shapes that collapse into 'approval' (AskUserQuestion, ExpiredApprovalActions) render a Dismiss button — the plain permission triad and ExitPlanMode do not. Reworded to "resolve the card in the chat", which is honest for all four without enumerating them (and matches the "resolve the card" phrasing already used in the design spec).
- hook-relay.ts: only emit permission-expired from the hold-timer path when respond() actually delivered the deny — mirrors Android's if (respond(...)) guard so desktop stops claiming "auto-denied" for a write that never happened. - Both platforms now derive the hold-hours count (and pluralize naturally) from their own tier constant instead of a literal "hour(s)" string (desktop) / hardcoded "2 hours" (Android), so a future tier change can't silently desync the copy. - CompactToolStrip's expired-card button now says "Dismiss — I answered in the terminal" instead of a bare "Dismiss" — matches ToolCard, since dismissing opens the PTY input gates and needs the same user assertion to be safe. - Added resolver coverage to use-prompt-detector.test.tsx (menu present/absent-once/absent-twice/live-card-bail) — this was the only thing exercising usePromptDetector's auto-resolve block, which previously had zero tests despite being the sole path that clears a retained 'hook-closed' card. - Pinned UNROUTABLE_HOLD_MS (60s dead-man cap for unroutable asks) in permission-timeout-margins.test.ts, the one tier constant that wasn't guarded against creeping into invisible-hang territory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the bug where a chat-view permission ask died after ~5 minutes with no indication anywhere — the card flipped to
failed, the session's red "needs you" dot went green, and Claude Code stayed blocked waiting for an answer that could no longer be delivered. The session started looking fine at the exact moment it stopped being fine.AskUserQuestion was the acute case: Claude Code's own question timeout defaults to
never, so our hook was the only clock in the system. When it fired, the session wedged permanently.Spec:
docs/active/specs/2026-07-30-permission-ask-timeout-design.md(workspace repo)Plan:
docs/active/plans/2026-07-31-permission-ask-timeout.mdThe design
Three staggered clocks so the app is always the party that ends the wait — it is the only one that knows why it ended and can label the card accurately:
Claude Code winning is the bad outcome: it kills the hook with no decision at all, which is exactly what wedges AskUserQuestion. The 30-minute margins keep it unreachable, and a test pins them so a future "tidy" back to equal values fails CI.
The expiry now carries a
reason. Only'hook-closed'(far end died, terminal menu may still be live) retains the card asawaiting-approval+expired: true— which keeps the red dot lit and the PTY input gates shut.'app-timeout','unroutable','delivery-failed', and an absent reason all resolve. Absent-resolves is load-bearing: the native permission broker's cancel path sends no reason, and native sessions have no PTY, so a retained native card would be unrecoverable.A retained card resolves itself once Claude Code's Ink menu has been absent from the terminal buffer for two consecutive flushes — so answering in the terminal quietly clears the chat card instead of stamping it "expired". Two flushes, not one, because a single check races the terminal in both directions; the dangerous direction is Claude Code's fallback menu rendering a beat after a hook kill, which a one-shot check would read as "resolved" while the session is still blocked.
Where the menu is still live and every option carries a digit, the card's buttons rebind to type that digit into the terminal. Gated hard: refuse the whole rebind if any option lacks a digit, never for AskUserQuestion, and render the menu's own labels from a fresh parse — clicking "No" and landing on "Yes, and don't ask again" is the failure mode worth refusing over.
Also fixed along the way
TRANSCRIPT_TOOL_USE's non-merge branch unconditionally overwrote an already-progressed tool card with a bare{status:'running'}. The transcript watcher re-emits tool-use on a repeated line uuid and its own comment calls that "harmless" because "the reducer dedupes by toolUseId" — which that branch was violating. It could flip the PTY input gates open while a menu was live.Bootstraponly appended the hook entry when missing, sotimeout: 300persisted forever — while the relay asset redeploys on every launch. Shipping the relay change alone would have put relay-2h30m against CC-300s: permanent wedge, strictly worse than the old 120s auto-deny. Now find-and-replace, matching desktop.{"allow":false}→ exit 2; the relay reads.decisionand exits 0 for any delivered decision). Fixed at source; the handoff doc now records a real measured run instead of a stale one.Verification
scripts/verify.sh --full: PASS types, tests, knip, ast-grep:app:testDebugUnitTestBUILD SUCCESSFULdesktop/docs/test-blocking-relay.js: 4/4 against the real relayExecuted task-by-task with a review gate per task plus a final whole-branch review. Those reviews found and fixed four distinct paths where a card could end up neither answerable nor resolvable, and the final pass found two more (a hold that claimed "auto-denied" when the deny was never delivered; a buddy-strip Dismiss whose label didn't justify opening the PTY gates).
Needs a human eye before merge
Interactive checks not scripted, per the workspace convention:
pkill -f relay-blocking) → card should stay with terminal-menu-labeled digit buttons; clicking one should answer the menu and clear the cardFollow-ups filed (not blocking)
ROADMAP.mdin the workspace: buddy chat's InputBar never consults the PTY gate at all (pre-existing, widened by the longer hold);HookRelaycan drop an expiry in one narrow socket-destroyed ordering;closeSocket()is dead on both platforms; and separately,PlanApprovalButtonssends arrows+Enter in one write — the shape measured broken on CC 2.1.220.🤖 Generated with Claude Code