Skip to content

fix(terminal): stop doubled ?997;1n color-scheme reply leaking into fish - #10054

Closed
brennanb2025 wants to merge 4 commits into
mainfrom
brennanb2025/sta2385-fish-escape-leak
Closed

brennanb2025 wants to merge 4 commits into
mainfrom
brennanb2025/sta2385-fish-escape-leak

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Fixes STA-2385.

fish 4.7.1 toggles CSI ?2031h and CSI ?2031l around terminal ownership. Per the color-scheme protocol, enabling mode 2031 subscribes to future palette-change notifications; it does not request an immediate CSI ?997...n report. Orca seeded a report on every subscription, sometimes through more than one lifecycle path, so a report could arrive after fish stopped reading terminal responses and render as literal text.

Fix:

  • make mounted, hidden-gated, parked, and remote-runtime mode-2031 subscriptions record-only
  • propagate both subscribe and unsubscribe final state through local and daemon fact channels
  • reserve CSI ?997...n for actual subscribed color-mode transitions and explicit CSI ?996n queries
  • remove the eight-attempt subscription retry timer and the parked subscription responder sidecar
  • stop classifying CSI ?2031h as a reply-eliciting query during remote snapshot replay

Reliability contract:

  • invariant: a mode-2031 enable produces zero PTY writes; only a later real color-mode transition produces one immediate notification while the subscription remains active
  • regression oracle: unit tests assert zero ordinary and immediate writes on subscribe, unsubscribe cleanup, split-chunk and final-state fact propagation, replay silence, same-mode suppression, and real flip notification
  • provider/platform scope: local, daemon, SSH/WSL fact relays, and remote-runtime paths share the protocol handling; no platform-specific path or new polling was added
  • performance: removes retry timers and an always-live parked PTY subscriber
  • residual gap: CI has protocol-level coverage but no live fish 4.7.1 E2E

Validation:

  • relevant local Vitest suites: 1,531 tests passed
  • local pnpm typecheck:web, pnpm typecheck:cli, reliability gates, max-lines ratchet, changed-file oxlint, and diff checks passed
  • local repository-wide pnpm typecheck and pnpm lint were run; the installed local toolchain stopped on unrelated existing files outside this diff
  • GitHub verify passed full lint, typecheck, tests, build, and packaged CLI smoke; Windows restart, crash-survival, Ubuntu/Windows native smoke, and Wayland input checks also passed

Fixes STA-2385.

fish probes color-scheme with two sequences (CSI ?2031h subscribe + CSI ?996n query). Orca answered BOTH with an identical CSI ?997;1n, so the reply doubled and leaked visibly into fish in regular and floating panes. #8206 only retimed the seed, so the double survived.

Fix: make the gate-managed mode-2031 subscribe fact record-only (no reply); the ?996n query answer remains the single responder.

Test plan: vitest pty-connection + query-responder green; reproduced live on macOS default config before/after.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c01e26e7-4b11-45bd-8d9d-3f6934154c42

📥 Commits

Reviewing files that changed from the base of the PR and between 7124574 and d305c06.

📒 Files selected for processing (4)
  • src/main/daemon/daemon-background-transient-facts.test.ts
  • src/main/daemon/daemon-background-transient-facts.ts
  • src/main/daemon/daemon-stream-events.ts
  • src/main/ipc/pty-hidden-delivery-gate.ts

📝 Walkthrough

Walkthrough

Updated mode-2031 transient-fact handling to emit unsubscribe events from background sessions and represent them in the daemon event model. Renderer subscribe facts now register pane subscriptions without sending immediate CSI 997 replies, with tests covering hidden and visible panes and chunk-split mode sequences.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately reflects the main change: stopping duplicate ?997;1n color-scheme replies in terminal mode-2031 handling.
Description check ✅ Passed The description is detailed and covers the fix, validation, and risk notes, even though it doesn’t follow every template heading verbatim.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dc452246-0f3b-4ac4-940f-a7a247f86829

📥 Commits

Reviewing files that changed from the base of the PR and between 4fce2de and 7124574.

📒 Files selected for processing (2)
  • src/renderer/src/components/terminal-pane/pty-connection.test.ts
  • src/renderer/src/components/terminal-pane/pty-connection.ts

Locks STA-2385's repro at the responder layer: a hidden/parked PTY that
receives fish's subscribe + query pair must emit exactly one CSI ?997;1n,
so neither a re-added subscribe seed nor a lost ?996n answer can regress.

Also repairs two comments mangled by the seed removal.
@nwparker

Copy link
Copy Markdown
Contributor

@brennanb2025 Coming back to this a year late — you were right, and this sat unmerged for the wrong reason.

I fixed #9993 today in #13904 and independently landed on your conclusion: make mode 2031 subscriptions silent. git log -S turned up your f84fedb ("fix(terminal): make mode 2031 subscriptions silent") after I'd already decided it, which was a useful confirmation.

Worth recording why this one probably stalled, since the fix direction was right. The commit message here says:

fish probes color-scheme with two sequences (CSI ?2031h subscribe + CSI ?996n query). Orca answered BOTH with an identical CSI ?997;1n, so the reply doubled

That premise doesn't hold: fish never sends ?996n — 0 occurrences across a full instrumented session. Its per-prompt probe burst is OSC 11, CPR, DA1. So the stated mechanism for the doubling was wrong even though the remedy was correct, and I'd guess that's what made it hard to trust.

The actual justification is simpler and checkable: replying to ?2031h at all is an Orca extension. Upstream xterm.js sets colorSchemeUpdates = true and emits nothing (InputHandler.ts:2035); the protocol's one-shot query is ?996n; the defined push is on an actual color change. And the reply can't win fish's ~1 ms subscribe→withdraw window across a renderer hop — measured deterministic leak at ≥1 ms, with the reply not stale by Orca's own view when written, which is why the later stale-reply gate proposals couldn't work either.

The real doubling, for the record, is xterm's own theme-write report plus maybePushMode2031Flip both firing on a color flip — two emitters colliding on a change, not on a subscribe. #13904 makes maybePushMode2031Flip the single owner.

Your diff also caught something I initially got wrong. You removed ?2031h from the replay grammar in terminal-reply-query-scan.ts; I kept it, reasoning a late-attaching remote client would otherwise lose the subscription entirely since snapshots drop control bytes. Review then showed keeping the arm without its withdraw reproduces #9993 on the remote path — a phone attaching to an idle fish session registers a subscription fish already retired. The answer was neither yours nor mine: carry both toggles so the replayed state is truthful.

Closing this in favor of #13904, which has the real-fish regression test this needed to be merge-able. Sorry it took this long to come back around — you had it first.

@nwparker

Copy link
Copy Markdown
Contributor

Superseded by #13904, now merged as 5ea7df1a5b8 — issue #9993 is closed.

Closing this properly; my earlier comment said I was closing it and I only left the comment.

To restate the credit: you had the right fix here a year ago — silent mode 2031 subscriptions. #13904 landed the same conclusion, reached independently and then confirmed against your commit. What it adds is the justification this needed to be merge-able: the Contour spec's "only send the DSR when the palette has been updated", Ghostty/iTerm2/xterm.js all emitting nothing on the DECSET, and a real-fish node-pty regression test that fails against pre-fix code.

@nwparker nwparker closed this Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants