# Audit Playbook

What to look for, per category. Each subagent (or direct audit pass) gets the relevant section plus the **Finding format** at the bottom. Adapt depth to repo size — a 2K-line CLI gets a lighter pass than a 500K-line monorepo.

Apply the shared flow, expected-load, and investigation-order judgments in **`mstar-audit` SKILL.md** §§ Phase 1–2. Reportability and structural fix selection follow **`references/finding-format.md`**.

---

## 1. Correctness / Bugs

The highest-trust category — real bugs found by reading, not speculation.

- Error handling: swallowed exceptions, empty catch blocks, `catch (e) { console.log(e) }` on critical paths, missing error states in UI code.
- Async hazards: unawaited promises, race conditions on shared state, missing cancellation/cleanup (stale closures in React effects, listeners never removed).
- Null/undefined flows: non-null assertions (`!`) on values that can be null, optional chaining hiding a value that must exist, unchecked array indexing.
- Boundary conditions: off-by-one, empty-collection handling, timezone/locale assumptions, integer overflow in counters/IDs.
- State machines: impossible-state combinations representable in types, status enums with unhandled branches (look for `default:` that silently no-ops).
- Concurrency: two operations can both pass a check before either writes; inspect the atomic claim/transaction and resulting shared-state effect, multi-write ordering, and retry idempotency (webhooks, queues). Check whether state or one-time work is incorrectly per-process when multiple processes must agree; respect the actual deployment model.
- Type escape hatches: `any` / `as` casts / `@ts-ignore` clusters — each one is a place the compiler was overruled.
- Resource lifetime: unclosed handles/connections/subscriptions, missing `finally`, and memory/maps/lists that only grow across requests or jobs; trace cleanup/eviction and the reachable accumulation at expected load.
- Derived-state drift: every cache, replay, projection, denormalized copy, or UI echo must trace to an authoritative source and an invalidation point; flag retained state with neither.
- Bounds covering the final operation: who owns the complete emitted/retained result (wrappers and metadata included)? Probe tiny/exact limits, oversized single chunks, and multibyte text against byte limits.

## 2. Security

Review only what is directly supported by code evidence. Keep findings framed as defensive maintenance: identify the code pattern, explain the production impact, describe the remediation. Keep remediation at the level of code changes, configuration changes, and tests.

**Handling rule:** never copy a secret value into a finding or the report — those files get committed. Reference the `file:line` and credential type only ("Stripe live key at `config.ts:12`"), and the fix sketch always includes rotation, not just removal.

**By-design is not a finding:** standard platform conventions are intentional behavior — honoring `https_proxy`/`NO_PROXY`, reading `~/.netrc`, an explicitly local dev tool shelling out to configured package managers. A tradeoff explicitly recorded in an ADR or decision doc is likewise settled. Flag these only when the *implementation* adds risk beyond the convention. Note: a **stale ADR is itself a finding** — if code has drifted from what the decision doc says, report the drift.

For the method behind this checklist — exploitability bar, false-positive discipline, input-source triage, and expanded surfaces — load **`references/security-review.md`** (deep method + FP discipline; load when the category focus is `security` or when the Security pass needs depth).

