Skip to content

feat(tui): add configurable external status line (review fixes) - #330

Open
Nick768 wants to merge 8 commits into
Kuberwastaken:mainfrom
Nick768:main
Open

feat(tui): add configurable external status line (review fixes)#330
Nick768 wants to merge 8 commits into
Kuberwastaken:mainfrom
Nick768:main

Conversation

@Nick768

@Nick768 Nick768 commented Jul 26, 2026

Copy link
Copy Markdown

No description provided.

nico added 8 commits July 26, 2026 15:07
Add a statusLine feature (analogous to Claude Code's statusLine):
- Config in settings.json: "statusLine": { "command": "...", "pollIntervalMs": 5000, "padding": 1 }
- Backward compat: CLAUDE_STATUS_COMMAND env var still works (500ms poll)
- Session data (model, context, cost) is passed as JSON on stdin
- Rendered as a separate row between prompt input and footer
- ANSI escape sequences stripped for plain-text display
- /cc-statusline command shows current configuration
- #[serde(default)] on Config struct to allow partial config deserialization
- Tests for StatusLineConfig deserialization
- status_line in merge() now always uses base (global) config,
  never over (project), preventing RCE via project .claurst/settings.json
- remove unrelated shell.nix from feature PR
- add regression test project_cannot_override_global_status_line
…ately (PR Kuberwastaken#327)

- clamp poll_interval_ms to 1000-300000
- timeout kills hung child processes
- CancellationToken stops the task on app exit
- output capped at 4 KB
- initial empty session payload so status script
  runs on first poll
- wait 500ms before first poll so session data is ready
- sleep after command execution, so the status line
  appears immediately when the app starts
- fix pre-existing settings_screen test bound (20 -> 21)
Child stderr was inherited while ratatui owns the terminal,
so command failures could corrupt the TUI. Pipe stderr to
null instead.
Clamp padding to half the available width, use saturating
arithmetic, and ensure at least 1 character width to prevent
u16 overflow or invalid rect creation.
- total_input_tokens now includes cache tokens (Claude Code compat)
- total_output_tokens uses cost_tracker.output_tokens()
- added current_usage breakdown (input, output, cache_creation, cache_read)
- session_id uses the real tool_ctx.session_id UUID
…astaken#327)

- replace naive SGR-only ANSI stripper with proper CSI/OSC/C0 handler
- add maxLines config field (default 10) to limit status line height
- compute status line height dynamically from actual line count
- truncate over-long lines with ellipsis instead of clipping
- add tests for max_lines deserialization
@Nick768

Nick768 commented Jul 26, 2026

Copy link
Copy Markdown
Author

Thank you for the thorough review on #327! All requested changes have been addressed in this new PR (#330).

Changes per review point:

  1. Security (RCE via project settings): status_line is now global-only — project .claurst/settings.json can never override it. Added regression test project_cannot_override_global_status_line.

  2. pollIntervalMs / lifecycle: Interval clamped to 1000–300000ms. Child processes have a timeout and are killed when exceeded. Output capped at 4 KB. A CancellationToken stops the poll task on app exit. First poll runs after 500ms (so session data is populated), then sleeps between iterations.

  3. stderr: Piped to Stdio::null() to prevent TUI corruption.

  4. Padding overflow: Clamped to half the available width, using saturating arithmetic throughout.

  5. Session payload: Now uses the real session_id UUID, total_input_tokens includes cache tokens (matching Claude Code's format), total_output_tokens is properly split, and current_usage provides the full breakdown.

  6. ANSI stripping: Replaced the naive SGR-only stripper with proper CSI/OSC/C0 handling. Added maxLines config (default 10) for multiline output. Long lines are truncated with ... instead of clipping.

  7. shell.nix removed from this feature PR.

All existing tests pass (505 core, 658 TUI). Happy to iterate further if anything needs adjustment.

@Kuberwastaken Kuberwastaken left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough rework — the trust issue from #327 is properly closed (status_line only comes from base in Settings::merge, project files never reach it; env fallback is safe since project env is never applied to the process). Hardening (clamp, timeout+kill, stderr null, 4 KiB cap, control stripping, cancel token) all checks out and tests pass.

A few things before merge:

  1. clippy -D warnings fails: render.rs:2938 explicit_counter_loop in truncate_with_ellipsis (use char_indices()/enumerate()), app.rs:7336 unnecessary_map_or (test).
  2. Output capture can deadlock: child.wait() is raced against the 5s timeout but stdout is only read after wait returns (main.rs:2054-2069). A command that writes >64 KiB blocks on the pipe, gets killed every poll, and the line never updates. Read stdout concurrently (wait_with_output() under the timeout, or copy into a take(4096) reader).
  3. #[serde(default)] on Config is a silent schema change — on main a partial "config": {...} fails with missing field permission_mode; this PR makes every field optional. Probably desirable, but call it out in the PR and add a partial-config parse test (or split it out).
  4. Three Mutex::lock().unwrap() in prod paths (main.rs:2012, 2028, 3731) — use ParkingMutex (already imported in that file) or handle poisoning.
  5. entries.len() <= 2021 in settings_screen.rs:840 — if the PR adds a settings entry say so, otherwise revert. cc-statusline is intercepted but not in the slash-command list/help; a statusLine entry in docs/configuration.md is missing. Narrow-terminal/large-padding rendering has no test.

1–4 blocking, 5 can be a follow-up if you prefer. Then lgtm.

@Nick768

Nick768 commented Aug 23, 2026

Copy link
Copy Markdown
Author

Thanks for the review, I am looking into it, but give me some time

@Kuberwastaken

Copy link
Copy Markdown
Owner

No worries, take your time :) thank you

@Kuberwastaken

Copy link
Copy Markdown
Owner

Following up on my review above before you push the next round — I found one more thing while re-reading, and it's the most important item on the list, so I'd rather you fix it in the same pass than discover it in round three. Sorry for the extra scope.

#[serde(default)] on Config is a security change, not just a schema convenience.

Right now Config has 18 fields with no serde default (permission_mode, theme, output_format, mcp_servers, allowed_tools, env, custom_system_prompt, …), and find_project_settings (core/src/lib.rs:1883-1911) throws away the entire project settings file if deserialization fails anywhere. That means a repo shipping a partial "config": { … } block is currently ignored outright — which, entirely by accident, is the only thing standing between us and the repro in #389:

{ "config": { "hooks": { "UserPromptSubmit": [ { "command": "" } ] } } }

With #[serde(default)] that file parses, and hooks: merge_map(base, over) merges it with the project winning. Same for skills.urls (#389's second sink), mcp_servers, and env. So this PR would close the statusLine trust hole and simultaneously make an open, unpatched security issue much easier to hit.

There's a second edge to it: permission_mode: over.config.permission_mode (core/src/lib.rs:1940) is unconditional. Once partial configs parse, a project config block that merely omits permissionMode silently resets the user's global mode, and a hostile one sets "permissionMode": "bypassPermissions" in three lines.

What I'd like: drop #[serde(default)] from this PR. If statusLine needs partial configs to be ergonomic, put #[serde(default)] on the individual fields you actually need, or land the schema loosening as its own PR where we can review the trust implications on their own — it needs permission_mode/theme/output_format moved to the base. guard pattern we already use for trust_project_mcp_servers and skip_dangerous_mode_permission_prompt first.

Two corrections to my earlier review while I'm here:

  • Point 5, the 2021 settings bound: you were right, ignore me. all_entries is 18 base entries plus 3 when file_injection_enabled, and SettingsScreen::new() reads the user's real ~/.claurst/settings.json via load_sync() — so it's 18 in CI (no settings file) and 21 on any machine that has one. That's a test-isolation bug on our side, not yours. Please pull that line out of this PR and I'll fix the test properly.
  • Point 2, the deadlock: worth noting stdin.write_all().await also runs before the timeout is armed, so a child that never reads stdin can hang the task outright. wait_with_output() under the timeout fixes both halves — right now >64 KiB of output fills the pipe while child.wait() is being raced against the timeout, so the child blocks and gets killed every poll.

One new perf note: the session-JSON block sits inside the main loop, which polls at 16 ms (cli/src/main.rs:2240), so we serialize a fresh payload ~60×/s for a consumer that reads it every 5 s. Same for strip_ansi, which runs twice per frame (height calc + render) plus truncate_with_ellipsis. Strip and truncate once when the text arrives in the drain and stash the clean string on App; build the JSON inside the poll task or only when the counters actually change.

Also two clippy heads-ups for when CI runs (fork PRs need approval here — I'll approve on your next push): unnecessary_map_or in the new test and explicit_counter_loop in truncate_with_ellipsis will both fail the -D warnings gate.

To be clear about where this stands: the trust fix from #327 is correct and the hardening work is good — I'm not asking you to redo any of it. It's the one serde(default) line plus the stdout read that are holding this up.

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.

3 participants