Skip to content

STYLE_GUIDE.md

Latest commit

 

History

History
1572 lines (1135 loc) · 59.6 KB

STYLE_GUIDE.md

File metadata and controls

1572 lines (1135 loc) · 59.6 KB
Raw

Style Guide

Conventions for code, documentation, and other project artifacts in the omni-dev project. Each item has a unique ID for easy reference.

Tag-based lookup

Before writing or reviewing code, documentation, or other project artifacts, identify which tags apply to the changes and search this file for those tags. Each rule has a Tags line immediately after its heading.

Search command: grep "Tags:.*<tag>" docs/STYLE_GUIDE.md returns matching rule headings.

When you are… Search for tags
Adding or modifying a function code-style, naming, documentation
Adding a type, enum, or trait api-design, naming, documentation
Adding or changing error handling error-handling
Creating or restructuring a module/file module-organization, naming
Writing or updating tests testing
Adding a new Atlassian API client method testing, api-design
Adding a new CLI command testing, module-organization
Changing visibility (pub, pub(crate)) api-design, module-organization
Adding constants or replacing magic values code-style, naming
Writing commit messages commits
After creating commits (before push / PR) commits
Suppressing a lint or considering unsafe code-style, unsafe
Writing or updating an ADR adrs
Adding an MCP tool, resource, or param struct api-design, module-organization, testing, documentation
Adding or modifying a docs/plan/ file documentation, adrs
Reading env vars, or testing env-dependent code testing, module-organization
Reviewing code for style compliance All tags relevant to the changed code

STYLE-0000: Style guide structure

Tags: meta

Situation

A new convention needs to be added to this style guide.

Guidance

Assign the next sequential ID (currently next is STYLE-0030) and include:

  1. A Tags line immediately after the heading — a comma-separated list of category labels from the tag vocabulary below.
  2. Three subheadings:
    • Situation — when this rule applies
    • Guidance — what to do (with examples where helpful)
    • Motivation — why this rule exists

Tag vocabulary (extend as needed):

Tag Covers
meta Style guide structure and process
error-handling Error types, context messages, panics, suppression
module-organization File layout, visibility, cohesion
naming Naming conventions for types, functions, files
commits Commit message format, scope rules, discipline
documentation Doc comments, examples
testing Test structure, fixtures, snapshots
code-style Imports, clippy, constants, function length
api-design Ownership, must_use, type safety, string params
unsafe Unsafe code policy
adrs Architecture Decision Record format and process

A rule may have multiple tags — e.g., a rule about error messages in tests could be tagged error-handling, testing.

Items are ordered by ID. Do not group items under section headings; use tags for categorisation instead.

Motivation

Consistent structure makes the guide scannable, and stable IDs allow code review comments and ADRs to reference specific rules unambiguously. Tags replace section headings so that items can remain in strict ID order without needing to be shuffled between sections when categories overlap or new categories are introduced.

STYLE-0001: Default error type

Tags: error-handling

Situation

A function can fail and needs to return an error.

Guidance

Use anyhow::Result<T> as the return type. Import both Context and Result:

use anyhow::{Context, Result};

fn open_repo() -> Result<Repository> {
    Repository::open(".").context("Failed to open git repository")?;
    // ...
}

Reserve thiserror enums for domain boundaries where callers need to match on specific error variants. Currently the only custom error type is ClaudeError in src/claude/error.rs, which covers API-specific failure modes (key not found, rate limit, network error). These convert to anyhow::Error automatically via the blanket impl.

Use anyhow::bail!() for early returns with an error message:

anyhow::bail!("Repository is in detached HEAD state");

Motivation

anyhow provides lightweight error chaining without defining boilerplate error types. Reserving thiserror for domain boundaries keeps the type surface small while still allowing pattern matching where it matters.

STYLE-0002: Context message style

Tags: error-handling

Situation

Adding .context() or .with_context() to a fallible operation.

Guidance

Write context messages in sentence case describing the failed operation:

// Good — describes the operation that failed
.context("Failed to get HEAD reference")?;
.context("Cannot amend commits with uncommitted changes")?;
.context("Not in a git repository")?;

// Bad — includes function name
.context("open_repo: could not open")?;

// Bad — too generic
.context("error")?;

Use .with_context() when the message needs runtime values:

.with_context(|| format!("Failed to parse start commit: {}", start_spec))?;

Prefer .context() over .with_context() for static messages since it avoids the closure allocation.

Motivation

Sentence-case messages read naturally in error chains printed by main.rs. Describing the operation (not the function) keeps messages useful regardless of refactoring. The with_context pattern avoids allocating format strings on the success path.

STYLE-0003: Panicking operations

Tags: error-handling

Situation

Considering unwrap(), expect(), or other panicking calls.

Guidance

unwrap() is acceptable in these cases only:

  • Static regex — use std::sync::LazyLock so the pattern is compiled once and the unwrap() is confined to the initialiser. Clippy's invalid_regex lint (deny by default) validates the literal at compile time, so the unwrap() is provably safe.

    use std::sync::LazyLock;
use regex::Regex;

static SCOPE_RE: LazyLock<Regex> =
    LazyLock::new(|| Regex::new(r"^[a-z][a-z0-9-]*$").unwrap());

  • Known-safe constructors — FixedOffset::east_opt(0).unwrap() where the argument is a constant that cannot fail.

  • Test code — tests may use unwrap() freely.

expect() is acceptable for truly catastrophic I/O that should terminate the process:

io::stdout().flush().expect("Failed to flush stdout");

Never use unwrap() or expect() on user-supplied or runtime data in library code. Use ? with .context() instead.

Motivation

Panics in library code produce poor diagnostics and cannot be handled by callers. Limiting panics to provably-safe or catastrophic cases keeps the error surface predictable. A lazy static avoids recompiling the regex on every call and makes the safety argument obvious at the declaration site.

STYLE-0004: Module file layout

Tags: module-organization

Situation

Adding a new module or reorganizing an existing one.

Guidance

Use the named-file layout (Rust 2018+) for modules with submodules. Place the parent module in a file named after the module alongside a directory of the same name:

src/
├── claude.rs           # declares submodules, re-exports public types
├── claude/
│   ├── client.rs
│   ├── error.rs
│   ├── prompts.rs
│   ├── ai.rs           # declares ai submodules
│   ├── ai/
│   │   ├── bedrock.rs
│   │   ├── claude.rs
│   │   └── openai.rs
│   ├── context.rs      # declares context submodules
│   └── context/
│       ├── branch.rs
│       ├── discovery.rs
│       ├── files.rs
│       └── patterns.rs
├── core.rs             # no submodules, so just a single file
├── lib.rs
└── main.rs

Do not use mod.rs for new modules. The named-file layout gives every module root a unique filename, which avoids ambiguous editor tabs and search results when multiple mod.rs files exist.

Re-export key public types from each module root so consumers can import from the parent module:

// src/git.rs
pub use amendment::AmendmentHandler;
pub use commit::{CommitAnalysis, CommitInfo};
pub use repository::GitRepository;

Only re-export types that appear in the module's public API signatures. Internal helpers, intermediate types, and implementation details should stay private to their submodule even if they are pub there. A re-export is a promise that the type is part of the module's contract.

Motivation

The named-file layout is recommended by the Rust Book and is the default assumed by rust-analyzer. Each module root has a distinct filename (e.g., claude.rs vs context.rs) instead of multiple mod.rs files, making editor tabs, file search, and git log output unambiguous. Re-exports in the module root present a clean public interface per module. Limiting re-exports to API-surface types prevents leaking implementation details that would be hard to remove later.

STYLE-0005: Visibility

Tags: module-organization, api-design

Situation

Deciding whether to make an item pub, pub(crate), or private.

Guidance

Default to private (no visibility modifier). Use three visibility levels:

Visibility Meaning Use when
(none) Private to the module Internal helpers
pub(crate) Visible within the crate Shared across modules but not part of the external API
pub Fully public Part of the crate's published API surface 
impl AmendmentFile {
    pub fn load_from_file<P: AsRef<Path>>(path: P) -> Result<Self> { ... }  // public API
    pub(crate) fn validate_schema(&self) -> Result<()> { ... }              // crate-internal
    fn format_multiline_yaml(&self, yaml: &str) -> String { ... }          // module-private
}

When in doubt, start private and widen visibility only when needed. Prefer pub(crate) over pub for items that other modules need but external consumers should not rely on.

The rustc lint unreachable_pub (allowed by default) can be enabled to detect pub items that are not actually reachable from outside the crate.

Motivation

Minimal visibility reduces the API surface that must be maintained (Effective Rust, Item 22). Using pub(crate) for internal cross-module items prevents accidentally promising API stability to external consumers. Making a public item private is a breaking change; making a private item public is not.

STYLE-0006: Naming patterns

Tags: naming

Situation

Naming a new type, function, CLI command, environment variable, or YAML field.

Guidance

Element Convention Examples
Structs / Enums PascalCase CommitInfo, ClaudeError, WorkType
Traits PascalCase (adj/verb) AiClient, Serialize, Display
Functions/Methods snake_case from_git_commit(), analyze_commit()
Type aliases PascalCase Result<T> (for crate-local aliases)
Constants UPPER_SNAKE_CASE VERSION
Environment vars UPPER_SNAKE_CASE CLAUDE_API_KEY, AI_SCRATCH
CLI commands kebab-case help-all, commit message view
YAML fields snake_case original_message, in_main_branches
Modules / files snake_case model_config.rs, ai_scratch.rs

Project-specific pattern — *ForAI suffix: When a data structure has a variant that includes additional content for AI processing (e.g., full diff text), suffix the variant with ForAI:

pub struct CommitInfo { ... }       // standard version
pub struct CommitInfoForAI { ... }  // includes diff_content field

Motivation

Standard Rust naming (PascalCase types, snake_case functions) is enforced by compiler warnings and clippy. The *ForAI suffix convention makes it immediately clear which structs carry the heavier AI-oriented payload. Kebab-case CLI commands follow clap conventions and are standard across Unix tools.

STYLE-0007: Commit message format

Tags: commits

Situation

Writing a commit message.

Guidance

Follow .omni-dev/commit-guidelines.md for the full specification including types, scopes, subject line rules, body guidelines, and breaking change conventions. See omni-dev-directory.md for the file's format contract, validation behaviour, and how it is resolved relative to local overrides and the global fallback.

The commit guidelines must themselves follow Conventional Commits and remain consistent with the scope definitions in .omni-dev/scopes.yaml:

  1. Scope list — the - `name` - description bullets in the ## Scopes section of commit-guidelines.md must match the scopes defined in scopes.yaml name-for-name and description-for-description. When a scope is added, removed, or renamed in scopes.yaml, update commit-guidelines.md to match. Prose in the same section is not part of the list — the note recording the ecosystem default scopes that merge_ecosystem_scopes adds (cargo, core, lib, test for a Rust project) belongs there, outside the bullets.
  2. Examples — every <scope> used in the ## Examples section must be a scope that exists in scopes.yaml. Do not use scopes from other projects or hypothetical scopes.
  3. Single source of truth — scopes.yaml is the canonical definition. The scope list in commit-guidelines.md exists only so the AI prompt has inline context; it must never diverge from the YAML file.
  4. Tree coverage — every tracked file under src/, editors/ and .github/ must be matched by some scope's file_patterns (or listed in the allow: list for files that legitimately belong to no subsystem). When a new subsystem or module facade lands, scopes.yaml must gain a pattern for it in the same change.

Clauses 1–3 are enforced by tests/commit_guidelines_scopes_test.rs; clause 4 is enforced by omni-dev config scopes lint, exercised end-to-end by the binary_config_scopes_lint_* tests in tests/integration_test.rs. Together they make a divergence fail the build rather than silently degrading the prompt.

Motivation

Keeping the detailed commit specification in .omni-dev/commit-guidelines.md allows the AI context system to consume it directly, avoiding duplication between this style guide and the machine-readable guidelines.

