Repository navigation
fix(pi): show input modals as waiting instead of working - #18836
Merged
nwparker merged 2 commits intoSep 8, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Pi status extension now tracks nested UI prompts and posts prompt lifecycle events. Active prompts add Merge Risk: ⚪ Minimal · up to Pi dialogs now show the existing action-required state while open and return to working or done when closed, with coverage for supported prompt types and modal outcomes. No concrete merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 10 files. (1 skipped: 1 unsupported.)
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. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Observe Pi's real modal lifecycle — a new generated handler (
agent-status-ui-prompt-source.ts) listens topi.on('ui_prompt_start'/'ui_prompt_end')and maps an open modal to the existingwaitingstate (orange question glyph), withctx.isIdle()at close decidingdonevsworking. - Preserve the wait through coalescing — the generated
post()tags every outgoing Pi snapshot withui_prompt_active: truewhile a modal is open, so concurrent tool/completion events and latest-only transport coalescing can't overwritewaiting. - Normalizer plumbing —
pi-family-events.tsforceswaitingforui_prompt_start/ui_prompt_active, mapsui_prompt_endthroughis_idle, and keeps a session-switching modal waiting;pi-family-tool-fields.tsclears stale tool/question fields on modal events. - Gating — OMP,
prime-agent, and legacy Pi are all left unchanged (isOmpRuntime()guards +kind === 'pi'scoping + a runtime-routed-OMP test). - Tests — meaningful unit tests (coalescing, nested/overlapping prompts, session switch, unmatched end, process isolation), a CDP E2E, and a real-Pi
ExtensionRunnersmoke test; the smoke test is intentionally not in CI.
The implementation is correct and well-tested. The waiting state is already supported by the renderer for Pi (e.g. synthetic-agent-title maps it to "Pi - action required"), so no new wire opcode or renderer state is introduced, and the two documented residual limitations (manual-only runtime smoke check; reliance on Pi's outer lifecycle pair) are called out in the PR body rather than hidden.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
|
Reviewing this one |
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Fixed the E2E status-label assertion — the terminal tab renders
waiting(andblocked) as the collapsedpermissiondot labeledNeeds attention, not the dashboard'sWaiting for input, so the spec now targets the correct label and additionally pollsagentStatusByPaneKey[paneKey].state === 'waiting'to disambiguate fromblocked, which collapses to the same dot. - Added real-Pi CDP verification tooling — a
pi-ui-prompt-cdp-smoke.mjsscript plus api-ui-prompt-extension.mjs(/orca-modal) drive all five real Pi dialogs through Electron CDP and assert backendwaiting→donealongside the visible icon;pi-ui-prompt-verification.mddocuments the manual reproduction.
The delta is test/tooling only — no production code changed. The label fix is correct (verified via terminal-tab-activity-status.ts collapsing waiting/blocked → hasPermission → AgentStateDot permission → Needs attention), and the store poll correctly uses the store to reach the state while the final assertions prove it in the DOM. The new .mjs tools are intentionally not in CI and require a real Pi (≥ 0.84.4), matching the previously documented verification boundary.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
nwparker
added a commit
that referenced
this pull request
Sep 8, 2026
* fix(pi): carry modal waits to mobile and stop losing the dialog close Follow-ups to #18836, from its readiness review. - Paint pi's `!` needs-input state marker while a dialog is open, so the 80ms spinner frame stops repainting a working title over a mid-turn wait. Mobile and the CLI read the title, so they saw `working` where the desktop already showed `waiting`. - Keep the assistant reply that lands while a dialog is open. The modal guard cleared tool fields and the `message_end` capture with them, so a turn ending under a dialog left the preview on the previous message. - Report `ui_prompt_end` even when `ctx.isIdle()` throws on a runner the modal itself invalidated; the lost post stranded the pane on `waiting`. - Declare the `esbuild` the runtime smoke tool imports. * fix(pi): hold the needs-input marker until the dialog actually closes From review of the previous commit. - Settling under an open dialog no longer retires the marker. stopAnimation painted the plain title unconditionally, so agent_settled, a resolved agent_end, or an idle auto_compaction_end erased it mid-dialog — and because that also cleared the timer, the close then painted the plain title again and the wait was lost for good. - Track the dialog as a boolean, not a depth counter. Pi does its own nesting accounting and emits one pair per stack, which is what the status extension already assumes; two files disagreeing on that would have let an inner close release the outer wait. - Reset the flag on agent_start in both extensions. A turn cannot begin under a dialog holding input focus, so it is the one boundary that can recover a close that never arrived instead of pinning the pane forever. - Leave OMP to its approval events: it reports waits through those already, and painting the marker there too would put title and hook in disagreement. * fix(pi): do not ring the completion bell for a dialog that lost its close From review of the previous commit. - Report working, not done, when ui_prompt_end's isIdle() throws. done is not cosmetic: it reaches dispatchCompletion and fires the pane's finished notification, so a turn that is still running would announce itself. The real done still arrives from agent_end/agent_settled. - Keep the idle-maintenance frame cap accruing while a dialog holds the title, so a dialog left open cannot suspend the guard that stops a compaction spinner whose end event never came. - Guard the dialog handlers against a ctx without ui. The source is generated and untypechecked, and pi does not document the ctx it passes these two events; a TypeError there would surface on every dialog. * fix(pi): let a turn still complete after a dialog loses its runner From review of the previous commit. - Re-arm the completion report when ui_prompt_end's isIdle() throws. The fallback posts working, but the finished turn had already reported its end, so nothing further would ever fire and an idle pane sat spinning. - Count dialog depth in both extensions instead of trusting pi to emit one pair per stack. The guarantee is undocumented, and if it ever does emit a pair per dialog, an inner close would release the wait the outer dialog still holds. A counter costs nothing and drops the dependency. * fix(pi): decide a dialog close from turn state, not from a guess From review of the previous commit. - Fall back to agentEndReported when ctx.isIdle is unavailable or throws. The previous guess of working stranded the common case — a dialog opened at idle — because no later event was coming to correct it, and the agentEndReported re-arm it relied on could not fire either. A turn that already reported its end is not still running, and that is knowledge this process holds without needing ctx at all. - Only suppress spinner frames once the marker is actually painted. Pi may pass a ctx with no ui, and freezing the title on its last working frame is the opposite of what the marker is for. - Gate the titlebar dialog handlers on the OMP runtime too, not just the installed kind: a bare-shell OMP launch runs inside a pi-kind pane, and the status extension already defers there. Extracted that check so both extensions share it rather than carrying two copies. * fix(pi): treat a pane that never ran a turn as idle, not busy From review of the previous commit. - Track turn-in-flight separately from agentEndReported. That flag also dedupes the completion post, so it starts false on a pane that has not run a turn — which read as still-running and left a dialog opened before the first prompt spinning forever. - Retry the marker paint on each dialog open instead of only the outermost, so an outer ctx without ui cannot decide the whole nested stack goes unmarked. - Fall back to the opening ctx when the close carries no ui. Nothing else clears the needs-input marker, so the pane would have kept asking for attention until the next turn. * fix(pi): keep a dying dialog ctx from stranding the needs-input marker The close path paints through the ctx captured at open time, which is the one a session-switching modal is most likely to have invalidated. Guard both paint sites so a throw cannot reject the handler and leave the title on the needs-input marker, and make local turn state the floor for the status extension's idleness verdict instead of a fallback. * fix(pi): hold the dialog wait against pi's own title writes and lost closes Reviewed against real Pi 0.85.1 source rather than inference: - ctx.ui is a getter that calls assertActive() and throws once a session- replacing dialog invalidates the runner, so optional chaining never screened it out and the probe sat outside the try. A throw landed after the depth decrement but before markerPainted cleared, stranding the needs-input marker until the next turn. - Pi writes the same terminal title from its own writers with no event we observe, so the marker is now re-asserted rather than merely not overwritten, on a slow timer that outlives the spinner and its cap. - resetExtensionUI drops an open dialog without resolving its promise, so a replaced or reloaded session never emits the matching ui_prompt_end. Both extensions now release the wait on session_start and shutdown. * fix(pi): build the title inside the guard, not as an argument to it paintTitle caught the setTitle throw but not the two calls one argument to its left: pi.getSessionName() asserts runner liveness the same way ctx.ui does, and process.cwd() throws ENOENT once the worktree is unlinked under a live pane. Four of the six call sites are timer callbacks, where an escape is an uncaught exception and pi exits(1) through its own handler — so the cwd route was reachable today. paintTitle now takes a builder and runs it inside the existing try. * fix(pi): let only the pane-owning process assert the needs-input marker The spinner is harmlessly per-process, but the marker is status the pane reports, and child agents inherit ORCA_PANE_KEY. Gate the two dialog handlers on a PID claim, mirroring ORCA_PI_STATUS_OWNED in the status hook.
7 of 9 tasks
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
2 participants
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.
ELI5
When Pi opens a dialog that needs an answer, Orca now shows the same orange waiting-for-input icon as Claude and Codex instead of looking busy. The icon stays until the dialog closes, then returns to working or done according to Pi's actual state.
What Changed
ui_prompt_start/ui_prompt_endevents forselect,confirm,input,editor, andcustomdialogs, including extension tools such asask_user.waitingstate and orange question glyph. Clear stale tool/question-card details; do not publish dialog titles or answers.waiting.Why
Pi already exposes this lifecycle (added in 0.84.4, earendil-works/pi#8355). The proposed
tool_approval_requestedintegration is an OMP contract, not the signal emitted by real Pi dialogs. A tool-name whitelist also misses arbitrary extension modals; elapsed-time heuristics confuse long tools with input waits.Linked Issue
Related: #15145
Related: #17236
This is a new implementation using Pi's shipped prompt events, not a continuation of the proposed approval-event branch. See the startup-dialog boundary in Notes before treating the broader issue as fully closed.
Visual Proof
No visual change to the existing indicator design; Pi now reaches the existing orange waiting state.
Linux Electron/CDP screenshots from the new end-to-end test: the same pane before and after delivering the modal-start hook. These show the real Orca UI; the test injects hook payloads rather than running an LLM. The separate runtime smoke check verifies that real Pi emits those hooks.
Testing
Maintainer takeover verification on macOS, using the PR worktree:
~/projects/pi-mono0.83.0,packages/coding-agent/src/core/extensions/runner.ts(no prompt lifecycle), and installed Pi 0.85.1,dist/core/extensions/runner.js:269–308(five wrapped UI methods, outermost depth accounting, finally/error cleanup, microtask event emission).waitingand the orange Needs attention icon, thendoneand the green completion icon. Repeated after restarting Pi. No model response is needed for these dialog flows.ORCA_BACKGROUND_LAUNCH=1 pnpm test:e2e tests/e2e/pi-ui-prompt-status.spec.ts --workers=1. Its Electron and CLI builds passed. Covers working → waiting, unrelated output while waiting, close → working → done.pnpm tc, changed-file oxlint, formatting,git diff --check, andpnpm run check:code-quality:changedpassed (zero new native/type-aware/React Doctor findings).typecheck:e2ehas 195 diagnostics in other existing E2E files; none remain in this PR's test. The full unit suite and full lint suite were not rerun during takeover.Reproduction: real-Pi verification instructions.
Real Pi visual proof (macOS)
These screenshots show actual Pi 0.85.1 dialogs in Orca, not injected hook payloads. The original Linux before/after evidence above remains for the working → waiting transition.
Readiness review
PASS for the scoped change; zero unresolved P0/P1/P2 candidates. All eight checklist areas reviewed. Counsel skipped because no candidate findings remain. This is not a claim that every supported topology was exercised.
waitingprojection and optional hook fields; no schema, persistence, pairing, route, opcode, or migration changes. Mobile receives the existing normalized state; no new mobile-facing contract touched.tests/tools.No change-specific release fix remains. Coverage limits: live active-model modal turn (active state covered by actual-runner smoke and injected-hook E2E), remote failure matrix, other operating systems, paired/mobile rendering, and full repository tests.
AI Disclosure
Original implementation assisted by OpenAI in Pi; maintainer takeover review, real-Pi validation, and test updates assisted by Codex.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
ctx.uidialogs, including arbitrary extension modals. Pre-runtime startup selectors (for example initial project trust before the extension runner exists) do not emit these events and remain an upstream observability boundary. This change does not infer their state or alter trust/approval policy.waitingstate. Real cross-host validation remains a limitation, not a claimed pass.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Regular typechecks, Electron/CLI builds, changed-code quality, focused unit suites, and the E2E pass during takeover. The combined item stays unchecked because the full suite was not rerun and the separate E2E typecheck has unrelated baseline errors.