feat(slash): client-side command surface (/help, /diff, /status, /config)#55
Merged
Conversation
Surveys Claude Code, OpenAI Codex, and opencode for registry shape, parser, dispatch, autocomplete, output, and custom-command discovery. Locks in the v1 design choices (trait registry, two-column popup, SystemMessageBlock for output, runtime-only mutations to keep NixOS read-only configs honoured).
Adds a `slash` module with a trait registry (`SlashCommand`), prefix-aware parser, and one built-in (`/help`). `App::dispatch_user_action` intercepts `UserAction::SubmitPrompt`: matched names dispatch locally and never reach the agent loop, so slash commands don't pollute conversation history or token accounting. Output lands in a new `SystemMessageBlock` with a `▎` left-bar in `accent` so command results read as distinct from agent prose; unknown names render through the existing `ErrorBlock`. Aliases route to a single canonical impl and display inline as `/name (alias, alias)` — one popup row per command, not one per name. The trait shape (`name`, `aliases`, `description`, `usage`, `execute`) mirrors `tool::Tool`; one file per command, registered through a `&[&dyn SlashCommand]` slice — no central match arm. The parser already accepts `:` in names so a future plugin-namespace layer (`/plugin:cmd`) doesn't need a rewrite.
`_ =` already discards; the `let` adds nothing. Matches the rest of the crate, which uses `_ =` at write-macro call sites.
Shells out to `git diff HEAD` (falling back to `git diff --cached` in a fresh repo with no commits) and appends untracked filenames, rendered as a `SystemMessageBlock`. Output is capped at 64 KB on a UTF-8 boundary with a footer noting how many bytes were dropped, so a binary diff or generated-file dump can't lock the render loop. Pure-display command — no app-state plumbing; lives entirely behind the existing `SlashContext` shape.
Plumbs a frozen `SessionInfo` snapshot (model, cwd, version, auth method, session id) from `run_tui` into the slash dispatcher, then adds `/status` as the first reader. The snapshot is built once at TUI startup so commands stay sync and read-only — no shared cell to keep in sync with the StatusBar. Drive-by: tighten the gutter-alignment tests in `/help` and `/status` to locate descriptions / values by substring rather than scanning for double-spaces. The earlier probe accidentally landed on the padding spaces when keys differed in length, hiding the bug behind the single-row registry that existed before this commit.
Adds a read-only `/config` that prints the effective values
(`model`, `model id`, `base url`, `auth`, `effort`, `max tokens`,
`prompt cache ttl`, `show thinking`) plus the layered TOML files
those values were assembled from. Path discovery happens at execute
time so a file the user just edited shows up immediately; missing
files render an explicit `(absent)` so "is my project ox.toml being
picked up?" is answerable without `strace`.
This command never writes — slash commands stay session-local by
design (see docs/research/design/slash-commands.md). The auth secret
itself is never surfaced; only the method label ("API key" / "OAuth")
threads through `ConfigSnapshot`.
Mechanism: a new `ConfigSnapshot` (in config.rs) captures the
secret-free resolved view before `Client::new` consumes the full
`Config`. `SessionInfo` embeds it, so `/status` and `/config` share
one read path. `config::file::user_config_path` /
`find_project_config` are bumped to `pub(crate)` so the slash layer
can re-discover paths without a back-channel.
Mirror the four-command slash surface that just shipped: add `slash.rs`
+ `slash/{config,context,diff,help,parser,registry,status}.rs` plus
`tui/components/chat/blocks/system.rs` to the crate tree, and rewrite
roadmap's slash section into a Working Today entry covering what's
landed and a Current Focus continuation listing what's still ahead.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bundles the high-leverage findings from PR #55's review pass: - `SlashCommand::execute` now returns `Result<(), String>`. The dispatcher renders the single `ErrorBlock` per failure as `"/name: message"`; commands no longer touch `push_error`. One error-render policy, not N — and matches `tool::Tool`'s shape. - Unknown-command error now lists every registered `/name` and mentions the `//foo` literal-text escape so the user can discover the surface and send literal `/etc/hosts`-style prompts. Pinned by a new dispatch test asserting both. - New `slash/format.rs::write_kv_table` is the single home for `key value` table rendering. `/help`, `/status`, `/config` all adopt it; the three near-identical inline `writeln!`-with-padding loops are gone. - Heading shape unified across `/help`, `/status`, `/config`: `Heading\n\n` (no trailing colon, blank-line separator). `/config`'s second-section heading rewrote `Loaded from` → `Sources` to read as a noun phrase rather than a sentence fragment. - `/config` rewrote `show thinking true|false` → `thinking shown|hidden` (verb-phrase next to a bool was confusing) and `(model default)` → `(default)` (matches CLI conventions). - `/diff` truncation footer renders the dropped size in human- readable units (`12.4 KB more`, `1.5 MB more`) instead of raw bytes. Integer arithmetic — no `as f64` precision-loss cast. - `/diff` empty-tree response is now `Working tree clean.` (matches `git status`'s phrasing) instead of `No uncommitted changes.` which was ambiguous about whether untracked files counted. - `/diff` switched to `anyhow::Result` internally with `Context` on git invocations — matches the project-wide error convention. - IO functions (`collect_diff_in`, `inside_git_repo`, `has_head`, `run_git_in`) now take `cwd: &Path` instead of reading process state, so tests can run against a tempdir-rooted git repo without racing parallel tests on `set_current_dir`. New tests cover every IO branch — fresh repo, post-commit edits, untracked listing, outside-repo error, stderr propagation. `slash/diff.rs` line coverage 62% → 91%. - New `ChatBlock::error_text(&self) -> Option<&str>` test-only accessor + `ChatView::last_error_text()` so dispatch tests can assert on rendered wording instead of grepping the rendered glyph stream. - `Help` / `Status` / `Config` / `Diff` unit structs renamed to `HelpCmd` / `StatusCmd` / `ConfigCmd` / `DiffCmd` so they no longer shadow `tui::components::status::Status`, `crate::config::Config`, and the like — a future grep for "where is `Config`" stays unambiguous. `display_label` dropped to private (was over-exposed for a popup that's deferred). - Fixed a rustdoc bug: `Config::snapshot`'s doc-comment was glued onto `Config::load`'s, leaving `load` undocumented in `cargo doc`. - `render_help_aligns_descriptions_to_a_shared_gutter` and the `format::write_kv_table` test pin the actual gutter column the renderer agreed on, not just "all rows match each other" — a uniformly broken renderer would slip past the latter. Two `slash/` files reach 100% line coverage (`status.rs`, `config.rs`); `parser.rs`, `context.rs`, `format.rs` were already near-complete. Project total stays at 98.24% line.
- CLAUDE.md grows a `Discarding Results` section documenting the bare `_ =` form (vs. `let _ =`) for `writeln!`/`write!` infallible results — the rest of the crate already uses it; this just makes the convention explicit so future tool calls don't reintroduce `let _ =` and need a follow-up cleanup commit. - Roadmap's slash-commands section now spells out *why* `/model` and `/theme` mutations are session-local and what the explicit- subcommand persistence path would look like (rejecting Claude Code's `~/.claude.json` silent-merge pattern). Pre-empts the inevitable "how do I make my model choice persist?" question with a documented design intent rather than re-litigating each time. - `writeln` added to cspell dictionary for the new prose mention.
Long /config Sources paths and oversized /diff lines clipped at the viewport edge because the system block emitted one ratatui Line per input line without wrapping — every other text-bearing block routes through `wrap_line`. Wrap each input line at `ctx.width` with a continuation prefix that re-emits the styled bar, mirroring the `tool::bordered_row` shape so the block still reads as one visual unit on narrow terminals.
Five small but related corrections, all scoped to the `/diff` command: - Truncate now caps strictly at MAX_BYTES instead of overshooting by up to 3 bytes when the boundary lands inside a multi-byte char. The take_while predicate previously accepted any char *starting* below the cap; the kept prefix could exceed the limit. Strengthen the truncate test to pin the exact kept-bytes count, and rename the multi-byte test to spell out the strict-≤ contract. - Add a `format_size(MB - 1)` boundary case so the KB→MB rollover is pinned: an off-by-one in the `<` comparison would now fail visibly. Rename the test to `_truncates_to_one_decimal` so the name matches the implementation (integer truncation, not rounding). - Empty-tree message moves from "Working tree clean." to "No uncommitted changes." — sticks to git's vocabulary and avoids the ambiguity a reader might infer when untracked files are present. - Trim two over-engineered doc-comments (`format_size`'s "modulo trick avoids precision loss" claim is misleading — the real reason is determinism; `MAX_BYTES`'s "~1300 lines at 50 chars" is imprecise compared to the bare 64 KB). - Mirror test section dividers to production order (`collect_diff_in → format_diff → inside_git_repo → has_head → run_git_in → truncate → format_size`) and lift the shared `git_setup` / `fresh_repo` helpers + `tempfile` imports above the first divider per CLAUDE.md's section-order rule.
The previous `dispatch_command_failure_renders_error_block_prefixed_with_name`
test inlined the dispatcher's tail (`if let Err { push_error }`)
instead of calling `dispatch` — a regression that dropped the wrap
entirely would still pass. Three reviewers flagged the tautology
independently. Likewise `lookup`'s alias branch (`name == n ||
aliases.contains`) was dead in the live registry today (no built-in
carries an alias), so flipping the OR to AND would survive.
Extract `dispatch_with(commands, parsed, ctx)` and `lookup_in(slice,
name)` so tests can drive the real production code against a
synthetic registry. The public `dispatch` becomes a one-liner over
`BUILT_INS`. The unknown-command hint's comma-joined list moves to
`format_available`, which writes directly into a single `String`
instead of allocating a `Vec<String>` just to call `join`.
New tests pin three branches that were previously uncovered or
papered over: alias resolution to the canonical impl, the
dispatcher's `/{name}: {msg}` error wrapping (now exercised against
the real `dispatch_with`), and the App-level slash path
(`apply_action_locally` → `parse_slash` → `dispatch` → chat) — the
last one would catch a fall-through where a slash command leaks to
`user_tx.try_send`.
Tighten `BUILT_INS` / `SlashCommand` / `lookup_in` to `pub(super)`;
they're slash-internal. The unused `slash::lookup` re-export drops
with the visibility tightening.
Three commands all rendered "Heading\n\n" + write_kv_table; with three call sites the pattern is real (per CLAUDE.md "Extract after duplication appears"). `write_kv_section` owns the heading + blank- separator + table sequence, and inserts an extra blank between sections when called twice in sequence — the shape `/config` uses for its `Resolved config` / `Source files` pair. Three callers collapse to one shared seam. Drops a speculative migration note about `unicode_width` (no multi-width keys exist; the comment violated CLAUDE.md's "scope discipline" rule) and the inaccurate "every read-only command" phrasing (`/diff` is read-only too but renders raw text, not a kv table). Pin the section-stacking contract with a dedicated test.
Polish the four shipped commands so they read as a parallel family.
A fresh-user usability review surfaced inconsistencies and ambiguous
copy across description strings, section headings, field labels, and
path markers; this commit folds each fix into the renderer that owns
that surface.
Descriptions: all four commands now use a "show" verb with no
parenthetical hedges. Earlier prose mixed "list" vs. "show", and two
commands carried trailing parentheses (`(model, cwd, auth, ...)` for
/status, `(read-only)` for /config) that either previewed details
better belonged in the body or hedged a behavior that's already the
default.
Headings: `Status` → `Session status`; `Config (resolved)` →
`Resolved config`; `Sources` → `Source files`. The adjective-noun
shape mirrors `Available commands` so the four commands read
consistently. Adopt `write_kv_section` so the heading + blank +
table sequence lives in one place per the previous commit.
Field labels: rename `session` → `session id` to match `/config`'s
two-word convention (`model id`, `prompt cache ttl`). Add a
`model id` row to `/status` so a user debugging routing sees both
the marketing name and the API id at a glance — same pair `/config`
already shows.
`/config` rendering:
- `(default)` for missing effort → `(model default)` so "default
what?" doesn't linger.
- `thinking shown/hidden` row → `show thinking yes/no` mirroring the
toml key directly; the old wording read as a verb-phrase rather
than a setting.
- Path markers: `(none)` → `(not configured)`, `(absent)` → `(not
found)`. The pretty-printed path is always shown (when known) so
the user can see *where* /config looked even when the file is
missing — answers "why isn't my project ox.toml being picked up?"
without source-reading.
`/help` gains a one-line tip footer teaching the `//foo` literal-
slash escape; previously the only path to discovering it was typing
an unknown command, a worst-case onboarding trap.
Drop function-local `use crate::slash::{SlashContext, test_session_info}`
duplicates from the test modules — both items already arrive via
`super::*` and the explicit `crate::slash::test_session_info` import.
Strengthen the test suite to pin the new wording and to count
rendered rows so an accidental row drop fails before any value
substring check; pin column offsets numerically (not just "all rows
agree") so a uniformly-broken renderer can't pass.
Tree drifted as `slash/format.rs` landed in the polish pass — the shared `write_kv_section` / `write_kv_table` renderer was missing from the diagram entirely. Also correct two stale per-row blurbs: `slash.rs` claimed to host `SessionInfo` (it lives in `context.rs`, re-exported), and `slash/status.rs`'s row didn't reflect the new `model id` column. Per CLAUDE.md's own rule: "Crate structure diagrams must match the actual filesystem."
The PR added both a `label()` projection (used by `/status` / `/config` to name the auth source without leaking the secret) and a `snapshot()` constructor (the secret-free read-only view threaded through `SessionInfo`). Neither had a direct test — both branches of `label` and every field of `snapshot` were dead in coverage even though the slash tests exercised them implicitly via the snapshot fixture. Pin both: assert each `label` arm returns its exact wording and that `snapshot` copies every user-facing field (including `effort`, `prompt_cache_ttl`, and `show_thinking`) while dropping the auth secret.
`DiffCmd::execute` read process cwd then delegated to `collect_diff_in`, leaving the dispatch arm (the empty-trim guard, the body push, the trait-boundary `String` error wrapping) outside any test. Pursuing the wrapper directly would race other parallel tests on `current_dir`, so split it: `execute` keeps the `current_dir` read, and a new `execute_in(cwd, ctx)` takes the directory as data. Three tests drive `execute_in` against tempdirs — clean repo (empty diff renders the friendly marker), dirty repo (body lands as a SystemMessageBlock with no error), and outside-a-repo (the "not inside a git repository" wording reaches the trait error wrapper). The 4-line outer `execute` wrapper stays uncovered; covering it would require process-state mutation that doesn't buy more than the seam already does.
…_label usage Three coverage gaps cluster around test scaffolding the patch introduced: - Synthetic `SlashCommand` fixtures (`Failing`, `Fake`, `AliasedCmd`) carry required-but-unused `description` / `execute` slots so the trait surface satisfies the compiler. Each gap closes with one fixture-sanity test that asserts the stub's contract (name, aliases, description, execute return) — pins what later tests implicitly assume about the fixture and turns the stub bodies from dead lines into documented invariants. - `display_label`'s `usage()` branch had no live exerciser (no built-in carries a usage hint today). Restructure the help Fake fixture into a struct with per-case constants so one test can drive both the no-alias-with-usage and both-present cases without a fresh struct per assertion. - `ChatBlock::error_text`'s test-only default returns `None` and is overridden by `ErrorBlock`. The slash dispatch tests only reached it via `last_error_text()` on `ErrorBlock`, never on a non-error block; extend `dispatch_known_command_runs_and_does_not_push_error` to assert `last_error_text() == None` so the default executes. Remaining uncovered lines (`assert!` failure-message expansion in registry/system tests; `bail!` for git-failure-with-empty-stderr; the 4-line `DiffCmd::execute` cwd wrapper) are defensive paths that should not execute in normal runs — covering them would mean either writing failing assertions or racing process state.
Lowercase labels like `model id`, `cwd`, `session id`, `base url`, `prompt cache ttl`, `show thinking` read as variable names rather than display labels. Acronyms in particular (ID, CWD, URL, TTL) deserve their conventional capitalization. Bump every row label in `/help`, `/status`, `/config` to Title Case with proper acronyms — `Model`, `Model ID`, `CWD`, `Session ID`, `Base URL`, `Max Tokens`, `Prompt Cache TTL`, `Show Thinking`. Section headings move with them: `Available Commands`, `Session Status`, `Resolved Config`, `Source Files`. Gutter widths happen to be unchanged (longest labels were the same length pre/post case flip), so the alignment tests' pinned column offsets still hold.
…ool reuse Both row primitives lived at `pub(super)` inside `blocks/tool/`, visible only to tool-result body renderers. They're now general shared scaffolding — the slash `/diff` body wants the same red / green row bg + line-number gutter — so widen visibility to `pub(in super::super)` (visible across `blocks/`, not crate-wide). No behavior change. Doc comments updated to name the wider audience and explain why the files stay in `blocks/tool/` for now (physical move can happen organically when more block types reach in).
…hetic Replace the plain SystemMessageBlock dump for `/diff` with a parsed GitDiffBlock that mirrors the Edit-tool diff body: - Red row bg on `-` lines, green on `+`, dim text for context. - Line numbers in a left gutter, parsed from `@@ -A,B +C,D @@` hunk headers and walked per-row. - File path bold; `diff --git` / `index` / `--- a/...` / `+++ b/...` metadata suppressed since the path header already names the file. - Untracked-files heading + paths and the truncation footer render as plain bordered rows outside any hunk. Reuses `numbered_row::Renderer::with_style` and `bordered_row::render` under the hood — same visual conventions as `tool::diff`, no parallel renderer. Empty-tree path keeps the friendly SystemMessage marker; only the non-empty branch flips to GitDiffBlock so the `No uncommitted changes.` line stays consistent with the rest of the slash output. CLAUDE.md crate tree picks up `blocks/git_diff.rs`.
Three real gaps closed in addition to a regression discovered on the
context-line branch test.
- `render_emits_path_header_then_hunk_then_body_rows` was using
`\<newline>` line continuation, which strips leading whitespace on
the next line — so ` context\n` became `context\n` and the assertion
silently exercised the defensive fallback instead of the
`strip_prefix(' ')` branch it was meant to cover. Switched to
`indoc!`, which preserves the leading space.
- Added a dedicated test for the defensive fallback (corrupt hunk-body
lines without `+`/`-`/space prefix), which was previously only hit
by accident through the broken context-line test.
- Added `block_kind_is_other` so the `BlockKind` slot is asserted
directly rather than via the live ChatView render path.
- Added `execute_forwards_process_cwd_through_execute_in` to cover the
3-line `DiffCmd::execute` wrapper around `current_dir()`.
Converted the remaining `\<newline>` strings in the test module to
`indoc!` for consistency with the rest of the test suite, and shortened
the `git_diff.rs` annotation in the CLAUDE.md crate tree to match peer
entries.
Patch coverage 17 → 7 missing lines; the remaining 7 are `assert!`
failure-message expansions (only hit on assertion failure) and one
defensive `bail!` for a git-fails-with-empty-stderr path that real
git does not produce.
…dded quotes
Two related test-fixture readability sweeps:
1. Multi-line `\n` string literals → `indoc!`. Same content either
way; indoc shows line-by-line structure at a glance and sidesteps
the `\<newline>` line-continuation footgun (strips leading
whitespace on continuation lines, silently dropping
leading-space-significant content).
2. Strings with embedded `\"` escapes → raw strings. `\"` inside
a `"..."` literal hides the actual character; `r#"...""#`
shows the quote unescaped.
Indoc closing-brace style:
- Trailing-newline form (string ends with `\n`): `"});` together on
its own line below content.
- No-trailing-newline form (`"` glued to last content line): `"`
stays with content; `}` (and trailing `,` / `;`) on its own line
below.
Files touched:
- `slash/diff.rs`: format_diff combined-separator and untracked-only
assertions.
- `tui/components/chat/blocks/git_diff.rs`: add / del / context /
untracked / defensive / truncation tests.
- `tui/components/chat/blocks/system.rs`: bar-prefix and
trailing-newline tests.
- `tui/components/chat/blocks/tool/numbered_row.rs`: `println!("x")`
fixture switched from `\"` escapes to raw string.
- `tui/theme/loader.rs`: error message containing the literal word
"reset" switched to raw.
- `tool/edit.rs`: three `std::fs::write` setup fixtures
(`A\nB\nC\nB\n`, `X\nY\nA\nX\nY\nB\nX\nY\nC\n`) and one
`build_diff_chunks` input (`x\nx\nx\nx\nfn foo()\n`).
- `tool/read.rs`: three `std::fs::write` fixtures.
- `tool/glob.rs`: embedded-blank-line path-list fixture.
- `tool/grep.rs`: skipped-warning structured fixture, the truncation
footer indoc, and the `println!("hi")` indoc + `text:` field —
switched to raw form. JSON-schema description with embedded
`"content" / "files_with_matches" / "count"` quotes also switched
to raw.
Skipped — keeping the inline form is more precise:
- Mixed `\r\n` / `\n` cases in `tool/edit.rs` (EOL-handling tests):
indoc can't preserve `\r` line endings without escapes.
- Tab-bearing strings in `tool/read.rs`: indoc strips leading
whitespace and would mangle the `\t` separator semantics.
- Single-line `\n\n` paragraph-boundary probes in
`tui/components/chat.rs`: the `\n\n` is a sentinel under test, not
multi-line content.
- 3-line trivial fixtures inside compact 4-arg calls (e.g.
`build_diff_chunks("A\nB\nC\n", "B", "X", 1)`): splitting one short
arg across 6 lines while the others sit on one each obscures the
call shape.
- `client/anthropic/wire.rs:444` JSON-inside-JSON test: the inner
`\"` is a JSON-on-the-wire escape, not a Rust escape, and is
preserved literally inside the outer raw string.
hakula139
force-pushed
the
feat/slash-commands
branch
from
3beb713 to
0f76158
Compare
`inside_git_repo` previously called `run_git_in` and used `is_ok_and`, collapsing two distinct failures — git not on PATH and "this dir is outside a work tree" — into the single fallback "not inside a git repository". On a system without git, that message sent the user looking for the wrong problem. Split the spawn check from the work-tree check: only "spawn failed" propagates as Err; a successful spawn whose status is non-zero or whose output isn't `true` returns `Ok(false)` and routes through the existing "not inside a git repository" bail.
`format_diff` previously called `trim_end()` on the tracked diff, which strips both the trailing newline git appends *and* any trailing whitespace on the last content line. A user staging an edit that legitimately added trailing spaces would see them silently disappear from `/diff` output. Strip only `'\n'` so the body remains byte-for-byte faithful, and gate the empty-skip on `trim().is_empty()` so whitespace-only input (unrealistic from real git, but pinned by an existing test) still collapses to an empty result.
A bare `io::Error` reaching the dispatcher's `/diff:` prefix lands
as `/diff: No such file or directory (os error 2)`, which reads as
a problem with `/diff` itself rather than a process-cwd failure.
Add a `.context()` layer so the chained `{e:#}` format surfaces the
intent: `/diff: failed to read current directory: <io error>`.
The cap gate is `if s.len() <= MAX_BYTES { return s; }` — exactly
`MAX_BYTES` is meant to pass through unchanged. The existing tests
covered short input (3 bytes) and oversized (`+100`), leaving the
boundary unverified. A mutation flipping `<=` to `<` would slip
past CI and start appending a footer to every full-cap diff.
`max_line_number_width_uses_largest_hunk_extent` previously used
two symmetric hunks (`-27,20 +27,20` and `-1000,3 +1000,3`). Two
load-bearing details survived as silent mutants:
- dropping the `.max()` in `parse_hunk_extents` (old/new equal)
- flipping `count.saturating_sub(1)` to `count` (1002 and 1003
both have width 4)
Tighten the integration test to a single asymmetric hunk
(`-1,1 +1,10`) so a missing `.max()` collapses the rendered gutter
to width 1, and add direct unit tests for `parse_hunk_extents` and
`parse_range_extent` so the building blocks fail visibly under any
future refactor — including count=1 and the omitted-count branch.
`render_walks_line_numbers_per_hunk_starts` exercises only `+`/`-` rows, leaving the context branch's `old_ln += 1; new_ln += 1` as a silent mutant target — dropping either increment would mis-number the surrounding marker rows. Add a fixture that interleaves `+ context + -` so the trailing `+` rides advanced `new_ln` and the trailing `-` rides advanced `old_ln`. One test catches both directions.
The escape only works because `slash::parse_slash` returns `None`
for `//foo` and `apply_action_locally` then falls through to the
normal forwarding branch. No app-level test verified that the
agent actually receives the prompt — a future change adding a
second prefix check could silently start swallowing it.
Add a test that dispatches `SubmitPrompt("//etc/hosts")` and
asserts the chat shows just the user message (no slash output),
input flips to streaming, and `user_rx` receives the literal
prompt unchanged.
Section headings on the slash surface ("Available Commands",
"Session Status", "Resolved Config", "Source Files") are Title
Case, but the per-command descriptions sat in lowercase as
sentence-fragment prose. The case asymmetry read as two-author
copy. Capitalize the first word of each so the help table parallels
its own headings.
"Show uncommitted git changes" reads narrower than what /diff actually does — the body also lists untracked files under their own heading, and a user thinking "uncommitted" only of staged-but-not- committed work might not recognize that signal. Reword as "Show working-tree changes (including untracked files)" so the help row matches both halves of the output.
"No uncommitted changes." technically excludes untracked files — "uncommitted" reads to many users as "tracked but not yet committed". Since /diff actually walks both tracked diff *and* untracked files, the existing marker undersold the all-clear. Match `git status`'s wording so the message uses vocabulary the user already shares with git, and accurately reflects what we checked.
`(truncated: 12.3 KB more)` told the user the cap kicked in but not how to see the rest. Add the actionable next step inline: the exact command (\`git diff HEAD\`) that produces the equivalent untruncated output. Update the footer assertion accordingly.
The /status surface otherwise spells out compound labels — "Model ID", "Session ID", "Auth" — leaving "CWD" the lone shell- jargon abbreviation in an otherwise prose-cased column. Expand it for parallelism. Adjust the gutter assertion: the longest label is now "Working Directory" (17), so values land at col 21.
Sweep PR-introduced comments for verbosity. Cut narration ("a future
refactor that ..."), restated mechanics ("$macro adds a trailing
$x"), and multi-line explanations that fit on one line. Keep the
WHY: invariants, hidden contracts, mutation rationale, surprising
test fixture choices.
Net 232 lines removed; behavior, tests, and lint all unchanged.
The two contract tests (`built_ins_aliases_do_not_collide_with_any_canonical_name` and `built_ins_have_non_empty_name_and_description`) used `assert!` format-args that llvm-cov flags as uncovered on the success path, since their inner loops never run when BUILT_INS satisfies the contract. Refactor each to a "collect violations → assert empty" pattern with a helper that runs unconditionally, then add synthetic positive-path tests (`ColliderCmd`, `EmptyDescCmd`) so the violation branches execute. Pin the synthetic fixtures' trait stubs through a shared `run_execute` helper so `description` and `execute` bodies don't stay dead code. Replace `unwrap_or_else(|| panic!(...))` with `.expect(...)` in the alias-resolution loop for the same lazy-eval reason. slash/registry.rs is now 100% region/function/line covered.
Three coverage gaps remained on the PR's diff/system surfaces, all caused by `assert!`-macro lazy-eval (format args evaluated only on panic) plus one genuine defensive branch: - system.rs (render_wraps_long_body_under_bar): pre-bind the bar prefix into a local so the format-arg expression evaluates unconditionally. - diff.rs `git_setup` test helper: pre-bind `stderr` into a local for the same reason. - diff.rs `run_git_in_propagates_stderr_on_failure`: pre-evaluate both alternatives of the `||` so a short-circuit on git versions that echo the SHA can't leave the second arm unexecuted. - diff.rs `truncate_oversized_input_appends_footer_with_human_size`: pre-bind the tail slice used in the failure message. For the production-code defensive branch in `run_git_in`, extract a pure `git_failure_message` helper and unit-test both arms (blank stderr → synthetic `git <args> failed`; populated stderr → trimmed pass-through). Add `run_git_in_wraps_spawn_failure_with_args_in_context` and `collect_diff_in_surfaces_spawn_failure_when_git_missing` that drive the `with_context` and `?`-propagation arms by clearing PATH via `temp_env::with_var` and asserting the spawn-failure message reaches the caller. Patch coverage: 100% on the lines this PR added. File totals: slash/diff.rs 99.69% (1 missed line is the DiffCmd::execute current_dir() defensive arm — closing it would require deleting the cwd mid-test); blocks/system.rs 100%.
The two `temp_env::with_var("PATH", "")` tests added for spawn-failure
coverage mutate global process env, which races with parallel tests
that spawn `git` directly outside of `temp_env`'s mutex. Drop them and
accept the ~5 missed regions on `run_git_in`'s `with_context` arm and
`collect_diff_in`'s `?` propagation as defensive coverage debt — closing
those without a global serialization layer would require a refactor to
take a Command-builder closure.
Reorder remaining tests per CLAUDE.md "happy → variants → edge / error":
- collect_diff_in section: outside-a-repo error moves to the end after
fresh_repo / after_commit / separates happy paths
- git_failure_message section: passes_through_trimmed_stderr (happy)
first, falls_back_to_synthetic (edge) second
… block CLAUDE.md is explicit: "super:: and crate:: paths belong together in the internal block — do not split them." Six files violated this with a blank-line break between the two prefix groups inside a single internal import block. Files: tui/markdown.rs and the test mods of bordered_row.rs, diff.rs, glob.rs, grep.rs, numbered_row.rs (all under tui/components/chat/blocks/tool/). No production-code change; rustfmt's other ordering choices (`super::Type` before `super::scoped::path`, bare ratatui types before ratatui::scoped::path, etc.) are deterministic and the codebase already follows them.
Per CLAUDE.md: "Within each section, order: happy path → variants → edge / error cases." Eight files had error-case tests sandwiched before the canonical happy path, which obscures what the function is *supposed* to do when reading the suite top-down. Reordered sections (one per file unless noted): - file_tracker.rs::check_stat — full_match_passes leads - session/actor.rs::run — flush_error moves to the end - session/handle.rs (3 sections) — actor_gone failure tests trail their happy paths in record_tool_metadata_batch, append_ai_title, finish - session/list_view.rs::render_sessions — populated rows precede the empty-input edge cases - session/resolver.rs::resolve_session — happy paths cluster, then the four error variants follow at the end - session/state.rs::finish_entries — after_record_returns_summary leads, pending_writer_returns_empty trails as edge case - slash.rs::dispatch — known_command_runs leads, the two unknown_* error tests follow - tui/components/input.rs::submit — submit_clears_textarea (happy) precedes submit_empty_produces_no_action (edge) No behavior change — pure reorder. All tests still pass.
CLAUDE.md requires test sections to mirror the production function order in the same file so a top-down read of the test module tracks the top-down read of the source. Three files diverged: - tool.rs: cap_output (prod L476, in "Output Cap") was tested after resolve_base_dir / display_path / file_name (prod L525-580, in "Path Utilities"). Moved cap_output's test section above them. - tool/edit.rs: parse_replacement_count (prod L424) and synthesize_chunk (prod L438) — both in "Result View", which sits after "Line Endings" — were tested out of place. Moved both test sections to follow apply_eol (prod L408), preserving prod order parse_replacement_count → synthesize_chunk. - tui/components/chat/blocks/tool/diff.rs: render (prod L53, the entry point) was the last test section, with three helpers tested before it. Reordered to match prod: render → render_locations_footer → any_chunk_has_content → chunk_anchor_line. No behavior change.
Trim a handful of newly-added doc/inline comments that drifted long: - slash/diff.rs::git_failure_message — collapse the rationale to one sentence; the input/output pair already documents the branch. - slash/diff.rs::run_git_in_propagates_stderr_on_failure — shorten the eager-`||` rationale. - slash/diff.rs::git_failure_message_falls_back_to_synthetic — drop the redundant restatement of what the function does. - slash/registry.rs::run_execute — single sentence is enough. - slash/registry.rs::alias_collisions / empty_metadata_offenders fixtures — drop the "what the helper reads" preamble; the call site is already concrete.
hakula139
added a commit
that referenced
this pull request
May 2, 2026
Adding `/clear` deep-dive material directly under `slash-commands.md` made the doc asymmetric — `/clear` got a full section while every other shipped command did not, and the file was set up to balloon as `/model`, `/resume`, `/compact`, `/init` each earned their own design notes. Restructure so per-command docs scale without bloating the surface design. - `slash-commands.md` → `slash-commands/README.md` — keeps the cross-command surface (registry, parser, popup, dispatch, cross-command design decisions). GitHub auto-renders README.md on directory navigation. - `slash-commands/clear.md` — new feature doc for `/clear`: Claude Code reference flow + mapping table, oxide-code's reset surface, design decisions specific to `/clear` (roll vs. truncate, send-first ordering, session-id-scoped title, etc.), and deferred behaviors gated on subsystems oxide-code doesn't yet ship. - Strip the trailing `## /clear` section, the journal-voice "PR #55 shipped" paragraph at the end of Design Decisions, and the intermediate `### Notable findings worth deferring` meta-narration from `README.md`. - `docs/research/README.md` — point the index entry at the directory. Future commands earn their own file under `slash-commands/` only when the design has depth worth a dedicated doc; simple read-only commands ride the surface decisions in `README.md`.
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
1 participant
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.
Summary
oxide-code now has a client-side slash-command surface. Submitted prompts that match a parsed
/cmdroute locally and never enterMessagehistory or token accounting; everything else still forwards to the agent loop. Four commands ship:/help,/status,/config(read-only),/diff(Edit-tool diff aesthetic). Remaining v1 surface (/clear,/init,/theme,/resume,/model, autocomplete popup) is deferred — seedocs/research/design/slash-commands.mdanddocs/roadmap.md.Design decisions
match-on-enum dispatcher. Mirrorstool::Tool; adding a new command is one file plus one slice entry.lookup_in/dispatch_withtake any registry slice so tests drive synthetic registries through the production path.App::apply_action_locally, not inInputArea. Input stays a dumb text widget; routing decisions live one layer up where they have access toChatViewandSessionInfo. Slash text never reachesMessage/ JSONL / token accounting./config, NixOS-friendly persistence stance. Resolved values + source file paths; never writes. Future runtime mutations (/model,/theme) will be session-local; persistence requires an explicit subcommand to an explicit path. Explicitly not repeating Claude Code's~/.claude.jsonmega-file pattern.ConfigSnapshotseparates the secret from the descriptor. Read-only view ofConfigbuilt beforeClient::newconsumes the full struct; the auth secret never leavesClient./diffreuses the Edit-tool diff renderer. Widenednumbered_row::Renderer/bordered_row::rendervisibility topub(in super::super)soblocks/git_diff.rscalls them directly — same red / green row backgrounds and line-number gutter, no parallel renderer.Changes
Slash module —
slash.rs,slash/{parser,registry,context,format,help,status,config,diff}.rsSlashCommandtrait +BUILT_INSslice + alias-awarelookup_in+dispatch_withtest seam.SlashContextborrows&mut ChatView+&SessionInfo;format::write_kv_sectionis the shared kv-table renderer.:and.in names for plugin-namespace forward-compat./diffextractsexecute_in(cwd, ctx)so tests run end-to-end against tempdir repos. Row labels are Title Case (Model ID,CWD,Session ID,Base URL,Prompt Cache TTL).git_failure_messagefactored out ofrun_git_inso blank-stderr exits get a synthetic diagnostic that's unit-testable.Chat blocks —
tui/components/chat/blocks/{system,git_diff}.rs,tool/{numbered_row,bordered_row}.rs,blocks.rs,chat.rsSystemMessageBlock(▎accent bar) for/help//status//configoutput.GitDiffBlockparses unified-diff output and reusesnumbered_row::Rendererfor+/-rows with red / green row bgs and line numbers from@@headers.numbered_row/bordered_rowvisibility topub(in super::super)so non-tool block modules reuse them without a physical move.ChatView::push_system_message/push_git_diffhelpers.Wiring —
tui/app.rs,config.rs,config/file.rs,main.rsapply_action_locallyrunsslash::parse_slashfirst; on hit, dispatches locally and short-circuits.ConfigSnapshot+Config::snapshot()+Auth::label(). Path discovery helpers becomepub(crate)so/configre-discovers fresh paths at execute time.run_tuibuildsSessionInfofrom the resumed session id +ConfigSnapshotand hands it intoApp::new.Docs —
docs/research/design/slash-commands.md,docs/roadmap.md,CLAUDE.mdTest hygiene sweep — alongside the feature, a codebase-wide pass aligned tests with CLAUDE.md ordering rules: import groupings (drop blank lines splitting
super::andcrate::), intra-section order (happy → variants → errors), and test-section order matching production function order.Test plan
cargo fmt --all --checkcargo build/cargo clippy --all-targets -- -D warningscargo test— 1430 passpnpm lint/pnpm spellcheckcargo llvm-cov— overall 98.8% / 97.8% / 98.4% (lines / functions / regions). Patch coverage ~100% on lines this PR added; the few remaining file-level gaps are pre-existing defensive error-path closures (current_dir()failure, git-spawn failure oninside_git_repo) that need syscall mocking to exercise./,/help,/status,/config,/diff(clean + dirty trees),/foo.Out of scope
/clear,/init,/theme,/resume— state-mutating commands without API client coupling./modelmid-session swap — needsClient.model: Arc<RwLock<String>>and per-turn re-read.docs/roadmap.md./compact,/cost,/login,/logout, custom user commands — seedocs/research/design/slash-commands.md.