- Credential hygiene: hardcoded keys/tokens/passwords, credentials in committed `.env` files, credentials logged or persisted in event/history stores.
- Data crossing into interpreters or privileged APIs: SQL or shell operations assembled from request data (injection), HTML sinks fed by user-controlled content (XSS), dynamic execution APIs used with runtime input, filesystem paths derived from request data (path traversal).
- Access control: endpoints/server actions that lack server-side identity checks, authorization enforced only in the client, object access by ID without ownership or tenant checks (IDOR), missing request authenticity checks (CSRF) on state-changing routes.
- Input contracts: API boundaries that trust request bodies without schema validation, file upload handling without clear type/size/storage constraints, broad object assignment from request data into persistence models (mass assignment).
- Dependency posture: run the ecosystem's audit command (`npm audit`, `pip-audit`, `cargo audit`) in read-only mode. Report only critical/high advisories that affect reachable runtime code. Triage by reachability: critical/high + reachable → fix now; unreachable → lower priority. Never propose forced remediation (`audit fix --force`).
- Production configuration: overly broad CORS where credentials are allowed, missing response-hardening headers (e.g. CSP), cookies missing appropriate `HttpOnly`/`Secure`/`SameSite` attributes, debug/verbose behavior enabled in production.
- Data minimization: PII or sensitive operational data in logs, stack traces returned to clients, internal error details exposed through API responses.
- Enforcement bypass: for every validation/rejection point, look for alternate callers that route around it — direct calls, wrappers, facades, schema-less paths, listener ordering.
- Cross-file data-flow sweep: entry points → sinks across files; second-order injection (stored then reused unsafely); injection via field names/headers/metadata, not just values.
- Auth/session mechanics: JWT validation gaps (alg/claims/key-selection), session rotation on privilege change, password-reset token binding/single-use/expiry.
- Rate-limiting & abuse surfaces: auth/reset/expensive endpoints without limits — respecting deployment model (CDN-level limiting counts).
- AI/LLM feature surfaces (if present): model output treated as untrusted at its sink; tool permissions scoped per-user-resource; ingestion sources as indirect-injection vectors; consumption caps.
- Supply chain & pipelines: single authoritative lockfile at the install boundary, unreviewed dependency lifecycle scripts, unpinned CI actions / `pull_request_target`, typosquat signals on new deps.
- Infra/config surfaces: Dockerfile/K8s/Terraform misconfigs, debug modes & default credentials, exposed debug/actuator endpoints.
- Privacy retention: personal-data stores without TTL + working deletion path (backups/caches/indexes included).

## 3. Performance

Look for algorithmic and architectural wins, not micro-optimizations.

