refactor: split review responsibilities — code review in lite-execute, convergence review in lite-test-review

- lite-plan LP-Phase 4: split single "Review" into two selections (Code Review + Convergence Review) - lite-execute: add Step 4 Code Review (agent/codex/gemini) with code-review.md artifact, Step 5 passes convergenceReviewTool - lite-test-review: rename reviewTool → convergenceReviewTool, TR-Phase 2 focused on convergence criteria verification - All autoYes paths default both reviews to Skip - Data structures updated across all three files for consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-03-22 19:18:47 +08:00 · 2026-03-21 17:18:20 +08:00
parent fab07c2e97
commit fcd0b9a2c4
3 changed files with 172 additions and 51 deletions
--- a/.claude/skills/workflow-lite-execute/SKILL.md
+++ b/.claude/skills/workflow-lite-execute/SKILL.md
@@ -74,7 +74,7 @@ function selectExecutionOptions() {
  const autoYes = workflowPreferences?.autoYes ?? false

  if (autoYes) {
-    return { execution_method: "Auto", code_review_tool: "Skip" }
+    return { execution_method: "Auto", code_review_tool: "Skip", convergence_review_tool: "Skip" }
  }

  return AskUserQuestion({
@@ -90,14 +90,25 @@ function selectExecutionOptions() {
        ]
      },
      {
-        question: "Review tool for test-review phase?",
-        header: "Review Tool (passed to lite-test-review)",
+        question: "Code review after execution? (runs here in lite-execute)",
+        header: "Code Review",
        multiSelect: false,
        options: [
-          { label: "Agent Review", description: "Agent review in test-review (default)" },
-          { label: "Gemini Review", description: "Gemini CLI in test-review" },
-          { label: "Codex Review", description: "Codex CLI in test-review" },
-          { label: "Skip", description: "Skip review in test-review" }
+          { label: "Gemini Review", description: "Gemini CLI: git diff quality review" },
+          { label: "Codex Review", description: "Codex CLI: git-aware code review (--mode review)" },
+          { label: "Agent Review", description: "@code-reviewer agent" },
+          { label: "Skip", description: "No code review" }
+        ]
+      },
+      {
+        question: "Convergence review in test-review phase?",
+        header: "Convergence Review",
+        multiSelect: false,
+        options: [
+          { label: "Agent", description: "Agent: verify convergence criteria" },
+          { label: "Gemini", description: "Gemini CLI: convergence verification" },
+          { label: "Codex", description: "Codex CLI: convergence verification" },
+          { label: "Skip", description: "Skip convergence review, run tests only" }
        ]
      }
    ]
@@ -117,7 +128,8 @@ if (executionContext) {
  console.log(`
  Execution Strategy (from lite-plan):
   Method: ${executionContext.executionMethod}
-   Review: ${executionContext.codeReviewTool}
+   Code Review: ${executionContext.codeReviewTool}
+   Convergence Review: ${executionContext.convergenceReviewTool}
   Tasks: ${getTasks(executionContext.planObject).length}
   Complexity: ${executionContext.planObject.complexity}
 ${executionContext.executorAssignments ? `   Assignments: ${JSON.stringify(executionContext.executorAssignments)}` : ''}
@@ -367,18 +379,97 @@ ${(t.test?.success_metrics || []).length > 0 ? `**Success metrics**: ${t.test.su
 }
 ```

-### Step 4: Chain to Test Review & Post-Completion
+### Step 4: Code Review

-> **Note**: Spec sync (session:sync) is handled by lite-test-review's TR-Phase 5, not here. This avoids duplicate sync and ensures test fix changes are also captured.
+**Skip if**: `codeReviewTool === 'Skip'`

-**Map review tool**: Convert lite-execute's `codeReviewTool` to test-review tool name.
+**Resolve review tool**: From `executionContext.codeReviewTool` (Mode 1) or `userSelection.code_review_tool` (Mode 2/3).

 ```javascript
-function mapReviewTool(codeReviewTool) {
+const codeReviewTool = executionContext?.codeReviewTool || userSelection?.code_review_tool || 'Skip'
+const resolvedTool = (() => {
  if (!codeReviewTool || codeReviewTool === 'Skip') return 'skip'
  if (/gemini/i.test(codeReviewTool)) return 'gemini'
  if (/codex/i.test(codeReviewTool)) return 'codex'
  return 'agent'
+})()
+
+if (resolvedTool === 'skip') {
+  console.log('[Code Review] Skipped')
+} else {
+  // proceed with review
+}
+```
+
+**Agent Code Review** (resolvedTool === 'agent'):
+
+```javascript
+Agent({
+  subagent_type: "code-reviewer",
+  run_in_background: false,
+  description: `Code review: ${planObject.summary}`,
+  prompt: `## Code Review — Post-Execution Quality Check
+
+**Goal**: ${originalUserInput}
+**Plan Summary**: ${planObject.summary}
+
+### Changed Files
+Run \`git diff --name-only HEAD~${getTasks(planObject).length}..HEAD\` to identify changes.
+
+### Review Focus
+1. **Code quality**: Readability, naming, structure, dead code
+2. **Correctness**: Logic errors, off-by-one, null handling, edge cases
+3. **Patterns**: Consistency with existing codebase conventions
+4. **Security**: Injection, XSS, auth bypass, secrets exposure
+5. **Performance**: Unnecessary loops, N+1 queries, missing indexes
+
+### Instructions
+1. Run git diff to see actual changes
+2. Read changed files for full context
+3. For each issue found: severity (Critical/High/Medium/Low) + file:line + description + fix suggestion
+4. Return structured review: issues[], summary, overall verdict (PASS/WARN/FAIL)`
+})
+```
+
+**CLI Code Review — Codex** (resolvedTool === 'codex'):
+
+```javascript
+const reviewId = `${sessionId}-code-review`
+Bash(`ccw cli -p "Review code changes for quality, correctness, security, and pattern compliance. Focus: ${planObject.summary}" --tool codex --mode review --id ${reviewId}`, { run_in_background: true })
+// STOP - wait for hook callback
+```
+
+**CLI Code Review — Gemini** (resolvedTool === 'gemini'):
+
+```javascript
+const reviewId = `${sessionId}-code-review`
+Bash(`ccw cli -p "PURPOSE: Post-execution code quality review for: ${planObject.summary}
+TASK: • Run git diff to identify all changes • Review each changed file for quality, correctness, security • Check pattern compliance with existing codebase • Identify potential bugs, edge cases, performance issues
+MODE: analysis
+CONTEXT: @**/* | Memory: lite-execute completed, reviewing code quality
+EXPECTED: Per-file review with severity levels (Critical/High/Medium/Low), file:line references, fix suggestions, overall verdict
+CONSTRAINTS: Read-only | Focus on code quality not convergence" --tool gemini --mode analysis --rule analysis-review-code-quality --id ${reviewId}`, { run_in_background: true })
+// STOP - wait for hook callback
+```
+
+**Write review artifact** (if session folder exists):
+```javascript
+if (executionContext?.session?.folder) {
+  Write(`${executionContext.session.folder}/code-review.md`, codeReviewOutput)
+}
+```
+
+### Step 5: Chain to Test Review & Post-Completion
+
+**Resolve convergence review tool**: From `executionContext.convergenceReviewTool` (Mode 1) or `userSelection.convergence_review_tool` (Mode 2/3).
+
+```javascript
+function resolveConvergenceTool(ctx, selection) {
+  const raw = ctx?.convergenceReviewTool || selection?.convergence_review_tool || 'skip'
+  if (!raw || raw === 'Skip') return 'skip'
+  if (/gemini/i.test(raw)) return 'gemini'
+  if (/codex/i.test(raw)) return 'codex'
+  return 'agent'
 }
 ```

@@ -389,7 +480,7 @@ testReviewContext = {
  planObject: planObject,
  taskFiles: executionContext?.taskFiles
    || getTasks(planObject).map(t => ({ id: t.id, path: `${executionContext?.session?.folder}/.task/${t.id}.json` })),
-  reviewTool: mapReviewTool(executionContext?.codeReviewTool),
+  convergenceReviewTool: resolveConvergenceTool(executionContext, userSelection),
  executionResults: previousExecutionResults,
  originalUserInput: originalUserInput,
  session: executionContext?.session || {
@@ -442,7 +533,8 @@ Skill("lite-test-review")
  explorationManifest: {...} | null,
  clarificationContext: {...} | null,
  executionMethod: "Agent" | "Codex" | "Auto",
-  codeReviewTool: "Skip" | "Gemini Review" | "Agent Review" | string,
+  codeReviewTool: "Skip" | "Gemini Review" | "Codex Review" | "Agent Review",
+  convergenceReviewTool: "Skip" | "Agent" | "Gemini" | "Codex",
  originalUserInput: string,
  executorAssignments: {            // per-task override, priority over executionMethod
    [taskId]: { executor: "gemini" | "codex" | "agent", reason: string }
--- a/.claude/skills/workflow-lite-plan/SKILL.md
+++ b/.claude/skills/workflow-lite-plan/SKILL.md
@@ -492,8 +492,8 @@ ${tasks.map((t, i) => `${i+1}. ${t.title} (${t.scope || t.files?.[0]?.path || ''
 let userSelection

 if (workflowPreferences.autoYes) {
-  console.log(`[Auto] Allow & Execute | Auto | Skip`)
-  userSelection = { confirmation: "Allow", execution_method: "Auto", code_review_tool: "Skip" }
+  console.log(`[Auto] Allow & Execute | Auto | Skip + Skip`)
+  userSelection = { confirmation: "Allow", execution_method: "Auto", code_review_tool: "Skip", convergence_review_tool: "Skip" }
 } else {
  // "Other" in Execution allows specifying CLI tools from ~/.claude/cli-tools.json
  userSelection = AskUserQuestion({
@@ -519,14 +519,25 @@ if (workflowPreferences.autoYes) {
        ]
      },
      {
-        question: "Code review after execution?",
-        header: "Review",
+        question: "Code review after execution? (runs in lite-execute)",
+        header: "Code Review",
        multiSelect: false,
        options: [
-          { label: "Gemini Review", description: "Gemini CLI review" },
-          { label: "Codex Review", description: "Git-aware review (prompt OR --uncommitted)" },
+          { label: "Gemini Review", description: "Gemini CLI: git diff quality review" },
+          { label: "Codex Review", description: "Codex CLI: git-aware code review (--mode review)" },
          { label: "Agent Review", description: "@code-reviewer agent" },
-          { label: "Skip", description: "No review" }
+          { label: "Skip", description: "No code review" }
+        ]
+      },
+      {
+        question: "Convergence review in test-review phase?",
+        header: "Convergence Review",
+        multiSelect: false,
+        options: [
+          { label: "Agent", description: "Agent: verify convergence criteria against implementation" },
+          { label: "Gemini", description: "Gemini CLI: convergence verification" },
+          { label: "Codex", description: "Codex CLI: convergence verification" },
+          { label: "Skip", description: "Skip convergence review, run tests only" }
        ]
      }
    ]
@@ -534,7 +545,7 @@ if (workflowPreferences.autoYes) {
 }
 ```

-// TodoWrite: Phase 4 → completed `[${userSelection.execution_method} + ${userSelection.code_review_tool}]`, Phase 5 → in_progress
+// TodoWrite: Phase 4 → completed `[${userSelection.execution_method} | CR:${userSelection.code_review_tool} | CVR:${userSelection.convergence_review_tool}]`, Phase 5 → in_progress

 ## 10. LP-Phase 5: Handoff to Execution

@@ -561,6 +572,7 @@ executionContext = {
  clarificationContext: clarificationContext || null,
  executionMethod: userSelection.execution_method,
  codeReviewTool: userSelection.code_review_tool,
+  convergenceReviewTool: userSelection.convergence_review_tool,
  originalUserInput: task_description,
  executorAssignments: executorAssignments,  // { taskId: { executor, reason } } — overrides executionMethod
  session: {
@@ -588,7 +600,7 @@ TodoWrite({ todos: [
  { content: "LP-Phase 1: Exploration", status: "completed" },
  { content: "LP-Phase 2: Clarification", status: "completed" },
  { content: "LP-Phase 3: Planning", status: "completed" },
-  { content: `LP-Phase 4: Confirmed [${userSelection.execution_method}]`, status: "completed" },
+  { content: `LP-Phase 4: Confirmed [${userSelection.execution_method} | CR:${userSelection.code_review_tool} | CVR:${userSelection.convergence_review_tool}]`, status: "completed" },
  { content: `LP-Phase 5: Handoff → lite-execute`, status: "completed" },
  { content: `LE-Phase 1: Task Loading [${taskCount} tasks]`, status: "in_progress", activeForm: "Loading tasks" }
 ]})
@@ -605,6 +617,7 @@ Skill("lite-execute")
 ├── explorations-manifest.json        # Exploration index
 ├── planning-context.md               # Evidence paths + understanding
 ├── plan.json                         # Plan overview (task_ids[])
+├── code-review.md                    # Generated by lite-execute Step 4
 ├── test-checklist.json               # Generated by lite-test-review
 ├── test-review.md                    # Generated by lite-test-review
 └── .task/
@@ -618,9 +631,12 @@ Skill("lite-execute")
 ```
 lite-plan (LP-Phase 1-5)
  └─ Skill("lite-execute")     ← executionContext (global)
-       ├─ Step 1-4: Execute + Review
+       ├─ Step 1-3: Task Execution
+       ├─ Step 4: Code Review (quality/correctness/security)
       └─ Step 5: Skill("lite-test-review")  ← testReviewContext (global)
-            ├─ TR-Phase 1-4: Test + Fix
+            ├─ TR-Phase 1: Detect test framework
+            ├─ TR-Phase 2: Convergence verification (plan criteria)
+            ├─ TR-Phase 3-4: Run tests + Auto-fix
            └─ TR-Phase 5: Report + Sync specs
 ```

--- a/.claude/skills/workflow-lite-test-review/SKILL.md
+++ b/.claude/skills/workflow-lite-test-review/SKILL.md
@@ -31,31 +31,31 @@ Test review and fix engine for lite-execute chain or standalone invocation.

 **Input Source**: `testReviewContext` global variable set by lite-execute Step 4

-**Behavior**: Skip session discovery, inherit reviewTool from execution chain, proceed directly to TR-Phase 1.
+**Behavior**: Skip session discovery, inherit convergenceReviewTool from execution chain, proceed directly to TR-Phase 1.

-> **Note**: lite-execute Step 4 is the chain gate. Mode 1 invocation means execution is complete — proceed with test review.
+> **Note**: lite-execute Step 5 is the chain gate. Mode 1 invocation means execution + code review are complete — proceed with convergence verification + tests.

 ### Mode 2: Standalone

 **Trigger**: User calls with session path or `--last`

-**Behavior**: Discover session → load plan + tasks → `reviewTool = 'agent'` → proceed to TR-Phase 1.
+**Behavior**: Discover session → load plan + tasks → `convergenceReviewTool = 'agent'` → proceed to TR-Phase 1.

 ```javascript
-let sessionPath, plan, taskFiles, reviewTool
+let sessionPath, plan, taskFiles, convergenceReviewTool

 if (testReviewContext) {
  // Mode 1: from lite-execute chain
  sessionPath = testReviewContext.session.folder
  plan = testReviewContext.planObject
  taskFiles = testReviewContext.taskFiles.map(tf => JSON.parse(Read(tf.path)))
-  reviewTool = testReviewContext.reviewTool || 'agent'
+  convergenceReviewTool = testReviewContext.convergenceReviewTool || 'agent'
 } else {
  // Mode 2: standalone — find last session or use provided path
  sessionPath = resolveSessionPath($ARGUMENTS)  // Glob('.workflow/.lite-plan/*/plan.json'), take last
  plan = JSON.parse(Read(`${sessionPath}/plan.json`))
  taskFiles = plan.task_ids.map(id => JSON.parse(Read(`${sessionPath}/.task/${id}.json`)))
-  reviewTool = 'agent'
+  convergenceReviewTool = 'agent'
 }

 const skipFix = $ARGUMENTS?.includes('--skip-fix') || false
@@ -66,7 +66,7 @@ const skipFix = $ARGUMENTS?.includes('--skip-fix') || false
 | Phase | Core Action | Output |
 |-------|-------------|--------|
 | TR-Phase 1 | Detect test framework + gather changes | testConfig, changedFiles |
-| TR-Phase 2 | Review implementation against convergence criteria | reviewResults[] |
+| TR-Phase 2 | Convergence verification against plan criteria | reviewResults[] |
 | TR-Phase 3 | Run tests + generate checklist | test-checklist.json |
 | TR-Phase 4 | Auto-fix failures (iterative, max 3 rounds) | Fixed code + updated checklist |
 | TR-Phase 5 | Output report + chain to session:sync | test-review.md |
@@ -93,32 +93,45 @@ Output: `testConfig = { command, framework, type }` + `changedFiles[]`

 // TodoWrite: Phase 1 → completed, Phase 2 → in_progress

-## TR-Phase 2: Review Implementation Against Plan
+## TR-Phase 2: Convergence Verification

-**Skip if**: `reviewTool === 'skip'`  — set all tasks to PASS, proceed to Phase 3.
+**Skip if**: `convergenceReviewTool === 'skip'`  — set all tasks to PASS, proceed to Phase 3.

-For each task, verify convergence criteria and identify test gaps.
+Verify each task's convergence criteria are met in the implementation and identify test gaps.

-**Agent Review** (reviewTool === 'agent', default):
+**Agent Convergence Review** (convergenceReviewTool === 'agent', default):

 For each task in taskFiles:
-1. Extract `convergence.criteria[]` and `test` requirements
-2. Find changed files matching `task.files[].path` against `changedFiles`
-3. Read matched files, evaluate each criterion against implementation
-4. Check test coverage: if `task.test.unit` exists but no test files in changedFiles → mark as test gap
-5. Same for `task.test.integration`
-6. Build `reviewResult = { taskId, title, criteria_met[], criteria_unmet[], test_gaps[], files_reviewed[] }`
+1. Extract `convergence.criteria[]` from the task
+2. Match `task.files[].path` against `changedFiles` to find actually-changed files
+3. Read each matched file, verify each convergence criterion with file:line evidence
+4. Check test coverage gaps:
+   - If `task.test.unit` defined but no matching test files in changedFiles → mark as test gap
+   - If `task.test.integration` defined but no integration test in changedFiles → mark as test gap
+5. Build `reviewResult = { taskId, title, criteria_met[], criteria_unmet[], test_gaps[], files_reviewed[] }`

-**CLI Review** (reviewTool === 'gemini' or 'codex'):
+**Verdict logic**:
+- PASS = all `convergence.criteria` met + no test gaps
+- PARTIAL = some criteria met OR has test gaps
+- FAIL = no criteria met
+
+**CLI Convergence Review** (convergenceReviewTool === 'gemini' or 'codex'):

 ```javascript
-const reviewId = `${sessionId}-tr-review`
-Bash(`ccw cli -p "PURPOSE: Post-execution test review — verify convergence criteria met and identify test gaps
-TASK: • Read plan.json and .task/*.json convergence criteria • For each criterion, check implementation in changed files • Identify missing unit/integration tests • List unmet criteria with file:line evidence
+const reviewId = `${sessionId}-convergence`
+const taskCriteria = taskFiles.map(t => `${t.id}: [${(t.convergence?.criteria || []).join(' | ')}]`).join('\n')
+Bash(`ccw cli -p "PURPOSE: Convergence verification — check each task's completion criteria against actual implementation
+TASK: • For each task below, verify every convergence criterion is satisfied in the changed files • Mark each criterion as MET (with file:line evidence) or UNMET (with what's missing) • Identify test coverage gaps (planned tests not found in changes)
+
+TASK CRITERIA:
+${taskCriteria}
+
+CHANGED FILES: ${changedFiles.join(', ')}
+
 MODE: analysis
-CONTEXT: @${sessionPath}/plan.json @${sessionPath}/.task/*.json @**/* | Memory: lite-execute completed, reviewing convergence
-EXPECTED: Per-task verdict table (PASS/PARTIAL/FAIL) + unmet criteria list + test gap list
-CONSTRAINTS: Read-only | Focus on convergence verification" --tool ${reviewTool} --mode analysis --id ${reviewId}`, { run_in_background: true })
+CONTEXT: @${sessionPath}/plan.json @${sessionPath}/.task/*.json @**/* | Memory: lite-execute completed
+EXPECTED: Per-task verdict (PASS/PARTIAL/FAIL) with per-criterion evidence + test gap list
+CONSTRAINTS: Read-only | Focus strictly on convergence criteria verification, NOT code quality (code review already done in lite-execute)" --tool ${convergenceReviewTool} --mode analysis --id ${reviewId}`, { run_in_background: true })
 // STOP - wait for hook callback, then parse CLI output into reviewResults format
 ```

@@ -207,13 +220,13 @@ Skill({ skill: "workflow:session:sync", args: `-y "Test review: ${testChecklist.

 ## Data Structures

-### testReviewContext (Input - Mode 1, set by lite-execute)
+### testReviewContext (Input - Mode 1, set by lite-execute Step 5)

 ```javascript
 {
  planObject: { /* same as executionContext.planObject */ },
  taskFiles: [{ id: string, path: string }],
-  reviewTool: "skip" | "agent" | "gemini" | "codex",
+  convergenceReviewTool: "skip" | "agent" | "gemini" | "codex",
  executionResults: [...],
  originalUserInput: string,
  session: {