diff --git a/apps/claude-code/unic-pr-review/CONTEXT.md b/apps/claude-code/unic-pr-review/CONTEXT.md index 65680fbf..4f0672f9 100644 --- a/apps/claude-code/unic-pr-review/CONTEXT.md +++ b/apps/claude-code/unic-pr-review/CONTEXT.md @@ -77,7 +77,7 @@ One specialised sub-agent lens applied to the whole diff — e.g. `code-reviewer _Avoid_: dimension, pass, check **Spawn Set**: -The `Set` of Review Aspect agent names returned by `decideSpawnSet()` in `scripts/lib/changed-file-analyser.mjs`. Computed once before any agent runs, based on changed-file categories (ADR-0008). +The `Set` of Review Aspect agent names returned by `decideSpawnSet()` in `scripts/lib/changed-file-analyser.mjs`. Computed once before any agent runs, based on changed-file categories (ADR-0008); additionally content-aware for the three semantic gates (`comment-analyzer`, `silent-failure-hunter`, `type-design-analyzer`) via deterministic diff-content sampling (ADR-0008 amendment, 2026-06). _Avoid_: agent list, run set, active agents ## Relationships diff --git a/apps/claude-code/unic-pr-review/docs/adr/0008-conditional-sub-agent-spawning.md b/apps/claude-code/unic-pr-review/docs/adr/0008-conditional-sub-agent-spawning.md index 9c41dd87..d2919a23 100644 --- a/apps/claude-code/unic-pr-review/docs/adr/0008-conditional-sub-agent-spawning.md +++ b/apps/claude-code/unic-pr-review/docs/adr/0008-conditional-sub-agent-spawning.md @@ -1,6 +1,6 @@ # 0008. Conditional sub-agent spawning over per-file chunking -**Status:** Accepted (2026-05) +**Status:** Accepted (2026-05); spawn-gating amended to Y-det hybrid for semantic gates (2026-06, issue #212) ## Context @@ -13,7 +13,7 @@ Two alternatives were considered: ## Decision -The Plugin reviews a PR by fanning out to specialised Review Aspect sub-agents in parallel, each handling the whole diff under one lens. Spawning is conditional on what the diff contains — `code-reviewer` always runs; `pr-test-analyzer` runs only when test files changed; `silent-failure-hunter` runs when at least one non-test source file changed (a path/extension heuristic, not diff-content inspection); etc. The Plugin does NOT split the diff into per-file chunks. +The Plugin reviews a PR by fanning out to specialised Review Aspect sub-agents in parallel, each handling the whole diff under one lens. Spawning is conditional on what the diff contains — `code-reviewer` always runs; `pr-test-analyzer` runs only when test files changed; `silent-failure-hunter` runs when at least one non-test source file changed (a path/extension heuristic, not diff-content inspection — see amendment 2026-06 for the three semantic gates); etc. The Plugin does NOT split the diff into per-file chunks. ## Consequences @@ -21,13 +21,33 @@ The Plugin reviews a PR by fanning out to specialised Review Aspect sub-agents i - The Intent Checker is the one exception — it always runs first (regardless of file types) because its output seeds every other agent's context. Its result is broadcast to every spawned aspect agent. - Adding a new Review Aspect is additive: define the agent in `agents/.md`, add one entry to `SPAWN_TABLE` in `changed-file-analyser.mjs` — no orchestration rewrite needed. -### Spawn Table (as of PR #158) +### Spawn Table (as of PR #158; rows marked † amended by 2026-06 content-gating — see amendment section below) -| Agent | Spawn condition | -| ----------------------- | --------------------------------------------------------------------------------------------------- | -| `code-reviewer` | Always — any non-empty diff | -| `silent-failure-hunter` | At least one non-test source file (`.mjs`, `.cjs`, `.js`, `.ts`, `.tsx`, `.jsx`; excluding `.d.ts`) | -| `type-design-analyzer` | At least one `.d.ts`, `.ts`, `.tsx`, or file under `types/`, `schemas/`, `interfaces/` | -| `pr-test-analyzer` | At least one test file (`.test.*`, `.spec.*`, or under `tests/`, `__tests__/`) | -| `comment-analyzer` | At least one `.md` / `.mdx` file or file under `docs/` | -| `code-simplifier` | Three or more non-test source files (excluding `.d.ts`) | +| Agent | Spawn condition | +| ----------------------- | ----------------------------------------------------------------------------------------------------- | +| `code-reviewer` | Always — any non-empty diff | +| `silent-failure-hunter` | † At least one non-test source file (`.mjs`, `.cjs`, `.js`, `.ts`, `.tsx`, `.jsx`; excluding `.d.ts`) | +| `type-design-analyzer` | † At least one `.d.ts`, `.ts`, `.tsx`, or file under `types/`, `schemas/`, `interfaces/` | +| `pr-test-analyzer` | At least one test file (`.test.*`, `.spec.*`, or under `tests/`, `__tests__/`) | +| `comment-analyzer` | † At least one `.md` / `.mdx` file or file under `docs/` | +| `code-simplifier` | Three or more non-test source files (excluding `.d.ts`) | + +† Path classification is the fast path; additionally gated by deterministic diff-content sampling per the amendment below. + +## Content-gated spawning for semantic gates (amended 2026-06, issue #212) + +### Context for the amendment + +A direct comparison of unic-pr-review v2.1.2 against the `pr-review-toolkit` on PR #5612 revealed that path/extension classification alone causes spawn divergences on three gates: the `comment-analyzer` missed findings when documentation comments were added inside source files (not under `docs/`); the `silent-failure-hunter` over-fired on PRs with no error-handling changes; the `type-design-analyzer` over-fired on PRs that touched `.ts` files but introduced no new types. The toolkit uses semantic (model-read) gate conditions. + +Adopting model-assisted gating wholesale (Y-llm) was considered and rejected — it would break the unit-testable, pure-function classifier that is the foundation of the deterministic spawn decision. + +### Decision (amendment) + +For the three semantic gates — **comment-analyzer** (comments gate), **silent-failure-hunter** (errors gate), and **type-design-analyzer** (types gate) — the classifier additionally reads a small content sample from the diff using **deterministic parsing** (**Y-det**): pure functions over `+`/`-` diff hunks, unit-testable with fixtures, no model calls. Path classification remains the fast path and continues to gate `pr-test-analyzer` and `code-simplifier` without change. + +Semantic gates are **biased toward spawning on ambiguity**: a false-positive spawn produces an empty result block (cheap); a false-negative miss silently omits a potentially valid finding set (as seen in the #5612 comparison). When the classifier cannot determine confidently that a gate's semantic condition is absent, it spawns. + +### Y-llm promotion caveat + +The **errors gate** (`silent-failure-hunter`) is the weakest of the three as a deterministic predicate — "error handling changed" is genuinely semantic and the hardest to capture reliably with regex over diff hunks. It is the **first candidate to promote to Y-llm** (model-assisted gate classification) if Y-det proves too noisy or too blind for that gate in practice. diff --git a/apps/claude-code/unic-pr-review/docs/issues/unic-pr-review/PRD.md b/apps/claude-code/unic-pr-review/docs/issues/unic-pr-review/PRD.md index 8b11a85c..a1d6b3f0 100644 --- a/apps/claude-code/unic-pr-review/docs/issues/unic-pr-review/PRD.md +++ b/apps/claude-code/unic-pr-review/docs/issues/unic-pr-review/PRD.md @@ -200,7 +200,7 @@ What makes a good test for this Plugin: tests assert observable external behavio - **Approval Loop** — Finding-state-machine transitions (accept / edit / skip), non-TTY abort, `--yes` bulk-accept, resumability after interruption, head-SHA mismatch prompt on resume, `/.unic-pr-review//` write + `.gitignore` self-ignore, cleanup on Writer success. Tested by piping scripted stdin and asserting output JSON + state-file contents. - **Mode detector** — input `{ hasUrl, hasPriorSignature, revisionsAvailable }` → mode literal (`pre-pr` / `first-review` / `re-review` / `first-review-fallback`). Pure-function table-driven. - **Severity bucketer** — input confidence integer → severity bucket name (and the `<60` drop). Pure-function. -- **Changed-file analyser** — input changed-files list + a small content sample → set of aspect agents to spawn. Pure-function table-driven. +- **Changed-file analyser** — input changed-files list + a small content sample (content-aware for the comment, errors, and types semantic gates via deterministic diff-parsing; path classification is the fast path for all gates) → set of aspect agents to spawn. Pure-function table-driven. - **Renderers** (`review-summary-renderer.mjs`, `inline-comment-renderer.mjs`) — input structured Findings / Coordinator plan → exact markdown blobs including the Bot Signature footer drawn from `signature.mjs`. Snapshot-style fixtures. - **Setup Wizards** — file writing under `chmod 600` (where supported), idempotent re-runs, env-var override precedence. Tested against a temp HOME. Windows path branches assert "warn but don't fail". - **Doctor** — preflight predicates (`az` present, extension present, `az devops login` valid, Confluence ping, Jira ping when configured). Tested with each predicate stubbed. @@ -226,6 +226,6 @@ What makes a good test for this Plugin: tests assert observable external behavio ## Further Notes - The Plugin retires the existing `apps/claude-code/pr-review` Plugin after acceptance testing. As of 2026-05 both coexist in the monorepo as **fully independent** Plugins — `unic-pr-review` shares no code, no prompts, no fixtures, and no soft dependency with `pr-review`. The maintainer will delete `pr-review` once `unic-pr-review` proves itself on the two target projects (one ADO, one Jira). -- The six aspect agents are written from scratch in this Plugin's own voice with our Confidence-Score rubric embedded verbatim. The conditional-spawning pattern is loosely inspired by Anthropic's `pr-review-toolkit`, but we take no soft dependency and copy no prompts. +- The six code-analysis aspect agents maintain a **behavioural-parity contract** with Anthropic's `pr-review-toolkit`: their spawn-gating conditions and analysis behaviour approximate the toolkit's semantic patterns for the comment, error-handling, and types aspects. The clean-slate doctrine holds — no code, no prompts, and no fixtures are taken from `apps/claude-code/pr-review/`; parity is by independent derivation against this PRD and the ADRs (see ADR-0008 amendment, issue #212). - The root `CONTEXT-MAP.md` must be updated to add this Plugin's `CONTEXT.md` — that update is tracked by issue #143 (plugin scaffold), which also creates the `CONTEXT.md` file itself. - The Plugin's `plugin.json` keywords include `pr-review`, `azure-devops`, `jira`, `confluence`, `code-review`, `unic`.