- N+1 patterns: query/fetch per item inside loops or per list-row rendering; missing batching or dataloader.
- Wrong complexity: nested scans over the same collection, repeated `find`/`filter` inside hot loops where a Map keyed lookup belongs.
- Caching gaps: identical expensive computations or fetches repeated per request/render; missing memoization at clear function boundaries.
- Payload size: over-fetching (select *, full objects where IDs suffice), missing pagination on unbounded lists, large JSON shipped to clients.
- Frontend (if applicable): bundle composition, missing code-splitting on rarely-hit routes, unoptimized images/fonts, render waterfalls.
- Backend: synchronous work that belongs in a queue, missing indexes implied by query patterns (flag for verification — don't claim without schema evidence), connection-per-request patterns where pooling exists.
- Build/CI: slow CI from missing caching, redundant pipeline steps, test suites that could parallelize.

## 4. Test Coverage

The goal is not a percentage — it's *which untested code is dangerous*.

- Map risky behavior (branches, parsers, money, auth, data mutation, the feature the repo exists for) to a meaningful check that would fail if it broke. A missing-test finding names the unprotected case, why existing assertions miss it, and the smallest owner-boundary regression check — not a coverage target. Apply the authoring gate in `references/test-audit.md` § 3.
- Modules with high churn (git log) + no tests = top refactor risk; flag as "characterization-tests-first" candidates.
- Existing test quality: tests that assert nothing meaningful, heavy mocking that tests the mocks, snapshot tests nobody reads, flaky patterns (real timers, real network, order dependence).
- Missing test layers: unit-only suites with zero integration coverage on API boundaries, or the inverse.
- Verification infrastructure: is there a one-command way to know the codebase works? If not, that's finding #1 and a prerequisite for any risky change.
- Real entry path: do tests exercise the shipped entry (CLI, loader, plugin boot) rather than a hand-mounted equivalent?
- Externally observable state: assertions verify logs, events, files, exit codes — never implementation restatement or agent-reported success.
- User-visible output is behavior (conditional): in repos shipping UI copy, CLI output, API error shapes, or prompt text, wording is behavior — snapshot or e2e coverage should pin it.
- Test keep/delete check: keep product-behaviour assertions and fails-first regression defences; delete incidental / source-shape / wiring / environment-constant assertions — deleted, never renamed or re-pinned. The auditor reports the non-product-assertion class and concludes **"delete, or replace with a product-behaviour assertion"**. PR #280 counter-examples: manifest parity guard, platform build-byte pins, raw-SQLite fixture, test-only header regression. Classify the assertion against its production-owner contract, not its name; apply the retention and candidate-evidence bars in `references/test-audit.md`.

For the deep method behind this category — value bar, authoring gate (used both to grade existing tests and to gate the test changes a finding recommends), the junk-pattern sweep, the retention bar, per-candidate evidence fields, discovery lanes, and campaign scope — load **`references/test-audit.md`** (load when the category focus is `tests`, via `/amazing-test-audit`, or when the Test Coverage pass needs depth).

## 5. Tech Debt & Architecture

- Duplication: near-copies of the same rule that must change together, even in two places; identify the canonical owner and any meaningful semantic differences before proposing reuse/merge.
- Layering violations: UI importing from data layer internals, circular dependencies, "utils" modules that became a junk drawer with high fan-in.
- Dead code: unexported-and-unused modules, feature flags fully rolled out but still branching, commented-out blocks, deps in the manifest no longer imported.
- Mixed responsibilities: a function/class/module doing unrelated jobs that obscure independent behavior or tests; identify the responsibility boundary, not a length target. Long but cohesive code does not need helpers merely to shorten it.
- Inconsistent patterns: three ways of doing data fetching / error handling / styling — pick the winner (the one the team converged on most recently) and recommend the consolidation.
- Abstraction mismatches: premature abstractions with a single implementation, or missing abstractions where the same change always requires touching N files in lockstep.
- Public-but-one-caller: a public method on a generic service with a single internal caller is a private-capability closure candidate.
- Unjustified defaults/public options: flag defaults or public operations/formats with no current-consumer evidence or prior art.

### Prove-or-reject before reporting DEBT (`simplify` scope)

**Prove before reporting dead code.** Classify consumers first — production corpus / tests-docs-only / ambiguous (examples, scripts: inspect, don't assume). Grep the exact symbol, plus event/field/config names, both quoted and bare. Read the call sites — a grep hit is a lead, not a verdict. "Tests are the only consumer" is a finding-enabler when the pinned behavior is non-load-bearing; "an invariant/test existing only to protect an unused API" is itself the signal.

**Hand-rolled vs dependency swap bar.** Name the exact surface the package covers — residual semantics count against the swap. Health-check the dependency honestly. A recorded decision (ADR/knowledge doc) beats the swap claim — re-litigating a settled tradeoff needs new evidence. Weigh net deletion: implementation + dedicated tests + docs − remaining glue. A wrapper that relocates the same complexity is not a win.

**Mirrored-fact test.** When several mechanisms track the same liveness/settlement fact, propose one controller — but preserve machinery protecting publication, rollback, callback containment, and first-terminal arbitration.

**Strong-candidate families** (one line per family):
- Symbols with no production consumer.
- Tests/docs-only consumers pinning non-load-bearing behavior.
- Two representations mirroring the same fact.
- Seam methods fully implemented for support but with zero consumption.
- Speculative product generality.
- Invariants/rollbacks/expected-outputs existing only to protect an unused API.
- Hand-rolled where a dependency exists.

**Guards.** A production caller exists → feature decision, not cleanup (reject). A recorded seam/ADR rationale → new evidence must beat it. Tiny-but-real items → "considered and rejected" rows in the index, never inline TODOs (Hard Rule 1).
**Chesterton's Fence.** Before classifying code as dead or removable when no recorded rationale exists, check history (`git log --follow <file>`, `git blame <path>`). Original reason still valid → reject the cleanup finding (one line in considered-and-rejected); reason obsolete → cite the committing context in the finding.

**Over-simplification trap guards.** Applies to the `simplify` variant and any DEBT simplification recommendation:
- Don't inline a named concept into harder-to-read call sites.
- Don't merge unrelated logic just to reduce the count.
- Don't remove abstractions carrying extensibility/testability value when no recorded rationale marks them stale.
- Line count is not the metric — comprehension is. If the "simplified" shape is longer or harder to follow, withdraw the recommendation.

## 6. Dependencies & Migrations

- Major-version lag on core framework/runtime (the ones with real cost to staying behind: EOL, security-fix cutoffs, ecosystem incompatibility).
- Deprecated APIs in use that have announced removal timelines.
- Abandoned dependencies (no release in years, archived repos) on critical paths.
- Duplicate dependencies solving the same problem (two date libs, two HTTP clients).
- Lockfile/manifest drift, version pinning inconsistencies across a monorepo.
- For each migration candidate, estimate blast radius (files touched) — that drives effort and whether to recommend it at all.
**Adding dependencies.** Every dependency is a liability — prefer the existing stack (stdlib, current utils). Flag an add as a DEP finding when any holds: the existing stack already solves the need; the footprint is unjustified for the problem; the package is not actively maintained; it carries known vulnerabilities reachable in practice; or its license is incompatible.
- **Upgrading dependencies:** read the changelog, not the version number; one dependency per change; suite green before *and* after — thin coverage around the dependency is itself a finding; review the lockfile diff including the transitive graph; never hand-edit the lockfile.

## 7. DX & Tooling

- Missing or broken: typecheck script, lint config, formatter, pre-commit hooks, editorconfig.
- Slow feedback loops: dev-server or test startup measured in minutes, no watch mode, CI without caching.
- Onboarding friction: README setup steps that are wrong/incomplete, undocumented required env vars, no `.env.example`.
- Missing `AGENTS.md` / `CLAUDE.md` — for repos where agents execute planned work, this is high-leverage.
- Error messages/logging: unstructured logs on services, missing request IDs/correlation, debugging requiring code changes.

## 8. Docs

Lowest default priority — only flag where absence has a concrete cost:

- Public API surface (published packages) without reference docs.
- Architectural decisions nobody can reconstruct (why X over Y) for actively contested areas.
- Stale docs that are actively wrong (worse than missing) — setup instructions, API examples that no longer compile.

## 9. Direction — features & where to take this next

Forward-looking: not what's broken, but what this codebase wants to become. **Grounding rule:** every suggestion must cite evidence from the repo itself — a suggestion that could apply to any project ("add dark mode", "add AI") is noise. Sources of grounded direction signal:

- **Unfinished intent**: TODO/FIXME clusters around one theme, feature flags never rolled out, stubbed or half-built modules, abandoned mid-feature work visible in git history.
- **Stated-but-undelivered**: README/docs/roadmap promises with no corresponding code, CLI flags or config options that are no-ops. A `STRATEGY.md` or `PRODUCT.md` that names users, use cases, or a direction the code hasn't caught up to is the strongest grounding signal — never propose something a decision doc already rejected (note the contradiction instead).
- **Surface asymmetries**: one-directional pairs (export without import, create without bulk-create), entities with CRUD minus one, a public API that internal code clearly needed and hand-rolled around.
- **The adjacent possible**: capabilities the existing architecture makes disproportionately cheap — a plugin system one interface away, a public API one route file from the existing service layer.
- **Friction worth productizing**: things users evidently do by hand around it (visible in docs, examples, issues).

Direction findings use the standard format with two adaptations: **Impact** is product/user value, and **Confidence** reflects how grounded the evidence is. Direction findings are usually pursued as design/spike work (investigate, prototype, define the API, list open questions) rather than as a build-everything change.

---

## Finding format

Use **`references/finding-format.md`** for the shared template, reportability bar, and structural remedies; read it before returning findings.

## Prioritization rubric

Use **`references/finding-format.md`** § Prioritization for the leverage rubric and tiebreakers.