Both commit-guidelines.md and scopes.yaml are injected into the AI prompt for commit checking. If the two files list different scopes the AI receives contradictory instructions and may incorrectly flag valid scopes as invalid — or accept scopes that no longer exist.

The list is hand-maintained, and before it was tested it had drifted by nine entries (#1421) without producing a visible failure: the judge happened to resolve the contradiction in favour of scopes.yaml. Relying on that is a coin flip, which is why the rule is now checked by a test.

STYLE-0008: Doc comments

Tags: documentation

Situation

Adding or updating documentation on a module, type, or function.

Guidance

Module-level docs — every module file starts with a //! comment:

//! Git commit operations and analysis.

Item-level docs — every public struct, enum, field, variant, and method gets ///:

/// Represents a single commit with its metadata and analysis.
pub struct CommitInfo {
    /// Full SHA-1 hash of the commit.
    pub hash: String,
    /// Commit author name and email address.
    pub author: String,
}

Summary line style — write in third-person singular present indicative per RFC 505. Use full sentences ending with a period:

/// Creates a `CommitInfo` from a `git2::Commit`.
pub fn from_git_commit(...) -> Result<Self> { ... }

/// Returns the suggested level of detail for commit messages.
pub fn suggested_verbosity(&self) -> VerbosityLevel { ... }
Correct (third-person) Incorrect (imperative)
/// Returns the length. /// Return the length.
/// Creates a new client. /// Create a new client.
/// Parses the input string. /// Parse the input string.

The crate-level lint #![warn(missing_docs)] in src/lib.rs will warn on any public item missing a doc comment.

# Examples sections — public functions that are not self-explanatory should include a doc example. These are compiled and run by cargo test, so they serve as both documentation and regression tests:

/// Parses a conventional commit subject line.
///
/// # Examples
///
/// ```
/// let parsed = parse_subject("feat(cli): add --fresh flag");
/// assert_eq!(parsed.commit_type, "feat");
/// assert_eq!(parsed.scope, Some("cli"));
/// ```
pub fn parse_subject(input: &str) -> ParsedSubject { ... }

Doc examples are not required for trivial getters, builders, or From/Into implementations where the behaviour is obvious from the type signature.

Motivation

The third-person convention matches the Rust standard library and rustdoc output, where doc summaries read as descriptions of what the item does (e.g., Vec::push — "Appends an element to the back of a collection."). RFC 505 codifies this as the official Rust API documentation style. #![warn(missing_docs)] turns documentation into a compile-time obligation rather than an afterthought. Doc examples provide compile-tested usage patterns and catch API regressions that unit tests might miss.

STYLE-0009: Test structure

Tags: testing

Situation

Writing a new test.

Guidance

Place unit tests in a #[cfg(test)] mod tests block at the end of the source file:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn app_creation() {
        let app = App::new();
        assert!(!app.config.verbose);
    }
}

Naming pattern: <thing_being_tested>[_<condition>] — omit the test_ prefix since the #[test] attribute and tests module already identify these as tests. Clippy's redundant_test_prefix lint (restriction group) flags the prefix as redundant.

fn load_model_registry() { ... }
fn parse_beta_header_valid() { ... }
fn app_with_config() { ... }

When a test uses ? for error propagation, return Result<()>:

#[test]
fn amend_command_with_temporary_repo() -> Result<()> {
    let repo = TestRepo::new()?;
    // ...
    Ok(())
}

Place integration tests in the tests/ directory.

Test attributes:

  • #[should_panic] — avoid in favour of Result-returning tests that assert on the error. #[should_panic] matches on panic message substrings which are brittle across refactors. Use it only when testing that a documented panic condition (e.g., an expect() from STYLE-0003) fires correctly.
  • #[ignore] — acceptable for tests that require external resources (network, API keys) or are unusually slow. Always add a reason: #[ignore = "requires CLAUDE_API_KEY"]. Run ignored tests explicitly with cargo test -- --ignored.

Motivation

The mod tests convention is idiomatic Rust and gives tests access to private items via use super::*. Dropping the test_ prefix avoids the triple-redundancy of tests::test_foo in cargo test output. Consistent naming makes cargo test parse_beta filtering predictable.

STYLE-0010: Test data and fixtures

Tags: testing

Situation

A test needs a git repository, temporary files, or other fixture data.

Guidance

Use tempfile::TempDir for isolated file system fixtures. For git-based tests, use a helper struct that wraps the temp directory:

struct TestRepo {
    _temp_dir: TempDir,
    repo_path: PathBuf,
    repo: Repository,
    commits: Vec<git2::Oid>,
}

impl TestRepo {
    fn new() -> Result<Self> { ... }
    fn add_commit(&mut self, message: &str, content: &str) -> Result<()> { ... }
}

Use the insta crate for snapshot (golden) tests where output stability matters.

Do not commit large binary fixtures. Prefer constructing test data programmatically.

Motivation

Temporary directories prevent tests from interfering with each other or with the real working directory. Snapshot testing with insta catches unintended output regressions without manually maintaining expected-output files.

STYLE-0011: Import ordering

Tags: code-style

Situation

Adding use statements to a file.

Guidance

Group imports into three blocks separated by a blank line, in this order:

  1. Standard library (std, core, alloc)
  2. External crates (everything from Cargo.toml dependencies)
  3. Crate-internal (crate::, super::, self::)

Within each group, let cargo fmt sort alphabetically.

use std::fs;
use std::path::PathBuf;

use anyhow::{Context, Result};
use serde::{Deserialize, Serialize};

use crate::data::context::ScopeDefinition;
use crate::git::CommitInfo;

Enforcement note: The rustfmt option group_imports = "StdExternalCrate" that codifies this convention is still unstable. The three-group ordering is therefore a manual discipline — cargo fmt will sort within a group but will not insert or enforce the blank-line separators between groups. Review for this during code review.

Motivation

Grouped imports make it easy to see at a glance what a module depends on externally versus internally. The three-group convention is widely used in the Rust ecosystem. Alphabetical ordering within groups is enforced by cargo fmt.

STYLE-0012: Clippy configuration

Tags: code-style

Situation

Configuring or overriding Clippy lints.

