Skip to content

fix(agent-hooks): stop navigation Escape from inferring completion - #20149

Merged
nwparker merged 5 commits into
mainfrom
fix/escape-inference
Sep 13, 2026
Merged

nwparker merged 5 commits into
mainfrom
fix/escape-inference

Conversation

@nwparker

@nwparker nwparker commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​554 $\color{#cf222e}{\Huge{\mathbf{−}}}$​3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​551
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​45 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​45

In plain terms

In these agent CLIs, Escape does double duty: it closes a menu, a settings view or a pop-up question, and it also cancels a running turn. Orca could not tell those two apart. So when you hit Escape just to dismiss something while your agent was busy — say, halfway through a long shell command — Orca decided the agent had been stopped: the pane went grey, the work was marked finished-and-interrupted, and a "stopped" notification fired, all while the agent was actually still working. The reason it got this wrong is that it treated "no update arrived in the last half-second" as proof of a cancellation, when during a long-running tool that silence is completely normal.

The fix makes Orca look at what the agent last told it. If the agent's most recent message was "I'm starting a tool" (and it has not yet said "I finished it"), a single Escape is no longer allowed to end the pane — only the agent itself can, by reporting that it's done. Ctrl+C is untouched, because it only ever means cancel.

Fixes #13547
Fixes #9208

Credit

The shape of this fix is @brennanb2025's and @rodboev's, not ours. #15674 established the rule for Claude and #9289 established it for OMP and Pi, independently and before this PR. Between them they are the fix. Our contribution is joining the two halves, extending them to Prime Agent, the wire-level and rendered evidence, and the completeness work that showed the extra machinery we had built was not needed.

@rodboev's two-sided instinct was also right and we adopted it: the renderer skips the round-trip, and main re-checks the same rule, so a stale or direct inference request cannot route around the renderer. An earlier revision of this PR guarded only in main.

The invariant

For the agent types whose TUI also binds a bare Escape to navigation, a plain Escape may never be promoted to done/interrupted. Only the provider's own hook may retire the row.

Why baseline equality was not enough

The interrupt fallback exists for the case where a provider misses its final hook. It fired when, 500ms after an Escape, the pane's row still matched the baseline captured at keypress. But baseline equality proves exactly one thing: no new hook arrived in that window. On a fresh working row driven by a long tool call, "no new hook" is the expected state — the tool is running. Claude's /btw composer and OMP/Pi's focused-child and settings views all close on a bare Escape, so the keypress itself carries no interrupt signal either. The result was a live agent's row turning grey-and-interrupted mid-tool.

The fix stops trying to read intent out of the keystroke. Escape is ambiguous at the source for these TUIs, and which meaning it had is focus state only the TUI holds — nothing downstream can recover it. So for these agents the keypress is never evidence a turn ended, and only the provider's own hook retires the row.

  • ESCAPE_ALSO_NAVIGATES_AGENT_TYPES = claude, omp, pi, prime-agent
  • plain-escape only. Ctrl+C has no navigation meaning in these TUIs, so it still infers immediately.
  • No double-Escape gate is copied to OMP: a second Escape there is more navigation, not an interrupt.

Why no hook-event allowlist

An earlier revision of this PR gated on which hook event the row last sat on, refusing only while a tool call was open. That has been removed, for two reasons.

It did not survive contact. The set had to track five providers' event vocabularies, and it was wrong twice inside a single review cycle — tool_approval_resolved and ui_prompt_end both land working with work still outstanding and were missing. A rule that needs a probe sweep to specify correctly will drift as providers change.

It was also incoherent. It trusted the provider's closing hook during an open tool call while still inferring completion from a keystroke outside one. Either the provider's signal is authoritative or it is not; it cannot be both depending on where in the turn the user pressed a key.

The agent-type list stays, because any scoped fix needs one — and note that an open-tool counter at ingest would not have removed the allowlist, only relocated it, since the counter still has to know which events open and close.

Guard placement is load-bearing

The guard sits after the dismissesClaudeQuestion branch and after the strict baseline-match block, and it has to:

  • Claude's AskUserQuestion dismissal is a plain Escape on a waiting row. Run the guard earlier and it would swallow that Escape, and the question card would stop clearing. fix(agent-hooks): trust Claude Stop hooks over plain Escape (STA-3817) #15674 placed its check at the same anchor for the same reason. The renderer-side skip is scoped to state === 'working' for this same constraint.
  • A stale or mismatched baseline must keep failing first, so the guard never becomes the reason a genuinely retired row is refused.

Both constraints are pinned by tests.

The one remaining list

Whether a TUI binds a bare Escape to navigation is a per-TUI UX fact the repo cannot enumerate, so ESCAPE_ALSO_NAVIGATES_AGENT_TYPES is evidence-driven rather than exhaustive: Claude (#13547), OMP (#9208), and Pi / Prime Agent, which share OMP's runtime and overlay model. Agents whose first Escape is a cancel (opencode, copilot) already have the separate double-Escape gate. claude-agent-teams, openclaude and openclaw are covered for free — they report through the claude hook source, which hardcodes agentType: 'claude'.

A missing entry here degrades to today's behaviour for that agent, which is the same failure mode the repo already has. A missing entry in the removed event allowlist silently reintroduced the bug for an agent already declared in scope — a strictly worse failure, and the reason that list is gone.

Provider event-vocabulary audit that showed the event list was unnecessary

Done while the allowlist still existed. It is no longer load-bearing, but it is what established that the allowlist bought nothing the agent-type rule does not already cover.

  • Claude. The shipped CLI's complete hook-event vocabulary (extracted from the installed claude 2.1.269 binary): PreToolUse, PostToolUse, PostToolUseFailure, PermissionRequest, Notification, UserPromptSubmit, Stop, StopFailure, SubagentStart, SubagentStop, TeammateIdle, PreCompact, PostCompact, SessionStart, SessionEnd. Its own help text confirms the tool-context subset: "Only available for tool events: PreToolUse, PostToolUse, PermissionRequest." PreToolUse was Claude's only tool-opening event that can produce a working row.
  • Pi / OMP / Prime Agent. Orca generates and installs the extension that emits these hooks (src/main/pi/agent-status-handler-source.ts), so the emitted set is closed at the source: session_start, before_agent_start, agent_start, tool_call, tool_execution_start, tool_execution_end, tool_approval_requested, tool_approval_resolved, the Pi ui-prompt pair, message_end, agent_end.
  • Even with both vocabularies fully enumerated, the allowlist was still specified wrongly twice — tool_approval_resolved and ui_prompt_end land working with work outstanding without being tool openers. Enumerability was never the problem; the framing was.

Visual proof

Same running Orca instance, same live terminal pane, same real Escape keypress. The only thing that differs between the two runs is the interrupt-inference rule — the right-hand run has it, the left-hand run uses the pre-fix version of that file, everything else byte-identical.

These were captured against the earlier revision of this PR. They are kept because the scenario they show — a Claude row mid-Bash, one real Escape — and its rendered outcome are identical under the simplified rule (wire case A below, still false / working). Only the reason changed: the row is now held because Escape is ambiguous for Claude at all, not because a tool call happened to be open.

Before — parent commit After — this branch
BEFORE.png AFTER.png
Red dot, "migrate the schema — Interrupte…". The agent is still mid-Bash; Orca has retired the row and would fire a completion notification. Amber spinner, "migrate the schema — Bash: pn…". The row stays live until the agent itself reports otherwise.

The store transition behind each shot, read out of the renderer:

parent commit:  before=[{state:"working",tool:"Bash"}]  ->  after=[{state:"done",interrupted:true}]
this branch:    before=[{state:"working",tool:"Bash"}]  ->  after=[{state:"working",tool:"Bash"}]

How it was driven, end to end through real surfaces: a repo and worktree were added through the real IPC, a real terminal tab with a real PTY was opened, a real UserPromptSubmit + PreToolUse pair was POSTed to that app instance's own loopback hook listener (port and token read from its agent-hooks/endpoint.env), and the Escape was a real page.keyboard.press('Escape') into the focused xterm pane — which is what produces the \x1b byte that observeSentTerminalInputIntent keys on. The screenshots are CDP captures of a hidden renderer launched with ORCA_BACKGROUND_LAUNCH=1; no window was shown, raised, or focused.

Verification

Driven over the real loopback hook listener — AgentHookServer.start(), literal POST http://127.0.0.1:<port>/hook/<source> with the real token header — then through the real renderer Escape path (isPlainEscapeKeyEvent → createAgentInterruptInference → 500ms settle → the exact inferInterrupt call src/main/ipc/agent-hooks.ts:60 makes), then reading the real published status.

# Wire sequence inferInterrupt Published row
A UserPromptSubmit → PreToolUse{Bash}, Escape false working
B A + PostToolUse, Escape false working
C /hook/pi before_agent_start → tool_call{shell}, Escape false working
D UserPromptSubmit → PreToolUse, Ctrl+C true done, interrupted: true
E /hook/omp before_agent_start → tool_execution_start, Escape false working
F E + a forged ui_prompt_end{is_idle:false}, Escape false working
G /hook/omp tool_call → tool_approval_requested → tool_approval_resolved{approved}, Escape false working
H /hook/pi tool_call → ui_prompt_start → ui_prompt_end{is_idle:false}, Escape false working
I UserPromptSubmit only — no tool ever ran, Escape false working
J /hook/codex UserPromptSubmit, Escape — out of scope true done

B and I are what changed when the event allowlist came out, and they are the behaviour change worth reviewing: a Claude row whose tool call has closed (B), or which never ran a tool at all (I), no longer infers from Escape either. J confirms agents outside the list are untouched.

E is #9208's own reported repro. Dismissing an OMP focused-child or settings view emits no hook at all — the silence is the bug.

G and H are the two cases the old allowlist missed. They now hold for a structural reason rather than because two more strings were added to a set.

Two pre-existing suites also had to change: server-interrupt-inference-guards and server-relay-listener-replay each asserted that a Claude plain Escape infers once a background-work gate clears. Both now assert that with Ctrl+C, which exercises the same intent-agnostic gate. That those assertions existed is itself a measure of how much behaviour this removes.

ui_prompt_end can never reach an OMP row, for two independent reasons: Orca's generated extension registers those handlers only when kind === 'pi' (agent-status-ui-prompt-source.ts:5-7, plus a runtime isOmpRuntime() bail at lines 11 and 18), and normalizePiCompatibleEvent gates isPiUiPromptEnd on agentType === 'pi' (pi-family-events.ts:38). F forges it onto the wire anyway and confirms the row is untouched.

The A/B contrast pair is committed as a test against the same loopback listener. Unit coverage additionally pins: OMP single and double Escape both refused; an OMP agent_end still settling the row afterwards; a genuine Claude Stop{is_interrupt} staying authoritative; AskUserQuestion dismissal unaffected; OpenCode/Copilot double-Escape and Droid Ctrl+C unchanged.

Not verified: no real agent TUI was driven end to end. Confirming that Claude actually emits its closing hook when a user interrupts mid-tool needs a live provider session, and that is the assumption the residual risk below rests on.

Suites: 5,107 passed across src/main/agent-hooks/, the Pi-compatible listener, and the whole terminal-pane renderer tree, re-run after rebasing onto current main. The full wire table above was re-run post-rebase and is unchanged. Static gates clean: oxlint, the native-plugin and type-aware code-quality audits, changed-code quality, React Doctor, the max-lines / ts-nocheck / runtime-electron ratchets, reliability gates, the RPC params catalog, oxfmt --check, and all three typecheck projects.

Exercised in the real app

Driven end to end in a hidden Electron instance: real repo and worktree added over IPC, three real terminal tabs with real PTYs, hooks POSTed to that instance's own loopback listener, and every keypress a real page.keyboard.press into the focused xterm. Values below are read out of the renderer store.

Each scenario carries its own positive control in the same run, because "nothing happened" is indistinguishable from "the keypress never arrived". An early pass of this exercise was wrong for exactly that reason — with several tabs open, querySelector('.xterm-screen') returns the first match, which is a hidden pane, so the clicks landed nowhere and OMP and Codex both looked inert. Re-run against the visible pane, they behave as designed.

1. Claude mid-tool — #13547

before            {agentType:"claude", state:"working", tool:"Bash", completionUnread:false, notifications:0}
after Escape      {agentType:"claude", state:"working", tool:"Bash", completionUnread:false, notifications:0}
after Ctrl+C      {agentType:"claude", state:"done", interrupted:true}          <- positive control

2. OMP work in progress — #9208

before            {agentType:"omp", state:"working", tool:"shell", completionUnread:false, notifications:0}
after Escape      {agentType:"omp", state:"working", tool:"shell", completionUnread:false, notifications:0}
after Ctrl+C      {agentType:"omp", state:"done", interrupted:true}             <- positive control

3. A real provider Stop hook still retires the row

POST /hook/claude {"hook_event_name":"Stop"} -> 204
row               {state:"done"}     // cleanly done, not interrupted — correct, this was not an interrupt

4. Codex — outside the scoped agent types, untouched

before            {agentType:"codex", state:"working"}
after Escape      {agentType:"codex", state:"done", interrupted:true}

On the spurious "stopped" notification, the user-visible symptom in #13547: the Notification constructor was never invoked in any scenario, and the completion-unread marker was never set. The decisive evidence is upstream of both — the row never enters done/interrupted on Escape, and that transition is what the notification and unread surfaces key off. Stated precisely because this dev profile reports Notification.permission === "denied", so the constructor spy alone would be weak evidence.

All three panes in one frame — Claude still spinning after its Escape, OMP and Codex both retired by their respective real interrupts:

tab bar

Related community PRs

Three community PRs target these two issues. All three are on the right axis, and two of them arrived at the core of this invariant first.

Residual risk

  • For Claude, OMP, Pi and Prime Agent, a plain Escape no longer marks a turn interrupted at all. This is broader than the earlier revision of this PR, which refused only mid-tool. A genuine Escape interrupt is now reported by the provider's own hook or not at all — and if a provider build emits neither a closing tool hook nor a turn boundary, the row stays working until the 30-minute AGENT_STATUS_STALE_AFTER_MS decay instead of going red in ~500 ms. Previously that delay was reachable only mid-tool; now it applies across the whole turn. For Claude this is the normal path rather than a fallback — claude-events.ts already treats Stop{is_interrupt: true} as authoritative — so the exposure is builds that drop that hook. The row ages out; it does not wedge. Ctrl+C still marks these agents interrupted immediately, and every other agent type is untouched.
  • Rows carrying no hook event are now covered, where they previously fell through. OSC-parsed rows and relay envelopes without hookEventName bypassed the old event gate entirely. The agent-type rule does not read the event, so they are handled like any other row — a behaviour change in the safe direction, pinned by a test.
  • Hydration is unaffected: hydrated non-done rows are marked restoredUnconfirmed (server-hydration.ts:101), which inferInterrupt already refuses before reaching this guard. No persisted schema change, no wire change, no mobile-facing surface.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds a shared classifier for plain Escape navigation in Claude, OMP, Pi, and prime-agent. Renderer inference now ignores this input while work is active. Server inference repeats the guard before synthesizing an interruption. New tests cover provider lifecycle events, genuine interrupts, stale baselines, active subagents, loopback handling, and existing agent-specific behavior.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 97a6a

A quick Escape after Ctrl+C can leave an active agent displayed as working rather than reporting the requested interruption. Preserve the pending Ctrl+C inference before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR meets the coding requirements in [#13547] and [#9208]. The shared isNavigationEscapeIntent helper covers Claude, OMP, Pi, and Prime Agent. Renderer inference ignores plain Escape while a matc…
Out of Scope Changes check ✅ Passed The changes stay within the scope of [#13547] and [#9208]. The shared helper, renderer guard, main-process guard, and regression tests directly prevent false interruption inference and preserve provid…
Title check ✅ Passed The title clearly and concisely describes the main change: preventing navigation Escape from inferring agent completion or interruption.
Description check ✅ Passed The description is detailed and covers the change, rationale, linked issues, visual proof, testing, residual risk, and preserved behavior. It does not reproduce every template heading or checklist ite…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • server-status-inference.ts — a new isNavigationEscapeDuringOpenToolCall guard, placed after the AskUserQuestion-dismissal branch and the strict baseline-match block, refuses to promote a single plain Escape to done/interrupted while the pane's last hook event is a tool-opening event for a navigation-Escape agent (claude, omp, pi, prime-agent).
  • server-constants.ts — homes the OPEN_TOOL_CALL_HOOK_EVENTS set (PreToolUse, tool_call, tool_execution_start) alongside TOOL_PROGRESS_HOOK_EVENTS.
  • Tests — a new server-escape-navigation-inference.test.ts drives the guard through ingestRemote and the real loopback hook listener, pinning both the block and the still-infer cases (PostToolUse, Ctrl+C, AskUserQuestion dismissal, provider Stop, OMP agent_end, opencode/copilot double-Escape, droid Ctrl+C); agent-interrupt-inference.test.ts pins that the renderer still asks main to adjudicate a single Escape.

The guard is deliberately conservative: it only blocks an inference, Ctrl+C is untouched, opencode/copilot keep their double-Escape gate, and rows without hookEventName (OSC-parsed, older relays) fall through ungated to today's behavior. Guard placement is load-bearing and correctly pinned by tests — the AskUserQuestion card still dismisses because its waiting PreToolUse row is handled by dismissesClaudeQuestion before the guard, and the guard itself only ever sees working rows after the baseline match. The one residual risk (a genuine interrupt mid-tool now leaning on the provider's own closing hook rather than the silence fallback) is well-documented and degrades to the stale-decay path, not a wedge.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (since the prior review)

  • server-constants.ts — OPEN_TOOL_CALL_HOOK_EVENTS is renamed OPEN_PROVIDER_WORK_HOOK_EVENTS and widened by two events: tool_approval_resolved (OMP's granted-approval-to-tool_execution_start gap) and ui_prompt_end (Pi's modal closing over a still-running tool). The set now names the open-work window rather than only the tool-open window, and the comment documents why each new entry is safe.
  • server-status-inference.ts — the guard is renamed isNavigationEscapeWhileProviderWorkOpen; the check is otherwise unchanged.
  • Tests — two new cases pin the OMP approval→execution gap and the Pi modal-close gap; both would fail without the widened set, so they are real coverage, not theatre.

These two additions close exactly the two gaps the previous revision flagged as out of scope, and both are sound: ui_prompt_end only ever reaches the guard in its is_idle: false → working form (the state === 'working' gate drops the is_idle: true → done case), and tool_approval_resolved only ever lands on OMP rows, since the Pi/prime-agent extension never emits it. The one over-broad case — a denied approval still mapping to working and being held — is correctly documented as the safe side, since approved is dropped by normalization and a denial continues the turn with its own next event; Ctrl+C remains ungated as the genuine interrupt signal throughout. No dangling references to the renamed symbol remain.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker force-pushed the fix/escape-inference branch 2 times, most recently from c6e8610 to c2959b8 Compare September 12, 2026 03:48

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes (since the prior pullfrog review)

  • Replaced the hook-event gate with an agent-type rule — isNavigationEscapeIntent now lives in src/shared/agent-interrupt-intent.ts and refuses a plain Escape for claude/omp/pi/prime-agent regardless of the row's last hook event; OPEN_PROVIDER_WORK_HOOK_EVENTS is deleted.
  • Renderer now skips the round-trip for those agents while working — shouldIgnoreInterruptIntent short-circuits a working plain Escape, leaving Claude's waiting AskUserQuestion dismissal on its inferQuestionAnswered path.
  • Main re-checks the same rule after dismissesClaudeQuestion and the strict baseline match, so a direct or stale request cannot synthesise a done.
  • Tests — server-escape-navigation-inference.test.ts flips to "plain Escape always refused" cases (closed tool call, no tool call, no hook event) and its loopback table now pins PostToolUse+Escape and PreToolUse+Ctrl+C; two Claude background/relay guard tests switch their intent to ctrl-c so they keep exercising the guards they name.

ℹ️ PR description still documents the removed hook-event gate

The description's central invariant — "a single plain Escape may not be promoted … while the pane's last hook still shows the provider owing a result" — and its OPEN_PROVIDER_WORK_HOOK_EVENTS completeness argument describe the previous revision, not the shipped rule (a plain Escape is now refused for these agents no matter the last hook). Worth a rewrite so the merge commit carries an accurate rationale.

Technical details
# Stale PR description

## Affected sites
- PR body, "The invariant", "Why baseline equality was not enough", and "Completeness of the two hardcoded sets" — all reference `EnrichedAgentHookEventPayload.hookEventName`, `OPEN_PROVIDER_WORK_HOOK_EVENTS`, and the `PreToolUse`/`tool_call`/… event set, which the head commit deletes.

## Required outcome
- Update the invariant and implementation description to the agent-type rule (`isNavigationEscapeIntent`), and drop the now-removed event-set reasoning, so the description matches `c2959b8`.

## Suggested approach (optional)
- The "Completeness of the two hardcoded sets" section can shrink to just the agent-type list (`ESCAPE_ALSO_NAVIGATES_AGENT_TYPES`) and its evidence-driven nature.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 5eeb2584-8c70-4757-bbd6-34aba597dba3

📥 Commits

Reviewing files that changed from the base of the PR and between 24eaf6d and c2959b8.

📒 Files selected for processing (7)
  • src/main/agent-hooks/server-escape-navigation-inference.test.ts
  • src/main/agent-hooks/server-interrupt-inference-guards.test.ts
  • src/main/agent-hooks/server-relay-listener-replay.test.ts
  • src/main/agent-hooks/server/server-status-inference.ts
  • src/renderer/src/components/terminal-pane/agent-interrupt-inference.test.ts
  • src/renderer/src/components/terminal-pane/agent-interrupt-inference.ts
  • src/shared/agent-interrupt-intent.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.

nwparker and others added 4 commits September 12, 2026 22:05
Hook silence during an open tool call is evidence the tool is still running,
not that the turn was interrupted. Only the provider's closing hook may retire
such a row.

Fixes #13547
Fixes #9208
…lings

Sits next to TOOL_PROGRESS_HOOK_EVENTS, the existing hook-event-name set read by a
main-process transition guard, so the concept keeps one home. Adds pi/prime-agent
coverage and drives the contrast pair over the real loopback hook listener, which is
what stamps hookEventName for a locally launched agent.
A wire-level probe of #9208's neighbourhood found two events that land `working`
while the provider still owes a result, and so still inferred a false interrupt:
OMP's tool_approval_resolved (granted, before tool_execution_start) and Pi's
ui_prompt_end over a still-running tool.

Neither needs payload plumbing. The guard runs after the `state === 'working'`
gate, which already discriminates both: ui_prompt_end with is_idle:true maps to
`done` and never reaches it. A denied OMP approval is held too, costing only the
wait for the agent's own next event.
Escape is ambiguous at the source for Claude, OMP, Pi and Prime Agent: the same
key closes an overlay and cancels a turn, and which one it meant is focus state
only the TUI has. Nothing outside the TUI can recover it, so a plain Escape is
never evidence a turn ended for these agents — the provider's own hook decides.

Replaces the hook-event allowlist this branch carried earlier. That set had to
track five providers' event vocabularies and was wrong twice during review
(tool_approval_resolved, ui_prompt_end); the rule it approximated needs no such
set. It also removes an incoherence: trusting the provider's closing hook during
an open tool while still inferring from a keystroke outside one.

The renderer skips the round-trip and main re-checks the same rule, so a stale or
direct request cannot route around it. Ctrl+C is untouched.

Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
Co-authored-by: Rod Boev <rod.boev@gmail.com>
@nwparker
nwparker force-pushed the fix/escape-inference branch from c2959b8 to 97a6aa4 Compare September 13, 2026 05:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: f2a78aac-8c22-4c0f-a8fd-4560b38f977e

📥 Commits

Reviewing files that changed from the base of the PR and between c2959b8 and 97a6aa4.

📒 Files selected for processing (1)
  • src/main/agent-hooks/server-escape-navigation-inference.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


// Why: a stale baseline is refused ahead of the navigation guard, for either intent.
vi.setSystemTime(1_000 + 31 * 60 * 1000)
expect(pressInterruptKey(server, 'plain-escape')).toBe(false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Test a replaced status baseline.

pressInterruptKey reads the current row, so line 383 tests only the stale-age guard. Capture the initial row’s baseline, ingest a newer working row, then call server.inferInterrupt with the captured baseline. The server must return false when receivedAt or stateStartedAt no longer matches.

The renderer skip for a navigation Escape shared a clearPending() with the droid
Ctrl+C skip. Widening that predicate made the clear reachable on a common
sequence — Ctrl+C, then an Escape inside the 500ms settle window — and the real
interrupt was dropped. Escape is not a retraction.

Also retargets a renderer test that went vacuous: with claude short-circuiting,
it asserted nothing about main refusing an inference.

Reported by CodeRabbit and pullfrog on #20149.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes (incremental, since the prior pullfrog review at c2959b8)

  • Preserved a pending Ctrl+C across a navigation Escape — the renderer now early-returns from isIgnorableNavigationEscape instead of calling clearPending(), so a Ctrl+C still waiting to settle is not revoked when an overlay-dismissing Escape follows it.
  • Split the combined guard — shouldIgnoreInterruptIntent is back to the droid && ctrl-c case only, with the navigation-Escape skip moved into a dedicated isIgnorableNavigationEscape helper scoped to state === 'working'.
  • Tests — a new four-agent it.each pins that the pending Ctrl+C request survives (fails under the old clearPending() behavior); the previously-vacuous "reports main refusing" case now uses custom-agent so its mocked false actually round-trips, retiring the prior open thread.
  • Test-only refactor (97a6aa4) — the loopback case table is now a typed object array.

The behavior change is well-scoped: main only refuses plain-escape, so the surviving intent: 'ctrl-c' request is unaffected by the agent-type guard, and flushPending still re-checks baseline equality before firing, so a navigation Escape that actually changes the row cannot produce a stale interrupt. The new test genuinely fails without the fix. Suites pass locally (agent-interrupt-inference.test.ts, server-escape-navigation-inference.test.ts — 57 tests).

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit fe4237c into main Sep 13, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant