diff --git a/.fabro/skills/rust-style-guide/SKILL.md b/.fabro/skills/rust-style-guide/SKILL.md new file mode 100644 index 000000000..1a2c1f9bf --- /dev/null +++ b/.fabro/skills/rust-style-guide/SKILL.md @@ -0,0 +1,44 @@ +--- +name: rust-style-guide +description: Apply this Rust style guide when writing, reviewing, refactoring, or configuring Rust code for this project. Covers Rust 2024/MSRV, library vs application conventions, public API design, errors, panics, ownership and cloning, async/Tokio/concurrency, tracing, rustfmt/Clippy, testing with nextest, and unsafe/macro policy. Also use when setting up new Rust projects, investigating Rust performance, verifying library releases, or reviewing Rust code changes. +--- + +# Rust Style Guide + +Use this skill to apply the project's Rust style conventions while writing, reviewing, refactoring, or configuring Rust code. + +> **Location:** This skill's supporting files live in `.fabro/skills/rust-style-guide/` at the repository root. Every linked path below (`guidelines.md`, `guidelines/*.md`, `workflows/*.md`) is relative to that directory. Read them with that prefix — e.g. `.fabro/skills/rust-style-guide/guidelines.md`. + +## Supporting Files + +- [guidelines.md](guidelines.md) - index of Rust style policy pages. Load this for ordinary Rust work, then load only the guideline pages relevant to the task. +- [workflows/new-rust-project.md](workflows/new-rust-project.md) - workflow for creating or configuring a new Rust crate, workspace, CLI, library, service, or application. +- [workflows/reusable-library-release.md](workflows/reusable-library-release.md) - workflow for verifying reusable library releases, feature combinations, dependency checks, and out-of-box builds. +- [workflows/performance-investigation.md](workflows/performance-investigation.md) - workflow for measuring, profiling, and changing performance-sensitive Rust code. +- [workflows/code-review-refactor.md](workflows/code-review-refactor.md) - workflow for reviewing, refactoring, or changing existing Rust code. + +## Routing Examples + +| Task | Load | +| --- | --- | +| Create a new Rust project | [workflows/new-rust-project.md](workflows/new-rust-project.md), [guidelines.md](guidelines.md) | +| Verify a reusable library release | [workflows/reusable-library-release.md](workflows/reusable-library-release.md), [guidelines.md](guidelines.md) | +| Investigate performance | [workflows/performance-investigation.md](workflows/performance-investigation.md), [guidelines.md](guidelines.md) | +| Review or refactor code | [workflows/code-review-refactor.md](workflows/code-review-refactor.md), [guidelines.md](guidelines.md) | +| Define a public library error type | [guidelines.md](guidelines.md), library/application errors, error propagation, public API evolution | +| Handle top-level CLI/application errors | [guidelines.md](guidelines.md), library/application errors, error propagation, panics | +| Choose enum vs trait vs trait object | [guidelines.md](guidelines.md), enums vs traits, trait design, public API evolution | +| Add a domain ID or validated value | [guidelines.md](guidelines.md), newtypes, constructors, validation | +| Write async service code | [guidelines.md](guidelines.md), async runtime, task lifecycle, shutdown, logging | +| Add instrumentation | [guidelines.md](guidelines.md), logging and observability, error messages | +| Configure formatting, lints, or tests | [guidelines.md](guidelines.md), rustfmt, Clippy, Cargo, CI | +| Review unsafe code or macros | [guidelines.md](guidelines.md), unsafe and macros, public API evolution | + +## Core Behavior + +- Load only the pages the task needs; guideline pages are the policy, workflow pages are the procedures. +- Prefer concrete Rust guidance over language tutorials. +- Keep library/application differences explicit. +- Use the project's OO-leaning Rust default without forcing inheritance-shaped designs. +- Prefer strong, compiler-backed types over primitive-heavy APIs. +- Apply the loaded rules directly. Ask one focused question only when required project context is missing. diff --git a/.fabro/skills/rust-style-guide/guidelines.md b/.fabro/skills/rust-style-guide/guidelines.md new file mode 100644 index 000000000..845c25a7b --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines.md @@ -0,0 +1,78 @@ +# Guidelines + +Load this file for Rust style policy, then load only the guideline pages needed for the task. + +Guideline pages are policy. Do not load every guideline page by default. + +## Foundations + +- [House style and Rust philosophy](guidelines/house-style-and-rust-philosophy.md) - load for overall code shape, OO-leaning defaults, and Rust idiom tradeoffs. +- [Library vs application conventions](guidelines/library-vs-application-conventions.md) - load before choosing policies that differ for libraries, apps, CLIs, tests, or services. +- [Rust edition and MSRV](guidelines/rust-edition-and-msrv.md) - load when setting edition, `rust-version`, stable/nightly posture, or checking MSRV impact. + +## Tooling and Project Shape + +- [rustfmt and formatting](guidelines/rustfmt-and-formatting.md) - load when configuring rustfmt or handling formatting exceptions. +- [rustc and Clippy lints](guidelines/rustc-and-clippy-lints.md) - load when configuring lints, fixing Clippy, or justifying lint exceptions. +- [Cargo, workspaces, features, and dependencies](guidelines/cargo-workspaces-features-and-dependencies.md) - load for workspace layout, features, dependency choices, and MSRV-aware dependency changes. +- [Modules, visibility, and re-exports](guidelines/modules-visibility-and-re-exports.md) - load when changing `mod`, `pub`, facades, re-exports, or public paths. +- [Naming, imports, and prelude policy](guidelines/naming-imports-and-prelude-policy.md) - load for item names, acronym casing, imports, getters, and preludes. +- [Documentation and rustdoc examples](guidelines/documentation-and-rustdoc-examples.md) - load when writing rustdoc, public docs, examples, or `Errors`/`Panics`/`Safety` sections. + +## Type and API Design + +- [Struct design and encapsulation](guidelines/struct-design-and-encapsulation.md) - load when designing structs, fields, invariants, receivers, or encapsulation boundaries. +- [Constructors and builders](guidelines/constructors-and-builders.md) - load when choosing `new`, `try_new`, `Default`, builders, or typestate builders. +- [Newtype pattern and semantic wrappers](guidelines/newtype-pattern-and-semantic-wrappers.md) - load when adding IDs, units, validated strings, value objects, or orphan-rule wrappers. +- [Enums vs traits vs generics vs trait objects](guidelines/enums-vs-traits-vs-generics-vs-trait-objects.md) - load when choosing closed sets, extension points, static dispatch, or dynamic dispatch. +- [Trait design](guidelines/trait-design.md) - load when designing traits, bounds, associated types, blanket impls, sealed traits, or object-safe APIs. +- [Deriving and common trait implementations](guidelines/deriving-and-common-trait-implementations.md) - load when adding derives or manual impls for standard traits. +- [Conversions, getters, and method naming](guidelines/conversions-getters-and-method-naming.md) - load for `From`, `TryFrom`, `AsRef`, `Deref`, accessors, and `as_`/`to_`/`into_` names. +- [Typestate and state machines](guidelines/typestate-and-state-machines.md) - load for ordered workflow states, data-bearing enums, `PhantomData`, or compile-time transitions. +- [Public API evolution](guidelines/public-api-evolution.md) - load for externally consumed APIs, semver, `#[non_exhaustive]`, `#[must_use]`, public fields, or sealed traits. + +## Ownership and Data Flow + +- [Ownership, borrowing, and clone policy](guidelines/ownership-borrowing-and-clone-policy.md) - load when choosing borrowed inputs, owned outputs, `String`/`&str`, `Path` parameters, `IntoIterator`, `AsRef`, `Cow`, accessors, snapshots, or clone tradeoffs. +- [Lifetimes](guidelines/lifetimes.md) - load when explicit lifetimes, borrowed structs, or lifetime-heavy APIs appear. +- [Smart pointers and interior mutability](guidelines/smart-pointers-and-interior-mutability.md) - load when choosing `Box`, `Rc`, `Cell`, `RefCell`, `Weak`, or one-time initialization. +- [Collections and data structures](guidelines/collections-and-data-structures.md) - load when choosing `Vec`, maps, sets, deterministic ordering, capacity, or specialized collection crates. + +## Errors, Safety, and Diagnostics + +- [Error taxonomy and layer boundaries](guidelines/error-taxonomy-and-layer-boundaries.md) - load when defining domain, infrastructure, boundary, or branch-oriented error layers. +- [Library errors vs application errors](guidelines/library-errors-vs-application-errors.md) - load before choosing `thiserror`, `anyhow`, `miette`, or public error stability. +- [Error propagation, context, and messages](guidelines/error-propagation-context-and-messages.md) - load when adding `?`, context, source chains, or error message text. +- [Panics, unwrap, expect, and assertions](guidelines/panics-unwrap-expect-and-assertions.md) - load when using panic, `unwrap`, `expect`, assertions, `unreachable!`, `todo!`, or public panic docs. +- [Validation and invariants](guidelines/validation-and-invariants.md) - load when parsing inputs, enforcing constructors, encoding invariants, or re-checking stale state. +- [Logging and observability](guidelines/logging-and-observability.md) - load when adding `tracing`, spans, fields, levels, error logs, or redaction. + +## Async and Concurrency + +- [Async runtime and when to use async](guidelines/async-runtime-and-when-to-use-async.md) - load when deciding sync vs async posture, Tokio use, or runtime boundaries. +- [Async API design and task lifecycle](guidelines/async-api-design-and-task-lifecycle.md) - load when adding async APIs, async traits, spawning, task owners, `Send`, or shutdown handles. +- [Cancellation, shutdown, and blocking work](guidelines/cancellation-shutdown-and-blocking-work.md) - load for cancellation tokens, `select!`, timeouts, `spawn_blocking`, CPU work, or graceful shutdown. +- [Concurrency primitives](guidelines/concurrency-primitives.md) - load when adding channels, locks, atomics, `Arc` shared state, worker pools, or blocking APIs on async paths. + +## Everyday Implementation + +- [Control flow](guidelines/control-flow.md) - load when choosing `match`, `if let`, `let else`, guards, early returns, combinators, mutable locals, or in-place updates. +- [Option and Result idioms](guidelines/option-and-result-idioms.md) - load when transforming `Option`/`Result`, using `ok_or_else`, `transpose`, `map`, or explicit branching. +- [Iterators, closures, and loops](guidelines/iterators-closures-and-loops.md) - load when choosing iterator chains, loops, closure capture, `collect`, `fold`, or `try_fold`. + +## Testing and Release + +- [Testing and doctests](guidelines/testing-and-doctests.md) - load when writing unit tests, integration tests, doctests, fixtures, or test helpers. +- [Property tests, snapshots, benchmarks, and CI](guidelines/property-tests-snapshots-benchmarks-and-ci.md) - load when configuring test commands, snapshots, property tests, benchmarks, or CI gates. +- [Unsafe code and macros](guidelines/unsafe-code-and-macros.md) - load when touching `unsafe`, FFI, raw pointers, `macro_rules!`, proc macros, or generated APIs. + +## Routing Notes + +- For new Rust project setup, load [workflows/new-rust-project.md](workflows/new-rust-project.md) before individual setup guidelines. +- For reusable library release verification, load [workflows/reusable-library-release.md](workflows/reusable-library-release.md) before individual release guidelines. +- For performance investigation, load [workflows/performance-investigation.md](workflows/performance-investigation.md) before individual performance-related guidelines. +- For code review or refactor work, load [workflows/code-review-refactor.md](workflows/code-review-refactor.md) before individual review guidelines. +- For public API work, always include public API evolution. +- For async service work, include logging and observability. +- For error-handling work, distinguish library errors from application errors before choosing crates. +- For advanced topics like typestate, unsafe, macros, or specialized collections, load the page only when the task directly needs it. diff --git a/.fabro/skills/rust-style-guide/guidelines/async-api-design-and-task-lifecycle.md b/.fabro/skills/rust-style-guide/guidelines/async-api-design-and-task-lifecycle.md new file mode 100644 index 000000000..ccf4dd3ec --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/async-api-design-and-task-lifecycle.md @@ -0,0 +1,105 @@ +# Async API Design and Task Lifecycle + +## Rule + +Design async APIs so task ownership is explicit: applications own spawned tasks and shutdown, while reusable libraries expose awaitable work or return an owner type instead of hiding background tasks. + +## Why + +Spawned tasks can outlive the call that created them. If no API owns cancellation, errors, and joining, work leaks, failures disappear, shutdown becomes unreliable, and tests become timing-dependent. + +## Activation + +Load this page when adding async APIs, spawning Tokio tasks, introducing async traits, adding `Send + 'static` bounds, or changing shutdown behavior. Load the async runtime page first if the project posture is not documented. + +## Do + +- Prefer `async fn` returning `Result` for operations callers should await directly; keep pure helpers synchronous per [async runtime](async-runtime-and-when-to-use-async.md). +- Use async traits only when callers need an abstraction, not just because implementations are async. +- Add `Send + 'static` bounds only when values cross a spawned task, thread, or stored future boundary. +- Keep spawned futures and task-boundary errors `Send + 'static`; `tokio::spawn` requires only `Send + 'static`, and adding `Sync` to erased errors is an interop convention for `anyhow`-style errors, not a spawn requirement. +- Spawn tasks from an owner that stores handles, cancellation tokens, and task-specific state. +- Model long-lived application services, external connections, gateways, pollers, and subscribers as owner structs with `new` and `run`/`shutdown` methods, even when the first version only awaits one client future. +- Name task owner types by responsibility, such as `Poller`, `WorkerSet`, `TaskGroup`, or `Supervisor`. +- Store `JoinHandle>` when task failures must be reported. +- Provide an explicit `shutdown`, `stop`, or `join` method that cancels and awaits owned tasks. +- Pass cancellation or shutdown signals into long-lived loops. +- Attach `tracing` spans or fields that identify the task, entity ID, and operation. +- In reusable libraries, expose `async fn`, futures, streams, or an owner type; let callers decide where task spawning belongs. + +## Avoid + +- Do not call `tokio::spawn` and drop the `JoinHandle` for important work. +- Do not assume dropping a `JoinHandle` cancels the task; it detaches, and the task keeps running, so dropping an owner type without calling `shutdown` leaks the loop unless `Drop` cancels the token. +- Do not hide background tasks inside constructors unless the returned value owns their lifecycle. +- Do not swallow task errors with `let _ = handle.await`. +- Do not spawn in a library merely to make the API look nonblocking. +- Do not add `Send`, `Sync`, or `'static` bounds by habit on ordinary async functions. +- Do not hold non-`Send` values across `.await` in tasks that must run on a multithreaded Tokio runtime. +- Do not let `Rc`, `RefCell`, or non-`Send` guards leak into public futures that should run on Tokio's multithreaded runtime. + +## Library vs Application + +Applications own runtime setup, task spawning, cancellation, shutdown, and joining. They can provide application-level owners for workers, pollers, subscribers, schedulers, and service task groups. + +Use a plain `async fn` for one-shot operations. Use an owner type for long-lived services whose state, lifecycle, or shutdown may grow. + +Libraries should normally return awaitable work and let callers spawn it. If a library truly owns background work, return an owner or guard type that makes shutdown observable and reports task failures. + +## Example + +Prefer an owner type for application background tasks: + +```rust +use tokio::{select, task::JoinHandle}; +use tokio_util::sync::CancellationToken; + +pub struct Poller { + shutdown: CancellationToken, + task: JoinHandle>, +} + +impl Poller { + pub fn start(client: Client) -> Self { + let shutdown = CancellationToken::new(); + let task_shutdown = shutdown.clone(); + + let task = tokio::spawn(async move { + run_poller(client, task_shutdown).await + }); + + Self { shutdown, task } + } + + pub async fn shutdown(self) -> Result<(), PollerError> { + self.shutdown.cancel(); + + match self.task.await { + Ok(result) => result, + Err(error) => Err(PollerError::Join(error)), + } + } +} + +pub async fn run_poller( + client: Client, + shutdown: CancellationToken, +) -> Result<(), PollerError> { + loop { + select! { + () = shutdown.cancelled() => return Ok(()), + result = poll_once(&client) => result?, + } + } +} +``` + +Dropping a `Poller` without calling `shutdown` detaches the task: the loop keeps running until the token is cancelled. + +Reusable libraries should expose the `run_poller`-style future unless they need the owner type for real lifecycle behavior. + +## Exceptions + +- Fire-and-forget spawning is acceptable only for best-effort work where loss is acceptable and documented, such as opportunistic telemetry or cache warming. +- Tests may spawn short-lived tasks when the test owns aborting or joining them. +- Application convenience APIs may spawn internally when they return a value that controls cancellation and shutdown. diff --git a/.fabro/skills/rust-style-guide/guidelines/async-runtime-and-when-to-use-async.md b/.fabro/skills/rust-style-guide/guidelines/async-runtime-and-when-to-use-async.md new file mode 100644 index 000000000..27adfbd3f --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/async-runtime-and-when-to-use-async.md @@ -0,0 +1,83 @@ +# Async Runtime and When to Use Async + +## Rule + +Treat sync vs async as an explicit project-level architecture decision; document the project posture first, and use Tokio when the project chooses async. + +## Why + +Async changes function signatures, trait design, tests, runtime setup, cancellation, shutdown, and dependency choices. It spreads through a codebase, so agents should not introduce or remove async as a local convenience. + +## Activation + +Load this page when choosing or reviewing a project's sync-vs-async posture or when adding the first async dependency. The task-lifecycle, cancellation, and concurrency pages cover the details once the posture is set. + +## Do + +- Check the project's documented async posture before adding async APIs, blocking calls, runtime setup, or spawned tasks. +- Document the posture when it is missing: sync or async. +- Document where async is allowed, such as HTTP handlers, workers, clients, subprocess orchestration, streaming, or background tasks. +- Document runtime conventions: Tokio version/features, test macros, shutdown style, timeout policy, and blocking-work policy. +- Use Tokio for async runtime integration when the project is async. +- Use async for real async work: network I/O, timers, streaming, subprocess orchestration, concurrent service work, and APIs that are already Tokio-based. +- Keep CPU-bound computation, parsing, validation, formatting, and simple local transforms synchronous. +- Use sync helpers inside async code when they are short, CPU-local, and do not block on I/O or hold contended locks; see [concurrency primitives](concurrency-primitives.md) for the lock policy. +- For reusable libraries, make runtime assumptions visible in docs, feature names, or crate-level conventions. + +## Avoid + +- Do not convert a module to async only because the caller is async. +- Do not hide runtime creation inside a reusable library. +- Do not put blocking I/O or long CPU work directly on Tokio worker threads; [cancellation, shutdown, and blocking work](cancellation-shutdown-and-blocking-work.md) owns the isolation rules. +- Do not add runtime-agnostic abstraction after the project has explicitly chosen Tokio and no caller needs another runtime. +- Do not expose async APIs from a library without documenting runtime assumptions. +- Do not maintain parallel sync and async APIs unless both are real project requirements. +- Do not make tests async unless the behavior under test needs async. + +## Library vs Application + +Applications own the runtime, task lifecycle, shutdown, and subscriber setup. Async applications use Tokio when services, workers, clients, or orchestration need async. + +Libraries should not install runtimes or hide task lifecycles. A library may expose Tokio-based APIs when async behavior is central to its purpose, but the runtime dependency should be documented instead of accidental. + +## Example + +Document the project posture near the project rules: + +```markdown +## Async Policy + +This project is async and uses Tokio for HTTP handlers, background workers, +external API clients, timers, and subprocess orchestration. + +Keep parsing, validation, formatting, and pure domain logic synchronous. Do not +add parallel sync and async APIs without an explicit caller requirement. + +Applications own `#[tokio::main]`, task spawning, cancellation, and shutdown. +Library crates may expose async functions but must not create a Tokio runtime. +Use `#[tokio::test]` only for tests that await async behavior. +``` + +Use async at the operation boundary and sync for local computation: + +```rust +pub async fn handle_request(request: Request, client: &ApiClient) -> Result { + let command = parse_command(&request)?; + let record = client.fetch_record(command.record_id()).await?; + Ok(render_response(record)) +} + +fn parse_command(request: &Request) -> Result { + Command::try_new(request.path(), request.query()) +} + +fn render_response(record: Record) -> Response { + Response::from_record(record) +} +``` + +## Exceptions + +- Use a sync posture for CLIs, libraries, or tools whose work is mostly local, CPU-bound, or short-lived. +- Add runtime abstraction only when the project has real callers on multiple runtimes. +- Keep a small sync wrapper around async code only when it is an application convenience and runtime ownership is obvious. The obvious implementation (`Runtime::block_on` or `Handle::block_on`) panics when called from within a runtime, so the wrapper must be reachable only from genuinely synchronous call paths. diff --git a/.fabro/skills/rust-style-guide/guidelines/cancellation-shutdown-and-blocking-work.md b/.fabro/skills/rust-style-guide/guidelines/cancellation-shutdown-and-blocking-work.md new file mode 100644 index 000000000..68075159f --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/cancellation-shutdown-and-blocking-work.md @@ -0,0 +1,103 @@ +# Cancellation, Shutdown, and Blocking Work + +## Rule + +Use cooperative shutdown by default: pass explicit cancellation signals into long-lived async work, race loops with `select!`, join owned tasks, put timeouts at boundaries, and isolate blocking or CPU-bound work from Tokio worker threads. + +## Why + +Async cancellation can happen at any `.await`. Code that ignores cancellation, scatters timeouts, or blocks Tokio workers is harder to shut down cleanly and can make unrelated async work stall. + +## Activation + +Load this page when adding long-lived async loops, graceful shutdown, timeouts, external calls, blocking I/O, CPU-heavy work, or task teardown behavior. + +## Do + +- Pass an explicit shutdown signal, usually a cancellation token, into long-lived tasks. +- Use `select!` in service loops to race normal work with shutdown. +- Join owned tasks during shutdown and surface task errors; task owners and handles are defined on [async API design and task lifecycle](async-api-design-and-task-lifecycle.md). +- Put timeouts at operation boundaries: external calls, subprocesses, requests, jobs, and shutdown phases. +- Keep inner helper functions timeout-free unless they own a real operation boundary. +- Make cancellable sections idempotent or restartable when an `.await` can interrupt progress. +- Treat losing `select!` branches as dropped futures; keep partial reads, buffers, and side effects recoverable. +- Commit external side effects in small, explicit steps with clear retry or rollback behavior. +- Use `tokio::task::spawn_blocking` for blocking filesystem, compression, parsing through blocking APIs, or short CPU-heavy work. +- Use a dedicated pool, work queue, or `rayon` for sustained CPU-bound workloads. +- Drop locks before `.await`, blocking work, callbacks, or expensive computation. +- Log shutdown start, timeout, task failure, and final shutdown outcome with structured fields. + +## Avoid + +- Do not rely on dropping a future as the only shutdown mechanism for important work. +- Do not call blocking I/O, `std::thread::sleep`, or long CPU work directly on Tokio worker threads. +- Do not add `timeout` around every small helper call. +- Do not use `abort` as the normal shutdown path for tasks that need cleanup. +- Do not hold a lock guard across `.await` unless the design explicitly requires an async lock. +- Do not put non-cancel-safe work directly in a `select!` branch without owning the state needed to resume or retry it. +- Do not assume `spawn_blocking` makes unlimited CPU work cheap; it still needs backpressure. +- Do not expect `spawn_blocking` closures to be cancelled once started; cancellation tokens and `abort` do not interrupt them, and runtime shutdown waits for them, so keep blocking sections short or chunked with cancellation checks between chunks. + +## Example + +Race work with shutdown, place the timeout around the external operation, and isolate blocking work: + +```rust +use std::path::PathBuf; +use std::time::Duration; + +use tokio::sync::mpsc; +use tokio::time::timeout; +use tokio::{select, task}; +use tokio_util::sync::CancellationToken; + +pub async fn run_worker( + mut jobs: mpsc::Receiver, + shutdown: CancellationToken, + client: Client, +) -> Result<(), WorkerError> { + loop { + let job = select! { + () = shutdown.cancelled() => return Ok(()), + maybe_job = jobs.recv() => match maybe_job { + Some(job) => job, + None => return Ok(()), + }, + }; + + process_job(&client, job).await?; + } +} + +async fn process_job(client: &Client, job: Job) -> Result<(), WorkerError> { + let record = timeout( + Duration::from_secs(10), + client.fetch(job.record_id()), + ) + .await + .map_err(|_| WorkerError::FetchTimedOut { + record_id: job.record_id(), + })??; + + let digest = hash_file(job.path()).await?; + client.store_digest(record.id(), digest).await?; + + Ok(()) +} + +async fn hash_file(path: PathBuf) -> Result { + task::spawn_blocking(move || Digest::from_file(path)) + .await + .map_err(WorkerError::HashJoin)? + .map_err(WorkerError::Hash) +} +``` + +Shutdown interrupts only the idle wait: a job that has been received is driven to completion, bounded by the timeout inside `process_job`. Race in-progress work against shutdown only when something owns the state needed to resume or retry it. + +## Exceptions + +- Use `abort` for teardown of best-effort tasks that do not own external state and do not need cleanup. +- Let short-lived request tasks complete naturally when the caller already owns cancellation through request drop or timeout. +- Use shorter inner timeouts only when a lower-level operation has an independent service-level objective or resource limit. +- Keep CPU-heavy work on Tokio only when it is known to be tiny and bounded. diff --git a/.fabro/skills/rust-style-guide/guidelines/cargo-workspaces-features-and-dependencies.md b/.fabro/skills/rust-style-guide/guidelines/cargo-workspaces-features-and-dependencies.md new file mode 100644 index 000000000..1212a72f5 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/cargo-workspaces-features-and-dependencies.md @@ -0,0 +1,88 @@ +# Cargo, Workspaces, Features, and Dependencies + +## Rule + +Keep Cargo configuration explicit: use workspaces for shared policy, add dependencies deliberately, keep library features additive and minimal, and verify dependency changes against the declared MSRV. + +## Why + +Cargo choices shape compile time, public API, downstream compatibility, binary size, and release stability. Agents should avoid convenience changes that quietly become long-term constraints. + +## Do + +- Use a workspace when multiple crates share version, edition, dependencies, lints, or profiles. +- Put shared dependency versions in `[workspace.dependencies]`. +- Put shared lint policy in `[workspace.lints]`. +- Use conservative dependency policy for libraries. +- Use pragmatic dependency policy for applications when a dependency materially improves clarity or reliability. +- Prefer mature, maintained crates for domain behavior over small convenience crates. +- For application CLIs with subcommands, environment-backed options, generated help, or user-facing argument errors, prefer `clap` derive. Hand parsing is only for tiny private binaries with trivial arguments. +- Keep reusable library features additive and opt-in. +- Make `serde` optional for reusable libraries unless serialization is core to the crate. +- Verify reusable library changes with `--all-features` so feature-gated code stays compiled, linted, and tested. +- Check MSRV after adding dependencies or using newly stabilized APIs; [Rust edition and MSRV](rust-edition-and-msrv.md) owns the MSRV policy and verification command. + +## Avoid + +- Do not add a dependency for a trivial wrapper around `std`. +- Do not expose dependency types in public APIs unless that dependency is part of the intended contract. +- Do not use mutually exclusive Cargo features. +- Do not make default library features pull in heavy optional integrations. +- Do not add feature flags before there is a real optional integration. +- Do not derive serialization for a public type without deciding its wire-format compatibility policy. + +## Library vs Application + +Libraries should minimize default dependencies and keep feature flags additive. Applications can depend directly on the concrete crates they use and usually do not need feature flags around internal implementation details. + +For libraries, treat public dependency exposure and MSRV bumps as compatibility decisions. For applications, still keep `rust-version` honest, but prefer simple direct configuration over library-style feature plumbing. + +Treat serialized formats as API contracts. Choose field names, enum representation, defaults, and unknown-field behavior deliberately before publishing data that other processes or versions must read. + +## Example + +Use the new project workflow for initial workspace scaffolding. This page covers how to keep Cargo configuration simple after the project exists. + +Library with additive optional integration: + +```toml +[package] +name = "example-id" +edition.workspace = true +rust-version.workspace = true + +[dependencies] +serde = { workspace = true, optional = true } +thiserror.workspace = true + +[features] +serde = ["dep:serde"] +``` + +```rust +#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct RunId(String); +``` + +Async application with direct concrete dependencies: + +```toml +[package] +name = "example-service" +edition.workspace = true +rust-version.workspace = true + +[dependencies] +anyhow.workspace = true +tokio = { version = "1", features = ["full"] } +tracing.workspace = true +``` + +## Exceptions + +- Use a heavier dependency when it is the mature ecosystem standard for the domain. +- Use default features in a library when the crate is intentionally batteries-included and downstream compile impact is acceptable. +- Use exact or pinned dependency versions only when reproducibility, upstream breakage, or security response requires it. +- Split a crate from the workspace only when it has a truly different release, MSRV, or dependency policy. +- Use a documented feature matrix instead of `--all-features` only when a crate intentionally supports mutually incompatible feature sets. diff --git a/.fabro/skills/rust-style-guide/guidelines/collections-and-data-structures.md b/.fabro/skills/rust-style-guide/guidelines/collections-and-data-structures.md new file mode 100644 index 000000000..cebf728ed --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/collections-and-data-structures.md @@ -0,0 +1,102 @@ +# Collections and Data Structures + +## Rule + +Use standard-library collections by default; add specialized collection crates only when required semantics, deterministic ordering, or known performance needs justify them. + +## Why + +Standard collections are familiar, well-tested, dependency-free, and usually fast enough. Specialized collections are useful when they express real behavior, but they should not become incidental dependencies. + +## Do + +- Use `Vec` for ordered, indexable, append-heavy lists. +- Use `VecDeque` for queue-like data that pushes and pops at both ends. +- Use `HashMap` and `HashSet` for unordered lookup. +- Use `BTreeMap` and `BTreeSet` when sorted iteration or deterministic order matters. +- Sort a `Vec` before output when deterministic order is only needed at the boundary. +- Use capacity hints such as `Vec::with_capacity` when the size is already known. +- Use `retain`, `drain`, and `std::mem::take` for clear in-place collection updates. +- Use `entry(key).or_insert_with(...)` or `or_default()` for map insert-or-update instead of a `contains_key` check followed by `insert`, the double lookup clippy's `map_entry` flags. +- Use newtypes around collections when the collection has domain invariants or behavior. +- Add crates such as `indexmap`, `smallvec`, or domain-specific data structures only when their semantics or measured performance matter. + +## Avoid + +- Do not add collection crates just because they are convenient in one small spot. +- Do not use `HashMap` when iteration order affects tests, logs, serialization, or public output. +- Do not use `BTreeMap` only because it feels more stable if lookup performance or ordering does not matter. +- Do not use `Vec` for repeated front removal; use `VecDeque`. +- Do not expose raw collection fields when the collection has invariants. +- Do not preallocate capacity when the estimate is guesswork. +- Do not optimize collection choice before the data size and access pattern are known. + +## Public API Notes + +Public APIs should prefer standard-library collection types unless another collection type is part of the API's real semantics. Exposing a specialized collection type makes that crate part of the public contract. + +Return iterators or owned standard collections when that keeps the API independent of internal storage. + +## Example + +```rust +use std::collections::{BTreeMap, HashMap, VecDeque}; + +#[derive(Clone, Debug, Default)] +pub struct JobQueue { + pending: VecDeque, +} + +impl JobQueue { + pub fn push(&mut self, job: Job) { + self.pending.push_back(job); + } + + pub fn pop(&mut self) -> Option { + self.pending.pop_front() + } +} + +#[derive(Clone, Debug, Default)] +pub struct UserIndex { + by_id: HashMap, +} + +impl UserIndex { + pub fn insert(&mut self, user: User) { + self.by_id.insert(user.id, user); + } + + pub fn get(&self, id: UserId) -> Option<&User> { + self.by_id.get(&id) + } + + pub fn display_names_by_id(&self) -> BTreeMap { + self.by_id + .iter() + .map(|(id, user)| (*id, user.name.clone())) + .collect() + } +} + +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub struct UserId(u64); + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct User { + id: UserId, + name: String, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Job { + id: UserId, +} +``` + +## Exceptions + +- Use `IndexMap` when insertion order is part of the data model or stable output is required while preserving insertion order. +- Use `SmallVec`, arena allocators, or specialized collections when profiling or domain knowledge shows allocation or layout matters. +- Use domain-specific crates for well-known data structures that are hard to implement correctly. +- Use deterministic collections in tests when order stability keeps assertions clear. diff --git a/.fabro/skills/rust-style-guide/guidelines/concurrency-primitives.md b/.fabro/skills/rust-style-guide/guidelines/concurrency-primitives.md new file mode 100644 index 000000000..f8937165f --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/concurrency-primitives.md @@ -0,0 +1,139 @@ +# Concurrency Primitives + +## Rule + +Choose the simplest primitive by ownership shape: owned values first, channels for ownership transfer, standard-library locks for short synchronous critical sections, Tokio locks only for async waiting, and dedicated CPU/blocking work tools when work is not async I/O. + +## Why + +Concurrency primitives encode ownership and scheduling choices. Picking the smallest primitive that matches the shape of the data keeps async code predictable and avoids blocking Tokio workers by accident. + +## Activation + +Load this page when adding channels, locks, atomics, worker pools, shared state, runtime boundaries, or CPU parallelism. + +## Do + +- Prefer one clear owner for mutable state. +- Use channels when a value or command should move to an owning task or worker. +- Use bounded channels when producers can outrun consumers. +- Use `Arc` for shared ownership across threads or Tokio tasks. +- Use `std::sync::Mutex` or `std::sync::RwLock` for short, synchronous critical sections. +- Use `tokio::sync::Mutex`, `RwLock`, `Semaphore`, `Notify`, or channels when awaiting for coordination is part of the design. +- Keep lock scopes small and copy or clone owned data out before `.await`. +- Start with `Mutex`; use `RwLock` only when read-heavy access and contention make it worthwhile. +- Use atomics only for simple counters, flags, and low-level coordination with obvious ordering. +- Use `spawn_blocking` for bounded blocking work from async code. +- Use `rayon`, a dedicated pool, or a work queue for sustained CPU-bound work. +- Document lock ordering when more than one lock can be held at once. + +## Avoid + +- Do not choose `tokio::sync::Mutex` only because the surrounding function is async. +- Do not hold a standard-library lock guard across `.await`. +- Do not use `Arc>` to avoid deciding who owns the state. +- Do not use channels for simple shared counters or snapshots. +- Do not use unbounded channels unless memory growth is impossible or intentionally accepted. +- Do not use `RwLock` as a default replacement for `Mutex`. +- Do not put blocking I/O, subprocesses, sleep, or long CPU work directly on Tokio worker threads. +- Do not use `std::thread::spawn` from Tokio code unless a dedicated OS thread is intentional and documented. + +## Async Notes + +Async projects should enforce blocking-API bans with `clippy::disallowed_methods` and `clippy::disallowed_types`; the lint tables in [the new project workflow](../workflows/new-rust-project.md) are the baseline. Both lints match item paths, not modules: list functions such as `std::thread::sleep`, `std::thread::spawn`, and `std::process::Command::new` under `disallowed_methods`, and types or traits such as `std::net::TcpStream` and `std::io::Read` under `disallowed_types`. + +Do not treat those lints as a blanket ban on `std::sync`. Standard-library locks are fine in async code when the critical section is short, does not block, and the guard is dropped before `.await`. + +## Example + +Use a standard lock for quick shared state, and do async work outside the lock: + +```rust +use std::sync::{Arc, Mutex}; + +#[derive(Clone, Debug)] +pub struct SharedMetrics { + inner: Arc>, +} + +impl SharedMetrics { + pub fn record(&self, event: Event) { + let mut metrics = self.inner.lock().expect("metrics mutex poisoned"); + metrics.record(event); + } + + pub fn snapshot(&self) -> Metrics { + self.inner + .lock() + .expect("metrics mutex poisoned") + .clone() + } +} + +pub async fn handle_job( + client: &Client, + metrics: &SharedMetrics, + job: Job, +) -> Result<(), Error> { + let record = client.fetch(job.record_id()).await?; + metrics.record(Event::Fetched); + + process(record).await?; + metrics.record(Event::Processed); + + Ok(()) +} +``` + +Use a channel when ownership should move to a worker: + +```rust +use tokio::sync::mpsc; + +pub struct JobQueue { + sender: mpsc::Sender, +} + +impl JobQueue { + pub async fn enqueue(&self, job: Job) -> Result<(), QueueClosed> { + self.sender.send(job).await.map_err(|_| QueueClosed) + } +} + +pub async fn run_worker(mut jobs: mpsc::Receiver) -> Result<(), Error> { + while let Some(job) = jobs.recv().await { + process_job(job).await?; + } + + Ok(()) +} +``` + +Bad: hold a lock while doing blocking or async work. + +```rust +let mut cache = cache.lock().expect("cache mutex poisoned"); +let path = cache.entry(key).or_insert_with(default_path).clone(); +let bytes = std::fs::read(path)?; +client.upload(bytes).await?; +``` + +Good: copy the needed value out, drop the lock, and isolate blocking work. + +```rust +let path = { + let mut cache = cache.lock().expect("cache mutex poisoned"); + cache.entry(key).or_insert_with(default_path).clone() +}; + +let bytes = tokio::task::spawn_blocking(move || std::fs::read(path)).await??; +client.upload(bytes).await?; +``` + +## Exceptions + +- Use Tokio locks when a task must wait asynchronously for shared state or a guard must intentionally live across `.await`. +- Use `std::sync::RwLock` or `tokio::sync::RwLock` when measured or obvious read contention justifies it. +- Use dedicated OS threads for blocking APIs that require thread affinity or long-lived blocking ownership, with a local `#[expect]` reason if lints disallow it. +- Use unbounded channels only for naturally bounded streams or explicit best-effort telemetry paths. +- Use channels even for same-thread code when ownership transfer makes control flow clearer. diff --git a/.fabro/skills/rust-style-guide/guidelines/constructors-and-builders.md b/.fabro/skills/rust-style-guide/guidelines/constructors-and-builders.md new file mode 100644 index 000000000..31b8847f2 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/constructors-and-builders.md @@ -0,0 +1,149 @@ +# Constructors and Builders + +## Rule + +Use `new` and `try_new` for required fields, add builders when optional configuration makes call sites clearer, and reserve typestate builders for important invariants. + +## Why + +Simple constructors keep invariants close to the type. Builders are useful when names and defaults matter, but they add API surface. Typestate can prevent invalid states at compile time, but it is too much machinery for ordinary configuration. + +## Do + +- Use `new` for infallible construction from required values. +- Use `try_new` when construction validates caller input or can fail; reserve `parse` for `FromStr`-backed textual parsing. +- Keep validation inside the constructor or `build` method. +- Use `Default` only when there is an obvious, useful default value. +- Use a builder when a type has several optional fields, many defaults, or call sites would otherwise pass booleans and `None` values. +- Prefer consuming builder setters like `fn timeout(mut self, value: Duration) -> Self` for owned configuration builders. +- Use `with_*` for derived variants or optional modifications, not as a substitute for a clear primary constructor. +- Use typestate builders only when the compile-time ordering protects an important invariant or prevents a dangerous operation; for workflow state machines, follow [typestate and state machines](typestate-and-state-machines.md). + +## Avoid + +- Do not add a builder for every struct by habit. +- Do not make fields public just to avoid writing a constructor. +- Do not write a `new` function that panics or unwraps on caller-provided input. +- Do not use long constructors with boolean flags or repeated `None` arguments. +- Do not encode ordinary optional configuration with typestate. +- Do not use `Default` when the value would be surprising, invalid, or environment-dependent. + +## Public API Notes + +For public libraries, constructors and builders are part of the stable API. If a type is likely to gain optional settings over time, prefer a builder before adding many constructor parameters. + +Adding a required constructor parameter is usually a breaking change. Adding an optional builder method is usually easier to evolve. + +## Example + +```rust +use std::time::Duration; + +#[derive(Clone, Debug)] +pub struct RetryPolicy { + max_attempts: u32, + backoff: Duration, +} + +impl RetryPolicy { + pub fn try_new(max_attempts: u32, backoff: Duration) -> Result { + if max_attempts == 0 { + return Err(RetryPolicyError::NoAttempts); + } + + Ok(Self { + max_attempts, + backoff, + }) + } + + pub fn max_attempts(&self) -> u32 { + self.max_attempts + } + + pub fn backoff(&self) -> Duration { + self.backoff + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum RetryPolicyError { + NoAttempts, +} + +#[derive(Clone, Debug)] +pub struct ClientOptions { + timeout: Duration, + retry_policy: RetryPolicy, + user_agent: Option, +} + +impl ClientOptions { + pub fn new(timeout: Duration, retry_policy: RetryPolicy) -> Self { + Self { + timeout, + retry_policy, + user_agent: None, + } + } + + pub fn builder() -> ClientOptionsBuilder { + ClientOptionsBuilder::default() + } + + pub fn timeout(&self) -> Duration { + self.timeout + } +} + +#[derive(Clone, Debug)] +#[must_use] +pub struct ClientOptionsBuilder { + timeout: Duration, + retry_policy: RetryPolicy, + user_agent: Option, +} + +impl Default for ClientOptionsBuilder { + fn default() -> Self { + Self { + timeout: Duration::from_secs(30), + retry_policy: RetryPolicy::try_new(3, Duration::from_millis(200)) + .expect("default retry policy is valid"), + user_agent: None, + } + } +} + +impl ClientOptionsBuilder { + pub fn timeout(mut self, timeout: Duration) -> Self { + self.timeout = timeout; + self + } + + pub fn retry_policy(mut self, retry_policy: RetryPolicy) -> Self { + self.retry_policy = retry_policy; + self + } + + pub fn user_agent(mut self, user_agent: impl Into) -> Self { + self.user_agent = Some(user_agent.into()); + self + } + + pub fn build(self) -> ClientOptions { + ClientOptions { + timeout: self.timeout, + retry_policy: self.retry_policy, + user_agent: self.user_agent, + } + } +} +``` + +## Exceptions + +- Use public fields and struct literals for plain data types with no invariants. +- Use `&mut self` builder methods when matching an existing API style or when callers need to reuse the builder. +- Use generated builder crates only when the project already depends on them or has enough builder-heavy types to justify the dependency. +- Use typestate for important protocols, state machines, or safety boundaries where invalid ordering should not compile. diff --git a/.fabro/skills/rust-style-guide/guidelines/control-flow.md b/.fabro/skills/rust-style-guide/guidelines/control-flow.md new file mode 100644 index 000000000..83b7d2a88 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/control-flow.md @@ -0,0 +1,122 @@ +# Control Flow + +## Rule + +Use clarity-first branching: prefer `?`, `let else`, `if let`, and `match` to make branches and exits explicit, and keep mutation in small, validated scopes. + +## Why + +Control flow carries invariants, error paths, and state transitions. Explicit branches and small mutable scopes are easier for agents to modify safely than clever expression chains, hidden exits, or partially updated state. + +## Do + +- Use `?` when the local code only needs to propagate a fallible result. +- Use early returns for invalid inputs, missing prerequisites, and permission checks. +- Use `let else` when a required pattern must be present and the fallback exits the current scope. +- Use `if let` when only one pattern needs special handling. +- Use `while let` for loops that repeatedly consume optional or result-like values. +- Use `match` when multiple variants matter, exhaustiveness matters, or each branch has distinct behavior. +- Keep `match` arms small; extract a helper when a branch grows past the local decision. +- Prefer naming meaningful enum variants over `_` when future variants should force a revisit. +- Use match guards only when the guard is short and directly tied to the arm. +- Keep the main path linear after validation and setup. +- Use `let mut` for local accumulators, builders, counters, and staged values; keep mutable scopes small and return to immutable locals once setup is complete. +- Validate fallible inputs before mutating long-lived state; prefer computing a new value locally and assigning it once when that avoids partial updates. +- Use `std::mem::take` or `std::mem::replace` when moving a field out while leaving the struct valid. +- Treat Clippy as authoritative for local control-flow idioms; refactor instead of adding local bypasses ([rustc and Clippy lints](rustc-and-clippy-lints.md)). + +## Avoid + +- Do not write combinator chains that hide branching or side effects; [Option and Result idioms](option-and-result-idioms.md) owns the combinator-vs-branching line. +- Do not use `match` on `bool`; use `if` with a named condition. +- Do not use `_` to ignore meaningful domain states. +- Do not deeply nest `if` or `match` blocks when guard clauses would make exits clearer. +- Do not use `let else` when the fallback contains substantial recovery logic; use `match`. +- Do not replace explicit error handling with `unwrap` or `expect`. +- Do not force a functional style when a small mutable local is clearer. +- Do not mutate object state before fallible validation unless the partial state is intentional and documented. + +## Example + +Prefer visible exits and exhaustive domain handling: + +```rust +pub fn plan_action(request: Request) -> Result { + let Some(user_id) = request.user_id() else { + return Err(Error::MissingUserId); + }; + + let command = Command::parse(request.command())?; + + if !request.permissions().can_run(&user_id, &command) { + return Err(Error::Forbidden { user_id }); + } + + let action = match command { + Command::Start { target } => { + let target = Target::try_new(target)?; + Action::Start { target } + } + Command::Stop { target } => Action::Stop { target }, + Command::Status => Action::Status, + }; + + Ok(action) +} +``` + +Validate first, then mutate the owned state in a small block: + +```rust +pub struct UserAccount { + email: EmailAddress, + labels: Vec, + active: bool, +} + +impl UserAccount { + pub fn update(&mut self, update: UserUpdate) -> Result<(), Error> { + let email = match update.email() { + Some(value) => Some(EmailAddress::try_new(value)?), + None => None, + }; + + let mut labels = Vec::new(); + for label in update.labels() { + labels.push(Label::try_new(label)?.into_string()); + } + + if let Some(email) = email { + self.email = email; + } + + self.labels = labels; + + if update.deactivate() { + self.active = false; + } + + Ok(()) + } +} +``` + +Use combinators for simple local transformations: + +```rust +impl User { + pub fn display_name(&self) -> String { + self.nickname() + .filter(|name| !name.is_empty()) + .unwrap_or_else(|| self.username()) + .to_owned() + } +} +``` + +## Exceptions + +- Use combinators when the transformation is short, linear, and side-effect free. +- Use `_` for intentionally ignored variants in tests, logging, metrics, or external `#[non_exhaustive]` enums. +- Use a `match` even for two cases when it documents a domain state machine or prepares for likely new variants. +- Mutate as you go when each step is independently valid and there is no meaningful rollback requirement. diff --git a/.fabro/skills/rust-style-guide/guidelines/conversions-getters-and-method-naming.md b/.fabro/skills/rust-style-guide/guidelines/conversions-getters-and-method-naming.md new file mode 100644 index 000000000..9969847bd --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/conversions-getters-and-method-naming.md @@ -0,0 +1,101 @@ +# Conversions, Getters, and Method Naming + +## Rule + +Use `From` only for infallible conversions and `TryFrom` or `FromStr` for validated ones, and follow Rust naming so method names carry ownership expectations: `as_` borrows, `to_` allocates, `into_` consumes, and accessors use bare field names. + +## Why + +Rust method names carry ownership and allocation expectations, and conversion trait impls become part of the public API. Consistent names and honest conversions let callers reason about cost and failure without reading function bodies. + +Parameter and return ownership defaults live on [ownership, borrowing, and clone policy](ownership-borrowing-and-clone-policy.md). + +## Do + +- Use `From` for infallible, obvious conversions. +- Use `TryFrom` or `FromStr` for validation and fallible parsing. +- Use `From` for lossless numeric widening and `TryFrom` or `TryInto` for narrowing or signedness changes. +- Choose explicit integer overflow behavior with `checked_*`, `saturating_*`, `wrapping_*`, or `overflowing_*` when overflow is possible and meaningful. +- Use `as_*` for cheap borrowed or scalar views. +- Use `to_*` for cloning, allocation, or conversion without consuming `self`. +- Use `into_*` for consuming conversions. +- Use Rust-style accessors such as `id()`, `name()`, and `status()` instead of `get_id()`; borrow unless returning a small `Copy` value. +- Use predicate names for booleans: `is_active()`, `has_children()`, `can_retry()`. + +## Avoid + +- Do not use `From` for conversions that can fail, validate, allocate surprisingly, or lose important meaning. +- Do not use `as` for narrowing numeric casts or float-to-integer conversion unless range, sign, and NaN behavior are checked locally. +- Do not use `as_*` for methods that allocate or clone. +- Do not use `==` for approximate float equality; use a named tolerance, and use `total_cmp` when sorting floats that may include NaN. +- Do not use `get_*` for simple field-like accessors. +- Do not generate accessors for every private field by habit. +- Do not implement `Deref` just to forward methods from an inner value. + +## Public API Notes + +Trait impls such as `From`, `TryFrom`, `AsRef`, and `Deref` become part of the public API. Add them only when the conversion semantics are stable. + +## Example + +```rust +use std::fmt; +use std::str::FromStr; + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ProjectName(String); + +impl ProjectName { + pub fn try_new(value: &str) -> Result { + let value = value.trim(); + + if value.is_empty() { + return Err(ProjectNameError::Empty); + } + + Ok(Self(value.to_owned())) + } + + pub fn as_str(&self) -> &str { + &self.0 + } + + pub fn to_slug(&self) -> String { + self.0.to_ascii_lowercase().replace(' ', "-") + } + + pub fn into_string(self) -> String { + self.0 + } +} + +impl fmt::Display for ProjectName { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(self.as_str()) + } +} + +impl FromStr for ProjectName { + type Err = ProjectNameError; + + fn from_str(value: &str) -> Result { + Self::try_new(value) + } +} + +impl From for String { + fn from(name: ProjectName) -> Self { + name.into_string() + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ProjectNameError { + Empty, +} +``` + +## Exceptions + +- Use `get_*` for keyed lookups, cache retrieval, or fallible or computed access where the method is not simple field-like observation. +- Return owned snapshots from methods whose names signal ownership, such as `snapshot`, `to_*`, or `*_snapshot`. diff --git a/.fabro/skills/rust-style-guide/guidelines/deriving-and-common-trait-implementations.md b/.fabro/skills/rust-style-guide/guidelines/deriving-and-common-trait-implementations.md new file mode 100644 index 000000000..cf9324de5 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/deriving-and-common-trait-implementations.md @@ -0,0 +1,95 @@ +# Deriving and Common Trait Implementations + +## Rule + +Derive standard traits when their semantics are obvious, hand-write `Display`, and avoid deriving semantics-heavy traits by habit. + +## Why + +Derived impls are cheap and correct when the type's structure matches the trait semantics. They become misleading when equality, ordering, defaults, debug output, or cloning require domain judgment. + +## Do + +- Derive `Debug` for ordinary data types. +- Hand-write `Debug` for secret-bearing types or types whose internals should not leak. +- Derive `Clone` when the type has value semantics and clone cost is acceptable. +- Derive `Copy` only for small scalar-like types with no ownership, resource, or surprising duplication behavior. +- Derive `PartialEq` and `Eq` when field-by-field equality is the domain equality. +- Derive `Hash` only when equality and hashing should use the same stable fields. +- Derive `Ord` and `PartialOrd` only when there is one obvious total ordering. +- Keep hand-written `PartialEq`, `Eq`, `Hash`, and `Ord` coherent: `a == b` must imply equal hashes, every impl must use the same fields, and mixing a manual `PartialEq` with a derived `Hash` silently breaks `HashMap` and `HashSet` lookups. +- Derive or implement `Default` only when the default is valid, useful, and unsurprising. +- Hand-write `Display` for stable user-facing text. + +## Avoid + +- Do not derive traits just to satisfy a test, log statement, or temporary call site. +- Do not derive `Debug` for tokens, credentials, or secret-bearing structs. +- Do not derive `Copy` for types that may grow owned data or represent scarce resources. +- Do not derive `Ord` when ordering is arbitrary or caller-specific. +- Do not derive `Default` when the result would be invalid, empty-but-broken, or environment-dependent. +- Do not use `Display` for programmer diagnostics; use `Debug` for that. +- Do not derive external serialization traits unless the wire format is intentionally part of the type's role. + +## Public API Notes + +For public libraries, trait impls are part of the API surface. Removing a public impl is breaking, and adding broad impls can affect downstream method resolution or trait coherence. Derive only traits the type is meant to support over time. + +## Example + +```rust +use std::{fmt, num::NonZeroU64}; + +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub struct UserId(NonZeroU64); + +impl UserId { + pub fn new(value: NonZeroU64) -> Self { + Self(value) + } + + pub fn as_u64(self) -> u64 { + self.0.get() + } +} + +impl fmt::Display for UserId { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(formatter, "{}", self.0) + } +} + +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub enum RetryMode { + Disabled, + #[default] + Standard, + Aggressive, +} + +#[derive(Clone, Eq, PartialEq)] +pub struct ApiToken(String); + +impl ApiToken { + pub fn new(value: impl Into) -> Self { + Self(value.into()) + } + + pub fn expose_secret(&self) -> &str { + &self.0 + } +} + +impl fmt::Debug for ApiToken { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("ApiToken()") + } +} +``` + +## Exceptions + +- Keep impl surface smaller for public types whose long-term semantics are not settled. +- Derive additional traits for test-only helper types when the trait does not leak into production API. +- Hand-write equality, hashing, or ordering when the domain semantics differ from field-by-field behavior. +- Derive `Default` for configuration structs when all field defaults are valid and match the documented behavior. diff --git a/.fabro/skills/rust-style-guide/guidelines/documentation-and-rustdoc-examples.md b/.fabro/skills/rust-style-guide/guidelines/documentation-and-rustdoc-examples.md new file mode 100644 index 000000000..a1c202e47 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/documentation-and-rustdoc-examples.md @@ -0,0 +1,81 @@ +# Documentation and Rustdoc Examples + +## Rule + +Document non-obvious public API behavior; when the project intentionally maintains rustdoc examples, write them as fallible snippets that use `?` instead of `unwrap`. + +## Why + +Rustdoc should explain intent, contracts, and caveats that names and types cannot express. Over-documenting obvious items adds noise, while maintained examples that panic teach careless error handling. + +## Do + +- Add rustdoc when a public item has non-obvious behavior, invariants, caveats, side effects, or examples. +- Use module docs (`//!`) for modules that define an important concept or public surface. +- Use item docs (`///`) for public types, traits, functions, and methods whose contract is not obvious. +- Include `# Errors` when a public `Result` function has caller-relevant failure modes. +- Include `# Panics` when a public function can panic. +- Include `# Safety` for every `unsafe` function or unsafe trait. +- Add rustdoc examples only when they materially clarify public API use and the project has opted into maintaining them. +- When rustdoc examples are used, prefer snippets that compile and use `?`. +- Hide boilerplate with `#` lines when it distracts from the example. + +## Avoid + +- Do not require `#![deny(missing_docs)]` as house style. +- Do not restate the name in prose. +- Do not document private helpers unless the explanation prevents mistakes. +- Do not use doctests as default test coverage. +- Do not use bare `unwrap` in public rustdoc examples. +- Do not include long examples that become harder to maintain than the API. +- Do not mark examples `ignore` just to avoid maintaining them; move behavior coverage to normal tests instead. + +## Public API Notes + +For reusable libraries, prioritize docs on public concepts, constructors, fallible operations, trait contracts, and behavior that affects callers. Internal application crates may keep docs sparse unless the module is a shared boundary or the behavior is easy to misuse. + +## Example + +```rust +use std::path::Path; + +/// Loads application configuration from a TOML file. +/// +/// Environment-specific overrides are applied after the file is parsed. +/// +/// # Errors +/// +/// Returns an error if the file cannot be read, the TOML is invalid, or a +/// required setting is missing. +/// +/// # Examples +/// +/// ```rust,no_run +/// # use example_config::Config; +/// # fn main() -> Result<(), Box> { +/// let config = Config::load("app.toml")?; +/// assert_eq!(config.profile(), "default"); +/// # Ok(()) +/// # } +/// ``` +pub fn load(path: impl AsRef) -> Result { + todo!() +} +``` + +Use `expect` only for setup invariants that are part of the example: + +```rust +/// ``` +/// # use example_config::Config; +/// let config = Config::from_static(include_str!("../../fixtures/app.toml")) +/// .expect("fixture app.toml should be valid"); +/// assert_eq!(config.profile(), "default"); +/// ``` +``` + +## Exceptions + +- Use `no_run` for examples that should compile but would start servers, make network calls, or read or mutate real state. +- Use `ignore` only when an example cannot be made portable. +- Use `expect` in examples for fixed fixtures or impossible setup failures when a fallible `main` would obscure the API being shown. diff --git a/.fabro/skills/rust-style-guide/guidelines/enums-vs-traits-vs-generics-vs-trait-objects.md b/.fabro/skills/rust-style-guide/guidelines/enums-vs-traits-vs-generics-vs-trait-objects.md new file mode 100644 index 000000000..0d9b60e17 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/enums-vs-traits-vs-generics-vs-trait-objects.md @@ -0,0 +1,100 @@ +# Enums vs Traits vs Generics vs Trait Objects + +## Rule + +Use enums for closed sets, traits for open extension points, generics for static dispatch, and `dyn Trait` for runtime heterogeneity. + +## Why + +These choices encode different extension models. Enums make known variants explicit and exhaustively checked. Traits allow new implementors. Generics keep dispatch static when one implementor type flows through a call. Trait objects trade static dispatch for runtime selection and mixed collections. + +## Do + +- Use an enum when all variants are known to this crate or module. +- Put behavior directly on a closed enum when callers should not add new variants. +- Use a trait when downstream code or another layer should be able to provide new behavior. +- Use `impl Trait` or `T: Trait` when a function accepts one concrete implementor type at a time. +- Use `&dyn Trait`, `Box`, or `Arc` for plugin lists, runtime selection, or heterogeneous collections. +- Keep object-safety in mind when a trait is meant to be used as `dyn Trait`. +- Prefer returning concrete types or `impl Trait` unless callers need runtime polymorphism. + +## Avoid + +- Do not create a trait just because several closed enum variants share method names. +- Do not use a growing enum when external users are expected to add variants. +- Do not spread generic type parameters through many layers when a trait object would localize the choice. +- Do not use `dyn Trait` just to avoid writing a generic parameter. +- Do not make a public trait object API from a trait that is not object-safe. + +## Public API Notes + +For public libraries, choosing an enum means the crate controls the set of variants. Adding a variant can require downstream match updates unless the enum is marked `#[non_exhaustive]`. + +Choosing a public trait means outside crates may implement it. Adding required methods later is usually a breaking change, so keep public traits small and intentional. + +## Example + +```rust +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum DeliveryTarget { + Email(EmailAddress), + Webhook(WebhookUrl), +} + +impl DeliveryTarget { + pub fn kind(&self) -> &'static str { + match self { + Self::Email(_) => "email", + Self::Webhook(_) => "webhook", + } + } +} + +pub trait Notifier { + fn notify(&self, message: &Message) -> Result<(), NotifyError>; +} + +pub fn notify_once(notifier: &N, message: &Message) -> Result<(), NotifyError> +where + N: Notifier, +{ + notifier.notify(message) +} + +pub struct Broadcast { + notifiers: Vec>, +} + +impl Broadcast { + pub fn new(notifiers: Vec>) -> Self { + Self { notifiers } + } + + pub fn notify_all(&self, message: &Message) -> Result<(), NotifyError> { + for notifier in &self.notifiers { + notifier.notify(message)?; + } + + Ok(()) + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct EmailAddress(String); + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct WebhookUrl(String); + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Message(String); + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct NotifyError; +``` + +## Exceptions + +- Use a trait for a small closed set when the behavior must be supplied by generic infrastructure that already expects a trait. +- Use an enum wrapper around trait objects when the public API needs a closed high-level category but each category uses runtime dispatch internally. +- Use `dyn Trait` in application code when runtime configuration matters more than static dispatch. +- Use generics in public APIs only when the caller benefits from type flexibility and the extra type parameter does not leak complexity. diff --git a/.fabro/skills/rust-style-guide/guidelines/error-propagation-context-and-messages.md b/.fabro/skills/rust-style-guide/guidelines/error-propagation-context-and-messages.md new file mode 100644 index 000000000..893c5dda1 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/error-propagation-context-and-messages.md @@ -0,0 +1,117 @@ +# Error Propagation, Context, and Messages + +## Rule + +Propagate errors with `?`, add context at operation and layer boundaries, keep inner propagation sparse when typed errors already explain the local failure, and never stringify a source error just to add context. + +## Why + +Good error chains explain both the local cause and the larger operation. Too little context hides what the program was trying to do; context on every fallible line creates noisy, repetitive chains. + +## Do + +- Use `?` for normal propagation. +- Use `From` or `#[from]` when converting a source error without adding extra fields. +- Use `.context(...)` for static application context. +- Use `.with_context(...)` when the context formats values or clones data. +- Add context at command, request, job, service, task, crate, or layer boundaries. +- Include safe identifiers such as paths, IDs, operation names, and remote resource names when they help diagnose the failure. +- Preserve source chains with `#[source]`, `#[from]`, `anyhow::Context`, or explicit source fields. +- Write context messages as concise operation descriptions, such as `failed to load configuration`. +- Keep typed error `Display` messages specific to the variant's local failure. +- Walk the source chain explicitly when rendering typed errors at a boundary that should show causes. + +## Avoid + +- Do not add context to every `?` by habit. +- Do not add context that only restates the lower-level error. +- Do not write `.map_err(|err| err.to_string())`. +- Do not write `.map_err(|err| anyhow::anyhow!("{err}"))`. +- Do not interpolate the source error into a new context string. +- Do not turn internal propagation messages into final user-facing copy. +- Do not put secrets, credentials, raw tokens, or unredacted request bodies in error messages. + +## Library vs Application + +Libraries should prefer typed errors whose variants describe local failures and preserve sources. Applications should add `anyhow` context at meaningful operation boundaries and let the final CLI, API, worker, or log boundary decide how much of the chain to render. + +## Example + +Library code describes local failures: + +```rust +use std::{ + io, + path::{Path, PathBuf}, +}; + +#[derive(Debug, thiserror::Error)] +pub enum ConfigError { + #[error("reading configuration file {path}")] + Read { + path: PathBuf, + + #[source] + source: io::Error, + }, + + #[error("parsing configuration file {path}")] + Parse { + path: PathBuf, + + #[source] + source: toml::de::Error, + }, +} + +pub fn load_config(path: &Path) -> Result { + let contents = std::fs::read_to_string(path).map_err(|source| ConfigError::Read { + path: path.to_path_buf(), + source, + })?; + + toml::from_str(&contents).map_err(|source| ConfigError::Parse { + path: path.to_path_buf(), + source, + }) +} +``` + +Good: application code adds boundary context and preserves the source: + +```rust +use std::path::PathBuf; + +use anyhow::{Context, Result}; + +fn run() -> Result<()> { + let path = PathBuf::from("config.toml"); + let config = config_lib::load_config(&path) + .with_context(|| format!("failed to load configuration from {}", path.display()))?; + + start_server(config).context("failed to start server")?; + Ok(()) +} +``` + +At the outermost boundary, render an `anyhow` chain with the alternate format (`{err:#}`) or by returning `Result` from `main`; `Display` on `anyhow::Error` prints only the outermost context. + +```rust +#[expect(clippy::print_stderr, reason = "top-level CLI error report")] +fn report_error(error: &anyhow::Error) { + eprintln!("error: {error:#}"); +} +``` + +Bad: flatten the source into text and lose the chain: + +```rust +let config = config_lib::load_config(&path) + .map_err(|err| anyhow::anyhow!("failed to load config: {err}"))?; +``` + +## Exceptions + +- Add context close to a fallible call when there is no meaningful higher boundary that can explain the operation. +- Add more context in quick scripts when it improves debugging and does not create repetitive chains. +- Keep propagation minimal in very small typed libraries where variants and sources already make the operation obvious. diff --git a/.fabro/skills/rust-style-guide/guidelines/error-taxonomy-and-layer-boundaries.md b/.fabro/skills/rust-style-guide/guidelines/error-taxonomy-and-layer-boundaries.md new file mode 100644 index 000000000..64e8e192c --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/error-taxonomy-and-layer-boundaries.md @@ -0,0 +1,94 @@ +# Error Taxonomy and Layer Boundaries + +## Rule + +Use layered, branch-oriented errors: model domain failures where callers branch, convert infrastructure errors at boundaries, preserve source chains and data, and render errors to strings only at external boundaries. + +## Why + +Error values are structured control-flow and diagnostics. Turning errors into strings inside Rust code drops type information, source chains, and useful fields before the right boundary can decide how to log, display, redact, or recover. + +## Do + +- Create typed domain variants for failures callers can act on, such as not found, duplicate, forbidden, invalid state, or validation failure. +- Keep infrastructure causes as error sources with `#[source]` or `#[from]` when using `thiserror`. +- Keep useful fields on error variants, such as IDs, paths, states, retry hints, and safe context values. +- Convert lower-layer errors into the current layer's error type at crate, domain, service, command, or API boundaries. +- Add context at layer crossings so operators can tell which operation failed. +- Preserve `source()` chains until a rendering boundary; [error propagation](error-propagation-context-and-messages.md) owns how to render the chain. +- Render to `String` only for CLI output, API response details, logs, telemetry, serialized files, or external contracts that require text. +- For public API responses, log the internal chain but return a curated safe message. + +## Avoid + +- Do not add enum variants for every low-level failure unless callers branch on them. +- Do not expose database, HTTP, SDK, or parser errors from a public domain API unless that dependency is intentionally part of the contract. +- Do not transport internal errors as `String`, `Message(String)`, or `Other(String)` just because the real error type is inconvenient. +- Do not stringify errors during propagation; the `.map_err(to_string)` and `anyhow!("{err}")` bans live on [error propagation](error-propagation-context-and-messages.md). +- Do not include secrets, tokens, raw URLs with credentials, or unredacted request bodies in error fields or display messages. + +## Library vs Application + +Reusable libraries should expose typed errors for their public boundary and keep implementation details behind variants or sources. Internal application code may use `anyhow`, but it should keep typed domain errors where code needs to branch and should not stringify errors before the final rendering boundary. + +## Example + +```rust +use std::path::PathBuf; + +#[derive(Debug, thiserror::Error)] +pub enum LoadProfileError { + #[error("profile {id} was not found")] + NotFound { id: ProfileId }, + + #[error("reading profile file {path}")] + Read { + path: PathBuf, + + #[source] + source: std::io::Error, + }, + + #[error("parsing profile file {path}")] + Parse { + path: PathBuf, + + #[source] + source: toml::de::Error, + }, +} + +pub fn load_profile(id: ProfileId) -> Result { + let path = profile_path(id); + let contents = std::fs::read_to_string(&path) + .map_err(|source| LoadProfileError::Read { + path: path.clone(), + source, + })?; + + toml::from_str(&contents).map_err(|source| LoadProfileError::Parse { path, source }) +} +``` + +At the boundary, render or serialize deliberately: + +```rust +fn to_api_error(err: LoadProfileError) -> ApiError { + match &err { + LoadProfileError::NotFound { id } => { + tracing::warn!(error = ?err, "profile not found"); + ApiError::not_found(format!("profile {id} not found")) + } + _ => { + tracing::error!(error = ?err, "failed to load profile"); + ApiError::internal("failed to load profile") + } + } +} +``` + +## Exceptions + +- Use a coarse error variant when callers cannot make a different decision and the source chain carries the detail. +- Use text-only errors at external boundaries that are already rendered projections. +- Use cloneable domain errors or a shared error wrapper before falling back to `String` for clone-bound storage. diff --git a/.fabro/skills/rust-style-guide/guidelines/house-style-and-rust-philosophy.md b/.fabro/skills/rust-style-guide/guidelines/house-style-and-rust-philosophy.md new file mode 100644 index 000000000..25f4eafe3 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/house-style-and-rust-philosophy.md @@ -0,0 +1,100 @@ +# House Style and Rust Philosophy + +## Rule + +Write idiomatic Rust with an OO-leaning default: model domain concepts as structs with methods and encapsulated invariants, compose behavior explicitly, and choose loops or iterator chains by clarity. + +## Why + +Rust supports data with behavior without inheritance. Clear types, ownership, and explicit composition give agents useful structure without forcing object-oriented patterns that do not fit Rust. + +## Do + +- Start with domain types instead of primitive-heavy APIs when the value has meaning. +- Put behavior on the type that owns the data or invariant. +- Keep fields private unless the type is plain data with no invariants. +- Prefer direct composition with explicit fields and methods. +- Use small, behavior-focused traits for open extension points. +- Use iterator chains for simple transformations and loops for branching, mutation, early exits, or multi-step logic; see [iterators, closures, and loops](iterators-closures-and-loops.md). +- Keep parsing, normalization, validation, and command behavior on the domain type that owns the data when there is a natural receiver. + +## Avoid + +- Do not emulate inheritance hierarchies with traits, enums, or nested structs. +- Do not split all behavior into stateless helper functions when methods would make ownership and invariants clearer. +- Do not expose free functions as public API merely to make tests reach private behavior. +- Do not create pass-through wrapper types whose main job is forwarding. +- Do not add delegation crates or macros to hide a confused boundary. +- Do not choose pattern names over Rust's simpler type, module, and ownership tools. + +## Example + +```rust +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct Money { + cents: u64, +} + +impl Money { + pub const ZERO: Self = Self { cents: 0 }; + + pub fn checked_add(self, other: Self) -> Option { + self.cents + .checked_add(other.cents) + .map(|cents| Self { cents }) + } +} + +pub struct CartItem { + price: Money, + requires_shipping: bool, +} + +impl CartItem { + pub fn new(price: Money, requires_shipping: bool) -> Self { + Self { + price, + requires_shipping, + } + } + + pub fn price(&self) -> Money { + self.price + } + + pub fn requires_shipping(&self) -> bool { + self.requires_shipping + } +} + +pub struct Cart { + items: Vec, +} + +impl Cart { + pub fn add_item(&mut self, item: CartItem) { + self.items.push(item); + } + + pub fn total(&self) -> Option { + let mut total = Money::ZERO; + + for item in &self.items { + total = total.checked_add(item.price())?; + } + + Some(total) + } + + pub fn shippable_items(&self) -> impl Iterator { + self.items.iter().filter(|item| item.requires_shipping()) + } +} +``` + +## Exceptions + +- Use free functions for pure algorithms or cross-type operations with no natural receiver; if a helper must be public, first ask whether it should be a method or a value type. +- Use plain data structs with public fields when the fields are the API and there are no invariants to protect. +- Prefer a functional pipeline over methods when a transformation chain is genuinely clearer than stateful updates. +- Introduce a trait before a second implementation exists only when callers need substitution or a testing seam now. diff --git a/.fabro/skills/rust-style-guide/guidelines/iterators-closures-and-loops.md b/.fabro/skills/rust-style-guide/guidelines/iterators-closures-and-loops.md new file mode 100644 index 000000000..095924df4 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/iterators-closures-and-loops.md @@ -0,0 +1,91 @@ +# Iterators, Closures, and Loops + +## Rule + +Use iterator chains for simple transformations and loops for branching, mutation, early exits, or multi-step logic; treat Clippy as authoritative for local iterator-vs-loop idioms. + +## Why + +Iterator chains are compact when they read as a pipeline. Loops are clearer when the code carries state, exits early, performs side effects, or needs named intermediate steps. + +## Do + +- Use `.iter()`, `.iter_mut()`, and `.into_iter()` intentionally based on whether the code borrows, mutates, or consumes values. +- Use `map`, `filter`, `filter_map`, `flat_map`, `find`, `any`, `all`, and `position` when they directly name the operation. +- Use `collect` when the target collection is clear; add a type annotation when inference makes the result hard to see. +- Collect fallible maps with `collect::, _>>()` (or the `Option` equivalent) to fail fast on the first error; reserve `try_fold` for accumulation that carries state. +- Use `try_fold` or `try_for_each` for short fallible accumulation or validation when it stays readable. +- Use `for` loops for branching, mutation, early `break`/`continue`, multiple accumulators, or nontrivial error handling. +- Keep closures short; extract a named helper when a closure has branching, side effects, or reused logic. +- Use `move` closures when a closure outlives the current scope, is spawned, or ownership is clearer than borrowing. +- Clone into closures when that avoids awkward lifetimes and the cost is not known to matter. +- Prefer `enumerate` and `zip` over manual index tracking when pairing is direct. + +## Avoid + +- Do not write long iterator chains that hide control flow. +- Do not use `for_each` for side-effect-heavy loops when a `for` loop is clearer. +- Do not use `fold` with a complex mutable accumulator when a loop communicates the state better. +- Do not `collect` into a temporary collection only to iterate over it once. +- Do not hide logging, metrics, mutation, or I/O inside `map` or `filter` closures. +- Do not rely on dense closure inference when a named helper or local type annotation would clarify intent. + +## Example + +Use an iterator pipeline for simple extraction: + +```rust +pub fn active_names(runs: &[Run]) -> Vec { + runs.iter() + .filter(|run| run.is_active()) + .map(|run| run.name().to_owned()) + .collect() +} +``` + +Use a loop when the code branches, accumulates state, and can fail: + +```rust +pub fn failed_runs(runs: &[Run]) -> Result, Error> { + let mut failed = Vec::new(); + + for run in runs { + if !run.is_finished() { + continue; + } + + let Some(exit_status) = run.exit_status() else { + continue; + }; + + if exit_status.success() { + continue; + } + + failed.push(FailedRun { + id: run.id(), + reason: failure_reason(run, exit_status)?, + }); + } + + Ok(failed) +} +``` + +Use `try_fold` only when fallible accumulation stays compact: + +```rust +pub fn total_size(files: &[FileEntry]) -> Result { + files.iter().try_fold(0_u64, |total, file| { + total + .checked_add(file.size()?) + .ok_or(Error::SizeOverflow) + }) +} +``` + +## Exceptions + +- Use a loop for a simple transform when Clippy or the project lint set prefers it. +- Use an iterator chain for branching logic only when each step is named clearly and Clippy accepts it. +- Use `for_each` for fluent APIs where side effects are intentionally local and Clippy does not object. diff --git a/.fabro/skills/rust-style-guide/guidelines/library-errors-vs-application-errors.md b/.fabro/skills/rust-style-guide/guidelines/library-errors-vs-application-errors.md new file mode 100644 index 000000000..5545bd6dc --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/library-errors-vs-application-errors.md @@ -0,0 +1,97 @@ +# Library Errors vs Application Errors + +## Rule + +Expose typed errors from reusable library boundaries, usually with `thiserror`; use `anyhow` inside applications and CLIs, and use `miette` only when rich user-facing diagnostics are worth the extra structure. + +## Why + +Library callers need stable types they can inspect and branch on. Applications usually need fast propagation, useful context, and deliberate rendering at the final boundary. + +## Do + +- Define a crate-local `Error` enum and `Result` alias when a library crate has one cohesive error surface. +- Use `thiserror::Error` for ordinary typed errors. +- Keep public error variants branch-oriented, not a dump of every dependency failure. +- Preserve causes with `#[source]` or `#[from]`. +- Keep useful structured fields on typed errors instead of folding them into `String`. +- Add `#[non_exhaustive]` to public error enums that may grow in a published API. +- Use `anyhow::Result` in binaries, command handlers, workers, tests, and internal application glue. +- Add application context with `.context(...)` or `.with_context(...)` instead of stringifying the source error. +- Use `miette` for CLI diagnostics that benefit from labels, source snippets, help text, or polished reports. +- Convert to `miette` only at the presentation layer: std and `thiserror` errors do not cross `?` into `miette::Report` without `IntoDiagnostic::into_diagnostic()` or `#[derive(Diagnostic)]`, so keep internal errors on `thiserror` or `anyhow`. +- Keep typed domain errors in application code when code branches on the failure. + +## Avoid + +- Do not expose `anyhow::Error` from reusable library APIs. +- Do not use `miette` as a general internal application error type. +- Do not mix `anyhow` and `eyre` in the same application without a project-level reason. +- Do not make `Box` the default public error strategy. +- Do not leak dependency error types from public APIs or stringify errors between layers; [error taxonomy](error-taxonomy-and-layer-boundaries.md) and [error propagation](error-propagation-context-and-messages.md) own those rules. +- Do not create public variants only to mirror each dependency error. + +## Public API Notes + +`thiserror` is usually fine for public libraries because it generates standard trait impls without becoming part of function signatures. Be more careful with the fields on public error variants: exposed source types can make dependencies part of the public contract. + +For published crates, prefer stable domain variants and hide implementation details when callers should not depend on them. For internal application crates, optimize for clarity and accept breaking error-shape changes. + +## Example + +Library crate: + +```rust +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum ConfigError { + #[error("configuration file {path} was not found")] + NotFound { + path: std::path::PathBuf, + + #[source] + source: std::io::Error, + }, + + #[error("reading configuration file {path}")] + Read { + path: std::path::PathBuf, + + #[source] + source: std::io::Error, + }, + + #[error("configuration value {key} is invalid")] + InvalidValue { key: String }, +} + +pub type Result = std::result::Result; + +pub fn load_config(path: &std::path::Path) -> Result { + let contents = match std::fs::read_to_string(path) { + Ok(contents) => contents, + Err(source) if source.kind() == std::io::ErrorKind::NotFound => { + return Err(ConfigError::NotFound { + path: path.to_path_buf(), + source, + }); + } + Err(source) => { + return Err(ConfigError::Read { + path: path.to_path_buf(), + source, + }); + } + }; + + parse_config(&contents) +} +``` + +For the application-boundary side (`anyhow` context over a typed library error), see the example on [error propagation, context, and messages](error-propagation-context-and-messages.md). + +## Exceptions + +- Use hand-written error impls when avoiding a dependency or tightly controlling a public API matters. +- Use `anyhow` in internal libraries that are only application implementation details and are not consumed as reusable APIs. +- Use `miette` at the CLI presentation layer when the diagnostic output is part of the product experience. diff --git a/.fabro/skills/rust-style-guide/guidelines/library-vs-application-conventions.md b/.fabro/skills/rust-style-guide/guidelines/library-vs-application-conventions.md new file mode 100644 index 000000000..61aebdfa0 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/library-vs-application-conventions.md @@ -0,0 +1,55 @@ +# Library vs Application Conventions + +## Rule + +Identify the code context first: reusable library, shared in-repo crate, application or service, CLI, or test code. Libraries optimize for stable, caller-controlled APIs; applications, CLIs, and tests optimize for delivery and local clarity. + +## Why + +Library choices become another crate's constraints, while application choices optimize for delivery, observability, and deployment. Most policies in this guide split on this classification, so classifying wrong applies the wrong half of every other page. + +## Do + +- Classify code before choosing policies: published or reusable library, shared in-repo workspace crate, application or service, CLI, or test support. +- Treat public library APIs as long-lived contracts; treat application internals as freely refactorable with their callers. +- Follow the owner page for each policy that splits by context: + - Errors: typed `thiserror` errors at library boundaries, `anyhow` inside applications; see [library errors vs application errors](library-errors-vs-application-errors.md). + - Instrumentation: libraries emit `tracing` events, applications own subscriber setup; see [logging and observability](logging-and-observability.md). + - Async: applications own the runtime, spawned tasks, and shutdown; see [async runtime](async-runtime-and-when-to-use-async.md) and [task lifecycle](async-api-design-and-task-lifecycle.md). + - Dependencies and features: conservative for libraries, pragmatic for applications; see [Cargo, workspaces, features, and dependencies](cargo-workspaces-features-and-dependencies.md). + - API evolution: semver care only for externally consumed code; see [public API evolution](public-api-evolution.md). + +## Avoid + +- Do not force library-level abstraction into application code when one concrete type is enough. +- Do not over-model one-off CLI failure paths with large public error enums. +- Do not apply application shortcuts, such as global process setup or `anyhow` in signatures, to reusable library boundaries. +- Do not treat shared in-repo crates as published libraries; they follow application rules until something outside the repo consumes them independently. + +## Library vs Application + +Library code protects caller choice where it affects API stability: typed errors, careful dependency exposure, documented runtime assumptions, and no global process setup. + +Application and CLI code chooses concrete dependencies directly and owns process-wide setup: runtime, subscribers, configuration, and shutdown. + +## Example + +The same operation, classified two ways: + +```rust +// Reusable library boundary: typed error, no process-wide assumptions. +pub fn parse_manifest(source: &str) -> Result { + todo!() +} + +// Application command handler: concrete choices, anyhow at the boundary. +pub async fn run_deploy(args: DeployArgs) -> anyhow::Result<()> { + todo!() +} +``` + +## Exceptions + +- Keep application internals typed when the caller must recover differently from different failures. +- Use a library-specific dependency when it is part of the crate's purpose and documented API. +- Use lighter examples or test helpers in tests when production error and logging structure would obscure the behavior under test. diff --git a/.fabro/skills/rust-style-guide/guidelines/lifetimes.md b/.fabro/skills/rust-style-guide/guidelines/lifetimes.md new file mode 100644 index 000000000..e3dbfb807 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/lifetimes.md @@ -0,0 +1,81 @@ +# Lifetimes + +## Rule + +Prefer lifetime elision, and reserve explicit lifetimes for APIs where borrowing is the point: views, parsers, iterators, and zero-copy abstractions. + +## Why + +Explicit lifetimes are valuable for borrowed views into another value, but they add coupling that agents often spread too far through signatures and structs. Most APIs are easier to call and refactor when they own returned data; the borrow/own/clone defaults live on [ownership, borrowing, and clone policy](ownership-borrowing-and-clone-policy.md). + +## Do + +- Rely on lifetime elision for ordinary `&self`, `&str`, `&[T]`, and `&Path` APIs. +- Use lifetime-bearing structs only for real borrowed views into another value. +- Name lifetimes when an output borrow must clearly be tied to a particular input borrow. +- Use `'_` when the lifetime exists but does not need a name in the local API. +- Use iterator lifetimes such as `impl Iterator + '_` when returning borrowed iteration is the natural API. +- Keep lifetime parameters local; do not push them through unrelated types. + +## Avoid + +- Do not use self-referential structs in ordinary code. +- Do not add named lifetimes where elision communicates the relationship. +- Do not make public APIs lifetime-heavy unless borrowing is the point of the abstraction. + +## Public API Notes + +Published library APIs may use explicit lifetimes when the crate is fundamentally a parser, view, iterator, or zero-copy abstraction. For ordinary libraries and application code, keep lifetime complexity low and prefer owned outputs. + +## Example + +```rust +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Document { + body: String, +} + +impl Document { + pub fn new(body: &str) -> Self { + Self { + body: body.to_owned(), + } + } + + pub fn words(&self) -> impl Iterator + '_ { + self.body.split_whitespace() + } + + pub fn first_word(&self) -> Option> { + first_token(&self.body) + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct Token<'a> { + text: &'a str, +} + +impl Token<'_> { + pub fn as_str(&self) -> &str { + self.text + } +} + +pub fn first_token(input: &str) -> Option> { + input + .split_whitespace() + .next() + .map(|text| Token { text }) +} + +pub fn owned_tokens(input: &str) -> Vec { + input.split_whitespace().map(str::to_owned).collect() +} +``` + +## Exceptions + +- Use explicit lifetimes for borrowed views, parsers, iterators, and APIs where zero-copy behavior is the main value. +- Use lifetime-bearing structs for short-lived adapters that cannot outlive their source. +- Accept more lifetime complexity in measured hot paths where allocation cost is known to matter. diff --git a/.fabro/skills/rust-style-guide/guidelines/logging-and-observability.md b/.fabro/skills/rust-style-guide/guidelines/logging-and-observability.md new file mode 100644 index 000000000..da7f75579 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/logging-and-observability.md @@ -0,0 +1,118 @@ +# Logging and Observability + +## Rule + +Use `tracing` for structured operation traces: spans for operations, fields for IDs and state, events for meaningful milestones and failures, and fixed message strings instead of prose-only logs. + +## Why + +Structured traces make logs searchable, aggregatable, and useful after the fact. Fixed messages identify event kinds, while fields carry the data that changes per run. + +## Do + +- Use `tracing` everywhere; applications configure subscribers and libraries only emit spans and events. +- Add spans around meaningful operations such as requests, jobs, commands, tasks, external calls, and workflow steps. +- Prefer `#[tracing::instrument(skip_all, fields(...))]` for function-shaped spans, opting fields in explicitly. +- Attach structured fields for IDs, names, states, attempts, counts, durations, and safe error summaries. +- Use fixed message strings; put variable data in fields. +- Write log messages in lowercase with no trailing period, matching the style of error `Display` and `anyhow` context messages. +- Keep INFO low-volume and high-signal: startup, shutdown, operation start/end, and key outcomes. +- Use DEBUG for investigation detail: branches taken, retries, resolved config, request metadata, and intermediate state. +- Use WARN for degraded behavior or retryable unexpected conditions. +- Use ERROR when the current operation failed and cannot continue. +- Log errors in an `error` field and render or collect full cause chains deliberately at boundaries that need them. +- Record error values with Debug capture (`?err`) or a `&dyn Error` field so source chains stay visible; Display capture (`%err`) prints only the top-level message and drops the chain. +- Use snake_case field names consistently across the codebase. +- Prefer counts, byte lengths, hashes, redacted displays, or booleans over raw sensitive values. + +## Avoid + +- Do not interpolate variable values into the message string. +- Do not log prose-only messages when fields would make the event queryable. +- Do not duplicate events already emitted by a parent operation or domain event. +- Do not log hot loops, per-token streams, or high-cardinality chatter at INFO. +- Do not configure a subscriber inside reusable libraries. +- Do not use tracing events as user-facing CLI or API output. +- Do not log secrets, API keys, bearer tokens, cookies, raw credentials, unredacted URLs, raw command output, or request bodies. +- Do not use bare `#[instrument]` on functions that take configs or credentials; it records every argument via Debug. +- Do not rely on logs for behavior that should be represented as durable events, metrics, or user-visible output. + +## Library vs Application + +Libraries may depend on `tracing` and emit events, but they should not initialize global subscribers or choose output formats. Applications own subscriber setup, filtering, formatting, destinations, and propagation to worker processes. + +## Example + +```rust +use tracing::{debug, error, info, warn, Instrument}; + +pub async fn sync_account(account_id: AccountId, client: &BillingClient) -> Result<(), SyncError> { + let span = tracing::info_span!("sync_account", account_id = %account_id); + + async move { + info!("starting account sync"); + + let invoices = client + .list_invoices(account_id) + .await + .map_err(SyncError::ListInvoices)?; + + debug!(invoice_count = invoices.len(), "listed invoices"); + + for invoice in invoices { + if invoice.is_stale() { + warn!(invoice_id = %invoice.id(), "skipping stale invoice"); + continue; + } + + client + .sync_invoice(&invoice) + .await + .map_err(SyncError::SyncInvoice)?; + } + + info!("account sync complete"); + Ok(()) + } + .instrument(span) + .await +} + +pub fn log_sync_failure(account_id: AccountId, error: &SyncError) { + error!(account_id = %account_id, error = ?error, "account sync failed"); +} +``` + +Prefer this shape over interpolated messages: + +```rust +info!(account_id = %account_id, invoice_count = count, "account sync complete"); +``` + +Avoid: + +```rust +info!("account {account_id} sync complete with {count} invoices"); +``` + +Bad: log secrets or unredacted high-cardinality data. + +```rust +info!("calling {url} with bearer token {token}"); +``` + +Good: log safe fields and fixed messages. + +```rust +info!( + host = %request.host(), + token_present = request.token().is_some(), + "calling upstream" +); +``` + +## Exceptions + +- Send user-facing CLI output through the command's output path (writer, printer, or table renderer), not developer logs. `print_stdout`/`print_stderr` are warn-level lints enforced in CI; where raw `println!`/`eprintln!` is right (curated help, fatal pre-exit message), annotate the site with `#[expect(clippy::print_stdout, reason = "...")]`. +- Add more DEBUG detail temporarily while investigating a hard problem, then keep only the durable signal. +- Use metrics or durable domain events instead of logs when data must drive alerts, billing, audit, or product behavior. diff --git a/.fabro/skills/rust-style-guide/guidelines/modules-visibility-and-re-exports.md b/.fabro/skills/rust-style-guide/guidelines/modules-visibility-and-re-exports.md new file mode 100644 index 000000000..feb4f512d --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/modules-visibility-and-re-exports.md @@ -0,0 +1,100 @@ +# Modules, Visibility, and Re-exports + +## Rule + +Keep modules and fields private by default, expose focused public facades, give each local item one intended public path, and avoid broad preludes unless the crate is a broad ecosystem crate. + +## Why + +Rust visibility is an API design tool. Smaller public surfaces make invariants easier to protect and let crates reorganize internals without breaking callers. + +## Do + +- Make modules private unless callers need the module path as part of the API. +- Keep struct fields private by default; [struct design](struct-design-and-encapsulation.md) owns the public-fields-for-plain-data exception. +- Use `pub(crate)` for real internal boundaries across modules. +- Use `pub(super)` only for tight parent-child module collaboration. +- Re-export the public types callers should name from the crate root or a focused facade module. +- Choose one canonical public path for each local item: either a facade re-export or a public module path. +- Use `#[doc(inline)]` when re-exporting from a public module or another crate so rustdoc presents the item at the facade path; re-exports from private modules are inlined automatically. +- Keep internal helper modules behind `mod`, not `pub mod`. + +## Avoid + +- Do not expose deep module paths by accident. +- Do not use `pub` when `pub(crate)` is enough. +- Do not create a prelude for a small crate. +- Do not re-export every internal type from the crate root. +- Do not expose the same local type through both a deep public module and a facade path by accident. +- Do not make module layout mirror implementation churn in the public API. + +## Public API Notes + +For libraries, every `pub` item is part of the compatibility contract unless hidden behind documented instability. Prefer a small public facade that names the crate's main concepts and hides helper modules. + +When a facade is the intended public API, keep implementation modules private and re-export the public item from the facade. If a deep module is itself a stable namespace, expose the module and avoid also re-exporting the same local item from the root unless the duplicate path is an intentional compatibility or ergonomics choice. + +For applications, `pub(crate)` is often enough for cross-module use. Avoid public exports from binary crates unless integration tests or generated code require them. + +## Example + +```rust +// lib.rs +mod client; +mod error; +mod request; +mod response; + +pub use client::Client; +pub use error::ClientError; +pub use request::Request; +pub use response::Response; +``` + +```rust +// client.rs +mod retry; +mod transport; + +use url::Url; + +use crate::{ClientError, Request, Response}; + +pub struct Client { + transport: transport::Transport, +} + +impl Client { + pub fn new(base_url: Url) -> Self { + Self { + transport: transport::Transport::new(base_url), + } + } + + pub async fn send(&self, request: Request) -> Result { + retry::with_retry(|| self.transport.send(&request)).await + } +} +``` + +```rust +// request.rs +pub struct Request { + path: String, +} + +impl Request { + pub fn new(path: impl Into) -> Self { + Self { path: path.into() } + } + + pub fn path(&self) -> &str { + &self.path + } +} +``` + +## Exceptions + +- Use `pub mod` when the module itself is a stable namespace callers should browse or import from. +- Add a `prelude` only when the crate has many commonly paired traits and types and users benefit from one import. diff --git a/.fabro/skills/rust-style-guide/guidelines/naming-imports-and-prelude-policy.md b/.fabro/skills/rust-style-guide/guidelines/naming-imports-and-prelude-policy.md new file mode 100644 index 000000000..71473841c --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/naming-imports-and-prelude-policy.md @@ -0,0 +1,72 @@ +# Naming, Imports, and Prelude Policy + +## Rule + +Use idiomatic Rust names, explicit module-level imports grouped by rustfmt, selective Rust-style accessors, and no broad prelude by default. + +## Why + +Consistent names and imports make code easier for agents to scan and modify. Rust-style accessors and focused imports keep APIs explicit without falling back to Java-style getters or hidden prelude-heavy dependencies. + +## Do + +- Use Rust-style acronym casing: `HttpClient`, `UrlParser`, `JsonBody`, `ApiToken`. +- Use `SCREAMING_SNAKE_CASE` for constants and statics. +- Use explicit module-level imports. +- Let rustfmt group imports with `group_imports = "StdExternalCrate"` and `imports_granularity = "Module"`. +- Prefer `as _` imports for extension traits used only for methods. +- Name accessors and conversions per [conversions, getters, and method naming](conversions-getters-and-method-naming.md): `id()` not `get_id()`, predicates like `is_active()`. + +## Avoid + +- Do not write all-caps acronyms inside type names like `HTTPClient` or `URLParser`. +- Do not use broad glob imports in production modules. +- Do not rely on a broad crate prelude for ordinary application or library code. + +## Example + +```rust +use std::path::Path; + +use anyhow::{Context as _, Result}; + +use crate::{Config, EmailAddress, RunId, RunStatus, Timestamp, UserId}; + +pub struct User { + id: UserId, + email: EmailAddress, + active: bool, +} + +impl User { + pub fn id(&self) -> UserId { + self.id + } + + pub fn email(&self) -> &EmailAddress { + &self.email + } + + pub fn is_active(&self) -> bool { + self.active + } +} + +#[derive(Clone, Debug)] +pub struct RunSummary { + pub id: RunId, + pub status: RunStatus, + pub started_at: Timestamp, + pub finished_at: Option, +} + +pub fn load_config(path: &Path) -> Result { + Config::load(path).context("loading config") +} +``` + +## Exceptions + +- Use wildcard imports in tests, test support, or third-party prelude APIs when they improve test readability. +- Add a crate `prelude` only for broad ecosystem crates where users commonly need many traits and types together. +- Preserve conventional uppercase names required by external protocols, generated code, or wire formats. diff --git a/.fabro/skills/rust-style-guide/guidelines/newtype-pattern-and-semantic-wrappers.md b/.fabro/skills/rust-style-guide/guidelines/newtype-pattern-and-semantic-wrappers.md new file mode 100644 index 000000000..3e3bf5850 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/newtype-pattern-and-semantic-wrappers.md @@ -0,0 +1,111 @@ +# Newtype Pattern and Semantic Wrappers + +## Rule + +Use newtypes for IDs, units, validated values, and public API meaning; avoid wrapping primitives when the wrapper adds no useful type safety or behavior. + +## Why + +Newtypes make invalid argument swaps harder, keep validation attached to the value, and give public APIs domain names without committing callers to raw primitive meaning. + +## Do + +- Use tuple structs for small semantic wrappers around primitives. +- Keep newtype fields private when the type has meaning, validation, or future API concerns. +- Use `new` for infallible wrappers and `try_new` for validated wrappers, following [constructors and builders](constructors-and-builders.md). +- Expose focused accessors such as `as_str`, `as_u64`, or `into_inner`. +- Derive standard traits when semantics are obvious: `Debug`, `Clone`, `Copy`, `Eq`, `PartialEq`, `Hash`, `PartialOrd`, `Ord` (`Ord` always requires `PartialOrd`). +- Implement `Display` when the wrapper has a stable user-facing representation. +- Use `From` only for conversions that cannot fail or violate invariants. +- Use `TryFrom` or `FromStr` for validated conversions. +- Use `#[repr(transparent)]` only when layout guarantees matter, such as FFI or carefully documented ABI boundaries. + +## Avoid + +- Do not wrap every primitive by default. +- Do not expose the inner value as a public field for invariant-bearing wrappers. +- Do not implement `Deref` to `str`, `String`, `Vec`, or other primitives just to inherit methods. +- Do not add `From` implementations that skip validation. +- Do not use vague wrapper names like `Value`, `Key`, or `Id` outside a narrow module where the domain is obvious. +- Do not create a newtype if a plain private field inside a behavior-bearing struct communicates the invariant better. + +## Public API Notes + +Public library APIs should use newtypes more readily than application internals when primitive arguments can be confused or have domain meaning. A `UserId` parameter is harder to misuse than a `u64`, and it gives the library room to change representation later. + +For application internals, prefer newtypes at boundaries, identifiers, units, and validated inputs. Do not add wrappers that only create conversion noise inside one small module. + +## Example + +```rust +use std::fmt; +use std::str::FromStr; + +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub struct UserId(u64); + +impl UserId { + pub fn new(value: u64) -> Self { + Self(value) + } + + pub fn as_u64(self) -> u64 { + self.0 + } +} + +impl fmt::Display for UserId { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(formatter, "{}", self.0) + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct EmailAddress(String); + +impl EmailAddress { + pub fn try_new(value: impl Into) -> Result { + let value = value.into(); + + if !value.contains('@') { + return Err(EmailAddressError::MissingAt); + } + + Ok(Self(value)) + } + + pub fn as_str(&self) -> &str { + &self.0 + } + + pub fn into_inner(self) -> String { + self.0 + } +} + +impl fmt::Display for EmailAddress { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(self.as_str()) + } +} + +impl FromStr for EmailAddress { + type Err = EmailAddressError; + + fn from_str(value: &str) -> Result { + Self::try_new(value) + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum EmailAddressError { + MissingAt, +} +``` + +## Exceptions + +- Use a public tuple field for intentionally transparent wrappers with no invariant and no expected evolution pressure. +- Use `Deref` for pointer-like wrappers where dereference behavior is the core abstraction, not for ordinary semantic wrappers. +- Use a plain primitive when the value is local, obvious, and not crossing an API boundary. +- Use a domain struct instead of multiple newtypes when the invariant belongs to a combined value. diff --git a/.fabro/skills/rust-style-guide/guidelines/option-and-result-idioms.md b/.fabro/skills/rust-style-guide/guidelines/option-and-result-idioms.md new file mode 100644 index 000000000..901a7f8e1 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/option-and-result-idioms.md @@ -0,0 +1,86 @@ +# Option and Result Idioms + +## Rule + +Use simple combinators for short local transformations, and switch to explicit branching when `Option` or `Result` handling carries behavior, side effects, context, or recovery logic. + +## Why + +`Option` and `Result` make absence and failure visible in the type system. Small combinators keep simple cases compact, but complex chains hide decisions that agents need to see and modify safely. + +## Do + +- Use `Option` for expected absence and `Result` for failures that need a reason. +- Use `?` to propagate `Result` in fallible functions. +- Use `?` on `Option` only inside functions that return `Option`. +- Convert required `Option` values to `Result` with `ok_or_else` when constructing the error is nontrivial. +- Use `ok_or` for cheap, static, or already-built errors. +- Use `map`, `filter`, and `unwrap_or_else` for short, side-effect-free `Option` transforms. +- Use `map_err` only for local typed error conversion that preserves the source error. +- Use `transpose` for `Option>` to produce `Result, E>`. +- Use `let else`, `if let`, or `match` when the missing/error case has branching, logging, metrics, cleanup, retries, or recovery. +- Add error context at boundaries per [error propagation](error-propagation-context-and-messages.md), not on every small combinator. +- Treat Clippy as authoritative for combinator-vs-branching idioms; refactor instead of adding local bypasses ([rustc and Clippy lints](rustc-and-clippy-lints.md)). + +## Avoid + +- Do not chain combinators until the control flow is harder to read than a `match`. +- Do not hide side effects in `map`, `and_then`, `or_else`, or `inspect`. +- Do not use `.ok()` unless intentionally discarding the error cause at a boundary where absence is the right model. +- Do not use `unwrap_or` when the fallback is expensive or allocates; use `unwrap_or_else`. +- Do not use `unwrap_or_default` when absence is a domain error. +- Do not use `is_some` followed by `unwrap`; use `if let`, `let else`, or `match`. +- Do not replace domain-specific errors with generic missing-value messages. + +## Example + +Use combinators for local extraction and explicit branching for meaningful decisions: + +```rust +pub fn build_request(input: &Input) -> Result { + let id = input + .id() + .ok_or(Error::MissingField { field: "id" })?; + + let label = input + .label() + .filter(|label| !label.trim().is_empty()) + .map(str::to_owned); + + let timeout = match input.timeout_ms() { + Some(0) => return Err(Error::InvalidTimeout), + Some(ms) => Timeout::from_millis(ms)?, + None => Timeout::default(), + }; + + let mode = input + .mode() + .map(Mode::parse) + .transpose()? + .unwrap_or_else(Mode::default); + + Ok(Request::new(id, label, timeout, mode)) +} +``` + +Prefer explicit handling when the error path has behavior: + +```rust +pub fn load_profile(name: Option<&str>, store: &ProfileStore) -> Result { + let Some(name) = name else { + tracing::debug!("profile omitted; using default profile"); + return store.default_profile().map_err(Error::DefaultProfile); + }; + + store.load(name).map_err(|source| Error::LoadProfile { + name: name.to_owned(), + source, + }) +} +``` + +## Exceptions + +- Use a longer combinator chain when every step is a pure transformation and the names remain clear. +- Use `match` for simple cases when exhaustiveness or domain documentation matters. +- Use `.ok()` at external boundaries where a detailed failure intentionally becomes optional data. diff --git a/.fabro/skills/rust-style-guide/guidelines/ownership-borrowing-and-clone-policy.md b/.fabro/skills/rust-style-guide/guidelines/ownership-borrowing-and-clone-policy.md new file mode 100644 index 000000000..8a3b8db9f --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/ownership-borrowing-and-clone-policy.md @@ -0,0 +1,148 @@ +# Ownership, Borrowing, and Clone Policy + +## Rule + +Accept concrete borrowed parameters, store and return owned values at boundaries, and clone freely to keep APIs simple; use flexible generic bounds only when they clearly improve caller ergonomics. + +## Why + +Borrowed inputs such as `&str`, `&[T]`, and `&Path` keep call sites flexible and accept the common owned and borrowed caller types. Owned values keep lifetimes out of structs, snapshots, and return types. Plain accessors should not hide ownership or allocation costs, and generic bounds help callers only when they stay local instead of spreading type parameters through the API. + +## Do + +- Use `&self` for observation, `&mut self` for in-place mutation, and `self` for consuming transitions. +- Accept `&str` instead of `&String`, `&[T]` instead of `&Vec`, and `&Path` instead of `&PathBuf` for read-only inputs. +- Store owned `String`, `Vec`, and `PathBuf` inside structs. +- Take owned values or `impl Into` in constructors and setters that store the value unchanged; borrow and clone at the boundary when storing a normalized or derived value. +- Return borrowed values from plain accessors when the lifetime is obvious. +- Return owned snapshots, IDs, handles, or collections when returning references would expose unnecessary lifetimes, and name owned snapshots explicitly. +- Use `.clone()` for ordinary values, `Rc`, and `Arc`; this deliberately deviates from the std docs' `Arc::clone(&value)` preference in favor of one consistent spelling. +- Use `IntoIterator` for APIs whose purpose is to consume or extend from a sequence of items. +- Use `AsRef`, `AsRef`, or `impl Into` bounds only when caller flexibility clearly helps and the bound stays local. +- Accept `impl Read` or `impl Write` when a reusable library should test I/O behavior without touching the filesystem. +- Use `Cow` only when the API genuinely often borrows but sometimes allocates, and the lifetime stays local. +- Revisit clone costs only when profiling or domain knowledge shows they matter. + +## Avoid + +- Do not accept owned `String`, `Vec`, or `PathBuf` when the function only reads the input. +- Do not accept `&String`, `&Vec`, or `&PathBuf` by habit. +- Do not store borrowed references in structs just to avoid allocation. +- Do not add lifetime parameters only to avoid cheap clones; see [lifetimes](lifetimes.md) for when explicit lifetimes are worth it. +- Do not hide clones in bare-noun accessors such as `labels() -> Vec<_>` or `settings() -> Arc<_>`. +- Do not return references from computed queries or snapshots when an owned value would make the API simpler. +- Do not add `AsRef`, `Into`, `Borrow`, or generic type parameters to every function by default; reserve `Borrow` for key-equivalence and lookup patterns. +- Do not use `Cow` as a general-purpose way to avoid deciding between borrowed and owned data. +- Do not mix `Arc::clone(&value)` and `value.clone()` styles in the same codebase. +- Do not hide expensive deep clones in hot paths once cost is known to matter. + +## Public API Notes + +For public APIs, concrete borrowed refs are usually clearer than generic bounds; add flexible bounds when they materially reduce caller friction without leaking type parameters through the API. For internal application code, favor the simplest signature and clone at boundaries. For published libraries, document ownership behavior when clones may be large or surprising. + +## Example + +Store owned data, borrow in plain accessors, and take `impl Into` when storing unchanged: + +```rust +use std::path::{Path, PathBuf}; + +#[derive(Clone, Debug)] +pub struct Settings { + service_name: String, + root: PathBuf, +} + +impl Settings { + pub fn new(service_name: impl Into, root: PathBuf) -> Self { + Self { + service_name: service_name.into(), + root, + } + } + + pub fn service_name(&self) -> &str { + &self.service_name + } + + pub fn root(&self) -> &Path { + &self.root + } +} +``` + +Bad: hide an owned clone behind a plain accessor. + +```rust +pub fn labels(&self) -> Vec { + self.labels.clone() +} +``` + +Good: borrow by default and name owned snapshots explicitly. + +```rust +pub fn labels(&self) -> &[String] { + &self.labels +} + +pub fn labels_snapshot(&self) -> Vec { + self.labels.clone() +} +``` + +Use flexible bounds where they genuinely help callers, and normalize at the boundary: + +```rust +use std::borrow::Cow; + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct FileMatcher { + extensions: Vec, +} + +impl FileMatcher { + pub fn from_extensions(extensions: I) -> Self + where + I: IntoIterator, + S: AsRef, + { + let extensions = extensions + .into_iter() + .map(|extension| normalize_extension(extension.as_ref()).into_owned()) + .collect(); + + Self { extensions } + } + + pub fn matches_extension(&self, extension: &str) -> bool { + let extension = normalize_extension(extension); + + self.extensions + .iter() + .any(|candidate| candidate.as_str() == extension.as_ref()) + } +} + +pub fn normalize_extension(extension: &str) -> Cow<'_, str> { + let trimmed = extension.trim(); + let normalized = trimmed.strip_prefix('.').unwrap_or(trimmed); + + if normalized.chars().any(|character| character.is_ascii_uppercase()) { + Cow::Owned(normalized.to_ascii_lowercase()) + } else { + Cow::Borrowed(normalized) + } +} +``` + +## Exceptions + +- Accept owned values when the function consumes ownership, stores without cloning, or mirrors a standard library convention. +- Use `impl AsRef` for top-level file-opening helpers when accepting many path-like caller types is the main ergonomic benefit. +- Use `impl Read` or `impl Write` for lower-level helpers whose purpose is data processing, not path handling. +- Use `Cow` in parsing, normalization, and formatting helpers that can usually return a borrowed value. +- Use slices of references, such as `&[&str]`, when the call sites naturally already have borrowed items. +- Return owned handles from methods whose names make shared ownership explicit. +- Avoid clones in measured hot paths, large data movement, or resource-heavy types. +- Use specialized clone spelling only when matching an existing local convention in code you are modifying. diff --git a/.fabro/skills/rust-style-guide/guidelines/panics-unwrap-expect-and-assertions.md b/.fabro/skills/rust-style-guide/guidelines/panics-unwrap-expect-and-assertions.md new file mode 100644 index 000000000..897109e4c --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/panics-unwrap-expect-and-assertions.md @@ -0,0 +1,93 @@ +# Panics, unwrap, expect, and assertions + +## Rule + +Return `Result` for recoverable failures; panic only for violated invariants or impossible states, and prefer `expect` with an invariant-focused message over bare `unwrap`. + +## Why + +Panics unwind by default; a panicking Tokio task surfaces as a `JoinError` at the join point, and under `panic = "abort"` the process dies. Either way, panics give callers no structured recovery path for expected failures, so they are appropriate when the program has reached a state that means the code is wrong, not when input, I/O, network, parsing, or configuration can fail normally. + +## Do + +- Return `Result` for user input, file I/O, network calls, parsing, validation, configuration, and external service failures. +- Use `expect` when failure would prove a hard-coded constant, static fixture, or internal invariant is wrong. +- Write `expect` messages that state the invariant, such as `DEFAULT_PORT should be a valid u16`. +- Use `assert!`, `assert_eq!`, and `assert_ne!` for tests and internal invariants. +- Use `debug_assert!` only for checks that are helpful in debug builds but not required for release correctness. +- Use `unreachable!` only after the code has already ruled out the state by construction. +- Add `# Panics` rustdoc when a public function can panic. +- In tests, prefer `expect` when the setup failure message will help diagnose the failed test. + +## Avoid + +- Do not use `unwrap` or `expect` for recoverable runtime failures. +- Do not use bare `unwrap` outside tests; the workspace denies `clippy::unwrap_used` (with `allow-unwrap-in-tests`), so use `expect` with an invariant message in production code. +- Do not use panics for normal validation failures. +- Do not write `expect("should work")`, `expect("failed")`, or messages that just repeat the error. +- Do not use `unreachable!` for states reachable from external input. +- Do not rely on `debug_assert!` for memory safety, security, validation, or release behavior. +- Do not leave `todo!()` or `unimplemented!()` in committed production paths. +- Do not hide fallible startup work behind panics when a clean diagnostic can be returned. + +## Library vs Application + +Libraries should be strict: return errors for caller-controlled failures and document any public panic behavior. Applications may fail fast during startup for violated build-time or configuration invariants, but ordinary operator mistakes should still become clean errors. + +## Example + +Use `Result` for runtime input: + +```rust +pub fn parse_port(raw: &str) -> Result { + raw.parse() +} +``` + +Bad: panic on operator input. + +```rust +let port = std::env::var("PORT").unwrap().parse::().unwrap(); +``` + +Good: return a diagnostic path. + +```rust +use anyhow::Context; + +let port = std::env::var("PORT") + .context("PORT is required")? + .parse::() + .context("PORT should be a valid u16")?; +``` + +Use `expect` when a checked-in invariant is wrong: + +```rust +const DEFAULT_PORT: &str = "8080"; + +pub fn default_port() -> u16 { + DEFAULT_PORT + .parse() + .expect("DEFAULT_PORT should be a valid u16") +} +``` + +Use assertions for internal invariants: + +```rust +fn split_parsed_record(fields: &[String]) -> (&str, &str) { + assert!( + fields.len() == 2, + "record parser should produce exactly two fields" + ); + + (fields[0].as_str(), fields[1].as_str()) +} +``` + +## Exceptions + +- Use `unwrap` in short tests when the failure location is obvious and `expect` would add noise. +- Use panics in examples or prototypes only when the surrounding context is intentionally disposable. +- Use `panic!` for impossible internal states when returning an error would imply callers can recover. diff --git a/.fabro/skills/rust-style-guide/guidelines/property-tests-snapshots-benchmarks-and-ci.md b/.fabro/skills/rust-style-guide/guidelines/property-tests-snapshots-benchmarks-and-ci.md new file mode 100644 index 000000000..ee9924284 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/property-tests-snapshots-benchmarks-and-ci.md @@ -0,0 +1,86 @@ +# Property Tests, Snapshots, Benchmarks, and CI + +## Rule + +Use `cargo nextest run --workspace --all-targets --all-features` as the default workspace test runner; add `insta` when snapshots make complex output easier to review, and add property or benchmark tools only for real invariant or performance needs. + +## Activation + +Load this page when configuring test commands, CI, snapshot tests, property tests, benchmarks, or release verification. + +## Why + +Nextest gives a consistent test runner for local and CI workflows. Snapshot, property, and benchmark tools are valuable when they match the code shape, but they add dependencies, review process, and maintenance cost. + +## Do + +- Run `cargo nextest run --workspace --all-targets --all-features` as the normal local and CI test command. +- Keep `cargo test` available for cases Nextest does not cover; the doctest opt-in policy lives on [testing and doctests](testing-and-doctests.md). +- Run pinned rustfmt and Clippy checks in CI alongside tests. +- Add `insta` for stable textual or structured outputs such as CLI output, diagnostics, generated config, serialized data, and rendered reports. +- Commit snapshot files and review snapshot diffs before accepting them. +- Redact, sort, or normalize nondeterministic fields before snapshotting values. +- Use `proptest` for parsers, serializers, round trips, normalization, state machines, and invariants over broad input spaces. +- Prefer `proptest` for new property tests; keep `quickcheck` only when the project already uses it. +- Use `criterion` when performance is a stated requirement or a likely regression risk. +- Keep benchmark inputs realistic, named, and stable across runs. + +## Avoid + +- Do not add every testing tool to every crate by default. +- Do not use snapshot tests for simple scalar assertions. +- Do not snapshot timestamps, random IDs, absolute paths, map iteration order, or environment-specific output without normalizing them. +- Do not blindly accept snapshot changes. +- Do not write property tests whose generated cases are so broad that failures are impossible to diagnose. +- Do not treat benchmarks as correctness tests. +- Do not fail ordinary CI on benchmark thresholds unless the project has stable performance infrastructure. +- Do not maintain separate local and CI test commands that cover different test sets without documenting the difference. + +## Example + +Run the configured CI commands before handing off Rust changes: + +```sh +cargo +nightly-2026-04-14 fmt --check --all +cargo clippy --locked --workspace --all-targets --all-features -- -D warnings +cargo nextest run --workspace --all-targets --all-features +``` + +Use the new project workflow for initial CI setup. + +Add snapshot tests when reviewing the full output is clearer than hand-picking many assertions: + +```rust +#[test] +fn renders_validation_errors() { + let report = render_validation_errors(&[ + ValidationError::missing_field("email"), + ValidationError::invalid_field("limit"), + ]); + + insta::assert_snapshot!(report); +} +``` + +Add property tests for broad invariants: + +```rust +use proptest::prelude::*; + +proptest! { + #[test] + fn trim_is_idempotent(input in "[a-zA-Z0-9 ]{0,64}") { + let once = normalize_whitespace(&input); + let twice = normalize_whitespace(&once); + + prop_assert_eq!(once, twice); + } +} +``` + +## Exceptions + +- Existing projects may keep `cargo test` as the primary runner until Nextest is deliberately added. +- Use `quickcheck` when it is already the established project convention. +- Use custom benchmark or load-test infrastructure for services where `criterion` does not model the real performance risk. +- Skip specialized tooling for small crates where ordinary tests make the behavior clear. diff --git a/.fabro/skills/rust-style-guide/guidelines/public-api-evolution.md b/.fabro/skills/rust-style-guide/guidelines/public-api-evolution.md new file mode 100644 index 000000000..3171710e1 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/public-api-evolution.md @@ -0,0 +1,91 @@ +# Public API Evolution + +## Rule + +Treat public API evolution as mostly relevant only for published crates or APIs consumed outside the repo; optimize internal application APIs for simplicity and accept coordinated breaking changes. + +## Why + +Most application code is changed with its callers. Semver ceremony, compatibility shims, sealed traits, and future-proof annotations add noise when the API is not externally consumed. Published library APIs are different: callers update independently, so compatibility becomes part of the contract. + +## Do + +- First classify the API as internal application code, shared in-repo workspace code, or externally consumed/published library code. +- Prefer simple current APIs for application and in-repo code. +- Accept breaking changes for internal APIs when the callers can be updated in the same change. +- Use `pub(crate)` for internal boundaries that should not become crate API. +- Keep published public APIs small and deliberate. +- For published crates, follow semver, use private fields, and consider `#[non_exhaustive]` where future fields or variants are likely. +- Add `#[must_use]` to types and methods where silently dropping the value is almost always a bug: builders, RAII guards, and task/owner types that must be shut down or joined. +- Seal public traits only when external implementations are not intended and the trait is part of a published API. + +## Avoid + +- Do not add semver compatibility shims for purely internal application code. +- Do not use `#[non_exhaustive]` in internal code just to future-proof ordinary enums or structs. +- Do not add `#[non_exhaustive]` to an already-published type as a later hardening step; adding it is itself a breaking change because downstream exhaustive matches, struct literals, and tuple-variant construction stop compiling. Apply it when the type is introduced. +- Do not create broad public facades for modules that are only used inside one application. +- Do not expose public fields on invariant-bearing types; [struct design](struct-design-and-encapsulation.md) owns the field-visibility policy. +- Do not leak dependency types through published public APIs unless that dependency is intentionally part of the contract. +- Do not remove or change published public APIs without treating it as a breaking change. +- Do not make public traits open for external implementations unless that extension point is intentional. +- Do not rely on the noisy `clippy::must_use_candidate` lint to find must-use types; apply `#[must_use]` deliberately where dropping the value is a real mistake. + +## Library vs Application + +Applications and internal workspace crates may optimize for directness. Refactor call sites together, delete stale APIs, and avoid compatibility layers that no outside caller needs. + +Published crates and externally consumed APIs should optimize for compatibility. Keep the public surface narrow, document behavior, and use semver-aware tools such as `#[non_exhaustive]`, deprecation periods, and sealed traits when they solve a real evolution problem. + +## Must-Use Types + +Mark types and methods with `#[must_use]` when ignoring the returned value is almost always a mistake. This turns a silent bug into a compile-time warning at the call site. + +- Use it on builders, RAII guards, and async task owners such as a `Poller` or `WorkerSet` that callers must shut down or join. +- Use it where discarding the value is almost certainly a bug: builders, guards, handles, and fallible or lazily-effective operations, not ordinary accessors. +- `Result` and `Option` are already `#[must_use]`, so the value comes from your own types. +- Apply it deliberately rather than enabling `clippy::must_use_candidate`, which is noisy. + +```rust +/// Owns a background task. Dropping it without calling `shutdown` leaks the task. +#[must_use = "call `shutdown` to stop and join the task"] +pub struct Poller { + shutdown: CancellationToken, + task: JoinHandle>, +} +``` + +## Example + +```rust +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct RunSnapshot { + pub id: RunId, + pub status: RunStatus, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum RunStatus { + Queued, + Running, + Succeeded, + Failed, +} + +#[non_exhaustive] +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ClientError { + Timeout, + Unauthorized, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct RunId(u64); +``` + +## Exceptions + +- Treat an internal API as external when another team, service, plugin, or generated client consumes it independently. +- Use conservative semver rules when publishing to crates.io or documenting a stable SDK surface. +- Keep temporary compatibility shims when a multi-step migration cannot update all callers in one change. +- Use `#[non_exhaustive]` internally only when it materially improves match-site clarity during active development. diff --git a/.fabro/skills/rust-style-guide/guidelines/rust-edition-and-msrv.md b/.fabro/skills/rust-style-guide/guidelines/rust-edition-and-msrv.md new file mode 100644 index 000000000..acbc30d20 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/rust-edition-and-msrv.md @@ -0,0 +1,60 @@ +# Rust Edition and MSRV + +## Rule + +Use Rust 2024 for new code and declare `rust-version` in every package; default Rust 2024 crates to `rust-version = "1.85"` unless project constraints require otherwise. + +## Why + +The edition controls language compatibility, and `rust-version` tells Cargo and users the minimum compiler the crate supports. Declaring both prevents agents from accidentally depending on newer compiler features without making that policy visible. + +## Do + +- Set `edition = "2024"` for Rust 2024 crates. +- Set `rust-version = "1.85"` for Rust 2024 crates unless the project has a higher documented MSRV; supporting a lower MSRV requires an older edition. +- Keep workspace member editions and MSRVs consistent unless a crate has a specific reason to differ. +- Treat MSRV bumps in reusable libraries as public compatibility changes. +- Check library changes against the declared MSRV, not only the local stable compiler, and include all feature-gated code. +- Use stable Rust by default. + +## Avoid + +- Do not omit `rust-version` from `Cargo.toml`. +- Do not use Rust 2021 for new crates by habit. +- Do not set an MSRV lower than the selected edition supports. +- Do not use APIs stabilized after the declared MSRV without bumping `rust-version`. +- Do not use nightly-only language features as house style. +- Do not let a dependency upgrade silently raise a library's practical MSRV. + +## Public API Notes + +For libraries, an MSRV bump can affect downstream users even when the Rust API is otherwise semver-compatible. Make the bump deliberate and document it in release notes or the changelog when the crate is published. + +Applications and internal services may track stable Rust more aggressively, but they should still declare `rust-version` so builds are reproducible and CI failures are easier to understand. + +## Example + +Package-level policy: + +```toml +[package] +name = "example-crate" +version = "0.1.0" +edition = "2024" +rust-version = "1.85" +``` + +When changing a reusable library, verify the declared MSRV explicitly: + +```sh +rustup toolchain install 1.85.0 +cargo +1.85.0 check --workspace --all-targets --all-features +``` + +Use the new project workflow for initial workspace setup. + +## Exceptions + +- Use Rust 2021 when required by embedded targets, downstream users, tooling, or dependency constraints. +- Use a higher MSRV when the project already requires newer stable compiler features. +- Migrate existing crates to a newer edition as a focused mechanical change when possible. diff --git a/.fabro/skills/rust-style-guide/guidelines/rustc-and-clippy-lints.md b/.fabro/skills/rust-style-guide/guidelines/rustc-and-clippy-lints.md new file mode 100644 index 000000000..9166066d8 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/rustc-and-clippy-lints.md @@ -0,0 +1,83 @@ +# rustc and Clippy Lints + +## Rule + +Use curated workspace lints: start from the workspace lint tables in the [new project workflow](../workflows/new-rust-project.md), tailor project-specific denies, and require justified local exceptions with `#[expect(..., reason = "...")]`. + +## Why + +A curated lint set catches real mistakes while the allow-list exempts the noisy pedantic lints the project has rejected; everything else is enforced in CI. Central policy keeps the baseline consistent, and local `expect` attributes make intentional exceptions auditable. + +## Do + +- Put shared lint policy in the workspace `Cargo.toml`. +- Run Clippy in CI with `cargo clippy --locked --workspace --all-targets --all-features -- -D warnings`. +- Enable `clippy::pedantic` at `warn`, then allow noisy lints the project has rejected. +- Deny lints that catch correctness or project-boundary violations. +- Use `#[expect(lint_name, reason = "...")]` for narrow local exceptions. +- Review the baseline `disallowed_methods` and `disallowed_types` in the [new project workflow](../workflows/new-rust-project.md) before copying them; these should reflect the target project's architecture. +- Put architecture-specific Clippy settings in `clippy.toml`. + +## Avoid + +- Do not enable all restriction lints. +- Do not deny all pedantic lints by default. +- Do not add unexplained `#[allow(...)]` attributes. +- Do not hide one-off exceptions in workspace-wide lint config. +- Do not copy project-specific disallowed methods, types, or environment rules without checking that they match the new codebase. +- Do not use local lint bypasses for combinator-vs-control-flow idioms; refactor to Clippy's preferred shape or change the workspace lint policy deliberately. + +## Lint Levels and CI + +CI runs Clippy with `-D warnings`, so the level controls where a violation is caught, not whether it is allowed: + +- `deny`: denied rustc lints fail `cargo build` everywhere, including local builds; denied Clippy lints fail only `cargo clippy`, so the Clippy run, locally and in CI, is what enforces them. +- `warn`: a local warning, but promoted to an error in CI by `-D warnings`. +- `allow`: the only true exemption; every lint not allowed is enforced in CI. + +Justify an intentional violation at the narrowest scope with `#[expect(lint, reason = "...")]`; a bare `#[allow]` is rejected by `allow_attributes_without_reason`. A CLI, for example, keeps `print_stdout = "warn"` and annotates each of its few real stdout functions: + +```rust +#[expect(clippy::print_stdout, reason = "curated help is written directly to stdout")] +fn print_help() { + println!("usage: app [options]"); +} +``` + +## Example + +Use the new project workflow for initial workspace lint tables. In existing projects, justify narrow local exceptions near the code: + +```rust +#[expect( + clippy::too_many_arguments, + reason = "Constructor mirrors the wire contract fields one-to-one" +)] +pub fn new( + id: RunId, + parent_id: Option, + status: RunStatus, + attempt: AttemptNumber, + started_at: Timestamp, + finished_at: Option, + labels: Labels, + metadata: Metadata, +) -> Self { + Self { + id, + parent_id, + status, + attempt, + started_at, + finished_at, + labels, + metadata, + } +} +``` + +## Exceptions + +- Use `#[allow]` only when `#[expect]` is unavailable or the lint is intentionally disabled for generated code. +- Move a lint to workspace config when the project has rejected it as policy, not because one function is inconvenient. +- Lower or remove `unsafe_code = "deny"` only for crates whose purpose requires unsafe code, then document the local unsafe policy. diff --git a/.fabro/skills/rust-style-guide/guidelines/rustfmt-and-formatting.md b/.fabro/skills/rust-style-guide/guidelines/rustfmt-and-formatting.md new file mode 100644 index 000000000..a7cb01ca8 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/rustfmt-and-formatting.md @@ -0,0 +1,43 @@ +# rustfmt and Formatting + +## Rule + +Use the checked-in `rustfmt.toml` as the formatting authority and run rustfmt with the pinned nightly toolchain. + +## Why + +Formatting should be mechanical and reproducible. A pinned rustfmt version prevents agents, editors, and CI from producing different diffs when the project uses unstable rustfmt options. + +## Do + +- Check in `rustfmt.toml` at the workspace root. +- Use `nightly-2026-04-14` for formatting. +- Run `cargo +nightly-2026-04-14 fmt --all` before committing Rust changes. +- Run `cargo +nightly-2026-04-14 fmt --check --all` in CI. +- Keep editor, agent, and CI commands aligned with the same pinned toolchain. +- Let rustfmt decide layout instead of hand-formatting around it. + +## Avoid + +- Do not run unpinned `cargo fmt` when the project has this config. +- Do not manually preserve formatting that rustfmt changes. +- Do not mix stable rustfmt and pinned nightly rustfmt in the same repository. +- Do not change formatting settings as part of unrelated feature work. +- Do not use `#[rustfmt::skip]` except for generated code or unusual literals where formatting would damage readability. + +## Example + +Run the checked-in formatter configuration: + +```sh +cargo +nightly-2026-04-14 fmt --all +cargo +nightly-2026-04-14 fmt --check --all +``` + +Use the new project workflow for the initial `rustfmt.toml` contents. + +## Exceptions + +- Existing projects may keep their current rustfmt pin until a focused formatting update. +- Generated code may opt out of formatting when regeneration controls the file layout. +- Public examples may use manual line breaks when rustfmt does not run on the snippet. diff --git a/.fabro/skills/rust-style-guide/guidelines/smart-pointers-and-interior-mutability.md b/.fabro/skills/rust-style-guide/guidelines/smart-pointers-and-interior-mutability.md new file mode 100644 index 000000000..4e91787f4 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/smart-pointers-and-interior-mutability.md @@ -0,0 +1,94 @@ +# Smart Pointers and Interior Mutability + +## Rule + +Prefer ordinary ownership first; use `Box` for single-owner heap allocation, `Rc` and `RefCell` only for single-threaded sharing and interior mutation, and `OnceLock` or `LazyLock` for one-time initialization. + +## Why + +Rust's ownership model is usually the simplest mutation model. Smart pointers and interior mutability are useful when ownership really is shared or mutation must happen through a shared handle, but they add coordination costs and failure modes. + +Cross-thread and cross-task sharing (`Arc`, locks, channels) is chosen on [concurrency primitives](concurrency-primitives.md). + +## Do + +- Use owned values and borrowing before introducing smart pointers. +- Use `Box` for recursive data, large enum variants, or single-owner heap allocation. +- Use `Box` for owned dynamic dispatch when one owner is enough. +- Use `Rc` only for single-threaded shared ownership. +- Use `Weak` (`std::rc::Weak` or `std::sync::Weak`) to break parent-child or observer cycles. +- Use `OnceLock` or `LazyLock` for one-time initialization. + +## Avoid + +- Do not use `Rc` or `RefCell` in multi-threaded code. +- Do not create `Rc` or `Arc` cycles; two strong references pointing at each other are never freed and leak the whole graph. +- Do not use `RefCell` when a normal `&mut self` API would work. +- Do not create global mutable state unless initialization and access rules are clear. + +## Pointer and Thread-Safety Table + +| Need | Prefer | Thread-safe use | +| --- | --- | --- | +| Single owner, heap allocation | `Box` | Movable across threads when `T: Send` | +| Single-thread shared ownership | `Rc` | No; use only on one thread | +| Single-thread interior mutation | `Cell` or `RefCell` | No; use only on one thread | +| One-time initialization | `OnceLock` or `LazyLock` | Yes when the initialized value is thread-safe | +| Cross-thread shared ownership | `Arc` | See [concurrency primitives](concurrency-primitives.md) | +| Shared mutable state | `Mutex` or `RwLock` | See [concurrency primitives](concurrency-primitives.md) | +| Ownership transfer | Channel | See [concurrency primitives](concurrency-primitives.md) | + +## Example + +`Box` for recursion, `Weak` to break the parent-child cycle, and `OnceLock` for one-time initialization: + +```rust +use std::cell::RefCell; +use std::rc::{Rc, Weak}; +use std::sync::OnceLock; + +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum Expr { + Literal(i64), + Add(Box, Box), +} + +impl Expr { + pub fn evaluate(&self) -> i64 { + match self { + Self::Literal(value) => *value, + Self::Add(left, right) => left.evaluate() + right.evaluate(), + } + } +} + +pub struct Node { + parent: RefCell>, + children: RefCell>>, +} + +impl Node { + pub fn new() -> Rc { + Rc::new(Self { + parent: RefCell::new(Weak::new()), + children: RefCell::new(Vec::new()), + }) + } + + pub fn add_child(parent: &Rc, child: Rc) { + *child.parent.borrow_mut() = Rc::downgrade(parent); + parent.children.borrow_mut().push(child); + } +} + +static DEFAULT_LOCALE: OnceLock = OnceLock::new(); + +pub fn default_locale() -> &'static str { + DEFAULT_LOCALE.get_or_init(|| "en-US".to_owned()) +} +``` + +## Exceptions + +- Use `Cell` or `RefCell` for narrow single-threaded caches, adapters, tests, or APIs where runtime borrow checking is genuinely simpler. +- Use `Box` for indirection only when recursion, variant size, or owned dynamic dispatch requires it, not by habit. diff --git a/.fabro/skills/rust-style-guide/guidelines/struct-design-and-encapsulation.md b/.fabro/skills/rust-style-guide/guidelines/struct-design-and-encapsulation.md new file mode 100644 index 000000000..0449b5466 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/struct-design-and-encapsulation.md @@ -0,0 +1,82 @@ +# Struct Design and Encapsulation + +## Rule + +Model meaningful concepts as structs with private fields and behavior-bearing methods; use public fields only for plain data with no invariants. + +## Why + +Rust structs can protect invariants without inheritance. Private fields let a type control construction and mutation, while methods make ownership and behavior explicit. + +## Do + +- Give a struct private fields when it has invariants, validation, or behavior. +- Put behavior on the type that owns the data it needs. +- Use `&self` for observation, `&mut self` for in-place mutation, and `self` for consuming transitions. +- Expose only the read accessors callers need. +- Use `pub(crate)` fields or methods only for real internal module boundaries. +- Use public fields for DTOs, config structs, snapshots, and other plain data. +- Keep structs focused enough that their invariants fit in one mental model. + +## Avoid + +- Do not make fields public just to avoid writing constructors or accessors. +- Do not create method-heavy wrappers around data they do not own. +- Do not split normal type behavior into unrelated helper modules when methods would be clearer. +- Do not generate getters and setters for every field by habit. +- Do not expose test-only mutation paths from production APIs. + +## Public API Notes + +For public libraries, public fields are hard to evolve because callers can construct and destructure them directly. Prefer private fields unless the type is intentionally plain data. + +For application internals, private fields are still the default, but `pub(crate)` can be pragmatic when a module boundary is real and narrower APIs would add noise. + +## Example + +`EmailAddress` is a validated newtype; its constructor and validation live on the [newtype pattern](newtype-pattern-and-semantic-wrappers.md) page. + +```rust +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct EmailAddress(String); + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct UserId(u64); + +pub struct UserAccount { + id: UserId, + email: EmailAddress, + active: bool, +} + +impl UserAccount { + pub fn id(&self) -> UserId { + self.id + } + + pub fn email(&self) -> &EmailAddress { + &self.email + } + + pub fn is_active(&self) -> bool { + self.active + } + + pub fn deactivate(&mut self) { + self.active = false; + } +} + +#[derive(Clone, Debug)] +pub struct UserSummary { + pub id: UserId, + pub email: EmailAddress, + pub active: bool, +} +``` + +## Exceptions + +- Use public fields for plain data structures whose fields are the intended API. +- Use tuple structs for small newtypes when the inner value has no invariant or when a public wrapper is intentional. +- Use free functions for algorithms that do not belong to one owner type. diff --git a/.fabro/skills/rust-style-guide/guidelines/testing-and-doctests.md b/.fabro/skills/rust-style-guide/guidelines/testing-and-doctests.md new file mode 100644 index 000000000..711e7f036 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/testing-and-doctests.md @@ -0,0 +1,109 @@ +# Testing and Doctests + +## Rule + +Use balanced behavior-focused testing: put unit tests near focused logic, integration tests around public behavior and workflows, and skip doctests by default. + +## Why + +Unit tests give fast feedback around dense logic and invariants. Integration tests protect the behavior callers actually depend on. Doctests add maintenance cost and should not become default coverage just because a public item has documentation. + +## Do + +- Test behavior, invariants, and observable state changes instead of private implementation steps. +- For each nontrivial source file, default to a bottom-of-file `#[cfg(test)] mod tests` covering that file's behavior and private helpers. Integration tests complement these module tests; they do not replace them. +- Put unit tests in the same module or a nearby test module when they exercise focused domain logic, parsing, validation, or small transformations. +- Put integration tests under `tests/` when they exercise public APIs, CLI behavior, cross-crate behavior, I/O boundaries, or multi-step workflows. +- Use module-private tests when they make hard-to-reach invariants clear; prefer public behavior when practical. +- Name tests as behavior descriptions, such as `rejects_zero_limit` or `loads_profile_from_env_override`. +- Use fallible tests returning `Result<(), Error>` when setup or assertions naturally use `?`. +- Keep setup helpers small, explicit, and named after domain concepts. +- Prefer real values and temp files or directories where practical; use fakes or mocks only at external, slow, or nondeterministic boundaries. +- For reusable libraries, expose narrow seams for file, network, time, randomness, subprocess, or OS behavior when edge cases must be tested. +- Put regression tests at the level where the bug was observable. +- Keep assertions specific about behavior, errors, and state changes. + +## Avoid + +- Do not add doctests by default. +- Do not use rustdoc examples as a substitute for normal tests. +- Do not test every private helper through brittle implementation details. +- Do not write tests that only mirror the implementation. +- Do not use bare `unwrap` in tests when `?` or `expect` would make failures clearer. +- Do not add sleeps or timing-dependent tests; use controlled clocks, explicit events, or boundary timeouts. +- Do not assert only that code "does not panic" when behavior can be checked. +- Do not introduce broad test-only public APIs. +- Do not make helpers `pub` only so integration tests can reach them; use module-local tests or expose a real domain API. +- Do not hide test-only controls in normal library APIs; gate them behind `cfg(test)` or a deliberate `test-util` feature. +- Do not skip meaningful integration coverage just because unit tests pass. + +## Example + +Keep unit tests close to focused logic: + +```rust +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct Limit(u32); + +impl Limit { + pub fn get(self) -> u32 { + self.0 + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum LimitError { + Invalid, + Zero, +} + +pub fn parse_limit(value: &str) -> Result { + let value = value.parse().map_err(|_| LimitError::Invalid)?; + + if value == 0 { + return Err(LimitError::Zero); + } + + Ok(Limit(value)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn rejects_zero_limit() { + let error = parse_limit("0").expect_err("zero limit should be rejected"); + assert_eq!(error, LimitError::Zero); + } + + #[test] + fn parses_positive_limit() -> Result<(), LimitError> { + let limit = parse_limit("25")?; + assert_eq!(limit.get(), 25); + Ok(()) + } +} +``` + +Use integration tests for public workflows: + +```rust +#[test] +fn creates_user_workflow() -> anyhow::Result<()> { + let app = TestApp::start()?; + + let response = app.create_user("ada@example.com")?; + + assert_eq!(response.status(), 201); + assert!(app.user_exists("ada@example.com")?); + Ok(()) +} +``` + +## Exceptions + +- Add doctests only when a project explicitly opts into maintaining public rustdoc examples. +- Use `no_run` or `ignore` for rustdoc examples only when the documentation page's rules apply. +- Use module-private tests for parsers, validators, state machines, or algorithmic code with dense edge cases. +- Use `#[cfg(test)]` helpers when they keep production APIs clean and do not hide the behavior under test. diff --git a/.fabro/skills/rust-style-guide/guidelines/trait-design.md b/.fabro/skills/rust-style-guide/guidelines/trait-design.md new file mode 100644 index 000000000..ae4b2813e --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/trait-design.md @@ -0,0 +1,113 @@ +# Trait Design + +## Rule + +Write small, behavior-focused traits; make public traits open only when external implementations are intended, and use sealed traits when the crate must control implementors. + +## Why + +Traits are extension contracts. Small traits are easier to implement, test, object-check, and evolve. Public traits invite downstream implementations unless sealed, so their required methods and semantics become part of the crate's stable API. + +## Do + +- Start with concrete types or enums; introduce a trait when code genuinely needs caller-supplied behavior or an open extension point. +- Keep required methods small and cohesive. +- Name traits after behavior or capability, such as `Notifier`, `Store`, or `TokenSource`. +- Put convenience methods on the trait as provided methods when they can be implemented from the required core methods. +- Document public trait contracts: what implementors must guarantee, error behavior, blocking behavior, and whether methods may be called concurrently. +- Use associated types when each implementor chooses a related type. +- Use generic methods when each caller chooses the type for that call. +- Keep bounds close to the function that needs them, preferably in a `where` clause for complex bounds. +- Make traits object-safe when they are intended for `dyn Trait`. +- Add `where Self: Sized` to generic provided methods, such as ones taking `impl Into`, on traits meant for trait objects; without that opt-out, a generic method makes the trait unusable as `dyn Trait`. +- Seal public traits when users should call trait methods but should not implement the trait outside the crate. + +## Avoid + +- Do not create a trait only to organize methods on one concrete type. +- Do not make broad traits with unrelated capabilities. +- Do not expose public traits by default for every behavior-bearing type. +- Do not add required methods to public traits casually; downstream implementors must update. +- Do not use blanket implementations unless the behavior is obvious and unlikely to block future impls. +- Do not make a trait object API from a trait with non-object-safe required methods. +- Do not encode inheritance hierarchies with supertraits unless each supertrait is a real contract. + +## Public API Notes + +An unsealed public trait is an open extension point. Treat it as a semver commitment to downstream implementors. + +A sealed public trait is still public API for callers, but external crates cannot add implementations. Use it when the crate owns the valid implementor set but trait syntax is useful for bounds or shared behavior. + +## Example + +```rust +pub trait Notifier { + fn notify(&self, message: &Message) -> Result<(), NotifyError>; + + fn notify_text(&self, body: impl Into) -> Result<(), NotifyError> + where + Self: Sized, + { + self.notify(&Message::new(body)) + } +} + +pub fn send_welcome(notifier: &N, user: &User) -> Result<(), NotifyError> +where + N: Notifier, +{ + notifier.notify_text(format!("welcome {}", user.name())) +} + +pub trait DeliveryChannel: sealed::Sealed { + fn name(&self) -> &'static str; +} + +pub struct EmailChannel; + +impl DeliveryChannel for EmailChannel { + fn name(&self) -> &'static str { + "email" + } +} + +mod sealed { + pub trait Sealed {} +} + +impl sealed::Sealed for EmailChannel {} + +pub struct Message { + body: String, +} + +impl Message { + pub fn new(body: impl Into) -> Self { + Self { body: body.into() } + } + + pub fn body(&self) -> &str { + &self.body + } +} + +pub struct User { + name: String, +} + +impl User { + pub fn name(&self) -> &str { + &self.name + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct NotifyError; +``` + +## Exceptions + +- Use a broader trait when matching a mature ecosystem abstraction that callers already know. +- Use a marker trait only when it carries a real compile-time contract that cannot be expressed more clearly another way. +- Leave a public trait unsealed when downstream crates are expected to provide their own implementations. +- Use concrete types instead of traits when variation is not required. diff --git a/.fabro/skills/rust-style-guide/guidelines/typestate-and-state-machines.md b/.fabro/skills/rust-style-guide/guidelines/typestate-and-state-machines.md new file mode 100644 index 000000000..605a7c724 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/typestate-and-state-machines.md @@ -0,0 +1,121 @@ +# Typestate and State Machines + +## Rule + +Use typestate broadly for workflows with ordered states; use runtime enums when state is dynamic, persisted, or naturally handled by exhaustive matching. + +## Why + +Typestate makes invalid transitions fail to compile. It is a good fit for workflows where values move through known phases and later operations require earlier steps to have happened. + +## Activation + +Load this page when a value moves through ordered phases such as draft-to-published or connected-to-authenticated, or when choosing between compile-time states and runtime state enums. Skip it for ordinary optional configuration, which uses plain constructors and builders. + +## Do + +- Use typestate for ordered workflows such as draft-to-published, configured-to-started, connected-to-authenticated, or parsed-to-validated. +- Model each compile-time state with a small marker type. +- Store shared data in one generic struct like `Workflow`. +- Put transition methods on the source state and return the destination state. +- Put state-independent accessors on `impl`. +- Use `PhantomData` when the state type is only a compile-time marker. +- Keep transition methods consuming when the old state should no longer be usable. +- Use runtime enums when state is read from a database, received over the network, chosen by users, or stored in a mixed collection. +- Keep ordinary optional-configuration builders simple unless the builder enforces important ordered steps. + +## Avoid + +- Do not use typestate for states that are only labels in a UI or report. +- Do not use typestate when every call site immediately erases the state into `dyn Trait` or an enum. +- Do not create many marker types for a workflow with unclear or frequently changing states. +- Do not encode runtime data as type parameters. +- Do not force typestate through async task boundaries, persistence layers, or message queues when runtime state is clearer. +- Do not use typestate to hide validation that still must happen at external boundaries. + +## Public API Notes + +Typestate-heavy public APIs expose type-level workflow structure to callers. Use clear state names and transition method names, and keep generic state parameters out of unrelated APIs. + +When a public library must evolve states over time, consider a runtime enum or a sealed state marker pattern so the crate can add states without forcing callers to name every marker type. + +## Example + +```rust +use std::marker::PhantomData; + +#[derive(Clone, Debug)] +pub struct Draft; + +#[derive(Clone, Debug)] +pub struct Reviewed; + +#[derive(Clone, Debug)] +pub struct Published; + +#[derive(Clone, Debug)] +pub struct Article { + title: String, + body: String, + marker: PhantomData, +} + +impl Article { + pub fn new(title: &str, body: &str) -> Self { + Self { + title: title.to_owned(), + body: body.to_owned(), + marker: PhantomData, + } + } + + pub fn revise(&mut self, body: &str) { + self.body = body.to_owned(); + } + + pub fn submit(self) -> Article { + Article { + title: self.title, + body: self.body, + marker: PhantomData, + } + } +} + +impl Article { + pub fn reject(self) -> Article { + Article { + title: self.title, + body: self.body, + marker: PhantomData, + } + } + + pub fn publish(self) -> Article { + Article { + title: self.title, + body: self.body, + marker: PhantomData, + } + } +} + +impl Article { + pub fn public_body(&self) -> &str { + &self.body + } +} + +impl Article { + pub fn title(&self) -> &str { + &self.title + } +} +``` + +## Exceptions + +- Use data-bearing enums when all states must be stored together, matched exhaustively, serialized, or loaded dynamically. +- Use runtime validation for inputs from outside the process even when the internal workflow uses typestate. +- Use a simpler builder when typestate would only enforce optional configuration order. +- Use a plain struct with validation when the workflow has only one meaningful transition. diff --git a/.fabro/skills/rust-style-guide/guidelines/unsafe-code-and-macros.md b/.fabro/skills/rust-style-guide/guidelines/unsafe-code-and-macros.md new file mode 100644 index 000000000..e3557b3a0 --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/unsafe-code-and-macros.md @@ -0,0 +1,117 @@ +# Unsafe Code and Macros + +## Rule + +Ban project-written unsafe code by default; allow `macro_rules!` and proc macros only when they materially improve code simplicity. + +## Activation + +Load this page when a task touches `unsafe`, FFI, raw pointers, custom macros, proc macros, generated implementations, or macro-heavy public APIs. + +## Why + +Unsafe code creates proof obligations the compiler cannot check, so the default should be no local unsafe. Macros can hide control flow and make errors harder to understand, but they are useful when they remove real repetition or express a small, consistent pattern better than ordinary Rust. + +## Do + +- Keep `unsafe_code = "deny"` in the default workspace lint policy. +- Prefer safe Rust and mature crates over project-written unsafe code. +- Treat project-written unsafe as an explicit crate-level exception, not a local convenience. +- If unsafe is truly required, isolate it behind the smallest safe API and document the crate's unsafe policy before implementation. +- Keep unsafe blocks as small as possible; put safe validation and branching outside them. +- Put a `SAFETY:` comment next to every unsafe block or impl in crates that are allowed to use unsafe. +- Document every public unsafe function or trait with `# Safety`. +- Run `cargo +nightly miri test` for crates with project-written unsafe when Miri supports the target (install once with `rustup +nightly component add miri`). +- Keep FFI crates thin: translate portable boundary types and call safe core logic. +- Use `macro_rules!` for repeated impls, repeated tests, small declarative patterns, and local boilerplate that ordinary functions or traits cannot simplify cleanly. +- Use proc macros only when a derive, attribute, or function-like macro materially reduces boilerplate across many call sites. +- Keep macro inputs narrow, generated APIs predictable, and compile errors understandable. +- Put proc macros in dedicated proc-macro crates and keep their public surface small. + +## Avoid + +- Do not add unsafe code to satisfy the borrow checker or optimize before measurement. +- Do not hide unsafe behavior behind broad helper names. +- Do not expose an unsafe public API unless callers truly must uphold invariants the crate cannot check. +- Do not lower `unsafe_code = "deny"` for a whole workspace because one crate needs an exception. +- Do not exchange Rust-owned allocations, `TypeId`-dependent values, or global-state assumptions across dynamic library boundaries. +- Do not use uninitialized memory patterns without a type-specific validity proof; prefer `MaybeUninit` when uninitialized memory is truly required. +- Do not write a macro for one or two call sites. +- Do not use macros to invent control flow that functions, traits, enums, or builders can express clearly. +- Do not write a proc macro when `macro_rules!`, a derive from a mature crate, or ordinary Rust would be enough. +- Do not make macro-generated names, modules, trait impls, or side effects surprising. + +## Safety Notes + +Project-written unsafe includes unsafe blocks, unsafe functions, unsafe traits and impls, raw-pointer dereferences, FFI boundaries, and other code that requires the `unsafe` keyword. Dependency code may contain unsafe, but that does not justify adding local unsafe to the project. + +When a crate is granted an unsafe exception, review the safe abstraction boundary first: callers should be able to use the public API without knowing the internal unsafe invariant. + +In Rust 2024, write FFI declarations and unsafe attributes in their explicit unsafe forms, such as `unsafe extern` and `#[unsafe(no_mangle)]`, when the language requires them. + +## Public API Notes + +Public macros are public API. Name them clearly, keep their accepted syntax small, document the generated behavior, and avoid exporting helper macros unless callers are meant to use them directly. + +## Example + +Keep the default lint strict: + +```toml +[workspace.lints.rust] +unsafe_code = "deny" +``` + +Use a macro when it removes repeated, mechanical boilerplate that ordinary functions and traits cannot. This macro fits opaque, server-assigned IDs that are always valid by construction and share an identical, validation-free shape. IDs that need validation, a custom `Display`, or distinct behavior should be written by hand following the newtype guidance. + +```rust +macro_rules! define_id_type { + ($name:ident) => { + #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] + pub struct $name(String); + + impl $name { + pub fn new(value: impl Into) -> Self { + Self(value.into()) + } + + pub fn as_str(&self) -> &str { + &self.0 + } + } + + impl std::fmt::Display for $name { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(&self.0) + } + } + }; +} + +define_id_type!(UserId); +define_id_type!(WorkspaceId); +define_id_type!(RunId); +``` + +The macro earns its place only because every generated type is identical and correct on its own. If one ID needs validation or different behavior, or if the macro stops being simpler than the expanded code, delete it and write the types directly. + +Bad: add ad hoc unsafe to bypass ordinary bounds or checks. + +```rust +let item = unsafe { items.get_unchecked(index) }; +``` + +Good: use safe Rust unless an unsafe exception has been approved and documented. + +```rust +let item = items + .get(index) + .ok_or_else(|| IndexError { index, len: items.len() })?; +``` + +## Exceptions + +- Allow unsafe in crates whose purpose requires it, such as FFI bindings, low-level platform integration, carefully measured performance primitives, or hardware-adjacent code. +- Keep an existing unsafe crate's local policy if removing unsafe is outside the current task; do not spread that exception to other crates. +- Use small test macros when they make repetitive case tables easier to scan. +- Use generated code or proc macros when they replace large, error-prone handwritten implementations with a smaller source of truth. diff --git a/.fabro/skills/rust-style-guide/guidelines/validation-and-invariants.md b/.fabro/skills/rust-style-guide/guidelines/validation-and-invariants.md new file mode 100644 index 000000000..12fa01c9e --- /dev/null +++ b/.fabro/skills/rust-style-guide/guidelines/validation-and-invariants.md @@ -0,0 +1,111 @@ +# Validation and Invariants + +## Rule + +Validate data at input boundaries, encode invariants in newtypes and constructors, and let internal code operate on trusted types instead of repeatedly checking raw values. + +## Why + +Boundary validation makes invalid data fail early and keeps checks close to parsing. Once a value has a validated type, internal code can rely on the invariant without repeating defensive checks everywhere. + +## Do + +- Validate external input at boundaries: CLI args, HTTP requests, config files, environment variables, database rows, messages, and deserialization. +- Convert raw values into domain types as soon as practical. +- Use `try_new`, `parse`, `TryFrom`, or `FromStr` for fallible construction. +- Keep invariant-bearing fields private. +- Use newtypes for validated strings, IDs, units, ranges, and values with public API meaning. +- Use `NonZero*` types when zero is invalid and the primitive representation still matters. +- Use fallible startup validation for configuration so services fail before doing work with invalid settings. +- Pass validated types through internal code instead of raw `String`, `u64`, or `bool` values. +- Deserialize into types that enforce invariants, or deserialize raw input and convert with `TryFrom`. +- Use assertions for internal invariants that should already have been guaranteed by earlier parsing or construction. + +## Avoid + +- Do not validate the same invariant at every use site by habit. +- Do not accept raw primitives deep inside the system when a validated domain type already exists. +- Do not expose public fields that allow callers to break a type's invariant. +- Do not make `new` panic for caller-provided input; use `try_new` for validation. +- Do not rely on comments like `// must be non-empty` when the type can enforce it. +- Do not push every invariant into typestate or generics when a fallible constructor is enough. +- Do not treat deserialization as validation unless the deserialized type enforces the invariant. + +## Library vs Application + +Libraries should encode public API invariants in types and constructors so callers cannot accidentally create invalid values. Applications should validate at process and request boundaries, then pass trusted domain types through services, jobs, and handlers. + +## Example + +```rust +use std::path::{Path, PathBuf}; + +use thiserror::Error; + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct WorkspaceName(String); + +impl WorkspaceName { + pub fn try_new(value: &str) -> Result { + let value = value.trim(); + + if value.is_empty() { + return Err(WorkspaceNameError::Empty); + } + + let valid = value + .chars() + .all(|character| character.is_ascii_alphanumeric() || character == '-'); + if !valid { + return Err(WorkspaceNameError::InvalidCharacter); + } + + Ok(Self(value.to_owned())) + } + + pub fn as_str(&self) -> &str { + &self.0 + } +} + +#[derive(Clone, Copy, Debug, Eq, Error, PartialEq)] +pub enum WorkspaceNameError { + #[error("workspace name must not be empty")] + Empty, + #[error("workspace name must contain only ASCII letters, digits, or '-'")] + InvalidCharacter, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Workspace { + name: WorkspaceName, +} + +impl Workspace { + pub fn new(name: WorkspaceName) -> Self { + Self { name } + } + + pub fn name(&self) -> &WorkspaceName { + &self.name + } +} + +pub fn create_workspace(raw_name: &str) -> Result { + let name = WorkspaceName::try_new(raw_name)?; + Ok(Workspace::new(name)) +} + +pub fn workspace_path(root: &Path, name: &WorkspaceName) -> PathBuf { + root.join(name.as_str()) +} +``` + +`workspace_path` does not re-check for an empty name or invalid character because the `WorkspaceName` constructor already owns that invariant. + +## Exceptions + +- Re-check constraints that depend on changing external state, such as authorization, database uniqueness, file existence, quotas, or time. +- Re-validate data loaded from untrusted storage, legacy tables, external caches, or older serialized formats. +- Use runtime checks inside hot paths only when profiling or safety requirements show they are needed. +- Use typestate when ordered workflow states are important enough that invalid transitions should not compile. diff --git a/.fabro/skills/rust-style-guide/workflows/code-review-refactor.md b/.fabro/skills/rust-style-guide/workflows/code-review-refactor.md new file mode 100644 index 000000000..9e65605f2 --- /dev/null +++ b/.fabro/skills/rust-style-guide/workflows/code-review-refactor.md @@ -0,0 +1,53 @@ +# Code Review and Refactor + +Use this workflow when reviewing, refactoring, or changing existing Rust code in a project that already has structure and conventions. + +## Required Guidelines + +Load [guidelines.md](../guidelines.md), then load these guideline pages as needed: + +- [Library vs application conventions](../guidelines/library-vs-application-conventions.md) +- [Public API evolution](../guidelines/public-api-evolution.md) +- [rustc and Clippy lints](../guidelines/rustc-and-clippy-lints.md) +- [Property tests, snapshots, benchmarks, and CI](../guidelines/property-tests-snapshots-benchmarks-and-ci.md) +- [Panics, unwrap, expect, and assertions](../guidelines/panics-unwrap-expect-and-assertions.md) +- [Error propagation, context, and messages](../guidelines/error-propagation-context-and-messages.md) +- [Ownership, borrowing, and clone policy](../guidelines/ownership-borrowing-and-clone-policy.md) +- [Concurrency primitives](../guidelines/concurrency-primitives.md) +- [Logging and observability](../guidelines/logging-and-observability.md) +- [Unsafe code and macros](../guidelines/unsafe-code-and-macros.md) + +Load narrower pages for the code you touch, such as newtypes, traits, async task lifecycle, validation, collections, or documentation. + +## Workflow + +1. Classify the code first: published library API, shared in-repo library, application/service, CLI, test support, or tests. +2. Identify the behavioral surface being changed and the callers affected. Treat externally consumed APIs as stricter than internal application code. +3. Load only the guideline pages relevant to that surface. +4. Scan high-risk patterns before editing: accidental public API changes, hidden panics, flattened errors, unnecessary clones or lifetimes, locks across `.await`, blocking work on async paths, unredacted logs, unsafe, and macro-generated behavior. +5. Make the smallest coherent change. Preserve existing local style unless it conflicts with this guide or the requested behavior. +6. Add or update tests at the level where the behavior is observable. +7. Run verification appropriate to the change: formatter, Clippy, tests, MSRV/all-features checks, or a narrower command when the project makes the full suite impractical. +8. Report what changed, what was verified, and any exceptions or skipped checks with the reason. + +## Review Checklist + +- Scope: Did the change affect library, application, CLI, or test-only behavior? +- API: Did `pub`, re-exports, features, MSRV, or public dependencies change? +- Errors: Are recoverable failures returned with source chains and boundary context? +- Panics: Are `unwrap`, `expect`, `panic!`, and assertions limited to invariants? +- Ownership: Are clones, borrows, and owned snapshots named honestly? +- Async/concurrency: Are task ownership, cancellation, blocking work, and lock scopes explicit? +- Observability: Are logs structured, low-noise, and free of secrets? +- Unsafe/macros: Is any unsafe or macro complexity justified, isolated, and documented? +- Tests: Does coverage protect behavior rather than private implementation churn? +- Verification: Were the commands run fresh, and are skipped checks explained? + +## Avoid + +- Do not load every guideline page by default. +- Do not refactor unrelated code while reviewing a focused change. +- Do not apply library-level ceremony to private application internals without a reason. +- Do not relax lint, test, or safety policy to make a local change easier. +- Do not report a change as verified without naming the commands that ran. +- Do not hide exceptions; document why the local case differs from the default rule. diff --git a/.fabro/skills/rust-style-guide/workflows/new-rust-project.md b/.fabro/skills/rust-style-guide/workflows/new-rust-project.md new file mode 100644 index 000000000..2d3afd043 --- /dev/null +++ b/.fabro/skills/rust-style-guide/workflows/new-rust-project.md @@ -0,0 +1,189 @@ +# New Rust Project + +Use this workflow when creating or configuring a new Rust crate, workspace, CLI, library, service, or application. + +## Required Guidelines + +Load [guidelines.md](../guidelines.md), then load these guideline pages as needed: + +- [House style and Rust philosophy](../guidelines/house-style-and-rust-philosophy.md) +- [Library vs application conventions](../guidelines/library-vs-application-conventions.md) +- [Rust edition and MSRV](../guidelines/rust-edition-and-msrv.md) +- [rustfmt and formatting](../guidelines/rustfmt-and-formatting.md) +- [rustc and Clippy lints](../guidelines/rustc-and-clippy-lints.md) +- [Cargo, workspaces, features, and dependencies](../guidelines/cargo-workspaces-features-and-dependencies.md) +- [Testing and doctests](../guidelines/testing-and-doctests.md) +- [Property tests, snapshots, benchmarks, and CI](../guidelines/property-tests-snapshots-benchmarks-and-ci.md) +- [Unsafe code and macros](../guidelines/unsafe-code-and-macros.md) + +Load the async guideline when the project is async. Load logging, public API, and error guidelines when those surfaces apply. + +## Workflow + +1. Identify the project shape: library, application, CLI, service, test support crate, or mixed workspace. +2. Make the sync-vs-async posture explicit before adding async dependencies; async projects use Tokio. +3. Prefer a workspace when multiple crates share version, edition, dependencies, lints, or profiles. +4. Set Rust 2024 and `rust-version = "1.85"` unless the project already has different constraints. +5. Add pinned rustfmt configuration and use `nightly-2026-04-14` for formatting. +6. Add curated workspace lints and tailor project-specific `clippy.toml` guardrails before copying async/blocking disallow rules. +7. Audit every Rust source file under `src/`, including nested modules: classify it as trivial or nontrivial, and add bottom-of-file `#[cfg(test)] mod tests` for each nontrivial file's focused behavior and private helpers. Record a specific exception when a nontrivial file does not get module-local tests. +8. Use `cargo nextest run --workspace --all-targets --all-features` as the normal workspace test runner. +9. Skip doctests by default; run `cargo test --doc --workspace --all-features` only when the project explicitly opts into maintaining rustdoc examples. +10. Add dependencies only when they remove real complexity or provide mature domain behavior. +11. Verify the project with the configured commands before handing it off. + +## Cargo Baseline + +Use a workspace shape when the project is likely to grow beyond one crate: + +```toml +[workspace] +members = ["crates/*"] +resolver = "3" + +[workspace.package] +edition = "2024" +rust-version = "1.85" + +[workspace.dependencies] +anyhow = "1" +serde = { version = "1", features = ["derive"] } +thiserror = "2" +tracing = "0.1" + +[workspace.lints.rust] +unsafe_code = "deny" +unreachable_pub = "warn" + +[workspace.lints.clippy] +pedantic = { level = "warn", priority = -2 } +allow_attributes_without_reason = "warn" + +implicit_hasher = "allow" +missing_errors_doc = "allow" +missing_panics_doc = "allow" +module_name_repetitions = "allow" +must_use_candidate = "allow" +similar_names = "allow" +struct_excessive_bools = "allow" +too_many_arguments = "allow" +too_many_lines = "allow" +cast_precision_loss = "allow" +doc_markdown = "allow" + +print_stdout = "warn" +print_stderr = "warn" +dbg_macro = "warn" +empty_drop = "warn" +empty_structs_with_brackets = "warn" +disallowed_methods = "deny" +exit = "warn" +get_unwrap = "warn" +unwrap_used = "deny" +rc_buffer = "warn" +rc_mutex = "warn" +rest_pat_in_fully_bound_structs = "warn" +use_self = "warn" +wildcard_imports = "warn" +absolute_paths = "warn" +``` + +Workspace lint inheritance is opt-in per member crate: every member crate must set `[lints] workspace = true` in its own `Cargo.toml`, or the workspace lint tables do nothing. + +```toml +[package] +name = "example-crate" +edition.workspace = true +rust-version.workspace = true + +[lints] +workspace = true +``` + +For a single crate, put the same package fields and lint tables in the crate's `Cargo.toml` instead of a workspace root, renaming the tables to `[lints.rust]` and `[lints.clippy]`; copied `[workspace.lints.*]` tables do nothing in a standalone manifest. + +For async projects, add Tokio deliberately to the package or workspace dependencies: + +```toml +tokio = { version = "1", features = ["full"] } +``` + +## rustfmt Baseline + +Use this `rustfmt.toml` at the project root: + +```toml +edition = "2024" +style_edition = "2024" + +max_width = 100 +comment_width = 80 + +group_imports = "StdExternalCrate" +imports_granularity = "Module" + +use_field_init_shorthand = true +merge_derives = true +overflow_delimited_expr = true +format_code_in_doc_comments = true +format_macro_matchers = true +normalize_doc_attributes = true +wrap_comments = true + +struct_field_align_threshold = 20 +enum_discrim_align_threshold = 20 +``` + +Install the pinned formatter, the MSRV toolchain, and the test runner used by the verification commands: + +```sh +rustup toolchain install nightly-2026-04-14 --profile minimal --component rustfmt +rustup toolchain install 1.85.0 --profile minimal +cargo install cargo-nextest --locked +``` + +## Optional Clippy Guardrails + +Use `clippy.toml` for project-specific architectural guardrails. For async projects, review rules like these before copying them: + +```toml +allow-unwrap-in-tests = true +allow-unwrap-types = ["std::sync::LockResult"] + +disallowed-methods = [ + { path = "std::thread::sleep", reason = "Prefer tokio::time::sleep on Tokio paths; document intentional blocking sleeps with #[expect(clippy::disallowed_methods, reason = \"...\")]", replacement = "tokio::time::sleep" }, + { path = "std::thread::spawn", reason = "Prefer Tokio task APIs on async paths; document intentional dedicated OS threads with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, + { path = "std::process::Command::new", reason = "Prefer tokio::process::Command on Tokio paths; document intentional synchronous subprocesses with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, +] + +disallowed-types = [ + { path = "std::io::Read", reason = "Blocking trait; prefer tokio::io::AsyncReadExt on Tokio paths. Document intentional sync I/O with #[expect(clippy::disallowed_types, reason = \"...\")]" }, + { path = "std::net::TcpStream", reason = "Blocking socket; prefer tokio::net::TcpStream on Tokio paths. Document intentional sync networking with #[expect(clippy::disallowed_types, reason = \"...\")]" }, +] +``` + +## Verification Commands + +Use these commands as the default new-project validation set: + +```sh +cargo +nightly-2026-04-14 fmt --check --all +cargo clippy --locked --workspace --all-targets --all-features -- -D warnings +cargo nextest run --workspace --all-targets --all-features +cargo +1.85.0 check --workspace --all-targets --all-features +``` + +If the project intentionally maintains doctests, add: + +```sh +cargo test --doc --workspace --all-features +``` + +## Avoid + +- Do not add async casually; document the project posture first. +- Do not add every standard dependency to every project by default. +- Do not copy Tokio-specific Clippy guardrails into sync projects. +- Do not create broad preludes, public facades, or feature flags before the project needs them. +- Do not lower `unsafe_code = "deny"` unless the new crate's purpose requires unsafe code. +- Do not let integration tests under `tests/` silently replace module-local tests for nontrivial source files. diff --git a/.fabro/skills/rust-style-guide/workflows/performance-investigation.md b/.fabro/skills/rust-style-guide/workflows/performance-investigation.md new file mode 100644 index 000000000..6155d0f8f --- /dev/null +++ b/.fabro/skills/rust-style-guide/workflows/performance-investigation.md @@ -0,0 +1,56 @@ +# Performance Investigation + +Use this workflow when investigating slow Rust code, performance regressions, excess resource use, or proposed optimization work. + +## Required Guidelines + +Load [guidelines.md](../guidelines.md), then load these guideline pages as needed: + +- [Property tests, snapshots, benchmarks, and CI](../guidelines/property-tests-snapshots-benchmarks-and-ci.md) +- [Collections and data structures](../guidelines/collections-and-data-structures.md) +- [Ownership, borrowing, and clone policy](../guidelines/ownership-borrowing-and-clone-policy.md) +- [Concurrency primitives](../guidelines/concurrency-primitives.md) +- [Cancellation, shutdown, and blocking work](../guidelines/cancellation-shutdown-and-blocking-work.md) +- [Logging and observability](../guidelines/logging-and-observability.md) + +Load async, Cargo/dependency, or public API guidelines when the suspected bottleneck touches those surfaces. + +## Workflow + +1. Define the symptom, workload, success metric, and acceptable tradeoffs before changing code. +2. Reproduce the issue with representative inputs in a release-like build; do not trust debug timings. +3. Record a baseline measurement and the exact command, input, machine, and feature set used. +4. Profile before optimizing. Use the project-standard profiler, `flamegraph`, `samply`, Instruments, `perf`, Tokio Console, or service telemetry as appropriate. +5. Identify the hot path from evidence, then classify the bottleneck: algorithm, allocation/copying, locking, blocking I/O, async scheduling, serialization, or logging overhead. +6. Change one thing at a time. Prefer simpler data flow, better algorithms, fewer clones, or narrower locks before allocator, profile, or compiler tuning. +7. Rerun the same measurement and keep the change only when it materially improves the target metric without violating style or correctness. +8. Add a benchmark, load test, regression test, or release note when the performance behavior is important enough to preserve. + +## Measurement Commands + +Use the tool that matches the code shape. Examples: + +```sh +cargo bench +cargo test --release targeted_case -- --nocapture +hyperfine 'target/release/app input.txt' +cargo flamegraph --bench parser +``` + +Profilers need debug symbols to produce readable stacks; before capturing flamegraphs, enable debuginfo in the profiled release or bench profile (or a dedicated profiling profile): + +```toml +[profile.release] +debug = true +``` + +For async services, prefer production-like tracing, metrics, load tests, and Tokio task/lock visibility over isolated microbenchmarks when the problem is scheduling or contention. + +## Avoid + +- Do not optimize before reproducing and measuring the issue. +- Do not compare debug builds to release builds. +- Do not tune allocators, profiles, `target-cpu`, or `#[inline]` before identifying a hot path. +- Do not keep changes that make code harder to understand without a measured win. +- Do not change several variables at once and then guess which one mattered. +- Do not use benchmarks with toy inputs when real workloads have different sizes, distributions, or contention. diff --git a/.fabro/skills/rust-style-guide/workflows/reusable-library-release.md b/.fabro/skills/rust-style-guide/workflows/reusable-library-release.md new file mode 100644 index 000000000..9f163acba --- /dev/null +++ b/.fabro/skills/rust-style-guide/workflows/reusable-library-release.md @@ -0,0 +1,106 @@ +# Reusable Library Release Verification + +Use this workflow before releasing or handing off a reusable library crate, especially when it has optional features, public APIs, or an explicit MSRV. + +## Required Guidelines + +Load [guidelines.md](../guidelines.md), then load these guideline pages as needed: + +- [Library vs application conventions](../guidelines/library-vs-application-conventions.md) +- [Rust edition and MSRV](../guidelines/rust-edition-and-msrv.md) +- [Cargo, workspaces, features, and dependencies](../guidelines/cargo-workspaces-features-and-dependencies.md) +- [rustc and Clippy lints](../guidelines/rustc-and-clippy-lints.md) +- [Testing and doctests](../guidelines/testing-and-doctests.md) +- [Property tests, snapshots, benchmarks, and CI](../guidelines/property-tests-snapshots-benchmarks-and-ci.md) +- [Public API evolution](../guidelines/public-api-evolution.md) + +Also load error, documentation, unsafe, async, or observability guidelines when those surfaces are part of the library API. + +## Workflow + +1. Confirm the crate is a reusable library and identify its public API, feature flags, and declared MSRV. +2. Verify all features are additive. If features are intentionally incompatible, document the supported feature matrix before release. +3. Check that public dependency types are exposed only when they are part of the intended contract. +4. Run the default all-features verification commands. +5. Run dependency and supply-chain checks when the project has the tools installed. +6. Verify out-of-box behavior for the default feature set. +7. For published crates, run `cargo semver-checks` to detect accidental public API breaks and `cargo publish --dry-run` to validate the release artifact. +8. Record any MSRV bump, public API break, new optional dependency, or feature behavior change in release notes or the changelog. + +## Default Verification + +Use these commands before releasing a reusable library: + +Use `--workspace` when verifying every library crate in the workspace. When releasing one crate from a mixed workspace, replace `--workspace` with `-p crate-name`. + +Run the MSRV check with the crate's declared `rust-version` from step 1; `+1.85.0` below is illustrative, so a crate that declares `rust-version = "1.78"` is verified with `cargo +1.78.0 check`. + +```sh +cargo +nightly-2026-04-14 fmt --check --all +cargo clippy --locked --workspace --all-targets --all-features -- -D warnings +cargo nextest run --workspace --all-targets --all-features +cargo +1.85.0 check --workspace --all-targets --all-features +cargo check --workspace --all-targets --no-default-features +``` + +If the project intentionally maintains doctests, add: + +```sh +cargo test --doc --workspace --all-features +``` + +## Feature Matrix + +Use `--all-features` by default. Replace it with an explicit matrix only when a crate intentionally has incompatible feature sets. + +For an explicit matrix, verify each supported combination that users can depend on: + +```sh +cargo check --workspace --all-targets --no-default-features +cargo check --workspace --all-targets --features serde +cargo check --workspace --all-targets --features tokio +cargo check --workspace --all-targets --features "serde tokio" +``` + +Keep the matrix small and documented. If the matrix grows large, reconsider whether the features are too granular or too tightly coupled. + +## Dependency Checks + +When the project has the tools installed, run: + +```sh +cargo audit +cargo deny check +cargo machete +``` + +Treat these as release gates for published crates when the project has adopted them. For internal libraries, use them when dependency churn, public dependency exposure, or supply-chain risk is material. + +## Semver and Artifact Checks + +For published crates, detect accidental public API breaks and validate the release artifact: + +```sh +cargo semver-checks +cargo publish --dry-run +``` + +Install the checker once with `cargo install cargo-semver-checks --locked`. Use `cargo package` instead of the dry-run publish when the crate is not published to a registry. Treat any semver-major finding as either a bug to fix or an intentional break to record in step 8. + +## Out-of-Box Build + +Reusable libraries should build with the default feature set without hidden setup: + +```sh +cargo check --workspace --all-targets +``` + +For crates with minimal default features, also verify the no-default-features build. Do not require users to enable unrelated integrations to compile the core crate. + +## Avoid + +- Do not release a library after checking only the default feature set when optional feature-gated code changed. +- Do not use `--all-features` as a substitute for documenting intentionally incompatible feature combinations. +- Do not let a dependency update raise MSRV without making that decision explicit. +- Do not add release-only verification commands that are never run locally or in CI. +- Do not require security or dependency tools for every tiny internal crate unless the project has adopted those gates.