Repository navigation
fix(automations): stop headless runs failing at the 5-minute tui-idle default - #22731
leilei3167 wants to merge 8 commits into
Conversation
… default Headless dispatch waited for tui-idle with no timeoutMs, so long agent runs inherited the runtime's 5-minute default and were marked dispatch_failed while still working. Observe completion through the retained-run terminal observer so timeouts re-arm until the shared 6h deadline.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one test-coverage nit inline.
Reviewed changes
- Headless dispatch now waits through the shared observer — the serve-mode
headlessDispatcherbuilds its completion promise fromcreateHeadlessAutomationCompletioninstead of a barewaitForTerminal(..., { condition: 'tui-idle' }), so eachtui-idletimeout re-arms until the observer's 6h deadline rather than failing at the runtime's 5-minute default. - New
headless-dispatch-terminal-completion.ts— thin helper that delegates toterminalObserver.observeCompletionwith a freshAbortControllersignal; the observer owns snapshot capture at the sameTERMINAL_SNAPSHOT_LIMIT = 2_000the removed inline block used. - New regression test — one case documents the bare 5-minute wait, the other drives the real observer + helper past that window to
completed.
Verification on the checked-out head: typecheck:node EXIT 0, check:code-quality:changed reports 0 new findings, and the two focused suites pass (8 tests).
ℹ️ Nitpicks
- The new test file re-declares its own
FakePane/FakeWaiter/createFakeRuntime(~60 lines) that closely mirrorruntime-terminal-run-observer.test.ts. Sharing the harness would keep the two fakes from drifting apart as the runtime's tui-idle satisfaction shape evolves.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe headless automation path now uses the terminal completion observer instead of waiting for Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Service shutdown may leave headless completion observations running. Confirm that they are cancelled before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change keeps completion within the existing automation service and improves handling of long-running agents and failed terminal exits. One setup-failure path may leave an observation running after the launch has been marked failed; no new external access or privilege was identified. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d86b85b1-ee9b-4a73-b008-5ca451f9c466
📒 Files selected for processing (3)
src/main/automations/headless-dispatch-terminal-completion.test.tssrc/main/automations/headless-dispatch-terminal-completion.tssrc/main/startup/main-process-automations.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Start the shared terminal observer as soon as the reused-workspace agent terminal exists, before awaiting showManagedWorktree, so a fast finish is not mistaken for leftover idle. Route the unit test through the production helper instead of a bare wait mock.
There was a problem hiding this comment.
ℹ️ No critical issues — one maintainability nit inline.
Reviewed changes
- Reused-pane race fix — the completion observer is now attached immediately after
launchAgentTerminaland beforeawait runtime.showManagedWorktree(...), so a reused pane that returns to idle during that await still records the busy edge instead of being reported as "Automation agent never started after the prompt was submitted." - First regression case now exercises production code — it drives
createHeadlessAutomationCompletionwith a stubbedobserveCompletionand asserts delegation with anAbortSignal, replacing the case that only pinned the fake runtime's own behavior.
Verified on the checked-out head: pnpm tc:node EXIT 0, and headless-dispatch-terminal-completion.test.ts + runtime-terminal-run-observer.test.ts pass 8/8.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
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: 1e803e3b-e506-4c03-8ec3-3ed27d96c0da
📒 Files selected for processing (2)
src/main/automations/headless-dispatch-terminal-completion.test.tssrc/main/startup/main-process-automations.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Attach a no-op rejection handler when creating the headless completion promise so an early observe failure during showManagedWorktree does not surface as main_unhandled_rejection before the dispatcher returns. Also return both workspace modes through one launch-result object.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the third commit since the prior pullfrog review.
- Single launch-result return — the reused-workspace early return and the second
completionconstruction are gone; both workspace modes now flow through one return object with onecompletiondeclaration, retiring the earlier "a field added to one return can go missing" nit. - Unhandled-rejection guard — a no-op
void completion.catch(() => {})covers the window before the runner attaches its own.then/.catch, so an earlyobserveCompletionfailure duringawait showManagedWorktreecannot logmain_unhandled_rejection. The runner's handler still observes the rejection and marksdispatch_failed, so no result is lost. - Ordering preserved — the observer still attaches right after
launchAgentTerminaland beforeshowManagedWorktreeon the reused path; the worktree lookup is now gated byworkspaceMode !== 'new_per_run', which matches the previouselse(the mode is a two-value union,'existing' | 'new_per_run').
Verification on the checked-out head: pnpm tc:node EXIT 0; headless-dispatch-terminal-completion.test.ts + runtime-terminal-run-observer.test.ts pass 8/8.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Handle an exited terminal before applying the busy-edge guard. · main-process-automations.ts:32-73
src/main/startup/main-process-automations.ts:32-73
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle an exited terminal before applying the busy-edge guard.
When
createManagedWorktreereturns after a fast-finished new-per-run terminal, the initialtui-idleprobe receivesstatus: 'exited'but the observer discardsstatusandexitCode. It treats the exited terminal as stale idle, waits for a busy edge that cannot occur, and persistsdispatch_failed.Use the re-exported
RuntimeTerminalWaittype. Skip the busy-edge guard for exited terminals, and classify only exit code0as completed.Suggested fix
import type { AutomationRunOutputSnapshot } from '../../shared/automations-types' +import type { RuntimeTerminalWait } from '../../shared/runtime-types' @@ waitForTerminal( handle: string, options?: { condition?: 'tui-idle'; timeoutMs?: number; signal?: AbortSignal } - ): Promise<{ satisfied: boolean; blockedReason?: string }> + ): Promise<RuntimeTerminalWait> @@ - return wait.satisfied + return wait.satisfied && wait.status !== 'exited' @@ runtime: AutomationRunTerminalHost, handle: string, - wait: { satisfied: boolean; blockedReason?: string } + wait: RuntimeTerminalWait ): Promise<AutomationRunCompletionObservation> { const outputSnapshot = await readTerminalSnapshot(runtime, handle) - if (wait.satisfied) { + if (wait.satisfied && (wait.status !== 'exited' || wait.exitCode === 0)) { return { status: 'completed', outputSnapshot, error: null } }
🟡 Minor · Handle reused-workspace lookup failure without orphaning… · main-process-automations.ts:63-85
src/main/startup/main-process-automations.ts:63-85
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle reused-workspace lookup failure without orphaning completion observation.
The runner already records
dispatch_failedwhen the dispatcher rejects. It also attaches the completion handler only after the dispatcher resolves. This behavior existed before this PR, so the missing eventual completion record is not newly introduced.This PR starts
createHeadlessAutomationCompletionbeforeshowManagedWorktree. If that lookup rejects, the dispatcher returnsdispatch_failedwhile the completion observer continues without a runner handler or an owned cancellation signal. The observer can continue until completion or its observation deadline.Make the display-name lookup nonfatal. This lets the dispatcher return the completion promise to the runner. The previously reported cancellation issue does not fix this path by itself because service-stop and watcher cancellation do not run when the dispatcher rejects.
Suggested fix
if (automation.workspaceMode !== 'new_per_run') { - const worktree = await runtime.showManagedWorktree(`id:${workspaceId}`) - workspaceDisplayName = worktree.displayName ?? null + try { + const worktree = await runtime.showManagedWorktree(`id:${workspaceId}`) + workspaceDisplayName = worktree.displayName ?? null + } catch { + // The terminal launch succeeded; the display name is optional. + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: caed81af-44c9-4ee8-85bb-b3947626b028
📒 Files selected for processing (1)
src/main/startup/main-process-automations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/startup/main-process-automations.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Skip the busy-edge guard when the initial tui-idle probe already reports status exited, and only classify exit code 0 as completed.
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: b6294b59-388b-4248-9324-1362bc3ef2ac
📒 Files selected for processing (3)
src/main/automations/headless-dispatch-terminal-completion.test.tssrc/main/automations/runtime-terminal-run-observer.test.tssrc/main/automations/runtime-terminal-run-observer.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
There was a problem hiding this comment.
Important
The narrowed RuntimeTerminalWait return type makes both test fakes non-assignable, so pnpm tc:node — and the pnpm run typecheck CI job (.github/workflows/pr.yml:304) — now fails with TS2322 in both touched test files.
Reviewed changes — re-review of the fourth commit since the prior pullfrog review (30f03c1).
- Exited terminals now settle as completion —
isTuiIdleSatisfiedNowno longer treats astatus: 'exited'wait as stale idle, so the busy-edge guard is skipped for a pane that already finished and the observation is classified from the wait result. - Non-zero exit codes fail the run —
buildObservationreportscompletedfor a satisfied wait only when the terminal is not exited or the exited terminal recordedexitCode === 0; any other exit code yieldsdispatch_failed. - Regression coverage for both exit outcomes — the observer suite gains a completed-on-exit-0 case and a failed-on-exit-1 case, and the fakes now carry
status/exitCodethroughFakeWaitResult.
ℹ️ Exited-during-wait still rejects instead of settling
isTuiIdleSatisfiedNow skips the busy-edge guard only when the initial probe reports status: 'exited'. A terminal that exits while the observer is already awaiting its long tui-idle wait is rejected with terminal_exited (src/main/runtime/orca-runtime-resolve-exit-waiters.ts:30, PTY twin at :80), and the observer rethrows anything that is not a timeout (runtime-terminal-run-observer.ts:187), so that path still lands on dispatch_failed rather than settling on the exit code. Worth confirming whether exited-as-completion is meant to cover only the already-exited probe or the whole run.
Technical details
# Mid-wait terminal exit is not classified by exit code
## Affected sites
- `src/main/automations/runtime-terminal-run-observer.ts:184-189` — the `catch` treats only `timeout` as retryable; `terminal_exited` is rethrown.
- `src/main/runtime/orca-runtime-resolve-exit-waiters.ts:26-34` and `:75-82` — a pending non-`exit` waiter is rejected, not resolved, when the terminal exits.
## Required outcome
- Decide whether an agent that finishes and exits its terminal mid-wait should settle like the already-exited probe case (completed on exit 0, dispatch_failed otherwise) or keep the rejection.
## Open questions for the human
- Is the headless agent terminal expected to stay alive after the agent finishes, making this unreachable in practice?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the fifth commit since the prior pullfrog review (970e45c6).
- Aligned the test fakes with
RuntimeTerminalWait— dropped the unusedblockedReason?: stringfield from bothFakeWaitResultdeclarations (and a trailing blank line in the observer suite). No fake ever set the field, so no coverage is lost, and the fakes are assignable toAutomationRunTerminalHostagain. - Cleared the prior CI failure — the TS2322 from the narrowed
waitForTerminalreturn type is gone:pnpm tc:nodeis EXIT 0 on this head, and the two focused suites pass 10/10.
All prior pullfrog feedback is addressed; the remaining exited-during-wait question from the previous review is unchanged and was left as an open question for the author, so it is not re-raised here.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Wire headless terminal observation through AutomationService-owned AbortControllers so stop() cancels the 6h observer path and skips persisting dispatch results after shutdown.
There was a problem hiding this comment.
Caution
The newest commit (31a88e0) breaks pnpm run typecheck in two places, so .github/workflows/pr.yml's typecheck job is red, and the abort registry it adds can record a fatal main_unhandled_rejection breadcrumb on the exact abort path it introduces.
Reviewed changes — re-review of the sixth commit since the prior pullfrog review (d5ecb50, which was approved).
- Headless completion can now be aborted on service stop — a new
HeadlessCompletionAbortRegistrywraps the serve dispatcher, allocates a per-runAbortController, threads its signal intocreateHeadlessAutomationCompletion, andAutomationService.stop()callsabortAll(). - The runner drops an aborted completion —
runHeadlessAutomationDispatchskipsmarkDispatchResultwhencompletionAbortSignalis aborted or the completion rejects withrequest_aborted, leaving the rundispatchedfor startup reconciliation. - Regression test —
run-completion-watcher.test.tscaptures the dispatcher'scompletionSignal, asserts it is aborted onstop(), and checks a late completion does not persist a terminal status.
Verification on the checked-out head 31a88e0: the three focused suites pass 21/21, but pnpm tc:node exits 1 with TS2741 in headless-dispatch-runner.ts:56 and TS2339 in run-completion-watcher.test.ts:296; pr.yml:304 runs that same command.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Begin completion abort before dispatch, pass the signal into the wrapped dispatcher, and swallow completion cleanup rejections after stop().
There was a problem hiding this comment.
Important
This commit clears both blockers from the previous review — pnpm tc:node now exits 0 and the finally-derived rejection is handled — but the abort-registry wrapper it retains still leaves the Unit tests job red: service.test.ts's headless scheduled-dispatch case now observes pending instead of dispatched.
Reviewed changes — re-review of the seventh commit since the prior pullfrog review (31a88e0).
- Runner owns the abort lifecycle —
HeadlessAutomationDispatchContextgainsbeginHeadlessCompletionAbort;runHeadlessAutomationDispatchbegins the abort before dispatch and passescompletionSignalto the dispatcher, satisfying the required field and clearing the TS2741 that failedpnpm tc:node. - Registry validates the signal —
HeadlessCompletionAbortRegistry.beginRunregisters the per-run controller, andwrapDispatcherrefuses a dispatch whosecompletionSignaldid not come frombeginRun. finallycleanup is handled —trackCompletionappends.catch(() => {})to thefinally-derived promise, so an aborted completion no longer recordsmain_unhandled_rejection.- Tests realigned — a typed
headlessLaunchOnlyhelper replaces the inline dispatcher mocks,service-precheck.test.tssuppliesbeginHeadlessCompletionAbort, and the abort test asserts through a non-nullcompletionSignal.
Verification on the checked-out head e387f24: pnpm tc:node EXIT 0; pnpm exec vitest run --config config/vitest.config.ts src/main/automations → 1 failed / 209 passed (service.test.ts:417).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-review of the eighth commit since the prior pullfrog review (e387f24).
- Test now awaits the
dispatchedtransition —service.test.ts's headless scheduled-dispatch case wraps the status assertion inawait vi.waitFor, so the extra microtask hop the abort-registry wrapper adds beforerunHeadlessAutomationDispatchrecordsdispatchedno longer races the assertion. The check still asserts the exact'dispatched'status, so it still fails if the transition never lands.
Verification on the checked-out head e53ed09: pnpm tc:node EXIT 0; pnpm exec vitest run --config config/vitest.config.ts src/main/automations → 210 passed / 0 failed (previously 1 failed at service.test.ts:417).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
Headless automation runs that take longer than five minutes were being marked as failed with a timeout error, even when the agent was still working and later finished successfully. This change makes the headless path watch for completion the same way other automation runs already do, so a long run can keep going past that five-minute mark.
What Changed
tui-idlewith notimeoutMs, so it inherited the runtime's 5-minute default and rejected withtimeoutwhile the agent was still busy.Why
The separate completion observer already treated
timeoutas retryable for up to 6 hours. Headless dispatch did not, so the same long agent run succeeded in one path and failed in the other. Routing headless through that observer is the smallest way to share the retry behavior instead of inventing a second deadline. Alternatives considered: passing a single 6-hourtimeoutMson the bare wait (fixes the cutoff, but skips the observer's reused-pane start edge and the shared "stopped watching after 6h" copy), or rewriting settlement in the larger open automation PRs (out of scope for this bug).Linked Issue
Fixes #22725
Visual Proof
N/A — headless completion waiting only; no UI or interaction change.
Testing
Regression:
pnpm exec vitest run --config config/vitest.config.ts src/main/automations/headless-dispatch-terminal-completion.test.tswaitForTerminal({ condition: 'tui-idle' })rejects withtimeoutat 5 minutes while the agent is still working (documents the pre-fix failure).completedafter the agent works past 5 minutes and then goes idle.pnpm exec vitest run --config config/vitest.config.ts src/main/automations/runtime-terminal-run-observer.test.ts— 6/6 passed.AI Disclosure
Review
Self-review notes:
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
No security, cross-platform, Remote SSH, mobile, backwards-compat, or performance concerns beyond the notes above. Desktop (non-serve) dispatch is unchanged.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)