Guidance

Lint configuration is centralized in Cargo.toml under [lints.rust] and [lints.clippy]. The project enables clippy::all, clippy::pedantic, and clippy::nursery as warnings, with specific lints allowed where they are too noisy or conflict with project conventions. See Cargo.toml for the full allow-list with justification comments.

Project-specific thresholds (argument count, cognitive complexity, etc.) are configured in clippy.toml. Formatting rules are documented in rustfmt.toml.

The only lint attribute remaining in src/lib.rs is #![warn(missing_docs)], which is kept there because it should only apply to the library crate, not to tests or the binary.

When suppressing a lint on a specific item, use #[allow(clippy::...)] with a justification comment explaining why the suppression is necessary:

#[allow(clippy::too_many_arguments)] // Builder pattern requires all fields at construction
fn new(title: &str, description: &str, ...) -> Self { ... }

Do not add blanket #[allow(...)] at module or crate level to silence warnings. Fix the warning or add the allow to Cargo.toml with a justification comment. Per-item suppression is preferred for one-off cases; Cargo.toml allows are for project-wide decisions.

Motivation

Enabling pedantic and nursery catches subtle issues that clippy::all misses, such as inefficient string conversions, redundant closures, and inconsistent formatting. Centralizing configuration in Cargo.toml makes the lint policy visible and auditable without searching through source files. The allow-list documents deliberate exceptions rather than silently suppressing noise.

STYLE-0013: Unsafe policy

Tags: unsafe, code-style

Situation

Considering the use of unsafe code.

Guidance

This project forbids unsafe code via #![deny(unsafe_code)] in src/lib.rs. This lint is a hard error and applies to the entire crate.

If unsafe is ever required (e.g., FFI), it must be:

  1. Justified in an ADR
  2. Isolated in a dedicated module
  3. Annotated with a // SAFETY: comment per Clippy's undocumented_unsafe_blocks lint

Motivation

omni-dev has no need for unsafe — it delegates low-level operations to well-audited dependencies (git2, reqwest, tokio). The deny lint makes this a compile-time guarantee rather than a convention. Requiring an ADR for any future exception ensures the decision is reviewed and documented.

STYLE-0014: #[must_use] annotation

Tags: api-design

Situation

A public function or method returns a computed value without side effects.

Guidance

Apply #[must_use] to public functions whose return value is the entire point of the call. Discarding the result is almost certainly a bug:

#[must_use]
pub fn suggested_verbosity(&self) -> VerbosityLevel { ... }

#[must_use]
pub fn is_conventional(&self) -> bool { ... }

Do not apply #[must_use] to:

  • Functions that return Result — the #[must_use] on Result itself already covers this.
  • Builder methods that return &mut Self — the builder pattern implies chaining.
  • Functions with meaningful side effects (I/O, mutation) where the return value is supplementary.

Motivation

#[must_use] turns silent logic errors (ignoring a return value) into compiler warnings. Applying it deliberately to pure computations catches bugs at compile time without producing false positives on side-effectful functions. This aligns with clippy::must_use_candidate from the pedantic group.

STYLE-0015: String parameter ownership

Tags: api-design

Situation

Deciding whether a function parameter should be &str, String, or generic.

Guidance

Use the cheapest type that satisfies the function's needs:

The function… Accept Example
Only reads the string &str fn parse_subject(input: &str)
Stores the string in a struct/Vec String fn set_title(&mut self, title: String)
Needs flexibility (public API surface) impl Into<String> fn new(name: impl Into<String>) -> Self

Prefer &str for internal helpers and impl Into<String> sparingly — only at public API boundaries where caller ergonomics justify the generic. Avoid impl AsRef<str> unless you genuinely need to accept both String and &str without conversion.

For return types, prefer &str when returning a reference to owned data, and String when returning a newly constructed value. Avoid Cow<'_, str> unless profiling shows the borrow-or-own flexibility is needed.

// Good — borrows for read-only access
pub fn commit_type(&self) -> &str {
    &self.commit_type
}

// Good — takes ownership because it stores the value
pub fn with_title(mut self, title: String) -> Self {
    self.title = title;
    self
}

// Good — constructs a new string
pub fn format_summary(&self) -> String {
    format!("{}: {}", self.commit_type, self.subject)
}

Motivation

Accepting &str avoids unnecessary allocations on the caller side. Taking String when ownership is needed makes the transfer explicit and avoids hidden .to_string() calls inside the function. The impl Into<String> pattern is convenient for public APIs but adds monomorphisation cost, so it should be used judiciously.

STYLE-0016: Named constants

Tags: code-style, naming

Situation

Using a numeric or string literal whose meaning is not obvious from surrounding context.

Guidance

Extract magic literals into named constants or const items. A literal is "magic" when its purpose is not self-evident at the usage site:

// Bad — what does 8 mean?
let short = &hash[..8];

// Good — the name documents the intent
const SHORT_HASH_LEN: usize = 8;
let short = &hash[..SHORT_HASH_LEN];
// Bad — why 3?
if auth_attempts > 3 {
    bail!("Too many authentication attempts");
}

// Good
const MAX_AUTH_ATTEMPTS: u32 = 3;
if auth_attempts > MAX_AUTH_ATTEMPTS {
    bail!("Too many authentication attempts");
}

Literals that do not need extraction:

  • Structural zeros and ones — Vec::with_capacity(1), index + 1, slice[0].
  • Format strings — format!("{}: {}", key, value).
  • Known-safe constructor arguments — FixedOffset::east_opt(0) (covered by STYLE-0003).
  • Test assertions — assert_eq!(result.len(), 3) where the value is local to the test.

Place constants at the narrowest useful scope: module-level const if used across functions in the same module, crate-level if shared across modules, or function-local const if truly local.

Motivation

Named constants make the code self-documenting and provide a single point of change when a value needs updating. Searching for SHORT_HASH_LEN finds every usage; searching for 8 returns hundreds of false positives. The exceptions prevent over-extraction of trivially obvious values.

STYLE-0017: Function length

Tags: code-style

Situation

Writing or reviewing a function that is growing long.

Guidance

Keep functions under ~50 lines of logic (excluding doc comments, blank lines, and closing braces). When a function exceeds this guideline, look for opportunities to extract coherent sub-operations into well-named helper functions.

Common extraction targets:

  • Setup / teardown — opening resources, building configuration structs.
  • Distinct phases — validation, transformation, output formatting.
  • Repeated patterns — similar blocks that differ only in parameters.
  • Nested closures or callbacks — especially credential handlers, diff callbacks.
// Before — 120-line execute() mixing validation, AI calls, file I/O, and display
fn execute(&self) -> Result<()> {
    // ... 120 lines ...
}

// After — orchestrator delegates to focused helpers
fn execute(&self) -> Result<()> {
    let repo_view = self.generate_repository_view()?;
    let context = self.collect_context(&repo_view)?;
    let amendments = self.generate_amendments(&repo_view, &context)?;
    self.apply_and_display(amendments)?;
    Ok(())
}

This is a guideline, not a hard limit. A 60-line function that reads linearly may be clearer than three 20-line functions with non-obvious data flow. Use judgement — the goal is readability, not a line count.

Motivation

Long functions are harder to name, test, and review. Extracting sub-operations gives each piece a name that serves as documentation and makes the top-level flow scannable. The ~50-line heuristic is a common industry threshold (Clean Code, Effective Rust) that balances granularity against fragmentation.

STYLE-0018: Silent error suppression

Tags: error-handling

Situation

Handling a Result or Option where the error/None case is intentionally ignored.

Guidance

Never silently discard an error that could indicate a real problem. Three patterns to watch for:

  1. let _ = fallible_call(); — If the operation can meaningfully fail, at least log the error at debug! or warn! level. If the failure is truly inconsequential (best-effort cleanup), add a comment explaining why:

    // Bad — caller has no idea the abort failed
let _ = Command::new("git").args(["rebase", "--abort"]).output();

// Good — intent is documented, failure is logged
// Best-effort cleanup; the rebase may already have been aborted.
if let Err(e) = Command::new("git").args(["rebase", "--abort"]).output() {
    tracing::debug!("Rebase abort during cleanup failed: {e}");
}

  2. if let Ok(x) = ... { use(x) } with no else — returning a silent default on parse or I/O failure hides broken configuration files from the user:

    // Bad — silently returns empty vec on malformed YAML
if let Ok(content) = fs::read_to_string(&path) {
    if let Ok(config) = serde_yaml::from_str(&content) {
        return config.scopes;
    }
}
Vec::new()

// Good — warns so the user knows their file was ignored
match fs::read_to_string(&path) {
    Ok(content) => match serde_yaml::from_str(&content) {
        Ok(config) => return config.scopes,
        Err(e) => tracing::warn!("Ignoring {}: {e}", path.display()),
    },
    Err(e) if e.kind() != io::ErrorKind::NotFound => {
        tracing::warn!("Cannot read {}: {e}", path.display());
    }
    _ => {} // File not found is expected in the fallback chain
}

  3. .unwrap_or_default() on non-trivial results — acceptable for genuinely optional data, but not as a blanket substitute for error handling on operations that should succeed.

Acceptable silent discards:

  • Closing a file or flushing a logger during shutdown.
  • Sending on a channel where the receiver may have been dropped.
  • Test cleanup in Drop implementations.

Motivation

Silent error suppression is one of the hardest bugs to diagnose because nothing visibly fails — the program simply produces wrong results or missing data. Logging at debug! or warn! level costs nothing on the success path and provides a trail when something goes wrong. The explicit comment requirement for let _ = forces the author to justify the suppression at write time, which often reveals that the error should not be ignored after all.

STYLE-0019: Type-safe variant selection

Tags: api-design, code-style

Situation

Routing behaviour based on a value that comes from a fixed, known set of alternatives (e.g., AI provider, output format, environment name).

Guidance

Model the set of alternatives as an enum and match on it. Do not use string comparisons to branch on known variants:

// Bad — brittle, easy to typo, no exhaustiveness checking
let provider_name = if provider.to_lowercase().contains("openai")
    || provider.to_lowercase().contains("ollama")
{
    "openai"
} else {
    "claude"
};

// Good — the compiler enforces every variant is handled
enum AiProvider {
    Claude,
    Bedrock,
    OpenAi,
    Ollama,
}

fn resolve_provider(raw: &str) -> Result<AiProvider> {
    match raw.to_lowercase().as_str() {
        s if s.contains("openai") => Ok(AiProvider::OpenAi),
        s if s.contains("ollama") => Ok(AiProvider::Ollama),
        s if s.contains("bedrock") => Ok(AiProvider::Bedrock),
        _ => Ok(AiProvider::Claude),
    }
}

Parse once, branch on the enum everywhere else. The string-to-enum conversion should happen at the boundary (CLI parsing, config loading, environment variable reading). All downstream code receives the enum and uses match, which the compiler checks for exhaustiveness.

This applies to any situation where the set of values is known at compile time — not just providers. Output formats, log levels, feature flags, and similar categories all benefit from the same pattern.

Motivation

String-based dispatching defeats Rust's exhaustiveness checking. When a new variant is added, the compiler cannot tell you which if chains need updating — you discover missed branches at runtime. An enum makes invalid states unrepresentable and turns forgotten branches into compile errors. The "parse at the boundary" pattern also eliminates repeated .to_lowercase().contains() calls scattered across the codebase.

STYLE-0020: Single-purpose commits

Tags: commits

Situation

Preparing a set of changes that involves refactoring, new functionality, or bug fixes.

Guidance

Each commit should do one kind of work. Keep refactoring commits separate from implementation commits, and both separate from bug-fix commits.

If a refactoring would make a subsequent implementation or fix cleaner, land the refactoring as an earlier commit so that:

  1. The refactoring can be reviewed on its own terms (no behaviour change expected).
  2. The implementation commit starts from a cleaner baseline and is easier to understand.
  3. Either commit can be reverted independently if needed.
# Good — reviewable, bisectable, revertible
git log --oneline
a1b2c3  refactor(cli): extract shared repository-view builder
d4e5f6  feat(cli): add --json output to check command

