Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

The root cause is architectural: `review-pr.md` conflates orchestration (which mode are we in? what agents to launch?) with platform integration (fetch ADO threads, post inline comments) and re-review state management (classify threads, match findings, reply).

The GitHub PR review workflow (`.claude/prompts/pr-review-workflow.prompt.md`) solves the same orchestration problem in ~80 lines by staying a thin coordinator and delegating everything else to focused agents. That pattern is the right model for `review-pr.md`.
The right model for `review-pr.md` is a thin coordinator: prerequisites block, mode detection block, and one delegation block per mode. The three focused agents own all data-fetch and write-back ADO operations. The one allowed inline ADO call is the mode-detection `az repos pr thread list` in the mode detection block — an orchestration concern, not a data-fetch or write-back operation; no `az devops invoke` commands remain in the orchestrator.

Comment thread
orioltf marked this conversation as resolved.
## Decision

Expand All @@ -25,12 +25,12 @@ Refactor `review-pr.md` into a **thin orchestrator** of ~200 lines that:
Three focused agents live in the plugin's `.agents/` directory (not in `pr-review-toolkit`, which is a read-only dependency):

- **`pr-review:ado-fetcher`** — fetches PR metadata, iterations, changed files, and raw diff from ADO. Used by first-review and re-review modes.
- **`pr-review:re-review-coordinator`** — owns Steps 3.5–10-Path-B: prior thread detection, partial-run check, thread classification, finding matching, and reply posting to classified threads. Used only in re-review mode.
- **`pr-review:re-review-coordinator`** — owns prior thread detection, partial-run check, thread classification, finding matching, and reply posting to classified threads. Used only in re-review mode.
- **`pr-review:ado-writer`** — owns the ADO write-back pipeline: posting inline threads, patching thread status, and posting the summary comment. Used by first-review and re-review modes.

Pre-PR mode skips the ADO fetcher and writer entirely; it goes straight from the orchestrator to the `pr-review-toolkit` review agents and presents findings locally.

**Compact sub-agent output.** Review agents (`pr-review-toolkit:code-reviewer`, etc.) are asked via the Step 8 prompt in `review-pr.md` to return structured findings (`severity`, `filePath`, `startLine`, `endLine`, `title`, `body`) rather than prose with embedded code quotes. This keeps what flows back into the parent context small. This guidance stays in `review-pr.md`'s prompt, not in the toolkit agent definitions, because `pr-review-toolkit` is not owned by this plugin.
**Compact sub-agent output.** Review agents (`pr-review-toolkit:code-reviewer`, etc.) are asked via the review-agent launch step in `review-pr.md` to return structured findings (`severity`, `filePath`, `startLine`, `endLine`, `title`, `body`) rather than prose with embedded code quotes. This keeps what flows back into the parent context small. This guidance stays in `review-pr.md`'s prompt, not in the toolkit agent definitions, because `pr-review-toolkit` is not owned by this plugin.

**Re-review logic ownership.** The four Node.js modules in `scripts/re-review/` are already algorithmically platform-agnostic; only their input shapes are ADO-specific. When a second write-back platform (GitHub) is built, normalising to a canonical thread shape and lifting these modules to `pr-review-toolkit` is the correct move. That work is deferred until a second platform consumer exists.

Expand All @@ -53,5 +53,3 @@ _Option B: re-review coordinator as a procedural agent_ — keep re-review logic

- `docs/issues/pr-review-orchestrator-split/PRD.md` for the feature PRD and implementation issues that deliver this split
- ADR 0008 (soft dependency on `pr-review-toolkit`)
- `.claude/prompts/pr-review-workflow.prompt.md` (the GitHub orchestrator pattern this
mirrors)
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@

## What to build

Create a new plugin agent (`pr-review:ado-fetcher`) that encapsulates all Azure DevOps read operations required for a PR review. The agent receives a PR URL (org, project, PR ID) and returns a structured context block containing: PR metadata (title, description, source/target branches, repo ID), latest iteration ID and its commit SHA, prior commit SHA (passed in for re-review, empty for first-review), changed files list, raw diff, and work-item IDs linked to the PR.
Create a new plugin agent (`pr-review:ado-fetcher`) that encapsulates all Azure DevOps read operations required for a PR review. The agent receives a PR URL (org, project, PR ID) and an optional prior iteration ID (passed in for re-review, empty string for first-review), and returns a structured context block containing: PR metadata (title, description, source/target branches, repo ID), latest iteration ID and its commit SHA, changed files list, raw diff, and work-item IDs linked to the PR.

