Skip to content

fix(orchestration): refuse dispatch --inject into a blocked pane - #10740

Open
Souptik96 wants to merge 1 commit into
stablyai:mainfrom
Souptik96:fix/dispatch-inject-blocked-prompt
Open

Souptik96 wants to merge 1 commit into
stablyai:mainfrom
Souptik96:fix/dispatch-inject-blocked-prompt

Conversation

@Souptik96

@Souptik96 Souptik96 commented Jul 26, 2026 •

Copy link
Copy Markdown

Summary

Addresses #10666. orca orchestration dispatch --inject delivers the preamble as terminal input terminated by Enter, with no check on whether the target pane is able to receive it. If the worker is sitting on Codex's directory-trust prompt, that Enter answers the prompt: the trust decision is granted as a side effect of task delivery, the TASK body is swallowed, and dispatch still returns injected: true. The coordinator then waits forever on a worker that received no work, and the task stays dispatched with no error surfaced anywhere — which is the mechanism behind the reporter's 11 orphaned codex processes accumulated over six days.

Two changes:

1. dispatch --inject refuses to inject into a pane parked on an interactive prompt. The gate reuses the readiness signal the renderer's own send path already refuses on (src/renderer/src/lib/active-agent-terminal-send-readiness.ts returns 'permission' and blocks the send). The CLI/orchestration path was simply weaker than the UI for the same hazard. The refusal happens before createDispatchContext, matching the placement of the existing "no recognized agent detected" pre-check, so no dispatch row is created and the task stays ready.

Added OrcaRuntimeService.isTerminalBlockedOnInteractivePrompt(), which is deliberately positive-evidence-only: readiness lookups throw for gone/exited/stale handles, and those cases return false rather than "blocked", so an unreachable terminal keeps surfacing its own precise error from the send path instead of a misleading one. That also keeps every existing dispatch test's behavior unchanged.

2. The Antigravity ready-prompt probe now reads the latest paint, not the first. This is what made the documented terminal wait --for tui-idle guard unreliable, which is why the issue's suggested workaround does not protect. findAntigravityReadyPromptIndex() recorded the first model line and first bare > line after the banner. Antigravity paints banner/model/prompt once, a startup modal (directory trust) is drawn after that paint, and it then repaints model/prompt lines without repeating the banner. So the ready evidence stayed positionally behind the modal text forever, findDismissedStartupModalIndex() never saw newer evidence, and the answered prompt kept reporting blockedReason: 'codex-trust-workspace' indefinitely. Recording the last match matches the lastIndexOf semantics that the Codex and Cursor probes directly beside it already use, and it is the same bug class already fixed for Cursor in #8210.

Deliberately not included (see Notes): the issue's third suggestion (surface stuck dispatches) is a design/UX change rather than a bug fix, and renaming the Codex-shaped codex-trust-workspace reason is a breaking change to a CLI-visible enum. Both are flagged for maintainers instead of decided here.

Screenshots

No visual change. Both changes are main-process logic; the error surfaces as a CLI/RPC error string.

Testing

Verified on Windows 11 with portable Node v24.18.0 (matching engines.node) and pnpm 9.15.9. Both touched suites import node:sqlite transitively, so they cannot even load on older Node — worth knowing if anyone else hits Error: No such built-in module: node:sqlite.

5 tests added. vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts src/main/runtime/rpc/methods/orchestration.test.ts:

Test files Tests
pristine upstream/main (f3d8edb) 2 passed 995 passed / 0 failed
this branch 2 passed 1000 passed / 0 failed

995 → 1000 is exactly the 5 added tests, no failures on either side and no pre-existing failures in these files.