# Bad — mixed intent, hard to review or revert half of it
git log --oneline
f7g8h9  feat(cli): add --json output and refactor repo-view builder

Acceptable exceptions:

  • Trivial renames or import cleanups that are a natural by-product of the implementation (a few lines, not a standalone refactoring effort).
  • Prototype or spike branches where commit hygiene is deferred to a squash before merge.

Motivation

Single-purpose commits make git bisect reliable, code review focused, and reverts surgical. When refactoring is interleaved with behaviour changes, reviewers cannot tell whether a difference is a deliberate new behaviour or a mechanical restructuring — so they must verify every line as if it were new logic. Separating the two cuts review effort roughly in half.

STYLE-0021: Module cohesion

Tags: module-organization

Situation

A source file is accumulating types, functions, or impl blocks that serve unrelated purposes.

Guidance

Each module should have a single, nameable responsibility. When you find it hard to describe what a module does without using "and," it likely contains unrelated code that would be clearer in separate submodules.

Signals that a module should be split:

  • It contains multiple independent command or handler types that share little or no private state (e.g., ViewCommand, CheckCommand, and CreatePrCommand in one file).
  • Unrelated sections require scanning past hundreds of lines to find the piece you need.
  • Changes to one logical area routinely cause merge conflicts with work in another area of the same file.
  • You struggle to name the file — broad names like commands.rs or helpers.rs suggest mixed responsibilities.

What is not a reason to split:

  • Line count alone. A 400-line module with a single cohesive type and its helpers is fine.
  • A few shared utility functions that genuinely serve every type in the module.

When splitting, apply the layout from STYLE-0004 and extract each distinct responsibility into its own submodule:

# Before — one file with five unrelated command types
src/cli/git.rs          # 3 700 lines, five commands + helpers

# After — each command owns its module, shared code is explicit
src/cli/
├── git.rs              # re-exports, shared types
└── git/
    ├── view.rs         # ViewCommand
    ├── twiddle.rs      # TwiddleCommand
    ├── check.rs        # CheckCommand
    ├── create_pr.rs    # CreatePrCommand
    └── helpers.rs      # shared repo-view builder, guidance display

Motivation

A module that mixes unrelated responsibilities is hard to navigate, produces noisy diffs, and invites merge conflicts between independent work streams. Splitting by responsibility makes each file's purpose obvious from its name, keeps diffs focused on the change at hand, and lets reviewers evaluate one concern at a time. The emphasis on cohesion rather than a rigid line limit avoids unnecessary churn on files that are large but focused, while still flagging files that are large because they mix concerns.

STYLE-0022: ADR format

Tags: adrs

Situation

Writing a new Architecture Decision Record or reviewing an existing one.

Guidance

Every ADR must use exactly the structure prescribed by ADR-0000: Title, Status, Context, Decision, Consequences. Do not add extra top-level sections (e.g., "Recommendations", "Alternatives", "References"). Content that might seem like a separate section should be incorporated into the appropriate prescribed section — alternatives belong in Context, recommendations belong in Decision or Consequences.

The Decision section must be stated in active voice ("We will ..."). The Consequences section should cover positive, negative, and neutral outcomes.

Motivation

A consistent structure makes ADRs scannable and sets clear expectations for both authors and reviewers. Extra sections blur the boundary between architectural decisions and operational guidance (which belongs in the style guide) or implementation detail (which belongs in code comments or docs).

STYLE-0023: Validate commit messages with omni-dev after creation

Tags: commits

Situation

After creating one or more commits and before pushing or opening a pull request.

Guidance

After every git commit, invoke the commit-twiddle skill to validate and fix the message against the guidelines in .omni-dev/commit-guidelines.md. The skill calls omni-dev git commit message view to analyse the commit, then omni-dev git commit message amend to rewrite the message if needed.

Constraints to observe:

  • The target commit must be at the branch tip with no merge commit above it. If a merge commit is present the amend step will fail — work on a branch before merging.
  • The amendments file requires the exact 40-character SHA from the commit output. An abbreviated hash silently skips the amendment or errors.
  • Do not include a Co-Authored-By footer unless a human co-author contributed. AI tool attribution footers must not be added to commit messages.

Motivation

Running the twiddle step after commit creation catches scope, casing, and footer violations before they reach the remote, avoiding the costly reset-and-redo cycle required to rewrite history once a commit has been merged to main.

STYLE-0024: Wiremock tests for Atlassian client methods

Tags: testing, api-design

Situation

Adding a new public method to AtlassianClient in src/atlassian/client.rs.

Guidance

Every new public method on AtlassianClient must have corresponding #[tokio::test] tests using wiremock::MockServer. At minimum, cover three cases:

  1. Success — mock the expected HTTP method and path, return a valid response, and assert on the parsed result fields.
  2. Empty / edge case — return a valid but minimal response (e.g., empty list, zero count) and assert the method handles it gracefully.
  3. API error — return a non-success status code (e.g., 404, 403) and assert the error is propagated with the status code in the message.

Follow the existing test pattern in client.rs:

#[tokio::test]
async fn get_watchers_success() {
    let server = wiremock::MockServer::start().await;

    wiremock::Mock::given(wiremock::matchers::method("GET"))
        .and(wiremock::matchers::path("/rest/api/3/issue/PROJ-1/watchers"))
        .respond_with(
            wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({
                "watchCount": 1,
                "watchers": [{"accountId": "abc123", "displayName": "Alice"}]
            })),
        )
        .expect(1)
        .mount(&server)
        .await;

    let client = AtlassianClient::new(&server.uri(), "user@test.com", "token").unwrap();
    let result = client.get_watchers("PROJ-1").await.unwrap();
    assert_eq!(result.watchers.len(), 1);
}

Motivation

Client methods are the project's primary integration boundary with the Atlassian REST API. Wiremock tests verify request construction (method, path, query params, body) and response parsing without hitting a live API. Skipping these tests leaves the entire HTTP layer uncovered, which CI coverage checks will flag as a patch coverage gap.

