@splitfireai / agent / commits / ea5b744

docs(skills): rewrite rust-coding-standard for splitfire-agent

The skill was still written for the Onde Inference SDK crate. Rewrite it to match this repo: anyhow (no thiserror/FFI), tracing instead of log, edition 2024, mod.rs module roots, the MusicTools Toolset four-method checklist, the deterministic exact-spelling correctness principle, and the real CI commands. Co-Authored-By: siGit Code v1.6.6-acp <noreply@sigit.si>

Seto committed Oct 11, 2026 at 10:07 UTC ea5b7445229858ac63098d29d746072849457957
1 file changed +107 -56
.agents/skills/rust-coding-standard/SKILL.md
+107 -56
@@ -1,116 +1,167 @@
1 ---
2 name: rust-coding-standard
3 -description: Rust coding standards for the onde crate. Covers error handling, naming, async patterns, platform gating, clippy compliance, and cross-platform SDK conventions. Apply to all Rust edits.
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 `onde` crate. These are traps to avoid, not maps to follow.
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
16 -Prioritize correctness and clarity. Performance is secondary unless explicitly stated. Complete, mostly-correct code beats perfect code that doesn't solve the problem.
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
20 -- Propagate errors with `?`. Never call `.unwrap()` or `.expect()` outside of `#[cfg(test)]` code.
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.
23 - - Use `log::warn!` or `log::error!` when you need to ignore but want visibility.
24 - - Use `match` or `if let Err(e)` when custom recovery logic is needed.
25 -- Use `anyhow` for internal application errors in `engine.rs`.
26 -- Use `thiserror` for the `InferenceError` enum — it crosses the FFI boundary via UniFFI.
27 -- Be careful with indexing operations that may panic if out of bounds. Prefer `.get()` and safe alternatives.
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
31 -- Use full words for all variable and parameter names. No single-letter names, no abbreviations (`environment` not `env`, `client` not `c`, `message` not `msg`).
32 -- `snake_case` for functions and variables, `CamelCase` for types, `SCREAMING_SNAKE_CASE` for constants.
33 -- Model constants live in `src/inference/models.rs`. Prefix HuggingFace repo IDs with the org name: `BARTOWSKI_QWEN25_...`.
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
37 -- Comments explain **why**, never **what**. Do not write comments that summarize or re-describe the code.
38 -- Doc comments (`///`) on public items are fine and encouraged — they explain the contract, not the implementation.
39 -- Do not add organizational separators like `// --- Helper functions ---` unless they are already established in the file's style.
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
43 -- **No `mod.rs` for new modules.** Use `src/some_module.rs` instead of `src/some_module/mod.rs`. Exception: `inference/mod.rs` already exists and is the intentional module root — don't move it.
44 -- Prefer implementing functionality in existing files unless it's a genuinely new logical component. Avoid creating many small single-purpose files.
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
48 -- Runtime is `tokio`. All async functions use `tokio::sync::Mutex`, not `std::sync::Mutex`.
49 -- Use variable shadowing to scope `.clone()` calls inside async closures, minimizing borrow lifetimes:
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({
53 - let engine = engine.clone();
104 + let server = server.clone();
105 async move {
55 - engine.do_work().await;
106 + server.start().await;
107 }
108 });
109 ```
110
111 ## Logging
112
62 -- Use `log` crate macros: `log::debug!`, `log::info!`, `log::warn!`, `log::error!`.
63 -- No `println!` or `eprintln!` in library code. Ever.
64 -- Log at `debug` for routine operations, `info` for lifecycle events (model loaded/unloaded), `warn` for recoverable issues, `error` for failures.
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
66 -## Platform Gating
121 +## Edition and Toolchain
122
68 -- Use `#[cfg(target_os = "...")]` blocks. Match `Cargo.toml`'s target-conditional dependency sections.
69 -- When a function is only used on specific platforms, gate the function itself — not just the call site. This prevents dead-code warnings on other targets.
70 -- When an import is only used on specific platforms, gate the `use` statement separately. Do not put platform-only imports inside a shared `use` block.
71 -- The supported `target_os` values are: `macos`, `ios`, `tvos`, `windows`, `linux`, `android`.
72 -- ISQ model loading is `#[cfg(target_os = "macos")]` only. Gate the `Isq` variant, its match arms, and its imports (`IsqBits`, `TextModelBuilder`) to macOS.
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
76 -All code must pass `cargo clippy --all-targets -- -D warnings`. Common traps:
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)`.
82 -- Redundant type annotations in closures: `.map(|c: &String| c.trim())` → `.map(|c| c.trim())`.
83 -- Unused imports/variants on platform-gated code → gate the import/variant to the same platform.
84 -
85 -## Types for FFI
86 -
87 -- `usize` / `isize` are not FFI-safe. Use `u64` / `i64` in any type that crosses the UniFFI boundary.
88 -- `impl Into<String>` is not FFI-safe. Use concrete `String` in exported signatures.
89 -- All UniFFI-exported types go in `src/inference/types.rs`. The FFI wrapper lives in `src/inference/ffi.rs`.
90 -- `ChatEngine` (engine.rs) owns Rust-idiomatic logic. `OndeChatEngine` (ffi.rs) is the thin FFI wrapper. Never put `#[uniffi::export]` on `ChatEngine`.
91 -
92 -## Re-exports
93 -
94 -`mistralrs`, `hf_hub`, and `mistralrs_core` are re-exported from `lib.rs` for downstream Rust consumers. Keep these in sync with what's actually available per platform.
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
98 -- Unit tests go in a `#[cfg(test)] mod tests` block at the bottom of the file they test.
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.
101 -- Test names describe the scenario: `engine_send_without_model_errors`, not `test_send` or `test_1`.
102 -- Run `cargo test --lib` to skip integration tests that require model downloads.
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
104 -## Adding a New Model
152 +## Build Verification
153
106 -This is a common task with a specific checklist:
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
108 -1. Add `pub const` entries in `src/inference/models.rs` for repo ID, GGUF filename, and `TOK_MODEL_ID`.
109 -2. Add the repo ID to `SUPPORTED_MODELS`.
110 -3. Add a `SupportedModelInfo` entry to `SUPPORTED_MODEL_INFO` with `expected_size_bytes` from the HF API.
111 -4. Add a constructor to `GgufModelConfig` in `engine.rs` (follow `qwen25_1_5b` pattern).
112 -5. Export a free function in `ffi.rs` (follow `qwen25_1_5b_config` pattern).
157 +```sh
158 +cargo fmt -- --check
159 +cargo clippy --locked --all-targets -- -D warnings
160 +cargo test --locked
161 +```
162
114 -## Build Verification
163 +To also check the embeddable library alone (no TUI, no MCP test stub):
164
116 -Always run `cargo check` before returning work. If code does not compile, fix it — do not return with TODOs. On macOS, also run `cargo clippy --all-targets -- -D warnings` to catch lint issues before CI does.
\ No newline at end of file
165 +```sh
166 +cargo check --no-default-features
167 +```