Repository navigation
Conversation
|
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
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
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check | For #24187, the send guard and status query now use a synchronized live screen, with retained-tail fallback. The regression test covers an old blocked tail and a ready live screen. The wait-stamp logi… |
Include the matched blocked rule and line in the agent_prompt_blocked error and agentWait. Add automated tests for both outputs. |
|
| Docstring Coverage | Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 8 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
Check name Status Explanation Title check ✅ Passed The title clearly describes the stale-wait clearing change and is concise enough to identify the main purpose. Description check ✅ Passed The description covers the required sections, explains the behavior and rationale, links issue #24187, and reports tests and platform limitations. It also states why visual proof is not applicable.Out of Scope Changes check ✅ Passed The live-screen check, conservative tail fallback, stale wait-stamp clearing, and related tests all address the stale blocked-wait behavior in #24187. No unrelated changes are shown.
Full details: Linked Issues check
-
Explanation
For
#24187, the send guard and status query now use a synchronized live screen, with retained-tail fallback. The regression test covers an old blocked tail and a ready live screen. The wait-stamp logic also clears a stamp when settled-ready evidence is available. However, the blocked response andagentWaitstill do not expose the matched rule and line requested by the issue. The status query continues to return only the general permission status for this case.
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Autopilot is currently an internal CodeRabbit preview.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
Important
The new clear cannot change the agent_prompt_blocked decision it targets. Both waitBlockedAt consumers bail out when detectTerminalWaitBlockedReason(terminal.waitText) is non-null, which is mutually exclusive with this PR's clear condition — so waitBlockedAt is never read when the clear fires. Details inline on tailSettledReadyClearsBlockedWait.
Reviewed changes
resolveWaitBlockedAt/tailSettledReadyClearsBlockedWait— new helpers interminal-wait-tail-state.tsthat stampwaitBlockedAton a newer blocked reason, clear it on a settled ready prompt, and otherwise preserve it.- Tail paths rerouted through the resolver —
orca-runtime-on-pty-data.ts:189(memoized leaf path) andorca-runtime-schedule-wait-blocked-check.ts:81(deferred check) now compute the stamp viaresolveWaitBlockedAt. - Deferred-clear leaf propagation —
orca-runtime-schedule-wait-blocked-check.ts:89-95nullswaitBlockedAton leaves that mirror the pty'stailBufferafter a deferred clear. - Tests — an exact-array case
[1,1,1,null,5,5], a table of negative cases fortailSettledReadyClearsBlockedWait, and the memoization sim authoring the new resolver.
ℹ️ Nitpicks
- The deferred clear loop in
runWaitBlockedCheck(orca-runtime-schedule-wait-blocked-check.ts:89-95) is the part that actually mutates leaf state, but no test exercises it — the new tests only cover the pure resolver. A test that drives a deferred check and asserts the mirroring leaf is cleared would pin the behavior the PR description highlights. resolveWaitBlockedAtcallstailSettledReadyClearsBlockedWaiteven whencurrent === null, so a candidate-bearing tail now lowercases and scans the fullwaitTextper chunk even when there is nothing to clear.
Test command from the PR body verified locally: 64 passed.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
780ac74 to
cf6ad0b
Compare
Sync update (
|
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the follow-up commit addresses both prior nitpicks.
Reviewed changes
- Hot-path guard —
resolveWaitBlockedAtnow runstailSettledReadyClearsBlockedWaitonly when the stamp is non-null, so a candidate-bearing tail no longer pays for the ready-prompt scan on every chunk (terminal-wait-tail-state.ts:122-123). - Deferred leaf-clear extracted — the inline mirrored-leaf loop in
runWaitBlockedCheckbecameclearMirroredLeafWaitStamps, with the same early-return and reference-equality semantics (terminal-wait-tail-state.ts:131-147,orca-runtime-schedule-wait-blocked-check.ts:90-96). - Tests — a
clearMirroredLeafWaitStampsunit test and aresolveWaitBlockedAtnull/set case; 66 passed locally with the PR's test command.
Not approving: the earlier finding that the clear cannot reach the agent_prompt_blocked refusal is still open and unaddressed. It is tracked in its own thread, so I am leaving that thread open rather than restating it here.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
cf6ad0b to
433ce82
Compare
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:
ae4b2f4e-e912-4855-8274-1aee35eb7b32
📥 Commits
Reviewing files that changed from the base of the PR and between cf6ad0b00563482a279abd95d879e80304edc3ea and 433ce82c8b0de4405f0357792c007cd4fd7f8f05.
📒 Files selected for processing (6)
src/main/runtime/agent-prompt-submission-runtime.test.tssrc/main/runtime/orca-runtime-resolve-authoritative-terminal-wait-permission.tssrc/main/runtime/orca-runtime-stop-requested-pty-ids.tssrc/main/runtime/orca-runtime-tail-wait-memo.test.tssrc/main/runtime/runtime-terminal-agent-status-query.tssrc/main/runtime/terminal-wait-tail-state.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the delta since the prior pullfrog review (cf6ad0b → 433ce82): one follow-up commit that retargets the agent_prompt_blocked gate at the live screen instead of the retained tail.
- Live-screen-first blocked gate —
RuntimeTerminalAgentStatusSnapshotgainedliveScreenText;RuntimeTerminalAgentStatusQuery.readStatusandresolveAuthoritativeTerminalWaitPermissionnow rundetectTerminalWaitBlockedReason(terminal.liveScreenText ?? terminal.waitText), so a settled live composer no longer inherits the tail's stale blocked phrase. - Caught-up screen wiring — the new
readLiveScreenLinesreturns the whole-grid projection only when the headless emulator'soutputSequencematches the PTY's; otherwisenullkeeps the retained-tail fallback, so provider-preferred, hydration-pending, and SSH/remote panes stay fail-closed. - Send regression test — a driving send with an old "Hooks need review" blocked tail and a settled ready live screen is now accepted.
This addresses the open concern from the earlier review: both consumers now read the current screen, so the clear no longer needs to reach the refusal. Tests pass locally (78 across the three focused files).
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
433ce82 to
3c6eef1
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the delta since the prior pullfrog review (433ce82 → 3c6eef1): one commit that makes the settled-ready tail clear actually reachable.
- Ready-prompt inspection on stamped tails —
computeTerminalTailWaitStategainsinspectTailForReadyPrompt; when set it rebuilds the retained tail, recordssettledReadyPromptviaisKnownReadyPromptSettled, and still returns the emptywaitTextso no rebuilt 256 KiB string is retained. - Clear now fires on the recorded decision —
tailSettledReadyClearsBlockedWaitreturns true whensettledReadyPrompt === true, which the previous empty-waitTextfast path could never produce, so a stale stamp on a clean ready tail now clears. - Flag gated on an existing stamp —
onPtyDataandrunWaitBlockedCheckpasswaitBlockedAt !== null, so the extra scan only runs while a stamp is present. - Test — a unit case proving the stamped state clears while the unstamped fast path does not.
waitText and signal outputs are unchanged across every branch — only the new optional field differs — so the already-reviewed live-screen consumers see no behavior change. Extended the focused run to seven wait-related files: 184 passed.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
bd3c6eb to
9c7c13b
Compare
…dy prompt A blocked stamp stayed after the dialog left, so a later send to an idle Codex composer was still refused. Clear it only when the tail is a settled ready prompt, and keep it while the tail is empty, inconclusive, or blocked.
…lear The deferred check still clears a leaf that shares the PTY tail, and that path is now tested without another output chunk. A null stamp no longer pays for the ready-prompt scan. Co-authored-by: Cursor <cursoragent@cursor.com>
9c7c13b to
07d1f42
Compare
…ine tail kept after the TUI erased it (#33485) A Codex (or Antigravity/Cline/Prime Agent) dialog drawn and erased in place no longer leaves the pane "blocked" forever when no hook/OSC 7501 clears it. The line tail still decides; the live screen may only clear a tail block when caught up, quiet 3s, no blocker anywhere on screen, and the agent's composer is ready — then the stale block stamp is cleared so the send's own echo doesn't re-block. Also defers the daemon re-attach seed's output sequence until the seed lands. Fixes #24187 Supersedes #24308 Co-authored-by: Wooseong Kim <2222333+innocarpe@users.noreply.github.com>
|
Thanks @innocarpe! Your core idea (check the current screen before refusing a send) landed in #33485, merged with your co-author credit, with extra guards so the screen can only clear a block — never miss a dialog that's really open. Closing in favor of it. |

ELI5
Orca used an old permission prompt from terminal history even after the live Codex screen was ready. It now checks the current screen when the runtime has a complete, up-to-date screen model.
What Changed
Why
A retained tail can contain a prompt that has already left the visible screen. Clearing the timestamp alone cannot change the send decision because that decision reads the blocked text first. Checking a synchronized current screen fixes that stale refusal while keeping the existing fail-closed tail fallback when the screen cannot be trusted.
Linked Issue
Fixes #24187
Visual Proof
N/A — no UI layout changes; this changes which terminal screen evidence the send guard reads.
Testing
Ran on macOS:
node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts src/main/runtime/orca-runtime-tail-wait-memo.test.ts src/main/runtime/terminal-wait-detection.test.ts src/main/runtime/agent-prompt-submission-runtime.test.tsResult: 3 test files passed, 78 tests passed. This did not include a manual live Codex pane check.
AI Disclosure
Codex (OpenAI GPT-6) assisted with this follow-up.
Review
The send guard and terminal status query now use the same current-screen-first evidence. The screen is trusted only when the headless model's output sequence matches the PTY output sequence; otherwise the existing retained-tail check remains in force.
Agent skill upstream boundary
Notes
The screen reader uses the current whole-screen headless model. Provider-preferred or stale models fall back to the existing tail checks. Cross-platform, SSH, and mobile environments were not exercised locally; CI is still awaiting maintainer approval.
Checklist
pnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (CI is awaiting maintainer approval)