top-down-review

Top-Down Review

Review a change as a tech lead: first its value and product contract (WHY), then the solution it should have (WHAT), and only then the implementation (HOW). Do not substitute a retelling of the diff for the review. Do not edit code unless the user explicitly asked to fix the findings. Do not limit yourself to the checklist: if you see a risk outside it, check it. Do not treat formal correctness as sufficient: code without an obvious bug can still be a finding if it implements a harmful product decision or an objectively worse architecture.

Project review profile

A project can complement this skill with a review profile: REVIEW.md at the repository root with review checks that neither this skill nor the general project documentation covers. Apply its checks in addition to this skill. The profile adds checks and project context; it does not replace the process or the report format.

Follow Project profile when the user asks to set up a profile. During a review, only offer it: if the project has no profile and the review relied on project-specific checks worth recording, add the offer to To discuss.

Process

1. Read the project's agent instructions and REVIEW.md.
2. Determine the review scope from the request and do not expand it without a reason.
3. Determine the review range from the request: the base ref and the reviewed revision.

Use the same range in every later command. Typical ranges:

• uncommitted changes: the base ref is HEAD, and the reviewed revision is the working tree, including the in-scope untracked files from git status --short;
• specific commits: the base ref is the parent of the first commit, or the empty tree for a root commit, and the reviewed revision is the last commit;
• a branch: first run git fetch origin <base>, adding <branch> when the branch is not checked out; if the fetch fails, use the local refs and say so in What was checked.

The base ref is the merge base of origin/<base> and the branch head, and the reviewed revision is HEAD, the working tree when the request includes uncommitted changes, or origin/<branch> when the branch is not checked out. Take the base branch <base> from the request or the MR/PR target; otherwise use the base branch named in REVIEW.md, then the output of git symbolic-ref --short refs/remotes/origin/HEAD without its origin/ prefix.

If the request does not define the range, review the uncommitted changes when there are any, otherwise the current branch.

1. Collect the statement of the task before reading the implementation: the request, the MR/PR description, the linked issue with its attachments such as mockups and screenshots, commit messages, and the list of changed files from git diff --stat over the review range.

Look for the issue where REVIEW.md says tasks live instead of guessing a tracker. If these do not state the task, read the diff first and reconstruct the goal from it.

1. Select the documents for the affected area through the project's agent instructions.

Read them and the existing behavior the change touches in the base version with git show <base-ref>:<path>, because the change may have already rewritten them. Review the change's edits to these documents in HOW.

1. WHY: reconstruct the business requirement and assess the product contract, see Purpose and contract.
2. WHAT: design the target solution, see Target solution, and briefly write it down before opening the diff.

If you saw the implementation before this step for any reason other than the list of changed files, state in Product and architecture assessment that the target solution is not independent of the implementation.

1. Read the diff over the review range.

If it is large, split it into logical groups and read them one at a time: contract, backend, frontend, tests, docs.

1. Open the changed files at the reviewed revision, using git show <revision>:<path> when it is not the working tree, together with the nearest patterns next to them and existing tests for the same scenario.

Refine WHY and WHAT with the facts you find, but do not move the target solution toward the implementation only because it exists.

1. Scale the depth of the review by risk, not by diff size.

If the change touches authorization, money, data deletion, public contracts, migrations, concurrent operations, trust boundaries, or irreversible actions, also check the callers and consumers of the affected contract, the reasons behind the previous implementation in git log and git blame, the effect on existing data, and recovery after a failure. For a low-risk change, keep WHY and WHAT to 1-2 lines each and skip the history and consumer checks.

1. Compare the target solution with the implemented one, see Comparison.
2. HOW: check correctness, maintainability, test coverage, and documentation directly in the changed code, see Implementation.
3. Make separate passes over naming and over prose, not mixed with the logic review.
4. Verify every finding before the report, see Verifying findings.
5. The review is static by default.

To confirm a specific suspected problem, use the shortest reliable check: an existing focused test, typecheck, or a throwaway probe script that leaves the working tree unchanged. Run broad linters only at the user's direct request.

What to check

Purpose and contract (WHY)

• Formulate the product problem, the user, the expected outcome, and the immutable business invariants before assessing the implementation.
• Separate the stated requirement from the behavior the change actually adds.
• Check whether the new rules agree with each other, with the domain model, with role permissions, and with neighboring user scenarios.
• Look not only for direct breakage but also for footguns: behavior that provokes a mistake, irreversible loss, a hidden data change, a bypass of the expected process, or a degraded product under normal use.
• Assess secondary effects: what other roles will see, what happens to existing data, repeated actions, integrations, and future extension of the scenario.
• If the business meaning cannot be reconstructed unambiguously, state the assumption or question, but continue the assessment for the explicitly named assumption.

Target solution (WHAT)

• Provisionally accept the business requirement as confirmed and design the target solution within the project's architecture.
• Design from the business invariants and the project's existing boundaries, not from the files and abstractions in the changeset.
• Choose the simplest solution that fully covers the requirements and leaves a natural point for likely extension; do not build in speculative flexibility.
• Determine which owner is responsible for each rule, where the transport/domain/UI/persistence boundary lies, and what minimal public contract is needed between them.
• For data, determine the source of truth, transaction boundaries, concurrent operations, and the shape of the data returned to readers; for UI, the states, availability of actions, and reaction to errors.
• Define the necessary entities, extension points, naming, and the way to verify the solution.
• For a large change, design each independent functional unit separately instead of one global alternative.

