feat: answer OSC colour queries and track dynamic colours - #95
Open
Ayman Bagabas (aymanbagabas) wants to merge 10 commits into
Open
feat: answer OSC colour queries and track dynamic colours#95Ayman Bagabas (aymanbagabas) wants to merge 10 commits into
Ayman Bagabas (aymanbagabas) wants to merge 10 commits into
Conversation
Programs ask the terminal what color it is before deciding whether to draw for a light or a dark background. Nothing answered, so every one of them blocked until it timed out and guessed. The emulator answers now, through `take_pending_writes`, which already exists for exactly this: the replies a terminal owes to device queries. alacritty parses the sequence, tracks what a program set, and hands back a formatter with the query's own prefix and terminator already captured, so the only missing piece was the color itself. That comes from the session profile, which the emulator is now constructed with. The alternative was parsing the sequences off the PTY stream, the way shell integration is tracked. That would have meant reimplementing color parsing, the runtime table, reply formatting, and terminator tracking, all of which the emulator already does — and getting the terminator wrong, since it is only visible to whoever parsed the sequence. Reading what the emulator already knows is both less code and more faithful. Colors resolve in three layers: what a program set, else the session profile, else the table the specification defines. A reset clears only the first, so the profile is unreachable from the byte stream and there is always something to restore. That is what the specification asks for, describing a reset as restoring "the color specified by the corresponding X resource". `Emulator` gains `color(slot)`, which every backend answers from its own state, plus a `palette()` snapshot of all 259 slots. The screenshot renderer and `expect --fg/--bg` take that snapshot rather than the emulator, so neither holds the session lock while it renders, and neither knows which backend produced the colors. Five conformance cases cover queries, terminator echo, set-then-reset, unconfigured indices, and that a cell follows whatever its slot now holds. They run against every backend, so a future one cannot answer differently. An end-to-end test drives a real program through the whole path: it reads the configured background, sets its own, resets, and gets the configured one back. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Follow-up to the previous commit, which routed color resolution through a `Palette` snapshot the session passed around. That put the fallback in the wrong place: the emulator reported only what a program had set, and every consumer had to know how to fill in the rest. The emulator now takes the session profile at construction and answers `color(slot)` for any slot, mapping its own table onto the profile when nothing has overridden it. Consumers ask the emulator and get a color, with no second layer to consult. `Palette` is gone. The 256-color table above the sixteen configurable slots is a static built at compile time rather than arithmetic run per lookup. It is the same in every terminal, so computing it repeatedly only invited the two implementations of it to drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The profile addressed colors by index, using 256, 257, 258 for the foreground, background, and cursor. That is alacritty's layout — it stores the dynamic colors after the palette — and it had spread into the profile, the emulator trait, and the conformance suite, none of which have any reason to know it. A backend that numbered its own table differently would have had to pretend otherwise. Slots are a `ColorSlot` enum now: `Indexed(u8)`, `Foreground`, `Background`, `Cursor`. The alacritty backend translates that to its own indices inside the one match that reads its table, and resolves an unset slot to the profile, falling back to the xterm table for an index the profile does not name. The tests for that resolution moved to the backend that performs it, where they exercise the real path rather than a helper that mirrored it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
A snapshot stores the palette slot a cell chose rather than the color that slot resolves to, which is what lets a saved baseline outlive a profile change: recoloring a terminal would otherwise invalidate every snapshot in a suite at once. That was already true and nothing checked it, so a change to how colors are serialized could have quietly made snapshots profile-dependent. The companion case pins the exception: a true-color cell names its own color, so that one is recorded literally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
…fter The color tests leaned almost entirely on `OSC 11`. That is the sequence programs actually reach for, but it meant the foreground and cursor were never set, never queried, and never reset, so wiring any of them to the wrong slot would have gone unnoticed. Three conformance cases now set all three to distinct colors and check that each reset frees only its own, that each answers its own query, and that a bare `OSC 104` resets the palette without touching them. Nothing covered the path from an escape sequence to a rendered pixel either. Both halves are pinned now: a screenshot paints the background a program set and recolors a cell whose slot it moved, and an assertion matches that same color while still comparing the index unchanged. Both return to the profile after a reset. They read the same state, so this is the earlier "a screenshot and an assertion agree" guarantee held at every point in a session rather than only at the start. The end-to-end test drives all three dynamic colors over a real PTY. It needs a wide terminal: its report is one line, and `text` returns the grid, so a narrower one wrapped the reply out of the assertion's reach. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The probe puts its own terminal in raw mode so it can read a reply that arrives without a newline and must not be echoed. That needs `termios`, which Windows CPython does not ship, so the test could only ever fail there, and a fail-fast matrix let it cancel the other two platforms. Nothing about the reply is platform specific. Its format is covered by conformance cases that run against every backend, and the write that carries it to the child is the same `pty.write` that every `type` and `submit` already exercises on Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Brings main's move of the request types out of the core crate down the stack. Colour resolution keeps main's renames and this branch's emulator argument, so assertions and screenshots still resolve through the emulator rather than through a profile the caller holds separately. `Session::profile` goes with it. The emulator takes the profile at construction and is the only thing that resolves a colour, so the copy the session kept had no readers left once the two call sites above moved over. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Ayman Bagabas (aymanbagabas)
changed the title
This was referenced Aug 5, 2026
Ayman Bagabas (aymanbagabas)
requested review from
cpendery (cpendery)
and
a lite review from Copilot
August 5, 2026 18:01
Copilot started reviewing on behalf of
Ayman Bagabas (aymanbagabas)
August 5, 2026 18:03
View session
There was a problem hiding this comment.
Pull request overview
Adds support for OSC color query/set/reset behavior to the headless terminal emulator layer, and updates rendering + assertions to resolve colors from the emulator’s effective (runtime-overridden) palette rather than only from the session profile. This makes terminals respond to OSC 10/11/12 and OSC 4 queries and ensures screenshots and expect --fg/--bg reflect what programs actually set during a session.
Changes:
- Introduces
ColorSlot/ static xterm 256-color table and exposesEmulator::color(...)+ defaultEmulator::resolve(...)for consistent color resolution across backends. - Implements color query capture/answering and runtime override resolution for the alacritty-based backend, wiring the session to provide a
Profileto the emulator. - Updates screenshot rendering and color assertions to resolve via the emulator (so runtime OSC changes affect screenshots/asserts) and adds extensive conformance + integration coverage.
Show a summary per file
| File | Description |
|---|---|
| SKILL.md | Documents runtime OSC color set/query/reset behavior and its interaction with profiles/screenshots. |
| crates/shell-use/src/terminal/emu.rs | Extends the emulator trait with color(ColorSlot) and a shared resolve(...) implementation. |
| crates/shell-use/src/terminal/conformance.rs | Updates conformance harness to construct emulators with a Profile and adds OSC color conformance cases. |
| crates/shell-use/src/terminal/alacritty.rs | Captures Event::ColorRequest, answers OSC color queries, and resolves colors via runtime overrides → profile → xterm table. |
| crates/shell-use/src/session.rs | Passes the session Profile into the emulator and removes the stored Session.profile field. |
| crates/shell-use/src/render/svg.rs | Renders via &dyn Emulator so screenshots reflect effective (runtime) colors; adds tests covering set/reset impact. |
| crates/shell-use/src/profile.rs | Adds ColorSlot and a static xterm 256-color table helper (xterm_color); simplifies Colors::rgb. |
| crates/shell-use/src/engine.rs | Updates expect color checking and screenshot rendering to resolve through the live emulator state. |
| crates/shell-use/src/assert/snapshot.rs | Adds tests ensuring snapshots record palette slots (not resolved RGB), while truecolor is recorded literally. |
| crates/shell-use/src/assert/color.rs | Makes matching/description resolve via &dyn Emulator and adds tests for runtime recoloring behavior. |
| crates/shell-use-cli/tests/session_lifecycle.rs | Adds an end-to-end Unix PTY test ensuring OSC queries are answered and set/reset is observable. |
| crates/shell-use-cli/src/monitor.rs | Updates test construction of AlacrittyEmu to pass a Profile. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+1192
to
+1196
| session | ||
| .state | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner) | ||
| .emu |
Member
Author
There was a problem hiding this comment.
The structure is as you describe — the reader thread does need this lock to process — so I measured how long it is actually held. Release build, timing render_svg alone:
| screenshot | rows | lock held |
|---|---|---|
| typical, 80x24 | 25 | 71 µs |
--full, 10k scrollback |
10001 | 15.9 ms |
The file write is already outside the lock; only the render is inside.
So the common case is 71 µs, and even a full render at the default scrollback is 16 ms — well inside the seconds a program waits for a query reply, and small against a PTY buffer. It scales with scrollback, so a much deeper one would make it worse.
I have not changed it, for a reason worth flagging: the fix you describe is a snapshot of the resolved palette, and this PR deliberately removed exactly that type. Colors resolve through the emulator now precisely so a screenshot shows what OSC set at that moment, and reintroducing a copied palette is the shape we took out. Given 71 µs in the common case I would rather not trade that back without a measurement showing it hurts.
Happy to revisit if you would rather have the shorter critical section — say the word and I will do it in a follow-up so it can be reviewed on its own.
Color answers were parked in their own buffer and appended after everything else the terminal had to say, so a chunk holding a color query followed by a device attributes request was answered attributes first. That order is load-bearing. A program pipelines a batch of queries and ends it with a device attributes request, whose reply every terminal sends, then reads until that reply and treats it as the end of the batch. An answer arriving after it looks like the query went unanswered, and is then read as though the user had typed it. Replies now share one ordered queue and a query is resolved where it sits, so answers leave in the order they were asked for. Covered by a conformance case, so any backend added later inherits it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Member
Author
|
Heads up on CI flakiness that is not from these changes, since it will keep showing up. Two timing-sensitive end-to-end tests fail intermittently when the suite runs in parallel under load. I reproduced it locally at roughly 1 in 3 full-suite runs, and checked whether it was mine by going back to the commit before the reply-ordering fix and running the same suite six times — a different test flaked there ( The two seen so far:
Both pass 6/6 in isolation. Worth a separate issue if it becomes annoying; happy to file one. |
The probe prints nothing until it is finished: its queries go to the terminal, which answers them rather than echoing them, so the screen stays unchanged for as long as python takes to start. The session runs `bash --norc` and so has no shell integration, which leaves `wait command` falling back to "the prompt came back and the screen is idle" — and an idle screen arrives immediately, long before the report does. On a loaded machine that fallback won the race and the test read the screen before the probe had written to it. Waiting for the line the test actually reads makes it wait for the right thing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second of a three PR stack: #94 → #95 → #96, based on #94 so the diff here is only this layer.
A terminal is supposed to answer when a program asks what colors it is using. Ours never did, so every program that asked had to time out and guess.
Before: the query goes unanswered
A program asking for the background (
OSC 11) and cursor (OSC 12) color:That silence is not harmless. Programs use
OSC 11to decide whether they are on a light or dark background, so a timeout means an editor or a diff tool picks a theme by guessing, and an agent driving the terminal sees whichever colors the guess produced rather than the ones actually configured. It also costs a real second of wall clock per query while the program waits.After: setting works too
OSC 10,OSC 11,OSC 12set the foreground, background, and cursor;OSC 4sets a palette entry.OSC 110,OSC 111,OSC 112, andOSC 104reset them.Background
#000000→#1c2833→#000000, red#800000→#ff5f87→#800000. The reset returns to the profile from #94, not to a second hardcoded default.How it resolves
A reset clears the runtime value only. It can never clear a configured one, so
OSC 104from a stray program cannot wipe the palette a test was pinned against.Notes for review
Event::ColorRequest, whose formatter already captured the query's prefix and terminator, so aBEL-terminated query gets aBEL-terminated answer and anST-terminated one getsST. There is no second parser over the PTY stream, andTerm::colors()is readable directly onceadvance()returns.PtyWrite, so ordering with other terminal output is unchanged.ColorSlotis an enum (Indexed(u8) | Foreground | Background | Cursor). Alacritty's internal 256/257/258 numbering stays insidealacritty.rsand does not leak into the profile type.