| 1 | --- |
| 2 | name: rust-coding-standard |
| 3 | description: Rust coding standards for the splitfire-agent crate. Covers error handling, naming, async patterns, tool/toolset conventions, deterministic music computation, logging, and clippy compliance. Apply to all Rust edits. |
| 4 | allowed-tools: Read, Write, Edit, Glob, Grep |
| 5 | user-invocable: false |
| 6 | --- |
| 7 | |
| 8 | # Rust Coding Standard |
| 9 | |
| 10 | Rules for writing Rust in the `splitfire-agent` crate. These are traps to avoid, not maps to follow. |
| 11 | |
| 12 | This crate holds only the *music* part of a music-domain ACP agent. The ACP server, session/turn |
| 13 | loop, approval, workspace tools, terminal UI and MCP layer all come from the Onde Agent Platform |
| 14 | crates (`ed-acp`, `ed-acp-tui`, `ed-mcp`). When something is protocol-, session-, or |
| 15 | approval-related, it belongs there, not here. |
| 16 | |
| 17 | --- |
| 18 | |
| 19 | ## Correctness First |
| 20 | |
| 21 | Prioritize correctness and clarity. Performance is secondary unless explicitly stated. Complete, |
| 22 | mostly-correct code beats perfect code that doesn't solve the problem. |
| 23 | |
| 24 | The music theory and audio code exists so the model *computes* answers instead of recalling them. |
| 25 | Results must be exact: diatonic spelling (letter plus accidental, never bare pitch classes), so C# |
| 26 | major really has E# and B#. A plausible-but-wrong answer is worse than no tool at all. |
| 27 | |
| 28 | ## Error Handling |
| 29 | |
| 30 | - This crate uses `anyhow` throughout. There is no custom error enum and no `thiserror`; don't |
| 31 | introduce one unless a real need appears. |
| 32 | - Propagate errors with `?`. Return `anyhow::Result<T>`. Use `anyhow!("...")` and `.context("...")` |
| 33 | to add meaning at the point of failure. |
| 34 | - Never call `.unwrap()` or `.expect()` outside of `#[cfg(test)]` code. |
| 35 | - Never silently discard errors with `let _ =` on fallible operations: |
| 36 | - Propagate with `?` when the caller should handle it. |
| 37 | - Use `tracing::warn!` or `tracing::error!` when you need to ignore but want visibility. |
| 38 | - Use `match` or `if let Err(error)` when custom recovery logic is needed. |
| 39 | - Tool handlers return `Result<ToolOutcome>`; surface a clear message for bad arguments |
| 40 | (`Err(anyhow!("unknown tool `{name}`"))`) rather than panicking. |
| 41 | - Be careful with indexing operations that may panic if out of bounds. Prefer `.get()` and safe |
| 42 | alternatives — especially when indexing audio sample buffers or note arrays. |
| 43 | |
| 44 | ## Naming |
| 45 | |
| 46 | - Use full words for all variable and parameter names. No single-letter names, no abbreviations |
| 47 | (`progression` not `prog`, `argument` not `arg`, `error` not `e`). |
| 48 | - `snake_case` for functions and variables, `CamelCase` for types, `SCREAMING_SNAKE_CASE` for |
| 49 | constants. |
| 50 | - Tool name constants and the env prefix follow the existing style: screaming-snake constants |
| 51 | (`NAME`, `INFO`, `DEMUCS`), env vars prefixed `SPLITFIRE_`. |
| 52 | - Tool names are lowercase with underscores and a domain prefix: `theory_scale`, `theory_chord`, |
| 53 | `analyze_audio`, `list_separations`. |
| 54 | |
| 55 | ## Comments |
| 56 | |
| 57 | - Comments explain **why**, never **what**. Do not write comments that summarize or re-describe the |
| 58 | code. |
| 59 | - Doc comments (`///` on items, `//!` at module top) are encouraged — they explain the contract and |
| 60 | the design intent. The existing modules lead with a `//!` that states the invariant (e.g. |
| 61 | "Deterministic music theory: pure functions, no I/O"). Keep that style. |
| 62 | - Do not add organizational separators like `// --- Helper functions ---` unless they are already |
| 63 | established in the file's style. |
| 64 | |
| 65 | ## Module Structure |
| 66 | |
| 67 | - This crate already uses `mod.rs` as the module root for `tools/`, `theory/` and `audio/`. Follow |
| 68 | the existing layout — add new theory under `src/theory/`, new audio under `src/audio/`, new tools |
| 69 | under `src/tools/`. Do not restructure established module roots. |
| 70 | - Prefer implementing functionality in existing files unless it's a genuinely new logical component. |
| 71 | Avoid creating many small single-purpose files. |
| 72 | - Keep the layering intact: |
| 73 | - `src/theory/` and `src/audio/` are pure computation — no ACP types, and `theory` does no I/O. |
| 74 | - `src/tools/` adapts that computation into `Toolset` entries. |
| 75 | - `src/profile.rs` is the single wiring point (identity, prompt, toolsets, slash commands, |
| 76 | built-in demucs). `src/prompt.rs` holds the system prompt and slash commands. |
| 77 | - `src/lib.rs` is the embeddable library (the `Profile` for hosts using `ed_acp::serve`); |
| 78 | `src/main.rs` is the binary (TUI / `--acp` / `--setup` / `--list-models`). Keep library logic out |
| 79 | of `main.rs`. |
| 80 | |
| 81 | ## Adding or Changing a Tool |
| 82 | |
| 83 | `MusicTools` is one `Toolset`. When you add or change a tool, update all of these together: |
| 84 | |
| 85 | 1. `definitions()` — the JSON schema for the tool's arguments. |
| 86 | 2. `handles()` — the name match so the toolset claims the call. |
| 87 | 3. `describe()` — the human title, `ToolKind`, and any `ToolCallLocation`s. |
| 88 | 4. `execute()` — the dispatch arm that runs it. |
| 89 | |
| 90 | Music tools are read-only and never ask for approval. Anything that writes files or runs commands |
| 91 | belongs to `ed-acp`'s workspace tools, not here. |
| 92 | |
| 93 | ## Async |
| 94 | |
| 95 | - Runtime is `tokio` (`features = ["full"]`). Use `tokio::sync::Mutex`, not `std::sync::Mutex`, for |
| 96 | async-held locks. |
| 97 | - `Toolset::execute` is `async` (via `async_trait`). Keep pure theory synchronous (it is) and only |
| 98 | reach for `async` where there is real I/O (audio decode, the separation library, demucs). |
| 99 | - Use variable shadowing to scope `.clone()` calls inside async closures, minimizing borrow |
| 100 | lifetimes: |
| 101 | |
| 102 | ```rust |
| 103 | tokio::spawn({ |
| 104 | let server = server.clone(); |
| 105 | async move { |
| 106 | server.start().await; |
| 107 | } |
| 108 | }); |
| 109 | ``` |
| 110 | |
| 111 | ## Logging |
| 112 | |
| 113 | - Use the `tracing` crate macros: `tracing::debug!`, `tracing::info!`, `tracing::warn!`, |
| 114 | `tracing::error!`. Logs go to stderr via `RUST_LOG`. |
| 115 | - No `println!` or `eprintln!` in library code (`src/lib.rs` and everything it pulls in). The one |
| 116 | place direct stdout is correct is `src/main.rs` CLI output (`--version`, `--list-models`), where |
| 117 | the printed text is the program's result. |
| 118 | - Log at `debug` for routine operations, `info` for lifecycle events (demucs started/disabled), |
| 119 | `warn` for recoverable issues (an MCP server that failed to start), `error` for failures. |
| 120 | |
| 121 | ## Edition and Toolchain |
| 122 | |
| 123 | - Edition 2024. Prefer modern idioms it enables. |
| 124 | - Dependencies are pinned in `Cargo.toml`; reuse what's already there (`symphonia`, `ebur128`, |
| 125 | `realfft`, `serde`/`serde_json`, `uuid`) before adding a crate. |
| 126 | |
| 127 | ## Clippy Compliance |
| 128 | |
| 129 | All code must pass `cargo clippy --locked --all-targets -- -D warnings`. Common traps: |
| 130 | |
| 131 | - `&PathBuf` in function parameters → use `&Path` instead. |
| 132 | - `unwrap_or_else(Type::default)` → use `unwrap_or_default()`. |
| 133 | - `filter_map(|x| { ... Some(y) })` where every path returns `Some` → use `map`. |
| 134 | - `.map(|x| f(x))` where `f` is a plain function → use `.map(f)`. |
| 135 | - Redundant type annotations in closures: `.map(|note: &String| note.trim())` → `.map(|note| note.trim())`. |
| 136 | - Prefer `and_then(Value::as_str)` over a closure, matching the existing arg-parsing style. |
| 137 | |
| 138 | ## Testing |
| 139 | |
| 140 | - Unit tests go in a `#[cfg(test)] mod tests` block at the bottom of the file they test — this is |
| 141 | where the theory and audio correctness lives, so test exact spellings and edge cases (enharmonics, |
| 142 | dotted/triplet tempo, cents offsets). |
| 143 | - Use `#[tokio::test]` for async tests. |
| 144 | - Tests may use `.unwrap()` and `.expect()` — the panics are the assertions. |
| 145 | - Test names describe the scenario: `transpose_keeps_enharmonic_spelling`, not `test_transpose` or |
| 146 | `test_1`. |
| 147 | - Integration tests (`tests/`) run the real binary against a scripted mock model and a test MCP |
| 148 | server, and include the platform conformance suite from `ed-acp-testkit`. Shared helpers live in |
| 149 | `tests/common/`. |
| 150 | - `tests/live.rs` hits real Onde Cloud and is skipped unless `ONDE_API_KEY=app-id:app-secret` is set. |
| 151 | |
| 152 | ## Build Verification |
| 153 | |
| 154 | Always run `cargo check` before returning work. If code does not compile, fix it — do not return |
| 155 | with TODOs. Before pushing, run exactly what CI runs: |
| 156 | |
| 157 | ```sh |
| 158 | cargo fmt -- --check |
| 159 | cargo clippy --locked --all-targets -- -D warnings |
| 160 | cargo test --locked |
| 161 | ``` |
| 162 | |
| 163 | To also check the embeddable library alone (no TUI, no MCP test stub): |
| 164 | |
| 165 | ```sh |
| 166 | cargo check --no-default-features |
| 167 | ``` |