hardwood-review

name

hardwood-review

description

Review a Hardwood PR, local branch, or staged diff against the project's CLAUDE.md conventions and recurring failure modes. Use whenever the user asks to "review", "look over", "check", "audit", or "assess" a PR number, a branch, or pending changes in the hardwood-hq/hardwood repo. Also use when the user pastes a PR URL or says they want feedback before merging. Produces a checkbox markdown file with findings grouped by priority.

Hardwood code review

Project-specific code review. Encodes the conventions in /workspace/CLAUDE.md and the recurring failure modes the maintainer has surfaced in past reviews, so the review covers them by construction instead of by memory.

Priority frame: the Code Review Pyramid

Review effort is weighted by how expensive the issue is to fix after merge. Width = weight: the wide base is the load-bearing tier, the narrow apex is the least important.

       ┌────────────────────┐
       │     Code style     │   ← apex. Automate it; don't burn review cycles here.
      ┌┴────────────────────┴┐
      │        Tests         │
     ┌┴──────────────────────┴┐
     │     Documentation      │   ← focus your review effort
    ┌┴────────────────────────┴┐
    │     Implementation       │   ← focus your review effort
   ┌┴──────────────────────────┴┐
   │       API semantics        │   ← base. Hardest to change later — spend the most thought here.
   └────────────────────────────┘

When writing the findings file, sort by pyramid tier, not by checklist section. A small API-shape concern outranks a big style nit. Style items (no var, JavaDoc syntax, formatting) belong in a "Nits" footer — flag them so the author can fix in passing, but don't elevate them to "Blockers".

Anchor reference: https://www.morling.dev/images/code_review_pyramid.svg

Workflow

1. Identify the target

Parse the user's request for one of:

• PR number (e.g. "418") → gh pr view <n> and gh pr diff <n>
• PR URL → extract number, same as above
• Branch name → git diff main...<branch> (or whatever the main branch is)
• No argument → git diff (working tree) plus git diff --staged; if both empty, gh pr list and ask which one

For PR diffs, persist large output to a file and read it in chunks rather than letting it land in the conversation as one blob. The diff for a non-trivial PR can run 5k+ lines.

2. Read the design context

If the PR touches a new design area, look in _designs/ for a matching markdown file. Hardwood requires non-trivial changes to land a design doc; if one is expected but missing, that's a finding. If one exists, skim it before reading code so the review can flag drift between intent and implementation.

3. Run the checklist

Walk every item in references/checklist.md. Each item has a brief rationale and the failure mode it catches. Skip items that don't apply to the diff (e.g. Dive TUI rules on a core-only change). Do not skip items just because they look unlikely — the checklist exists because each one has been missed at least once.

4. Cross-check tests

For every new behaviour or matcher, check that test coverage is breadth-first across the type/op axes, not just the one the author exercised. Common gap: an oracle test that proves parity for two types when the implementation supports six. List explicitly which (type, op) pairs are not covered.

5. Check doc/code drift

