Code Police

---
name: code-police
description: Review code for quality, simplicity, and common mistakes before declaring work complete.
context: fork
model: sonnet
argument-hint: "[--no-elegance]"
---

# Code Police

Review the current changes (scoped to the current branch/PR) against the rules below **plus any additional rules from the project**. The three passes — rule checklist, fact-check, elegance — run as parallel sub-agents on fresh contexts; the implementer's main context just wrote the diff and is biased to rationalize it, so reviewing inline laundered violations through. Sub-agents start cold, which is the point. After they return, the orchestrator stitches their findings into a single summary.

## Arguments

`--no-elegance` — skip Pass 3 (elegance) entirely. Pass 1 (rules) and Pass 2 (fact-check) still run. Use this when the elegance pass would be redundant because `/simplify` already ran over this same tree — e.g. a caller that invokes `/simplify` standalone and then `/code-police`. Without it, Pass 3 re-invokes `/simplify` on an already-simplified tree, paying a full skill invocation (agent spawn, diff re-read, model tokens) to re-derive a near-guaranteed no-op. When the flag is set, report Pass 3 as `Elegance | – | Skipped (--no-elegance)` in the summary.

## Project rules

Before spawning the pass sub-agents, read `.agency/code-police.md` if it exists. Treat any rules declared there — whether inline or as a pointer to another file (`See ./code-police-rules.md`) — as additions to the built-in rules below. They appear as separate rows in the Pass 1 checklist with the project's chosen rule IDs.

If `.agency/code-police.md` is missing, proceed with only the built-in rules. The file is project-defined and free-form (Markdown, no required frontmatter).

## Reviewing principles

These apply to **every** pass — Pass 1, Pass 2, Pass 3 alike. Each sub-agent's prompt restates them, but the burden also sits on the orchestrator: if a sub-agent comes back with prose that violates these, push it back rather than launder its dismissal through the summary.

- **NEVER talk yourself out of a finding.** If you identified a problem, it IS a problem. No "However…" or "acceptable tradeoff."
- **NEVER use "theoretically X but practically Y"** to dismiss fragility.
- **NEVER issue "no action needed"** on a finding you just described.
- **NEVER end with reassurance** ("the logic is sound", "no other issues") unless you genuinely found zero issues.
- **Assume the code is wrong until proven right.** You are a prosecutor, not a defense attorney.

## Rules

### dry-rule-of-three

Two similar instances are fine — don't abstract prematurely. Three is the threshold for extraction. But identical content that must stay in sync (same HTML, same version string) should be deduplicated immediately regardless of count. Versions, ports, paths — define once, reference everywhere.

### prefer-focused-library

