conductor(track+plan): superpowers review section 11 — requesting-code-review (brief)
This commit is contained in:
@@ -659,7 +659,67 @@ The project follows the discipline implicitly via its neutral communication styl
|
||||
|
||||
## 11. Requesting Code Review
|
||||
|
||||
<!-- Section 11 brief (50-150 LOC). Skill: requesting-code-review. Verdict pending. -->
|
||||
## 11. Requesting Code Review
|
||||
|
||||
### 11.1 What the skill prescribes
|
||||
|
||||
The `requesting-code-review` skill is the symmetric counterpart to `receiving-code-review`. Core principle: "Review early, review often." Mandatory review triggers: after each task in subagent-driven development; after completing major feature; before merge to main. Optional but valuable: when stuck (fresh perspective); before refactoring (baseline check); after fixing complex bug.
|
||||
|
||||
How to Request: (1) Get git SHAs (`BASE_SHA=$(git rev-parse HEAD~1)`; `HEAD_SHA=$(git rev-parse HEAD)`); (2) Dispatch code reviewer subagent using Task tool with `general-purpose` type, fill template at `code-reviewer.md`. Placeholders: DESCRIPTION (brief summary of what was built); PLAN_OR_REQUIREMENTS (what it should do); BASE_SHA (starting commit); HEAD_SHA (ending commit). (3) Act on feedback: Fix Critical issues immediately; Fix Important issues before proceeding; Note Minor issues for later; Push back if reviewer is wrong (with reasoning).
|
||||
|
||||
Integration with Workflows: Subagent-Driven Development (review after EACH task; catch issues before they compound; fix before moving to next task); Executing Plans (review after each task or at natural checkpoints); Ad-Hoc Development (review before merge; review when stuck).
|
||||
|
||||
Red Flags: Never skip review because "it's simple"; never ignore Critical issues; never proceed with unfixed Important issues; never argue with valid technical feedback.
|
||||
|
||||
### 11.2 Mapping to the project's existing pattern
|
||||
|
||||
The project doesn't have a dedicated "requesting code review" pattern in the meta-tooling domain. The Application domain has the live_gui fixture for visual verification (per `docs/guide_testing.md`) and the Phase Completion Verification for automated checks. The "review" is implicit in the per-task git notes + phase checkpoints.
|
||||
|
||||
| Skill rule | Project equivalent | Where |
|
||||
|---|---|---|
|
||||
| "Review after each task in subagent-driven development" | Per-task atomic commit + git note is the project's "review" artifact; the user can review at phase boundaries | `conductor/workflow.md` §"Task Workflow" step 9-10 |
|
||||
| "Get git SHAs (BASE_SHA + HEAD_SHA)" | The per-task commit + git note has the SHA recorded in `state.toml` (`commit_sha` field per task) | `conductor/tracks/<id>/state.toml` |
|
||||
| "Dispatch code reviewer subagent" | No explicit code-reviewer subagent; the Tier-4 QA agent is for error analysis (not success claims). The user is the implicit reviewer. | `conductor/workflow.md` §"Conductor Token Firewalling" |
|
||||
| "Fix Critical issues immediately" | The Phase Completion Verification §5: "If verification failed, the agent will halt the workflow, present the detailed failure logs to the user, and await further instructions for debugging or remediation." | `conductor/workflow.md` |
|
||||
| "Fix Important issues before proceeding" | The Pause-and-await-user-response pattern (per Phase Completion Verification §5) | `conductor/workflow.md` |
|
||||
| "Note Minor issues for later" | The "deferred_to_followup_tracks" field in metadata.json is the project's pattern for "note for later" | `conductor/tracks/<id>/metadata.json` |
|
||||
| "Ad-Hoc Development: review before merge" | The project doesn't use merge workflow (works on master directly per Section 8); "review before merge" doesn't apply | — |
|
||||
| "Review when stuck" | The Deduction Loop rule: "If you've tried 3 times and the test still fails, STOP and report to the user" — the user provides fresh perspective | `AGENTS.md` §"Process Anti-Patterns" §1 |
|
||||
| "Before refactoring (baseline check)" | The "feature_bleed_cleanup_20260302" track was a baseline check before refactoring; "before refactoring" is the project's pattern for `cruft_elimination` style tracks | `conductor/archive/feature_bleed_cleanup_20260302/` |
|
||||
| "Never skip review because 'it's simple'" | The per-task atomic commit + git note rule applies even for "simple" changes; no skip rule | `conductor/workflow.md` |
|
||||
|
||||
### 11.3 Where the project already follows the discipline
|
||||
|
||||
- **The per-task atomic commit + git note discipline** is the project's "review" artifact. The user can review at any time by reading the git log + git notes; the project's `state.toml` records each task's commit_sha.
|
||||
- **The `deferred_to_followup_tracks` field in metadata.json** is the project's "Note Minor issues for later" pattern. The 5 sub-tracks of the result_migration campaign are noted this way.
|
||||
- **The Phase Completion Verification "PAUSE and await the user's response" rule** is the project's "Fix Critical issues immediately + Fix Important issues before proceeding" pattern.
|
||||
- **The Tier-4 QA agent (per Section 7)** is the project's closest equivalent to a code reviewer subagent, but it's for error analysis, not for success claims or style review.
|
||||
|
||||
### 11.4 Where the project doesn't follow the discipline
|
||||
|
||||
- **No dedicated "code reviewer" subagent.** The project uses tier-4 QA for error analysis and the user for review. A code-quality reviewer (style, naming, structure) doesn't exist as a tier-4b.
|
||||
- **No "Ad-Hoc Development: review before merge"** because the project doesn't use merge workflow.
|
||||
- **No explicit "Minor issues for later" deferral mechanism beyond metadata.json's `deferred_to_followup_tracks` field.** Other categories of "minor" (style nits, naming) aren't tracked.
|
||||
|
||||
### 11.5 Recommendations summary
|
||||
|
||||
The project has the discipline's core elements (per-task atomic commits, git notes, user-as-reviewer, phase checkpoints, deferred_to_followup_tracks). The deferred rebuild may want to:
|
||||
|
||||
- **MEDIUM:** Consider a tier-4b "code quality reviewer" subagent for style/naming review (per Section 7 recommendation).
|
||||
- **LOW:** Add a `conductor/style_nits.md` (or similar) pattern for tracking minor style issues. Currently they're absorbed into the per-commit git history.
|
||||
|
||||
**Verdict.**
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| **Primary** | `PARITY` |
|
||||
| **Integration tag** | `INTEGRATED` |
|
||||
| **Section size** | brief |
|
||||
| **Cross-refs** | nagent_review_20260608 §10 (project's deferred_to_followup pattern); fable_review_20260617 §13 (project's "Genuinely Useful Patterns"); intent_dsl_survey_20260612 §6 |
|
||||
|
||||
**Rationale.** The project's per-task atomic commit + git note + state.toml SHA + Phase Completion Verification + deferred_to_followup_tracks is a faithful re-encoding of the skill's review request discipline. The "code reviewer subagent" pattern is replaced by the user-as-reviewer model (per Section 7's MMA architecture).
|
||||
|
||||
**Recommended change.** *(blank — no rebuild action. The project's user-as-reviewer model is intentional.)*
|
||||
|
||||
## 12. Finishing a Development Branch
|
||||
|
||||
|
||||
@@ -53,7 +53,7 @@ t3_4 = { status = "completed", commit_sha = "2ef7f4d6", description = "Write Sec
|
||||
|
||||
# Phase 4 tasks (Sections 9-14)
|
||||
t4_1 = { status = "completed", commit_sha = "32610beb", description = "Write Section 9 (dispatching-parallel-agents, brief). Commit." }
|
||||
t4_2 = { status = "pending", commit_sha = "", description = "Write Section 10 (receiving-code-review, medium). Commit." }
|
||||
t4_2 = { status = "completed", commit_sha = "5078d4e7", description = "Write Section 10 (receiving-code-review, medium). Commit." }
|
||||
t4_3 = { status = "pending", commit_sha = "", description = "Write Section 11 (requesting-code-review, brief). Commit." }
|
||||
t4_4 = { status = "pending", commit_sha = "", description = "Write Section 12 (finishing-a-development-branch, brief). Commit." }
|
||||
t4_5 = { status = "pending", commit_sha = "", description = "Write Section 13 (using-git-worktrees, brief). Commit." }
|
||||
|
||||
Reference in New Issue
Block a user