This agent replaces the inline ADO shell commands currently scattered across Steps 2–5 of the `review-pr` command. It is invoked by first-review and re-review modes; pre-PR mode never calls it.

The ADO Fetcher and the Doc Context Orchestrator agent must be invocable concurrently — the ADO Fetcher provides the work-item IDs that the Doc Context Orchestrator needs, so the Fetcher runs first, but the Fetcher and Doc Context Orchestrator may overlap in wall-clock time.
The ADO Fetcher is a prerequisite for the Doc Context Orchestrator — the Fetcher must complete first because its output (work-item IDs) is the input the Doc Context Orchestrator needs. Once the Fetcher returns, the Doc Context Orchestrator and review aspect agents launch concurrently with each other.

## Acceptance criteria

Expand Down Expand Up @@ -45,7 +45,7 @@ A new plugin agent (`pr-review:ado-fetcher`) accepts PR URL components and retur

**Key interfaces:**

- Input: org URL, project, PR ID, optional prior commit SHA (passed in for re-review)
- Input: org URL, project, PR ID, optional prior iteration ID (passed in for re-review)
- Output: structured context block — PR metadata, latest iteration ID, latest commit SHA, changed files list, raw diff, work-item IDs list
- The agent must handle zero-iteration PRs and already-merged PRs gracefully

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,31 +9,31 @@

## What to build

Create a new plugin agent (`pr-review:re-review-coordinator`) that owns the full re-review state machine. The agent receives the ADO Fetcher context block, the raw prior-threads JSON, and the diff hunks file path.
Create a new plugin agent (`pr-review:re-review-coordinator`) that owns the full re-review state machine. The agent receives the ADO Fetcher context block (which includes the raw diff) and the raw full PR threads JSON (the unfiltered ADO thread list). It parses the raw diff into diff hunks internally before calling `classify-thread` — the hunks file is a temp artefact managed inside the agent, not an input from the orchestrator.

It performs in order:

1. Calls the `detect-prior-review` Node.js module to identify prior bot threads and locate the summary thread.
2. Runs the partial-run check (looks for the completion marker for the prior iteration in the summary thread). Falls back to first-review mode if the marker is absent.
3. If no new commits exist since the prior review (prior commit SHA equals latest commit SHA), prints outstanding pending threads to the console and exits early — no ADO writes.
3. If no new revisions exist since the prior review (prior iteration ID equals latest iteration ID), prints outstanding pending threads to the console and exits early — no ADO writes.
4. Calls `classify-thread` on each prior thread against the diff hunks.
5. For each new finding passed in, calls `match-finding` to look for a matching prior thread.
6. Based on classification, posts replies to prior threads: acknowledges disputes, confirms resolutions (and PATCHes thread status to fixed), adds new evidence to pending threads with new information, skips pending threads with no new evidence, ignores obsolete threads.
7. Returns the classification counts (new, addressed, disputed, pending) and the updated findings list (unmatched findings pass through as fresh; matched findings are consumed).
7. Returns the classification counts (addressed, disputed, pending, obsolete), the fresh findings list (`freshFindings[]` — only unmatched findings; matched findings are consumed and not returned), and an `earlyExit` flag. `earlyExit` is `true` only on the no-new-revisions path (step 3); it is `false` on all other paths including normal completion with zero fresh findings.

The four Node.js modules (`detect-prior-review`, `classify-thread`, `match-finding`, `parse-signature`) remain in `scripts/re-review/` unchanged. This agent calls them via `node --input-type=module` inline scripts, exactly as the current `review-pr.md` does.

## Acceptance criteria

- [ ] The agent correctly detects prior bot threads using the `detect-prior-review` module
- [ ] The agent falls back to first-review mode when no completion marker is found for the prior iteration
- [ ] The agent exits early (console output only, no ADO writes) when prior and latest commit SHAs are identical
- [ ] The agent exits early (console output only, no ADO writes) when prior and latest iteration IDs are identical, and returns `earlyExit: true`
- [ ] The agent classifies all prior threads using the `classify-thread` module
- [ ] The agent matches new findings to prior threads using the `match-finding` module with ±3-line drift tolerance
- [ ] The agent posts a dispute acknowledgement reply to disputed threads including the ADO nudge
- [ ] The agent posts a resolution confirmation reply and PATCHes status to fixed for addressed threads
- [ ] The agent posts a new-evidence reply to pending threads that have new analysis; skips pending threads with no new evidence
- [ ] The agent returns classification counts and the unmatched (fresh) findings list
- [ ] The agent returns classification counts (addressed, disputed, pending, obsolete), the unmatched (fresh) findings list, and the `earlyExit` flag
- [ ] The existing re-review module unit tests (`detect-prior-review`, `classify-thread`, `match-finding`, `parse-signature`) pass unchanged