Added tests:

  • rejects inject when the target pane is parked on an interactive prompt — asserts the throw, that sendTerminalAgentPrompt is never called, that the task stays ready, and that no dispatch context is created.
  • still injects when the target pane reports no interactive prompt — guards against over-blocking normal dispatch.
  • does not block dispatch when pane readiness cannot be determined — pins the fail-open-on-unknown decision so it can't be changed silently.
  • resolves tui-idle for an Antigravity lane past its dismissed trust prompt (#10666).
  • still reports blocked for an Antigravity lane sitting on a live trust prompt (#10666) — the important one: it pins that reading the latest ready evidence does not weaken detection of a live trust prompt into a false "ready". A false ready here would re-enable the exact injection this PR is preventing.

Negative controls (each fix reverted independently, tests re-run):

  1. Removed only the dispatch guard → the test fails by reproducing the reported symptom exactly:

    AssertionError: promise resolved "{ dispatch: { …(11) }, injected: true }" instead of rejecting
    

    That is the bug verbatim: injected: true returned for a pane parked on a prompt.

  2. Restored only the first-match ordering in the Antigravity probe → the dismissed-trust test fails:

    FAIL ... resolves tui-idle for an Antigravity lane past its dismissed trust prompt (#10666)
    AssertionError: expected { …(6) } to match object { …(4) }
    -   "satisfied": true,
    +   "satisfied": false,
    

    and the 13 other Antigravity tests — including the live-trust-prompt test — still passed, so the failure is attributable to the ordering change alone.

Other gates (all on Node 24):

  • pnpm lint — partially. oxlint exit 0 (Found 27 warnings and 0 errors, all pre-existing, none in touched files). lint:switch-exhaustiveness, check-styled-scrollbars, check:reliability-gates, check:max-lines-ratchet, verify:bundled-skill-guides, verify:localization-catalog all exit 0. verify:skill-bundle-manifest and verify:localization-coverage exit 1, and fail identically on pristine upstream/main (stale generated resources/skills/*.json; Ghostty keyword coverage). Unrelated to this change; I did not regenerate artifacts I have no business regenerating.
  • pnpm typecheck — clean, all three projects exit 0.
  • pnpm test (full suite) — not run end-to-end; pnpm test first runs ensure-native-runtime.mjs, and the node-pty native rebuild needs an MSVC toolchain I don't have on this non-admin box. Ran vitest directly on the affected suites with the baseline comparison above.
  • pnpm build — not run (native + Electron packaging, same toolchain gap).
  • oxfmt --check on all four touched files → All matched files use the correct format.
  • High-quality tests that would catch the regression — proven by both negative controls.

What I could not verify, stated plainly: I have no codex-cli and no Antigravity CLI on this machine, so there is no live end-to-end repro. Fix 1 is verified at the unit level against the readiness contract, which is where the defect lives. Fix 2 is verified against the mechanism (paint ordering) using this repo's own Antigravity fixture shapes, including antigravityPromptBeforeModelReadyScreen, which shows agy re-emitting model/path/prompt lines without repeating the banner. If Antigravity's post-trust frame differs from those fixtures in a way that changes the probe's inputs — e.g. no bare > line, or a box-drawn input row — then the probe needs a real capture to fix properly. @lalagogo2012, if you can paste the raw tail of an agy pane right after answering the trust prompt, I'll confirm or correct that half.

AI Review Report

Written and reviewed with an AI coding agent (Kiro CLI). This PR is AI-authored; the review below is the agent's own.

Risks checked:

  • Correct root cause, not just the reported one. The issue asked to "refuse to dispatch into a known-blocked terminal", and the review confirmed the readiness signal already existed and was already honored by the renderer — so this is closing an inconsistency, not inventing a new gate. For the tui-idle half, the review rejected the issue's stated diagnosis ("the detector is keyed to Codex-specific strings"). That is not the mechanism: TERMINAL_WAIT_BLOCKED_SENTINEL_RE is generic (do you trust / trust this / trusted workspace) and matches Antigravity's prompt correctly. Only the label is Codex-shaped. The actual defect is first-match vs. last-match ordering in the Antigravity ready probe, which is why the signal never cleared.
  • Not weakening a security-relevant detector. Highest-priority risk here: making the ready probe return a later index makes isKnownReadyPromptPreview() more likely to report ready, and a false "ready" would re-enable injection into a live trust prompt. Mitigations: > only matches a line whose entire trimmed content is >, and Codex/Antigravity trust prompts use › (U+203A) for their selection cursor, not >; plus a dedicated test asserts a live trust prompt still reports blocked. Verified by negative control that this test passes with both orderings.
  • Fail-open vs fail-closed on unknown readiness. Deliberate and tested: only positive evidence blocks. Failing closed would break dispatch whenever the readiness probe throws (gone/exited/stale handle) and would change existing dispatch error semantics for unrelated reasons. Pinned by its own test.
  • No extra cost on the hot path. The readiness probe runs once per --inject dispatch, only after the existing agent check already passed. findAntigravityReadyPromptIndex keeps the same single-pass, no-split() scan; the change only drops two === null short-circuits, so worst-case work is unchanged (it already walked to end of tail) and the existing "no split() on the ready tail" performance test still passes.
  • Cross-platform (macOS / Linux / Windows) — explicitly reviewed. No platform branches, no paths, no shell invocation, no shortcuts or accelerators, no labels. The terminal-tail scanning is byte/string logic over PTY output and is platform-identical; \r is already handled by the existing whitespace helper. Tests were executed on Windows; the reported behavior was on macOS arm64. No Electron platform API touched.
  • Remote / SSH / local. Dispatch and readiness both already work through the same runtime handle abstraction for local, remote, and SSH worktrees; this adds no new assumption about where the PTY lives.
  • Agent / integration neutrality. The dispatch gate is agent-neutral (it keys on readiness, not on any agent name), which matters because the reporter hit this with Codex and Antigravity. The Antigravity probe change is scoped to the Antigravity probe and leaves the Codex and Cursor probes untouched.
  • Error message quality. Names the terminal, says what is blocking, why injecting is refused, and gives two ways forward.

Flagged during review and acted on: the first draft of the "rejects inject" test let sendTerminalAgentPrompt call through, so its negative control failed with an unrelated runtime_unavailable instead of demonstrating the bug. The mock was changed to resolve successfully, which is what makes the negative control print the real injected: true symptom quoted above.

Security Audit

  • This is a security fix. The core defect converts a trust decision into a side effect of task delivery: a directory is silently added to Codex's trust list with no human ever seeing the prompt, and every subsequent agent run in that directory inherits the elevated trust. Refusing the injection restores "a human answered the trust prompt" as an invariant.
  • Input handling — no new parsing. The readiness probe consumes already-captured PTY tail text; no new external input is trusted.
  • Command execution — none added. This PR removes an unintended write of keystrokes into a security prompt.
  • Path handling — none touched.
  • Secrets — none touched. The refusal message includes only the terminal handle, never task content, prompt text, or environment.
  • Auth / permissions — this is the permission-adjacent surface being fixed. The gate also now refuses injection into an ordinary agent permission prompt ("allow this command?"), where injected Enter would likewise have approved something unreviewed.
  • IPC / RPC — the new runtime method is not exposed as an RPC method and adds no new surface; it is an internal main-process helper called by an existing handler. No change to RPC params or shapes, so no client compatibility impact.
  • Fail-open review — the deliberate fail-open on undeterminable readiness is not a security regression: it preserves today's behavior only when there is no evidence of a prompt, and it never suppresses a positive detection. Fixes are strictly additive to safety.
  • Dependencies — none added or changed. pnpm-lock.yaml untouched.
  • Follow-up — the reporter also notes --dangerously-bypass-approvals-and-sandbox no longer suppresses the trust prompt (upstream [possible degradation] Directories are now not trusted by default even with the --dangerously-bypass-approvals-and-sandbox option. openai/codex#14345). That is Codex-side; this PR makes Orca behave safely regardless of whether that flag works.

Notes

  • Scope. Suggestion 3 in the issue ("surface stuck dispatches" — flag a task that stays dispatched past a heartbeat threshold) is intentionally not in this PR. It needs a product decision on threshold and surface, and it belongs in the coordinator rather than the dispatch handler. Happy to do it as a follow-up if you tell me where you'd want it surfaced.
  • codex-trust-workspace reported for an Antigravity pane is a naming wart, exactly as the reporter observed. I left it alone deliberately: RuntimeTerminalWaitBlockedReason is CLI-visible (terminal wait --json), so renaming it or adding an agent-neutral alias is a breaking change for existing coordinator scripts. Your call — I'd be glad to add a neutral reason with the old value kept for compatibility if you want it.
  • The reporter's two "minor" observations are not addressed here (terminal create --title not applying the title; automation run records with finishedAt: null). They look like genuine separate bugs; keeping this PR to one topic. They deserve their own issues.
  • Testing notes by platform: executed on Windows 11 / Node 24. The reported behavior is macOS arm64 with codex-cli 0.145.0. No live agent CLI was available to me, so end-to-end confirmation is CI's / a maintainer's.

X handle: I don't have one.

ELI5

Injecting orchestration tasks into a blocked agent pane (e.g. a trust prompt) could accidentally answer the prompt and lose the task. Dispatch now refuses inject into blocked panes so trust decisions and tasks stay separate.

`dispatch --inject` delivered the preamble as terminal input terminated
by Enter without checking pane readiness. A worker parked on Codex's
directory-trust prompt consumed that Enter to answer the prompt, which
silently granted the trust decision as a side effect of task delivery and
swallowed the TASK body. Dispatch still returned `injected: true`, the
worker received no work, and the task stayed `dispatched` forever with no
error surfaced anywhere.

Gate `--inject` on the same readiness signal the renderer's send path
already refuses on, and fail loudly with an actionable message. Readiness
lookup failures are treated as "not blocked" so an unreachable terminal
keeps surfacing its own precise error from the send path.

Also fix the Antigravity ready-prompt probe, which is what made the
documented `terminal wait --for tui-idle` guard unreliable: it recorded
the FIRST model/prompt line after the banner, so once a startup modal was
drawn over the initial paint, the ready evidence stayed behind the modal
text and the stale blocked signal never cleared. Record the last match
instead, matching the `lastIndexOf` semantics of the Codex and Cursor
probes beside it.

Refs stablyai#10666
@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds interactive-prompt detection before injected task delivery, preventing preambles from being consumed by permission prompts while allowing injection when no blockage is reported or readiness lookup fails. Updates Gemini header matching to use the latest evidence and adds Antigravity trust-prompt tests covering repaint and no-repaint TUI-idle outcomes.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely summarizes the main change: refusing inject dispatches into blocked panes.
Description check ✅ Passed The description matches the template and provides summary, screenshots, testing, AI review, security audit, and notes sections.

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.

@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.

🧹 Nitpick comments (1)
src/main/runtime/rpc/methods/orchestration.ts (1)

498-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Condense the guard rationale.

The six-line comment repeats details already present in the thrown error. Keep the non-obvious Enter/prompt hazard in one concise line.

Proposed simplification
-        // Why (`#10666`): the preamble is delivered as terminal input terminated by Enter. A pane
-        // parked on an interactive prompt (directory trust, permission, upgrade notice) consumes
-        // that Enter to answer the prompt — granting a trust decision as a side effect of task
-        // delivery — and swallows the TASK body, so the task stays dispatched forever with no
-        // error anywhere. Refuse loudly instead. The renderer's send path already gates on this
-        // same readiness signal; the dispatch path must not be weaker than the UI.
+        // Prevent Enter-terminated preambles from answering live interactive prompts (`#10666`).

Source: Coding guidelines


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ae18acd-d735-45be-b36d-c59375bd01c2

📥 Commits

Reviewing files that changed from the base of the PR and between f5f0266 and efe8743.

📒 Files selected for processing (4)
  • src/main/runtime/orca-runtime.test.ts
  • src/main/runtime/orca-runtime.ts
  • src/main/runtime/rpc/methods/orchestration.test.ts
  • src/main/runtime/rpc/methods/orchestration.ts

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants