jb/harness-tools
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make development workflow risk-based and verification proportionate

Browse Source
This commit is contained in:
jb
2026-10-07 17:43:19 -04:00
parent 284fdd638d
commit 0cc0e82bbc
13 changed files with 265 additions and 173 deletions
Download Patch File Download Diff File Expand all files Collapse all files
skills/develop/code-review/SKILL.md
+32 -51
View File
@@ -1,83 +1,64 @@
---
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.
description: Independently review changes against repository standards and the requested behavior using a pinned Git diff. Use for branch, PR, work-in-progress, or workflow-record review, with targeted follow-up after fixes.
---
# Code review
Run two independent, read-only reviews over one pinned diff:
Use one independent, read-only reviewer by default. Assess two axes and report them separately:
- **Standards:** repository instructions and documented conventions.
- **Specification:** the originating issue, spec, or approved workflow record.
- **Specification:** the originating request, issue, spec, or workflow record.
Keep the axes separate so conformity cannot hide incorrect scope and correct scope cannot hide standards violations.
Add specialist reviewers when distinct risk or repository policy warrants them, not merely because there are two axes. The agent that authored or mutated the diff cannot review it or its own remediation.
## 1. Pin the review bytes
Require a fixed point; ask when none was supplied. Resolve and record:
Resolve a fixed point from the caller's brief or workflow record; ask only when the intended scope cannot be established. 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> --`.
- review base and immutable product HEAD commits, never mutable `HEAD` in the brief;
- clean status, product tree hash, and `git log <base>..<product-head> --oneline`;
- exact command `git diff --binary --full-index --find-renames <base>...<product-head> --`;
- SHA-256 of those exact bytes, generated by the parent.
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.
Stop on a bad ref, dirty product state, or empty diff unless an empty comparison was expected. During a development workflow, review after focused checks and before the final full suite so fixes do not cause scheduled duplicate suite runs. Independent review does not establish test success.
## 2. Resolve specification and standards
Accept the first available specification source:
Use the caller's explicit request, supplied spec, or retained workflow record as the primary specification. Otherwise inspect linked issues (using repository issue-tracker instructions when configured) or matching repository docs. Ask only when the intended behavior remains unclear. A workflow record needs no issue-tracker configuration or ceremonial plan approval to be a valid specification.
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.
If the user explicitly requests standards-only review without a specification, report “specification not assessed”; do not claim both axes passed.
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. Where repository standards do not override it, consider this judgment-based baseline and skip tooling-enforced issues:
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:
- Names hide purpose; duplicated logic obscures a shared contract.
- Feature envy, data clumps, primitive obsession, or repeated switches complicate change.
- Shotgun surgery or divergent responsibilities scatter one change across unrelated areas.
- Speculative generality, middlemen, message chains, or inappropriate inheritance add indirection without a current need.
- **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.
Distinguish consequential defects and documented violations from optional style preferences. Do not require speculative abstractions, compatibility paths, or exhaustive tests without a concrete contract or risk. Judge coverage against changed behavior and credible failures.
## 3. Enforce reviewer independence
## 3. Enforce independence and read-only review
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.
List every author/mutator by display name and stable child/session identity when exposed. Select a reviewer outside that set with repository read access and Git-capable `bash`, but no declared `write`, `edit`, or nested-agent tools. When identity cannot be distinguished, exclude the ambiguous profile/run rather than assuming independence.
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.
Capture branch, HEAD, status, tree hash, and changed paths before review. Launch the reviewer directly with the pinned commits, exact diff command and digest, specification, standards, mutator exclusions, and identity-reporting requirement. Require the reviewer to:
Require each reviewer to:
1. Regenerate the exact diff and confirm its SHA-256 before analysis.
2. Remain read-only, run no mutating commands, and perform no remediation.
3. Assess both axes, grounding findings in file/hunk evidence and severity; quote the requirement for specification findings.
4. Report identity when exposed and stay under 400 words unless complexity warrants more.
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.
After delivery, verify Git state is unchanged. Missing output, digest mismatch, mutator overlap, or reviewer mutation means no valid verdict. Mutation also makes that reviewer ineligible for subsequent review of the diff.
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.
## 4. Report findings and remediate
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.
Present `## Standards` and `## Specification`, separating documented violations from judgment-based concerns. Preserve each axis's findings and severity; report approval explicitly when no material findings remain. Keep optional suggestions non-blocking unless tied to a requirement or concrete risk.
## 4. Aggregate without reranking
Validate findings against repository evidence. An original implementer or another non-reviewer makes accepted fixes, then runs affected checks. Use [verification policy](../develop-workflow/references/verification.md) for evidence reuse and final-suite timing; obey stricter repository requirements.
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.
For bounded corrections, resume the same independent reviewer with the previous verdict, old and new pinned commits/digests, fix delta, and check results. Require it to inspect fixes and affected interactions, confirm the new full-diff fingerprint, and state which prior conclusions remain applicable. A prior verdict alone never approves later product changes.
Bind approval to the base, product HEAD, clean status, tree hash, and diff digest.
Request a broader fresh review when scope, architecture, shared behavior, or uncertain interactions invalidate previous conclusions; use a new specialist identity only when useful or required. Ask the user only for changed requirements, approval-boundary risk, or an unresolved decision, not because an automatic remediation cycle count expired. Stop repetitive ineffective retries, reassess the cause, and surface blockers honestly.
## 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.
**Complete when:** independent review covers the final pinned product diff, applicable axes have no unresolved material findings, and before/after evidence proves the reviewer stayed read-only. Final test completion remains a separate integration requirement.