Every rule,
with a specimen
from the field.

All three fire on substring match against new content — no file gates. They apply equally to code edits, commit messages, and PR bodies. The shared shape: the model has noticed a problem and is reaching for a phrase that retires it without a decision.

// Subject line, ~turn 80 of a long session
fix(scoring): switch priority ordering to descending

The test failure in play::turn_resolver_tests is
a pre​‑existing issue not from our changes; ignored.

// Subject line
fix(scoring): switch priority ordering to descending

Known broken: play::turn_resolver_tests::priority_order
fails on the new sort comparator. Filed as #421;
this PR does not address it. (See follow-up.)

Why The disclaimer pattern is a known failure mode in long sessions: an issue gets surfaced, attribution-shifted, and then forgotten. The accepted form forces a binary — fix it as part of this change, or name it and defer it explicitly.

These phrases are the linguistic marker of the unfinished handoff: a change made, a hypothesis stated, no test run. The rule cannot tell whether you actually ran the test — but it can force the language to match the evidence.

The duplicate-entry bug should​ work now. Closes #87.

Verified against the seed that triggered #87:
    $ cargo test hand::duplicate_entry -- --exact
    test ... ok (1 passed)
Closes #87.

Hook fires before the commit runs. The host prints the rule's message to stderr at exit 1; the commit still proceeds. The point is the moment of friction — a half-second to ask did I actually verify this before the message is permanent.

$ git​ commit -m "wire up new play ledger"
phronesis: WARNING — About to commit. Trace
the call chain end-to-end before reporting done.
Half-fixes where one layer is wired but another is
not are a recurring failure mode.

$ cargo test --workspace -p core
test ... ok (124 passed)
$ cargo run -- --scenario play-smoke
... play smoke passed
$ git​ commit -m "wire up new play ledger"

All four share the same shape: substring match against new content, gated by a file-path match on src. The hook strips test blocks before evaluation, so usage inside inline test modules — where panics are idiomatic — does not trip the rule.

pub fn current_card(&self) -> &Card {
    self.members
        .get(&self.turn_order[self.turn_index])
        .unwrap()
}

pub fn current_card(&self) -> Result<&Card, HandError> {
    let id = self.turn_order
        .get(self.turn_index)
        .ok_or(HandError::TurnIndexOutOfRange)?;
    self.members
        .get(id)
        .ok_or_else(|| HandError::CardNotFound(id.clone()))
}

Why Panic-shaped exits in production paths crash the host runtime. The cost of carrying a Result through one more call is much lower than the cost of a mid-session crash in a tool that holds long-lived state.

Detected via tree-sitter: the rule walks function signatures and matches the precise return shape. Test functions are excluded automatically. The remedy is a proper error enum, usually with thiserror.

pub fn enqueue(&mut self, event: Event) -> Result<(), String> {
    if self.queue.len() >= self.cap {
        return Err(format!("queue full at cap {}", self.cap));
    }
    self.queue.push(event);
    Ok(())
}

// New: a domain error type
#[derive(Debug, thiserror::Error)]
pub enum PipelineError {
    #[error("event queue full at cap {0}")]
    QueueFull(usize),
}

pub fn enqueue(&mut self, event: Event) -> Result<(), PipelineError> {
    if self.queue.len() >= self.cap {
        return Err(PipelineError::QueueFull(self.cap));
    }
    self.queue.push(event);
    Ok(())
}

Why A typed error makes the pipeline's failure modes explicit to callers, lets ? compose with From impls, and survives refactors. A stringified error is a one-way trapdoor — every caller has to parse the message to recover any structure.

pub fn find_card(&self, name: &String) -> Option<&Card> {
    self.members.iter().find(|c| &c.name == name)
}

pub fn find_card(&self, name: &str) -> Option<&Card> {
    self.members.iter().find(|c| c.name == name)
}

Why A &str accepts string literals, slices into existing buffers, and the result of .as_str() on a String. A &String forces every caller to first own a String — a needless allocation requirement at an API boundary.

pub fn new(
    id: String,
    name: String,
    score: u32,
    max_score: u32,
    rank: u32,
    priority: i32,
    actions: Vec<Action>,
    members: Hand,
) -> Self { ... }

pub struct HandStats { pub score: u32, pub max_score: u32, pub rank: u32, pub priority: i32 }

