Do not send unsolicited DSR 997 when DECSET 2031 is enabled#44
Conversation
DECSET 2031 (ColorPaletteUpdates) should notify applications only when the color scheme changes, not immediately on enable. The unsolicited initial report leaks into applications that enable the mode without arranging to consume a response, surfacing as literal `?997;1n` text in the shell input buffer. Mirrors the fix applied to DECSET 1004 focus reporting in manaflow-ai#40. Fixes manaflow-ai/cmux#2636
📝 WalkthroughWalkthroughMode 2031 handling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/termio/stream_handler.zig (1)
1568-1614: Test looks good; consider also asserting the negative path for palette changes in a follow-up.The regression test is focused and verifies exactly the invariant this PR restores: mode bit set, no termio message, empty mailbox. Using
undefinedforsurface_mailbox,renderer_mailbox, andrenderer_wakeupis fine sincesetMode(.report_color_scheme, …)doesn't touch them on this path.One optional follow-up worth considering (not required for this PR): add a companion test that mutates the palette after enabling mode 2031 and asserts that a
color_scheme_reportis then produced, to guard against a future regression in the other direction (i.e., accidentally suppressing legitimate notifications).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/termio/stream_handler.zig` around lines 1568 - 1614, Add a companion test that after enabling mode 2031 (.report_color_scheme) mutates the palette and asserts a color scheme report is emitted: initialize Terminal, Mailbox, StreamHandler (same fields as the existing test), call handler.setMode(.report_color_scheme, true), then perform a palette mutation (e.g., via the same API/path your code uses to update the palette) and assert that handler.termio_messaged becomes true and mailbox.spsc.queue.pop() returns a non-null message of kind color_scheme_report; reference StreamHandler, setMode, .report_color_scheme, termio_messaged, mailbox.spsc.queue.pop, and the color_scheme_report message kind to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/termio/stream_handler.zig`:
- Around line 1568-1614: Add a companion test that after enabling mode 2031
(.report_color_scheme) mutates the palette and asserts a color scheme report is
emitted: initialize Terminal, Mailbox, StreamHandler (same fields as the
existing test), call handler.setMode(.report_color_scheme, true), then perform a
palette mutation (e.g., via the same API/path your code uses to update the
palette) and assert that handler.termio_messaged becomes true and
mailbox.spsc.queue.pop() returns a non-null message of kind color_scheme_report;
reference StreamHandler, setMode, .report_color_scheme, termio_messaged,
mailbox.spsc.queue.pop, and the color_scheme_report message kind to locate where
to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cac0ffe1-3096-433a-9fb1-006de608d0ec
📒 Files selected for processing (1)
src/termio/stream_handler.zig
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR removes the unsolicited Confidence Score: 5/5Safe to merge — targeted one-line behavioural fix with a clear rationale, matching mode-bit semantics are preserved, and a regression test confirms the fix. The change is minimal and correct: the mode bit is still set unconditionally at line 674 before the switch, future palette-change notifications are unaffected (gated on the mode bit in Termio.zig), and DSR 996 remains the correct explicit query path. No P0/P1 findings. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Application
participant SM as StreamHandler.setMode
participant MB as Termio Mailbox
participant TC as Termio
Note over App,TC: Before this PR - unsolicited report on enable
App->>SM: DECSET 2031 enable
SM->>SM: modes.set(report_color_scheme, true)
SM->>MB: messageWriter report_color_scheme
MB-->>App: DSR 997 leaks as literal text in shell buffer
Note over App,TC: After this PR - silent enable
App->>SM: DECSET 2031 enable
SM->>SM: modes.set(report_color_scheme, true)
Note right of SM: report_color_scheme => {} no side effect
Note over App,TC: Color scheme change still notifies correctly
TC->>TC: colorSchemeReportLocked force=false
TC->>TC: check mode bit is set
TC-->>App: DSR 997 notification sent
Note over App,TC: Explicit query via DSR 996 still works
App->>TC: DSR 996
TC->>TC: colorSchemeReportLocked force=true
TC-->>App: DSR 997 response
Reviews (1): Last reviewed commit: "Do not send unsolicited DSR 997 on mode ..." | Re-trigger Greptile |
1 participant
Summary
Mode 2031 (ColorPaletteUpdates) is defined to notify applications when the color scheme changes — it is not a query. PR #21 made DECSET 2031 synthesize an immediate
DSR 997report on enable. Applications that enable 2031 during startup without arranging to consume a response (e.g. Claude Code) have that CSI sequence leak into the shell input buffer as literal text like?997;1n.This matches the shape of the bug already fixed by #40 for DECSET 1004 (focus reporting): spontaneous reports on mode-enable are wrong, applications can query explicitly via DSR 996 if they want the current scheme.
Repro
In an affected cmux build on fish:
Prints
?997;1ninto the prompt. With this patch, nothing leaks.Originally reported against cmux: manaflow-ai/cmux#2636.
Change
src/termio/stream_handler.zig:.report_color_schemebranch insetModeno longer emits a color scheme report on enable. The mode bit is still toggled so future palette changes notify normally.Test
Adds
test "enabling mode 2031 does not emit an immediate color scheme report"in the same file, mirroring the structure of the focus-report regression test from #40. Verifies:setMode(.report_color_scheme, true).termio_messagedstays false.Verification
Built
GhosttyKit.xcframeworklocally with and without this patch. Confirmedprintf '\e[?2031h'leaks?997;1non the control build and is silent on the patched build.Refs: #40, manaflow-ai/cmux#2636.
Summary by cubic
Stop sending unsolicited DSR 997 when enabling DECSET 2031. The mode now only notifies on color scheme changes, preventing
?997;1nfrom leaking into shell input.setMode(.report_color_scheme, ...)no longer emits an immediate report; only toggles the mode bit.Written for commit 0584b6a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests