From 111c4f550b2615f5868a0c0fdb2e5d4c4f23cb1b Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sun, 5 Jul 2026 13:29:11 -0400 Subject: [PATCH] =?UTF-8?q?conductor(track+plan):=20superpowers=20review?= =?UTF-8?q?=20section=2011=20=E2=80=94=20requesting-code-review=20(brief)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../superpowers_review_20260619/report.md | 62 ++++++++++++++++++- .../superpowers_review_20260619/state.toml | 2 +- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/conductor/tracks/superpowers_review_20260619/report.md b/conductor/tracks/superpowers_review_20260619/report.md index ef9dbaa8..e491d7cd 100644 --- a/conductor/tracks/superpowers_review_20260619/report.md +++ b/conductor/tracks/superpowers_review_20260619/report.md @@ -659,7 +659,67 @@ The project follows the discipline implicitly via its neutral communication styl ## 11. Requesting Code Review - +## 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//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//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 diff --git a/conductor/tracks/superpowers_review_20260619/state.toml b/conductor/tracks/superpowers_review_20260619/state.toml index 06937e7b..8527d563 100644 --- a/conductor/tracks/superpowers_review_20260619/state.toml +++ b/conductor/tracks/superpowers_review_20260619/state.toml @@ -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." }