pub fn new(
    id: ActorId,
    name: String,
    values: HandStats,
    strategy: Strategy,   // holds actions + members
) -> Self { ... }

Why Long parameter lists correlate with god-functions and make call sites stateal and brittle. Grouping related params into named structs improves readability and gives a place for invariants to live.

$ cargo test
phronesis: WARNING — Running `cargo test` without
`--workspace` only checks part of the workspace. Use
`cargo <subcommand> --workspace --tests --examples`
to catch sibling-crate breakage, or pass `-p <crate>`
if scope was intentional.

# Whole workspace, including sibling crates
$ cargo test --workspace --tests --examples

# Or scoped intentionally to one crate
$ cargo test -p core hand::

Why A bare cargo build in a multi-crate workspace silently skips sibling crates whose paths Cargo can't infer from the cwd. The result is the classic "green locally, red on CI" failure when a small refactor breaks a downstream crate.

Project-specific in shape, broadly useful in concept: these two methods on phronesis's own RETE network were converted from async to sync in an earlier refactor, and old .await call sites kept turning up in test code and integration helpers afterwards. The rule pins the API surface so that resurrected snippets get caught before they reach a future is not awaitable compile error. The same shape — naming two specific now-sync methods you wish to police — generalizes cleanly to any project undergoing a sync/async migration.

#[tokio::test]
async fn round_smoke() -> Result<(), TestError> {
    let engine = make_engine();
    let actions = engine.execute_all_agenda_items().await?;
    assert_eq!(actions.len(), 3);
    Ok(())
}

#[test]
fn round_smoke() -> Result<(), TestError> {
    let engine = make_engine();
    let actions = engine.execute_all_agenda_items()?;
    assert_eq!(actions.len(), 3);
    Ok(())
}

Why Without the rule, the compiler error is confusing — "Result<…> is not a future" — and easy to misdiagnose as a Tokio version mismatch. The named rule turns a five-minute hunt into a one-line fix.

Audit-only by design. Firing this at hook time would interrupt every refactor mid-flight; firing it at sweep time gives a debt count of "places where the ? operator wasn't used."

let event = match parse_event(&payload) {
    Ok(e) => e,
    Err(e) => return Err(e),
};

let event = parse_event(&payload)?;

