84 lines
5.1 KiB
Markdown
84 lines
5.1 KiB
Markdown
---
|
|
name: code-review
|
|
description: Review changes since a fixed commit, branch, tag, or merge-base along separate repository-standards and specification axes. Use for branch, PR, work-in-progress, or workflow-record review after implementation and verification.
|
|
---
|
|
|
|
# Code review
|
|
|
|
Run two independent, read-only reviews over one pinned diff:
|
|
|
|
- **Standards:** repository instructions and documented conventions.
|
|
- **Specification:** the originating issue, spec, or approved workflow record.
|
|
|
|
Keep the axes separate so conformity cannot hide incorrect scope and correct scope cannot hide standards violations.
|
|
|
|
## 1. Pin the review bytes
|
|
|
|
Require a fixed point; ask when none was supplied. Resolve and record:
|
|
|
|
- review base commit;
|
|
- immutable product HEAD commit, never mutable `HEAD` in the reviewer brief;
|
|
- clean status and product tree hash;
|
|
- `git log <base>..<product-head> --oneline`;
|
|
- exact command `git diff --binary --full-index --find-renames <base>...<product-head> --`.
|
|
|
|
Run that command once in the parent, hash its exact bytes with SHA-256, and record the digest. Stop on a bad ref, dirty product state, or empty diff unless an empty comparison was explicitly expected.
|
|
|
|
## 2. Resolve specification and standards
|
|
|
|
Accept the first available specification source:
|
|
|
|
1. a caller-supplied approved workflow record or spec path;
|
|
2. issue references in commits, using `docs/agents/issue-tracker.md` when configured;
|
|
3. a matching file under `docs/`, `specs/`, or `.scratch/`;
|
|
4. an explicit user statement that no specification exists.
|
|
|
|
A workflow record is a complete specification source and never requires issue-tracker configuration. Ask only when no source or explicit no-spec decision exists. Skip the specification reviewer and report “no specification available” only after that explicit decision.
|
|
|
|
Collect applicable `AGENTS.md`, `CLAUDE.md`, contributing, coding-standard, and ADR sources. Add this smell baseline where repository standards do not override it; treat each as a judgment call and skip tooling-enforced issues:
|
|
|
|
- **Mysterious Name:** a name hides purpose.
|
|
- **Duplicated Code:** the same logic shape recurs.
|
|
- **Feature Envy:** behavior reaches into another object's data more than its own.
|
|
- **Data Clumps:** fields or parameters repeatedly travel together.
|
|
- **Primitive Obsession:** primitives stand in for a domain concept.
|
|
- **Repeated Switches:** the same type cascade recurs.
|
|
- **Shotgun Surgery:** one change requires scattered edits.
|
|
- **Divergent Change:** one module changes for unrelated reasons.
|
|
- **Speculative Generality:** abstraction exists without a current requirement.
|
|
- **Message Chains:** callers navigate long object chains.
|
|
- **Middle Man:** code delegates without adding value.
|
|
- **Refused Bequest:** an implementation ignores most inherited behavior.
|
|
|
|
## 3. Enforce reviewer independence
|
|
|
|
List every display name, profile, child/session ID, and ambiguous run that authored or mutated the diff. Select two different reviewer identities outside that set. Require repository read access plus Git-capable `bash`, with no declared `write`, `edit`, or nested-agent capability.
|
|
|
|
Capture branch, HEAD, status, tree hash, and changed paths before review. Launch the standards and specification reviewers as separate direct subagent calls rather than a wrapped parallel call. Give both the pinned commits, exact diff command, expected digest, commit list, mutator exclusions, and identity-reporting requirement.
|
|
|
|
Require each reviewer to:
|
|
|
|
1. regenerate the exact diff and confirm its SHA-256 before analysis;
|
|
2. remain read-only and perform no remediation;
|
|
3. bind every finding to file/hunk evidence and severity;
|
|
4. report display/profile/child/session identity when exposed;
|
|
5. stay under 400 words.
|
|
|
|
For standards, include every standards source and the full baseline above. Ask for documented violations separately from baseline judgments. For specification, include the approved source and ask for missing/partial requirements, scope creep, and apparently implemented but incorrect behavior with quoted requirements.
|
|
|
|
Treat missing delivery, digest mismatch, reviewer/mutator identity overlap, or before/after repository mutation as no verdict. Any reviewer mutation also makes that identity a mutator.
|
|
|
|
## 4. Aggregate without reranking
|
|
|
|
Present the reports under `## Standards` and `## Specification`. Lightly clean formatting only; keep findings and axis severity separate. End with finding counts and the worst issue within each axis, not one combined winner.
|
|
|
|
Bind approval to the base, product HEAD, clean status, tree hash, and diff digest.
|
|
|
|
## 5. Remediate through non-reviewers
|
|
|
|
Validate findings against repository evidence. Assign accepted fixes to an original implementer or another non-reviewer. Rerun affected checks and the full required suite, create a new pinned fingerprint, and launch two fresh independent reviewers.
|
|
|
|
Escalate after two unsuccessful remediation cycles or when a finding materially changes scope, acceptance criteria, ownership, verification, or architecture. Return material changes to plan approval before production mutation.
|
|
|
|
**Complete when:** both axes approve the exact verified fingerprint and before/after evidence proves reviewers stayed read-only.
|