Repository navigation
fix: correct managed OMP status and readiness detection - #24196
sauravhiremath wants to merge 17 commits into
Conversation
|
Comments Outside DiffThese findings could not be posted inline.
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 9a7f9f56-878b-448f-a1da-a4115d098664
📒 Files selected for processing (27)
docs/reference/agent-status-store.mdsrc/main/pi/agent-status-extension-omp-lifecycle.test.tssrc/main/pi/agent-status-extension-omp-prefill.test.tssrc/main/pi/agent-status-extension-source.test.tssrc/main/pi/agent-status-extension-source.tssrc/main/pi/agent-status-extension-test-harness.tssrc/main/pi/agent-status-handler-source.tssrc/main/pi/agent-status-omp-state-transitions.test.tssrc/main/pi/agent-status-ui-prompt-source.tssrc/main/pi/omp-session-status-owner-source.tssrc/main/pi/omp-session-status-owner.test.tssrc/main/pi/omp-status-lifecycle-source.tssrc/main/runtime/orca-runtime-apply-tracked-pty-title.tssrc/main/runtime/orca-runtime-maybe-hydrate-headless-from-renderer.tssrc/main/runtime/orca-runtime-resolve-exit-waiters.tssrc/main/runtime/orca-runtime-sync-window-graph.tssrc/main/runtime/orca-runtime-tests/terminal-side-effect-facts-part-03.spec.tssrc/main/runtime/runtime-terminal-state-records.tssrc/main/runtime/terminal-wait-detection.tssrc/main/runtime/terminal-wait-name-only-idle.test.tssrc/main/runtime/tui-idle-agent-fixture.mjssrc/main/runtime/tui-idle-evidence.test.tssrc/main/runtime/tui-idle-evidence.tssrc/main/runtime/tui-idle-name-only-real-pty.integration.test.tssrc/renderer/src/lib/agent-send-title-status.test.tssrc/renderer/src/lib/agent-send-title-status.tssrc/shared/agent-hook-listener/providers/pi-family-events.ts
💤 Files with no reviewable changes (3)
- src/main/pi/agent-status-extension-omp-lifecycle.test.ts
- src/main/pi/agent-status-extension-source.test.ts
- src/main/pi/agent-status-extension-omp-prefill.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
|
Fixed the async-job recheck finding in OMP continues polling owned jobs and pending messages after agent_settled; Pi keeps its settled-event guard. Added regressions that keep work active after agent_settled, then clear it without another lifecycle event and require a final done status. Both settled-event cases failed before the fix and pass now. Verification:
|
There was a problem hiding this comment.
ℹ️ Minor suggestions only.
Reviewed changes
First review of this PR at head 426cea0. It reworks managed OMP status and Pi/OMP readiness detection across the generated status extension, the shared hook normalizer, and the runtime title/tui-idle evidence paths.
- OMP lifecycle + dialogs (
omp-status-lifecycle-source.tsnew,agent-status-handler-source.ts,agent-status-extension-source.ts): session start/switch, retry/compaction, and UI dialogs now post activity snapshots (is_idle,has_pending_messages,has_active_jobs); an undelivered completion is preserved acrossmodel_select/ui_prompt_*; dialog open/close is tracked by wrappingctx.uimethods with an epoch/depth guard. - Hook normalizer (
pi-family-events.ts): OMP events map toworking/waiting/donewithsessionBoundary,mainAgent, andinterrupted;agent_endstaysworkingwhile jobs or pending messages remain;idleBoundarymarks readiness-onlydone. - Readiness evidence (
tui-idle-evidence.ts,terminal-wait-detection.ts, rendereragent-send-title-status.ts): Pi/OMP native markers (π >,Pi >,OMP >) count as explicit idle; inferred body evidence no longer overrides a fresh working hook; timer-cleared working titles are tracked vialastOscTitleStaleWorkingClearso they cannot satisfy a wait. - Tests/docs: new OMP state-transition suite, a real-PTY
omp-staleregression, and a status-store reference section.
Verification this run: the full src/main/pi + src/shared/agent-hook-listener suites (46 files / 628 tests) and the runtime idle/readiness suites, including the real-PTY stale-title regression, all pass.
ℹ️ Nitpicks
- The
tui-idle-evidence.tschange replaces a long JSDoc that documented why the ranking exists and, specifically, thatsyncWindowGraphrebuilds leaves from an explicit field list so a bespoke provenance field is silently dropped. This PR now relies on exactly that mechanism forlastOscTitleStaleWorkingClear(it had to patchsyncWindowGraph, hydration, and the record types). A one-line note near the new field would preserve the hazard for the next maintainer. - Removing the precise OMP raw-payload test (
agent-status-extension-source.test.ts, "keeps OMP runtime status payloads unchanged by Pi session metadata") drops the only assertion pinning the exact/hook/ompbody shape; the new suite asserts normalized rows instead. Consider re-adding a payload-shape case now thatsession_startposts.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
ℹ️ Follow-up commit reviewed — no new issues. The prior
sessionBoundaryquestion is unaffected and still open.
Reviewed changes (delta 426cea0 → 39e2d31)
agent-status-handler-source.ts:checkPendingAgentEndno longer treatsagentSettledSupportedas terminal for OMP (agentSettledSupported && !isOmpRuntime()), so owned jobs and pending messages keep being rechecked afteragent_settleduntil they clear. Pi keeps its settled short-circuit.- Tests: the pending-message and job-clock regressions now run both with and without an
agent_settledevent. - Docs: the status-store reference notes the OMP post-
agent_settledrecheck.
Verified: agent-status-omp-state-transitions.test.ts (22 tests) and agent-status-extension-omp-lifecycle.test.ts (13 tests) pass. The guard is correct — for OMP the recheck continues past agent_settled, but still stops once completionPostedGeneration === endedRunGeneration after the all-clear, and Pi/Prime behavior is unchanged.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
ℹ️ Follow-up commit reviewed — no new issues.
Reviewed changes (delta 39e2d31 → 38c5d65)
tui-idle-evidence.ts:hasExplicitIdleTitlenow returnsfalseoutright whilerecord.lastOscTitleStaleWorkingClearis set, instead of only skipping the entry that equalslastOscTitle. This closes the rebranding hole where a renderer title that differed from the stale-cleared OSC title could still prove idle. A genuine OSC title clears the flag viaapplyTrackedPtyTitle, restoring idle evidence.- Tests: the stale-clear case is now parametrized over
undefined/'π > project'/'OMP > project'renderer titles, and asserts the verdict flips toready-strongonce the flag clears. - Docs: the status-store reference records the rebranding rule.
Verified: tui-idle-evidence.test.ts (74 tests) and terminal-wait-name-only-idle.test.ts (20 tests) pass. The earlier sessionBoundary question remains open and is unaffected by this delta.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes (delta 38c5d65 → bb71fd2)
pi-family-events.ts: removed&& mainAgent?.state !== 'done'fromidleBoundary, so every OMPdonethat is notagent_end— retry/compaction end,ui_prompt_end,tool_approval_resolved— now carriessessionBoundary: true, even when the row keeps a completedmainAgent. This resolves the prior review's open question.agent-hook-listener-pi-compatible.test.ts: added a parametrized regression that runs a completed turn, restores readiness via retry / compaction / fallback / UI prompt / tool approval, and asserts the restore is a boundarydonewith no second completion signal.docs/reference/agent-status-store.md: documented that post-completion readiness keeps the prior main-agent outcome without another completion.
Verified: agent-hook-listener-pi-compatible.test.ts (20 tests) and agent-status-omp-state-transitions.test.ts (22 tests) pass.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review at bb71fd2: the merge of upstream main (e10b8357be) into the branch, which integrates upstream's screen-ruled readiness work with this PR's stale-title gating in the tui-idle evidence path. The branch's own changes are unchanged by the merge.
- Merged upstream screen-ruled readiness (
tui-idle-evidence.ts):screenReadernow routes screen-ruled agents (Antigravity, Cline, Prime Agent) toreadScreenRuledLines,hasQuietReadyScreenadmits them, and the newreadScreenDecidesReadinessgate shuts the weak lanes.ptyTuiIdleEvidencewas adapted to upstream's newisKnownReadyPromptBody/isQuietReadyScreenBodysignatures and now supplies bothreadScreenDecidesReadinessand the sharedreadScreenthunk, mirroringleafTuiIdleEvidence. - Branch behavior preserved: the stale-clear gating (
hasExplicitIdleTitleearly-return, the!lastOscTitleStaleWorkingClearadopted-idle guard inptyTuiIdleEvidence), the body-evidence-after-working-veto ordering, and theidleBoundary/agentSettledSupported && !isOmpRuntime()handling from the prior commits all survive the merge unchanged.
Verified this run on the merged tree: tui-idle-evidence.test.ts (74), terminal-wait-name-only-idle.test.ts (20), codex-quiet-ready-screen.test.ts (53), the real-PTY tui-idle-name-only-real-pty.integration.test.ts (4), agent-status-omp-state-transitions.test.ts (22), and agent-hook-listener-pi-compatible.test.ts (20) all pass. The sole prior open thread is resolved.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the two commits since the prior pullfrog review at 6573145: shell-confirmed command-finished retirement, and status/duration retention.
- Shell-confirmed launch retirement (
orca-runtime-get-orchestration-dispatch-authority.ts,orca-runtime-emit-daemon-pty-transient-fact.ts,orca-runtime-get-unpersisted-tracked-title-for-pty.ts): command-finished markers now defer retirement to an async foreground read that must report a known shell from the current controller, and the read is fenced on controller identity, PTY incarnation, lifecycle generation, launch identity, and the restored-inventory receipt. The synchronous retire on the daemon-fact and title-tracker paths was removed, so OMP redraw markers no longer clear authority. - Status retained on unanswered reads (
pty-connection/pane-agent-identity.ts,parked-terminal-command-status.ts):hasVerifiableAgentProcessis deleted end-to-end (server, IPC, preload, web), and an unanswered or failed foreground read now cancels pending cleanup instead of dropping the row — parked SSH rows and launch identity are kept as well. - Native-chat duration kept across readiness (
native-chat-terminal-turn.ts): a readinessdonethat still carries a completedmainAgentand the host turn stamp keeps its "Worked for …" duration, resolving the prior inline nit; a boundary row with no completed main agent still reports none. - Tests/docs: new
omp-command-finished-status.test.ts(bytes and daemon paths), updated OpenCode retirement tests, OMP repeated-command-finish renderer regressions, and the status-store reference.
Verified locally on this checkout: the five changed suites (omp-command-finished-status, opencode-finished-session-authority, native-chat-terminal-turn, parked-terminal-command-status, pty-connection-unanswered-command-finish) pass (78 tests), and the src/main/pi extension suites plus agent-hook-listener-pi-compatible/server-agent-presence pass (325 tests).
ℹ️ Kept rows and launch identity have no bounded cleanup when the execution host stays silent
The new rule keeps a pane's hook row and launch identity whenever the command-finished foreground read is unavailable or unanswered, even for a pane whose agent process the host cannot check (an SSH host that never answers, or a row with no process presence). That is the stated design — no exit is inferred from silence — but there is no timeout or other bound on those kept rows, so they persist until process-exit evidence or an explicit user dismissal. Worth confirming the maintenance cost is acceptable for long-lived unreachable SSH panes, where nothing on the desktop can ever produce the confirming read.
Technical details
# Unbounded retention on unavailable command-finished reads
## Affected sites
- `src/renderer/src/components/terminal-pane/pty-connection/pane-agent-identity.ts:116-128` — `settleDeferredCommandFinishedStatusDrop` now drops only when `options.confirmedShell`; the unanswered path clears the armed drop but keeps the row and launch config.
- `src/renderer/src/components/terminal-pane/parked-terminal-command-status.ts:148-158` — parked `onCommandFinished` no longer drops rows or clears launch config; remote panes cannot produce a confirming read at all.
- `src/main/runtime/orca-runtime-get-orchestration-dispatch-authority.ts:129-164` — retirement requires `result.available && isShellProcess(result.process)`, so an unavailable host leaves authority in place.
## Required outcome
- Confirm the kept-row/launch-identity lifetime is intentional for hosts that can never answer, or define the cleanup path (host-proved exit or dismissal) that bounds it.
## Open questions for the human
- For a pane the host can never verify (unreachable SSH), is indefinite retention of the row and inherited launch token acceptable?DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the single commit since the prior pullfrog review at 58a7bb7 (bee3570), a hardening pass over the command-finished retirement path.
- Stale-read fence (
runtime-agent-hook-retirement-fence.ts,orca-runtime-get-orchestration-dispatch-authority.ts):captureAgentHookRetirementFencesnapshots the PTY's hook rows before the foreground read and re-checks them after; any change to length,observation.{authorityId,incarnation,revision},receivedAt,providerSession,state, orlaunchTokencancels retirement, so a shell read begun for an older session can no longer retire the authority of a newer one. - Startup takeover gate (
launchNeedsHookAttestationonRuntimePtyWorktreeRecord,orca-runtime-verify-orchestration-compatibility-caller.ts): a command-finished marker sets the flag before the read starts and the token-onlycurrentRuntimeLaunchSufficientfast path now requires it to be unset, so the legacy takeover recovery path falls back to host hook attestation after a marker. The flag resets only on a genuinely fresh launch (registerPtywhenlaunchToken === null,createTerminal, newrecordPtyWorktree), so re-registering the same launch does not restore the exception. - Tests/docs: new fence regressions in
omp-command-finished-status.test.ts(new session / new turn / same-state fresh activity), a parametrizedorchestration-legacy-takeover-current-authority.test.tscase covering bytes/daemon markers with non-shell and failed reads plus the sticky-flag and fresh-token transitions, and the status-store reference documenting both rules. The obsolete provider-reattach sync-retire test is removed.
Verified on this checkout: the three touched suites (31 tests) pass and pnpm tc:node passes. Both greptile P1 threads on this function are resolved by these changes, and no prior Pullfrog thread remains open.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the merge of upstream main (de8bffe) into the branch (0b2b1bb), which integrates upstream's rule-file readiness engine and authoritative hook-turn lane while preserving this PR's stale-title gating, Pi/OMP state-marker recognition, and hookless-remote handling.
- Adopted the rule-file readiness engine (
tui-idle-evidence.ts,terminal-wait-detection.ts):evaluateTuiIdlenow runs the hook-turn lane at tier 0 and routes title/screen evidence throughevaluateAgentStateRules/showsIdleTitleAnchor; the branch'slastOscTitleStaleWorkingCleargating and the body-evidence-after-working-veto ordering survive the resolution unchanged. - Preserved Pi/OMP marker recognition:
computeExplicitIdleStatusFromTitlekeepsgetPiStateTitleStatus(title) === 'idle', and the legacyπ -idle title remains recognized through thepi.jsontitle anchor, so the merge does not regress the branch's hookless remote-wait handling. - Adopted upstream's OMP approval hold (
pi-family-events.ts): aworkingevent is dropped while an OMP non-asktool approval is pending, so omp's immediatetool_execution_startno longer clears the blocked row. - Docs: the status-store reference records that a fresh authoritative hook turn outranks title/screen evidence for Pi and OMP, falling back to rule-file anchors and Pi/OMP state markers when no hook turn applies.
Verified on this checkout on Linux: tui-idle-evidence.test.ts (74), terminal-wait-name-only-idle.test.ts (20), agent-hook-listener-pi-compatible.test.ts (20), and the real-PTY tui-idle-name-only-real-pty.integration.test.ts (4) all pass.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review at 0b2b1bb: commit 1b79ee7 reconciles the branch with upstream's display-only stale-title handling and native title evidence, dropping the branch's parallel readiness veto.
- Adopted display-only stale clear (
orca-runtime-apply-tracked-pty-title.ts,runtime-terminal-state-records.ts,tui-idle-evidence.ts,terminal-wait-detection.ts): removed thelastOscTitleStaleWorkingClearfield and every readiness/delivery guard keyed on it, because the 3s stale timer now writes onlytitleDisplayClear—lastOscTitle,lastAgentStatus, waiters, mailbox, and delivery keep the agent's own title. Reverted the now-redundant branch patches tomaybe-hydrate-headless-from-renderer.ts,resolve-exit-waiters.ts,sync-window-graph.ts, and the name-only-idle suite. - Reordered body evidence (
tui-idle-evidence.ts): movedreadPositiveBodyEvidence()from the tier-1 title lane to below the fresh first-partyworkingveto, so a retained ready prompt can no longer settle over a fresh working hook; the OMP native-title branch (isOmpIdleStateTitle, title-age + screen veto) now also precedes it. - Pi/OMP markers: reverted readiness recognition via
getPiStateTitleStatusincomputeExplicitIdleStatusFromTitle; OMP'sπ >idle title is handled by the existing age/screen-gated branch, whileagent-send-title-status.tsstill recognizesπ >/OMP >/Pi >and the legacyπ -for selected-target sends. - Tests/fixture: the
omp-stalefixture now emitsπ :(working) then a genuineπ -rest title; a graph-rebuild side-effect case asserts a display-clearedπ >does not satisfy and the realπ -does; the stale-clear verdict tests are replaced by an old-prompt-vs-working-hook case.
Verified on this checkout: tui-idle-evidence.test.ts (75), the new runtime side-effect case in orca-runtime.test.ts, the real-PTY tui-idle-name-only-real-pty.integration.test.ts (4), parked-terminal-command-status.test.ts (28), agent-send-title-status.test.ts (17), native-chat-terminal-turn.test.ts (22), and agent-hook-listener-pi-compatible.test.ts (20) all pass. Both prior Pullfrog threads were already resolved.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review at 1b79ee7: the merge of upstream main (2b0ce17) into the branch (a1f705a), reconciling this PR's OMP readiness path with upstream's TerminalAgent typing.
- Adopted upstream's
TerminalAgenttyping (runtime-terminal-state-records.ts,tui-idle-evidence.ts):foregroundAgentis nowTerminalAgent | null;nameOnlyIdleNeedsCorroborationguards withisTuiAgent, andquietForegroundLanebecame the exportedquietForegroundLaneForTerminalAgent, so a recognition-onlydsbpane readsopeninstead of a missing launch config. - Docs (
agent-status-store.md): recorded the tracked reports (#22017, #24436, #22854). - Preserved branch behavior: the body-evidence-after-working-veto ordering, the OMP native idle-title age/screen gate, and the display-only stale-title handling are unchanged by the merge.
Verified on this checkout: tui-idle-evidence.test.ts (77) and the real-PTY tui-idle-name-only-real-pty.integration.test.ts (4) pass, and pnpm tc:node passes. No open Pullfrog threads remain.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
Adding a released Windows-client/Linux-headless baseline to the platform coverage discussed here: Orca client and servers 1.4.224; a fresh isolated OMP process 18.10.4. On the WSL headless server, during a simple The installed runtime still retires launch authority unconditionally at both command-finished sites. This supports the distinction your PR makes between TUI repaint, turn completion/readiness, and confirmed process exit. I have not installed or tested this branch, so this is not a claim that the execution-host check or the broader lifecycle changes already fix our setup. Please include a live paired-headless check where process inspection is missing/unavailable, and verify that later tool/completion hooks remain accepted without a new prompt. Server retirement and client park/reveal retention (#15442/#15443) should be checked separately. Detailed isolated reproduction: comment on #24436. AI assistance disclosure: prepared with an OpenAI coding assistant via OMP, using my reported behavior and local runtime diagnostics/source inspection. Reproduction and validation limits are stated above. |

ELI5
OMP can stay marked working after it finishes, or disappear while it is still running. This change keeps its status tied to current work and confirmed process state. Native chat also keeps the completed turn duration when OMP becomes ready for more input.
What Changed
agent_settled. Child extensions cannot replace the root session's status.Merged the PR target,
stablyai/orca:main, ata3373af63531. The merge keeps the upstream runtime file moves, migrated-pane restore handling, and confirmed process-exit cleanup. Launch retirement uses a separate runtime layer to keep the existing file-size limit. The PR uses upstream session and title handling rather than duplicating it. Session tests check normalized status and ownership rather than hook-event counts.Why
Turn completion, readiness, and process exit are different facts. OMP can finish its own turn while messages or children remain. It can also emit shell command-finished markers during a redraw.
The fix extends the managed reporter and the existing host store instead of adding another status source or reader-side override. Upstream title and readiness rules remain in place.
Linked Issue
Fixes #22017
Fixes #24436
Fixes #26814
Refs #22854
The two open issues cover stuck working status and live status lost after
OSC 133;D. #22854 is now closed and remains related context, not an additional closure claim. #26814 records the additional macOS reproduction with completed workers and background jobs. The process-group/cache defect in #26014 is separate and is not claimed as fixed here.Visual Proof
Orca agent status display:
Testing
The operator checks the desktop UI after every change and confirms that the OMP status behavior works as expected.
Fresh checks after merging upstream
mainata3373af63531, on macOS ARM64 withORCA_BACKGROUND_LAUNCH=1:pnpm tcpassed: node, CLI, and web.pnpm tc:nodepassed again after the runtime split.pnpm exec lint-staged --diff=upstream/mainpassed Oxlint, React Doctor, and formatting for all 52 PR source and test files.ORCA_CODE_QUALITY_BASE=upstream/main pnpm run check:code-quality:changedpassed all six scans and the casting-rationale check with zero new findings across 41 checked files.No full build, full test suite, full lint, Windows run, or live SSH/WSL run was performed. The HTTP check reported an unavailable OpenCode SQLite worker in the source-only run; all OMP status assertions passed.
The repository commit hook was skipped with
HUSKY=0after running its lint and formatting tasks against the PR diff. This avoids formatting files inherited from upstream. The global secret scanner reported 46 matches in unchanged upstream test fixtures, source hashes, and a bundled-code patch. Those files were confirmed identical to upstream, and a separate Betterleaks scan of the staged PR diff found no secrets. Only that global scanner was skipped withPREK_SKIP=betterleaks; the remaining global commit checks passed. No hook configuration was changed.AI Disclosure
Updated with OpenAI through Oh My Pi.
Review
The execution host remains the status authority. Transport loss is not proof of exit. Confirmed PTY exit and explicit dismissal retain their cleanup paths. No new wire opcode is added, and folder workspaces do not need a Git worktree.
Agent skill upstream boundary
Notes
Installed apps and managed extensions need an updated build before these source changes take effect.
Checklist