When a PR adds a design doc, JavaDoc, or user-facing docs/content/*.md, read at least the load-bearing claims (gate descriptions, eligibility rules, supported types) and grep the code to confirm the wording matches. Mismatches between "≥ 2 distinct columns" in the doc and leaves.size() < 2 in the code are the dominant doc-bug class.

6. Prune to signal

Before writing the file, walk every candidate finding and apply the inclusion bar:

A finding describes something that is wrong: code that breaks, will regress under a plausible future change, violates a stated CLAUDE.md / design rule, or has a missing safety property the project relies on elsewhere.

Cut anything that fails this bar. Common cuts:

• Non-findings. "No public API change here, theme usage looks fine." If a pyramid tier has no findings, omit the section entirely — do not write a section header followed by a reassurance bullet.
• Taste calls without a stated rule. "Consider renaming kv to kvLines", "prefer Optional over null here." If CLAUDE.md or a design doc doesn't mandate it and the existing code is correct, drop it.
• Micro-optimisations on cold paths. Render-tier UI code, one-shot init, test fixtures. Flag allocations / branches only on paths that run thousands of times per second or are documented hot.
• Test-style polish when the test is correct. A brittle-but-correct assertion is not a finding; a missing test for a real edge case is. Worked cut: a test that asserts exact substrings depending on a width / indent constant. The test passes today. "It will break if anyone touches the constant" is not a defect, it's a property of every assertion. Don't flag. (A missing test that would catch the height-overflow bug at a different breakpoint is a defect — that survives.)
• PR-process / description hygiene. "The 'docs updated' checkbox is unchecked, please confirm." That belongs in a PR comment, not the review file.

Self-edit prompt for each candidate: "If the author shipped this exact line tomorrow, what specifically breaks, regresses, or fails a CI check / convention?" If you can't answer concretely in one sentence, delete the finding.

Lift decisions out of the findings. Some surviving items aren't pure fix-it work — they're forks the maintainer needs to answer. Move these to the ## Decisions section instead of leaving them mixed in with defects. The tell-tale shapes:

• "Either X or Y" phrasing ("drop the dead branch or move the helper", "document the -1 or remove it").
• Trade-offs the maintainer owns ("keep the per-class duplication for JIT specialisation or extract a shared helper" — project-wide pattern, not a local fix).
• Sub-pattern changes ("constructor-inject the filter or keep the setter" — both work; the choice is policy).

Decisions should be selectable — the maintainer can tick the option they pick and the file becomes a record of the answer. Format each as:

- **Q:** <question, one line>
  - [ ] **A.** <option, one clause — say what *changes* if this is picked>
  - [ ] **B.** <option>
  - [ ] **C.** <"keep as-is" — include this when the question implies a change, so disagreement has an explicit checkbox>
  - **Rec:** <A/B/C> — <one-line justification>

Always include a Rec: line, even if it's "no strong opinion — A reads cleaner". Posing a decision without doing the analysis homework just defers the work. Two options are typical; three when "keep as-is" is plausible; rarely more.

Worked example:

- **Q:** The `split("\n", -1)` branch in `wrapValue` is unreachable — no description contains a newline. Drop it or move the helper somewhere multi-line is exercised?
  - [ ] **A.** Drop the branch; `wrapValue` stays in `HelpOverlay`.
  - [ ] **B.** Move `wrapValue` to a shared `Strings` helper and exercise the multi-line path there (also resolves the duplication with `DataPreviewScreen`).
  - [ ] **C.** Keep as-is.
  - **Rec:** B — collapses two findings into one fix.

Items that are unambiguous fixes (one right answer, just hasn't been done) stay in their tier section, not in Decisions.

A decision must have at least two defensible branches. Some findings carry "either X or Y" phrasing but only one branch is a real fix — the other is just "document the magic / explain the constant", which is the lazy non-fix. These are not decisions. They stay under their tier (usually Implementation) as a defect.

Worked example:

wrapValue(description, Math.max(1, descBudget - 1)) — the -1 is unexplained. Either drop it or add a one-liner why.

The two branches are fix it vs paper over it with a comment. Only the first is a real answer. Stays under Implementation semantics as a defect, not a decision.

Compare to a real decision:

Drop the dead split("\n") branch, or move wrapValue to a shared helper where the multi-line case is actually exercised?

Both branches are defensible end-states — one shrinks the surface, the other widens reuse. Goes to Decisions.

If lifting a finding to Decisions would make the underlying defect disappear (because the question is "should we fix this or not"), keep the defect under its tier and only raise a Decision for genuine forks in how to fix.

Better to ship a short review with 5 real defects than a long one where the maintainer has to filter the signal out.

7. Write the findings file

Write to _reviews/pr-<N>-review.md (or _reviews/branch-<name>-review.md for non-PR runs). The _reviews/ directory already exists; do not write findings to the repo root.

Stamp the current session id on its own line under the PR URL, so "which session produced this review?" is answerable from the file alone instead of by grepping transcripts. Derive the id from the scratchpad path the harness gives this session: it is the path component immediately before /scratchpad (e.g. /tmp/claude-…/<session-id>/scratchpad). If that path isn't to hand, fall back to the newest *.jsonl under /claude-config/projects/-workspace/ — its basename is the session id. The id is a many-to-one pointer (a resumed session that reviews two PRs stamps the same id on both); that's expected — it identifies the session, not the review.

Format:

# PR #<N> — <title> — Review actions

<https://github.com/hardwood-hq/hardwood/pull/<N>>
Session: <session-id>

## Summary

**What:** <1–2 sentences. The actual change, in your own words. Not a copy of the PR description — what the diff does, structurally.>

**Why:** <1 sentence. The motivator, from the PR description / linked issue / design doc. If you can't tell, say "Motivator not stated in the PR or linked issue." — don't invent one.>

**Assessment:** <1–2 sentences. Headline verdict: ready to merge / ready with nits / needs work / not ready, and the single most load-bearing reason. The detail lives in the sections below; this is the elevator pitch.>

## Decisions
- **Q:** <question or fork — one line>
  - [ ] **A.** <option 1 — what changes if you pick this>
  - [ ] **B.** <option 2>
  - [ ] **C.** <option 3 — typically "keep as-is"       >
   — 


 [ ]  — one-sentence why


 [ ] ...


 [ ] ...


 [ ] ...


 [ ] ...


 [ ] ...

Emit only sections that have at least one entry. A pyramid tier (or Decisions / Blockers) with nothing in it gets no section header — silence is the signal. The template above lists all seven possible sections; a real review usually has two or three.

Use [ ] not [x] — the maintainer checks items off as they're addressed (per CLAUDE.md "Code Reviews" section).

8. Hand back a short summary

After writing the file, give the user a 3–5 sentence summary: what the PR does, the highest-priority finding(s), and the path to the findings file. Do not repeat the whole list inline — they can read the file.

When NOT to use this skill

• Reviewing a PR in a different repo. The checklist is hardwood-specific.
• Asked to apply a fix or implement a feature. This skill produces findings, not edits.
• Asked for a one-line opinion ("does this look OK?"). Just answer.

Output discipline

• Findings file uses [ ] checkboxes per CLAUDE.md.
• Group by pyramid tier (see template). Sections with no findings are omitted entirely — do not write a section header followed by "looks fine" or a non-finding bullet.
• Each item is one sentence stating the issue and one sentence (or clause) on why it matters / what fails if ignored. No multi-paragraph justifications.
• Reference specific files / line ranges where useful. Don't fabricate line numbers from memory — if you're not sure, drop the number.
• If a finding is judgment-dependent (e.g. "consider A/B-ing this duplication"), say so. Don't dress up an opinion as a defect.
• A short review with five real defects beats a long one where the maintainer has to filter the signal out. When you find yourself reaching for noise to "look thorough", stop.