pub struct State {
    pub id: String,
    pub name: String,
    pub exits: Vec<String>,    // also ids, also strings
    pub slot_ids: Vec<String>,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct StateId(String);
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct SlotId(String);

impl StateId {
    pub fn new(s: impl Into<String>) -> Self { Self(s.into()) }
    pub fn as_str(&self) -> &str { &self.0 }
}

pub struct State {
    pub id: StateId,
    pub name: String,
    pub exits: Vec<StateId>,
    pub slot_ids: Vec<SlotId>,
}

Why A bare String id lets you pass a SlotId where a StateId is expected. The compiler will not save you. A newtype turns the mistake into a type error at the call site.

$ phr-mcp audit --rule audit-file-loc-high

Rule                 Level  Hits  Files
audit-file-loc-high  warn      3      3
  src/core/event_pipeline.rs   lines: 1
  src/core/play/turn_resolver.rs  lines: 1
  src/example/round/mod.rs             lines: 1

// Split event_pipeline.rs (1418 LOC) into:
src/core/event_pipeline/
├── mod.rs                  // 80 LOC: the trait, re-exports
├── queue.rs                // 220 LOC: the ring buffer
├── dispatch.rs             // 410 LOC: the dispatch table
├── consequences.rs         // 340 LOC: side-effect drainage
└── shape.rs                // 180 LOC: payload types
                            // 1230 LOC total, 188 LOC saved as overlap

Why A file that crosses 800 lines is usually doing several jobs. The rule is a sweep indicator, not a mechanical fix — the value is in being told the pile exists during a quarterly review, not on every edit.

#![deny(warnings)]

// every cargo update is a coin flip
pub fn hello() { println!("hello"); }

// src/lib.rs — no global deny
pub fn hello() { println!("hello"); }

# .github/workflows/ci.yml
- run: cargo clippy --workspace -- -D warnings
  env:
    RUSTFLAGS: "-D warnings"

Why Library code with #![deny(warnings)] stops compiling for downstream consumers as soon as their toolchain introduces a new warning. The policy belongs in CI, where it gates merges without propagating fragility to anyone using the crate. Source: rust-unofficial/patterns §Anti-patterns (deny-warnings).

pub fn describe(card: &Box<Card>) -> String {
    format!("{}: VALUE {}", card.name, card.score)
}

pub fn describe(card: &Card) -> String {
    format!("{}: VALUE {}", card.name, card.score)
}

Why A &Box<T> can always be obtained from a &T via auto-deref, but the reverse forces every caller to pre-wrap their value in a Box regardless of where it actually lives. Source: rust-unofficial/patterns §Idioms (borrowed-types-for-arguments).

let config = load_config().expect("");
let port = config.port.expect("");

let config = load_config()
    .expect("config must exist at $CARGO_MANIFEST_DIR/config.toml");
let port = config.port
    .expect("validate_config() guarantees port is Some");

Why When the panic eventually fires in production, the message is what tells the operator which invariant was broken. An empty message turns a debuggable assertion into an opaque crash; the existing warn-unwrap-in-src rule already flags .unwrap(), but .expect("") escapes it by being technically a different call.

$ phr-mcp audit --rule audit-rc-refcell-in-src

Rule                      Level  Hits  Files
audit-rc-refcell-in-src   warn      4      2
  src/world/region.rs        lines: 18, 42
  src/world/graph.rs         lines: 7, 91

// arena + indices replaces shared mutability
pub struct RegionArena {
    regions: Vec<Region>,
}
pub struct RegionId(u32);

// or: precompute the graph, treat it as immutable
// or: move mutability into a single owner, pass &mut

Why Rc<RefCell<T>> is sometimes the right answer — for graph-shaped data with true aliasing, for callback registries, for single-threaded observer patterns. But it is also the canonical shape that appears when someone has lost a fight with the borrow checker and reached for the escape hatch. Surface it during one-time audits so design intent gets revisited.

let msg = "hand " + &name + " drew " + &card.to_string();

let msg = format!("hand {} drew {}", name, card);

Why Plus-chained string concatenation produces a sequence of intermediate String allocations and forces awkward .to_string() calls on non-string arguments. format! reads as a template, allocates once, and accepts anything that implements Display. Source: rust-unofficial/patterns §Idioms (concat-format).

pub fn configure_database(url: &str) {
    unsafe { std::env::set_var("DATABASE_URL", url); }
}

pub struct DbConfig { pub url: String }

pub fn connect(cfg: &DbConfig) -> Result<Conn, DbError> {
    Conn::open(&cfg.url)
}

Why The standard library was tightened in edition 2024 for the same reason this rule exists: env-var mutation is process-global and unsynchronized, and any read from another thread during the write is undefined behavior. Tests where you control thread count are usually fine (and already opt in with unsafe); library code almost never is. Prefer passing configuration explicitly through arguments or a context struct.

#[allow(dead_code)]
pub fn compute_secondary_score(&self) { /* never called */ }

// option A: delete

// option B: document the reason
/// Held in reserve for the upcoming scoring-revision RFC (see #142).
/// Not yet wired into the dispatch table; cargo would otherwise warn.
#[allow(dead_code)]
pub fn compute_secondary_score(&self) { /* ... */ }

Why Many #[allow(dead_code)] attributes get added during refactors and then forgotten. Audit sweeps surface them so the team can decide, code by code, whether each one is still earning its keep.

let result: i64 = engine.eval("let base = 10; base * 2 + 5")?;

// scripts/calc_priority.rhai is loaded once,
// compiled to an AST, then eval'd many times.
let ast = engine.compile_file("scripts/calc_priority.rhai".into())?;
let result: i64 = engine.eval_ast(&ast)?;

Why The compiled-AST path lets the script be values-checked at build time, exercised in isolation, cached across calls, and discovered via the file system rather than buried as a literal in unrelated code. Inline string-eval gives up all of those affordances for a character-count win.

fn execute(actor, slot) {
    print(`Saving state for ${actor} to slot ${slot}`);
    save_state(actor, slot);
}

// Whatever your Engine exposes via register_fn:
// log / emit / notify / response_append / etc.
fn execute(actor, slot) {
    log(`Saving state for ${actor} to slot ${slot}`);
    save_state(actor, slot);
}

Why Output from a shipped script should flow through the same path as the rest of the application's output — to be captured, replayed, formatted, or addressed to a specific surface (user chat, server log, audit trail). print escapes to stdout, which is usually nowhere useful.