feat(unic-pr-review): align code-simplifier with toolkit two-phase post-pass model

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"
     }
   ]
 }

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"
 }

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

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='<JSON.stringify(changedFiles from Step 6)>' FINDINGS='<JSON.stringify(all Phase 1 findings flattened)>'
+```
+
+- **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:

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`.

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"
 }

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.
  *

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([], []))
 	})
 })