Repository navigation
fix(automations): show why a hook-review-blocked automation fails, keep it running, and stop stuck-tab pile-up - #33638
Conversation
Codex opens a blocking 'Hooks need review' dialog when a hook in hooks.json is new or changed. Only a person can answer it, so every later scheduled run of an automation hit the same dialog and failed. Pause the schedule on the first such failure and say how to recover. Also re-enable Orca's own Codex hook on SSH installs, matching local installs. Refs #28252 #23289
Walkthrough
Priority: Severity of issue fixed: Medium Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
The PR provides an actionable hook-review error, renders it on automation and run pages, and closes earlier unused panes. It does not meet all coding requirements in [#28252]. Runs still dispatch on e… |
For [#28252], add the required blocked state and one-time notification, and prevent or coalesce dispatch while the known hook-review block remains. For [#23289], require enabled state in the managed-grant fast path, set and verify `enabled:… |
-
Check name Status Explanation Title check The title clearly summarizes the primary changes: reporting hook-review failures, keeping automations enabled, and preventing stuck-terminal accumulation. Description check The description is detailed and covers the user impact, root cause, implementation, linked issues, visual proof, testing, risks, and trade-offs. It is mostly complete despite not reproducing every tem… Out of Scope Changes check The changed error message, terminal cleanup, UI notices, tests, type narrowing, and localization support the hook-review automation behavior. The SSH trust-entry change supports the managed-hook requi… Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Full details: Description check
-
Explanation
The description is detailed and covers the user impact, root cause, implementation, linked issues, visual proof, testing, risks, and trade-offs. It is mostly complete despite not reproducing every template checklist section.
Full details: Out of Scope Changes check
-
Explanation
The changed error message, terminal cleanup, UI notices, tests, type narrowing, and localization support the hook-review automation behavior. The SSH trust-entry change supports the managed-hook requirement in [
#23289]. No unrelated change is demonstrated.
-
Full details: Linked Issues check
-
Explanation
The PR provides an actionable hook-review error, renders it on automation and run pages, and closes earlier unused panes. It does not meet all coding requirements in [
#28252]. Runs still dispatch on each schedule, remaindispatch_failed, and have no distinct blocked state or one-time notification. For [#23289], the SSH installer writesenabled: truefor Orca-managed entries, but the PR does not establish the required enabled-state checks and verification across the ledger, RPC, and fallback launch paths.Resolution
For [
#28252], add the required blocked state and one-time notification, and prevent or coalesce dispatch while the known hook-review block remains. For [#23289], require enabled state in the managed-grant fast path, set and verifyenabled: truein the RPC path, verify the fallback result, and return precise managed-hook status when enablement fails.
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c501b421-2215-480b-be42-827818f78cbe
-
src/main/automations/runtime-terminal-run-observer.tssrc/main/automations/service.test.tssrc/main/automations/service.tssrc/main/codex/codex-hook-remote-install.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
-
| /** Only a person can answer a hook review; later occurrences would block on the same prompt. */ | ||
| private async pauseScheduleBlockedOnHookReview( | ||
| result: AutomationDispatchResult | ||
| ): Promise<AutomationDispatchResult> { |
There was a problem hiding this comment.
Reduce service.ts below the enforced line limit.
The supplied pipeline reports that the added service code exceeds the file’s line limit. Both static analysis and the format check fail. Extract the new pause logic or otherwise reduce the file before merging.
Source: Pipeline failures
| try { | ||
| this.store.updateAutomation(automation.id, { enabled: false }) | ||
| await this.store.flushPendingOrThrowAsync({ drainToStableGeneration: false }) | ||
| } catch (error) { | ||
| console.warn('[automations] could not pause an automation blocked on hook review:', error) | ||
| return result |
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
#!/bin/bash # Inspect the Store update and flush contracts and their failure handling. set -euo pipefail rg -n -C 8 'updateAutomation\(|flushPendingOrThrowAsync\(' src/main/persistence src/main/runtime src/main/automations
Repository: stablyai/orca
Length of output: 45667
Script executed:
set -u echo "== service pause block ==" sed -n '200,262p' src/main/automations/service.ts echo "== Store definitions (non-test) ==" rg -n -F --glob '!*.test.ts' -e 'updateAutomation(' -e 'flushPendingOrThrowAsync(' src/main/persistence src/main/automations || test "$?" -eq 1 echo "== rollback handling in persistence (non-test) ==" rg -n -F --glob '!*.test.ts' -e 'rollback' src/main/persistence || test "$?" -eq 1
Repository: stablyai/orca
Length of output: 41547
Script executed:
set -u echo "== automation-persistence updateAutomation ==" sed -n '140,200p' src/main/persistence/loading-store/automation-persistence.ts echo "== automation-definition-operations updateAutomation ==" sed -n '120,175p' src/main/persistence/scheduling-automations/automation-definition-operations.ts echo "== flushPendingOrThrowAsync ==" sed -n '110,150p' src/main/persistence/loading-store/write-flush-barriers.ts echo "== primary-state-writes mutation/rollback ==" sed -n '70,135p' src/main/persistence/loading-store/primary-state-writes.ts
Repository: stablyai/orca
Length of output: 8214
Publish the pause after a failed flush instead of returning the blocked error.
updateAutomation sets enabled: false in memory before the flush. When the flush rejects, the catch returns the original blocked result. That result has no paused message or recovery steps, and no definition-change event is published, so clients keep the stale enabled state. The in-memory pause still stops later dispatches, but a restart can reload the durable enabled: true and retry the blocked prompt.
The rollback in primary-state-writes.ts runs only for durable mutations. updateAutomation does not run through that path, so the code shows no rollback of the in-memory pause. Keep the in-memory pause and publish the change in both outcomes. Log the failed persistence and return the paused result:
🐛 Suggested fix
-
--- "a/src/main/automations/service.ts" +++ "b/src/main/automations/service.ts" @@ -232,13 +232,12 @@ if (run?.trigger !== 'scheduled' || !automation?.enabled) { return result } try { this.store.updateAutomation(automation.id, { enabled: false }) await this.store.flushPendingOrThrowAsync({ drainToStableGeneration: false }) } catch (error) { - console.warn('[automations] could not pause an automation blocked on hook review:', error) - return result + console.warn('[automations] could not persist the hook-review pause:', error) } const selector = this.store.automationChangeSelector(automation.id) this.publish?.({ reason: 'definition', ...(selector ? { selector } : {}) }) return { ...result, error: AGENT_HOOKS_REVIEW_PAUSED_ERROR }
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changetry { this.store.updateAutomation(automation.id, { enabled: false }) await this.store.flushPendingOrThrowAsync({ drainToStableGeneration: false }) } catch (error) { console.warn('[automations] could not pause an automation blocked on hook review:', error) return result try { this.store.updateAutomation(automation.id, { enabled: false }) await this.store.flushPendingOrThrowAsync({ drainToStableGeneration: false }) } catch (error) { console.warn('[automations] could not persist the hook-review pause:', error)
Source: Learnings
…hooks-review-blocks-unattended
…hooks-review-blocks-unattended
… why it fails Replace the auto-pause with a visible, self-clearing failure: the run error explains the Codex hook review, the automation and run pages show it above saved output, and earlier tabs stuck on the same prompt close so only the newest stays open to answer. Runs succeed again on their own once the hooks are trusted or removed.
nwparker
changed the title
…that are provably unused - Cleanup resolves the pane through runOwnHandle, so a cold-restored pane holding a new process is left alone. - Close only on definite 'unused' input history, matching headless run-terminal retention. - Catch failures from the fire-and-forget cleanup call. - Show the run page error notice only for dispatch_failed runs, and add the missing en catalog keys.
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
04af2b05-f94c-45cb-8c9c-cc889b2ebda4
-
src/main/automations/headless-automation-agent-launch.tssrc/main/automations/hook-review-blocked-run-terminals.test.tssrc/main/automations/hook-review-blocked-run-terminals.tssrc/main/automations/runtime-automation-service.tssrc/main/automations/runtime-terminal-run-observer.tssrc/renderer/src/components/automations/AutomationDetail.tsxsrc/renderer/src/components/automations/AutomationRunDetailsPage.tsxsrc/renderer/src/components/automations/automation-page-parts.tsxsrc/renderer/src/i18n/locales/en.json
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
-
| runtime.readTerminalClientUse(run.terminalPtyId) !== 'unused' || | ||
| !(await isStillOnHookReview(runtime, handle)) |
There was a problem hiding this comment.
Recheck client use before closing the terminal.
If a client opens an earlier run while waitForTerminal is pending, Line 54 can record unused before the client subscribes or types. The helper then closes a terminal that the client is using. The runtime treats a subscriber or user input as used. (raw.githubusercontent.com) Recheck client use and PTY ownership after the await, immediately before closeTerminal.
ELI5
Codex sometimes stops at startup and asks "Hooks need review". That happens when a hook in
hooks.jsonis new or has changed, for example after another tool installs one. Only a person can answer the question. An automation has nobody watching, so every scheduled run got stuck on it and failed quietly. One user had 21 failures in about 10 hours, and each failure left another stuck tab open.With this change:
Root cause (upstream Codex
ff2346f33f)hooks/listfor the cwd. It shows the review if any hook isUntrustedorModifiedand--dangerously-bypass-hook-trustwas not passed:codex-rs/tui/src/startup_hooks_review.rs:52-80,282-292,codex-rs/tui/src/hooks_rpc.rs:57-62,codex-rs/tui/src/lib.rs:2090,2159-2163.trusted_hashwith the current hash of the hook. Plugins also hash their content:codex-rs/hooks/src/engine/discovery.rs:842-868,1000-1016. Hooks come from every config layer (user, project, managed) plus plugins (discovery.rs:118-230,280-330).startup_hooks_review.rs:144-146,204-205), so the next launch asks again. Only trusting or removing the hook clears it. The message says that and does not suggest skipping.enabled = truethe way local installs do ([Bug]: trusted-but-disabled Orca Codex hook leaves remote worker-start at agent_readiness #23289).Why there's no pre-launch trust check
To check before launch, Orca would have to copy Codex's layer discovery, project-trust gating and plugin content hashing on every host type: local, SSH, WSL and managed
CODEX_HOMEs. The other way is to spawncodex app-serverand callhooks/listbefore every run. Either one is too big for this fix and would go stale as Codex changes. The terminal-side signal Orca already has (agent-hooks-review-prompt) is exact and costs nothing. Using it, the schedule keeps running and stays bounded.What changed
runtime-terminal-run-observer.ts: the blocked-on-hook-review error is now the actionable sentence above, notAutomation agent is blocked: agent-hooks-review-prompt.hook-review-blocked-run-terminals.ts(new), wired inruntime-automation-service.ts, so desktop,orca serveand paired runtimes all get it. When a run blocks on hook review, Orca closes the panes of earlier runs of the same automation that are still on the dialog. It checks this live withwaitForTerminal. It skips panes a client typed into. Their run records keep the status and error, and only the terminal refs are cleared.AutomationDetailshows the latest failed run's error.AutomationRunDetailsPageshowsrun.errorabove the saved output. Both use one smallAutomationRunErrorNoticewith the existing destructive-tint pattern. This covers every failed run, not just hook review.enabled: false) fromAutomationService.How verified (real Codex 0.164.0, mock Responses server, isolated
HOME/CODEX_HOMEunder /tmp, hidden dev Orca withORCA_DEV_USER_DATA_PATH+ORCA_BACKGROUND_LAUNCH=1)PreToolUsehook to the isolatedhooks.jsonand created a* * * * *Codex automation on a git project.orca terminal listshowed 1 terminal after 23 runs. Each earlier stuck tab closed when the next run blocked. The automation stayed Enabled, with the next run scheduled.trusted_hashentries went from 18 to 19). That run finished, and the next scheduled run completed on its own. I did not re-enable anything.service.test.ts/runtime-terminal-run-observer.test.ts/ the newhook-review-blocked-run-terminals.test.ts, the renderer automation tests,pnpm tcandpnpm run check:code-quality:changedall pass.Visual proof
Before (main, from the earlier revision of this PR): each minute added another failed run and another tab stuck on the dialog.
After: the automation stays Enabled and says why it fails:
The run page shows the error above the saved Codex output:
Only the newest stuck tab is open (run 23):
After I trusted the hook in that tab, the next scheduled runs completed without anyone re-enabling the automation:
Risk / trade-offs
/hooks(Review hooks) in it, it stays open.Fixes #28252
Fixes #23289