Comparison

• Compare by clarity of contracts, coupling, number of entities, dependency direction, extensibility, complexity, and fit with the project's boundaries — by observable properties, not by similarity to your own sketch.
• When the task includes a mockup, compare the implementation with it and report each deviation that is not clearly intentional as a finding or in To discuss.
• An alternative is a finding only when all of these hold:
• it has a concrete advantage: it removes extra coupling or an entity, makes an invariant explicit, restores the dependency direction, simplifies extension, or reduces a real risk of errors;
• the advantage is material;
• you explored the alternative enough to propose it with confidence;
• its benefit outweighs the cost, risk, and scope of the rework.

Otherwise put it under Alternatives or, when the tradeoff needs a decision, under To discuss.

• If the implemented option is no worse, accept it as a full solution.

Under Alternatives, keep only architecture, flow, or naming options useful to the user, not micro-style or an enumeration of every possible implementation.

• Check what the change adds without need: state or a handler that an existing owner already covers, a layer that can be removed rather than moved into a new abstraction, changes the stated task does not require.
• Do not demand rewriting neighboring code unrelated to the change.

Implementation (HOW)

Correctness

• Check real failure scenarios, edge cases, pending and error states, and repeated user actions.
• Check whether a new data shape exposes data of deleted, hidden, anonymous, or inaccessible entities.
• Check that the UI shows actions to the same users the backend allows to perform them.
• Check optimistic UI, rollback, duplicates, repeated clicks, and competing updates if the change touches state.
• If the change allocates a value that must be unique, such as max()+1 or a counter, check that concurrent operations cannot both commit the same value, whatever the mechanism: a lock, a constraint with conflict handling, or another one.
• When the change introduces a constant — a limit, a threshold, a period — search for an existing setting or constant governing the same quantity and check that they agree.
• Check that an error does not turn into success through a broad catch, a fallback, logging without rethrowing, or an async operation nobody awaits.
• If the change alters a schema, an API, a storage format, configuration, or dependencies, check compatibility of old and new consumers, including mixed versions during a rollout, and backfill, rollback, rerun, and partial failure of a migration.

For a dependency upgrade, check the changelog and migration notes of the versions actually used.

• If the change replaces an implementation of existing behavior, such as a conversion, a parser, or a query, compare the old and new results on the same inputs, especially on inputs one side cannot represent, and name what changes or is lost.

Maintainability

• Flag an unclear contract, extra coupling, manual data stitching, and fragile structure.
• Do not propose an abstraction for its own sake.
• Propose a generalization if the diff repeats existing code or clearly creates a second source of truth.
• If a local pattern conflicts with a cleaner solution, explain the tradeoff and propose a safe next step.
• For a new entry point (mutation, procedure, endpoint), check its layer against the project's module boundary documentation: correct code in the wrong module is still a finding.
• For new files, renames, and public exports, classify the owner of each contract: domain API, transport input schema, response projection, persistence or config operation, UI presentation, test helper.

If a file name or layer does not match the owner of the contract, it is a finding.

• For new public types and data models, check whether they can express an invalid state, hide invariants, add unjustified optional fields, or break encapsulation.
• For new public exports, check that the declaration states the contract callers rely on: a caller that depends on a shape the implementation happens to produce turns that shape into an undeclared contract.

An inferred type is fine when the inferred shape is the intended contract.

• Check server/client, transport/domain, public API, and import boundaries.
• After a move or refactor, no old paths, wrappers, dead branches, or compatibility TODOs may remain unless existing consumers still need them.

Naming

• Go over new and changed names: types, functions, variables, files, API namespaces, DTO fields, UI states, and user-facing texts.
• For each new name of a domain entity or concept — in identifiers, comments and JSDoc, user-facing texts, and documentation — search with rg for how it is already named in the database schema, types, API namespaces, product documentation, and UI.

Local names of variables and helpers need no search. A new name for an already named entity is:

• major when it enters a public or persisted contract — an API, a URL, a database schema, an event — or spreads across modules;
• minor when the confusion stays within one module but needs an explanation;
• Polish when it is a self-explanatory local rename that changes no contract.
• A name that distorts the domain model, muddies a contract, or degrades API readability is a finding, not a matter of taste.

Prose

• Check every added or changed user-facing text, document, and code comment.
• Assess user-facing texts as product wording: natural language, not a literal rendering of code identifiers or another language.
• Flag filler: text that is technically correct but does not help its reader because it states the obvious, retells the code, repeats nearby text, or hedges generically.

Propose deleting it or a tighter replacement.

Tests

• For each new or changed behavior contract, name the invariant it must keep and ask which regression test would fail on the old code.
• Also ask which realistic breakage of the new behavior would make an existing test fail: an inverted access condition, removed error handling, a changed order of operations, or a shifted boundary value.