Before hand-rolling a utility (string tokenizer, quoted-string parser, date helper, semver comparator, URL builder, CLI arg parser, tree walker, regex-based matcher, path normalizer, etc.), check whether a focused library solves the same problem. If one exists with a matching scope and a reasonable bundle cost, **prefer it — even if it's a new dependency**. Hand-rolling is only justified when the library would add capabilities you actively don't want (tagging, env expansion, i18n layers, etc. you'd have to ignore), or when the hand-roll is genuinely a handful of lines with no branching.

_Rationale_: "Zero deps" is an easiness judgment dressed as a simplicity judgment. Code you don't own is genuinely simpler than code you do own — it doesn't accumulate private test fixtures, it doesn't bitrot when requirements shift, and its edge cases are someone else's problem to fix. Hand-rolled utilities routinely grow past the library they displaced as new edge cases surface. A small focused library with a single exported function is the _same_ complexity to your reader as a one-line utility import, and _less_ complexity than a 40-line loop with state variables.

_How to apply_: When you're about to write a loop with nested state-machine variables, a tokenizer, a parser, a semver comparator, a date math helper, a quoted-string handler, or a path normalizer — **stop and search for the focused library first**. Only fall back to hand-roll after seeing a concrete library and judging that its scope genuinely exceeds what you need.

_Anti-patterns in this rule's application_:

- "It's already in the tree" is not a _requirement_ for preferring the library. Adding a small focused dep is fine. The "already transitive" check is a convenience shortcut, not a gate.
- "Only ~40 lines, well-scoped" is not a license to hand-roll. 40 lines of state-machine code is usually worse than one line of library call plus a dep.
- "I don't want a dep" is dependency-aversion, not simplicity. State it as such in the eval and evaluate it honestly as a preference, not as a neutral principle.
- The gating criteria are **scope fit** and **bundle cost** — not ideology in either direction. Left-pad exists; don't flip to "always use a library." Judge each case on whether the library's surface matches what you need.

### invalid-states-unrepresentable

Use discriminated unions, not booleans or stringly-typed fields. If two fields can't both be `undefined` at the same time, model that in the type.

### no-dead-code

Aggressively remove unused code. No commented-out blocks, no "just in case" leftovers.

### no-silent-error-swallowing

Never silently swallow errors. Empty `catch {}` blocks, bare `catch: pass`, and `|| true` hide failures. At minimum, log the error. If the catch is intentional (best-effort operation), add a comment explaining _why_ the error is safe to ignore.
_Rationale_: Silent swallowing masks bugs — failures disappear without a trace, making debugging impossible.

### no-unbounded-growth

Collections, buffers, and listeners that grow with usage must have a bound or a cleanup path. Common violations:

- **Unbounded arrays/lists** — pushed in a callback or event handler with no cap or eviction. Ask: _can this grow forever during a long-running session?_
- **Missing debounce on high-frequency sources** — `fs.watch`, `resize`, `scroll`, `mousemove`, WebSocket `onmessage`, or any event that can fire many times per second. Each invocation that does non-trivial work (I/O, parsing, DOM mutation, allocation) needs a debounce or throttle. A bare handler is only acceptable if the work is O(1) and allocation-free.
- **Large allocations in hot paths** — reading an entire file/stream into a single buffer when the consumer processes it incrementally. Prefer streaming/chunked reads when the data source can grow without bound.
- **Duplicated watchers/listeners** — N callers each installing their own watcher on the same resource instead of sharing one. Each duplicate multiplies callback cost and file-descriptor usage.

_Rationale_: LLM-generated code defaults to the simplest correct implementation, which is often O(n) in session lifetime. These patterns silently degrade performance over hours/days and surface as "the app got slow" with no obvious cause. The fix is almost always straightforward (cap, debounce, stream, share) but must be applied at write time — it's rarely caught in review because the code is functionally correct.

### comment-the-non-obvious

Write a comment when a reader who didn't write the code can't tell, at first glance, **what it does** or **why it's structured this way**. The comment supplies whichever is missing — design intent (why this shape over the conventional one), control-flow semantics (what the cascade actually dispatches), or a hidden constraint the type system doesn't carry.

This is a positive prompt, applied per block: at every non-trivial declaration or block, ask the question. A "obvious to me because I just wrote it" reflex is the failure mode — reviewers and writers both pattern-match on what they already understand and skip the question.

## Running the passes

Spawn Pass 1 and Pass 2 as **two parallel sub-agents** via the harness's agent tool. On Claude Code this is the `Agent` tool with `subagent_type: "Explore"` (Pass 1 and Pass 2 are read-only audits — no edits). On opencode this is the `task` tool. On Codex, the sub-agent spawning tool. Emit both `Agent` tool_use blocks in a single response so they run concurrently — sequential spawning halves the parallelism.

Each sub-agent inherits no context from the implementer's main thread; the prompts below are self-contained and reference the rules-of-record by file path so a single source of truth stays in this skill.

Pass 3 runs **after** Pass 1 and Pass 2 return. It applies fixes (via `/simplify`) and would race against Pass 1/2's grep-and-read work if run in parallel; sequential is the safer ordering. See "Pass 3: Elegance" below. If `--no-elegance` was passed, skip Pass 3 — only Pass 1 and Pass 2 run.

Once all three pass outputs are in hand, stitch them into the summary table in the **Output** section.

### Pass 1: Rule checklist

Sub-agent prompt:

> You are Pass 1 (rule checklist) of the `/code-police` skill on this repo's current diff.
>
> Read the "Reviewing principles" and "Rules" sections of `.apm/skills/code-police/SKILL.md` for the built-in rule set. Also read `.agency/code-police.md` if it exists — its rules are additions to the built-in list (separate rows in the table, project-chosen rule IDs).
>
> **Scope:** the current diff against the merge base — run `git diff origin/HEAD...HEAD` (or the appropriate base-branch ref if `origin/HEAD` is unset).
>
> Produce a single table with **every rule** (built-in + project):
>
> | Rule ID | Violation found? | What was identified | Action taken |
> | ------- | ---------------- | ------------------- | ------------ |
>
> Every "No" requires a **`Checked by:`** field whose content is one of:
>
> - For purely-negative rules (e.g. `no-dead-code`, `no-silent-error-swallowing`): the grep that confirmed absence — _"grep'd for `head`, `tail`, `fromJust`, `(!!)`, `Map.!`, `error`, `undefined`; zero matches."_
> - For bidirectional rules (e.g. `comment-the-non-obvious`, `prefer-focused-library`): the enumeration of positive candidates ruled out — _"enumerated `am`, `keep`, guard branches, `G.stars`/`G.reachable` calls; for each, named why a fresh reader decodes the why from code alone."_
>
> A "No" without `Checked by:` is malformed and must be rewritten. Every rule must appear in the table — no skipping. Apply the "Reviewing principles" verbatim — do not rationalize a violation away because the diff "needs" it.
>
> Return only the table plus any per-finding rationale beneath it. Do not apply fixes; the orchestrator will route them.

### Pass 2: Fact-check

Sub-agent prompt:

> You are Pass 2 (fact-check) of the `/code-police` skill on this repo's current diff.
>
> Read the "Reviewing principles" section of `.apm/skills/code-police/SKILL.md` and apply them verbatim. This is **not** a style review — it is a logic review. Find places where the code lies to itself.
>
> **Scope:** the current diff against the merge base — run `git diff origin/HEAD...HEAD` (or the appropriate base-branch ref if `origin/HEAD` is unset).
>
> Flag:
>
> - **Silent error swallowing** — bare `try/catch: pass`, empty `catch {}`, `|| true`, errors caught but not propagated, `Result`/`Option` silently defaulted.
> - **Inaccurate fallbacks** — defaults masking misconfiguration, "sensible defaults" that aren't sensible for the failure case, fallback paths that silently degrade correctness.
> - **Wishful thinking** — assumptions about input shape without validation at boundaries, code that "can't fail" but actually can, race conditions papered over with comments.
> - **Logic errors** — always-true/false conditions, off-by-one, wrong operators, shadowed variables.
> - **Slow leaks** — collections that grow without bound, event handlers doing heavy work on every fire without debounce, watchers/listeners registered per-caller instead of shared, buffers sized to the full input when streaming would work.
>
> Operating principles:
>
> - **Fail loud over fail silent**: Code should scream when something is wrong, not quietly do the wrong thing.
> - **Fallbacks must be justified**: Every default/fallback needs a reason why that value is correct for the failure case, not just convenient.
> - **Precision over coverage**: Better to catch 3 real issues than flag 20 maybes.
>
> For each finding: file, line, one-line risk, concrete fix. If no issues, say so — don't invent problems. Do not apply fixes; the orchestrator will route them.

### Pass 3: Elegance

**Skip if `--no-elegance` was passed.** Do not run this pass and do not invoke `/simplify`; report `Elegance | – | Skipped (--no-elegance)` in the summary. The caller asserted `/simplify` already ran over this tree, so a second run is redundant. Pass 1 and Pass 2 are unaffected.

**Skip on tiny diffs.** Run `git diff origin/HEAD...HEAD --shortstat` (or the appropriate base-branch ref). If the diff is **under 10 lines**, skip this pass and report `Elegance | 0 | Skipped (tiny diff)` in the summary. The elegance pass's three-lens fan-out has overhead that's disproportionate to a few-line change; Pass 1 and Pass 2 still run. If the diff exceeds the threshold, proceed below.

Review the changes for elegance and simplicity.

**If running under Claude Code** (the `Skill` tool is available): invoke the bundled `/simplify` skill via the Skill tool. It runs three parallel lenses — reuse, quality, efficiency — over the current diff and applies fixes. Prefer this path.

**On other harnesses** (no `/simplify` available), run the inline loop instead. For each iteration (run 3 iterations):

1. **Understand** — Read through the changed files. Note patterns, repetition, unnecessary complexity, non-idiomatic code.
2. **Research** — Use WebSearch/WebFetch to research what simple, elegant (yet readable!) code looks like for the relevant technology.
3. **Apply** — Refactor based on what you learned. Prefer fewer lines, clearer intent, idiomatic style. Don't add abstractions — remove them.
4. **Verify** — Run tests/CI to check edits.

Principles:

- **Simple over clever**: Elegant code is simple code.
- **Readable over terse**: Brevity is good, but not at the cost of clarity.
- **Idiomatic over generic**: Use the language's strengths.
- **Each iteration builds on the last**: Don't undo previous improvements. Deepen them.

The "Reviewing principles" (prosecutor stance, no talk-out-of-findings) apply here too — `/simplify` and the inline loop both bind to them.

## Output

Stitch the three pass outputs into a single combined summary:

| Pass       | Issues found | Details                  |
| ---------- | ------------ | ------------------------ |
| Rules      | N            | Brief summary or "Clean" |
| Fact-check | N            | Brief summary or "Clean" |
| Elegance   | N            | Brief summary or "Clean" |

Below the table, reproduce each sub-agent's full findings (the Pass 1 rule table, Pass 2's per-finding list, Pass 3's `/simplify` log) verbatim so the orchestrator (`/do`'s police step) can commit each violation as its own fix.

If ANY pass found issues, clearly state: **"Violations or issues found"** so the workflow can route to a fix step.

If all passes are clean, state: **"All clear"**.

## Additional principles

### Simple, not easy (Rich Hickey)

Simple means _not interleaved_. Each module does one thing. Data flows through arguments and return values, not shared mutable state or indirection.

- No unnecessary abstractions. If a thing has one implementor, it doesn't need an interface/base class.
- No "for future use" code. Build what's needed now.
- Prefer plain data over objects with behavior.

### Completeness

- Implement the full spec. Read the plan/requirements and check every deliverable.
- Run CI locally before declaring done.
- Run tests.

### Justfile

- Every recipe must have a doc comment (line starting with `#` above the recipe name).

### Module structure — volatility-based decomposition

Group code by _rate of change_, not by technical layer. Things that change together live together; things that change independently get separate modules.

- Each module should own one volatility zone.
- Shared constants used by multiple modules get their own file.

### Readability

- Every exported type and every component needs a doc comment.
- Avoid deeply nested callbacks. Extract into named functions.