## Blocked by
Expand All @@ -53,24 +53,25 @@ None — can start immediately.
The re-review state machine (prior thread detection, partial-run check, Thread Classification, finding matching, reply/resolution posting) lives inline in `review-pr.md` across Steps 3.5–10-Path-B. It is loaded on every invocation regardless of mode.

**Desired behavior:**
A new plugin agent (`pr-review:re-review-coordinator`) receives the ADO Fetcher context block, raw prior-threads JSON, and diff hunks. It runs the full re-review state machine, posts classified replies directly to ADO, and returns classification counts plus the list of unmatched (fresh) findings for the ADO Writer to post as new threads. The four existing Node.js modules (`detect-prior-review`, `classify-thread`, `match-finding`, `parse-signature`) are called from this agent unchanged.
A new plugin agent (`pr-review:re-review-coordinator`) receives the ADO Fetcher context block (which includes the raw diff) and the raw full PR threads JSON (unfiltered). It calls `detect-prior-review` internally to identify bot threads, parses the raw diff into diff hunks internally, then runs the full re-review state machine, posts classified replies directly to ADO, and returns classification counts plus the list of unmatched (fresh) findings for the ADO Writer to post as new threads. The four existing Node.js modules (`detect-prior-review`, `classify-thread`, `match-finding`, `parse-signature`) are called from this agent unchanged.

**Key interfaces:**

- Input: ADO Fetcher context block, prior-threads JSON (from `detect-prior-review`), diff hunks JSON, new findings list, Bot Signature prefix constant
- Output: `{ addressed, disputed, pending, freshFindings[] }` — fresh findings are those with no matching prior thread
- Input: ADO Fetcher context block (includes raw diff), raw full PR threads JSON (captured by the orchestrator during mode detection via `az repos pr thread list` — not re-fetched; `detect-prior-review` filters this list inside the Coordinator), new findings list, Bot Signature prefix constant
- The Coordinator parses the raw diff into diff hunks internally; this is not an orchestrator concern
- Output: `{ addressed, disputed, pending, obsolete, freshFindings[], earlyExit }` — fresh findings are those with no matching prior thread; `earlyExit: true` signals the no-new-revisions path to the orchestrator
- The agent calls the four Node.js modules via `node --input-type=module` inline scripts (same pattern as current `review-pr.md`)
- Early-exit path: when prior commit SHA equals latest commit SHA, prints pending threads to console and returns with empty fresh findings — no ADO writes
- Early-exit path: when prior iteration ID equals latest iteration ID, prints pending threads to console and returns `{ earlyExit: true, freshFindings: [], addressed: 0, disputed: 0, pending: N, obsolete: 0 }` — all count fields are always present; the orchestrator must skip the ADO Writer entirely when `earlyExit: true`

**Acceptance criteria:**

- [ ] The agent correctly detects prior bot threads using the `detect-prior-review` module
- [ ] The agent falls back to first-review mode when no completion marker is found for the prior iteration
- [ ] The agent exits early when prior and latest commit SHAs are identical (console output only, no ADO writes)
- [ ] The agent exits early when prior and latest iteration IDs are identical (console output only, no ADO writes), returning `earlyExit: true`
- [ ] The agent classifies all prior threads using the `classify-thread` module
- [ ] The agent matches new findings to prior threads using `match-finding` with ±3-line drift tolerance
- [ ] The agent posts dispute acknowledgement, resolution confirmation, and new-evidence replies appropriately
- [ ] The agent returns classification counts and unmatched fresh findings
- [ ] The agent returns classification counts (addressed, disputed, pending, obsolete), the unmatched fresh findings list, and the `earlyExit` flag
- [ ] The four re-review module unit tests pass unchanged after this issue is implemented

**Out of scope:**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ Refactor `review-pr.md` into a thin orchestrator of approximately 200 lines. The

