feat(skills): enhance Code Review skill with 2-stage review and MAXSIM integration#13
feat(skills): enhance Code Review skill with 2-stage review and MAXSIM integration#13maystudios wants to merge 1 commit intomainfrom
Conversation
…M integration Add explicit 2-stage review protocol (Stage 1: spec compliance, Stage 2: code quality), context: fork frontmatter, and deeper MAXSIM integration with context loading, STATE.md hooks, artifact references, and push-back protocol. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the code-review skill template to enforce a stricter review flow and better integrate with MAXSIM plan execution artifacts.
Changes:
- Introduces a mandatory two-stage review protocol (Stage 1: spec compliance; Stage 2: code quality).
- Adds
context: forkfrontmatter for skill isolation. - Expands MAXSIM integration guidance (context loading, STATE.md hooks, artifact references, push-back protocol).
Comments suppressed due to low confidence (1)
templates/skills/code-review/SKILL.md:38
- The new Stage 1/Stage 2 protocol introduces an explicit spec-compliance verdict, but the review summary/output format later in this skill does not include any field/line for Stage 1 (e.g., SPEC COMPLIANCE: PASS|GAPS). As written, reviewers following the required protocol have no standardized place to record Stage 1 results; update the output format (or this section) so Stage 1 is captured consistently before Stage 2 findings.
## Two-Stage Review Protocol
Code review runs in two mandatory stages, in order:
### Stage 1: Spec Compliance Review
Verify the implementation matches what was planned:
- Does the code implement every task in the plan's `<done>` criteria?
- Are there changes NOT covered by the plan (scope creep)?
- Do the public interfaces match the plan's specifications?
- Are there missing pieces the plan required but implementation skipped?
**Stage 1 must PASS before proceeding to Stage 2.** Spec non-compliance is a blocker — the code does the wrong thing well.
### Stage 2: Code Quality Review
With spec compliance confirmed, review the implementation quality using the gate function steps below (SCOPE → SECURITY → INTERFACES → ERROR HANDLING → TESTS → QUALITY).
**Both stages must pass for sign-off.**
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| When reviewing within a MAXSIM project, load project context: | ||
|
|
||
| ```bash | ||
| node ~/.claude/maxsim/bin/maxsim-tools.cjs skill-context code-review | ||
| ``` |
There was a problem hiding this comment.
The suggested context-loading command node ~/.claude/maxsim/bin/maxsim-tools.cjs skill-context code-review appears to reference a skill-context subcommand that does not exist in maxsim-tools.cjs (no such command is registered in the CLI). Replace this with a real command sequence (e.g., using state-snapshot, find-phase, and/or roadmap commands) or document this as pseudocode if it’s not meant to be executed.
| For spec compliance review, load the plan: | ||
| 1. Read `.planning/phases/{current}/PLAN.md` — this is the spec | ||
| 2. For each task in the plan, check if the implementation satisfies its `<done>` criteria | ||
| 3. Flag any implementation that goes beyond or falls short of the plan | ||
| 4. Produce a Stage 1 verdict: COMPLIANT or NON-COMPLIANT (with list of gaps) | ||
|
|
||
| ### Stage 2 Integration | ||
|
|
||
| For code quality review, use the gate function steps (SCOPE through QUALITY) as defined above. | ||
|
|
||
| ### STATE.md Hooks | ||
|
|
||
| Record review verdicts as decisions: | ||
| - After Stage 1: record compliance verdict with any gaps found | ||
| - After Stage 2: record quality verdict with any blocking issues | ||
| - Review metrics feed into performance tracking (pass rate, common issues) | ||
|
|
||
| ### Artifact References | ||
|
|
||
| - Load plan from `.planning/phases/{current}/PLAN.md` for spec compliance | ||
| - Review summary included in `.planning/phases/{current}/SUMMARY.md` | ||
| - Blocking issues that require architectural decisions get recorded as STATE.md blockers | ||
| - Medium issues filed as `.planning/todos/` entries for follow-up |
There was a problem hiding this comment.
The file paths in Stage 1 / Artifact References don’t match the repo’s established planning layout. Phase plans are written as .planning/phases/XX-name/{phase}-{plan}-PLAN.md (or *-PLAN.md), and summaries as {phase}-{plan}-SUMMARY.md, not .planning/phases/{current}/PLAN.md and SUMMARY.md. Update these references so reviewers can locate the correct plan/summary files reliably.
Summary
context: forkfrontmatter for skill isolationTest plan
npx vitest run— 54 tests, 2 files)npm run build)🤖 Generated with Claude Code