feat(pr-review-orchestrator-split): pr-review — Orchestrator Split#29
Merged
Conversation
Replaces the 995-line monolith with a focused orchestrator that delegates to three focused agents (ADO Fetcher, Re-review Coordinator, ADO Writer). Mode detection via `az repos pr thread list` (not `az devops invoke`) drives Pre-PR / First-review / Re-review routing. Thread data captured once in step 4 and passed forward — never re-fetched downstream. Pre-PR mode is a stub. All 86 existing re-review module unit tests continue to pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hout ADO Replaces the "not yet implemented" stub with a full Pre-PR operating mode. When /pr-review:review-pr is invoked without a PR URL, the orchestrator now diffs the local branch against its upstream default branch, applies the same file skip-list used in ADO modes (*.g.cs, swagger.*, serialization YAMLs, generated/ directories), launches the pr-review-toolkit review aspect agents with the local diff and filtered file contents, and presents compact structured findings (severity, filePath, line range, title, body) in the Claude interface. No ADO credentials are required and no ADO API calls are made. Adds scripts/pre-pr.mjs with three pure helpers (parseChangedFilesFromDiff, shouldSkipFile, buildPrePrContext) and 32 new tests in tests/pre-pr.test.mjs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…prompts Closes issue 06 — compact sub-agent output guidance. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rator split Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add .agents/ directory to layout, remove monolithic-command claim, note ADO calls now live in the three focused agents. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds CHANGELOG entry covering orchestrator split, three focused agents (ADO Fetcher, Re-review Coordinator, ADO Writer), Pre-PR mode, and compact sub-agent output guidance. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d orchestrator
Static `import ... from <specifier>` only accepts a string literal — passing a
template literal or string-concatenation expression is a SyntaxError under
`node --input-type=module`, which is exactly how these snippets get executed.
Convert each call site to `const { X } = await import(...)` so the dynamic
specifier (built from `process.env.PLUGIN_R` / `CLAUDE_PLUGIN_ROOT`) is legal.
Call sites fixed:
- `.agents/ado-fetcher.md` Step 2 — `parseIterations` import
- `.agents/ado-fetcher.md` Step 5 — `parseWorkItemIds` import
- `.agents/ado-writer.md` Output — `parseAdoWriterResult` import; now also
round-trips the emitted block through the helper to fail fast on a malformed
result instead of leaving the import unused
- `commands/review-pr.md` Step 4 — `detectPriorReview` import (re-review mode)
- `commands/review-pr.md` Pre-PR mode — `buildPrePrContext` import
All 129 pr-review tests pass; `pnpm format` and `pnpm check` are clean.
…r portability
Replace the python3 heredoc that parses RAW_DIFF in re-review-coordinator with a
new pure Node helper `scripts/re-review/parse-diff-hunks.mjs` (TDD, 7 tests).
Resolves the cross-platform requirement violation: Windows native and minimal
Linux containers do not ship python3, and the project CLAUDE.md mandates Node.js
APIs over shell-language dependencies.
Replace every bare `/tmp/...` literal in ado-writer.md and re-review-coordinator.md
with `\${TMPDIR:-/tmp}/...` so temp files land in the OS-appropriate directory.
Drop the `.json` suffix from the mktemp templates because BSD mktemp on macOS
rejects suffixes after the X-placeholder run.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Honour the PRD acceptance criterion that the review-pr command file is ≤ 200 lines. The previous 297-line version inflated the parent-context token budget every invocation incurs, which is the very problem the orchestrator-split feature was created to solve. Three structural changes carried the trim: - Extract `Step 4` mode detection into a new pure helper `scripts/mode-detection.mjs` (`detectMode`, `formatModeEnv`) with unit tests covering first-review, re-review, partial-run, no-prior- iteration, and empty-thread cases. - Factor the duplicated first-review / re-review ADO Writer prompts into a single block parameterised by `MODE` and `SUMMARY_THREAD_ID`. - Consolidate the compact finding schema into one shared block (`### Compact finding schema`) referenced by both Step 6 and Pre-PR Step D, and realign the corresponding tests to assert against the shared schema block + each section's reference (removes the fragile Step-6/Step-D section-slice substring assertions flagged in PR review). Update plugin CLAUDE.md and CHANGELOG to state the real new line count and document the stable orchestrator floor (≤ 200 per PRD AC). The stale "skipped in Step 6" reference in CLAUDE.md is repointed to the `shouldSkipFile` helper in `scripts/pre-pr.mjs`, which is where the skip rules actually live after the refactor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-run paths PR #29 review remediation. Five targeted fixes to agent prompts and the orchestrator to convert silent failures into explicit errors — protecting the user-facing PRD contract ("post reviewer feedback to ADO") and the re-review state machine from corrupted state. - H1: ADO Writer Step 1 no longer increments FINDINGS_POSTED unconditionally after the threadContext fallback. Replaces the substring '"message"' heuristic with a structural check (exit code + numeric `id` parsed by Node); on confirmed failure logs the captured stderr and continues to the next finding instead of miscounting a missing post as success. - H2: ADO Writer Step 2 no longer swallows summary/delta POST failures. Captures exit code + parsed numeric `id`; on failure aborts the writer with a clear stderr message, because the completion marker and the next re-review's detection both depend on a valid SUMMARY_THREAD_ID — silent failure here corrupts re-review state forever. - H3: Orchestrator Step 4 no longer coerces `az repos pr thread list` failures to `[]`. Captures the exit separately; on non-zero exit emits a clear stderr error and exits 1, preventing a fetch failure from being mistaken for "no prior threads" and triggering a duplicate-post storm on re-review. Compensating prose cuts keep the orchestrator at 200 lines. - H4: Re-review Coordinator Step 3 partial-run check no longer conflates "marker missing" with "check crashed". Node heredoc wraps body in try/catch and exits with distinct codes (0 = found, 1 = not found, 2 = crash); bash branches on those codes and aborts the coordinator with exit 3 on a crash instead of silently downgrading to first-review mode and re-posting every prior thread. - H5: ADO Fetcher Step 4 branch-checkout fallback is now an executable `||` chain instead of a literal shell comment. If `az repos pr checkout` fails, the agent now actually runs the git fetch+checkout fallback and aborts with a clear stderr error if both fail. Tests: 142 passing. Orchestrator: 200 lines (cap). No new helpers required. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…after orchestrator split Seven small documentation cleanups surfaced by the comment-analyzer review of PR #29: - Renumber re-review-coordinator section headings so the inline cross-references that point to "Step 7 — Return result" resolve to the actual return-result heading (was Step 8). - State explicitly in the coordinator's Inputs section that PRIOR_ITERATION_ID is recomputed internally from RAW_THREADS_JSON rather than threaded in from the orchestrator. - Replace the misleading "fall back to first-review mode" prose in the coordinator's no-prior-threads and partial-run branches with what actually happens: the coordinator returns a zero-count result with freshFindings = FINDINGS and the orchestrator dispatch is unchanged. - Tell the coordinator's Step 6a reader that {finding.filePath} etc. are prompt-template placeholders to substitute, not bash variables. - Clarify in ADO Writer's zero-findings re-review branch that Step 3 still posts the completion marker on every successful run. - Flag LATEST_COMMIT_SHA in the ADO Fetcher output as reserved for future diff-range debugging — it is not consumed by any current downstream agent. - Drop the "PR title + description" claim from orchestrator Step 6, since the orchestrator does not parse those fields from the Fetcher output. The prose now reads "full diff and changed file contents" only, removing the contradiction with Step 5's parse list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three items deferred from the orchestrator-split PR's multi-agent review so they don't get lost: - pr-review-ado-error-hardening-pass — broader silent-failure pass across ADO Fetcher iterations / work-item fetch / diff-range fallback, the PATCH-to-fixed catch-all in Re-review Coordinator, the per-finding match parse, Pre-PR default-branch detection, and discriminated-union refactors for parseWorkItemIds / parseAdoWriterResult / parseIterations. - pr-review-prompt-content-tests-brittleness — the OR-chained substring assertions and section-slice approach in the test suite are fragile; replace with structured contract blocks or snapshot tests. - ci-test-job-missing-pr-review — .github/workflows/ci.yml's test matrix filter does not include pr-review, so every pr-review PR skips CI tests entirely. Also notes the unic-confluence/confluence-publish output-key typo alongside. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ction Biome flagged the broader @ts-ignore as suspicious (`lint/suspicious/noTsIgnore`). @ts-expect-error is the correct directive here because the type mismatch is real and intentional — `detectPriorReview` accepts the raw ADO thread shape while the helper's input is typed as `unknown[]`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the pr-review orchestrator split by refactoring review-pr.md into a thin mode-detecting orchestrator, introducing focused ADO sub-agents (fetch/write/re-review), adding Pre-PR mode, and standardizing compact JSON finding output. It also bumps pr-review to v1.0.0, updates changelog/docs, and adds several new triage/inbox notes.
Changes:
- Split orchestration into focused agents (
ado-fetcher,ado-writer,re-review-coordinator) with shared helper scripts and added unit tests. - Add Pre-PR mode (no ADO calls) and consolidate a shared “Compact finding schema” JSON contract for review-aspect agents.
- Version bump to
1.0.0, changelog updates, and issue/inbox documentation housekeeping.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/issues/pr-review-orchestrator-split/01-create-ado-fetcher-agent.md | Mark spec as resolved; checklist completed. |
| docs/issues/pr-review-orchestrator-split/02-create-ado-writer-agent.md | Mark spec as resolved; checklist completed. |
| docs/issues/pr-review-orchestrator-split/03-create-re-review-coordinator-agent.md | Mark spec as resolved. |
| docs/issues/pr-review-orchestrator-split/04-refactor-orchestrator.md | Mark spec as resolved. |
| docs/issues/pr-review-orchestrator-split/05-add-pre-pr-mode.md | Mark spec as resolved; checklist completed. |
| docs/issues/pr-review-orchestrator-split/06-compact-subagent-output.md | Mark spec as resolved. |
| docs/issues/pr-review-orchestrator-split/07-version-bump-and-release.md | Mark spec as resolved. |
| docs/issues/plugin-unic-prefix/PRD.md | Minor markdown formatting normalization. |
| docs/issues/github-copilot-config/PRD.md | Minor markdown formatting normalization. |
| docs/issues/conventional-commits-scopes/PRD.md | Minor markdown + table formatting normalization. |
| docs/issues/ci-node24-upgrade/PRD.md | Minor markdown formatting normalization. |
| docs/inbox/using-pr-review-on-active-ado-prs-wrongly.md | Add spacing in “What we still need” section. |
| docs/inbox/pr-review-request-user-confirmation-before.md | Add spacing in “What grilling needs” section. |
| docs/inbox/automate-qa-in-github.md | Add spacing in “What grilling needs” section. |
| docs/inbox/alternative-work-item-sources-for-doc-context.md | Add spacing in “Blocked on design decisions” section. |
| docs/inbox/alternative-doc-sources-for-doc-context.md | Add spacing in “Blocked on open design questions” section. |
| docs/inbox/add-github-support-to-pr-review.md | Add spacing in “What grilling needs” section. |
| docs/inbox/adapt-release-package-to-gitflow.md | Add spacing in “What grilling needs” section. |
| docs/inbox/pr-review-prompt-content-tests-brittleness.md | New inbox item documenting prompt-test brittleness. |
| docs/inbox/pr-review-ado-error-hardening-pass.md | New inbox item capturing deferred ADO hardening ideas. |
| docs/inbox/ci-test-job-missing-pr-review.md | New inbox item documenting CI test matrix omission. |
| apps/claude-code/pr-review/scripts/pre-pr.mjs | New Pre-PR helpers: skiplist + changed-file parsing + context builder. |
| apps/claude-code/pr-review/scripts/re-review/parse-diff-hunks.mjs | New diff hunk parser helper (pure, cross-platform). |
| apps/claude-code/pr-review/scripts/mode-detection.mjs | New helper to wrap re-review detection and format env output. |
| apps/claude-code/pr-review/scripts/ado-fetcher.mjs | New helper parsing iterations + work item IDs. |
| apps/claude-code/pr-review/scripts/ado-writer.mjs | New helper parsing the writer’s structured output block. |
| apps/claude-code/pr-review/tests/pre-pr.test.mjs | New unit tests for Pre-PR helpers + orchestrator content assertions. |
| apps/claude-code/pr-review/tests/parse-diff-hunks.test.mjs | New unit tests for diff hunk parsing. |
| apps/claude-code/pr-review/tests/mode-detection.test.mjs | New unit tests for mode detection + env formatting. |
| apps/claude-code/pr-review/tests/ado-fetcher.test.mjs | New unit tests for fetcher parsing + prompt content assertions. |
| apps/claude-code/pr-review/tests/ado-writer.test.mjs | New unit tests for writer output parsing + prompt content assertions. |
| apps/claude-code/pr-review/commands/review-pr.md | Major rewrite into thin orchestrator with Pre-PR mode + shared schema. |
| apps/claude-code/pr-review/.agents/ado-fetcher.md | New agent prompt for all ADO read operations and context block output. |
| apps/claude-code/pr-review/.agents/ado-writer.md | New agent prompt for all ADO write operations + structured result output. |
| apps/claude-code/pr-review/.agents/re-review-coordinator.md | New agent prompt owning the full re-review state machine. |
| apps/claude-code/pr-review/package.json | Bump to 1.0.0; expand pnpm test to run new test files. |
| apps/claude-code/pr-review/CLAUDE.md | Update repo layout + conventions to reflect orchestrator split. |
| apps/claude-code/pr-review/CHANGELOG.md | Document orchestrator split, helpers, and fixes; add 1.0.0 section. |
| apps/claude-code/pr-review/.claude-plugin/plugin.json | Version bump to 1.0.0. |
| apps/claude-code/pr-review/.claude-plugin/marketplace.json | Version bump to 1.0.0. |
Comments suppressed due to low confidence (1)
apps/claude-code/pr-review/scripts/pre-pr.mjs:60
parseChangedFilesFromDiffsplits on\nonly, so CRLF diffs will leave a trailing\ron matcheddiff --gitlines, producing paths like'/src/foo.ts\r'. Consider splitting with/\r?\n/(asparseDiffHunksdoes) or trimming the capture before prefixing with/.
export function parseChangedFilesFromDiff(diffText) {
if (!diffText) return []
const seen = new Set()
const paths = []
for (const line of diffText.split('\n')) {
const m = line.match(/^diff --git a\/.*? b\/(.+)$/)
if (m) {
const filePath = `/${m[1]}`
if (!seen.has(filePath)) {
seen.add(filePath)
paths.push(filePath)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) | ||
| NEW_THREAD_COUNT=$((NEW_THREAD_COUNT + 1)) | ||
| DEFAULT_BRANCH=$(git remote show origin 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}' || echo "main") |
Member
Author
There was a problem hiding this comment.
Fixed in 042da64. The pipeline now reads git remote show origin 2>/dev/null | awk '/HEAD branch/{print $NF}' | grep . || echo "main" — grep . filters empty awk output, so the || echo "main" fallback fires when there's no HEAD branch: line.
| if (basename.toLowerCase().startsWith('generated-types.')) return true | ||
|
|
||
| // files under a generated/ directory segment | ||
| if (filePath.includes('/generated/')) return true |
Member
Author
There was a problem hiding this comment.
Fixed in 042da64. shouldSkipFile now uses the lower-cased path (lower.includes('/generated/')) for the directory check too, so paths like /Source/Generated/ApiClient.cs are skipped consistently. Added a test covering the capitalised .NET-style path.
Four targeted fixes from the Copilot review on PR #29: - K1 (commands/review-pr.md Step A) — Pre-PR default-branch detection silently left `DEFAULT_BRANCH` empty when `git remote show origin` produced no `HEAD branch:` line, because the awk pipeline returns exit 0 with empty output and the `|| echo "main"` fallback never fires. Filter awk output through `grep .` so an empty result triggers the fallback as intended. - K2 (scripts/pre-pr.mjs:35) — `shouldSkipFile` lower-cased the path for extension checks but ran the `/generated/` directory check against the original `filePath`. Paths like `/Source/Generated/ApiClient.cs` were skipped only via the `.g.cs` extension rule, leaving generated `.ts` / `.cs` files under capitalised directories unskipped. Now uses the lowered path for the directory check too. - K4 (CHANGELOG.md) — `[Unreleased]` carried entries that describe the same release this PR is version-bumping to 1.0.0, risking ambiguous release notes and double-shipping. Moved every remediation Added / Changed / Fixed bullet into the `[1.0.0]` section so the whole PR ships as a coherent v1.0.0 release. - K5 (scripts/pre-pr.mjs:54) — `parseChangedFilesFromDiff` split on `\n` only, leaving a trailing `\r` on every captured path for diffs coming from Windows Git with `core.autocrlf=true`. Switched to `/\r?\n/`, matching the sibling `parseDiffHunks` helper. Added one test for each behaviour-changing fix: - `shouldSkipFile('/Source/Generated/ApiClient.cs') === true` - `parseChangedFilesFromDiff` against a CRLF-formatted diff returns clean paths. K3 (parseIterations defaults to iterationId=1 on empty input) was deferred — already captured in `docs/inbox/pr-review-ado-error-hardening-pass.md` as part of the broader ADO Fetcher silent-failure hardening pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature
docs/issues/pr-review-orchestrator-split/PRD.mdResolved issues
docs/issues/pr-review-orchestrator-split/01-create-ado-fetcher-agent.mddocs/issues/pr-review-orchestrator-split/02-create-ado-writer-agent.mddocs/issues/pr-review-orchestrator-split/03-create-re-review-coordinator-agent.mddocs/issues/pr-review-orchestrator-split/04-refactor-orchestrator.mddocs/issues/pr-review-orchestrator-split/05-add-pre-pr-mode.mddocs/issues/pr-review-orchestrator-split/06-compact-subagent-output.mddocs/issues/pr-review-orchestrator-split/07-version-bump-and-release.md🤖 Implemented by the Feature Runner via
/implement-feature pr-review-orchestrator-split