Repository navigation
fix(terminal): answer a background pane's startup queries while its paired view is paused - #33273
Conversation
…aired view is paused A paired desktop pauses the output of a terminal it is not showing. The host drops that output, but it still counted the paused view as the one answering terminal queries, so main's query responder stayed silent too. A fish started in such a pane (for example by `orca terminal create`) sent DA1, got no answer, and stalled 10 s before warning. A multiplex stream now registers with its pause state, and main answers the queries of output a paused view drops, exactly as it does for output the hidden-pane gate drops. A view that receives the output still answers it, so each query is answered once. Fixes #32944 Co-authored-by: Ben Grynhaus <ben.grynhaus@wiz.io>
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: answering startup queries for a background terminal while its paired view is paused. Linked Issues check Issue #32944requires a coding fix for unanswered Fish DA1 queries. The production changes pass the multiplex stream pause state throughregisterRemoteTerminalViewSubscriber, count only remote views…Out of Scope Changes check The changes stay within issue #32944. Production edits update remote-view query authority for paused multiplex streams. The tests cover the new authority and pause behavior. Documentation comments exp…Description check The pull request description follows the required template and provides a clear ELI5 summary, user-visible behavior, implementation details, rationale, linked issue, visual-proof justification, testin… Full details: Linked Issues check
-
Explanation
Issue
#32944requires a coding fix for unanswered Fish DA1 queries. The production changes pass the multiplex stream pause state throughregisterRemoteTerminalViewSubscriber, count only remote views that receive output, and let the existing main responder answer when all remote output is paused. The added tests verify DA1 ownership during pause, resume, multiple views, and release. The issue has no additional coding acceptance criteria.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#32944. Production edits update remote-view query authority for paused multiplex streams. The tests cover the new authority and pause behavior. Documentation comments explain the same behavior. No unrelated feature or protocol change is shown.
Full details: Description check
-
Explanation
The pull request description follows the required template and provides a clear ELI5 summary, user-visible behavior, implementation details, rationale, linked issue, visual-proof justification, testing results, compatibility considerations, known gaps, and completed checklist items. The AI Disclosure and Review sections are empty, but they are non-critical and do not prevent assessment.
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- 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.
…s unsent A chunk ingested while the view was still receiving yields its queries to that view. If the view then pauses, the host drops the chunk from the output batcher, or clears it from the ack backlog, before the view ever parses it. Nothing answered those queries. The pause now carves the query bytes out of what it discards, with the same extraction the hidden-pane gate uses, and hands them to main's existing responder. The responder answers only while no other view is receiving that terminal's output. Refs #32944 Co-authored-by: Ben Grynhaus <ben.grynhaus@wiz.io>
This reverts commit 3fc500f. Answering those queries later through main's emulator injects bytes into its live parser. That can cancel a partial escape sequence, it answers cursor and mode queries against state that has since moved on, and it can answer twice when two views pause one after the other. An exact fix has to capture the replies when the bytes are first parsed and track delivery per view, so it is left as a follow-up. Also renames a test helper to say what it reads.
One day of upstream past stage 13: 365 commits and 24 conflicting files, base `120f01331ca`. Three cluster analysts, one implementer, one independent critic, which returned ACCEPT with 0 blocking defects. The target is the parent of the PTY delivery refactor `ad1b81d53a0` (stablyai#33358, "group PTY delivery and drain ownership"), which is left to stage 15 on its own. The remaining gap after this merge is about 1,685 commits. ## Most PTY conflicts were comments Bisection showed the first upstream commit to make any PTY file conflict is `3286f3b2591` (stablyai#32966, a comment audit); before it there were 5 conflicts and no PTY ones. 16 of the 24 conflicts are that audit colliding with the fork's per-window delivery code. Each was resolved as the fork's code with upstream's comment text, adapted where it now misdescribes per-window behaviour, and the critic confirmed by token comparison that the resolved code is identical to `main` in all 16 (two more resolved files also qualify). ## Collateral outside the conflict list - `terminal-query-responder.test.ts`: upstream stablyai#33273 wrote a one-argument `markHiddenRendererPty` call against the single-window gate; the fork keys marks by `webContentsId`, so the test now marks `GATE_WINDOW`. - `pty-spawn-env-terminal-basics.test.ts`: a cast upstream removed is restored with a `SAFETY:` reason, because the fork's `registerPtyHandlers` takes a `BrowserWindow` for owner routing. - `local-file-access.test.ts`: the fork's census still named `WorkspaceEmptyState.tsx`, which upstream deleted when reverting stablyai#32182 (stablyai#32838). Its renderer-relative path hid it from the repo-rooted path sweep; the regression sweep found it. ## Identity `LOCAL_HOST_ROOT_NAME = 'Orcinus'` and the `sj900211/orcinus` release feed (`TAG_HREF_RE`) survive; the identity guard passes with `upstreamRefs=0`. The guard cannot see regex-escaped URLs, so the feed's unit test is what protects that hunk. Pre-existing `stablyai` drift in the prerelease feed's download regex and its readiness-test mocks is left for a separate commit so this merge stays attributable. ## Gates Cold typecheck 0 errors (17,824 files), full-tree oxlint 0 errors and 0 `max-lines` (`register-handlers.ts` and `daemon-host-relocation.ts` at 297/300), identity guard OK, fork contracts green. A full `discoverUnitFiles()` sweep on three trees (13,498 / 13,523 / 13,585 files, missing=0) raised 81 merge-only tests; isolated and serial rechecks attribute every one to Windows EPERM teardown or shell timing that also fails on the baselines, leaving 0. The critic's independent 807-file resample on LF trees agrees. Not applied here, by ruling: cosmetic comment ports into fork-only extractions, and upstream stablyai#32940's main-window-only project-move pause barrier, which has no production caller yet and is a watch item for when upstream wires one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ELI5
Fish asks the terminal "what are you?" when it starts and waits up to 10 seconds for the answer. On a paired or SSH host, a terminal opened in the background (for example with
orca terminal create) could get no answer at all. Fish then froze for 10 seconds and printed a warning. Now the host answers whenever the viewer has paused that terminal.What Changed
Before: a desktop paired to a remote Orca host (
orca serve, or a managed SSH host) pauses the output stream of every terminal tab it isn't showing. The host then drops that tab's output. It still counted the paused viewer as the one that answers terminal queries, though, so its own query responder stayed quiet. A Primary Device Attributes query (DA1,ESC [ c) sent in that window reached nobody. Fish stalled 10 s and printedfish could not read response to Primary Device Attribute query.After: a multiplexed terminal stream registers along with its pause state. While every remote view of a terminal is paused, main's existing query responder answers that terminal's queries. It already does the same for local panes the hidden-delivery gate drops. As soon as a view receives output again, that view answers and main goes quiet.
Mechanism (host side only):
RuntimeTerminalViewSubscribers.registerRemote(ptyId, isOutputPaused?)keeps each pausable view's pause read.hasRemoteReceivingOutputcounts only views that are not paused.hasRemoteTerminalViewSubscriber, which only the query-authority decision reads, now uses that.initializeMultiplexStreampasses() => stream.outputPaused.The decision is still made once per chunk, when the chunk is ingested. The paused stream drops the chunk in that same tick, so each query gets exactly one answer: from main if the chunk was dropped, from the view if it was delivered.
Why
Instrumenting a real run showed where the query was lost. Fish's first DA1 was answered by main, because no view was attached yet. The desktop then subscribed and paused the hidden tab at once. Fish's next DA1 arrived while the host counted that paused view as the answerer, so the host dropped it unanswered.
Local desktops never hit this: their hidden panes are dropped by main's hidden-delivery gate, and main answers those queries. The pause is that same kind of drop on a remote stream, so the fix makes the existing responder treat it the same way. It adds no second responder.
Alternatives considered:
Compatibility:
Linked Issue
Fixes #32944. Reported by @bengry, who also opened #32947.
Visual Proof
N/A. This is a host-side terminal protocol fix with nothing new on screen. The before/after is fish's warning text in the terminal (see Testing).
Testing
Live, on macOS (isolated profiles, hidden windows,
ORCA_BACKGROUND_LAUNCH=1), with a scratch Playwright spec that is not checked in. It starts a windowlessorca servehost and a paired desktop client, withSHELL=fish(fish 4). The client has the worktree open; the host creates a background terminal over RPC. The spec waits 13 s and reads the host terminal.1;2ctext in the prompt, so no double answer.remote=true, after the client'sSetOutputPaused true.Unit tests (both fail on origin/main):
terminal-query-responder.test.ts: main answers DA1 while the only remote view is paused, stays quiet once it resumes or while another view is receiving, and answers again after release.terminal-multiplex-paused-output-drop.test.ts: the multiplex stream yields query authority onSetOutputPaused, takes it back on resume, and drops it on close.Other checks:
src/main/runtime/rpc/terminal-*andrpc/methods/terminal-*, with--maxWorkers=1: 58 files, 409 tests pass.pr.ymlpasses locally, including typecheck, ratchets, anti-slop and the changed-code gate.pnpm run smoke:orcad-terminalpasses.Not verified live: the managed SSH host path. It runs the same host multiplex code; the Docker managed-orcad lane runs in CI. Windows and Linux desktops are covered by tests only.
Known gaps (they predate this PR and are left for a follow-up):
drop-duplicatepath inlive-data-callback.tsand the covered prefix inhidden-output-restore-drain.ts. The host cannot see those drops.AI Disclosure
Review
Agent skill upstream boundary
Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)