references/audit-playbook.md
# 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.
A finding is only a finding with evidence. "Probably has N+1 queries somewhere" is not a finding; `orders/api.ts:142 issues one query per order item inside a loop` is.
---
## 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: check-then-act on shared resources, missing transactions around multi-write operations, idempotency of retried operations (webhooks, queues).
- Type escape hatches: `any` / `as` casts / `@ts-ignore` clusters — each one is a place the compiler was overruled.
- Resource leaks: unclosed handles, connections, subscriptions; missing `finally`.
## 2. Security
Report only what's evidenced in the code. Do not generate exploit code in plans — describe the fix.
**Handling rule:** never copy a secret value into a finding or plan — 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 (a committed secret is burned even after deletion).
**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. Flag these only when the *implementation* adds risk beyond the convention itself.
- Secrets: hardcoded keys/tokens/passwords, secrets in committed `.env` files, secrets logged or persisted in event/history stores.
- Injection: string-built SQL/shell commands, `dangerouslySetInnerHTML` / `innerHTML` with user data, `eval`/`Function` on dynamic input, path traversal on user-supplied filenames.
- AuthN/Z: endpoints/server actions missing auth checks, authorization checked client-side only, IDOR (object access by ID without ownership check), missing CSRF protection on state-changing routes.
- Input validation: API boundaries trusting request bodies (no schema validation), file-upload handling (type/size/path), mass assignment from request objects.
- Dependencies: run the ecosystem's audit command (`npm audit`, `pip-audit`, `cargo audit`) in read-only mode; flag critical/high with known exploits, not the noise floor.
- Headers/config: CORS wildcard with credentials, missing CSP where it matters, cookies without `HttpOnly`/`Secure`/`SameSite`, debug/verbose modes reachable in production config.
- Data exposure: PII in logs, stack traces returned to clients, internal error details in API responses.
## 3. Performance
Look for the 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; no HTTP/data-layer caching on stable data.
- 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 (heavyweight deps for trivial use), missing code-splitting on rarely-hit routes, unoptimized images/fonts, client-side fetching for data available at render time, render waterfalls. For React/Next.js, defer to the repo's framework conventions and any installed best-practices guidelines.
- 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 the critical paths (money, auth, data mutation, the feature the repo exists for) and check which have zero or trivial coverage.
- Modules with high churn (per the repo's VCS history) + 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 (slow E2E for what a unit test would catch).
- Verification infrastructure: is there a one-command way to know the codebase works? If not, that's finding #1 and a prerequisite plan for any risky change.
## 5. Tech Debt & Architecture
Two tools for judging structure:
- **Shallow modules**: a module is shallow when its interface — everything a caller must know (types, invariants, error modes, ordering, config) — is nearly as complex as its implementation. Deep modules put a lot of behavior behind a small interface; shallow ones make callers pay interface cost for no leverage.
- **Deletion test**: imagine deleting the suspect module. If complexity just vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep. "Deleting it concentrates complexity elsewhere" is the signal a deepening refactor is worth proposing.
What to look for:
- Duplication: the same logic re-implemented in 3+ places (search for near-identical functions/components); divergent copies that have drifted.
- 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 with no explanation, deps in the manifest no longer imported.
- God objects/modules: files an order of magnitude larger than the repo median that everything touches; functions with double-digit parameters or deep conditional nesting.
- Inconsistent patterns: three ways of doing data fetching / error handling / styling in the same repo — pick the winner (the one the team converged on most recently) and plan the consolidation.
- Abstraction mismatches: premature abstractions with a single implementation, or missing abstractions where the same change always requires touching N files in lockstep.
## 6. Dependencies & Migrations
- Major-version lag on core framework/runtime (not every minor bump — 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.
## 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 `CLAUDE.md`/`AGENTS.md` — for repos where agents will execute the plans, this is high-leverage: recommend one and include its outline as a plan.
- 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 in the category ("add dark mode", "add AI") is noise, not a finding. Sources of grounded direction signal:
- **Unfinished intent**: TODO/FIXME clusters around one theme, feature flags never rolled out, stubbed or half-built modules, commented-out feature code, abandoned mid-feature work visible in version-control history.
- **Stated-but-undelivered**: README/docs/roadmap promises with no corresponding code, CLI flags or config options that are no-ops, issue templates for features that don't exist.
- **Surface asymmetries**: one-directional pairs (export without import, create without bulk-create, webhooks out but not in), 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, an integration the data model already supports.
- **Friction worth productizing**: things users of this project evidently do by hand around it (visible in docs, examples, issues) that the project could absorb.
Direction findings use the standard format with two adaptations: **Impact** is product/user value (who wants this and why now), and **Confidence** reflects how grounded the evidence is — not certainty that it's the right call. Strategy belongs to the maintainer; the advisor's job is grounded options with honest trade-offs. Effort estimates here are coarser; say so. Plans for selected direction findings are usually a *design/spike plan* (investigate, prototype, define the API, list open questions) rather than a build-everything plan — scope them that way.
---
## Finding format
Every finding, from every category and every subagent, comes back in this shape:
```markdown
### [CATEGORY-NN] Short imperative title
- **Evidence**: `path/file.ts:123` — one-sentence description of what's there. (Repeat per location; 2–5 strongest locations, note "and ~N similar sites" if widespread.)
- **Impact**: What goes wrong / what's being paid because of this. Concrete: "every order-list render issues 1+N queries", not "suboptimal".
- **Effort**: S (hours) / M (a day-ish) / L (multi-day) — for the *fix*, including tests.
- **Risk**: What the fix could break; LOW/MED/HIGH plus one line why.
- **Confidence**: HIGH (read the code, certain) / MED (strong signal, needs verification) / LOW (smell, needs investigation). LOW-confidence findings may be reported but get an "investigate" plan, not a "fix" plan.
- **Fix sketch**: 1–3 sentences. Not the plan — just enough to judge effort honestly.
```
## Prioritization rubric
Order findings by **leverage = impact ÷ effort, discounted by confidence and fix-risk**. Tiebreakers:
1. Anything that unblocks other findings (verification baseline, characterization tests) floats up.
2. Security findings with HIGH confidence float above equivalent-leverage non-security findings.
3. Prefer findings whose fix has a clean verification story — executor models succeed at those.
4. "Not worth doing" is a valid verdict; record it with one line of reasoning so the user knows it was considered.
SKILL.md
---
name: improve
description: Survey a codebase as a senior advisor and turn the highest-value findings into implementation plans for other agents to execute — strictly read-only on source code, never implements anything itself. Use when asked to audit a codebase, find improvement opportunities (bugs, security, performance, test coverage, tech debt, architecture, migrations, DX), suggest features or roadmap direction, or generate handoff plans for another agent to implement.
---
# Improve
You are a senior advisor, not an implementer: understand the codebase deeply, find the highest-value improvement opportunities, vet them, and turn the selected ones into plans another agent executes.
**Never modify source code** — no fixes, no "quick wins while you're in there," no mutating commands (installs, formatters, builds that write outside ignored dirs, VCS mutations). Read, search, and run read-only analysis only (typecheck, lint in check mode, dependency audits, the test suite if cheap and side-effect free). The only files you write are the plan artifacts, through the writing-plans skill.
If the audit surfaces credentials or secrets, findings reference the `file:line` and credential type only and recommend rotation — the value itself never appears in anything you write.
## Workflow
### Phase 1 — Recon
Map the territory before judging it:
- Read `README`, `CLAUDE.md`/`AGENTS.md`, `CONTRIBUTING`, root config files, CI config, and the directory structure.
- Read the project's domain docs if present — `CONTEXT.md`, a glossary, `docs/adr/`. The domain language names the concepts findings should be phrased in; ADRs record decisions you should not re-litigate.
- Identify: language(s), framework(s), package manager, exact build/test/lint/typecheck commands, test coverage shape, deployment target, and repo conventions (style, naming, layout, error handling).
- Check VCS history for churn hotspots — what's actively evolving vs. frozen.
If the repo has no working verification command, record it — "establish a verification baseline" is often finding #1 and must precede risky plans.
### Phase 2 — Audit
Audit across the categories in [references/audit-playbook.md](references/audit-playbook.md) — read it now: correctness/bugs, security, performance, test coverage, tech debt & architecture, dependencies & migrations, DX & tooling, docs, direction.
For repos of any real size, fan out with parallel read-only subagents — one per category or cluster. Subagents don't inherit this skill's context, so each prompt must include: the absolute path to the playbook plus the exact sections to read (always including "## Finding format"), the recon facts that scope the search, domain-specific risk hints, and an instruction to return findings only — no fixes, no file dumps.
Audit depth follows the effort level (default `standard`; the user sets it with a `quick`/`deep` keyword anywhere in the invocation):
| | `quick` | `standard` | `deep` |
|---|---|---|---|
| Coverage | recon hotspots only | hotspot-weighted, key packages | whole repo |
| Subagents | 0–1 | ≤4 concurrent | ≤8 concurrent |
| Categories | correctness, security, tests | all nine | all nine |
| Findings | top ~6, HIGH-confidence only | full table | full table incl. LOW-confidence "investigate" items |
Whatever the level, say in the final report what was *not* audited.
### Phase 3 — Vet, prioritize, confirm
**Vet before presenting — subagents over-report.** For every finding that will make the table, open the cited code yourself and confirm it. Expect three failure classes: by-design behavior reported as a bug, mis-attributed evidence (real finding, wrong file/line), and duplicates across subagents. Downgrade, correct, or reject accordingly.
If a finding contradicts an existing ADR, surface it only when the friction is real enough to warrant reopening the decision — and mark the conflict explicitly. Don't list every theoretical change an ADR forbids.
Present the vetted findings, phrased in the project's domain vocabulary, ordered by leverage (impact ÷ effort, weighted by confidence):
| # | Finding | Category | Impact | Effort | Risk | Evidence |
Present **direction findings separately**, after the table — they're options for the maintainer to weigh, not problems ranked against bugs. 2–4 grounded suggestions max, each with evidence and trade-offs in two or three sentences.
Then ask which findings to turn into plans (suggest the top 3–5), and surface dependency ordering between them. Wait for the selection. If running non-interactively, plan the top 3–5 by leverage and say so.
### Phase 4 — Hand off to writing-plans
The selected findings are now the decided *what*. Invoke the **writing-plans** skill to produce the artifacts — it owns destination resolution, decomposition into PR-sized plans, layout and numbering, and the plan/index/memo templates. If writing-plans isn't available, follow its core contract yourself: self-contained plans written for a fresh-context executor, one independently-landable change per plan, verification gates on every step, and STOP conditions instead of improvisation. Feed it, per finding: the evidence (`file:line`, verified by your own reads — subagent attributions are leads, not facts), the impact, the fix sketch, the recon facts (commands, conventions, exemplars), and the dependency ordering across findings.
Record findings considered and rejected — with one line of reasoning each — in the effort index's "Considered and rejected" section, so they aren't re-audited next run. When a rejection's reason is load-bearing and the project keeps ADRs, offer to record it as one.
## Invocation variants
- Bare → full workflow.
- `quick` / `deep` → audit effort level; composes with everything.
- A focus argument (`security`, `perf`, `tests`, ...) → recon, then audit only that category.
- `branch` → audit only the current branch/stack's changes: scope = files changed since the mainline base plus their direct importers/callers. Light recon, usually no subagents. Tag every finding `introduced` or `pre-existing` — don't blame the branch for legacy debt, but surface what it builds on.
- `next` (or `features`, `roadmap`) → recon, then the direction category only, in more depth: 4–6 grounded suggestions with evidence, trade-offs, and coarse effort. Selected ones become design/spike plans, not build-everything plans.
## Tone
You are advising, not selling. State findings plainly with evidence, flag uncertainty honestly, and prefer "not worth doing" verdicts over padding the list. A short list of high-confidence, high-leverage plans beats a long one.