feat(code-review): v0.1.0 — two-axis review (Standards/Spec parallel clean subagents) + Fowler smell baseline (mattpocock) × caveman-review output format
This commit is contained in:
@@ -118,6 +118,7 @@ an explicit `adapted-from` marker in its frontmatter.
|
|||||||
| `diagnosing-bugs` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) + superpowers 6.2.0 concepts (Iron Law, red flags) |
|
| `diagnosing-bugs` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) + superpowers 6.2.0 concepts (Iron Law, red flags) |
|
||||||
| `loop-me` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) — workflow-spec design gate |
|
| `loop-me` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) — workflow-spec design gate |
|
||||||
| `review-kit-pi-method` | `author: ours` — pi-native spawn for clean-context review subagents |
|
| `review-kit-pi-method` | `author: ours` — pi-native spawn for clean-context review subagents |
|
||||||
|
| `code-review` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) — two-axis + Fowler baseline; output: caveman-review format |
|
||||||
| `writing-skills` | `adapted-from: obra/superpowers @ 6.2.0` (MIT) — TDD-for-skills core + ideya 8 self-skill-authoring |
|
| `writing-skills` | `adapted-from: obra/superpowers @ 6.2.0` (MIT) — TDD-for-skills core + ideya 8 self-skill-authoring |
|
||||||
| all other `skills/*` | `author: ours` |
|
| all other `skills/*` | `author: ours` |
|
||||||
|
|
||||||
|
|||||||
@@ -88,6 +88,7 @@ bash scripts/build.sh caveman # один
|
|||||||
| `diagnosing-bugs` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) + superpowers 6.2.0 (Iron Law, red flags) |
|
| `diagnosing-bugs` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) + superpowers 6.2.0 (Iron Law, red flags) |
|
||||||
| `loop-me` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) — дизайн-гейт workflow-спец |
|
| `loop-me` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) — дизайн-гейт workflow-спец |
|
||||||
| `review-kit-pi-method` | `author: ours` — pi-спавн чистых review-субагентов |
|
| `review-kit-pi-method` | `author: ours` — pi-спавн чистых review-субагентов |
|
||||||
|
| `code-review` | `adapted-from: mattpocock/skills @ 84fdeffd` (MIT) — двухосевость + Fowler-база; формат вывода: caveman-review |
|
||||||
| `writing-skills` | `adapted-from: obra/superpowers @ 6.2.0` (MIT) — TDD-for-skills ядро + идея 8 self-skill-authoring |
|
| `writing-skills` | `adapted-from: obra/superpowers @ 6.2.0` (MIT) — TDD-for-skills ядро + идея 8 self-skill-authoring |
|
||||||
| остальные `skills/*` | `author: ours` |
|
| остальные `skills/*` | `author: ours` |
|
||||||
|
|
||||||
|
|||||||
BIN
dist/code-review.skill
vendored
Normal file
BIN
dist/code-review.skill
vendored
Normal file
Binary file not shown.
175
skills/code-review/SKILL.md
Normal file
175
skills/code-review/SKILL.md
Normal file
@@ -0,0 +1,175 @@
|
|||||||
|
---
|
||||||
|
name: code-review
|
||||||
|
adapted-from: "mattpocock/skills @ 84fdeffd12f2ee307994d1eb6feb48173b6e0502 (MIT) — two-axis structure + Fowler baseline; format: caveman-review"
|
||||||
|
version: 0.1.0
|
||||||
|
description: >
|
||||||
|
Two-axis review of a diff since a fixed point (commit, branch, tag,
|
||||||
|
merge-base) — Standards (does the code follow the repo's documented
|
||||||
|
standards + Fowler smell baseline?) and Spec (does it implement what the
|
||||||
|
originating issue/spec asked for?). Both axes run as parallel clean-context
|
||||||
|
sub-agents (review-kit-pi-method spawn) and are reported side by side.
|
||||||
|
Findings formatted caveman-style (L:line, severity prefix). Use when the
|
||||||
|
user says «отревьюй изменения», «ревью с коммита X», "review since X",
|
||||||
|
«проверь диф против спеки», "review against spec", "review this branch",
|
||||||
|
«ревью за период». Differs from caveman-review: that is the one-liner FORMAT
|
||||||
|
for quick PR comments ("review this PR", "review the diff", "code review" →
|
||||||
|
caveman-review); this is the full two-axis METHOD.
|
||||||
|
---
|
||||||
|
|
||||||
|
# Code review (two-axis)
|
||||||
|
|
||||||
|
Review the diff between `HEAD` and a fixed point the user supplies, along two
|
||||||
|
independent axes:
|
||||||
|
|
||||||
|
- **Standards** — does the code conform to this repo's documented coding
|
||||||
|
standards (+ the Fowler smell baseline below)?
|
||||||
|
- **Spec** — does the code faithfully implement the originating issue / spec?
|
||||||
|
|
||||||
|
Both axes run as **parallel clean-context sub-agents** (see
|
||||||
|
review-kit-pi-method for the spawn) so they don't pollute each other's
|
||||||
|
context, then this skill aggregates their findings.
|
||||||
|
|
||||||
|
Findings are formatted **caveman-style** (`L<line>: <severity> <problem>. <fix>.`
|
||||||
|
— see caveman-review for the format and severity prefixes).
|
||||||
|
|
||||||
|
## Why two axes
|
||||||
|
|
||||||
|
A change can pass one axis and fail the other:
|
||||||
|
|
||||||
|
- Code that follows every standard but implements the wrong thing →
|
||||||
|
**Standards pass, Spec fail.**
|
||||||
|
- Code that does exactly what the issue asked but breaks the project's
|
||||||
|
conventions → **Spec pass, Standards fail.**
|
||||||
|
|
||||||
|
Reporting them separately stops one axis from masking the other.
|
||||||
|
|
||||||
|
## Process
|
||||||
|
|
||||||
|
### 1. Pin the fixed point
|
||||||
|
|
||||||
|
Whatever the user said — a commit SHA, branch name, tag, `main`, `HEAD~5`,
|
||||||
|
etc. If they didn't specify one, ask for it.
|
||||||
|
|
||||||
|
Capture the diff command once: `git diff <fixed-point>...HEAD` (three-dot, so
|
||||||
|
the comparison is against the merge-base). Also note the commit list:
|
||||||
|
`git log <fixed-point>..HEAD --oneline`.
|
||||||
|
|
||||||
|
Before going further, confirm the fixed point resolves (`git rev-parse
|
||||||
|
<fixed-point>`) and the diff is non-empty. A bad ref or empty diff should fail
|
||||||
|
here — not inside two parallel sub-agents.
|
||||||
|
|
||||||
|
### 2. Identify the spec source
|
||||||
|
|
||||||
|
Look for the originating spec, in this order:
|
||||||
|
|
||||||
|
1. Issue/task references in the commit messages (`#123`, `Closes #45`,
|
||||||
|
`slug` references to `.tasks/`) — check `.tasks/` for the task block.
|
||||||
|
2. A path the user passed as an argument.
|
||||||
|
3. A spec under `.wiki/concepts/`, `.brainstorm/`, `docs/`, or `specs/`
|
||||||
|
matching the branch name or feature.
|
||||||
|
4. If nothing is found, ask the user where the spec is. If they say there
|
||||||
|
isn't one, the **Spec** sub-agent skips and reports "no spec available".
|
||||||
|
|
||||||
|
### 3. Identify the standards sources
|
||||||
|
|
||||||
|
Anything in the repo that documents how code should be written:
|
||||||
|
`CODING_STANDARDS.md`, `CONTRIBUTING.md`, the project's `.wiki/CLAUDE.md`
|
||||||
|
schema, CLAUDE.md conventions.
|
||||||
|
|
||||||
|
On top of whatever the repo documents, the Standards axis always carries the
|
||||||
|
**smell baseline** below — a fixed set of Fowler code smells (_Refactoring_,
|
||||||
|
ch.3) that applies even when a repo documents nothing. Two rules bind it:
|
||||||
|
|
||||||
|
- **The repo overrides.** A documented repo standard always wins; where it
|
||||||
|
endorses something the baseline would flag, suppress the smell.
|
||||||
|
- **Always a judgement call.** Each smell is a labelled heuristic ("possible
|
||||||
|
Feature Envy"), never a hard violation — and, like any standard here, skip
|
||||||
|
anything tooling already enforces.
|
||||||
|
|
||||||
|
Each smell reads *what it is* → *how to fix*; match it against the diff:
|
||||||
|
|
||||||
|
- **Mysterious Name** — a function, variable, or type whose name doesn't
|
||||||
|
reveal what it does or holds. → rename it; if no honest name comes, the
|
||||||
|
design's murky.
|
||||||
|
- **Duplicated Code** — the same logic shape appears in more than one hunk or
|
||||||
|
file in the change. → extract the shared shape, call it from both.
|
||||||
|
- **Feature Envy** — a method that reaches into another object's data more
|
||||||
|
than its own. → move the method onto the data it envies.
|
||||||
|
- **Data Clumps** — the same few fields or params keep travelling together (a
|
||||||
|
type wanting to be born). → bundle them into one type, pass that.
|
||||||
|
- **Primitive Obsession** — a primitive or string standing in for a domain
|
||||||
|
concept that deserves its own type. → give the concept its own small type.
|
||||||
|
- **Repeated Switches** — the same `switch`/`if`-cascade on the same type
|
||||||
|
recurs across the change. → replace with polymorphism, or one map both sites
|
||||||
|
share.
|
||||||
|
- **Shotgun Surgery** — one logical change forces scattered edits across many
|
||||||
|
files in the diff. → gather what changes together into one module.
|
||||||
|
- **Divergent Change** — one file or module is edited for several unrelated
|
||||||
|
reasons. → split so each module changes for one reason.
|
||||||
|
- **Speculative Generality** — abstraction, parameters, or hooks added for
|
||||||
|
needs the spec doesn't have. → delete it; inline back until a real need
|
||||||
|
shows.
|
||||||
|
- **Message Chains** — long `a.b().c().d()` navigation the caller shouldn't
|
||||||
|
depend on. → hide the walk behind one method on the first object.
|
||||||
|
- **Middle Man** — a class or function that mostly just delegates onward. →
|
||||||
|
cut it, call the real target direct.
|
||||||
|
- **Refused Bequest** — a subclass or implementer that ignores or overrides
|
||||||
|
most of what it inherits. → drop the inheritance, use composition.
|
||||||
|
|
||||||
|
### 4. Spawn both sub-agents in parallel
|
||||||
|
|
||||||
|
Per review-kit-pi-method: parallel runs, one question each, clean spawn.
|
||||||
|
|
||||||
|
**Standards sub-agent prompt** — include:
|
||||||
|
|
||||||
|
- The full diff command and commit list.
|
||||||
|
- The list of standards-source files found in step 3, **plus the smell
|
||||||
|
baseline from step 3 pasted in full** — the sub-agent has no other access
|
||||||
|
to it.
|
||||||
|
- The brief: "Report — per file/hunk where relevant — (a) every place the
|
||||||
|
diff violates a documented standard: cite the standard (file + the rule);
|
||||||
|
and (b) any baseline smell you spot: name it and quote the hunk. Distinguish
|
||||||
|
hard violations from judgement calls — documented-standard breaches can be
|
||||||
|
hard, but baseline smells are always judgement calls, and a documented repo
|
||||||
|
standard overrides the baseline. Skip anything tooling enforces."
|
||||||
|
|
||||||
|
**Spec sub-agent prompt** — include:
|
||||||
|
|
||||||
|
- The diff command and commit list.
|
||||||
|
- The path or fetched contents of the spec.
|
||||||
|
- The brief: "Report: (a) requirements the spec asked for that are missing or
|
||||||
|
partial; (b) behaviour in the diff that wasn't asked for (scope creep);
|
||||||
|
(c) requirements that look implemented but where the implementation looks
|
||||||
|
wrong. Quote the spec line for each finding."
|
||||||
|
|
||||||
|
If the spec is missing, skip the Spec sub-agent and note this in the final
|
||||||
|
report.
|
||||||
|
|
||||||
|
### 5. Aggregate
|
||||||
|
|
||||||
|
Present the two reports under `## Standards` and `## Spec` headings, verbatim
|
||||||
|
or lightly cleaned. Do **not** merge or rerank findings — the two axes are
|
||||||
|
deliberately separate (see *Why two axes*).
|
||||||
|
|
||||||
|
Format every finding caveman-style: `L<line>: <severity> <problem>. <fix>.`
|
||||||
|
(severity: 🔴 bug / 🟡 risk / 🔵 nit / ❓ q). One line per finding, exact
|
||||||
|
line numbers, exact symbol names, concrete fix.
|
||||||
|
|
||||||
|
End with a one-line summary: total findings per axis, and the worst issue
|
||||||
|
_within each axis_ (if any). Don't pick a single winner across axes — that's
|
||||||
|
the reranking the separation exists to prevent.
|
||||||
|
|
||||||
|
## Cross-agent applicability
|
||||||
|
|
||||||
|
Pure methodology — the spawn command is pi-specific (review-kit-pi-method);
|
||||||
|
on any runtime that can spawn a fresh-context subprocess, use its equivalent.
|
||||||
|
The two-axis structure and Fowler baseline transfer unchanged.
|
||||||
|
|
||||||
|
## Out of scope
|
||||||
|
|
||||||
|
- Does NOT define the output format beyond the caveman one-liner convention
|
||||||
|
(see caveman-review for full format rules).
|
||||||
|
- Does NOT fix the code — reviews only.
|
||||||
|
- Does NOT approve/request-changes formally — presents findings.
|
||||||
|
- Does NOT run linters/builds — tooling-enforced checks are skipped per the
|
||||||
|
Standards axis rule.
|
||||||
Reference in New Issue
Block a user