From c841aeda628f06ed098e1645180961be495baee0 Mon Sep 17 00:00:00 2001 From: Parker Henderson Date: Thu, 12 Feb 2026 16:04:59 -0800 Subject: [PATCH] add bt-review claude skill to review CLI structure and ux --- .claude/skills/bt-review/SKILL.md | 131 +++++++ .../bt-review/references/bt-patterns.md | 336 ++++++++++++++++++ .../bt-review/references/clig-checklist.md | 128 +++++++ 3 files changed, 595 insertions(+) create mode 100644 .claude/skills/bt-review/SKILL.md create mode 100644 .claude/skills/bt-review/references/bt-patterns.md create mode 100644 .claude/skills/bt-review/references/clig-checklist.md diff --git a/.claude/skills/bt-review/SKILL.md b/.claude/skills/bt-review/SKILL.md new file mode 100644 index 0000000..2b2b5c1 --- /dev/null +++ b/.claude/skills/bt-review/SKILL.md @@ -0,0 +1,131 @@ +--- +name: bt-review +description: > + This skill should be used to review and audit the bt CLI for adherence to CLI + best practices from clig.dev AND internal codebase patterns. It checks source + code for help text, flags, error handling, output formatting, subcommand + structure, pattern consistency, and more. Triggers on "review my code", + "audit the CLI", "check CLI best practices", or /bt-review. +--- + +# CLI Best Practices Review + +Audit the `bt` CLI codebase against two reference documents: + +1. **clig.dev guidelines** — industry CLI best practices +2. **bt codebase patterns** — established internal conventions for consistency + +## When to Use + +- When a user asks to review, audit, or check the CLI +- When triggered via `/bt-review` +- After implementing new commands or subcommands +- Before releases to ensure CLI quality + +## Review Process + +### 1. Scope the Review + +Determine what to review: + +- **Full audit**: All commands and subcommands +- **Targeted review**: Specific command or area (e.g., just `prompts`, just error handling) +- **Diff review**: Only changed files on the current branch vs main + +To scope a diff review, run `git diff main --name-only -- '*.rs'` and focus on changed files. + +### 2. Load Reference Documents + +Read both reference files: + +- `references/clig-checklist.md` — industry CLI guidelines organized by category +- `references/bt-patterns.md` — established codebase patterns to check for consistency + +### 3. Analyze Source Code + +#### clig.dev Compliance + +For each category in the checklist, examine relevant source files: + +- **Args & flags**: Read `src/args.rs` and `src/main.rs` — check clap derive attributes, flag naming, long/short forms +- **Help text**: Check all `#[command]` and `#[arg]` attributes for descriptions, examples, and help templates +- **Error handling**: Grep for `anyhow::`, `.context(`, `eprintln!`, and error types — verify human-readable messages +- **Output**: Check stdout vs stderr usage, TTY detection, color handling, JSON output support +- **Subcommands**: Review `src/*/mod.rs` files for consistency in naming and structure +- **Interactivity**: Check `dialoguer` usage for TTY guards and `--no-input` support +- **Robustness**: Look for timeout handling, progress indicators (`indicatif`), signal handling +- **Config**: Check env var handling (`dotenvy`, `clap` env features), XDG compliance + +#### Pattern Consistency + +Compare new or changed code against `references/bt-patterns.md`. Check: + +- **Module structure**: Does it follow `mod.rs` / `api.rs` / `list.rs` / `view.rs` / `delete.rs` layout? +- **run() dispatcher**: Does `mod.rs` have the standard `Args → Optional → match` with `None => List`? +- **api.rs conventions**: `ListResponse { objects }` wrapper, `get_by_*` returns `Option`, URL-encoded params +- **Interactive fallback**: `match identifier { Some → fetch, None → TTY check → fuzzy_select or bail }` +- **Delete confirmation**: `Confirm::new().default(false)`, only when stdin is terminal, `return Ok(())` on decline +- **Success/error status**: `print_command_status(CommandStatus::Success/Error, "Past tense message")` +- **List output**: JSON early return → summary line → `styled_table` → `print_with_pager` +- **Spinner usage**: `with_spinner("Present participle...", future)`, stderr-only, 300ms delay +- **Positional + flag dual args**: positional precedence over flag, both optional, `.identifier()` accessor method +- **Project resolution**: `base.project → interactive select → bail with env var hint` +- **Color/styling**: bold for names, dim for secondary, green/red for status, cyan for template vars +- **Import order**: std → external crates → `crate::` → `super::` +- **Error messages**: `bail!("thing required. Use: bt ")` + +### 4. Report Findings + +Produce a structured report document to `bt-review.md`: + +``` +# CLI Review: bt + +## Summary +[1-2 sentence overall assessment] + +## clig.dev Compliance + +### [Category Name] — [PASS / NEEDS WORK / NOT APPLICABLE] + +| Guideline | Status | Details | +|-----------|--------|---------| +| [item] | PASS/FAIL/PARTIAL | [specific finding with file:line references] | + +## Pattern Consistency + +### [Pattern Name] — [CONSISTENT / INCONSISTENT / NOT APPLICABLE] + +| Expected Pattern | Status | Details | +|------------------|--------|---------| +| [pattern] | OK/DEVIATION | [what differs and where, with file:line] | + +## Priority Fixes +1. [Most impactful issue with specific fix suggestion] +2. ... + +## Good Practices Already Followed +- [List what's already done well] +``` + +### 5. Prioritization + +Rank findings by impact: + +- **P0 — Broken**: Exit codes wrong, secrets in flags, no help text, crashes +- **P1 — Inconsistent**: Deviates from established patterns, missing TTY detection, inconsistent flags +- **P2 — Polish**: Missing `--json`, no pager, could suggest next commands +- **P3 — Nice-to-have**: Man pages, completion scripts, ASCII art + +Pattern deviations are typically P1 unless the deviation is an intentional improvement. + +## Important Notes + +- This is a Rust project — check `clap` derive patterns, not manual arg parsing +- The `projects/` module is the reference implementation — new resource modules should match its patterns +- The CLI uses `anyhow` for error handling — look for `.context()` calls for user-friendly errors +- Interactive features use `dialoguer` — verify TTY checks before prompting +- Progress uses `indicatif` — check spinner/progress bar usage for long ops +- Focus findings on actionable, specific issues with file paths and line numbers +- Do not suggest changes to test files or build configuration +- When a pattern deviation is found, reference both the new code and the established pattern location diff --git a/.claude/skills/bt-review/references/bt-patterns.md b/.claude/skills/bt-review/references/bt-patterns.md new file mode 100644 index 0000000..883054a --- /dev/null +++ b/.claude/skills/bt-review/references/bt-patterns.md @@ -0,0 +1,336 @@ +# bt CLI Codebase Patterns + +Established patterns in the bt CLI that new code must follow for consistency. + +## Module Structure + +### Resource modules follow a standard layout + +Every resource (projects, prompts) uses the same file structure: + +``` +src// +├── mod.rs # Args, subcommand enum, run() dispatcher +├── api.rs # HTTP API calls, request/response types +├── list.rs # List subcommand +├── view.rs # View subcommand +├── delete.rs # Delete subcommand +└── ... # Additional subcommands +``` + +### mod.rs pattern + +Each resource module's `mod.rs` follows this exact pattern: + +```rust +// 1. Args struct with Optional subcommand (None defaults to List) +#[derive(Debug, Clone, Args)] +pub struct ResourceArgs { + #[command(subcommand)] + command: Option, +} + +// 2. Subcommand enum with doc comments as help text +#[derive(Debug, Clone, Subcommand)] +enum ResourceCommands { + /// List all resources + List, + /// View a resource + View(ViewArgs), + /// Delete a resource + Delete(DeleteArgs), +} + +// 3. run() function: login → client → match dispatch +pub async fn run(base: BaseArgs, args: ResourceArgs) -> Result<()> { + let ctx = login(&base).await?; + let client = ApiClient::new(&ctx)?; + // ... resolve project if needed ... + match args.command { + None | Some(ResourceCommands::List) => list::run(...).await, + Some(ResourceCommands::View(a)) => view::run(...).await, + Some(ResourceCommands::Delete(a)) => delete::run(...).await, + } +} +``` + +**Key rule**: `None` always maps to `List` — running `bt ` with no subcommand shows the list. + +### api.rs pattern + +```rust +// 1. Main model struct: Serialize + Deserialize, pub fields +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Resource { + pub id: String, + pub name: String, + #[serde(default)] + pub description: Option, +} + +// 2. Private ListResponse wrapper +#[derive(Debug, Deserialize)] +struct ListResponse { + objects: Vec, +} + +// 3. Functions take &ApiClient, return Result +pub async fn list_resources(client: &ApiClient, ...) -> Result> { ... } +pub async fn get_resource_by_name(client: &ApiClient, ...) -> Result> { ... } +pub async fn delete_resource(client: &ApiClient, id: &str) -> Result<()> { ... } +``` + +**Key rules**: + +- URL-encode all query params with `urlencoding::encode()` +- `get_by_name` returns `Option` (not an error for missing) +- `list_` returns `Vec` via `ListResponse.objects` + +## UX Patterns + +### Interactive fallback pattern + +When a required identifier (name, slug) isn't provided: + +```rust +let resource = match identifier { + Some(s) => fetch_by_identifier(client, s).await?, + None => { + if !std::io::stdin().is_terminal() { + bail!(" required. Use: bt "); + } + select_resource_interactive(client).await? + } +}; +``` + +**Key rules**: + +- Always check `std::io::stdin().is_terminal()` before interactive prompts +- Bail message must include the exact command syntax to use non-interactively +- Use `select__interactive()` for fuzzy selection + +### Interactive selection pattern + +```rust +pub async fn select_resource_interactive(client: &ApiClient, ...) -> Result { + let mut items = with_spinner("Loading ...", api::list_resources(client, ...)).await?; + if items.is_empty() { + bail!("no found"); + } + items.sort_by(|a, b| a.name.cmp(&b.name)); + let names: Vec<&str> = items.iter().map(|i| i.name.as_str()).collect(); + let selection = ui::fuzzy_select("Select ", &names)?; + Ok(items[selection].clone()) +} +``` + +**Key rules**: + +- Sort alphabetically before display +- Use `ui::fuzzy_select()` (wraps dialoguer FuzzySelect with TTY guard) +- Bail early if list is empty +- Wrap API call in `with_spinner` + +### Delete confirmation pattern + +```rust +if std::io::stdin().is_terminal() { + let confirm = Confirm::new() + .with_prompt(format!("Delete '{}'?", name)) + .default(false) + .interact()?; + if !confirm { + return Ok(()); + } +} +``` + +**Key rules**: + +- Default to `false` (don't delete) +- Only prompt when stdin is terminal +- Silent (no confirmation) when non-interactive — relies on explicit identifier arg +- Return `Ok(())` on decline, not an error + +### Success/error status pattern + +Mutating operations (create, delete) use `print_command_status`: + +```rust +match with_spinner("Deleting...", api::delete(client, &id)).await { + Ok(_) => { + print_command_status(CommandStatus::Success, &format!("Deleted '{name}'")); + Ok(()) + } + Err(e) => { + print_command_status(CommandStatus::Error, &format!("Failed to delete '{name}'")); + Err(e) + } +} +``` + +**Key rules**: + +- `✓` green for success, `✗` red for error (via `CommandStatus` enum) +- Message format: past tense verb + quoted resource name +- Print status THEN return the error (so user sees both) + +### "Open in browser" pattern + +```rust +let url = format!( + "{}/app/{}/p/{}", + app_url.trim_end_matches('/'), + encode(org_name), + encode(&name) +); +open::that(&url)?; +print_command_status(CommandStatus::Success, &format!("Opened {url} in browser")); +``` + +### List output pattern + +```rust +if json { + println!("{}", serde_json::to_string(&items)?); +} else { + let mut output = String::new(); + // Summary line: count + context + writeln!(output, "{} found in {}\n", count, context)?; + // Table + let mut table = styled_table(); + table.set_header(vec![header("Col1"), header("Col2")]); + apply_column_padding(&mut table, (0, 6)); + for item in &items { + table.add_row(vec![...]); + } + write!(output, "{table}")?; + print_with_pager(&output)?; +} +``` + +**Key rules**: + +- JSON check first, early return +- Build output into a `String`, then pipe through `print_with_pager` +- Summary line before table: "{count} {resource}s found in {context}" +- Table uses `styled_table()` (NOTHING preset, dynamic width) +- Headers use `header()` (bold + dim) +- Column padding `(0, 6)` — no left padding, 6 right padding +- Truncate descriptions to 60 chars with `truncate(s, 60)` +- Missing descriptions display as `"-"` + +## Code Patterns + +### All subcommand functions are `pub async fn run(...) -> Result<()>` + +Entry point for every subcommand. Parameters are borrowed references, not owned. + +### BaseArgs are global and flattened via CLIArgs + +```rust +// In main.rs: +Commands::Resource(CLIArgs { base, args }) => resource::run(base, args).await? + +// BaseArgs contains: json, project, api_key, api_url, app_url, env_file +``` + +`--json` and `--project` are always available on any resource subcommand. + +### Error handling + +- Use `anyhow::Result` everywhere +- Use `.context("human-readable message")` on fallible operations +- Use `bail!("message")` for expected user errors +- Use `anyhow!("message")` for constructing error values +- HTTP errors: `"request failed ({status}): {body}"` format + +### Spinner usage + +- `with_spinner("Loading...", future)` — shows after 300ms, clears on completion +- `with_spinner_visible("Creating...", future, Duration)` — always shows, enforces minimum display time +- Spinner message: present participle + "..." (e.g., "Loading prompts...", "Deleting project...") +- Only shows when stderr is terminal + +### Positional + flag dual args pattern + +When a subcommand takes a primary identifier: + +```rust +#[derive(Debug, Clone, Args)] +pub struct ViewArgs { + /// Resource identifier (positional) + #[arg(value_name = "IDENTIFIER")] + identifier_positional: Option, + + /// Resource identifier (flag) + #[arg(long = "identifier", short = 'x')] + identifier_flag: Option, +} + +impl ViewArgs { + fn identifier(&self) -> Option<&str> { + self.identifier_positional + .as_deref() + .or(self.identifier_flag.as_deref()) + } +} +``` + +**Key rule**: Positional takes precedence over flag. Both are optional (falls back to interactive). + +### Project resolution pattern + +Commands that operate within a project scope: + +```rust +let project = match base.project { + Some(p) => p, + None if std::io::stdin().is_terminal() => select_project_interactive(&client).await?, + None => anyhow::bail!("--project required (or set BRAINTRUST_DEFAULT_PROJECT)"), +}; +``` + +### Color/styling conventions + +- `console::style(name).bold()` — resource names, identifiers +- `console::style(text).dim()` — secondary info, separators, headers +- `console::style(text).green()` — success, user role +- `console::style(text).blue()` — assistant role +- `console::style(text).red()` — errors (via CommandStatus) +- `console::style(text).yellow()` — tool names, function calls +- `console::style(text).cyan()` — template variables, spinners +- `console::style(text).magenta()` — section labels (e.g., "tools") + +### Import conventions + +```rust +// std imports first +use std::fmt::Write as _; +use std::io::IsTerminal; + +// External crates +use anyhow::{bail, Result}; +use dialoguer::console; + +// Internal crate imports +use crate::http::ApiClient; +use crate::ui::{...}; + +// Sibling module +use super::api; +``` + +### View output pattern (rich display) + +For detailed single-resource views: + +```rust +let mut output = String::new(); +writeln!(output, "Viewing {}", console::style(&name).bold())?; +// ... render fields ... +print_with_pager(&output)?; +``` + +Build into String, then page. Use box-drawing characters (`┃`, `│`) for structure. diff --git a/.claude/skills/bt-review/references/clig-checklist.md b/.claude/skills/bt-review/references/clig-checklist.md new file mode 100644 index 0000000..4180f5c --- /dev/null +++ b/.claude/skills/bt-review/references/clig-checklist.md @@ -0,0 +1,128 @@ +# CLI Guidelines Checklist (clig.dev) + +Audit checklist organized by category. Each item is a specific, verifiable guideline. + +## EXIT CODES & STREAMS + +- [ ] Return 0 on success, non-zero on failure +- [ ] Map distinct non-zero codes to important failure modes +- [ ] Primary output goes to stdout +- [ ] Errors, logs, status messages go to stderr + +## HELP + +- [ ] `-h` and `--help` show help +- [ ] Subcommands have their own `--help`/`-h` +- [ ] Running command with no required args shows concise help (description, examples, flag summary, pointer to `--help`) +- [ ] `myapp help` and `myapp help ` work (for git-like tools) +- [ ] Top-level help includes support path (website or GitHub link) +- [ ] Help text links to web documentation where applicable +- [ ] Help leads with examples showing common complex uses +- [ ] Most common flags and commands listed first +- [ ] Suggest corrections when users make typos in commands/flags +- [ ] If program expects piped stdin but gets interactive terminal, show help instead of hanging + +## OUTPUT + +- [ ] Detect TTY to distinguish human vs machine output +- [ ] Support `--json` flag for JSON output +- [ ] Support `--plain` flag for machine-readable plain text (if applicable) +- [ ] Display brief output on success confirming state changes +- [ ] Support `-q`/`--quiet` to suppress non-essential output +- [ ] Make current state easily visible (like `git status`) +- [ ] Suggest next commands in workflows +- [ ] Be explicit about boundary-crossing actions (file I/O, network calls) +- [ ] Use color intentionally, not saturated +- [ ] Disable color when stdout is not interactive TTY +- [ ] Disable color when `NO_COLOR` env var is set +- [ ] Disable color when `TERM=dumb` +- [ ] Support `--no-color` flag +- [ ] Disable animations when stdout is not interactive +- [ ] Don't output debug info by default +- [ ] Don't treat stderr like a log file (no log level labels by default) +- [ ] Use pager for large text output when stdout is interactive + +## ERRORS + +- [ ] Catch expected errors and rewrite for humans (conversational, suggest fixes) +- [ ] High signal-to-noise ratio (group similar errors) +- [ ] Important information at end of output where users look +- [ ] Unexpected errors: provide debug info + bug report instructions +- [ ] Streamline bug reporting (URLs with pre-populated info if possible) + +## ARGUMENTS & FLAGS + +- [ ] Prefer flags over positional arguments +- [ ] All flags have long-form versions (`--help` not just `-h`) +- [ ] Single-letter flags reserved for commonly used options only +- [ ] Use standard flag names where conventions exist: + - `-a, --all` | `-d, --debug` | `-f, --force` | `--json` + - `-h, --help` | `-n, --dry-run` | `--no-input` + - `-o, --output` | `-p, --port` | `-q, --quiet` + - `-u, --user` | `--version` | `-v` (version, not verbose — or skip) +- [ ] Defaults are correct for most users +- [ ] Prompt for missing input interactively +- [ ] Never _require_ prompts — always allow flags/args to skip them +- [ ] Skip prompts when stdin is non-interactive +- [ ] Confirm before dangerous actions (prompt or `--force`) +- [ ] Support `-` for stdin/stdout where applicable +- [ ] Arguments, flags, subcommands are order-independent where possible +- [ ] Never read secrets from flags (use files, stdin, or IPC) + +## INTERACTIVITY + +- [ ] Only use prompts when stdin is interactive TTY +- [ ] Support `--no-input` flag to disable prompts +- [ ] Don't echo passwords +- [ ] Ctrl-C works reliably to exit + +## SUBCOMMANDS + +- [ ] Consistent flag names across subcommands +- [ ] Consistent output formatting across subcommands +- [ ] Consistent naming convention (noun-verb or verb-noun) +- [ ] No ambiguous or similarly-named commands + +## ROBUSTNESS + +- [ ] Validate user input +- [ ] Output something within 100ms (responsive feel) +- [ ] Show progress for long operations (spinners/progress bars) +- [ ] Add timeouts with sensible defaults (no hanging forever) +- [ ] Handle Ctrl-C immediately, say something, then clean up +- [ ] Second Ctrl-C skips cleanup +- [ ] Handle uncleaned state from previous crashes + +## CONFIGURATION + +- [ ] Follow XDG Base Directory Specification for config files +- [ ] Parameter precedence: flags > env vars > project config > user config > system config +- [ ] Ask consent before modifying non-owned config +- [ ] Read from `.env` files where appropriate + +## ENVIRONMENT VARIABLES + +- [ ] Env var names: uppercase, numbers, underscores only +- [ ] Don't commandeer widely-used env var names +- [ ] Respect `NO_COLOR`, `EDITOR`, `HTTP_PROXY`, `TERM`, `PAGER`, `HOME` +- [ ] Don't read secrets from env vars (use credential files, stdin, IPC) + +## NAMING + +- [ ] Simple, memorable name +- [ ] Lowercase letters only (dashes if needed) +- [ ] Short enough for frequent typing +- [ ] Easy to type ergonomically + +## FUTURE-PROOFING + +- [ ] Keep changes additive (new flags rather than changed behavior) +- [ ] Warn before non-additive changes with migration guidance +- [ ] No catch-all subcommands that prevent future command names +- [ ] No arbitrary subcommand abbreviations +- [ ] Don't create time bombs (external dependencies that will disappear) + +## DISTRIBUTION + +- [ ] Distribute as single binary if possible +- [ ] Easy uninstall instructions available