STYLE-0025: Testable CLI execute methods

Tags: testing, module-organization

Situation

Adding or modifying a CLI command in src/cli/atlassian/.

Guidance

create_client() reads credentials from the environment, making any code after it unreachable in unit tests. When an execute method contains non-trivial logic beyond create_client(), extract that logic into a standalone run_* function that accepts an &AtlassianClient (or the relevant API wrapper) so it can be tested with wiremock.

Extract when the execute body contains any of:

  • Multi-step orchestration (e.g., fetch → resolve → mutate → confirm).
  • Branching or validation on user input (e.g., resolving a transition by name/ID, parsing and validating issue keys, confirmation prompts).
  • Logic that combines results from multiple API calls.
impl TransitionCommand {
    pub async fn execute(self) -> Result<()> {
        let (client, _instance_url) = create_client()?;
        run_transition(&client, &self.key, self.transition.as_deref(), self.list, &self.output).await
    }
}

async fn run_transition(
    client: &AtlassianClient, key: &str, transition: Option<&str>, list: bool, output: &OutputFormat,
) -> Result<()> {
    let transitions = client.get_transitions(key).await?;
    // ... resolve, execute, print ...
}

Write tests for run_* functions covering the success path, structured output formats (JSON/YAML), and API error propagation.

Do not extract when execute is a trivial pipeline — a single API call fed directly into output_as / print with no branching or validation:

impl ListCommand {
    pub async fn execute(self) -> Result<()> {
        let (client, _instance_url) = create_client()?;
        let result = client.get_projects(self.limit).await?;
        if output_as(&result, &self.output)? {
            return Ok(());
        }
        print_projects(&result);
        Ok(())
    }
}

Here the client method itself should have wiremock tests (per STYLE-0024), and extraction would add indirection without catching additional bugs. Inline is fine.

Motivation

The goal is testability of logic that can break, not mechanical conformance. Extracting a trivial pipeline adds a function boundary and a signature to maintain without delivering new test coverage beyond what STYLE-0024 client tests already provide. Reserving extraction for commands with real orchestration or validation keeps the codebase lean while ensuring the code most likely to harbour bugs is covered.

STYLE-0026: MCP tool and resource authoring conventions

Tags: api-design, module-organization, testing

Situation

Adding or modifying MCP tools, resources, or supporting types under src/mcp/.

Guidance

  1. Parameter structs. Every tool defines its input as a dedicated #[derive(Debug, Deserialize, schemars::JsonSchema)] struct with a name ending in Params (e.g. GitViewCommitsParams). All fields get a doc comment — it flows through to the tool's JSON schema and is what the assistant sees. Optional fields use #[serde(default)] and Option<T>; never Default::default() in the handler body.

  2. One tool router per module. Group related tools in their own submodule and expose the router via #[tool_router(router = name_tool_router, vis = "pub")] (see src/mcp/git_tools.rs). OmniDevServer::new combines all routers — add a new module there rather than cramming tools into an existing router.

  3. Error mapping. Inside tool handlers, bubble anyhow::Error out via the shared tool_error helper so the full error chain reaches the client. Do not build McpError values by hand with bespoke messages — go through tool_error so the format stays consistent across tools. Resource handlers use resources::not_found(uri, err) for URI-lookup failures so the raw URI appears in the response data.

  4. Blocking work belongs in spawn_blocking. Tools that call into synchronous business logic (e.g. git2 operations) must wrap the call in tokio::task::spawn_blocking — the MCP transport loop is async, and blocking it stalls every in-flight request.

  5. Output format. YAML for repository/commit analysis (matches the CLI), markdown for rendered prose (JIRA/Confluence JFM), JSON for raw structured payloads (ADF). Advertise the MIME type on resources so clients can route output appropriately.

  6. Resource URIs. New URI templates must round-trip through ResourceUri::parse with a dedicated unit test per template and per malformed-input class (unknown scheme, wrong path shape, empty identifier). Keep the catalogue in resource_templates() and resource_listing() in sync — add a templates_include_all_*_uris assertion when the count changes.

  7. Testing. Tools and resources both need at least:

    • A library-level unit test covering the success path with a fabricated input (temp repo, mock API, or hand-built ContentItem).
    • An integration test under tests/mcp_integration_test.rs that spins up OmniDevServer on an in-memory duplex and exercises the MCP protocol round-trip (list + read/call).

Motivation

The MCP surface is consumed by non-human clients that can only see what the schema and error messages tell them. Uniform parameter structs make the schema predictable; shared error mapping keeps diagnostics legible across tools; router splitting keeps modules small and testable; the paired unit+integration test requirement means a regression in protocol wiring is caught without requiring a live Claude Desktop to reproduce.

STYLE-0027: Plan-file status header and ADR cross-links

Tags: documentation, adrs

Situation

Adding or substantially editing a file in docs/plan/.

Guidance

  1. Status header. Immediately after the # Title heading, add a **Status:** … line using one of these four canonical tags:

    Tag Meaning
    Built The design has shipped. The doc may still be useful as a reference but is not a roadmap.
    In Progress Some phases shipped, others ongoing. Specify which phase is current.
    Aspirational Describes intent that is not yet started or has been superseded by a different approach.
    Historical Written for context that no longer matches current architecture; kept for institutional memory.

    A short qualifier after an em-dash (— canonical reference, — Phase 3 not started) is encouraged when it adds signal.

  2. ADR cross-links. When one or more ADRs describe the same decisions, add an **ADRs:** line immediately after the Status line, listing each ADR as a relative link separated by · (middle dot, surrounded by spaces). Example:

    **ADRs:** [ADR-0002](../adrs/adr-0002.md) · [ADR-0014](../adrs/adr-0014.md)

  3. When to retire. Once a plan's decisions are captured in one or more ADRs, change its status to Built (with ADR cross-links) or Historical rather than deleting it — preserving the doc keeps prior reasoning discoverable.

  4. When to promote. When a plan's high-level decisions stabilise, copy the decision and its rationale into a new ADR (see STYLE-0022) and update the plan's status to Built with a cross-link.

Motivation

