fix: unify colour resolution behind a terminal profile

[Ayman Bagabas (aymanbagabas)]

First of a three PR stack: #94 → #95 → #96.

A screenshot and a color assertion disagreed about what color a cell was, because each resolved palette indices through its own hardcoded table. render/svg.rs had a private Theme where red was #e88388; assert/color.rs had an ANSI16 table where red was #800000. Nothing kept them in sync and nothing noticed they had drifted.

The bug

Same program, same red text, on main:

$ shell-use submit 'printf "\033[31mERROR\033[0m: red text\n"'
$ shell-use screenshot --out shot.svg     # paints ERROR #e88388

$ shell-use expect text ERROR --fg '#800000'   # a color not in the image
$ echo $?
0                                              # ...passes

$ shell-use expect text ERROR --fg '#e88388'   # the color you can see
$ echo $?
1                                              # ...fails

Asserting the color the image actually paints failed, and asserting a color that appears nowhere passed. Either the picture was lying or the assertion was, and a test suite could not tell you which.

before — image paints #e88388	after — image paints #800000

Both tables are deleted. One Colors type resolves every index once, so the renderer and the assertion cannot disagree by construction. --fg '#800000' now passes and matches the picture.

Profiles

The palette is no longer a private constant, so a session can be given one. --config picks the file and --profile the entry; discovery cascades ./shell-use.toml then ~/.shell-use/shell-use.toml.

[profiles.solarized]
scrollback = 10000

[profiles.solarized.colors]
background = "#002b36"
foreground = "#839496"
cursor     = "#d33682"
red        = "#dc322f"
green      = "#959900"
blue       = "#268bd2"

$ shell-use run --config shell-use.toml --profile solarized -- bash

A profile is exactly 19 values: the 16 ANSI colors plus foreground, background, and cursor. It is read only for the life of the session, so a program cannot rewrite the palette a test was pinned against. Anything outside 0-15 comes from a static xterm table, and scrollback defaults to 10000.

Notes for review

• --fg <index> compares the cell's palette index, so it was never affected by this bug and still is not. Only #rrggbb resolves through the palette.
• Snapshots record the slot ("fg": 1), not RGB, so existing baselines survive a palette change. There are tests pinning that.
• Defaults are the VGA palette, foreground #c0c0c0 on background #000000, which is what the assertion side already used, so no assertion changes meaning.

[Copilot]

Pull request overview

This PR fixes a correctness gap where screenshots and expect --fg/--bg "#rrggbb" could disagree on the resolved RGB for the same ANSI palette index, by centralizing palette resolution in a session “profile” that is passed through open/run and used by both renderer and assertions.

Changes:

• Introduces Profile/Colors types (TOML-backed) and resolves them in the CLI via --config / --profile (with search fallback).
• Wires the resolved profile into session creation (scrollback) and into both SVG rendering and color assertion matching.
• Adds lifecycle tests to ensure screenshots and assertions stay consistent, including with a custom profile.

Show a summary per file

File	Description
SKILL.md	Documents new open flags and adds a configuration section.
README.md	Documents configuration/profile usage and color resolution behavior.
crates/shell-use/src/session.rs	Stores a per-session Profile and uses it for emulator scrollback.
crates/shell-use/src/render/svg.rs	Removes private theme table; renders using provided Colors.
crates/shell-use/src/profile.rs	Adds profile/config parsing and unified color resolution logic.
crates/shell-use/src/lib.rs	Exposes the new profile module.
crates/shell-use/src/engine.rs	Passes profile through open/run; uses it for expect and screenshot rendering.
crates/shell-use/src/assert/color.rs	Makes #rrggbb matching resolve via session Colors (unified with renderer).
crates/shell-use/src/api.rs	Adds profile: Profile to OpenOptions / RunOptions.
crates/shell-use/Cargo.toml	Adds toml dependency for profile/config parsing.
Cargo.toml	Adds workspace toml dependency version.
Cargo.lock	Locks new transitive dependencies for toml.
crates/shell-use-cli/tests/session_lifecycle.rs	Adds end-to-end regression tests for screenshot/assertion color agreement and profile selection errors.
crates/shell-use-cli/src/protocol.rs	Extends Request::Open to carry profile (serde default for compatibility).
crates/shell-use-cli/src/main.rs	Resolves ProfileArgs and includes it in requests for open/run.
crates/shell-use-cli/src/cli.rs	Adds ProfileArgs (--config, --profile) to open/run commands.
bindings/python/native/src/lib.rs	Supplies default profile for native open/run operations.
bindings/js/native/lib.rs	Supplies default profile for native open/run operations.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 17/18 changed files
• Comments generated: 2
• Review effort level: Lite

