[Audit] Embedded shell / terminal — 5 verified findings (2 medium, 3 low)

[cuttlefisch]

Batched findings for the shell-terminal capability slice, from the pre-v0.15 codebase audit (epic #592).

Traced end-to-end across every crate the capability touches, then independently re-verified by a
reviewer briefed to refute each claim. Only survivors appear here, at the verifier's corrected severity.

5 findings — 2 medium, 3 low. Tick off individually; split any one out
if it turns out to need real design work.

---

1. spawn_command splits the command string on whitespace, silently mangling any quoted argument

bug · medium

Impact. terminal_spawn {"command": "git commit -m 'fix the thing'"} execs git with argv [commit, -m, 'fix, the, thing'] — literal single quotes included and the message word-split. Nothing reports an error; the terminal simply runs a different command from the one requested. Same for any ai_agent_command with a quoted flag value (e.g. mae-agent --system-prompt "be terse"), which is a plain user-facing option in init.scm.

Evidence

crates/shell/src/terminal.rs:276-285:

// Parse command into program + args (simple space-split).
let parts: Vec<&str> = command.split_whitespace().collect();
let (program, args): (String, Vec<String>) = if parts.is_empty() {
    return Err("empty command".into());
} else {
    (
        parts[0].to_string(),
        parts[1..].iter().map(|s| s.to_string()).collect(),
    )
};

The resulting argv is handed straight to tty::Shell::new(program, args) (:303) or through login_wrapped_argv (:288), which deliberately passes args byte-for-byte via positional parameters (crates/shell/src/shell_invocation.rs:107-127) — so nothing downstream re-joins or re-parses the quoting. The limitation is acknowledged only inside a test helper comment, crates/shell/src/terminal.rs:759-761: "spawn_command's command parameter is a naive split_whitespace() (pre-existing, unrelated to this fix) — so the test target must be a single bare path, no embedded quoting/args." Reachable from the AI via terminal_spawn's command prop (crates/ai/src/tools/shell_tools.rs:52 -> crates/ai/src/tool_impls/shell.rs:114-119 -> editor.shell.agent_spawns) and from the user via the ai_agent_command option (crates/core/src/options.rs:170).

Verification

The mechanism is confirmed verbatim. crates/shell/src/terminal.rs:275-285: // Parse command into program + args (simple space-split). then let parts: Vec<&str> = command.split_whitespace().collect(); and (parts[0].to_string(), parts[1..].iter().map(|s| s.to_string()).collect()). Nothing downstream repairs it: login_wrapped_argv (shell_invocation.rs:110-129) builds [-i, -l, -c, script, program, ...args] and its own doc comment documents that adversarial args "pass through byte-for-byte with no reinterpretation", and the non-wrapped branch hands tty::Shell::new(program, args) straight to the PTY (terminal.rs:302-303). The limitation is acknowledged only in a test-helper comment (terminal.rs:758-761). AI reachability confirmed: shell_tools.rs:52 terminal_spawn.command -> tool_impls/shell.rs:114-115 editor.shell.agent_spawns.push((idx, cmd)); -> shell_lifecycle.rs:117-131 ShellTerminal::spawn_command(inner_cols, inner_rows, &command, cwd, extra_env, editor.ai.agent_login_shell).

Scope correction from verification: One named artifact does not exist and must be corrected: there is no ai_agent_command option. grep -rn ai_agent_command crates/ --include=*.rs returns nothing; options.rs:169-171 registers ai_editor ("Command to launch for AI agent shell sessions (e.g. mae-agent, claude, aider)", default mae-agent), and options.rs:172 is ai_agent_login_shell. The user-facing path is real but runs through ai_editor -> self.ai.editor_name (option_ops.rs:88/508) -> dispatch/ui.rs:449-450 let cmd = self.ai.editor_name.clone(); self.shell.agent_spawns.push((new_idx, cmd)); -> the same spawn_command, so (set-option! "ai_editor" "mae-agent --system-prompt \"be terse\"") is mangled exactly as described.

---

2. GUI shell renderer silently drops ITALIC, UNDERLINE, DIM and STRIKEOUT cell attributes that the TUI honours

parity-gap · medium

Impact. The Skia/GUI backend is the primary target per CLAUDE.md, yet in the GUI the embedded terminal renders man pages with no underlined/italic sections, git diff --color-moved/grep --color emphasis flattened, systemctl status dimmed secondary text at full intensity, and any TUI that uses strikeout (e.g. task-list tools) with the strike missing. The same program in the TUI backend shows all of them, so it reads as a GUI rendering bug rather than a terminal-program bug.

Evidence

TUI honours seven attributes — crates/renderer/src/shell_render.rs:104-125:

if flags.contains(CellFlags::INVERSE) { ... }
if flags.contains(CellFlags::BOLD) { style = style.add_modifier(Modifier::BOLD); }
if flags.contains(CellFlags::ITALIC) { style = style.add_modifier(Modifier::ITALIC); }
if flags.intersects(CellFlags::ALL_UNDERLINES) { style = style.add_modifier(Modifier::UNDERLINED); }
if flags.contains(CellFlags::DIM) { style = style.add_modifier(Modifier::DIM); }
if flags.contains(CellFlags::STRIKEOUT) { style = style.add_modifier(Modifier::CROSSED_OUT); }
if flags.contains(CellFlags::HIDDEN) { style = style.add_modifier(Modifier::HIDDEN); }

The GUI reads exactly two — crates/gui/src/shell_render.rs:145-152:

let hidden = flags.contains(CellFlags::HIDDEN);
grid[line_idx as usize][col_idx] = Some(CellInfo {
    fg: fg_color,
    bg: bg_color,
    ch: if hidden { ' ' } else { indexed.cell.c },
    bold: flags.contains(CellFlags::BOLD),
});

CellInfo (gui/shell_render.rs:100-105) has no italic/underline/strike/dim field, and the draw calls hardcode the italic argument to false (gui/shell_render.rs:245-253 draw_text_run(row, col, &run_buf, run_fg, run_bold, false, 1.0) and :263 draw_char(..., bold, false, 1.0)). This is not a Skia limitation: SkiaCanvas already exposes draw_text_run(..., bold: bool, italic: bool, ...) (crates/gui/src/canvas.rs:1107-1116), draw_underline_at_y (canvas.rs:742) and draw_strikethrough_at_y (canvas.rs:905) — all used by the GUI buffer renderer, just never by the shell renderer.

Verification

Confirmed verbatim on both sides. TUI: crates/renderer/src/shell_render.rs:104-125 handles INVERSE, BOLD, ITALIC, ALL_UNDERLINES, DIM, STRIKEOUT, HIDDEN. GUI: crates/gui/src/shell_render.rs:143-152 reads only HIDDEN (to blank the char) and BOLD; CellInfo (gui/shell_render.rs:99-105) has exactly four fields fg, bg, ch, bold (corroborated by docs/AUDIT_METRICS.json: max_struct_name: "CellInfo", max_struct_fields: 4). INVERSE is handled in the GUI too (fg/bg swap at :127-129), so the drop set is exactly ITALIC/UNDERLINE/DIM/STRIKEOUT — four attributes, as claimed. The draw calls hardcode italic=false: gui/shell_render.rs:245-253 canvas.draw_text_run(row, area_col + run_start, &run_buf, run_fg, run_bold, false, 1.0) and :262 canvas.draw_char(row, area_col + col_idx, ch, fg, bold, false, 1.0). Not a Skia limitation: draw_text_run (canvas.rs:1107), draw_underline_at_y (canvas.rs:742) and draw_strikethrough_at_y (canvas.rs:905) all exist and are used by the GUI buffer renderer.

---

3. Exited-shell buffer/window teardown is copy-pasted between the event path and the health-check path, and the copies have already drifted

duplication · low

Impact. The file's own caution comment says this ordering has already caused five-plus bugs; keeping two hand-synced copies of it guarantees the sixth. The drift is already observable: a shell whose child dies without an Exit event (the exact case health_check exists for) tears the buffer down with no Terminal exited — buffer closed status, so the buffer and its window vanish with no explanation.

Evidence

crates/mae/src/shell_lifecycle.rs:236-299 (ChildExit path) and :413-462 (health-check zombie path) are the same ~45-line sequence: same orphan_ids collection, same focused-window retarget to vi.alternate_buffer_idx, same window_mgr.close(win_id) for unfocused orphans, same last-window fallback, same buffers.remove(buf_idx) + notify_buffer_removed + index-shift loop, same sync_mode_to_buffer(). Only difference: the event path also does editor.set_status(label) (:245-249, :296) — the health-check path silently drops the buffer with no message. The file carries // @ai-caution: [shell-lifecycle] Agent shell window placement, orphan cleanup, and hook ordering have had 5+ bug fixes. Shell exit must: close window, sync mode, fire hooks IN THAT ORDER. (shell_lifecycle.rs:1-4) — i.e. an invariant that must now be maintained in two places. manage_shell_lifecycle is 190 lines (docs/AUDIT_METRICS.json, max_fn_lines: 190), well over the 80-line function ceiling.

Verification

Diffed the two blocks myself. crates/mae/src/shell_lifecycle.rs:236-299 (ChildExit) and :413-462 (health-check zombie) are the same sequence: ShellInsert->Normal mode reset, shell_terminals.remove(&buf_idx) + shutdown(), identical orphan_ids collection via iter_windows().filter(|w| w.buffer_idx == buf_idx), identical focused-window retarget to vi.alternate_buffer_idx.unwrap_or(0), identical window_mgr.close(win_id) for unfocused orphans with the same last-window fallback, identical buffers.remove(buf_idx) + notify_buffer_removed(buf_idx) + index-shift loop, and sync_mode_to_buffer(). The only difference is the event path's let label = ... / editor.set_status(label) (:245-249, :296), absent from the health-check path — so the claimed drift (a zombie shell's buffer and window vanish with no "Terminal exited — buffer closed" message) is real and observable. The file header comment is verbatim as quoted (shell_lifecycle.rs:1-4, // @ai-caution: [shell-lifecycle] ... Shell exit must: close window, sync mode, fire hooks IN THAT ORDER.), and docs/AUDIT_METRICS.json confirms max_fn_name: "manage_shell_lifecycle", max_fn_lines: 190 in a 480-line file.

---

4. Shell redraw tick is a hardcoded magic number that differs between backends, contradicting its own written rationale

missing-config · low

Impact. The GUI — the backend that actually costs CPU to rasterise — runs the shell repaint 50% more often than the TUI, while the TUI carries the comment justifying 20 fps as the idle-CPU-conscious choice. A user on a laptop who wants to trade smoothness for battery, or a user on a fast machine who wants 60 fps terminal output, has no :set for it; the two numbers can (and already did) drift apart independently.

Evidence

TUI, crates/mae/src/terminal_loop.rs:405-413:

let shell_tick = async {
    if has_shells {
        // 20fps for shell viewport refresh — smooth enough for terminal
        // output while keeping idle CPU reasonable (~40% less than 30fps).
        tokio::time::sleep(std::time::Duration::from_millis(50)).await;

GUI, crates/mae/src/gui_app.rs:216:

let mut shell_interval = tokio::time::interval(Duration::from_millis(33));

Neither value is registered in crates/core/src/options.rs; grep -n 'shell' crates/core/src/options.rs yields only shell_split_ratio, ai_agent_login_shell, babel_inherit_shell_env and unrelated strings. The consumers of the tick are otherwise identical generation-diff loops (terminal_loop.rs:655-664 and gui_app.rs:1039-1052).

Verification

Both numbers verified verbatim. crates/mae/src/terminal_loop.rs:406-411: let shell_tick = async { if has_shells { // 20fps for shell viewport refresh — smooth enough for terminal / output while keeping idle CPU reasonable (~40% less than 30fps). tokio::time::sleep(std::time::Duration::from_millis(50)).await;. crates/mae/src/gui_app.rs:216: let mut shell_interval = tokio::time::interval(Duration::from_millis(33)); with no comment. I ran grep -n 'shell' crates/core/src/options.rs myself: the only matches are ai_editor's description text (:170), ai_agent_login_shell (:172), shell_split_ratio (:357) and babel_inherit_shell_env (:413) — neither tick is registered.

Scope correction from verification: Correct at low but reframe: per the brief, a constant with a written rationale is not a principle-#7 violation, and the TUI's 50 ms has one. The actual defect is the GUI's undocumented 33 ms, which contradicts the TUI's stated idle-CPU rationale on the backend that costs more to rasterise — a principle-#8 divergence between two copies of the same loop, not necessarily a missing option. Fixing it by making the two agree (with the rationale carried across) is sufficient; promoting it to an OptionRegistry entry is optional.

---

5. No test exercises shell rendering, selection extraction, or scrolled-back grid mapping in either backend

test-gap · low

Impact. This is the gap that lets finding #1 (selection ignores display_offset) and finding #2 (GUI drops four cell attributes) both ship green: a single round-trip test — write a known pattern to the PTY, scroll up N rows, select rows [a,b], assert the extracted text equals the rendered rows — would have caught #1, and a shared assertion that both backends map the same CellFlags set would have caught #2. Per principle #14 the missing tests are exactly the falsifying ones (scrolled state, attribute matrix), not more happy-path spawn smoke tests.

Evidence

crates/renderer/src/shell_render.rs has test_count: 0 (docs/AUDIT_METRICS.json) across 269 lines; crates/gui/src/shell_render.rs has 5 tests, all of which only assert RGB values from the pure named_color_to_skia / resolve_named_from_theme helpers (gui/shell_render.rs:464-524) — nothing touches render_shell_grid, the display-offset mapping, the selection overlay, or CellFlags. crates/shell/src/terminal.rs's 5 tests (spawn_and_read_grid, resize_terminal, spawn_command_with_login_wrap_sources_bashrc, spawn_command_without_login_wrap_does_not_source_bashrc, child_exit_detected, terminal.rs:712-906) are linear happy-path PTY smoke tests with fixed sleep+poll loops; none of start_selection/update_selection/finish_selection, scroll_display, read_line, write_paste, set_theme_colors or resize-while-scrolled has any coverage. crates/core/src/editor/tests/shell_tests.rs (830 lines, 40 tests) covers window placement and buffer lifecycle only — it never constructs a ShellTerminal.

Verification

Every count checks out against docs/AUDIT_METRICS.json and the source. crates/renderer/src/shell_render.rs: lines: 269, test_count: 0. crates/gui/src/shell_render.rs: test_count: 5, and the five are named_color_black, named_color_bright_white, named_color_red, background_resolves_base03_for_solarized, black_falls_back_to_ui_background_style (gui/shell_render.rs:464-524) — all pure color-helper assertions, none touching render_shell_grid, the display-offset mapping, the selection overlay or CellFlags. crates/shell/src/terminal.rs: test_count: 5, named exactly as claimed (spawn_and_read_grid:713, resize_terminal:742, spawn_command_with_login_wrap_sources_bashrc:780, spawn_command_without_login_wrap_does_not_source_bashrc:841, child_exit_detected:886), with no coverage of start/update/finish_selection, scroll_display, read_line, write_paste, set_theme_colors or resize-while-scrolled. crates/core/src/editor/tests/shell_tests.rs is 830 lines with 40 #[test] attributes and zero occurrences of ShellTerminal. The causal link to findings #1 and #2 is sound.

---

From the pre-v0.15 codebase audit (epic #592). Traced end-to-end across crates, then independently re-verified by a reviewer briefed to refute it; severity is the verifier's corrected value. In this batch, 8 of 11 claimed-high were downgraded.

[cuttlefisch]

Part of the pre-v0.15 codebase audit — tracking epic #592.