review-o11y

What this skill does

# Observability Review

Review a codebase's logging and error messages for consistency, level appropriateness, value, and coverage at I/O boundaries. Language-agnostic. This skill does **not** judge error-handling strategy (where to catch, whether to retry, whether to degrade) — that belongs to `review-code`. This skill judges the *artifacts*: the log statements that get emitted and the error messages that get constructed.

> [!IMPORTANT]
> Consult [REFERENCE.md](REFERENCE.md) for the expected output format and level of detail.

## Scope

Determine the review scope before discovering files:

- If `$ARGUMENTS` is non-empty, treat it as a path (file or directory) and run:
  ```bash
  ${CLAUDE_PLUGIN_ROOT}/scripts/discover-files.sh "$ARGUMENTS"
  ```
- If `$ARGUMENTS` is empty, scope to files added or modified on the current branch relative to the default branch:
  ```bash
  ${CLAUDE_PLUGIN_ROOT}/scripts/discover-files.sh
  ```

Handle the script's exit codes:
- **0 with output** — use the listed paths as input to the discovery step below.
- **0 with empty output** — branch has no diff vs the default branch. Tell the user and ask which path to review.
- **non-zero** — script prints a message to stderr (path not found, not a git repo, on the default branch with no path, detached HEAD, or default branch indeterminate). Relay the message and ask the user which path to review.

The script returns paths language-blind. The discovery step below filters to source files; if the filter excludes everything but the script's output was non-empty, the language may not be in the pattern list — apply judgment to identify source files in the output.

## Prerequisites

This review assumes standard tooling is already running:
- **Linters** catch empty `catch` blocks, unused imports, obviously broken format strings
- **Security scanners** catch hardcoded secrets and a few well-known PII patterns

This skill focuses on observability design issues those tools cannot detect: whether logs are useful, whether levels carry meaning, whether context is preserved, whether error messages are consistent and actionable, and whether the codebase logs at the places that matter operationally.

## Philosophy — prescriptive vs descriptive

This skill has two modes of judgement. Understanding the split is critical so you don't flag style choices as bugs or give bad logging a clean pass because it is "consistent".

**Prescriptive** — the skill has a fixed opinion regardless of what the codebase currently does. These rules exist because they reflect a broad operational consensus: break them and real oncall pain follows. Flag violations even if the whole codebase is consistently wrong.

**Descriptive** — the skill detects the codebase's dominant convention and flags outliers. These cover style choices where reasonable shops legitimately differ (logger library, field-name casing, error-message punctuation). A shop's consistent choice is its right; the skill's job is to notice drift, not to impose taste.

**Rule of thumb: semantics are prescriptive, syntax is descriptive.** Whether logs carry correlation context is a semantic question — always enforced. Whether the correlation id is called `request_id`, `requestId`, or `trace_id` is a syntactic question — detect the dominant choice and flag outliers.

## Workflow

### Step 1 — Discover source files

From the script's output, filter to source files, excluding test files, generated code, vendored dependencies, and pure configuration. Record the file list and count.

### Step 2 — Detect the dominant conventions

Before flagging anything on the descriptive axis, determine what "normal" looks like in this codebase. Sample up to 40 source files (or all of them, whichever is smaller). For each of the following axes, tally the distinct patterns and pick the majority:

| Axis | What to record |
|------|---------------|
| Logger library | e.g. `slog`, `zap`, `logrus`, `winston`, `pino`, stdlib `logging`, direct `print`/`console.log` |
| Log call shape | structured (key-value fields) vs string-formatted vs mixed |
| Field-name casing | `snake_case`, `camelCase`, `kebab-case` |
| Correlation field names | e.g. `request_id`, `trace_id`, `user_id` — which names are used |
| Error-message capitalization | leading uppercase vs lowercase |
| Error-message trailing punctuation | period vs none |
| Error-layer separator | `": "`, `" - "`, `", caused by "`, etc. |
| Error-message verb form | `"failed to X"`, `"could not X"`, `"X failed"`, `"error while X"` |
| Error construction pattern | sentinel errors, factory helpers, wrap-at-call-site, exception subclasses |

**Decision rule for flagging outliers:**

- A pattern that represents **≥70%** of the sampled occurrences is the dominant convention. Flag minority-pattern instances as outliers.
- If the top pattern is **<70%** but **≥40%**, the codebase has a legitimate split. Do **not** flag either side as an outlier. Instead raise a single P3 pattern finding: "codebase uses two conventions for <axis>; pick one." List representative files for both sides.
- If no pattern exceeds 40%, the codebase is chaotic on that axis. Raise a single P2 pattern finding naming the chaos and listing the top two or three variants.

Record the detected conventions and use them as the baseline in Step 3. Include a short "Detected conventions" block at the top of the final report so the reader can see what you anchored against (and catch you if you anchored wrong).

### Step 3 — Choose execution strategy

- **1–2 files → Direct mode**: Read the files, evaluate against the Criteria below, then proceed to Pattern Collapsing.
- **3+ files → Parallel mode**: Batch files, spawn subagents, collect results, merge, then proceed to Pattern Collapsing.

### Parallel Review Mode

Use this mode when 3 or more source files are discovered.

#### Batching

Group files into batches based on total file count:

| Total files | Files per batch | ~Subagents |
|-------------|-----------------|------------|
| 3–10        | 1               | 3–10       |
| 11–20       | 2               | 6–10       |
| 21+         | 3               | 7–10       |

#### Spawn subagents

For each batch, use `Agent(subagent_type="general-purpose")`. **Spawn all subagents in a single message** so they run in parallel.

Each subagent prompt MUST include:

1. The file paths in its batch (instruct the subagent to read them)
2. The **Criteria** section from this skill — copy it verbatim into the prompt
3. The **Severity** section from this skill — copy it verbatim into the prompt
4. The **detected conventions** from Step 2 — so the subagent knows what counts as an "outlier" on the descriptive axes
5. The structured output format below
6. The explicit instruction: **"Do NOT use the Bash tool. Do NOT run any shell commands. Use only Read, Grep, and Glob tools. Return findings only."**
7. The explicit instruction: **"For every P3 finding, you MUST state a concrete consequence in the `explanation` field: 'a caller/operator would likely \<specific operational mistake\> because of this.' Omit P3 findings that lack this claim."**
8. The explicit instruction: **"For the `pattern` field, use a short, reusable label that names the underlying anti-pattern (e.g., 'unstructured logging', 'error messages drop wrapped cause', 'PII in request logs'). If two findings in your batch stem from the same root cause, they MUST use the same pattern label."**

Instruct each subagent to return findings in this exact delimited format (one block per finding):

```
---FINDING---
priority: P<1|2|3>
location: <file:line>
title: <short title>
category: <Logging Consistency|Log Level|Log Value|Missing Logs|Error Message Quality|Error Message Consistency>
axis: <prescriptive|descriptive>
pattern: <short label for the underlying anti-pattern — use the SAME label across findings that share the same root cause>
explanation: <what is wrong and why it matters operationally>
fix: <concrete prescription>
done_when: <verifia