[Ayman Bagabas (aymanbagabas)]

Review pass on the stack — validated each comment before touching anything, and two of the six turned out not to need a change.

Fixed here: the --env ellipsis, which my own edit had shifted onto --profile.

Not changed, thread left open for you: the ansi() array in Colors::rgb. Benchmarked at 5.9 ns vs 1.3 ns per lookup, which is 18 µs per screenshot — sixteen lines of match to buy that. Details on the thread, and note rgb() stops being the live path in #95 anyway.

The other four are on #95 and #96, two of which were real bugs:

• colour replies were reordered — a query answered after a device attributes reply reads as unanswered and then leaks into input (feat: answer OSC colour queries and track dynamic colours #95)
• cursor row truncated at 65535 — scrollback = 70000 drew the cursor on row 2467 instead of 68002 (feat(render): draw the cursor in screenshots #96)

[Copilot]

Review details

Suppressed comments (6)

README.md:165

• The CLI now accepts --config/--profile for run as well (per ProfileArgs being flattened into Command::Run), but the command table still shows run <program> [args...] without those flags. Please update the README command summary to include the new run flags (or explicitly document that they’re open-only if that’s intended).

| Command                                                      | Description                                 |
| ------------------------------------------------------------ | ------------------------------------------- |
| `open [--shell S] [--cols N --rows N] [--cwd D] [--env K=V] [--config F] [--profile P] [--timeout-<class> MS]` | Spawn a shell session.                      |
| `run <program> [args...]`                                    | Spawn a session running a program directly. |

SKILL.md:64

• The skill docs reflect --config/--profile for open, but not for run, even though run now also supports these options via the flattened ProfileArgs. Please update this table row for run to avoid users missing the new configuration knobs.

| `open [--shell S] [--cols N] [--rows N] [--cwd D] [--env K=V]... [--config F] [--profile P]` | Spawn a shell session (auto-starts the daemon). `--env` is repeatable. |
| `run <program> [args...] [--cols N] [--rows N] [--cwd D] [--env K=V]...` | Spawn a session running a program directly (no shell).                 |

crates/shell-use/src/profile.rs:69

• The parsing logic explicitly allows omitting the leading #, but the error message says the color must be #rgb or #rrggbb. Consider adjusting the message to reflect the actual accepted formats (e.g., rgb/rrggbb with optional leading #) to reduce confusion when users supply values like 800000.

    /// Parse `#rgb` or `#rrggbb`. The leading `#` is optional so a TOML value
    /// that lost it to a stray quote still reads sensibly.
    pub fn parse(s: &str) -> Result<Self, String> {
        let hex = s.trim().trim_start_matches('#');
        let read = |i: usize, n: usize| -> Result<u8, String> {
            u8::from_str_radix(&hex[i..i + n], 16)
                .map(|v| if n == 1 { v * 17 } else { v })
                .map_err(|_| format!("invalid hex color {s:?}"))
        };
        match hex.len() {
            3 => Ok(Rgb::new(read(0, 1)?, read(1, 1)?, read(2, 1)?)),
            6 => Ok(Rgb::new(read(0, 2)?, read(2, 2)?, read(4, 2)?)),
            _ => Err(format!("color must be #rgb or #rrggbb (got {s:?})")),
        }
    }

crates/shell-use/src/profile.rs:201

• Colors::rgb() calls self.ansi() for indices 0..=15, which constructs a fresh [Rgb; 16] array every time. Since rgb()/resolve() are likely called per-cell during rendering and assertions, this adds avoidable work. Consider resolving 0..=15 via a match on index (returning the corresponding field), or storing the 16-color array once (e.g., as a field or via a small helper returning references) so repeated lookups don’t rebuild the array.

impl Colors {
    /// The sixteen ANSI slots, in palette order.
    pub fn ansi(&self) -> [Rgb; 16] {
        [
            self.black,
            self.red,
            self.green,
            self.yellow,
            self.blue,
            self.magenta,
            self.cyan,
            self.white,
            self.bright_black,
            self.bright_red,
            self.bright_green,
            self.bright_yellow,
            self.bright_blue,
            self.bright_magenta,
            self.bright_cyan,
            self.bright_white,
        ]
    }

    /// The name a slot goes by in the config file.
    pub fn slot_name(index: u8) -> Option<&'static str> {
        Some(match NamedColor::from_index(index)? {
            NamedColor::Black => "black",
            NamedColor::Red => "red",
            NamedColor::Green => "green",
            NamedColor::Yellow => "yellow",
            NamedColor::Blue => "blue",
            NamedColor::Magenta => "magenta",
            NamedColor::Cyan => "cyan",
            NamedColor::White => "white",
            NamedColor::BrightBlack => "bright_black",
            NamedColor::BrightRed => "bright_red",
            NamedColor::BrightGreen => "bright_green",
            NamedColor::BrightYellow => "bright_yellow",
            NamedColor::BrightBlue => "bright_blue",
            NamedColor::BrightMagenta => "bright_magenta",
            NamedColor::BrightCyan => "bright_cyan",
            NamedColor::BrightWhite => "bright_white",
        })
    }

    /// Resolve any 256-color index.
    ///
    /// Slots 0-15 come from the profile. The color cube (16-231) and gray ramp
    /// (232-255) are fixed by the xterm spec and identical under every profile.
    pub fn rgb(&self, index: u8) -> Rgb {
        match index {
            0..=15 => self.ansi()[index as usize],
            16..=231 => {

crates/shell-use/src/profile.rs:490

• This unit test hard-codes POSIX /tmp/... paths, which will fail on Windows (and potentially in restricted environments). Using std::env::temp_dir() (or constructing a relative cwd like Path::new(\"some-project\")) and building the pinned path via temp_dir().join(\"pinned.toml\") would keep the test platform-independent while still validating search ordering and environment override behavior.

    fn the_search_order_puts_the_project_first() {
        static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
        let _guard = ENV_LOCK.lock().unwrap();

        let old = std::env::var_os("SHELL_USE_CONFIG");
        std::env::remove_var("SHELL_USE_CONFIG");
        let cwd = Path::new("/tmp/some-project");
        let result = std::panic::catch_unwind(|| {
            let paths = search_paths(cwd);
            assert_eq!(paths.len(), 2);
            assert_eq!(paths[0], cwd.join(CONFIG_FILE), "the project file is first");
            assert!(
                paths[1].ends_with(CONFIG_FILE) && paths[1] != paths[0],
                "the user file is second: {:?}",
                paths[1]
            );

            std::env::set_var("SHELL_USE_CONFIG", "/tmp/pinned.toml");
            let pinned = search_paths(cwd);
            assert_eq!(
                pinned,
                vec![PathBuf::from("/tmp/pinned.toml")],
                "an explicit config replaces the search entirely"
            );
        });

crates/shell-use/src/api.rs:40

• Adding a new required field to a public struct (OpenOptions.profile, similarly for RunOptions) is an API-breaking change for downstream users constructing these via struct literals. If maintaining source compatibility is important, consider making the field optional with a defaulting accessor, or providing constructor/builder APIs and marking the struct #[non_exhaustive] to discourage literal construction going forward.

#[derive(Debug, Clone)]
pub struct OpenOptions {
    pub shell: Option<Shell>,
    /// Terminal settings, already resolved from the config file by the
    /// client. The daemon never reads that file: it is long-lived and shared,
    /// so it has no single working directory to resolve a project-local config
    /// against.
    pub profile: crate::profile::Profile,
    pub cols: u16,
    pub rows: u16,
    pub cwd: Option<String>,

• Files reviewed: 17/18 changed files
• Comments generated: 0 new
• Review effort level: Lite

[cpendery (cpendery)]

Overall looks good, one gap to call out is the handling of dim / inverse. The SVG renderer still handles these properly, but the assertions are just based on the raw SGR state.

We should at least use the transformed color for the assertions. Additionally, adding --dim / --inverse flags would allow for easier assertions for the end user on those transformed colors

[cpendery (cpendery)]

I think we should also honor XDG_CACHE_HOME if we are adding a configuration file

[cpendery (cpendery)]

If we are adding a config, we should add some validation to it, ex: validating that we are getting valid colors for the profile so we don't have random crashes