If the tests would still pass, they check an indirect effect or the shape of the implementation rather than the behavior. Answer this mentally; do not modify files or run mutation tests. If the answer depends on behavior the code does not define, such as row order without ORDER BY, treat it as an assumption.

• Match each invariant from WHY and each new or changed behavior contract to its direct test.

A gap is a finding unless you state why the risk is not real. Propose a test only when its expected result can be derived independently of the code under test; when a test could only restate the code, such as the branches of a template or the values its own mock returns, there is no gap.

• A test in the change that restates the implementation instead of checking an invariant is a finding; the fix may be to delete it rather than extend it.
• The fixtures must not already satisfy the expected outcome: an ordering assertion over rows inserted in the expected order, or a filter over data where every row matches, proves nothing.
• Do not accept a broad snapshot or an indirect effect as a replacement for a specific regression test.
• Check the test level against the project's testing conventions: the test must be at the lowest sufficient level.

Documentation

• If the change alters product behavior, a workflow, a boundary, or a development rule, check the owning document.
• If the change alters what users see or do, also check the product's user-facing documentation.
• Determine the owning document through the project's agent instructions and documentation conventions, not by the location of the nearest changed file.
• Do not demand a documentation update for a purely internal change without a change in observable behavior or a rule.
• If the change edits normative documentation, check that it describes the current state of the system rather than its history and does not duplicate a rule owned elsewhere.

Verifying findings

Keep a finding only if you can answer each question:

• Which concrete scenario or consumer exposes the problem?
• What confirms it: code, a contract, a test, or documentation?

When the finding depends on the behavior of a framework, ORM, database, or library, check the version the project actually uses through its lockfile, types, or sources, not documentation of another version.

• Does the change introduce or materially worsen the problem?

Mention a material pre-existing problem in one line as outside the change, not as a finding.

• Is every assumption behind it confirmed?
• Which minimal change fixes its root cause, and does it work in the proposed form?

When the finding prescribes a specific shape of the fix, sketch it against the actual code.

Drop a finding that fails the check; do not downgrade an unproven hypothesis to minor or to a question. If the code contradicts a requirement, it is a finding. If a requirement looks implemented but only a runtime check can confirm it, it is a residual risk: name the concrete check.

Report format

Respond in this order:

• Summary — 1-2 lines on the review range and the affected surfaces.
• Verdict — mergeable or not, with the main reason.

Any blocker makes the change not mergeable. With major findings, state whether merging is acceptable only with explicitly accepted technical debt and what that debt costs. When an architecture alternative decides the verdict, state the decisive tradeoff: what fixing it now costs and what accepting the current solution leads to.

• Product and architecture assessment — a brief business contract, the key invariants, and the target solution the changeset was assessed against.
• Findings — product issues first, then architecture, then implementation; within each layer, by descending severity.
• Polish — self-explanatory local fixes, see Polish.
• Alternatives — only useful options equivalent to the implemented one or not worth the switch.
• To discuss — open questions, assumptions the conclusion rests on, and decisions that need confirmation of the business requirement, a tradeoff, or the architecture direction; only what affects the conclusion or further actions.
• What was checked — what was checked and how: code reading, matching against tests, commands run.

A fully static review is normal content for this section; do not run checks just to fill it.

Omit empty Polish, Alternatives, and To discuss; with no findings, write Findings: none.

Severity

• blocker — must not be merged: behavior, access or security, data, or a public contract breaks.
• major — significant technical debt, an architectural mistake, a fragile contract, or an uncovered risk with a real cost.
• minor — a local improvement of readability, naming, a test, or documentation that does not block the merge but needs an explanation of its harm.

Findings

For each finding, give its severity, file, and line. State its layer: product (WHY), architecture (WHAT), or implementation (HOW). Write the concrete harm: what breaks, which contract becomes murkier, or what technical debt appears. Propose a concrete change, not general advice. A finding not referenced in To discuss is meant to be fixed as proposed. Number each finding and give it a short descriptive title — what is wrong and where — so it can be referenced without going back to the list. Do not leave withdrawn findings or severity changes made while reasoning in the numbered list.

When referring to a finding in To discuss, give its number and title together (for example #4 (order.list contract)), not a bare number.

Polish

Polish is a local fix that changes neither behavior nor a contract anything relies on and whose replacement explains itself: wording, minor naming, phrasing of comments and docs, formatting, small consistency fixes. Removing a parameter, an export, or a branch without consumers is polish; when a consumer may exist outside the reviewed code, such as a public API, it is a finding. If the harm needs explaining, it is a finding, not polish.

List all polish you found; do not drop it because blocker or major findings exist. Exception: skip polish in code that a finding proposes to rewrite or delete, and state in one line which findings' areas were not polished.

Group polish by file, with one nested line per fix: line — "old" → "new". Merge fixes of the same kind into one line that lists every location. Polish needs no severity, layer, or numbering.

Common rules

In every review, name the residual risk: what remained unchecked or where uncertainty remains. Do not mention staging, index, or commit status.

Feedback on a review

When the user or another reviewer points out a missed issue or a wrong finding, or asks to analyze a review, follow Review feedback.