A plan directory that mixes shipped, in-progress, and superseded content with no signalling forces every new contributor to read every file and cross-check the codebase before they can act on it. A one-line status header costs the author nothing and gives the reader an immediate orientation; ADR cross-links make the canonical decision discoverable.

STYLE-0028: Inject the environment, don't mutate it in tests

Tags: testing, module-organization

Situation

Writing code that reads an environment variable, or writing a test for code whose behaviour depends on the environment (HOME, XDG_CONFIG_HOME, USE_OPENAI, ATLASSIAN_*, OMNI_DEV_*, provider/API-key vars, …).

Guidance

Read the environment only at a thin boundary wrapper; put the logic in an inner seam that takes the resolved input as a value. Then tests exercise the inner seam with a constructed value and never mutate the process-global environment. Pick the seam by what is read:

  1. Resolved domain value — incidental config. Provide a *_from(value) constructor alongside the env-resolving entry point. (e.g. create_client_from, DatadogClient::from_credentials.)

    pub fn create_client() -> Result<(Client, String)> {
    create_client_from(load_credentials()?)        // prod: resolve env → value
}
pub fn create_client_from(creds: Credentials) -> Result<(Client, String)> { /* … */ }

  2. std::env::var parsing boundary — "given these vars, what do we do?". Take &impl EnvSource (see crate::utils::env); the prod wrapper passes &SystemEnv, tests pass a MapEnv.

    pub fn check_ai_credentials(model: Option<&str>) -> Result<Info> {
    check_ai_credentials_with(&SystemEnv, model)    // thin wrapper
}
fn check_ai_credentials_with(env: &impl EnvSource, model: Option<&str>) -> Result<Info> { /* … */ }

  3. dirs::home_dir() / dirs::config_dir() — these read HOME / XDG_CONFIG_HOME inside the dirs crate, where EnvSource can't reach. Thread the resolved base directory as a parameter (prod default = dirs::home_dir()). For a subprocess that needs HOME, set it scoped on the Command (.env("HOME", …)), never on the process.

Never call std::env::set_var / remove_var in a test, and never add a per-module env mutex to "protect" such mutation.

Motivation

The process environment is a single shared mutable global. Per-module mutexes over it provide no mutual exclusion across modules (issue #821/#950/#1030), and set_var/remove_var are unsafe in Rust 2024. Injecting the resolved value removes the shared hazard entirely: env-dependent tests become pure, order-independent, lock-free, and fully parallel. A fresh bespoke env lock added after #821 (datadog/*) is exactly the regression this rule prevents.

STYLE-0029: MCP tool & parameter description checklist

Tags: documentation, api-design

Situation

Writing or revising any #[tool(description = "…")] text or any doc comment on a *Params struct field under src/mcp/. These strings are the only thing an AI agent reads to decide how to call a tool: field doc comments flow through schemars::JsonSchema into each field's JSON-schema description, and description = "…" sets the tool-level text. STYLE-0026 covers the wiring (struct shape, router, tests); this rule covers the prose quality the agent actually reads.

Guidance

Every tool description and parameter doc comment must satisfy this checklist. The reference exemplars are LinkCreateParams (inward/outward with the concrete Blocks example) and the git_* tool descriptions.

Tool-level description:

  1. One-line "what it does" as the first sentence — a single, skimmable summary an agent reads before the params.
  2. CLI cross-reference, both directions. The tool description names its equivalent subcommand: Mirrors `omni-dev <subcommand>`. The clap subcommand's doc comment carries the reverse, ending with (mirrors the `<tool_name>` MCP tool) (see src/cli/atlassian/jira/link.rs). The two must stay in lock-step. A tool with no CLI equivalent (e.g. atlassian_convert's system_prompt override) says so explicitly.
  3. "When to use vs <sibling>" wherever two tools overlap or could be confused (e.g. updating a body via jira_write vs setting hierarchy via jira_link_parent; jira_link_create vs jira_link_parent). One sentence pointing at the sibling and when to prefer it.
  4. A concrete example in the tool text, not only on fields — a real key (PROJ-123), a real enum value, or a one-line call shape. Surface the single most error-prone value at the tool level the agent skims first.
  5. Mutating/destructive affordances. State any dry_run, confirm, or preflight behaviour and its default (e.g. "Set dry_run = true to return the would-be request without sending it"; "Requires confirm: true").

Per-parameter doc comment:

  1. A concrete example value — e.g. `PROJ-123` , e.g. `2025-01-31` , e.g. `Blocks` .
  2. Allowed values for enums / closed sets spelled out (one of `future`, `active`, `closed` ) — don't make the agent guess.
  3. Expected wire format — JFM markdown vs ADF JSON, YYYY-MM-DD, JIRA duration (2h 30m), Atlassian accountId vs display name, JQL/CQL.
  4. Directional / order-dependent semantics spelled out — never "source" / "target" alone. Name which end is which, with an example: "Source (inward) issue — for Blocks, the issue doing the blocking." This is the #1054 regression this rule exists to prevent.
  5. Required vs optional. Optional fields use #[serde(default)] + Option<T> (per STYLE-0026) and the doc comment states the default behaviour when the field is omitted.

Keep docs/mcp.md's tool catalog in sync when a tool's purpose or CLI mapping changes, and run the update-snapshots skill whenever the reverse-reference edits change CLI --help text.

The present and non-empty floor of this checklist (items 1 and 6) is enforced mechanically by all_tools_advertise_descriptions_and_param_schemas in tests/mcp_integration_test.rs: it fails if any advertised tool or top-level parameter ships without a description, so a new tool cannot silently regress the surface. The prose-quality items (2–5, 7–10) still need review — the test guarantees the strings exist, not that they are good.

Motivation

An agent's success rate is bounded by how unambiguous these strings are. A vague description produces wrong-but-silent calls — an inverted Blocks dependency (#1054), a display name where an accountId was required, a body update where a parent link was meant — each costing a recovery round-trip or quietly corrupting data. #1049 fixed one tool (jira_link_create) to this bar; this checklist generalises it so the whole src/mcp/ surface meets the same standard rather than drifting tool-by-tool.