|
| 1 | +--- |
| 2 | +name: rust-coding-skill |
| 3 | +description: "Use this skill whenever editing `*.rs` files in the `rust/` SDK in order to write idiomatic, efficient, well-structured Rust code" |
| 4 | +--- |
| 5 | + |
| 6 | +# Rust Coding Skill |
| 7 | + |
| 8 | +Opinionated Rust rules for the Copilot Rust SDK (`rust/`). Priority order: |
| 9 | + |
| 10 | +1. **Readable code** — every line should earn its place |
| 11 | +2. **Correct code** — especially in concurrent/async contexts |
| 12 | +3. **Performant code** — think about allocations, data structures, hot paths |
| 13 | + |
| 14 | +## Error handling |
| 15 | + |
| 16 | +The SDK's public error type is `crate::Error` (`rust/src/error.rs`). Add new |
| 17 | +variants there rather than introducing parallel error enums per module — every |
| 18 | +public failure mode is part of the API contract and should be expressible in one |
| 19 | +type. Internal modules can use `thiserror` enums when a richer local taxonomy |
| 20 | +helps; convert at the boundary. |
| 21 | + |
| 22 | +`anyhow` is reserved for binaries and example code. Library code never returns |
| 23 | +`anyhow::Result` — callers can't pattern-match on `anyhow::Error`, so it would |
| 24 | +prevent them from handling specific failures. |
| 25 | + |
| 26 | +In production code, prefer `?`, `let-else`, and `if let`. Reach for `expect("…")` |
| 27 | +when an invariant cannot fail and the message would help debug a future |
| 28 | +regression. `unwrap()` belongs in tests only — Clippy enforces this in the SDK |
| 29 | +via `#![cfg_attr(test, allow(clippy::unwrap_used))]` in `lib.rs`. |
| 30 | + |
| 31 | +When you need to log on the way through, prefer |
| 32 | +`.inspect_err(|e| warn!(error = ?e, "context"))?` over a `match` that logs and |
| 33 | +re-wraps. It reads top-to-bottom and keeps the happy path uncluttered. |
| 34 | + |
| 35 | +## Async and concurrency |
| 36 | + |
| 37 | +The default for request-scoped I/O is `async fn` plus `.await` — futures |
| 38 | +inherit cancellation from their parent task and can borrow local references. |
| 39 | +Reach for `tokio::spawn` only when you genuinely need background work (an event |
| 40 | +loop, a long-lived watcher) and track the `JoinHandle` so you can cancel or join |
| 41 | +it on shutdown. Fire-and-forget spawns silently swallow panics and outlive the |
| 42 | +session; don't. |
| 43 | + |
| 44 | +Blocking calls (filesystem, subprocess wait) belong in |
| 45 | +`tokio::task::spawn_blocking`, *not* on the async runtime. The blocking pool is |
| 46 | +bounded, so for genuinely long-lived workers (think: file watchers that run for |
| 47 | +the lifetime of a session) prefer `std::thread::spawn` with a channel back into |
| 48 | +async land. |
| 49 | + |
| 50 | +Lock choice matters. `tokio::sync::Mutex` is correct when you must hold the |
| 51 | +guard across `.await`; `parking_lot::Mutex` (or `RwLock`) is faster on hot |
| 52 | +synchronous paths and is what `session.rs` uses for capability state. |
| 53 | +`std::sync::Mutex` is rarely the right answer in this crate — its poisoning |
| 54 | +semantics buy us nothing and it's slower than `parking_lot`. Never hold a |
| 55 | +`std::sync::Mutex` guard across an `.await`; Clippy will catch this, but the |
| 56 | +fix is to move the await out, not silence the lint. |
| 57 | + |
| 58 | +For lazy statics use `std::sync::LazyLock`. The `once_cell` crate is no longer |
| 59 | +needed. |
| 60 | + |
| 61 | +## Traits and conversions |
| 62 | + |
| 63 | +Plain functions on a type beat traits for navigability. Use them as the default. |
| 64 | + |
| 65 | +**Trait-based extension points are different.** When a consumer must plug behaviour into the SDK, prefer one trait with one default-impl method per event over per-event `Box<dyn Fn>` callback fields. This is what `tower_lsp::LanguageServer`, `rmcp::ServerHandler`, and `notify::EventHandler` do — the dominant idiom in async Rust for "wire-protocol handler" traits. Callback fields fight `Send + Sync + 'static`, fragment consumer state across closures, and skip exhaustiveness checks. |
| 66 | + |
| 67 | +The four extension-point traits in this crate: |
| 68 | + |
| 69 | +- **`SessionHandler`** (`rust/src/handler.rs`) — per-event methods (`on_permission_request`, `on_user_input`, `on_external_tool`, `on_elicitation`, `on_exit_plan_mode`, `on_auto_mode_switch`, `on_session_event`) each with a default impl. The dispatcher `on_event(HandlerEvent)` is itself a default method that fans out to them; override per-event methods in normal use, override `on_event` only when you want a single exhaustive match. Concurrent invocations are possible (notification-triggered events run on spawned tasks), so `Send + Sync + 'static` is required on the trait. |
| 70 | +- **`SessionHooks`** (`rust/src/hooks.rs`) — optional lifecycle callbacks. The SDK auto-enables hooks when an impl is supplied to `create_session` / `resume_session`. |
| 71 | +- **`SystemMessageTransform`** (`rust/src/system_message.rs`) — declare `section_ids()` and return content from `transform_section()`. |
| 72 | +- **`ToolHandler`** (`rust/src/tool.rs`) — client-side tool implementations, dispatched by name via `ToolHandlerRouter`. |
| 73 | + |
| 74 | +`ApproveAllHandler` is the standard test handler for `SessionHandler`. |
| 75 | + |
| 76 | +**Don't add traits without a clear extension story.** Don't implement `From`/`Into` for SDK-internal conversions: they can't take extra parameters, can't return `Result`, and hide which conversion is happening at call sites. Prefer named methods like `to_info(&self)` or `MyType::from_record(record, ctx)`. |
| 77 | + |
| 78 | +Trivial field re-shaping is best inlined. Closures should stay short (under ~10 lines); extract to named functions when they grow. Visitor patterns are a closure-fest — expose `iter()` and let the consumer drive. |
| 79 | + |
| 80 | +## Concurrency primitives |
| 81 | + |
| 82 | +**Channels, not callback closures, for event flow.** Closures fight `Send + Sync + 'static` and don't compose with `select!`. Channel choice by semantics: |
| 83 | + |
| 84 | +| Use case | Primitive | |
| 85 | +|---|---| |
| 86 | +| One producer → one consumer with backpressure | `tokio::sync::mpsc` (cap 1) or `tokio::sync::oneshot` for single value | |
| 87 | +| Many producers → one consumer | `tokio::sync::mpsc` | |
| 88 | +| One producer → many consumers, every event delivered (pub/sub) | `tokio::sync::broadcast` | |
| 89 | +| One producer → many consumers, only the latest value matters | `tokio::sync::watch` | |
| 90 | + |
| 91 | +For the **public** API, prefer returning `impl Stream<Item = Event>` (wrap a `broadcast::Receiver` in `tokio_stream::wrappers::BroadcastStream`). `Stream` composes with `select!`, `take`, `map`, `filter`, `timeout`. See `EventSubscription` and `LifecycleSubscription`. |
| 92 | + |
| 93 | +**Cancellation: drop is the primitive; `tokio_util::sync::CancellationToken` for SDK-internal task coordination.** |
| 94 | + |
| 95 | +- **Caller-owned futures** (`send_message`, subscription streams): drop / `select!` / `tokio::time::timeout`. Don't accept a token parameter — it duplicates what `select!` already provides. Document cancel-safety on every `.await` in the hot path. |
| 96 | +- **SDK-internal tasks** (event loops, subprocess readers, anything `tokio::spawn`ed by the SDK): use `CancellationToken` stored on the long-lived handle. `Drop` calls `cancel()`. `Session::cancellation_token()` exposes a child token so callers can bind external work to the session lifetime. |
| 97 | + |
| 98 | +Refs: [`CancellationToken`][ctoken] · [`tonic` example][tonic-cancel] · [withoutboats: async clean-up][wb-cleanup] · [Cybernetist: cancellation patterns][cybernetist]. |
| 99 | + |
| 100 | +[ctoken]: https://docs.rs/tokio-util/latest/tokio_util/sync/struct.CancellationToken.html |
| 101 | +[tonic-cancel]: https://github.com/hyperium/tonic/blob/master/examples/src/cancellation/server.rs |
| 102 | +[wb-cleanup]: https://without.boats/blog/asynchronous-clean-up/ |
| 103 | +[cybernetist]: https://cybernetist.com/2024/04/19/rust-tokio-task-cancellation-patterns/ |
| 104 | + |
| 105 | +## Optional fields and serde |
| 106 | + |
| 107 | +Use `Option<T>` for optional fields, not nullable references or sentinel values. Defaults come from `Default` impls. Pair with `#[non_exhaustive]` on public config structs and a builder so adding fields stays non-breaking. |
| 108 | + |
| 109 | +For required builder fields: prefer `build() -> Result<Self, BuildError>` over typestate unless required-field count is tiny (1-2). |
| 110 | + |
| 111 | +JSON: `#[serde(rename_all = "camelCase")]` at the type level, per-field `#[serde(rename = "…")]` for outliers, `#[serde(skip_serializing_if = "Option::is_none")]` for output, `#[serde(default)]` for input tolerance. Reach for `serde_with` only for non-trivial transforms (durations, base64, numeric-as-string keys). |
| 112 | + |
| 113 | +## Tracing — `#[tracing::instrument]` is banned |
| 114 | + |
| 115 | +Banned via `clippy.toml`. Use manual spans with `error_span!`: |
| 116 | + |
| 117 | +- **Almost always use `error_span!`**, not `info_span!`. Span level controls |
| 118 | + the *minimum* filter at which the span appears. An `info_span` disappears when |
| 119 | + the filter is `warn` or `error` — taking all child events with it, even |
| 120 | + errors. `error_span!` ensures the span is always present. |
| 121 | +- **Spawned tasks lose parent context.** Attach a span with `.instrument()` or |
| 122 | + events inside won't correlate. |
| 123 | +- **Never hold `span.enter()` guards across `.await`** — use `.instrument(span)` |
| 124 | + instead (also enforced by Clippy). |
| 125 | + |
| 126 | +```rust |
| 127 | +use tracing::Instrument; |
| 128 | + |
| 129 | +async fn send_message(&self, session_id: &str, prompt: &str) -> Result<(), Error> { |
| 130 | + let span = tracing::error_span!("send_message", session_id = %session_id); |
| 131 | + async { /* body */ }.instrument(span).await |
| 132 | +} |
| 133 | + |
| 134 | +let span = tracing::error_span!("event_loop", session_id = %id); |
| 135 | +tokio::spawn(async move { run_loop().await }.instrument(span)); |
| 136 | +``` |
| 137 | + |
| 138 | +Log with structured fields: `info!(session_id = %id, "Session created")`. |
| 139 | +Static messages stay greppable; dynamic data goes in named fields, not |
| 140 | +interpolated into the message string. |
| 141 | + |
| 142 | +## Idioms that don't port from other languages |
| 143 | + |
| 144 | +When porting from Node, Python, Go, or .NET: see the **Concurrency primitives** and **Traits and conversions** sections above. The two patterns that most reliably translate poorly are (1) per-event `Box<dyn Fn>` callback fields — use a trait with default-impl methods (the `tower_lsp::LanguageServer` / `rmcp::ServerHandler` / `notify::EventHandler` shape) — and (2) plumbing `context.Context` / `CancellationToken` through every call site — drop-cancel for caller-owned futures, `tokio_util::sync::CancellationToken` for SDK-internal tasks. |
| 145 | + |
| 146 | +## Code organization |
| 147 | + |
| 148 | +- **Public API:** every `pub` item in the crate is part of the SDK's contract. |
| 149 | + Adding a field to a `pub struct` is a breaking change unless the struct is |
| 150 | + `#[non_exhaustive]` or constructors hide field-by-field literals. Prefer |
| 151 | + `Default + ..Default::default()` patterns and document new fields with |
| 152 | + rustdoc. |
| 153 | +- **Generated code lives in `rust/src/generated/`** and must not be |
| 154 | + hand-edited. Regenerate with `cd scripts/codegen && npm run generate:rust`. |
| 155 | + When a generated type lacks a field the schema doesn't yet describe (e.g. |
| 156 | + `Tool::overrides_built_in_tool`), hand-author the user-facing type in |
| 157 | + `rust/src/types.rs` and stop re-exporting the generated one. |
| 158 | +- **`#[expect(dead_code)]`** instead of `#[allow(dead_code)]` on individual |
| 159 | + fields — it forces a cleanup once the field gets used. |
| 160 | +- **`..Default::default()`** — avoid in production code (be explicit about |
| 161 | + which fields you're setting); prefer it in tests and doc examples to keep |
| 162 | + the focus on the values that matter for the test. |
| 163 | +- **Import grouping** — three blocks separated by blank lines: |
| 164 | + (1) `std`/`core`/`alloc`, (2) external crates, (3) |
| 165 | + `crate::`/`super::`/`self::`. Enforced by nightly `cargo fmt` via |
| 166 | + `rust/.rustfmt.nightly.toml`. |
| 167 | +- **`pub(crate)` vs `pub`** — most modules in `lib.rs` are private (`mod`), so |
| 168 | + `pub` items inside them are already crate-private. Use `pub(crate)` only when |
| 169 | + you want to be explicit that an item must not become part of the public API. |
| 170 | + |
| 171 | +## Testing |
| 172 | + |
| 173 | +- **No mock testing.** Depend on real implementations, spin up lightweight |
| 174 | + versions (e.g. `MockServer` in tests), or restructure code so the logic |
| 175 | + under test takes its dependency's output as input. |
| 176 | +- `assert_eq!(actual, expected)` — actual first, for readable diffs. |
| 177 | +- Tests at end of file: `#[cfg(test)] mod tests`. Never place production code |
| 178 | + after the test module. |
| 179 | +- Keep tests concurrent-safe — unique temp dirs (`tempfile::tempdir()`), |
| 180 | + unique data, no global state. |
| 181 | +- `ApproveAllHandler` is the standard test handler for sessions that don't |
| 182 | + exercise permission logic — see `rust/src/handler.rs:174`. |
| 183 | + |
| 184 | +## Cross-platform |
| 185 | + |
| 186 | +The SDK ships on macOS, Windows, and Linux; CI exercises all three. Construct |
| 187 | +paths with `Path::join` rather than string concatenation — `/` and `\` are not |
| 188 | +interchangeable, and string equality breaks on Windows UNC paths. Log paths |
| 189 | +with `path.display()`; serialize with `to_string_lossy()` only when you need a |
| 190 | +`String`. |
| 191 | + |
| 192 | +Process spawning needs care. The SDK applies `CREATE_NO_WINDOW` on Windows |
| 193 | +when launching the CLI (see `Client::build_command`); preserve that if you |
| 194 | +touch process spawning. Subprocess stdout often contains `\r` on Windows — strip |
| 195 | +or split on `\r?\n` rather than assuming `\n`. |
| 196 | + |
| 197 | +Tests must use `tempfile::tempdir()`, never hardcoded `/tmp/`, and any test |
| 198 | +that asserts on a path string needs to normalize separators or use |
| 199 | +`std::path::MAIN_SEPARATOR`. |
| 200 | + |
| 201 | +## Build speed |
| 202 | + |
| 203 | +Specify Tokio features explicitly — never `features = ["full"]`. Iterate with |
| 204 | +`cargo check`; reach for `cargo build` only when you need the binary. Audit |
| 205 | +new dependency feature flags with `cargo tree` before committing. |
| 206 | + |
| 207 | +## Comments |
| 208 | + |
| 209 | +Explain **why**, never **what**. No comments that restate code. No decorative |
| 210 | +banners (`// ── Section ────────`). |
| 211 | + |
| 212 | +**Never compare to other SDKs in code comments or rustdoc.** Don't write |
| 213 | +"Mirrors Node's `Foo`", "Like Go's `Bar`", "Unlike Python's `Baz`", or include |
| 214 | +file/line citations into other SDKs (`nodejs/src/types.ts:1592`, `go/types.go:14`). |
| 215 | +The Rust SDK seeks parity with the Node, Python, Go, and .NET SDKs, and that |
| 216 | +fact is stated once at the top of `rust/README.md`. Intentional divergences |
| 217 | +live in the README's "Differences From Other SDKs" section. Repeating the |
| 218 | +relationship per-symbol is unscalable, drifts as the other SDKs evolve, and |
| 219 | +adds noise to consumer-facing rustdoc — Rust users care about the Rust API, |
| 220 | +not its lineage. Self-references within the Rust crate (e.g. "Mirrors |
| 221 | +[`from_streams`] but adds…") are fine. |
| 222 | + |
| 223 | +## Toolchain |
| 224 | + |
| 225 | +The SDK is pinned to `rust 1.94.0` via `rust/rust-toolchain.toml`. Formatting |
| 226 | +uses nightly (`nightly-2026-04-14`) so unstable rustfmt options like grouped |
| 227 | +imports work — see `rust/.rustfmt.nightly.toml`. CI runs: |
| 228 | + |
| 229 | +```bash |
| 230 | +cd rust |
| 231 | +cargo +nightly-2026-04-14 fmt --check |
| 232 | +cargo clippy --all-features --all-targets -- -D warnings |
| 233 | +cargo test --all-features |
| 234 | +``` |
| 235 | + |
| 236 | +Match those exact commands locally before pushing. |
| 237 | + |
| 238 | +## Codegen |
| 239 | + |
| 240 | +JSON-RPC and session-event types are generated from the Copilot CLI schema: |
| 241 | + |
| 242 | +| Source | Output | |
| 243 | +|---|---| |
| 244 | +| `nodejs/node_modules/@github/copilot/schemas/api.schema.json` | `rust/src/generated/api_types.rs` | |
| 245 | +| `nodejs/node_modules/@github/copilot/schemas/session-events.schema.json` | `rust/src/generated/session_events.rs` | |
| 246 | + |
| 247 | +Regenerate with: |
| 248 | + |
| 249 | +```bash |
| 250 | +cd scripts/codegen && npm run generate:rust |
| 251 | +``` |
| 252 | + |
| 253 | +Never hand-edit files under `rust/src/generated/`. If a generated type needs a |
| 254 | +field the schema lacks, hand-author the user-facing type in `rust/src/types.rs` |
| 255 | +and stop re-exporting the generated one. |
0 commit comments