From fcd0b9a2c49ae6e538349de1fd5f9e59a50ed758 Mon Sep 17 00:00:00 2001 From: catlog22 Date: Sat, 21 Mar 2026 17:18:20 +0800 Subject: [PATCH] =?UTF-8?q?refactor:=20split=20review=20responsibilities?= =?UTF-8?q?=20=E2=80=94=20code=20review=20in=20lite-execute,=20convergence?= =?UTF-8?q?=20review=20in=20lite-test-review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .claude/skills/workflow-lite-execute/SKILL.md | 120 ++++++++++++++++-- .claude/skills/workflow-lite-plan/SKILL.md | 38 ++++-- .../skills/workflow-lite-test-review/SKILL.md | 65 ++++++---- 3 files changed, 172 insertions(+), 51 deletions(-) diff --git a/.claude/skills/workflow-lite-execute/SKILL.md b/.claude/skills/workflow-lite-execute/SKILL.md index d13ea906..44ea66a5 100644 --- 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 } diff --git a/.claude/skills/workflow-lite-plan/SKILL.md b/.claude/skills/workflow-lite-plan/SKILL.md index b3bf9a18..60841ec8 100644 --- 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 ``` diff --git a/.claude/skills/workflow-lite-test-review/SKILL.md b/.claude/skills/workflow-lite-test-review/SKILL.md index 1628f85f..21d2c33c 100644 --- 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: {