feat(unic-pr-review): remaining five aspect agents + conditional spawning

[orioltf]

Summary

• Add five Review Aspect agents: silent-failure-hunter (Argus/red), type-design-analyzer (Euclid/magenta), pr-test-analyzer (Vesta/green), comment-analyzer (Scribe/yellow), code-simplifier (Occam/blue)
• Rewrite changed-file-analyser.mjs to export decideSpawnSet(changedFiles) → Set<string> with a full table-driven spawn-decision implementation (ADR-0008)
• Expand tests/changed-file-analyser.test.mjs to cover all six spawn predicates with positive and negative fixtures each

Why

Closes #146. Completes the six-agent fan-out specified in ADR-0008 — code-reviewer was the only aspect agent that existed; the other five conditional agents were deferred from issue #145.

Test plan

• All spawn predicate tests pass (positive + negative for each of the five conditional agents)
• Mixed-content diff test confirms all six agents are spawned when the diff touches code + tests + docs + types (≥3 source files)
• pnpm --filter unic-pr-review test green
• pnpm --filter unic-pr-review typecheck clean
• pnpm ci:check clean
• Each agent has a distinct name and colour (US 44)

🤖 Generated with Claude Code

[orioltf]

🔍 Comprehensive PR Review

PR: #158 — feat(unic-pr-review): remaining five aspect agents + conditional spawning
Reviewed by: 4 specialized agents (code-review, error-handling, test-coverage, docs-impact)
Date: 2026-05-28

---

Summary

The PR cleanly implements the five remaining Review Aspect agents and rewrites changed-file-analyser.mjs to a table-driven spawn decision module. Logic, structure, and predicate unit tests are all solid. Two issues must be fixed before merge: the CLI guard uses a URL idiom that silently breaks on Windows CI (the review fans out with zero agents), and the CLI entry block — the actual interface review-pr.md calls — has no test coverage.

Verdict: REQUEST_CHANGES

Severity	Count
🔴 HIGH	2
🟡 MEDIUM	8
🟢 LOW	2

---

🔴 HIGH Issues (Fix Before Merge)

1. CLI guard uses non-portable new URL() — silently breaks on Windows

📍 apps/claude-code/unic-pr-review/scripts/lib/changed-file-analyser.mjs:62

The guard if (import.meta.url === new URL(process.argv[1], 'file:').href) fails on Windows: backslashes in process.argv[1] make the URL comparison never match, so the CLI block never runs, the script exits 0 with empty stdout, and review-pr.md Step 4a parses [] — every review silently produces zero findings. Also missing the process.argv[1] && null guard (throws TypeError in REPL/unusual contexts).

Every other CLI script in this plugin already uses the correct pattern.

View fix

// top of file — add import
import { pathToFileURL } from 'node:url'