1. Validates prerequisites in a mode-aware way: always checks `git` availability and `pr-review-toolkit`; checks Azure CLI and `azure-devops` extension only when a PR URL is present (Pre-PR mode requires no ADO credentials).
2. Parses `$ARGUMENTS` for a PR URL. If absent, sets mode to Pre-PR; if present, proceeds to detection.
3. For PR URL cases: makes a lightweight ADO thread-list call directly (not via the ADO Fetcher) to check for a prior Bot Signature, determining mode and extracting the prior commit SHA if found; then invokes the ADO Fetcher agent (passing the prior commit SHA for re-review runs).
3. For PR URL cases: makes a lightweight ADO thread-list call directly (not via the ADO Fetcher) using `az repos pr thread list` (not `az devops invoke`) to check for a prior Bot Signature, determining mode and extracting the prior iteration ID if found. The full thread list from this call is captured and passed forward to the Re-review Coordinator in step 6 — no second ADO thread-list call is made.
4. Logs the detected mode clearly before delegating.
5. For First-review: runs Doc Context Orchestrator + review aspect agents in parallel, collects compact findings, delegates write-back to the ADO Writer agent.
6. For Re-review: runs Doc Context Orchestrator + review aspect agents in parallel, passes findings and prior-thread data to the Re-review Coordinator agent (which handles replies), then passes remaining fresh findings to the ADO Writer agent.
5. For First-review: invokes the ADO Fetcher agent (passing org URL, project, PR ID), then runs Doc Context Orchestrator + review aspect agents in parallel, collects compact findings, delegates write-back to the ADO Writer agent.
6. For Re-review: invokes the ADO Fetcher agent (passing org URL, project, PR ID, and prior iteration ID), then runs Doc Context Orchestrator + review aspect agents in parallel. Once all review aspect agents return their findings, passes the complete findings list and prior-thread data to the Re-review Coordinator agent (which handles replies). If the Coordinator returns `earlyExit: true` (no new revisions), the orchestrator stops — ADO Writer is not called. Otherwise passes fresh findings to the ADO Writer agent.
7. Pre-PR mode is a stub at this slice — it detects the mode and prints a "Pre-PR mode not yet implemented" message. Full Pre-PR behaviour is delivered in issue 05.

The `review-pr.md` file must contain no `az devops invoke` shell commands after this refactor — all ADO operations live in the three focused agents. The Bot Signature constants and detection prefix are unchanged. All existing re-review module unit tests must pass.
The `review-pr.md` file must contain no `az devops invoke` shell commands after this refactor — the three focused agents own all data-fetch and write-back ADO operations. The one allowed inline ADO call is the mode-detection `az repos pr thread list` in step 3, which is an orchestration concern, not a data-fetch or write-back operation. The Bot Signature constants and detection prefix are unchanged. All existing re-review module unit tests must pass.

## Acceptance criteria

Expand Down Expand Up @@ -57,12 +57,12 @@ The `review-pr.md` file must contain no `az devops invoke` shell commands after

**Key interfaces:**

- Mode detection sequence: no URL → Pre-PR; URL → orchestrator makes a lightweight ADO thread-list call → no Bot Signature → First-review; Bot Signature found → extract prior commit SHA → Re-review
- Mode detection sequence: no URL → Pre-PR; URL → orchestrator calls `az repos pr thread list` (not `az devops invoke`) → no Bot Signature → First-review; Bot Signature found → extract prior iteration ID → Re-review
- Bot Signature detection prefix: `🤖 *Reviewed by Claude Code*` — must not change
- ADO Fetcher agent invocation: passes org URL, project, PR ID
- Re-review Coordinator agent invocation (re-review only): passes ADO Fetcher context + new findings list
- ADO Writer agent invocation: passes PR context + fresh findings list + mode flag
- The GitHub prompt (`.claude/prompts/pr-review-workflow.prompt.md`) is the structural reference for what the orchestrator should look like
- ADO Fetcher agent invocation: passes org URL, project, PR ID (plus prior iteration ID in re-review)
- Re-review Coordinator agent invocation (re-review only): called after all review aspect agents complete; passes ADO Fetcher context + full PR threads JSON (captured from mode detection in step 3, not re-fetched) + new findings list; returns `{ earlyExit, freshFindings[], addressed, disputed, pending, obsolete }`
- If Coordinator returns `earlyExit: true`, orchestrator stops — ADO Writer is not called
- ADO Writer agent invocation: passes PR context + fresh findings list + mode (`"first-review"` | `"re-review"`); the mode determines whether ADO Writer posts a new Review Summary (first-review) or a delta reply (re-review)

**Acceptance criteria:**

Expand Down
Loading
Loading