diff --git a/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json b/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json index a664ff0a..6f2739b1 100644 --- a/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json +++ b/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json @@ -21,7 +21,7 @@ "name": "unic-pr-review", "source": "./", "tags": ["productivity", "code-review", "azure-devops"], - "version": "2.1.2" + "version": "2.1.3" } ] } diff --git a/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json b/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json index 27a9363a..6a2223f6 100644 --- a/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json +++ b/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json @@ -15,5 +15,5 @@ "keywords": ["pr-review", "azure-devops", "jira", "confluence", "code-review", "unic"], "license": "LGPL-3.0-or-later", "name": "unic-pr-review", - "version": "2.1.2" + "version": "2.1.3" } diff --git a/apps/claude-code/unic-pr-review/CHANGELOG.md b/apps/claude-code/unic-pr-review/CHANGELOG.md index fc9eb241..28c4fc07 100644 --- a/apps/claude-code/unic-pr-review/CHANGELOG.md +++ b/apps/claude-code/unic-pr-review/CHANGELOG.md @@ -16,6 +16,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - (none) +## [2.1.3] — 2026-06-05 + +### Breaking +- (none) + +### Added +- Two-phase code-simplifier model (issue #216, ADR-0013): `code-simplifier` is removed from the initial parallel fan-out and runs as a second phase only when Phase 1 yields no Critical and no Important findings and ≥3 non-test source files changed; Phase 2 honours preview / `--dry-run` (computes and renders, posts nothing); new `shouldRunPhase2` pure helper in `changed-file-analyser.mjs` with full unit-test coverage + +### Fixed +- (none) + ## [2.1.2] — 2026-06-04 ### Breaking diff --git a/apps/claude-code/unic-pr-review/commands/review-pr.md b/apps/claude-code/unic-pr-review/commands/review-pr.md index 175f9ef9..05a88a11 100644 --- a/apps/claude-code/unic-pr-review/commands/review-pr.md +++ b/apps/claude-code/unic-pr-review/commands/review-pr.md @@ -187,6 +187,8 @@ Print the spawn set to the terminal. Otherwise (`rawDiff` non-empty), proceed as in Step 7 (Pre-PR): launch every agent in `SPAWN_SET` simultaneously, seeding each with the diff (and `intentBrief` as a preamble when it is defined). Spawn the Intent Assessor in the **same parallel batch** when `intentBrief` is defined **and** the `intentCheck` skeleton is non-empty (ADR-0011) — it is never added to `SPAWN_SET`. +After all Phase 1 agents finish, evaluate and run the **Phase 2 gate** exactly as described in the Pre-PR Step 7 "Phase 2 — Code Simplifier" section (ADR-0013): call `shouldRunPhase2` with `FETCHER_OUTPUT.changedFiles` and the flattened Phase 1 findings; if true, launch `agents/code-simplifier.md` sequentially with the same diff input (and `intentBrief` preamble when defined), wait for it, and merge its output into the full findings set before proceeding to Step 1.9. + #### Step 1.9 — Merge findings and render (ADO mode) Same as Step 8 (Pre-PR): merge all agents' findings and positive observations, run the overlay merger when the Assessor was spawned, and pass `FINDINGS_JSON`, `INTENT_CHECK_JSON` (if applicable), and `NOTICES_JSON` (if applicable) to `render-summary.mjs`. Always relay the helper's stderr; stop on a non-zero exit. @@ -363,6 +365,8 @@ Prior Findings to re-assess: The aspect agents use `priorFindings` to emit `priorFindingVerdicts` in their output. Store the full aspect-agent response map as `ASPECT_RESPONSES` (keyed by agent name). +After the Phase 1 aspect agents complete, evaluate and run the **Phase 2 gate** (ADR-0013) using `FETCHER_OUTPUT.changedFiles` and the flattened Phase 1 findings from `ASPECT_RESPONSES`. If `shouldRunPhase2` returns true, launch `agents/code-simplifier.md` sequentially (with the delta diff and `intentBrief` preamble when defined), wait for it, and store its response alongside `ASPECT_RESPONSES` so the Coordinator receives the full finding set. + #### Step 1.8a — Invoke Re-review Coordinator (re-review mode only) After all aspect agents complete, use the Agent tool to launch `unic-pr-review:re-review-coordinator`. Provide: @@ -627,6 +631,24 @@ The Intent Assessor is **not** a Review Aspect and is **not** in the spawn set r Store the Assessor's response separately as `ASSESSOR_RESPONSE`. +### Phase 2 — Code Simplifier (conditional, sequential, after Phase 1 completes) + +After all Phase 1 agents finish, evaluate the Phase 2 gate (ADR-0013). The gate is implemented by `shouldRunPhase2` in `scripts/lib/changed-file-analyser.mjs` — call it via an inline Node.js one-liner: + +```sh +node -e " +const {shouldRunPhase2}=await import('${CLAUDE_PLUGIN_ROOT}/scripts/lib/changed-file-analyser.mjs') +const files=JSON.parse(process.env.FILES) +const findings=JSON.parse(process.env.FINDINGS) +process.stdout.write(shouldRunPhase2(files,findings)?'true':'false') +" FILES='' FINDINGS='' +``` + +- **Output `true`**: launch `agents/code-simplifier.md` sequentially (wait for it before Step 8). Provide the same diff (and `intentBrief` preamble when defined) as in the Phase 1 fan-out. Wait for it to complete and merge its `findings` and `positiveObservations` into the full set alongside the Phase 1 results. +- **Output `false`**: skip Phase 2 entirely and proceed to Step 8. + +Phase 2 honours the preview / `--dry-run` principle from ADR-0003: it always computes and renders, but nothing in Phase 2 changes the write path — if `IS_POST` is false the preview is terminal-only regardless. + ## Step 8 — Merge findings and render the Review Summary Merge the responses from all agents: diff --git a/apps/claude-code/unic-pr-review/docs/adr/0013-two-phase-code-simplifier.md b/apps/claude-code/unic-pr-review/docs/adr/0013-two-phase-code-simplifier.md new file mode 100644 index 00000000..1fa2ea70 --- /dev/null +++ b/apps/claude-code/unic-pr-review/docs/adr/0013-two-phase-code-simplifier.md @@ -0,0 +1,39 @@ +# 0013. Two-phase code-simplifier model + +**Status:** Accepted (2026-06) + +## Context + +The `pr-review-toolkit` (Anthropic's reference implementation) runs `code-simplifier` as a **post-pass** — only after the review has passed — rather than in the initial parallel fan-out alongside the other Review Aspect agents. `unic-pr-review` originally included `code-simplifier` in the Phase 1 fan-out under the SPAWN_TABLE heuristic `≥3 non-test source files` (ADR-0008). This means a simplification pass can run on code that still has real correctness or quality problems, wasting tokens and potentially surfacing misleading "polish" suggestions on broken code. + +Two approaches were considered: + +- **Keep the single-phase fan-out.** Rejected — running a simplification pass on code that still has Critical or Important problems is noise. A simplification suggestion ("extract this helper") is unhelpful when the code is structurally incorrect. +- **Two-phase model: Review Aspect fan-out first, then an optional simplification phase.** Accepted — mirrors the `pr-review-toolkit` approach, aligns with the intuition that simplification is only valuable on code that is already correct. + +## Decision + +`code-simplifier` is removed from the Phase 1 SPAWN_TABLE (ADR-0008). After Phase 1 completes, the orchestrator checks two conditions before launching Phase 2: + +1. **Severity gate** (ADR-0002): the merged Phase 1 findings array contains **zero** entries with severity `critical` or `important`. Minor findings are acceptable — they do not block Phase 2. +2. **File-count gate**: the diff contains **three or more** non-test source files (the same threshold previously used in SPAWN_TABLE). + +Only when both conditions are true does the orchestrator launch `code-simplifier` as a sequential Phase 2 step. `code-simplifier`'s findings are merged into the full findings set before rendering, so they appear in the Review Summary alongside the Phase 1 findings. + +A pure-function helper `shouldRunPhase2(changedFiles, findings)` is exported from `scripts/lib/changed-file-analyser.mjs` so the gate is unit-testable independently of the orchestrator. + +## Interaction with the Approval Loop (ADR-0003) and re-review mode (ADR-0007) + +- **Preview / no `--post`**: Phase 2 always runs (when conditions are met) and its findings appear in the terminal preview. Nothing is posted to ADO — this is unchanged from the default dry-run behaviour. +- **`--post` (first-review)**: Phase 2 runs before the Approval Loop. Its findings enter the same loop as Phase 1 findings — they are not distinguished from Phase 1 findings in the Approval Loop or in the ADO threads. +- **`--post --yes`**: same as above, bulk-accepted without prompting. +- **Re-review mode**: Phase 2 runs (when conditions are met) after the Re-review Coordinator produces its plan. Phase 2 findings are treated as fresh findings and follow the same write path as other fresh findings. +- **`diffUnavailable` guard**: when the ADO Fetcher sets `diffUnavailable: true`, neither Phase 1 nor Phase 2 spawns agents — the existing guard in Step 1.8 covers both phases. + +## Consequences + +- The ADR-0008 Spawn Table loses `code-simplifier`. The entry in `changed-file-analyser.mjs` that previously listed it is removed. +- `code-simplifier` findings can no longer appear for PRs with Critical or Important problems, which is the intended behaviour. +- A PR with ≥3 source files and only Minor Phase 1 findings gets a polish pass; the reviewer sees it alongside the Minor findings. +- The file-count gate is now evaluated at Phase 2 decision time (after Phase 1), not at spawn-set computation time. The threshold (≥3) is unchanged. +- Tests for `decideSpawnSet` must be updated to remove `code-simplifier` assertions. New tests for `shouldRunPhase2` cover the three canonical scenarios: pass+≥3-source → `true`; pass+<3-source → `false`; Important present → `false`. diff --git a/apps/claude-code/unic-pr-review/package.json b/apps/claude-code/unic-pr-review/package.json index 9286f360..9eb8dc93 100644 --- a/apps/claude-code/unic-pr-review/package.json +++ b/apps/claude-code/unic-pr-review/package.json @@ -22,5 +22,5 @@ "verify:changelog": "unic-verify-changelog" }, "type": "module", - "version": "2.1.2" + "version": "2.1.3" } diff --git a/apps/claude-code/unic-pr-review/scripts/lib/changed-file-analyser.mjs b/apps/claude-code/unic-pr-review/scripts/lib/changed-file-analyser.mjs index b4d57920..455f68d6 100644 --- a/apps/claude-code/unic-pr-review/scripts/lib/changed-file-analyser.mjs +++ b/apps/claude-code/unic-pr-review/scripts/lib/changed-file-analyser.mjs @@ -30,6 +30,10 @@ const isDocFile = (f) => /\.(md|mdx)$/.test(f) || /(^|[/\\])docs?[/\\]/i.test(f) * Spawn-decision table (ADR-0008). Each entry maps an agent name to its spawn * predicate. The table is evaluated in order; code-reviewer is always first. * + * code-simplifier is absent deliberately — it runs as a Phase 2 post-pass only + * when Phase 1 yields no Critical/Important findings and ≥3 source files changed + * (ADR-0013). Use shouldRunPhase2() to evaluate that gate. Never add it here. + * * @type {Array<{ agent: string, predicate: (files: string[]) => boolean }>} */ // Intent Assessor is absent deliberately — spawned by intent presence, not file categories (ADR-0011). Never add it here. @@ -39,7 +43,6 @@ const SPAWN_TABLE = [ { agent: 'type-design-analyzer', predicate: (files) => files.some(isTypeFile) }, { agent: 'pr-test-analyzer', predicate: (files) => files.some(isTestFile) }, { agent: 'comment-analyzer', predicate: (files) => files.some(isDocFile) }, - { agent: 'code-simplifier', predicate: (files) => files.filter(isSourceFile).length >= 3 }, ] /** @@ -60,6 +63,26 @@ export function decideSpawnSet(changedFiles) { return new Set(SPAWN_TABLE.filter(({ predicate }) => predicate(changedFiles)).map(({ agent }) => agent)) } +/** + * Decide whether to run the Phase 2 code-simplifier pass (ADR-0013). + * + * Returns true only when both conditions hold: + * 1. No Critical or Important findings in Phase 1 (severity gate, ADR-0002). + * 2. Three or more non-test source files changed (file-count gate). + * + * @param {string[]} changedFiles - relative paths of files changed in the diff + * @param {Array<{ severity?: string }>} findings - merged Phase 1 findings + * @returns {boolean} + */ +export function shouldRunPhase2(changedFiles, findings) { + if (!Array.isArray(changedFiles)) + throw new Error(`shouldRunPhase2: changedFiles must be an array, got ${typeof changedFiles}`) + if (!Array.isArray(findings)) throw new Error(`shouldRunPhase2: findings must be an array, got ${typeof findings}`) + const hasBlocker = findings.some((f) => f.severity === 'critical' || f.severity === 'important') + if (hasBlocker) return false + return changedFiles.filter(isSourceFile).length >= 3 +} + /** * Parse raw stdin into a clean list of changed-file paths. * diff --git a/apps/claude-code/unic-pr-review/tests/changed-file-analyser.test.mjs b/apps/claude-code/unic-pr-review/tests/changed-file-analyser.test.mjs index b686486c..08f1d96f 100644 --- a/apps/claude-code/unic-pr-review/tests/changed-file-analyser.test.mjs +++ b/apps/claude-code/unic-pr-review/tests/changed-file-analyser.test.mjs @@ -7,7 +7,7 @@ import { spawnSync } from 'node:child_process' import path from 'node:path' import { describe, it } from 'node:test' import { fileURLToPath } from 'node:url' -import { decideSpawnSet, parseStdin } from '../scripts/lib/changed-file-analyser.mjs' +import { decideSpawnSet, parseStdin, shouldRunPhase2 } from '../scripts/lib/changed-file-analyser.mjs' const SCRIPT = path.resolve(fileURLToPath(import.meta.url), '../../scripts/lib/changed-file-analyser.mjs') @@ -130,34 +130,15 @@ describe('decideSpawnSet', () => { }) describe('code-simplifier', () => { - it('is spawned when 3 or more source files changed (complexity heuristic)', () => { - assert.ok(decideSpawnSet(['src/a.mjs', 'src/b.mjs', 'src/c.mjs']).has('code-simplifier')) + it('is never in the Phase 1 spawn set — it runs as a Phase 2 post-pass (ADR-0013)', () => { + const three = ['src/a.mjs', 'src/b.mjs', 'src/c.mjs'] + assert.ok(!decideSpawnSet(three).has('code-simplifier'), 'not in spawn set for 3 source files') }) - it('is spawned when more than 3 source files changed', () => { + it('is absent from the spawn set even for many source files', () => { const files = ['src/a.mjs', 'src/b.mjs', 'src/c.mjs', 'src/d.ts'] - assert.ok(decideSpawnSet(files).has('code-simplifier')) - }) - - it('is NOT spawned when only 2 source files changed', () => { - assert.ok(!decideSpawnSet(['src/a.mjs', 'src/b.mjs']).has('code-simplifier')) - }) - - it('is NOT spawned when only 1 source file changed', () => { - assert.ok(!decideSpawnSet(['src/a.mjs']).has('code-simplifier')) - }) - - it('test files do not count toward the source-file threshold', () => { - const files = ['src/a.mjs', 'tests/a.test.mjs', 'tests/b.test.mjs'] assert.ok(!decideSpawnSet(files).has('code-simplifier')) }) - - it('.d.ts declaration files do not count toward the source-file threshold', () => { - const files = ['src/types/a.d.ts', 'src/types/b.d.ts', 'src/types/c.d.ts'] - const result = decideSpawnSet(files) - assert.ok(!result.has('code-simplifier'), 'code-simplifier not spawned') - assert.ok(result.has('type-design-analyzer'), 'type-design-analyzer still spawned') - }) }) describe('mixed-content diff', () => { @@ -176,7 +157,7 @@ describe('decideSpawnSet', () => { assert.ok(result.has('type-design-analyzer'), 'type-design-analyzer') assert.ok(result.has('pr-test-analyzer'), 'pr-test-analyzer') assert.ok(result.has('comment-analyzer'), 'comment-analyzer') - assert.ok(result.has('code-simplifier'), 'code-simplifier (3+ source files)') + assert.ok(!result.has('code-simplifier'), 'code-simplifier is Phase 2 only — not in Phase 1 spawn set (ADR-0013)') }) it('spawns code-reviewer and silent-failure-hunter for a single plain source file', () => { @@ -278,7 +259,7 @@ describe('CLI entry point', () => { assert.doesNotThrow(() => JSON.parse(result.stdout.trim())) }) - it('spawns all six agents for a mixed-content diff via stdin', () => { + it('spawns five Phase 1 agents for a mixed-content diff via stdin (code-simplifier is Phase 2 only)', () => { const input = `${[ 'src/service.mjs', 'src/utils.mjs', @@ -296,6 +277,72 @@ describe('CLI entry point', () => { assert.ok(agents.includes('type-design-analyzer')) assert.ok(agents.includes('pr-test-analyzer')) assert.ok(agents.includes('comment-analyzer')) - assert.ok(agents.includes('code-simplifier')) + assert.ok(!agents.includes('code-simplifier'), 'code-simplifier absent from Phase 1 spawn set (ADR-0013)') + }) +}) + +describe('shouldRunPhase2', () => { + const THREE_SOURCE = ['src/a.mjs', 'src/b.mjs', 'src/c.mjs'] + const TWO_SOURCE = ['src/a.mjs', 'src/b.mjs'] + + it('throws when changedFiles is not an array', () => { + // @ts-expect-error — intentional misuse + assert.throws(() => shouldRunPhase2(null, []), /changedFiles must be an array/) + }) + + it('throws when findings is not an array', () => { + // @ts-expect-error — intentional misuse + assert.throws(() => shouldRunPhase2(THREE_SOURCE, null), /findings must be an array/) + }) + + it('returns true when Phase 1 passes and ≥3 source files changed (canonical AC scenario)', () => { + const findings = [{ severity: 'minor' }, { severity: 'minor' }] + assert.ok(shouldRunPhase2(THREE_SOURCE, findings)) + }) + + it('returns true when Phase 1 has zero findings and ≥3 source files changed', () => { + assert.ok(shouldRunPhase2(THREE_SOURCE, [])) + }) + + it('returns false when Phase 1 passes but <3 source files changed (canonical AC scenario)', () => { + assert.ok(!shouldRunPhase2(TWO_SOURCE, [])) + }) + + it('returns false when Phase 1 passes but only 1 source file changed', () => { + assert.ok(!shouldRunPhase2(['src/a.mjs'], [])) + }) + + it('returns false when Phase 1 has an Important finding — even with ≥3 source files (canonical AC scenario)', () => { + const findings = [{ severity: 'important' }] + assert.ok(!shouldRunPhase2(THREE_SOURCE, findings)) + }) + + it('returns false when Phase 1 has a Critical finding — even with ≥3 source files', () => { + const findings = [{ severity: 'critical' }] + assert.ok(!shouldRunPhase2(THREE_SOURCE, findings)) + }) + + it('returns false when Phase 1 has mixed Critical and Minor findings', () => { + const findings = [{ severity: 'critical' }, { severity: 'minor' }] + assert.ok(!shouldRunPhase2(THREE_SOURCE, findings)) + }) + + it('test files do not count toward the ≥3 source-file threshold', () => { + const files = ['src/a.mjs', 'tests/a.test.mjs', 'tests/b.test.mjs'] + assert.ok(!shouldRunPhase2(files, [])) + }) + + it('.d.ts declaration files do not count toward the ≥3 source-file threshold', () => { + const files = ['src/types/a.d.ts', 'src/types/b.d.ts', 'src/types/c.d.ts'] + assert.ok(!shouldRunPhase2(files, [])) + }) + + it('returns true for exactly 3 source files with only Minor findings', () => { + const findings = [{ severity: 'minor' }] + assert.ok(shouldRunPhase2(['src/x.mjs', 'src/y.mjs', 'src/z.ts'], findings)) + }) + + it('returns false for an empty changed-files list', () => { + assert.ok(!shouldRunPhase2([], [])) }) })