// line 62
if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {

Pattern reference: apps/claude-code/unic-pr-review/scripts/lib/base-branch-resolver.mjs:67

---

2. CLI entry point is entirely untested

📍 apps/claude-code/unic-pr-review/scripts/lib/changed-file-analyser.mjs:62-77

The stdin→JSON pipeline — the exact interface invoked by review-pr.md via | node ... — has zero tests. The decideSpawnSet() unit tests are thorough but they bypass the CLI parsing entirely. A regression (e.g. JSON.stringify called on the Set instead of the spread) would produce empty output and zero agents with no test failure.

View recommended tests

Add a describe('CLI entry point') block to tests/changed-file-analyser.test.mjs:

import { spawnSync } from 'node:child_process'
import { fileURLToPath } from 'node:url'
import path from 'node:path'

const SCRIPT = path.resolve(fileURLToPath(import.meta.url), '../../scripts/lib/changed-file-analyser.mjs')

describe('CLI entry point', () => {
	it('emits a JSON array of agent names to stdout for a source file', () => {
		const result = spawnSync('node', [SCRIPT], { input: 'src/service.mjs\n', encoding: 'utf8' })
		assert.equal(result.status, 0)
		const agents = JSON.parse(result.stdout.trim())
		assert.ok(Array.isArray(agents))
		assert.ok(agents.includes('code-reviewer'))
	})

	it('emits an empty JSON array for empty stdin', () => {
		const result = spawnSync('node', [SCRIPT], { input: '', encoding: 'utf8' })
		assert.equal(result.status, 0)
		assert.deepEqual(JSON.parse(result.stdout.trim()), [])
	})

	it('emits an empty JSON array for whitespace-only stdin', () => {
		const result = spawnSync('node', [SCRIPT], { input: '   \n\n  \n', encoding: 'utf8' })
		assert.equal(result.status, 0)
		assert.deepEqual(JSON.parse(result.stdout.trim()), [])
	})

	it('produces valid JSON with a trailing newline', () => {
		const result = spawnSync('node', [SCRIPT], { input: 'src/a.mjs\n', encoding: 'utf8' })
		assert.ok(result.stdout.endsWith('\n'))
		assert.doesNotThrow(() => JSON.parse(result.stdout.trim()))
	})
})

---

🟡 Medium Issues (Needs Decision)

3. Test description contradicts its assertions

📍 tests/changed-file-analyser.test.mjs:162

Test named 'spawns only code-reviewer for a single non-typed non-doc source file' but body also asserts silent-failure-hunter. Fix: rename to 'spawns code-reviewer and silent-failure-hunter for a single plain source file'.

4. Missing stdin 'error' event handler

📍 scripts/lib/changed-file-analyser.mjs:65

'data' and 'end' handlers present but no 'error' handler. An EPIPE on stdin throws an uncaught exception bypassing the changed-file-analyser: <message> error format. Fix: add process.stdin.on('error', (err) => { process.stderr.write(...); process.exit(1) }).

5. .tsx files not matched by isTypeFile

📍 scripts/lib/changed-file-analyser.mjs:20-22

/\.ts$/ doesn't match .tsx. A PR changing only React component files will not spawn type-design-analyzer. Fix: change to /\.tsx?$/.

6. comment-analyzer predicate misses JSDoc rot in source files

📍 scripts/lib/changed-file-analyser.mjs:37

The agent's scope includes JSDoc @param/@returns rot in source files, but its predicate only fires for .md/docs changes. A pure-code PR silently skips comment rot. Consider extending to files.some(isDocFile) || files.some(isSourceFile) — but worth an ADR-0008 discussion first.

7. Windows path separator coverage missing in tests

📍 tests/changed-file-analyser.test.mjs

All tests use Unix forward-slash paths. The predicates have [/\\] for Windows backslashes but this branch is never exercised by tests. Add a few backslash-path variants.

8. ADR-0008 remains future-tense after full implementation

📍 docs/adr/0008-conditional-sub-agent-spawning.md

Consequences section says the plugin "needs" a changed-file analyser — it now has one. Spawn predicates for all 6 agents are not documented in the ADR.

9. "Review Aspect" and "Spawn Set" missing from CONTEXT.md vocabulary

📍 apps/claude-code/unic-pr-review/CONTEXT.md

Both terms appear throughout review-pr.md and ADR-0008 but have no glossary entries. Relationships section also incorrectly states agents receive an "Intent Brief" (not yet wired).

10. AGENTS.md missing two-part checklist for adding new Review Aspect agents

📍 apps/claude-code/unic-pr-review/AGENTS.md

The two-part contract (1: create agent .md in agents/, 2: add entry to SPAWN_TABLE) isn't documented. An agent file without a SPAWN_TABLE entry is silently never spawned.

---

🟢 Low Issues

View 2 low-priority suggestions

Issue	Location	Suggestion
Agent files use wrong-domain JSON examples	agents/code-simplifier.md etc.	Replace "Null pointer" example with a domain-appropriate finding per agent
README outdated — review-pr command missing	README.md	Add review-pr to Commands table; update Quick Start from "coming soon" to functional

---

✅ What's Good

• Table-driven SPAWN_TABLE is clean and extensible — one entry per aspect agent
• Predicate unit tests are excellent: all 6 entries have positive + negative fixtures, threshold boundary tests, and a mixed-content integration scenario
• review-pr.md accurately reflects the implementation: invocation, spawn-set print, parallel launch, merge step, and agent-name-to-file mapping
• Five new agent prompts follow the same structure as code-reviewer.md — consistent and immediately readable
• decideSpawnSet returns Set<string> — O(1) membership checks, no duplicates
• Zero new runtime dependencies — consistent with the plugin's zero-dep philosophy

---

Next Steps

1. 🔴 Fix HIGH issues 1 & 2 (URL guard + CLI tests)
2. 🟡 Fix trivial MEDIUMs in this PR: 3, 4, 5, 7, 10
3. 📋 Create issues for 6 (predicate alignment), 8 (ADR), 9 (CONTEXT.md)

---

Reviewed by Archon comprehensive-pr-review workflow
Full artifacts: /Users/oriol.torrent/.archon/workspaces/unic/unic-agents-plugins/artifacts/runs/70d84536fd313df77c802e42714464e8/review/

[orioltf]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-fix-issue-146 (commit 0caa345)
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (11 total)

Severity	Count
🔴 HIGH	2
🟠 MEDIUM	7
🟢 LOW	2

View all fixes

• ✅ CLI guard non-portable new URL() (scripts/lib/changed-file-analyser.mjs:62) — Replaced with pathToFileURL(process.argv[1]).href; added process.argv[1] && guard. Portable on Windows.
• ✅ CLI entry point entirely untested (tests/changed-file-analyser.test.mjs) — Added describe('CLI entry point') with 5 spawnSync integration tests covering stdin→stdout pipeline.
• ✅ Test description contradicts assertions (tests/changed-file-analyser.test.mjs:162) — Renamed to 'spawns code-reviewer and silent-failure-hunter for a single plain source file'.
• ✅ Missing stdin 'error' event handler (scripts/lib/changed-file-analyser.mjs:65) — Added process.stdin.on('error', …) with correct error format.
• ✅ .tsx not matched by isTypeFile (scripts/lib/changed-file-analyser.mjs:22) — Changed /\.ts$/ → /\.tsx?$/.
• ✅ Windows path separator coverage missing (tests/changed-file-analyser.test.mjs) — Added 5 backslash-path tests in describe('Windows backslash paths').
• ✅ ADR-0008 future-tense Consequences (docs/adr/0008-conditional-sub-agent-spawning.md) — Updated tense; added Spawn Table documenting all 6 agent predicates.
• ✅ CONTEXT.md missing Review Aspect + Spawn Set (CONTEXT.md) — Added both glossary entries with Avoid terms; fixed Intent Brief Relationships note.
• ✅ AGENTS.md missing two-part checklist (AGENTS.md) — Added agents/ to Layout tree; added "Adding a new Review Aspect" subsection with mandatory 2-step checklist.
• ✅ README missing review-pr command + stale Quick Start (README.md) — Added review-pr row to Commands table; updated Quick Start from "coming soon" language.
• ✅ Agent files use wrong-domain JSON examples (5 agents/*.md files) — Replaced generic null-pointer example with domain-accurate examples (simplification, JSDoc rot, swallowed error, uncovered branch, type invariant).

---

Tests Added

• CLI entry point (5 tests): stdin→JSON for source file; empty stdin; whitespace-only stdin; trailing newline; 6-agent mixed diff
• Windows backslash paths (5 tests): exercising [/\\] branch in all 4 predicates

Total: 211 tests, 0 failures

---

Skipped (1)

Finding	Reason
comment-analyzer spawn predicate misses JSDoc rot in source files	New concern: extending predicate scope to all source files requires ADR-0008 owner decision — could be intentional design

Suggested follow-up: "Align comment-analyzer spawn predicate with JSDoc-in-source use case" (P2)

---

Validation

✅ Type check | ✅ Lint (exit 0) | ✅ Tests (211 passed)

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-fix-issue-146

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.