Architectural Quality Audit: $ARGUMENTS

Target: a file, directory, changeset, or commit range. Empty $ARGUMENTS → audit the files changed on this branch, and say that is what you scoped to.

Read-only. This skill classifies and recommends; it never edits code. The deliverable is a written report plus a refactor plan, not a set of fixes.

Principle

Architecturally correct, even when longer, beats a quick hack that "just works". Every piece of code should be what you would design knowing the requirements will change. Where it is a shortcut, say so plainly and show what SOUND would have looked like here — a verdict without the correct alternative is a complaint.

"Correct" means correct for this codebase: the layer map, the canonical patterns, and the sanctioned exceptions all come from the profile, never from a generic notion of good design.

Phase 0 — Load the architecture you are auditing against

• PROJECT.md → Architecture: the layer/module map, the dependency direction, where logic belongs and where it must not go. This is the layer map for Check 2 — do not invent one.
• PROJECT.md → the canonical exemplars it names (the factory, the registry, the entry shape) — these are the "canonical example" column of Check 3.
• .claude/rules/_generic/*.md and every .claude/rules/ file whose paths frontmatter matches the target. Project rules and CONTEXT.md carry the sanctioned exceptions — a documented, blessed deviation is not a finding.
• CONTEXT.md for domain vocabulary, so findings are named in the project's own terms.
• No profile yet? If PROJECT.md is missing or still TEMPLATE, do not stop: prefer any architecture the root CLAUDE.md carries, else infer the intended structure from the tree and the target's siblings. State the assumption at the top of the report and proceed — an audit against an inferred architecture is worth having, an audit that pretends to a profile it doesn't have is not.

Phase 1 — Read before judging

Read every file in scope fully, plus enough surrounding code to judge pattern conformance, not style: the callers, the module's siblings, the interface or factory it should conform to.

The evidence gate — before any HACK or SHORTCUT verdict. A pattern that looks wrong and has a logged reason is intent, not a hack. Prove it, or the file stays SOUND:

1. git log -p -5 -- <file> — was this shape introduced deliberately, and does the message say why?
2. The callers (grep the imports/uses) — does the "wrong" shape exist because of a real constraint at the call site?
3. The sanctioned exceptions in the project rules / CONTEXT.md — a blessed seam is legal by definition.
4. Prior settled decisions (ADR / decision log / the plan that shipped it) — re-litigating a decision the project already made is out of scope here; route it to finding-verifier.

Only after those four does a finding get a HACK or SHORTCUT verdict.

The checks

Run all eleven over the scope. A check that does not apply to this stack (9, for a project with no database) is stated as not-applicable, not silently dropped.

Check 1 — Abstraction quality

• Is there polymorphism expressed as type-switching (isinstance chains, if kind == "…") where the language's interface/protocol mechanism belongs?
• Is construction direct where the codebase has a factory, and dispatch hardcoded where it has a registry?
• Are string literals doing the job of an enumerated type?
• Are there magic numbers that belong in named constants or config?

Check 2 — Layer placement

Against the layer map from PROJECT.md → Architecture. The recurring red flags, expressed layer-neutrally:

• A domain/core module doing I/O.
• A service module doing presentation/formatting.
• An infrastructure module carrying business rules.
• An entry-point module (CLI command, controller, handler) doing anything beyond building config and delegating — see rules/_generic/service-layer.md.
• A module importing a sibling in the same outer layer where the profile says bounded contexts must not know each other.

Check 3 — Pattern conformance

Does new code follow the established pattern, or introduce a one-off? Build the table from the profile's exemplars — one row per pattern the codebase actually has:

N divergent shapes for one job in sibling files is itself a finding: which one does a newcomer copy?

Check 4 — Dependency direction

• Grep the imports in every target file and trace the chains: does any lower layer import a higher one?
• Circular-import risk, and annotation-only imports that should be guarded.
• Does this changeset introduce an upward dependency that did not exist before? That is the one worth blocking on — a pre-existing one is a finding for the plan, not a blocker for this change.

Check 5 — Hack smell

For each hit, say why it is a hack and what the correct pattern is:

• Magic numbers in logic · string constants used as dispatch · isinstance/type-switch chains.
• Global mutable state; module-level state mutated at runtime.
• Runtime attribute injection / monkey-patching.
• Defensive masking — catch-all handlers, silent fallbacks that hide the bug instead of handling a real failure mode (rules/_generic/exception-patterns.md).
• Copy-paste: 3+ lines duplicated with minor variation where a helper belongs.
• A flag parameter (a literal at every caller AND a branch in the body; enums count) — two functions.
• Deep nesting past ~3 levels where guard clauses would flatten it.
• Explanatory hack comments — # workaround for … is the code telling you the finding.

Check 6 — Composition over inheritance

• Inheritance for anything but a true IS-A: a supertype method that does not apply to the subtype, or a subtype instance invalid where the supertype is valid (Liskov substitution, the /prepare SOLID row) — name the condition, not the slogan. Tells: an override raising "not implemented"; callers branching on the concrete type; inputs narrowed or outputs widened against the parent.
• Callers normalising the result (cast, null-coalesce, type check) — the callee broke the contract.
• A second axis of variation in the hierarchy, or a class modelling both a type and its instances.
• super() chained through several levels — fragile; prefer explicit delegation.

Check 7 — Extensibility

If the requirements shift slightly, how much has to change?

• Are the extension points explicit (interfaces, registries, factories, plugin hooks), or does extending mean editing existing code?
• Count it: how many files change to add one more variant of the thing this module is about? The healthy answer is usually two — the new file, and the registry that lists it.
• Would swapping one external dependency touch more than its adapter?
• Ask first whether a second variant is coming. A module with one provider in two years is not weakly extensible: an absent extension point is a finding only when the variation is real.
• A finding whose fix would remove, move or collapse a seam carries an ignore-if line — the condition under which the shape is deliberate, and the check that ruled it out.

Check 8 — Error handling

• Handled at the right level — not swallowed at the boundary, not propagated past the layer that can act on it.
• Domain errors distinguished from infrastructure errors.
• Recovery paths explicit and tested, not hoped-for.
• Silent swallows, and error handling that masks a bug rather than handling an expected failure.
• Retries on non-idempotent operations (see Check 10).

Check 9 — Data-access patterns (where the project has a datastore)

Per the stack rules for the actual engine. Engine-neutral shape of the check:

• Indexes present for the columns the queried code filters, joins and orders on.
• No unbounded column selection in production paths.
• No N+1 access pattern — a query inside a loop over rows.
• The client's commit/transaction semantics are the ones the code assumes — an async or autocommit-off client that the code treats as autocommit is a silent data-loss bug.
• Multi-step writes that must be all-or-nothing are inside one transaction.
• Connection-time settings applied where the engine requires them.

Check 10 — Write-path idempotency (mandatory, and it can block a SOUND verdict)

The invariant is already a write-rule (rules/_generic/resilience.md → Idempotency); this is its read-side obligation. The audit must open the write-path, not infer health from structure.

For every state-mutating path in scope (persist, accumulate, finalize, publish, notify):

• Fires twice — replay, restart, retry, a re-delivered event: same end state, or a doubled one? A blind += on a counter or a balance with no processed-id guard is the canonical defect.
• Fires a day later — by the time the retry runs, the state it read has moved: a token expired, a price changed, the row was deleted. A decision recomputed from live state then does nothing, or the wrong thing, and "same end state" still passes — so the check above cannot see it. A decision that depends on a value at trigger time is passed that value. Partial idempotency is fine when named.
• Two racing paths reach the same write — a normal path and its timeout/cleanup twin, a reconciliation loop overlapping the live handler.
• The guard is real, not conventional — a predicate in the UPDATE, a unique constraint, an upsert on a stable key, a deterministic idempotency key, a processed-id set. It has to be enforced at the write — a duplicate key raised and caught — not a SELECT-then-INSERT check, which passes this audit today and still races. "The caller only calls it once" is not a guard.
• The transaction boundary matches the invariant — a multi-step write that commits per step is a partial apply on crash.

Verdict consequence. A module that writes state and whose write-path you did not open cannot be classified SOUND — either open it, or name the gap in the verdict line itself (core.md → verdicts carry denominators: "sound across the read-paths I opened; write-path unread"). A structural audit that never opens the write-path can say "the structure is sound", never "sound". A double-apply found here is a correctness bug → Phase 0 of the refactor plan, not structural debt.

Check 11 — Prose as a claim about the code

Docstrings, comments, help strings and plan sentences that were true when written and are false now. Checks 1–10 read the code; this one re-derives the sentences about it as claims. (Mined from a 287-entry agent-memory corpus: ~60 of 109 stack-neutral lessons are this shape, the single most repeated one 43×.)

• Grep the rule as prose, not as the symbol — the symbol finds the code, never its twin sentence. Search the old condition's wording, the cardinal ("four routers"), every enumerated member, the old text as a quoted literal, the flag token, header "limits" lists, the comment above a changed constant, help strings, operator docs.
• A completed enumeration must not carry a quantifier — "both", "all", "the only" over a list someone will extend: enumerate the members or drop the count. One added member falsifies every universally quantified header above it.
• A constant changed by more than ~2× invalidates its numberless premise — the sentence with no number in it (the cost, the reason, the headroom) is the one nobody re-derived.
• Comparatives name both populations — "stricter"/"looser" than what, over which inputs; a delta labelled as one job's cost names the other writers of the measured store.
• An absolute guarantee is read against the producer's non-happy branches (absent, default, inferred, empty-is-truthy). Where prose claims a guarantee and also lists a residual, the residual wins.
• "Now redundant" / "0 live today" / a past-tense migration note is a licence to delete that a later session will act on. Re-run the census, or the change ships the permission ahead of the fact.

A finding is path:line + the sentence + the single grep or input that falsifies it — not "this comment looks stale". Rarer shapes and worked cases: docs/claims-audit-patterns.md.

Verdict rubric

Classify each file by the worst thing in it:

• SOUND — architecturally correct for THIS codebase: right layer, right seam, conforms to the pattern its siblings established, invariants respected, tested where the profile expects tests. Would survive a requirements change. No action.
• SHORTCUT — works, but takes a debt-generating shortcut: duplicated logic instead of reusing the existing seam, a bypassed abstraction, a missing test for behaviour the profile classes as critical, a hardcoded value that belongs in config, divergence from the sibling pattern. Bounded fix; schedule it before building dependent features.
• HACK — violates the architecture or plants a trap: logic in a forbidden layer, an upward/sibling dependency the profile forbids, a design decision leaked across modules, swallowed errors or fail-open paths, an unguarded state mutation two paths can reach (double-applies on replay or a race), fake conformance (implements the interface, breaks its contract). Must be fixed before building on top.

A "cleaner would be nicer" observation with no concrete harm scenario — a failure, a newcomer's confusion, a future extension cost — is not a finding. Drop it.

Completeness pass (multi-file targets)

Run this against the draft audit, on yourself, before writing the verdict — the scope being more than a couple of files is what makes it mandatory:

• Every file in scope got a row. Rebuild the scope list from the target (glob or git diff --name-only), diff it against the table, and name any file that has no row.
• Every SOUND verdict rests on cited evidence, not on the file having been skimmed.
• Every check ran or was declared not-applicable — layer placement, data access, and write-path idempotency especially. A SOUND row on a state-writing file with no Check-10 evidence is exactly what this pass exists to catch.

A gap found here is closed before reporting, not footnoted.

Why this is a self-check and not the completeness-critic agent. Neither context this rubric runs in can spawn: in a fork the spawn tool stays listed but returns an error instead of launching, and preloaded into quality-auditor it is a subagent, where Agent is stripped outright (rules/_generic/delegation.md § Tools an agent will not get). Either way a fan-out here would read on the way out as a failed phase rather than as a configuration mistake. The independent critic is not lost — it moves one layer up: the run hands its report back, so the caller can run completeness-critic over the written file (its input contract takes a path), which is a stronger check anyway — a genuinely separate context, reading the artifact rather than the draft. Recommend it in the return line when the scope was large.

Report format

## Quality audit: <target>

| File | Verdict | Key finding (path:line) | Correct alternative | Effort | Blocking? |
|------|---------|-------------------------|---------------------|--------|-----------|

### Overall — SOUND | NEEDS WORK | ARCHITECTURAL DEBT
<1–3 lines: is this safe to build on?>

### Coverage
write-paths opened: <paths> · read in full: <paths> · read by diff or window only: <paths, or
"none"> · unread: <paths, or "none in scope">

### Priority fixes
1. <most impactful — file, what, why>
2. …

Architectural debt estimate: N files need refactoring before this area can be safely extended.

Every finding is anchored to path:line as the code exists now, and only in files you actually opened.

The Coverage line is not optional, and it names the route, not just the set — "read in full" is checkable against your own tool log and is routinely false. Measured over a 146-run subagent corpus (2026-07-26..28): of 10 reports asserting the whole changed set was read in full, 6 were contradicted by their own calls — three had windowed every read of a changed file, and three had run only a diff. Splitting the line makes the honest answer sayable: "read by diff only" is a legitimate coverage level; claiming it as a full read is not.

Refactor plan (REQUIRED — findings alone are half the job)

An audit that stops at a list hands back unordered work. After the verdict, produce a prioritized, ready-to-work plan:

1. Separate bugs from refactoring. A HACK that misbehaves at runtime (wrong result, silent no-op, corrupted state) is a correctness bug, not structural debt. Bug fixes go in Phase 0 — an independent first phase that ships before any restructuring and does not depend on it.
2. Phase the rest by risk and dependency. Each phase states what it changes, why, and its reversibility (easy-undo vs one-way). Pure relocations and extractions with no behaviour change are grouped and labelled as such — that is the safe bulk.
3. Name the invariants that must not break across the refactor: the behavioural contract the module guarantees today. These are the regression guardrails.
4. Say what NOT to do. Explicitly decline work that is not worth it — a facade worth keeping, a file worth leaving alone. A plan that refactors everything is as wrong as one that refactors nothing.
5. Untested surfaces. Flag touched code with no test coverage: that is where the plan is riskiest, and a characterization test goes in before the change.

| Phase | Type (bug/structure) | Changes | Reversibility | Invariants at risk |
|-------|----------------------|---------|---------------|--------------------|
| 0 | bug | <correctness fixes, independent, ship first> | — | <what must still hold> |
| 1 | structure | <relocations / extractions> | easy-undo | |

**Do NOT touch:** <what to deliberately leave, and why>
**Test-first:** <untested surfaces the plan hits — characterization test before changing>

Write the report to a file (REQUIRED)

This skill runs in a fork, so a result left only in the conversation is lost. Write the full report — per-file table, Overall, Coverage, Priority fixes, Refactor plan — to the plans/backlog location from PROJECT.md (else offer a path and confirm):

<plans>/<target-slug>-audit.md

where the slug is the audited path. If that file exists, write -audit-2.md — never overwrite a prior audit; the old one is the evidence that a finding is a repeat. Then return, in the conversation, a short summary plus the file path — not the whole report. When the scope ran past a handful of files, add one line recommending the main thread run completeness-critic over that file: the fork cannot spawn it, the thread that receives the path can.

Exception — preloaded into quality-auditor. That agent carries this file as its rubric (skills: frontmatter) and /implement spawns it synchronously, collecting its reply and continuing it by SendMessage. No fork, no notification to lose: there the final message is the deliverable (agents/quality-auditor.md → Output), the agent declares no Write, and no file is written. The obligation above is for the /audit-quality run.

Hard rules

• Read-only on app code. Report, never fix. The only file you write is the audit report.
• Evidence before verdict. The four-step gate in Phase 1 runs before any HACK/SHORTCUT.
• Every architecture fact from the profile. Layer maps, canonical patterns, and sanctioned exceptions are read, never assumed.
• Check 10 gates SOUND. A state-writing file with an unopened write-path gets a named gap, not a clean verdict.

See also

• /code-review — defects in a diff (correctness, security, performance). This skill asks whether the design is right; that one asks whether the code is wrong.
• /arch-health — the whole repository, ranking where debt is concentrated. This skill is scoped to a module or changeset.
• /refactor — applies quality cleanups. This skill produces the plan those fixes follow.
• quality-auditor agent — the same rubric as a fan-out unit over a disjoint file list, for when an orchestrating skill needs per-file verdicts rather than a report.