From 14a3e4324f861cc3c6bbf289f8fe9b3b7198e7b6 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 12 May 2026 14:54:40 +0200 Subject: [PATCH 01/16] feat(pr-review): refactor review-pr.md to thin orchestrator (~199 lines) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../pr-review/.agents/ado-fetcher.md | 242 +++++ .../pr-review/.agents/ado-writer.md | 308 ++++++ .../.agents/re-review-coordinator.md | 444 ++++++++ .../pr-review/commands/review-pr.md | 988 ++---------------- apps/claude-code/pr-review/package.json | 2 +- .../pr-review/scripts/ado-fetcher.mjs | 37 + .../pr-review/scripts/ado-writer.mjs | 29 + .../pr-review/tests/ado-fetcher.test.mjs | 152 +++ .../pr-review/tests/ado-writer.test.mjs | 196 ++++ .../01-create-ado-fetcher-agent.md | 26 +- .../02-create-ado-writer-agent.md | 18 +- .../03-create-re-review-coordinator-agent.md | 2 +- .../04-refactor-orchestrator.md | 2 +- 13 files changed, 1529 insertions(+), 917 deletions(-) create mode 100644 apps/claude-code/pr-review/.agents/ado-fetcher.md create mode 100644 apps/claude-code/pr-review/.agents/ado-writer.md create mode 100644 apps/claude-code/pr-review/.agents/re-review-coordinator.md create mode 100644 apps/claude-code/pr-review/scripts/ado-fetcher.mjs create mode 100644 apps/claude-code/pr-review/scripts/ado-writer.mjs create mode 100644 apps/claude-code/pr-review/tests/ado-fetcher.test.mjs create mode 100644 apps/claude-code/pr-review/tests/ado-writer.test.mjs diff --git a/apps/claude-code/pr-review/.agents/ado-fetcher.md b/apps/claude-code/pr-review/.agents/ado-fetcher.md new file mode 100644 index 0000000..43b8de3 --- /dev/null +++ b/apps/claude-code/pr-review/.agents/ado-fetcher.md @@ -0,0 +1,242 @@ +--- +allowed-tools: ['Bash'] +description: 'Fetch all Azure DevOps read data required for a PR review: PR metadata, latest iteration, changed files, raw diff, and linked work-item IDs. Read-only — no write operations.' +--- + +# ADO Fetcher + +You fetch all Azure DevOps data required for a PR review and return a structured context block. You make no write operations — this agent is purely read-only. + +You receive all required context in this prompt as literal strings. Do not read environment variables — agents do not inherit them. + +--- + +## Inputs + +You receive: + +- `ORG_URL` — the Azure DevOps organisation URL (e.g. `https://dev.azure.com/myorg`) +- `PROJECT` — the ADO project name +- `PR_ID` — the pull request ID (integer as string) +- `PRIOR_ITERATION_ID` — the iteration ID from the prior review (integer as string, or empty string for first-review) +- `PLUGIN_ROOT` — absolute path to this plugin's directory (for Node.js helper scripts) + +--- + +## Step 1 — Fetch PR metadata + +```bash +az repos pr show --id {PR_ID} --org {ORG_URL} --output json +``` + +Capture and remember: + +- `repository.id` → `REPO_ID` +- `repository.project.name` → `PROJECT` (update if it differs from the input) +- `sourceRefName` → `SOURCE_REF` (e.g. `refs/heads/feature/my-branch`) +- `targetRefName` → `TARGET_REF` (e.g. `refs/heads/develop`) +- `title` → `PR_TITLE` +- `description` → `PR_DESCRIPTION` +- `status` — note if already merged (`mergeStatus: succeeded`); continue without error — comments are still useful as a review record + +Strip `refs/heads/` prefix from `SOURCE_REF` and `TARGET_REF` to get plain branch names (`SOURCE_BRANCH`, `TARGET_BRANCH`). + +--- + +## Step 2 — Fetch PR iterations and resolve latest + +```bash +ITERATIONS_JSON=$(az devops invoke \ + --area git \ + --resource pullRequestIterations \ + --route-parameters "project=$PROJECT" "repositoryId=$REPO_ID" "pullRequestId=$PR_ID" \ + --org "$ORG_URL" \ + --api-version "7.1" \ + --output json) +``` + +Parse via the helper script — handles the zero-iteration case gracefully: + +```bash +ITER_RESULT=$( + ITERATIONS_JSON_STR="$ITERATIONS_JSON" \ + PLUGIN_R="$PLUGIN_ROOT" \ + node --input-type=module << 'EOJS' +import { parseIterations } from `file://${process.env.PLUGIN_R}/scripts/ado-fetcher.mjs` +const value = JSON.parse(process.env.ITERATIONS_JSON_STR).value ?? [] +const result = parseIterations(value) +process.stdout.write(JSON.stringify(result)) +EOJS +) + +LATEST_ITERATION_ID=$(echo "$ITER_RESULT" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).latestIterationId))") +LATEST_COMMIT_SHA=$(echo "$ITER_RESULT" | node -e "process.stdout.write(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).latestCommitSha)") +``` + +If `LATEST_ITERATION_ID` resolves to `1` and iterations were empty, log: + +``` +Warning: no iterations returned — defaulting to iteration 1 +``` + +--- + +## Step 3 — List changed files + +```bash +az devops invoke \ + --area git \ + --resource pullRequestIterationChanges \ + --route-parameters "repositoryId=$REPO_ID" "pullRequestId=$PR_ID" "iterationId=$LATEST_ITERATION_ID" \ + --org "$ORG_URL" \ + --api-version "7.1" \ + --output json +``` + +Extract file paths and change types: + +```bash +CHANGED_FILES=$(az devops invoke \ + --area git \ + --resource pullRequestIterationChanges \ + --route-parameters "repositoryId=$REPO_ID" "pullRequestId=$PR_ID" "iterationId=$LATEST_ITERATION_ID" \ + --org "$ORG_URL" \ + --api-version "7.1" \ + --output json | node -e " +const chunks = [] +process.stdin.on('data', c => chunks.push(c)) +process.stdin.on('end', () => { + const data = JSON.parse(Buffer.concat(chunks).toString()) + for (const c of data.changeEntries ?? []) { + const path = c.item?.path ?? '' + const ct = c.changeType ?? '' + process.stdout.write(ct + ': ' + path + '\n') + } +}) +") +``` + +--- + +## Step 4 — Get the raw diff + +Check whether the local branch matches the PR source branch: + +```bash +git branch --show-current +``` + +If it does not match, check out the PR branch: + +```bash +az repos pr checkout --id "$PR_ID" --org "$ORG_URL" +# fallback: git fetch origin "$SOURCE_BRANCH" && git checkout "$SOURCE_BRANCH" +``` + +If `PRIOR_ITERATION_ID` is non-empty, determine the incremental diff range. Fetch the prior iteration's commit SHA from the iterations list: + +```bash +PRIOR_COMMIT_SHA=$(echo "$ITERATIONS_JSON" | node -e " +const chunks = [] +process.stdin.on('data', c => chunks.push(c)) +process.stdin.on('end', () => { + const id = Number(process.env.PRIOR_ITER_ID) + const value = JSON.parse(Buffer.concat(chunks).toString()).value ?? [] + const it = value.find(v => v.id === id) + process.stdout.write(it?.sourceRefCommit?.commitId ?? '') +}) +" PRIOR_ITER_ID="$PRIOR_ITERATION_ID") +``` + +### Diff strategy + +Branch on whether `PRIOR_ITERATION_ID` is set and whether commits are available: + +**First-review (`PRIOR_ITERATION_ID` empty) or fallback:** + +```bash +RAW_DIFF=$(git diff "origin/${TARGET_BRANCH}...HEAD") +``` + +**Re-review with resolvable prior commit (`PRIOR_COMMIT_SHA` non-empty, differs from `LATEST_COMMIT_SHA`):** + +```bash +if git fetch origin "$PRIOR_COMMIT_SHA" 2>/dev/null; then + RAW_DIFF=$(git diff "${PRIOR_COMMIT_SHA}..${LATEST_COMMIT_SHA}") +else + echo "Warning: prior commit $PRIOR_COMMIT_SHA unreachable — falling back to full diff." + RAW_DIFF=$(git diff "origin/${TARGET_BRANCH}...HEAD") +fi +``` + +**Re-review with no new commits (`PRIOR_COMMIT_SHA == LATEST_COMMIT_SHA`):** + +```bash +echo "No new commits since last review." +RAW_DIFF="" +``` + +--- + +## Step 5 — Fetch linked work-item IDs + +```bash +WI_RESPONSE=$(az devops invoke \ + --area git \ + --resource pullRequestWorkItems \ + --route-parameters "repositoryId=$REPO_ID" "pullRequestId=$PR_ID" \ + --org "$ORG_URL" \ + --api-version "7.1" \ + --output json 2>/dev/null) || WI_RESPONSE="" +``` + +Parse with the helper script — returns an empty array on failure: + +```bash +WORK_ITEM_IDS=$( + WI_RESP="$WI_RESPONSE" \ + PLUGIN_R="$PLUGIN_ROOT" \ + node --input-type=module << 'EOJS' +import { parseWorkItemIds } from `file://${process.env.PLUGIN_R}/scripts/ado-fetcher.mjs` +const response = process.env.WI_RESP ? JSON.parse(process.env.WI_RESP) : null +const ids = parseWorkItemIds(response) +process.stdout.write(JSON.stringify(ids)) +EOJS +) +``` + +--- + +## Output + +Return the following structured context block as your final output. Fill in all values gathered above. This block is consumed verbatim by the orchestrator and downstream agents: + +``` +ADO_FETCHER_RESULT_START +ORG_URL: {ORG_URL} +PROJECT: {PROJECT} +PR_ID: {PR_ID} +REPO_ID: {REPO_ID} +PR_TITLE: {PR_TITLE} +PR_DESCRIPTION: +{PR_DESCRIPTION} +SOURCE_BRANCH: {SOURCE_BRANCH} +TARGET_BRANCH: {TARGET_BRANCH} +LATEST_ITERATION_ID: {LATEST_ITERATION_ID} +LATEST_COMMIT_SHA: {LATEST_COMMIT_SHA} +WORK_ITEM_IDS: {WORK_ITEM_IDS} + +CHANGED_FILES: +{CHANGED_FILES} + +RAW_DIFF: +{RAW_DIFF} +ADO_FETCHER_RESULT_END +``` + +Where: +- `WORK_ITEM_IDS` is the JSON array from Step 5, e.g. `[42, 7]` or `[]` +- `CHANGED_FILES` is the newline-separated list from Step 3, e.g. `edit: /src/api.ts` +- `RAW_DIFF` is the full diff text from Step 4 (may be empty if no new commits) + +**Never add any ADO write operations (POST, PATCH, DELETE) to this agent.** diff --git a/apps/claude-code/pr-review/.agents/ado-writer.md b/apps/claude-code/pr-review/.agents/ado-writer.md new file mode 100644 index 0000000..72ba1da --- /dev/null +++ b/apps/claude-code/pr-review/.agents/ado-writer.md @@ -0,0 +1,308 @@ +--- +allowed-tools: ['Bash'] +description: 'Post all Azure DevOps write-back operations for a PR review: inline comment threads per finding, Review Summary or delta reply, and completion marker. Write-only — no read operations.' +--- + +# ADO Writer + +You post all Azure DevOps comments for a PR review and return a structured result block. You make no read operations — this agent is purely write-only. + +You receive all required context in this prompt as literal strings. Do not read environment variables — agents do not inherit them. + +--- + +## Inputs + +You receive: + +- `ORG_URL` — the Azure DevOps organisation URL (e.g. `https://dev.azure.com/myorg`) +- `PROJECT` — the ADO project name +- `REPO_ID` — the repository UUID (e.g. `99bf5e9b-...`) +- `PR_ID` — the pull request ID (integer as string) +- `LATEST_ITERATION_ID` — the latest PR iteration ID (integer as string) +- `SUMMARY_THREAD_ID` — the existing summary thread ID from a prior review, or empty string for first-review +- `MODE` — `first-review` or `re-review` +- `PLUGIN_ROOT` — absolute path to this plugin's directory (for Node.js helper scripts) +- `FINDINGS` — a JSON array of compact findings: `{ severity, filePath, startLine, endLine, title, body }[]` + +--- + +## Constants + +```bash +SIGNATURE_PREFIX="🤖 *Reviewed by Claude Code*" +SIGNATURE="🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}" +FINDINGS_POSTED=0 +``` + +Every comment posted — inline or summary — **must** end with this trailer: + +``` +--- +🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID} +``` + +--- + +## Step 1 — Post inline comment threads + +For each finding in `FINDINGS`, post one new Inline Comment thread to ADO at the correct file path and line range. + +Use a unique temp file per comment (e.g. `/tmp/ado_writer_thread_1.json`, `_2.json`, etc.). + +```bash +cat > /tmp/ado_writer_thread_N.json << 'ENDJSON' +{ + "comments": [ + { + "commentType": 1, + "content": "{SEVERITY_EMOJI} **{title}**\n\n{body}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}" + } + ], + "status": 1, + "threadContext": { + "filePath": "{filePath}", + "rightFileEnd": { "line": {endLine}, "offset": 1 }, + "rightFileStart": { "line": {startLine}, "offset": 1 } + } +} +ENDJSON + +THREAD_RESPONSE=$(az devops invoke \ + --area git \ + --resource pullRequestThreads \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/ado_writer_thread_N.json \ + --api-version "7.1" \ + --output json 2>/tmp/ado_writer_thread_N.err) +THREAD_EXIT=$? +``` + +Map severity to emoji before writing the content: + +- `critical` → `🔴` +- `important` → `🟠` +- `minor` → `🟡` +- any other value → use as-is + +### threadContext rejection fallback + +If the `az devops invoke` call fails (non-zero exit) or the response contains an error related to `threadContext` (file not in diff, invalid path), **retry without `threadContext`** to post as a general comment: + +```bash +if [ $THREAD_EXIT -ne 0 ] || echo "$THREAD_RESPONSE" | grep -qi '"message"'; then + cat > /tmp/ado_writer_thread_N_fallback.json << 'ENDJSON' + { + "comments": [ + { + "commentType": 1, + "content": "{SEVERITY_EMOJI} **{title}** ({filePath}:{startLine})\n\n{body}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}" + } + ], + "status": 1 + } +ENDJSON + + THREAD_RESPONSE=$(az devops invoke \ + --area git \ + --resource pullRequestThreads \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/ado_writer_thread_N_fallback.json \ + --api-version "7.1" \ + --output json) +fi +``` + +After each successful post (primary or fallback): + +```bash +FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) +echo "Thread posted: $(echo "$THREAD_RESPONSE" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))")" +``` + +--- + +## Step 2 — Post Review Summary or delta reply + +Branch on `MODE` and the `SUMMARY_THREAD_ID` value. + +--- + +### MODE=first-review — Post full Review Summary + +Post one general thread **without** `threadContext`: + +```bash +cat > /tmp/ado_writer_summary.json << 'ENDJSON' +{ + "comments": [ + { + "commentType": 1, + "content": "## PR Review Summary\n\n{SUMMARY_CONTENT}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}" + } + ], + "status": 1 +} +ENDJSON + +SUMMARY_RESPONSE=$(az devops invoke \ + --area git \ + --resource pullRequestThreads \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/ado_writer_summary.json \ + --api-version "7.1" \ + --output json) + +SUMMARY_THREAD_ID=$(echo "$SUMMARY_RESPONSE" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))") +echo "Summary thread posted: ${SUMMARY_THREAD_ID}" +``` + +The `{SUMMARY_CONTENT}` must be structured as: + +```markdown +### 🔴 Critical (X found) + +- **[{filePath}:{startLine}]** {title} + +### 🟠 Important (X found) + +- **[{filePath}:{startLine}]** {title} + +### 🟡 Minor / Suggestions + +- {title} + +### ✅ What's good + +- (positive observations if any) +``` + +--- + +### MODE=re-review, zero new findings — skip summary reply + +If `FINDINGS_POSTED=0` (no new findings were posted in Step 1): + +```bash +echo "Re-review: no new findings — skipping summary reply." +``` + +Do not post anything. `SUMMARY_THREAD_ID` remains as provided. + +--- + +### MODE=re-review, at least one new finding — delta reply + +If `FINDINGS_POSTED > 0`: + +#### SUMMARY_THREAD_ID set — post delta reply to existing summary thread + +Reply to the existing summary thread via `pullRequestThreadComments`: + +```bash +cat > /tmp/ado_writer_delta.json << 'ENDJSON' +{ + "content": "🔄 Re-review delta — Iteration {LATEST_ITERATION_ID}\n\n{FINDINGS_POSTED} new finding(s).\n\n{BULLET_LIST_OF_NEW_FINDING_TITLES}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", + "commentType": 1 +} +ENDJSON + +az devops invoke \ + --area git \ + --resource pullRequestThreadComments \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${SUMMARY_THREAD_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/ado_writer_delta.json \ + --api-version "7.1" \ + --output json | node -e "process.stdout.write('Delta reply posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" +``` + +`{BULLET_LIST_OF_NEW_FINDING_TITLES}` — one bullet per finding posted in Step 1, format: + +``` +- **[{filePath}:{startLine}]** {title} +``` + +#### SUMMARY_THREAD_ID empty — full summary fallback + +If `SUMMARY_THREAD_ID` is empty, the prior summary thread was deleted. Fall back to first-review mode: post a full Review Summary as a new general thread (use the MODE=first-review code above) and update `SUMMARY_THREAD_ID`. + +--- + +## Step 3 — Post completion marker (final action) + +After Step 2 completes, post one final reply to the summary thread. This is the last write action of every successful run: + +```bash +if [ -n "${SUMMARY_THREAD_ID}" ]; then + cat > /tmp/ado_writer_completion.json << 'ENDJSON' + { + "content": "✅ Review complete — Iteration {LATEST_ITERATION_ID} ({FINDINGS_POSTED} findings posted)\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", + "commentType": 1 + } +ENDJSON + + az devops invoke \ + --area git \ + --resource pullRequestThreadComments \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${SUMMARY_THREAD_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/ado_writer_completion.json \ + --api-version "7.1" \ + --output json | node -e "process.stdout.write('Completion marker posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" +else + echo "No summary thread — skipping completion marker." +fi +``` + +The absence of this marker for `LATEST_ITERATION_ID` on the next run signals a partial prior run. + +--- + +## Step 4 — Clean up + +```bash +rm -f /tmp/ado_writer_thread_*.json /tmp/ado_writer_thread_*.err /tmp/ado_writer_summary.json /tmp/ado_writer_delta.json /tmp/ado_writer_completion.json +``` + +--- + +## Output + +Parse the result using the helper script and return the following structured block as your final output. This block is consumed verbatim by the orchestrator: + +```bash +RESULT=$( + SID="${SUMMARY_THREAD_ID}" \ + FP="${FINDINGS_POSTED}" \ + PLUGIN_R="${PLUGIN_ROOT}" \ + node --input-type=module << 'EOJS' +import { parseAdoWriterResult } from `file://${process.env.PLUGIN_R}/scripts/ado-writer.mjs` +const output = `ADO_WRITER_RESULT_START\nSUMMARY_THREAD_ID: ${process.env.SID}\nFINDINGS_POSTED: ${process.env.FP}\nADO_WRITER_RESULT_END` +process.stdout.write(output) +EOJS +) +echo "$RESULT" +``` + +``` +ADO_WRITER_RESULT_START +SUMMARY_THREAD_ID: {SUMMARY_THREAD_ID} +FINDINGS_POSTED: {FINDINGS_POSTED} +ADO_WRITER_RESULT_END +``` + +Where: + +- `SUMMARY_THREAD_ID` is the integer ID of the summary thread (updated if a new one was posted), or empty string if none +- `FINDINGS_POSTED` is the total count of inline comment threads successfully posted + +**Never add any ADO read operations (GET) to this agent.** diff --git a/apps/claude-code/pr-review/.agents/re-review-coordinator.md b/apps/claude-code/pr-review/.agents/re-review-coordinator.md new file mode 100644 index 0000000..4ccdd84 --- /dev/null +++ b/apps/claude-code/pr-review/.agents/re-review-coordinator.md @@ -0,0 +1,444 @@ +--- +allowed-tools: ['Bash'] +description: 'Own the full re-review state machine: prior-thread detection, partial-run check, thread classification, finding matching, and reply posting to classified threads. Returns classification counts, fresh findings, and an earlyExit flag.' +--- + +# Re-review Coordinator + +You own the complete re-review state machine. You receive the ADO Fetcher context block (which includes the raw diff), the raw full PR threads JSON, a list of new findings, and the bot signature prefix. You parse the raw diff into diff hunks internally, run all re-review logic, and post replies to classified threads. You never re-fetch ADO data — all inputs are passed to you verbatim. + +You receive all required context in this prompt as literal strings. Do not read environment variables — agents do not inherit them. + +--- + +## Inputs + +You receive: + +- `ADO_FETCHER_RESULT` — the structured context block from the ADO Fetcher agent (between `ADO_FETCHER_RESULT_START` and `ADO_FETCHER_RESULT_END`). Parse fields from it: + - `ORG_URL` + - `PROJECT` + - `REPO_ID` + - `PR_ID` + - `LATEST_ITERATION_ID` + - `RAW_DIFF` — the raw git diff text (may be empty) +- `RAW_THREADS_JSON` — the full unfiltered ADO thread list as a JSON array (fetched by the orchestrator via `az repos pr thread list`; not re-fetched here) +- `FINDINGS` — a JSON array of new findings: `{ severity, filePath, startLine, endLine, title, body }[]` +- `SIGNATURE_PREFIX` — always `🤖 *Reviewed by Claude Code*` +- `PLUGIN_ROOT` — absolute path to this plugin's directory (for Node.js helper scripts) + +--- + +## Constants + +```bash +SIGNATURE_PREFIX="🤖 *Reviewed by Claude Code*" +SIGNATURE="🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}" +``` + +--- + +## Step 1 — Parse RAW_DIFF into diff hunks + +Parse the raw diff text into a JSON array of `{ filePath, startLine, endLine }` objects. Store in a temp file. + +```bash +DIFF_HUNKS_FILE="$(mktemp "${TMPDIR:-/tmp}/re_review_hunks_XXXXXX.json")" +echo '[]' > "$DIFF_HUNKS_FILE" +``` + +Parse hunk boundaries from `RAW_DIFF`: + +```bash +printf '%s' "$RAW_DIFF" | python3 -c " +import sys, json, re +hunks = [] +current_file = None +for line in sys.stdin: + m = re.match(r'^diff --git a/.* b/(.*)', line.rstrip()) + if m: + current_file = '/' + m.group(1) + continue + m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@', line) + if m and current_file: + start = int(m.group(1)) + count = int(m.group(2)) if m.group(2) is not None else 1 + end = start + max(count - 1, 0) + hunks.append({'filePath': current_file, 'startLine': start, 'endLine': end}) +print(json.dumps(hunks)) +" > "$DIFF_HUNKS_FILE" +``` + +If `RAW_DIFF` is empty, `DIFF_HUNKS_FILE` remains `[]` — this is valid for a no-new-commits path. + +--- + +## Step 2 — Detect prior bot threads + +Call `detect-prior-review` on the raw threads JSON: + +```bash +PRIOR_THREADS_FILE="$(mktemp "${TMPDIR:-/tmp}/re_review_prior_threads_XXXXXX.json")" + +DETECT_JSON=$( + RAW_THREADS="$RAW_THREADS_JSON" \ + SIG_P="$SIGNATURE_PREFIX" \ + THREADS_OUT_F="$PRIOR_THREADS_FILE" \ + PLUGIN_R="$PLUGIN_ROOT" \ + node --input-type=module << 'EOJS' +import { writeFileSync } from 'node:fs' +const { detectPriorReview } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/detect-prior-review.mjs') +const threads = JSON.parse(process.env.RAW_THREADS) +const r = detectPriorReview({ threads, signaturePrefix: process.env.SIG_P }) +writeFileSync(process.env.THREADS_OUT_F, JSON.stringify(r.priorThreads)) +process.stdout.write(JSON.stringify({ + isRereview: r.isRereview, + summaryThreadId: r.summaryThread != null ? r.summaryThread.threadId : '', + priorIterationId: r.priorIterationId, + count: r.priorThreads.length, +})) +EOJS +) + +IS_REREVIEW=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).isRereview))") +BOT_THREAD_COUNT=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).count))") +SUMMARY_THREAD_ID=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).summaryThreadId))") +PRIOR_ITERATION_ID=$(printf '%s' "$DETECT_JSON" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')); process.stdout.write(d.priorIterationId != null ? String(d.priorIterationId) : 'null')") +``` + +If `IS_REREVIEW=false`: no prior bot threads found. Fall back to first-review mode — skip to [Step 7 — Return result](#step-7--return-result) with all counts zero, `freshFindings` = `FINDINGS`, `earlyExit: false`. + +Log: + +```bash +if [ "$IS_REREVIEW" = "true" ]; then + echo "Detected $BOT_THREAD_COUNT prior bot threads — re-review mode." +else + echo "No prior bot threads detected — first-review mode. Returning all findings as fresh." +fi +``` + +--- + +## Step 3 — Partial-run check + +If `IS_REREVIEW=true`, `SUMMARY_THREAD_ID` is non-empty, and `PRIOR_ITERATION_ID` is not `"null"`, verify the prior review completed. Check the summary thread for the completion marker `✅ Review complete — Iteration {PRIOR_ITERATION_ID}`: + +```bash +if [ "$IS_REREVIEW" = "true" ] && [ -n "$SUMMARY_THREAD_ID" ] && [ "$PRIOR_ITERATION_ID" != "null" ]; then + MARKER_FOUND=$( + THREADS_F="$PRIOR_THREADS_FILE" SID="$SUMMARY_THREAD_ID" PID="$PRIOR_ITERATION_ID" \ + node --input-type=module << 'EOJS' +import { readFileSync } from 'node:fs' +const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) +const sid = Number(process.env.SID) +const prefix = '✅ Review complete — Iteration ' + process.env.PID +const found = threads.some(t => t.threadId === sid && (t.comments ?? []).some(c => (c.content ?? '').startsWith(prefix))) +console.log(found ? 'true' : 'false') +EOJS + ) || { echo "ERROR: partial-run check failed — falling back to first-review mode."; MARKER_FOUND="false"; } + + if [ "$MARKER_FOUND" != "true" ] && [ "$MARKER_FOUND" != "false" ]; then + echo "ERROR: unexpected MARKER_FOUND value '${MARKER_FOUND}' — falling back to first-review mode." + MARKER_FOUND="false" + fi + + if [ "$MARKER_FOUND" = "false" ]; then + echo "No completion marker for Iteration $PRIOR_ITERATION_ID — partial prior run. Falling back to first-review mode." + IS_REREVIEW=false + SUMMARY_THREAD_ID="" + PRIOR_ITERATION_ID="null" + fi +fi +``` + +If `IS_REREVIEW` is now `false` after the partial-run check: skip to [Step 7 — Return result](#step-7--return-result) with all counts zero, `freshFindings` = `FINDINGS`, `earlyExit: false`. + +--- + +## Step 4 — Early-exit check (no new revisions) + +Compare `PRIOR_ITERATION_ID` with `LATEST_ITERATION_ID`. If they are equal (and both non-null/non-empty), no new commits have been pushed since the prior review. Print pending threads to the console and exit early — **no ADO writes**. + +```bash +if [ "$IS_REREVIEW" = "true" ] && [ "$PRIOR_ITERATION_ID" != "null" ] && [ "$PRIOR_ITERATION_ID" = "$LATEST_ITERATION_ID" ]; then + echo "No new revisions since prior review (both iterations: $LATEST_ITERATION_ID)." + echo "" + echo "Pending threads from prior review:" + THREADS_F="$PRIOR_THREADS_FILE" node --input-type=module << 'EOJS' +import { readFileSync } from 'node:fs' +const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) +for (const t of threads) { + if (t.isSummaryThread) continue + if (t.status === 'active' || t.status === 'pending' || t.status === 1) { + const loc = t.filePath ? `${t.filePath} L${t.start?.line ?? '?'}-${t.end?.line ?? '?'}` : '(general)' + process.stdout.write(' ' + loc + '\n') + } +} +EOJS + # Count active/pending threads for the result + PENDING_COUNT=$( + THREADS_F="$PRIOR_THREADS_FILE" node --input-type=module << 'EOJS' +import { readFileSync } from 'node:fs' +const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) +const n = threads.filter(t => !t.isSummaryThread && (t.status === 'active' || t.status === 'pending' || t.status === 1)).length +process.stdout.write(String(n)) +EOJS + ) + # Clean up and return early + rm -f "$PRIOR_THREADS_FILE" "$DIFF_HUNKS_FILE" + # Output early-exit result block + cat << RESULT_EOF +RE_REVIEW_COORDINATOR_RESULT_START +earlyExit: true +addressed: 0 +disputed: 0 +pending: ${PENDING_COUNT} +obsolete: 0 +freshFindings: [] +RE_REVIEW_COORDINATOR_RESULT_END +RESULT_EOF + exit 0 +fi +``` + +--- + +## Step 5 — Classify all prior threads + +Classify each non-summary thread using `classify-thread` and update `PRIOR_THREADS_FILE` in place with the `classification` field. Capture counts. + +```bash +CLASSIFY_COUNTS=$( + THREADS_F="$PRIOR_THREADS_FILE" \ + HUNKS_F="$DIFF_HUNKS_FILE" \ + SIG_P="$SIGNATURE_PREFIX" \ + PLUGIN_R="$PLUGIN_ROOT" \ + node --input-type=module << 'EOJS' +import { readFileSync, writeFileSync } from 'node:fs' +const { classifyThread } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/classify-thread.mjs') +const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) +const diffHunks = JSON.parse(readFileSync(process.env.HUNKS_F, 'utf8')) +const signaturePrefix = process.env.SIG_P +const counts = { addressed: 0, disputed: 0, pending: 0, obsolete: 0 } +for (const t of threads) { + if (t.isSummaryThread) continue + const cls = classifyThread({ thread: t, diffHunks, signaturePrefix }) + t.classification = cls + counts[cls]++ +} +writeFileSync(process.env.THREADS_F, JSON.stringify(threads)) +process.stdout.write(JSON.stringify(counts)) +EOJS +) + +ADDRESSED_COUNT=$(printf '%s' "$CLASSIFY_COUNTS" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).addressed))") +DISPUTED_COUNT=$(printf '%s' "$CLASSIFY_COUNTS" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).disputed))") +PENDING_COUNT=$(printf '%s' "$CLASSIFY_COUNTS" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).pending))") +OBSOLETE_COUNT=$(printf '%s' "$CLASSIFY_COUNTS" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).obsolete))") + +echo "Thread classification: ${ADDRESSED_COUNT} addressed, ${DISPUTED_COUNT} disputed, ${PENDING_COUNT} pending, ${OBSOLETE_COUNT} obsolete" +``` + +--- + +## Step 6 — Match findings, post replies, collect fresh findings + +For each finding in `FINDINGS`, call `match-finding` to look for a matching prior thread. Track which findings are consumed (matched). Unmatched findings become `freshFindings`. + +Reset the reply counts before iterating: + +```bash +FRESH_FINDINGS_JSON='[]' +``` + +Process each finding one at a time. For each finding: + +### 6a — Find matching prior thread + +```bash +MATCH=$( + THREADS_F="$PRIOR_THREADS_FILE" \ + FINDING_FILE="{finding.filePath}" \ + FINDING_START="{finding.startLine}" \ + FINDING_END="{finding.endLine}" \ + PLUGIN_R="$PLUGIN_ROOT" \ + node --input-type=module << 'EOJS' +import { readFileSync } from 'node:fs' +const { matchFinding } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/match-finding.mjs') +const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) +const result = matchFinding({ + finding: { + filePath: process.env.FINDING_FILE, + startLine: Number(process.env.FINDING_START), + endLine: Number(process.env.FINDING_END), + }, + priorThreads: threads, +}) +process.stdout.write(result != null ? JSON.stringify(result) : '') +EOJS +) + +CLASSIFICATION=$(printf '%s' "$MATCH" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')||'{}'); process.stdout.write(d.classification ?? '')" 2>/dev/null || echo "") +THREAD_ID=$(printf '%s' "$MATCH" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')||'{}'); process.stdout.write(String(d.threadId ?? ''))" 2>/dev/null || echo "") +``` + +### 6b — Dispatch on classification + +**No match (`MATCH` is empty) → add to freshFindings** + +The finding has no prior thread. Add it to `FRESH_FINDINGS_JSON` (do not post here — the orchestrator will pass fresh findings to the ADO Writer). + +**`obsolete` → skip** + +No action. Do not post. + +**`pending` → evaluate for new evidence** + +Read the most recent bot comment from the matched thread (last comment whose content contains `SIGNATURE_PREFIX`). Compare its text against the current finding's body text. + +- If **no new evidence** (same issue, same analysis): skip. Do not post. +- If the matched thread has `filePath = null` (general pending thread): always skip. +- If **new evidence** (additional analysis, different suggested fix, new code examples): post a new-evidence reply: + +```bash +cat > /tmp/re_review_reply_${THREAD_ID}.json << ENDJSON +{ + "content": "{NEW_EVIDENCE_CONTENT}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}", + "commentType": 1 +} +ENDJSON + +az devops invoke \ + --area git \ + --resource pullRequestThreadComments \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/re_review_reply_${THREAD_ID}.json \ + --api-version "7.1" \ + --output json | node -e "process.stdout.write('New-evidence reply posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" +``` + +**`disputed` → post dispute acknowledgement** + +Briefly acknowledge the reviewer's perspective without re-asserting the finding. Always include the ADO nudge before the signature: + +```bash +cat > /tmp/re_review_reply_${THREAD_ID}.json << ENDJSON +{ + "content": "{BRIEF_ACKNOWLEDGEMENT}\n\nIf you consider this resolved, please mark the thread as fixed in Azure DevOps.\n\n---\n🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}", + "commentType": 1 +} +ENDJSON + +az devops invoke \ + --area git \ + --resource pullRequestThreadComments \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/re_review_reply_${THREAD_ID}.json \ + --api-version "7.1" \ + --output json | node -e "process.stdout.write('Dispute acknowledgement posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" +``` + +**`addressed` → post resolution confirmation and PATCH thread status to fixed** + +```bash +# 1. Post resolution reply +cat > /tmp/re_review_reply_${THREAD_ID}.json << ENDJSON +{ + "content": "Resolved as of Iteration ${LATEST_ITERATION_ID} — thanks!\n\n---\n🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}", + "commentType": 1 +} +ENDJSON + +az devops invoke \ + --area git \ + --resource pullRequestThreadComments \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ + --org "${ORG_URL}" \ + --http-method POST \ + --in-file /tmp/re_review_reply_${THREAD_ID}.json \ + --api-version "7.1" \ + --output json | node -e "process.stdout.write('Resolution reply posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" + +# 2. PATCH thread status to fixed (2) +cat > /tmp/re_review_patch_${THREAD_ID}.json << ENDJSON +{ "status": 2 } +ENDJSON + +az devops invoke \ + --area git \ + --resource pullRequestThreads \ + --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ + --org "${ORG_URL}" \ + --http-method PATCH \ + --in-file /tmp/re_review_patch_${THREAD_ID}.json \ + --api-version "7.1" \ + --output json 2>/tmp/re_review_patch_${THREAD_ID}.err | \ + node -e " +try { + const d = JSON.parse(require('fs').readFileSync('/dev/stdin', 'utf8')) + process.stdout.write('Thread ' + d.id + ' patched to fixed') +} catch (e) { + const err = require('fs').readFileSync('/tmp/re_review_patch_${THREAD_ID}.err', 'utf8') + if (err.includes('409') || err.toLowerCase().includes('conflict')) { + process.stdout.write('409 Conflict — thread resolved concurrently. Continuing.') + } else { + process.stdout.write('PATCH warning: ' + err.slice(0, 200)) + } +} +" +``` + +--- + +## Step 7 — Clean up temp files + +```bash +rm -f "$PRIOR_THREADS_FILE" "$DIFF_HUNKS_FILE" +rm -f /tmp/re_review_reply_*.json /tmp/re_review_patch_*.json /tmp/re_review_patch_*.err +``` + +--- + +## Step 8 — Return result + +Return the following structured block as your final output. This block is consumed verbatim by the orchestrator. + +`freshFindings` contains only the findings that had **no matching prior thread** — the orchestrator passes these to the ADO Writer to post as new threads. Findings that matched a prior thread (any classification) are consumed here and **not** included in `freshFindings`. + +`earlyExit` is `true` only on the no-new-revisions path (Step 4). On all other paths — including normal completion with zero fresh findings — `earlyExit` is `false`. + +``` +RE_REVIEW_COORDINATOR_RESULT_START +earlyExit: false +addressed: {ADDRESSED_COUNT} +disputed: {DISPUTED_COUNT} +pending: {PENDING_COUNT} +obsolete: {OBSOLETE_COUNT} +freshFindings: {FRESH_FINDINGS_JSON} +RE_REVIEW_COORDINATOR_RESULT_END +``` + +Where: + +- `earlyExit` — `true` only when prior and latest iteration IDs were equal (no-new-revisions path); `false` otherwise +- `addressed` — count of prior threads classified as addressed (and replied to with resolution confirmation) +- `disputed` — count of prior threads classified as disputed (and replied to with acknowledgement) +- `pending` — count of prior threads classified as pending (may include threads that received a new-evidence reply or were skipped) +- `obsolete` — count of prior threads classified as obsolete +- `freshFindings` — JSON array of unmatched findings in the same shape as the input `FINDINGS` array; empty array `[]` if all findings matched prior threads or if `earlyExit` is `true` + +--- + +## Important invariants + +- **No ADO reads**: do not call `az devops invoke` for GET operations. All data is passed as inputs. +- **No re-fetch of threads**: the orchestrator already captured `RAW_THREADS_JSON` during mode detection — do not call `az repos pr thread list` again. +- **Early exit has no ADO writes**: the no-new-revisions path (Step 4) only prints to console and returns the result block — it never posts replies or PATCHes threads. +- **All four count fields are always present** in the result block, even when zero. +- **Matched findings are consumed**: a finding matched to any classified prior thread is excluded from `freshFindings`, regardless of whether a reply was posted. +- The completion marker is posted by the ADO Writer, not by this coordinator. diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index a297a48..b18cb09 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -6,990 +6,194 @@ description: 'Review an Azure DevOps pull request: fetch diff, run multi-agent a # Azure DevOps PR Review -Perform a comprehensive code review for an Azure DevOps pull request, then post findings as threaded comments directly on the PR (inline where possible) and one general summary comment. - **Arguments:** "$ARGUMENTS" --- -## Prerequisites check - -Before starting, verify: - -```bash -az --version 2>&1 | head -1 -az extension list --output table 2>&1 | grep azure-devops -``` +## Step 1 — Prerequisites (always) -If `azure-devops` extension is missing: `az extension add --name azure-devops` +Verify `pr-review-toolkit` is available (`pr-review-toolkit:code-reviewer` agent). If missing, stop and tell the user to install and enable it via Claude Code settings → Plugins. -Also verify `pr-review-toolkit` is available by checking if the agent `pr-review-toolkit:code-reviewer` can be invoked. If that plugin is not installed and enabled, stop immediately and tell the user: - -> This command requires the `pr-review-toolkit` plugin (from `anthropics/claude-plugins-official`) to be installed and enabled. Enable it via Claude Code settings → Plugins, then re-run this command. +Verify `git` is available: `git --version` --- -## Step 1 — Parse the PR URL - -Extract from `$ARGUMENTS`. Expected ADO format: - -```txt -https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id} -``` - -Variables to extract: +## Step 2 — Parse arguments and detect mode -- `ORG_URL` = `https://dev.azure.com/{org}` -- `PROJECT` = `{project}` -- `PR_ID` = `{id}` +Extract a PR URL from `$ARGUMENTS`. Expected format: +`https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}` -**GitHub URLs** (`https://github.com/...`) are not supported — tell the user and stop. +**GitHub URLs** are not supported — tell the user and stop. -If no URL provided, run `az repos pr list --status active --output table` to help them pick one. +If **no URL** provided → `MODE=pre-pr` → jump to [Pre-PR mode](#pre-pr-mode). ---- - -## Step 2 — Check the default `az` org - -```bash -az devops configure --list -``` - -Note the configured `organization`. If it differs from `ORG_URL`, pass `--org {ORG_URL}` explicitly in every `az` command below. +Extract: `ORG_URL=https://dev.azure.com/{org}`, `PROJECT={project}`, `PR_ID={id}` --- -## Step 3 — Fetch PR metadata - -```bash -az repos pr show --id {PR_ID} --org {ORG_URL} --output json -``` - -Capture and remember: - -- `repository.id` → `REPO_ID` (UUID, e.g. `99bf5e9b-...`) -- `sourceRefName` → source branch (e.g. `refs/heads/feature/my-branch`) -- `targetRefName` → target branch (e.g. `refs/heads/develop`) -- `title`, `description` -- `status` — note if already merged (`mergeStatus: succeeded`); continue anyway, comments are still useful as a review record -- `createdBy.displayName` - -Strip `refs/heads/` prefix to get plain branch names for git commands. +## Step 3 — Azure CLI check (PR modes only) -Capture additionally: - -- `repository.project.name` → `PROJECT` +Run `az --version` and check `az extension list` for `azure-devops`. If missing: `az extension add --name azure-devops` --- -## Step 3.5 — Detect prior review - -Fetch all existing PR threads and check for prior Claude Code comments. This step runs **unconditionally** and performs **no write actions**. - -### Variables exported by this step - -| Variable | Type | Description | -| -------------------- | ------------------- | -------------------------------------------------------------- | -| `IS_REREVIEW` | `true`/`false` | Whether a prior Claude Code review was found | -| `PRIOR_THREADS_FILE` | path | Temp file — jq-readable JSON array of prior bot threads | -| `SUMMARY_THREAD_ID` | integer or `""` | Thread ID of the prior summary thread (if any) | -| `PRIOR_ITERATION_ID` | integer or `"null"` | Iteration number parsed from the most recent prior bot comment | +## Step 4 — Mode detection -### Fetch all threads (paginated) +Fetch the full thread list **once** — captured here and passed forward; never re-fetched downstream. ```bash -PRIOR_THREADS_RAW="$(mktemp "${TMPDIR:-/tmp}/pr_threads_raw_XXXXXX.json")" -PRIOR_THREADS_ALL="$(mktemp "${TMPDIR:-/tmp}/pr_threads_all_XXXXXX.json")" -echo '[]' > "$PRIOR_THREADS_ALL" - -CONTINUATION_TOKEN="" -while true; do - EXTRA_ARGS=() - if [ -n "$CONTINUATION_TOKEN" ]; then - EXTRA_ARGS=(--query-parameters "continuationToken=$CONTINUATION_TOKEN") - fi - - az devops invoke \ - --area git \ - --resource pullRequestThreads \ - --route-parameters "project=$PROJECT" "repositoryId=$REPO_ID" "pullRequestId=$PR_ID" \ - --org "$ORG_URL" \ - --api-version "7.1" \ - "${EXTRA_ARGS[@]}" \ - --output json > "$PRIOR_THREADS_RAW" - - jq -s '.[0] + .[1].value' "$PRIOR_THREADS_ALL" "$PRIOR_THREADS_RAW" \ - > "${PRIOR_THREADS_ALL}.tmp" \ - && mv "${PRIOR_THREADS_ALL}.tmp" "$PRIOR_THREADS_ALL" - - CONTINUATION_TOKEN=$(jq -r '.continuationToken // empty' "$PRIOR_THREADS_RAW") - [ -z "$CONTINUATION_TOKEN" ] && break -done -rm -f "$PRIOR_THREADS_RAW" +RAW_THREADS_JSON=$(az repos pr thread list \ + --id "$PR_ID" --org "$ORG_URL" --output json 2>/dev/null) || RAW_THREADS_JSON="[]" ``` -### Parse bot threads +Check for a prior Bot Signature: ```bash -PRIOR_THREADS_FILE="$(mktemp "${TMPDIR:-/tmp}/pr_prior_threads_XXXXXX.json")" SIGNATURE_PREFIX="🤖 *Reviewed by Claude Code*" DETECT_JSON=$( - THREADS_ALL_F="$PRIOR_THREADS_ALL" \ - SIG_P="$SIGNATURE_PREFIX" \ - THREADS_OUT_F="$PRIOR_THREADS_FILE" \ - PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ + RAW_T="$RAW_THREADS_JSON" SIG_P="$SIGNATURE_PREFIX" PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ node --input-type=module << 'EOJS' -import { readFileSync, writeFileSync } from 'node:fs' -const { detectPriorReview } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/detect-prior-review.mjs') -const threads = JSON.parse(readFileSync(process.env.THREADS_ALL_F, 'utf8')) -const r = detectPriorReview({ threads, signaturePrefix: process.env.SIG_P }) -writeFileSync(process.env.THREADS_OUT_F, JSON.stringify(r.priorThreads)) +import { detectPriorReview } from 'file://' + process.env.PLUGIN_R + '/scripts/re-review/detect-prior-review.mjs' +const r = detectPriorReview({ threads: JSON.parse(process.env.RAW_T || '[]'), signaturePrefix: process.env.SIG_P }) process.stdout.write(JSON.stringify({ isRereview: r.isRereview, - summaryThreadId: r.summaryThread != null ? r.summaryThread.threadId : '', - priorIterationId: r.priorIterationId, - count: r.priorThreads.length, + priorIterationId: r.priorIterationId != null ? String(r.priorIterationId) : '', + summaryThreadId: r.summaryThread != null ? String(r.summaryThread.threadId) : '', })) EOJS ) -rm -f "$PRIOR_THREADS_ALL" -``` - -### Set detection variables - -```bash -IS_REREVIEW=$(echo "$DETECT_JSON" | jq -r '.isRereview') -BOT_THREAD_COUNT=$(echo "$DETECT_JSON" | jq -r '.count') -SUMMARY_THREAD_ID=$(echo "$DETECT_JSON" | jq -r '.summaryThreadId // ""') -PRIOR_ITERATION_ID=$(echo "$DETECT_JSON" | jq -r 'if .priorIterationId == null then "null" else (.priorIterationId | tostring) end') - -if [ "$IS_REREVIEW" = "true" ]; then - echo "Detected $BOT_THREAD_COUNT prior Claude Code threads — re-review mode ON" -else - SUMMARY_THREAD_ID="" - PRIOR_ITERATION_ID="null" - echo "Detected 0 prior Claude Code threads — re-review mode OFF" -fi -``` -### Partial-prior-run check +IS_REREVIEW=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).isRereview))") +PRIOR_ITERATION_ID=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).priorIterationId)") +SUMMARY_THREAD_ID=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).summaryThreadId)") -If `IS_REREVIEW=true`, verify the prior review completed **before** committing to re-review mode. This ensures the entire pipeline — diff range, agent analysis, and comment-posting — uses a consistent mode. - -If the summary thread is known, check it for a completion marker for `PRIOR_ITERATION_ID`. If none is found, the prior run was partial — reset to first-review mode now so Steps 4–10 all run in the same path. - -Skip this check when `PRIOR_ITERATION_ID` is `"null"` (no iteration suffix was parsed from the prior signature) — assume the prior run completed: - -```bash -if [ "$IS_REREVIEW" = "true" ] && [ -n "$SUMMARY_THREAD_ID" ] && [ "$PRIOR_ITERATION_ID" != "null" ]; then - MARKER_FOUND=$( - THREADS_F="$PRIOR_THREADS_FILE" SID="$SUMMARY_THREAD_ID" PID="$PRIOR_ITERATION_ID" \ - node --input-type=module << 'EOJS' -import { readFileSync } from 'node:fs' -const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) -const sid = Number(process.env.SID) -const prefix = '✅ Review complete — Iteration ' + process.env.PID -const found = threads.some(t => t.threadId === sid && (t.comments ?? []).some(c => (c.content ?? '').startsWith(prefix))) -console.log(found ? 'true' : 'false') -EOJS - ) || { echo "ERROR: partial-run check script failed — falling back to first-review mode for safety."; MARKER_FOUND="false"; } - - if [ "$MARKER_FOUND" != "true" ] && [ "$MARKER_FOUND" != "false" ]; then - echo "ERROR: unexpected MARKER_FOUND value '${MARKER_FOUND}' — falling back to first-review mode for safety." - MARKER_FOUND="false" - fi - - if [ "$MARKER_FOUND" = "false" ]; then - echo "No completion marker for Iteration $PRIOR_ITERATION_ID — partial prior run. Falling back to first-review mode." - IS_REREVIEW=false - SUMMARY_THREAD_ID="" - PRIOR_ITERATION_ID="null" - fi -fi +[ "$IS_REREVIEW" = "true" ] && MODE="re-review" || MODE="first-review" +echo "Mode detected: $MODE" ``` --- -## Step 3.6 — Fetch PR iterations - -Resolve the latest iteration ID and capture its commit SHA. These values drive the file-list query (Step 4) and the incremental diff baseline (spec 04). - -```bash -ITERATIONS_JSON=$(az devops invoke \ - --area git \ - --resource pullRequestIterations \ - --route-parameters "project=$PROJECT" "repositoryId=$REPO_ID" "pullRequestId=$PR_ID" \ - --org "$ORG_URL" \ - --api-version "7.1" \ - --output json) - -ITERATIONS_VALUE=$(echo "$ITERATIONS_JSON" | jq '.value // []') -ITERATION_COUNT=$(echo "$ITERATIONS_VALUE" | jq 'length') - -if [ "$ITERATION_COUNT" -eq 0 ]; then - echo "Warning: no iterations returned — defaulting to iteration 1" - LATEST_ITERATION_ID=1 - LATEST_COMMIT_ID="" -else - LATEST_ITERATION_ID=$(echo "$ITERATIONS_VALUE" | jq 'max_by(.id) | .id') - LATEST_COMMIT_ID=$(echo "$ITERATIONS_VALUE" | jq -r --argjson id "$LATEST_ITERATION_ID" \ - '.[] | select(.id == $id) | .sourceRefCommit.commitId // ""') -fi -echo "Latest iteration: $LATEST_ITERATION_ID (commit: ${LATEST_COMMIT_ID:-n/a})" -``` +## Step 5 — ADO Fetcher -When `IS_REREVIEW=true`, resolve the prior commit for spec 04's incremental diff: - -```bash -if [ "$IS_REREVIEW" = "true" ]; then - if [ "$PRIOR_ITERATION_ID" != "null" ]; then - # Iteration ID was parsed directly from the "— Iteration N" signature suffix - PRIOR_COMMIT_ID=$(echo "$ITERATIONS_VALUE" | jq -r --argjson id "$PRIOR_ITERATION_ID" \ - '.[] | select(.id == $id) | .sourceRefCommit.commitId // ""') - else - # Timestamp fallback: the prior comment had no "— Iteration N" suffix. - # Find the max publishedDate across all prior bot comments, then pick the - # highest iteration whose createdDate is still ≤ that timestamp. - PRIOR_MAX_DATE=$(jq -r '[.[].comments[].publishedDate // empty] | max // ""' "$PRIOR_THREADS_FILE") - if [ -n "$PRIOR_MAX_DATE" ]; then - PRIOR_ITERATION_ID=$(echo "$ITERATIONS_VALUE" | jq -r --arg d "$PRIOR_MAX_DATE" \ - '[.[] | select(.createdDate <= $d)] | max_by(.id) | .id // "null"') - if [ "$PRIOR_ITERATION_ID" != "null" ]; then - PRIOR_COMMIT_ID=$(echo "$ITERATIONS_VALUE" | jq -r --argjson id "$PRIOR_ITERATION_ID" \ - '.[] | select(.id == $id) | .sourceRefCommit.commitId // ""') - else - PRIOR_COMMIT_ID="" - fi - else - PRIOR_COMMIT_ID="" - fi - fi - echo "Prior iteration: $PRIOR_ITERATION_ID (commit: ${PRIOR_COMMIT_ID:-n/a})" -fi +```txt +Agent( + subagent_type: "pr-review:ado-fetcher", + prompt: "Fetch all ADO data for this PR review. + ORG_URL: {ORG_URL} + PROJECT: {PROJECT} + PR_ID: {PR_ID} + PRIOR_ITERATION_ID: {PRIOR_ITERATION_ID} + PLUGIN_ROOT: {CLAUDE_PLUGIN_ROOT}" +) ``` ---- - -## Step 4 — List changed files - -Use the ADO REST API (note: `az repos pr` has no file-list subcommand): - -```bash -az devops invoke \ - --area git \ - --resource pullRequestIterationChanges \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" "iterationId=$LATEST_ITERATION_ID" \ - --org {ORG_URL} \ - --api-version "7.1" \ - --output json | python3 -c " -import json, sys -data = json.load(sys.stdin) -for c in data.get('changeEntries', []): - path = c.get('item', {}).get('path', '') - ct = c.get('changeType', '') - print(f'{ct}: {path}') -" -``` +Store full output as `ADO_FETCHER_RESULT`. Parse `LATEST_ITERATION_ID`, `REPO_ID`, `CHANGED_FILES`, `RAW_DIFF`, `WORK_ITEM_IDS` from the `ADO_FETCHER_RESULT_START/END` block. --- -## Step 4a — Gather Doc Context (work items + Confluence pages) +## Step 6 — Doc Context Orchestrator + review agents (parallel) -```bash -DOC_CONTEXT='' -``` - -Fetch work items linked to the PR and capture the output: - -```bash -WI_JSON=$(az devops invoke \ - --area git \ - --resource pullRequestWorkItems \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" \ - --org {ORG_URL} \ - --api-version "7.1" \ - --output json 2>/dev/null) || WI_JSON="" -``` - -Extract the work item IDs into a comma-separated string: - -```bash -WI_IDS=$(echo "$WI_JSON" | jq -r '[.value[]?.id | tostring] | join(",")' 2>/dev/null) || WI_IDS="" -``` - -If `WI_JSON` is empty, the command failed, or `WI_IDS` is empty (the `value` array -had no entries), leave `DOC_CONTEXT=''` and skip the orchestrator spawn — step 5 (diff) continues independently. - -Otherwise, wait for the diff from step 5 to be available (step 4a and step 5 run -concurrently up to this point; only the orchestrator spawn waits for the diff). +Launch all of the following in a **single message**: -Resolve the plugin path: - -```bash -CONFLUENCE_CLIENT_PATH="${CLAUDE_PLUGIN_ROOT}/scripts/confluence-client.mjs" -``` - -Delegate to the Doc Context Orchestrator agent: +**Doc Context Orchestrator:** ```txt Agent( subagent_type: "pr-review:doc-context-orchestrator", prompt: "Orchestrate Doc Context gathering. - ORG_URL: {ORG_URL} PR_ID: {PR_ID} - Work item IDs: {WI_IDS} - Confluence client path: {CONFLUENCE_CLIENT_PATH} - + Work item IDs: {WORK_ITEM_IDS} + Confluence client path: {CLAUDE_PLUGIN_ROOT}/scripts/confluence-client.mjs Changed files: - {CHANGED_FILES_LIST} - + {CHANGED_FILES} Diff: {RAW_DIFF} - - Return the complete Doc Context markdown block, or an empty string if no - meaningful context could be gathered." + Return the complete Doc Context markdown block, or an empty string." ) ``` -Store the agent's output as `DOC_CONTEXT`. - -Step 4a pre-fetch (work item IDs) runs in parallel with step 5. The orchestrator agent spawn waits for the diff from step 5. Step 8 waits for the orchestrator agent to complete before launching review agents. - ---- - -## Step 5 — Get the diff locally - -Check if the local branch matches the PR source branch: - -```bash -git branch --show-current -``` - -If it does not match, check out the PR branch: - -```bash -az repos pr checkout --id {PR_ID} --org {ORG_URL} -# or: git fetch origin {source-branch} && git checkout {source-branch} -``` - -Create the diff hunks output file (consumed by spec 05 for thread classification): - -```bash -DIFF_HUNKS_FILE="$(mktemp "${TMPDIR:-/tmp}/pr_diff_hunks_XXXXXX.json")" -echo '[]' > "$DIFF_HUNKS_FILE" -``` - -### Diff strategy - -Branch on `IS_REREVIEW` to decide which diff range to use. - -#### Path A — First-time review (`IS_REREVIEW=false`) - -Run the full branch diff: - -```bash -git diff origin/{target-branch}...HEAD --name-only -RAW_DIFF=$(git diff origin/{target-branch}...HEAD) -``` - -Then [parse hunk boundaries](#hunk-boundary-parsing). - -#### Path B — Re-review, no prior commit (`IS_REREVIEW=true`, `PRIOR_COMMIT_ID` empty) - -```bash -echo "Warning: could not resolve prior commit — falling back to full diff." -git diff origin/{target-branch}...HEAD --name-only -RAW_DIFF=$(git diff origin/{target-branch}...HEAD) -``` - -Then [parse hunk boundaries](#hunk-boundary-parsing). +Store output as `DOC_CONTEXT`. -#### Path B2 — Re-review, no latest commit (`IS_REREVIEW=true`, `LATEST_COMMIT_ID` empty) +**Review aspect agents** — parse `$ARGUMENTS` for aspect filter (`code`/`errors`/`tests`/`comments`/`types`/`all`); default `all`. Always run `pr-review-toolkit:code-reviewer` and `pr-review-toolkit:silent-failure-hunter`. Also run `pr-review-toolkit:pr-test-analyzer` if test files changed, `pr-review-toolkit:comment-analyzer` if docs/comments added, `pr-review-toolkit:type-design-analyzer` if new types introduced. -```bash -echo "Warning: could not resolve latest commit — falling back to full diff." -git diff origin/{target-branch}...HEAD --name-only -RAW_DIFF=$(git diff origin/{target-branch}...HEAD) -``` - -Then [parse hunk boundaries](#hunk-boundary-parsing). +For each agent provide: PR title + description, full diff, changed file contents. Prepend `DOC_CONTEXT` as preamble if non-empty. -#### Path C — Re-review, no new commits (`IS_REREVIEW=true`, `PRIOR_COMMIT_ID == LATEST_COMMIT_ID`) - -```bash -echo "No new commits since last review." -echo "" -echo "Pending threads from prior review:" -jq -r '.[] | select(.status == "active" or .status == "pending") | - " \(.filePath // "(general)") L\(.start.line // "?")-\(.end.line // "?")"' "$PRIOR_THREADS_FILE" -``` - -**Stop here — do not proceed to Steps 5.5–11.** Clean up temp files and return to the user: - -```bash -rm -f "$PRIOR_THREADS_FILE" "$DIFF_HUNKS_FILE" -``` - -#### Path D — Re-review, new commits (`IS_REREVIEW=true`, `PRIOR_COMMIT_ID != LATEST_COMMIT_ID`) - -Attempt to fetch the prior commit, then diff only the new range: - -```bash -if git fetch origin "$PRIOR_COMMIT_ID" 2>/dev/null; then - git diff "${PRIOR_COMMIT_ID}".."${LATEST_COMMIT_ID}" --name-only - RAW_DIFF=$(git diff "${PRIOR_COMMIT_ID}".."${LATEST_COMMIT_ID}") -else - echo "Warning: prior commit ${PRIOR_COMMIT_ID} unreachable; latest commit ${LATEST_COMMIT_ID} — falling back to full diff." - git diff origin/{target-branch}...HEAD --name-only - RAW_DIFF=$(git diff origin/{target-branch}...HEAD) -fi -``` - -Then [parse hunk boundaries](#hunk-boundary-parsing). - -### Hunk boundary parsing - -After obtaining `$RAW_DIFF` in Paths A, B, B2, or D, parse file paths and line ranges into `DIFF_HUNKS_FILE`: - -```bash -echo "$RAW_DIFF" | python3 -c " -import sys, json, re -hunks = [] -current_file = None -for line in sys.stdin: - m = re.match(r'^diff --git a/.* b/(.*)', line.rstrip()) - if m: - current_file = '/' + m.group(1) - continue - m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@', line) - if m and current_file: - start = int(m.group(1)) - count = int(m.group(2)) if m.group(2) is not None else 1 - end = start + max(count - 1, 0) - hunks.append({'filePath': current_file, 'startLine': start, 'endLine': end}) -print(json.dumps(hunks)) -" > "$DIFF_HUNKS_FILE" -``` - -If the diff is very large (>500 lines), focus on the most significant changed files rather than trying to pass the entire diff to agents. +Collect findings. For each assign: severity (`critical`/`important`/`minor`), `filePath` (leading `/`, forward slashes matching ADO), `startLine`, `endLine`, `title`, `body`. Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. --- -## Step 5.5 — Classify existing threads - -For each non-summary thread in `PRIOR_THREADS_FILE`, assign exactly one classification using diff hunks from `DIFF_HUNKS_FILE`. This step runs **unconditionally** — it is a no-op when `PRIOR_THREADS_FILE` is empty. - -**Classification rules (evaluated in order):** - -1. **`addressed`** — ADO status is `fixed`, `wontFix`, `closed`, or `byDesign` (string or numeric 2–5), **or** status is `active`/`pending` and the thread's `[start.line, end.line]` range intersects a changed hunk. -2. **`obsolete`** — `filePath` is non-null and does not appear in the diff at all. -3. **`disputed`** — status is `active` and at least one comment does not contain the signature prefix `🤖 *Reviewed by Claude Code*`. -4. **`pending`** — status is `active` and all comments contain the signature prefix (bot-only thread). - -General threads (`filePath = null`, non-summary): rules 1 (intersection) and 2 do not apply; classify as `disputed` or `pending` only. - -```bash -THREADS_FILE="$PRIOR_THREADS_FILE" \ -HUNKS_FILE="$DIFF_HUNKS_FILE" \ -SIG_P="$SIGNATURE_PREFIX" \ -PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ -node --input-type=module << 'EOJS' -import { readFileSync, writeFileSync } from 'node:fs' -const { classifyThread } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/classify-thread.mjs') -const threads = JSON.parse(readFileSync(process.env.THREADS_FILE, 'utf8')) -const diffHunks = JSON.parse(readFileSync(process.env.HUNKS_FILE, 'utf8')) -const signaturePrefix = process.env.SIG_P -const counts = { addressed: 0, disputed: 0, pending: 0, obsolete: 0 } -for (const t of threads) { - if (t.isSummaryThread) continue - const cls = classifyThread({ thread: t, diffHunks, signaturePrefix }) - t.classification = cls - counts[cls]++ -} -writeFileSync(process.env.THREADS_FILE, JSON.stringify(threads)) -console.log(`Threads: ${counts.addressed} addressed, ${counts.disputed} disputed, ${counts.pending} pending, ${counts.obsolete} obsolete`) -EOJS -``` - ---- - -## Step 6 — Read key changed files - -Use the `Read` tool on the most important changed files (application logic, hooks, contracts, config). Skip auto-generated files: - -- `*/generate-types/output/**` -- `*.Designer.cs`, `*.g.cs`, `*.generated.*` -- `**/serialization/**/*.yml` (Sitecore serialization) -- `**/swagger.md` (generated API contract) - ---- - -## Step 7 — Determine review aspects - -Parse `$ARGUMENTS` for an aspect filter: `code`, `errors`, `tests`, `comments`, `types`, `all` (default). - -Map aspects to agents: +## Step 7 — Write-back (branch on mode) -- `code` → `pr-review-toolkit:code-reviewer` (always run) -- `errors` → `pr-review-toolkit:silent-failure-hunter` (always run) -- `tests` → `pr-review-toolkit:pr-test-analyzer` (if test files changed) -- `comments` → `pr-review-toolkit:comment-analyzer` (if docs/comments added) -- `types` → `pr-review-toolkit:type-design-analyzer` (if new types introduced) - ---- - -## Step 8 — Launch review agents in parallel - -Launch at least `code-reviewer` and `silent-failure-hunter` in a **single message** (parallel). For each agent, provide a self-contained prompt including: - -1. The Doc Context block from step 4a (if `DOC_CONTEXT` is non-empty) -2. The PR title and description -3. The full diff (or the most important sections if large) -4. The content of key changed files (from Step 6) -5. Project conventions from `CLAUDE.md` if present -6. File paths and language context - -Inject `DOC_CONTEXT` as a preamble before the diff content. If `DOC_CONTEXT` is empty, omit the preamble and agents receive the same prompt as today. - -Prompt structure when `DOC_CONTEXT` is non-empty: - -``` -{DOC_CONTEXT} - -## Diff -{diff content} - -## Changed files -{file contents} -``` - -**Example agent invocations (parallel):** +### First-review ```txt Agent( - subagent_type: "pr-review-toolkit:code-reviewer", - prompt: "Review PR '{title}' targeting {target-branch}. {DOC_CONTEXT if non-empty}\n\n## Diff\n[diff content]\n\n## Changed files\n[key file contents]\n\n[CLAUDE.md conventions]" -) - -Agent( - subagent_type: "pr-review-toolkit:silent-failure-hunter", - prompt: "Review PR '{title}' for silent failures. {DOC_CONTEXT if non-empty}\n\n## Diff\n[diff content]\n\n## Changed files\n[key file contents]" + subagent_type: "pr-review:ado-writer", + prompt: "Post all ADO comments for this first-review. + ORG_URL: {ORG_URL} + PROJECT: {PROJECT} + REPO_ID: {REPO_ID} + PR_ID: {PR_ID} + LATEST_ITERATION_ID: {LATEST_ITERATION_ID} + SUMMARY_THREAD_ID: + MODE: first-review + PLUGIN_ROOT: {CLAUDE_PLUGIN_ROOT} + FINDINGS: {FINDINGS_JSON}" ) ``` ---- - -## Step 9 — Aggregate findings - -Combine results from all agents. For each finding assign: - -- **Severity**: 🔴 Critical / 🟠 Important / 🟡 Minor -- **File path** — exactly as it appears in the ADO PR (leading `/`, forward slashes, e.g. `/fe/src/pages/_app.tsx`) -- **Line number(s)** — use the **right/new file** line numbers (post-diff) -- **Comment text** — clear, actionable, with a suggested fix where possible - ---- - -## Step 10 — Post inline comments - -Initialize the findings-posted counter and re-review delta counters: - -```bash -FINDINGS_POSTED=0 -NEW_THREAD_COUNT=0 -ADDRESSED_COUNT=0 -DISPUTED_COUNT=0 -PENDING_COUNT=0 -``` - -Branch on `IS_REREVIEW`. - ---- +### Re-review -### Path A — IS_REREVIEW=false (first-review flow) - -For each finding with a known file and line, post a PR thread: - -```bash -cat > /tmp/pr_thread_N.json << 'ENDJSON' -{ - "comments": [ - { - "commentType": 1, - "content": "{COMMENT_TEXT}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}" - } - ], - "status": 1, - "threadContext": { - "filePath": "/{path/to/file}", - "rightFileEnd": { "line": END_LINE, "offset": 1 }, - "rightFileStart": { "line": START_LINE, "offset": 1 } - } -} -ENDJSON - -az devops invoke \ - --area git \ - --resource pullRequestThreads \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" \ - --org {ORG_URL} \ - --http-method POST \ - --in-file /tmp/pr_thread_N.json \ - --api-version "7.1" \ - --output json | python3 -c "import json,sys; d=json.load(sys.stdin); print('Thread', d.get('id'), d.get('status'))" - -FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) -NEW_THREAD_COUNT=$((NEW_THREAD_COUNT + 1)) -``` - -**Rules:** - -- File paths: leading `/`, forward slashes, must match ADO exactly (as listed in Step 4) -- Line numbers: new/right file (post-diff), not original file -- `offset` can always be `1` -- Multi-line findings: set `rightFileStart.line` to first line, `rightFileEnd.line` to last -- If exact line is unknown, omit `threadContext` entirely (becomes a general comment) -- Use a unique temp file name per comment (e.g. `/tmp/pr_thread_1.json`, `/tmp/pr_thread_2.json`) - ---- - -### Path B — IS_REREVIEW=true (re-review reply flow) - -#### Thread matching - -For each finding (`{FINDING_FILE}`, line range `{FINDING_START}`–`{FINDING_END}`), search `PRIOR_THREADS_FILE` for a matching prior thread using filePath equality and line-range overlap with ±3 line drift: - -```bash -MATCH=$( - THREADS_F="$PRIOR_THREADS_FILE" \ - FINDING_F="{FINDING_FILE}" \ - FINDING_S="{FINDING_START}" \ - FINDING_E="{FINDING_END}" \ - PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ - node --input-type=module << 'EOJS' -import { readFileSync } from 'node:fs' -const { matchFinding } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/match-finding.mjs') -const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) -const result = matchFinding({ - finding: { filePath: process.env.FINDING_F, startLine: Number(process.env.FINDING_S), endLine: Number(process.env.FINDING_E) }, - priorThreads: threads, -}) -process.stdout.write(result != null ? JSON.stringify(result) : '') -EOJS +```txt +Agent( + subagent_type: "pr-review:re-review-coordinator", + prompt: "Run the re-review state machine. + ADO_FETCHER_RESULT: + {ADO_FETCHER_RESULT} + RAW_THREADS_JSON: + {RAW_THREADS_JSON} + FINDINGS: {FINDINGS_JSON} + SIGNATURE_PREFIX: 🤖 *Reviewed by Claude Code* + PLUGIN_ROOT: {CLAUDE_PLUGIN_ROOT}" ) - -CLASSIFICATION=$(printf '%s' "$MATCH" | jq -r '.classification // ""' 2>/dev/null || echo "") -THREAD_ID=$(printf '%s' "$MATCH" | jq -r '.threadId // ""' 2>/dev/null || echo "") -``` - -- If `MATCH` is empty → **no prior thread**: post a fresh thread via Path A (increment `FINDINGS_POSTED` and `NEW_THREAD_COUNT`). -- If `MATCH` is non-empty → **prior thread found**: dispatch on `CLASSIFICATION` below. - -#### `obsolete` — skip - -No action. Do not post. Do not increment `FINDINGS_POSTED`. - -#### `pending` — evaluate for new evidence - -Increment `PENDING_COUNT` for each matched `pending` thread (whether replied to or skipped): - -```bash -PENDING_COUNT=$((PENDING_COUNT + 1)) -``` - -Read the most recent bot comment from the matched thread (last entry in `matched_thread['comments']` where the content contains `SIGNATURE_PREFIX`). Compare its text against the current finding's comment. - -- **No new evidence** (same issue, no additional analysis): skip. Do not post. Do not increment `FINDINGS_POSTED`. -- General `pending` threads with no `filePath` (non-summary): always skip. - -- **New evidence** (additional analysis, different suggested fix, new code examples not present in the prior comment): reply with only the new content: - -```bash -cat > /tmp/pr_reply_N.json << 'ENDJSON' -{ - "content": "{NEW_EVIDENCE_CONTENT}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", - "commentType": 1 -} -ENDJSON - -az devops invoke \ - --area git \ - --resource pullRequestThreadComments \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" "threadId=$THREAD_ID" \ - --org {ORG_URL} \ - --http-method POST \ - --in-file /tmp/pr_reply_N.json \ - --api-version "7.1" \ - --output json | python3 -c "import json,sys; d=json.load(sys.stdin); print('Reply posted, comment', d.get('id'))" - -FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) -``` - -#### `disputed` — acknowledge the author's point - -Reply without re-asserting the finding. Briefly acknowledge the author's perspective. Always include the ADO nudge before the signature: - -```bash -cat > /tmp/pr_reply_N.json << 'ENDJSON' -{ - "content": "{BRIEF_ACKNOWLEDGEMENT}\n\nIf you consider this resolved, please mark the thread as fixed in Azure DevOps.\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", - "commentType": 1 -} -ENDJSON - -az devops invoke \ - --area git \ - --resource pullRequestThreadComments \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" "threadId=$THREAD_ID" \ - --org {ORG_URL} \ - --http-method POST \ - --in-file /tmp/pr_reply_N.json \ - --api-version "7.1" \ - --output json | python3 -c "import json,sys; d=json.load(sys.stdin); print('Reply posted, comment', d.get('id'))" - -FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) -DISPUTED_COUNT=$((DISPUTED_COUNT + 1)) -``` - -#### `addressed` — confirm resolution and mark thread fixed - -Reply to confirm the fix, then PATCH the thread status to `fixed` (`status: 2`). Log 409 and continue: - -```bash -# 1. Post reply -cat > /tmp/pr_reply_N.json << 'ENDJSON' -{ - "content": "Resolved as of Iteration {LATEST_ITERATION_ID} — thanks!\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", - "commentType": 1 -} -ENDJSON - -az devops invoke \ - --area git \ - --resource pullRequestThreadComments \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" "threadId=$THREAD_ID" \ - --org {ORG_URL} \ - --http-method POST \ - --in-file /tmp/pr_reply_N.json \ - --api-version "7.1" \ - --output json | python3 -c "import json,sys; d=json.load(sys.stdin); print('Reply posted, comment', d.get('id'))" - -# 2. PATCH thread status to fixed (2) -cat > /tmp/pr_thread_patch_N.json << 'ENDJSON' -{ "status": 2 } -ENDJSON - -az devops invoke \ - --area git \ - --resource pullRequestThreads \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" "threadId=$THREAD_ID" \ - --org {ORG_URL} \ - --http-method PATCH \ - --in-file /tmp/pr_thread_patch_N.json \ - --api-version "7.1" \ - --output json 2>/tmp/pr_patch_err_N.json | \ - python3 -c " -import json, sys -try: - d = json.load(sys.stdin) - print('Thread patched to fixed') -except Exception: - err = open('/tmp/pr_patch_err_N.json').read() - if '409' in err or 'conflict' in err.lower(): - print('409 Conflict — thread resolved concurrently. Continuing.') - else: - print('PATCH warning:', err[:200]) -" - -FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) -ADDRESSED_COUNT=$((ADDRESSED_COUNT + 1)) -``` - ---- - -## Step 11 — Post summary comment - -Branch on `IS_REREVIEW` and the counters set in Step 10. - ---- - -### IS_REREVIEW=false — full summary (unchanged behaviour) - -Post one general thread **without** `threadContext`: - -```bash -cat > /tmp/pr_summary.json << 'ENDJSON' -{ - "comments": [ - { - "commentType": 1, - "content": "## PR Review Summary — {PR_TITLE}\n\n{SUMMARY_CONTENT}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}" - } - ], - "status": 1 -} -ENDJSON - -SUMMARY_RESPONSE=$(az devops invoke \ - --area git \ - --resource pullRequestThreads \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" \ - --org {ORG_URL} \ - --http-method POST \ - --in-file /tmp/pr_summary.json \ - --api-version "7.1" \ - --output json) -echo "$SUMMARY_RESPONSE" | python3 -c "import json,sys; d=json.load(sys.stdin); print('Summary thread', d.get('id'), d.get('status'))" -# Always update SUMMARY_THREAD_ID to the newly posted thread so Step 11.5 posts the -# completion marker to the current run's summary thread, not the prior one. -SUMMARY_THREAD_ID=$(echo "$SUMMARY_RESPONSE" | python3 -c "import json,sys; d=json.load(sys.stdin); print(d.get('id',''))") -``` - -**Summary structure:** - -```markdown -## PR Review Summary — {title} - -### 🔴 Critical (X found) - -- **[file:line]** Issue description - -### 🟠 Important (X found) - -- **[file:line]** Issue description - -### 🟡 Minor / Suggestions - -- Suggestion - -### ✅ What's good - -- Positive observation - ---- - -🤖 _Reviewed by Claude Code_ — Iteration {N} ``` ---- - -### IS_REREVIEW=true, all counters zero — skip +Parse `RE_REVIEW_COORDINATOR_RESULT_START/END`. Extract `earlyExit` and `freshFindings`. -If `NEW_THREAD_COUNT=0` AND `ADDRESSED_COUNT=0` AND `DISPUTED_COUNT=0`: - -```bash -echo "Re-review: nothing changed — skipping summary comment." -``` - -Do not post anything. `SUMMARY_THREAD_ID` remains set from Step 3.5 so Step 11.5 can still post the completion marker to the existing summary thread. - ---- +If `earlyExit: true` — stop here; do **not** invoke ADO Writer. -### IS_REREVIEW=true, at least one counter > 0 — delta reply or fallback +Otherwise: -#### SUMMARY_THREAD_ID set — post delta reply to existing summary thread - -Reply to the existing summary thread via `pullRequestThreadComments`: - -```bash -cat > /tmp/pr_delta.json << 'ENDJSON' -{ - "content": "🤖 *Reviewed by Claude Code* — Re-review delta (Iteration {LATEST_ITERATION_ID})\n\n{NEW_THREAD_COUNT} new findings, {ADDRESSED_COUNT} resolved, {DISPUTED_COUNT} disputed, {PENDING_COUNT} pending.\n\n{BULLET_LIST_OF_NEW_FINDING_TITLES}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", - "commentType": 1 -} -ENDJSON - -az devops invoke \ - --area git \ - --resource pullRequestThreadComments \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" "threadId=$SUMMARY_THREAD_ID" \ - --org {ORG_URL} \ - --http-method POST \ - --in-file /tmp/pr_delta.json \ - --api-version "7.1" \ - --output json | python3 -c "import json,sys; d=json.load(sys.stdin); print('Delta reply posted, comment', d.get('id'))" -``` - -`{BULLET_LIST_OF_NEW_FINDING_TITLES}` — one bullet per new thread posted in Step 10, format: - -``` -- **[{filePath}:{startLine}]** {one-line finding title} +```txt +Agent( + subagent_type: "pr-review:ado-writer", + prompt: "Post all ADO comments for this re-review. + ORG_URL: {ORG_URL} + PROJECT: {PROJECT} + REPO_ID: {REPO_ID} + PR_ID: {PR_ID} + LATEST_ITERATION_ID: {LATEST_ITERATION_ID} + SUMMARY_THREAD_ID: {SUMMARY_THREAD_ID} + MODE: re-review + PLUGIN_ROOT: {CLAUDE_PLUGIN_ROOT} + FINDINGS: {FRESH_FINDINGS_JSON}" +) ``` -Include only threads created in this run (`NEW_THREAD_COUNT` threads). No prose, no section headings. - -`SUMMARY_THREAD_ID` is **not** updated — it already points to the existing summary thread for Step 11.5 to use. - -#### SUMMARY_THREAD_ID empty — full summary fallback - -The prior summary thread was deleted. Fall back to first-review mode: post a full summary as a new general thread (use the IS_REREVIEW=false code above) and update `SUMMARY_THREAD_ID`. - --- -## Step 11.5 — Post completion marker +## Pre-PR mode -After Step 11 completes, post one final reply to the summary thread **if `SUMMARY_THREAD_ID` is set**. Skip silently if it is empty (this can happen when prior bot threads exist but no summary thread was detected). This is the last write action of every successful run: +> **Pre-PR mode is not yet implemented.** Re-run this command with an ADO PR URL to perform a full review. -```bash -if [ -n "$SUMMARY_THREAD_ID" ]; then - cat > /tmp/pr_completion_marker.json << 'ENDJSON' -{ - "content": "✅ Review complete — Iteration {LATEST_ITERATION_ID} ({FINDINGS_POSTED} findings posted)\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", - "commentType": 1 -} -ENDJSON - - az devops invoke \ - --area git \ - --resource pullRequestThreadComments \ - --route-parameters "repositoryId={REPO_ID}" "pullRequestId={PR_ID}" "threadId=$SUMMARY_THREAD_ID" \ - --org {ORG_URL} \ - --http-method POST \ - --in-file /tmp/pr_completion_marker.json \ - --api-version "7.1" \ - --output json | python3 -c "import json,sys; d=json.load(sys.stdin); print('Completion marker posted, comment', d.get('id'))" -else - echo "No summary thread — skipping completion marker." -fi -``` - -The absence of this marker for `LATEST_ITERATION_ID` on the next run signals a partial prior run — Step 3.5 detects this and falls back to first-review mode. - ---- - -## Step 12 — Clean up - -```bash -rm -f /tmp/pr_thread_*.json /tmp/pr_reply_*.json /tmp/pr_thread_patch_*.json /tmp/pr_patch_err_*.json /tmp/pr_completion_marker.json /tmp/pr_summary.json /tmp/pr_delta.json -rm -f "$PRIOR_THREADS_FILE" "$DIFF_HUNKS_FILE" -``` +Exit cleanly. --- ## Comment signature -Every comment — inline or summary — **must** end with this trailer on its own line: - -```txt ---- -🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID} -``` - -Two constants govern signature generation: - -- `SIGNATURE_PREFIX` = `🤖 *Reviewed by Claude Code*` -- `SIGNATURE` = `🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}` (resolved at post time) - -Never alter the prefix — re-review detection depends on it. - ---- - -## Notes +Every comment must end with `---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}`. -- The PR may already be merged — post comments anyway as a review record. -- Use `az repos pr checkout --id {PR_ID} --org {ORG_URL}` if the local branch doesn't match the source branch. -- Always use the latest iteration of the PR (`LATEST_ITERATION_ID`). Re-reviews additionally compute `PRIOR_ITERATION_ID` — see Step 3.5 and Step 3.6. -- If `az devops invoke` returns an error on `threadContext` (e.g. file not found in the diff), retry without `threadContext` to post as a general comment. -- The detection prefix is `🤖 *Reviewed by Claude Code*` (substring match). The full emitted form is `🤖 *Reviewed by Claude Code* — Iteration N`. Never alter the prefix — re-review detection depends on it. +`SIGNATURE_PREFIX` = `🤖 *Reviewed by Claude Code*` — never alter; re-review detection depends on it. diff --git a/apps/claude-code/pr-review/package.json b/apps/claude-code/pr-review/package.json index 05b6354..51792f1 100644 --- a/apps/claude-code/pr-review/package.json +++ b/apps/claude-code/pr-review/package.json @@ -10,7 +10,7 @@ "pnpm": ">=10" }, "scripts": { - "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs", + "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs tests/ado-fetcher.test.mjs tests/ado-writer.test.mjs", "bump": "unic-bump", "sync-version": "unic-sync-version", "tag": "unic-tag", diff --git a/apps/claude-code/pr-review/scripts/ado-fetcher.mjs b/apps/claude-code/pr-review/scripts/ado-fetcher.mjs new file mode 100644 index 0000000..ad39ce7 --- /dev/null +++ b/apps/claude-code/pr-review/scripts/ado-fetcher.mjs @@ -0,0 +1,37 @@ +// @ts-check + +/** + * @typedef {{ id: number, sourceRefCommit?: { commitId?: string } | null }} ADOIteration + * @typedef {{ latestIterationId: number, latestCommitSha: string }} IterationResult + */ + +/** + * Parses the ADO pullRequestIterations value array and returns the latest + * iteration ID and its commit SHA. Defaults gracefully when no iterations + * are returned. + * + * @param {ADOIteration[]} iterations + * @returns {IterationResult} + */ +export function parseIterations(iterations) { + if (iterations.length === 0) { + return { latestIterationId: 1, latestCommitSha: '' } + } + + const latest = iterations.reduce((max, it) => (it.id > max.id ? it : max), iterations[0]) + return { + latestIterationId: latest.id, + latestCommitSha: latest.sourceRefCommit?.commitId ?? '', + } +} + +/** + * Parses the ADO pullRequestWorkItems response and returns an array of work item IDs. + * Returns an empty array when no work items are linked or when the command failed. + * + * @param {{ value?: Array<{ id: number }> } | null | undefined} response + * @returns {number[]} + */ +export function parseWorkItemIds(response) { + return (response?.value ?? []).map((wi) => wi.id) +} diff --git a/apps/claude-code/pr-review/scripts/ado-writer.mjs b/apps/claude-code/pr-review/scripts/ado-writer.mjs new file mode 100644 index 0000000..a80fa9c --- /dev/null +++ b/apps/claude-code/pr-review/scripts/ado-writer.mjs @@ -0,0 +1,29 @@ +// @ts-check + +/** + * @typedef {{ summaryThreadId: number | null, findingsPosted: number | null }} AdoWriterResult + */ + +/** + * Parses the ADO Writer agent's output block into a structured result. + * Returns null for both fields when the result block is absent from the output. + * + * @param {string} output + * @returns {AdoWriterResult} + */ +export function parseAdoWriterResult(output) { + const blockMatch = output.match(/ADO_WRITER_RESULT_START([\s\S]*?)ADO_WRITER_RESULT_END/) + if (!blockMatch) { + return { summaryThreadId: null, findingsPosted: null } + } + + const block = blockMatch[1] + + const threadIdMatch = block.match(/SUMMARY_THREAD_ID:\s*(\d+)/) + const summaryThreadId = threadIdMatch ? Number(threadIdMatch[1]) : null + + const findingsMatch = block.match(/FINDINGS_POSTED:\s*(\d+)/) + const findingsPosted = findingsMatch ? Number(findingsMatch[1]) : null + + return { summaryThreadId, findingsPosted } +} diff --git a/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs b/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs new file mode 100644 index 0000000..4cc62dc --- /dev/null +++ b/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs @@ -0,0 +1,152 @@ +// @ts-check + +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { describe, it } from 'node:test' +import { parseIterations, parseWorkItemIds } from '../scripts/ado-fetcher.mjs' + +/** Reads the ado-fetcher agent markdown for content assertions */ +const agentContent = readFileSync( + new URL('../.agents/ado-fetcher.md', import.meta.url), + 'utf8', +) + +describe('ado-fetcher agent content', () => { + it('contains no ADO write HTTP methods (POST/PATCH/DELETE)', () => { + // Allow POST only in comments/explanatory text preceded by 'no' or 'Never' + // The guard: strip lines that are clearly explanatory (contain "Never" or "no write") + const lines = agentContent.split('\n') + const suspectLines = lines.filter((line) => { + const trimmed = line.trim() + // Skip comment lines and the "Never add" instruction line itself + if (trimmed.startsWith('#')) return false + if (trimmed.toLowerCase().includes('never add')) return false + if (trimmed.toLowerCase().includes('no write')) return false + // Flag --http-method POST/PATCH/DELETE + return /--http-method\s+(POST|PATCH|DELETE)/i.test(trimmed) + }) + assert.deepEqual( + suspectLines, + [], + `Agent contains write operations: ${suspectLines.join(' | ')}`, + ) + }) + + it('declares allowed-tools in frontmatter', () => { + assert.ok(agentContent.startsWith('---'), 'Missing YAML frontmatter') + assert.ok(agentContent.includes('allowed-tools:'), 'Missing allowed-tools key') + }) + + it('outputs a structured context block with required fields', () => { + const requiredFields = [ + 'ADO_FETCHER_RESULT_START', + 'ADO_FETCHER_RESULT_END', + 'REPO_ID', + 'PR_TITLE', + 'LATEST_ITERATION_ID', + 'LATEST_COMMIT_SHA', + 'WORK_ITEM_IDS', + 'CHANGED_FILES', + 'RAW_DIFF', + ] + for (const field of requiredFields) { + assert.ok(agentContent.includes(field), `Missing required output field: ${field}`) + } + }) + + it('documents graceful handling of zero-iteration PRs', () => { + assert.ok( + agentContent.includes('no iterations returned') || + agentContent.includes('zero-iteration') || + agentContent.includes('defaulting to iteration 1'), + 'Agent must document zero-iteration fallback behaviour', + ) + }) + + it('documents that merged PRs are handled without error', () => { + assert.ok( + agentContent.includes('already merged') || + agentContent.includes('mergeStatus') || + agentContent.includes('continue without error'), + 'Agent must document handling of already-merged PRs', + ) + }) + + it('invokes the parseIterations helper from ado-fetcher.mjs', () => { + assert.ok( + agentContent.includes('parseIterations'), + 'Agent must delegate iteration parsing to parseIterations helper', + ) + }) + + it('invokes the parseWorkItemIds helper from ado-fetcher.mjs', () => { + assert.ok( + agentContent.includes('parseWorkItemIds'), + 'Agent must delegate work-item ID parsing to parseWorkItemIds helper', + ) + }) +}) + +describe('parseIterations', () => { + it('zero iterations → defaults to id=1, commitSha=""', () => { + const result = parseIterations([]) + assert.equal(result.latestIterationId, 1) + assert.equal(result.latestCommitSha, '') + }) + + it('single iteration → returns its id and commit SHA', () => { + const iterations = [ + { id: 1, sourceRefCommit: { commitId: 'abc123' } }, + ] + const result = parseIterations(iterations) + assert.equal(result.latestIterationId, 1) + assert.equal(result.latestCommitSha, 'abc123') + }) + + it('multiple iterations → returns the max id and its commit SHA', () => { + const iterations = [ + { id: 1, sourceRefCommit: { commitId: 'aaa' } }, + { id: 3, sourceRefCommit: { commitId: 'ccc' } }, + { id: 2, sourceRefCommit: { commitId: 'bbb' } }, + ] + const result = parseIterations(iterations) + assert.equal(result.latestIterationId, 3) + assert.equal(result.latestCommitSha, 'ccc') + }) + + it('iteration with null sourceRefCommit → commitSha defaults to ""', () => { + const iterations = [{ id: 2, sourceRefCommit: null }] + const result = parseIterations(iterations) + assert.equal(result.latestIterationId, 2) + assert.equal(result.latestCommitSha, '') + }) + + it('iteration with missing commitId field → commitSha defaults to ""', () => { + const iterations = [{ id: 4, sourceRefCommit: {} }] + const result = parseIterations(iterations) + assert.equal(result.latestIterationId, 4) + assert.equal(result.latestCommitSha, '') + }) +}) + +describe('parseWorkItemIds', () => { + it('no work items linked → returns empty array', () => { + const result = parseWorkItemIds({ value: [] }) + assert.deepEqual(result, []) + }) + + it('work items present → returns array of numeric IDs', () => { + const result = parseWorkItemIds({ value: [{ id: 42 }, { id: 7 }] }) + assert.deepEqual(result, [42, 7]) + }) + + it('null response (command failed) → returns empty array', () => { + const result = parseWorkItemIds(null) + assert.deepEqual(result, []) + }) + + it('response with no value key → returns empty array', () => { + const result = parseWorkItemIds({}) + assert.deepEqual(result, []) + }) +}) diff --git a/apps/claude-code/pr-review/tests/ado-writer.test.mjs b/apps/claude-code/pr-review/tests/ado-writer.test.mjs new file mode 100644 index 0000000..b90f60f --- /dev/null +++ b/apps/claude-code/pr-review/tests/ado-writer.test.mjs @@ -0,0 +1,196 @@ +// @ts-check + +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { describe, it } from 'node:test' +import { parseAdoWriterResult } from '../scripts/ado-writer.mjs' + +/** Reads the ado-writer agent markdown for content assertions */ +const agentContent = readFileSync(new URL('../.agents/ado-writer.md', import.meta.url), 'utf8') + +describe('ado-writer agent content', () => { + it('declares allowed-tools in frontmatter', () => { + assert.ok(agentContent.startsWith('---'), 'Missing YAML frontmatter') + assert.ok(agentContent.includes('allowed-tools:'), 'Missing allowed-tools key') + }) + + it('contains no ADO read-only operations (GET)', () => { + const lines = agentContent.split('\n') + const suspectLines = lines.filter((line) => { + const trimmed = line.trim() + if (trimmed.startsWith('#')) return false + return /--http-method\s+GET/i.test(trimmed) + }) + assert.deepEqual( + suspectLines, + [], + `Agent contains GET operations (reads should stay in ado-fetcher): ${suspectLines.join(' | ')}` + ) + }) + + it('accepts all required input fields', () => { + const requiredInputs = [ + 'ORG_URL', + 'PROJECT', + 'REPO_ID', + 'PR_ID', + 'LATEST_ITERATION_ID', + 'SUMMARY_THREAD_ID', + 'MODE', + ] + for (const field of requiredInputs) { + assert.ok(agentContent.includes(field), `Missing required input field: ${field}`) + } + }) + + it('accepts a findings list with the compact finding schema', () => { + const requiredFindingFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] + for (const field of requiredFindingFields) { + assert.ok(agentContent.includes(field), `Missing compact finding field: ${field}`) + } + }) + + it('posts inline comment threads using POST to pullRequestThreads', () => { + assert.ok( + agentContent.includes('pullRequestThreads') && agentContent.includes('--http-method POST'), + 'Agent must POST to pullRequestThreads for inline comments' + ) + }) + + it('includes threadContext with filePath and line range in inline comments', () => { + assert.ok(agentContent.includes('threadContext'), 'Agent must use threadContext for inline comments') + assert.ok(agentContent.includes('rightFileStart'), 'Agent must set rightFileStart in threadContext') + assert.ok(agentContent.includes('rightFileEnd'), 'Agent must set rightFileEnd in threadContext') + }) + + it('appends canonical Bot Signature to every comment', () => { + assert.ok(agentContent.includes('🤖 *Reviewed by Claude Code*'), 'Agent must include the canonical Bot Signature') + assert.ok(agentContent.includes('LATEST_ITERATION_ID'), 'Agent must include LATEST_ITERATION_ID in the signature') + }) + + it('posts full Review Summary on first-review mode', () => { + assert.ok( + agentContent.includes('first-review') || + agentContent.includes('first_review') || + agentContent.includes('IS_REREVIEW=false'), + 'Agent must handle first-review mode' + ) + assert.ok(agentContent.includes('PR Review Summary'), 'Agent must post PR Review Summary on first-review') + }) + + it('posts delta reply to existing summary thread on re-review with findings', () => { + assert.ok( + agentContent.includes('re-review') || agentContent.includes('IS_REREVIEW=true'), + 'Agent must handle re-review mode' + ) + assert.ok( + agentContent.includes('pullRequestThreadComments'), + 'Agent must POST to pullRequestThreadComments for delta reply' + ) + }) + + it('skips summary reply on re-review with zero new findings', () => { + assert.ok( + agentContent.includes('zero') || + agentContent.includes('no new findings') || + agentContent.includes('FINDINGS_POSTED=0') || + agentContent.includes('nothing to report') || + agentContent.includes('skip'), + 'Agent must document skipping summary reply when there are no new findings' + ) + }) + + it('retries without threadContext on ADO rejection', () => { + assert.ok( + agentContent.includes('threadContext') && + (agentContent.includes('retry') || + agentContent.includes('without') || + agentContent.includes('fallback') || + agentContent.includes('fall back') || + agentContent.includes('general comment')), + 'Agent must retry without threadContext when ADO rejects the inline placement' + ) + }) + + it('posts completion marker as final action', () => { + assert.ok(agentContent.includes('✅ Review complete'), 'Agent must post completion marker reply') + assert.ok( + agentContent.includes('completion marker') || + agentContent.includes('Completion marker') || + agentContent.includes('final action'), + 'Agent must document completion marker as final action' + ) + }) + + it('returns structured output block with SUMMARY_THREAD_ID and FINDINGS_POSTED', () => { + const requiredOutputFields = [ + 'ADO_WRITER_RESULT_START', + 'ADO_WRITER_RESULT_END', + 'SUMMARY_THREAD_ID', + 'FINDINGS_POSTED', + ] + for (const field of requiredOutputFields) { + assert.ok(agentContent.includes(field), `Missing required output field: ${field}`) + } + }) + + it('invokes parseAdoWriterResult helper from ado-writer.mjs', () => { + assert.ok( + agentContent.includes('parseAdoWriterResult'), + 'Agent must delegate output parsing to parseAdoWriterResult helper' + ) + }) +}) + +describe('parseAdoWriterResult', () => { + it('parses a valid result block into summaryThreadId and findingsPosted', () => { + const output = ` +ADO_WRITER_RESULT_START +SUMMARY_THREAD_ID: 42 +FINDINGS_POSTED: 5 +ADO_WRITER_RESULT_END +`.trim() + const result = parseAdoWriterResult(output) + assert.equal(result.summaryThreadId, 42) + assert.equal(result.findingsPosted, 5) + }) + + it('returns summaryThreadId=null when SUMMARY_THREAD_ID is empty', () => { + const output = ` +ADO_WRITER_RESULT_START +SUMMARY_THREAD_ID: +FINDINGS_POSTED: 0 +ADO_WRITER_RESULT_END +`.trim() + const result = parseAdoWriterResult(output) + assert.equal(result.summaryThreadId, null) + assert.equal(result.findingsPosted, 0) + }) + + it('returns null for both fields when block is missing', () => { + const result = parseAdoWriterResult('No result block here') + assert.equal(result.summaryThreadId, null) + assert.equal(result.findingsPosted, null) + }) + + it('handles FINDINGS_POSTED=0 explicitly', () => { + const output = `ADO_WRITER_RESULT_START\nSUMMARY_THREAD_ID: 7\nFINDINGS_POSTED: 0\nADO_WRITER_RESULT_END` + const result = parseAdoWriterResult(output) + assert.equal(result.summaryThreadId, 7) + assert.equal(result.findingsPosted, 0) + }) + + it('handles output with extra content around the result block', () => { + const output = [ + 'Posting inline comments...', + 'ADO_WRITER_RESULT_START', + 'SUMMARY_THREAD_ID: 99', + 'FINDINGS_POSTED: 3', + 'ADO_WRITER_RESULT_END', + 'Done.', + ].join('\n') + const result = parseAdoWriterResult(output) + assert.equal(result.summaryThreadId, 99) + assert.equal(result.findingsPosted, 3) + }) +}) diff --git a/docs/issues/pr-review-orchestrator-split/01-create-ado-fetcher-agent.md b/docs/issues/pr-review-orchestrator-split/01-create-ado-fetcher-agent.md index 783dfe6..6e6e2cb 100644 --- a/docs/issues/pr-review-orchestrator-split/01-create-ado-fetcher-agent.md +++ b/docs/issues/pr-review-orchestrator-split/01-create-ado-fetcher-agent.md @@ -1,6 +1,6 @@ # Create ADO Fetcher agent -**Status:** ready-for-agent +**Status:** resolved **Category:** enhancement ## Parent @@ -17,12 +17,12 @@ The ADO Fetcher is a prerequisite for the Doc Context Orchestrator — the Fetch ## Acceptance criteria -- [ ] The agent accepts PR URL components (org URL, project, PR ID) and returns a structured context block -- [ ] The context block includes PR metadata, latest iteration ID, latest commit SHA, changed files list, and raw diff -- [ ] The context block includes the work-item IDs linked to the PR (empty list if none) -- [ ] The agent handles the case where no iterations are returned (defaults gracefully) -- [ ] The agent handles PRs that are already merged (continues without error) -- [ ] The agent contains no write operations — it is purely a read agent +- [x] The agent accepts PR URL components (org URL, project, PR ID) and returns a structured context block +- [x] The context block includes PR metadata, latest iteration ID, latest commit SHA, changed files list, and raw diff +- [x] The context block includes the work-item IDs linked to the PR (empty list if none) +- [x] The agent handles the case where no iterations are returned (defaults gracefully) +- [x] The agent handles PRs that are already merged (continues without error) +- [x] The agent contains no write operations — it is purely a read agent ## Blocked by @@ -51,12 +51,12 @@ A new plugin agent (`pr-review:ado-fetcher`) accepts PR URL components and retur **Acceptance criteria:** -- [ ] The agent accepts PR URL components and returns a structured context block -- [ ] The context block includes PR metadata, latest iteration ID, latest commit SHA, changed files list, and raw diff -- [ ] The context block includes the work-item IDs linked to the PR (empty list if none) -- [ ] The agent handles the case where no iterations are returned (defaults gracefully) -- [ ] The agent handles PRs that are already merged (continues without error) -- [ ] The agent contains no write operations — it is purely a read agent +- [x] The agent accepts PR URL components and returns a structured context block +- [x] The context block includes PR metadata, latest iteration ID, latest commit SHA, changed files list, and raw diff +- [x] The context block includes the work-item IDs linked to the PR (empty list if none) +- [x] The agent handles the case where no iterations are returned (defaults gracefully) +- [x] The agent handles PRs that are already merged (continues without error) +- [x] The agent contains no write operations — it is purely a read agent **Out of scope:** diff --git a/docs/issues/pr-review-orchestrator-split/02-create-ado-writer-agent.md b/docs/issues/pr-review-orchestrator-split/02-create-ado-writer-agent.md index 813ecab..ac545a3 100644 --- a/docs/issues/pr-review-orchestrator-split/02-create-ado-writer-agent.md +++ b/docs/issues/pr-review-orchestrator-split/02-create-ado-writer-agent.md @@ -1,6 +1,6 @@ # Create ADO Writer agent -**Status:** ready-for-agent +**Status:** resolved **Category:** enhancement ## Parent @@ -19,14 +19,14 @@ This agent is used by both first-review and re-review modes. It is not invoked i ## Acceptance criteria -- [ ] The agent posts one Inline Comment thread per finding at the correct file path and line range -- [ ] Each posted comment ends with the canonical Bot Signature including the iteration number -- [ ] On first-review, the agent posts a full Review Summary as a new general thread -- [ ] On re-review with at least one new finding, the agent posts a delta reply to the existing summary thread -- [ ] On re-review with zero new findings, the agent skips the summary reply -- [ ] The agent posts a completion marker reply (`✅ Review complete — Iteration N`) to the summary thread as its final action -- [ ] If `threadContext` is rejected by ADO (file not in diff), the agent retries without `threadContext` (general comment fallback) -- [ ] The agent returns the final `SUMMARY_THREAD_ID` and `FINDINGS_POSTED` count to the caller +- [x] The agent posts one Inline Comment thread per finding at the correct file path and line range +- [x] Each posted comment ends with the canonical Bot Signature including the iteration number +- [x] On first-review, the agent posts a full Review Summary as a new general thread +- [x] On re-review with at least one new finding, the agent posts a delta reply to the existing summary thread +- [x] On re-review with zero new findings, the agent skips the summary reply +- [x] The agent posts a completion marker reply (`✅ Review complete — Iteration N`) to the summary thread as its final action +- [x] If `threadContext` is rejected by ADO (file not in diff), the agent retries without `threadContext` (general comment fallback) +- [x] The agent returns the final `SUMMARY_THREAD_ID` and `FINDINGS_POSTED` count to the caller ## Blocked by diff --git a/docs/issues/pr-review-orchestrator-split/03-create-re-review-coordinator-agent.md b/docs/issues/pr-review-orchestrator-split/03-create-re-review-coordinator-agent.md index 7337d4d..35ed752 100644 --- a/docs/issues/pr-review-orchestrator-split/03-create-re-review-coordinator-agent.md +++ b/docs/issues/pr-review-orchestrator-split/03-create-re-review-coordinator-agent.md @@ -1,6 +1,6 @@ # Create Re-review Coordinator agent -**Status:** ready-for-agent +**Status:** resolved **Category:** enhancement ## Parent diff --git a/docs/issues/pr-review-orchestrator-split/04-refactor-orchestrator.md b/docs/issues/pr-review-orchestrator-split/04-refactor-orchestrator.md index ce09dee..3a150b7 100644 --- a/docs/issues/pr-review-orchestrator-split/04-refactor-orchestrator.md +++ b/docs/issues/pr-review-orchestrator-split/04-refactor-orchestrator.md @@ -1,6 +1,6 @@ # Refactor review-pr.md to thin orchestrator -**Status:** ready-for-agent +**Status:** resolved **Category:** enhancement ## Parent From 355f77dc992feef0af122ba126039bc39907d486 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 12 May 2026 15:04:46 +0200 Subject: [PATCH 02/16] =?UTF-8?q?feat(pr-review):=20implement=20Pre-PR=20m?= =?UTF-8?q?ode=20=E2=80=94=20local=20branch=20diff=20review=20without=20AD?= =?UTF-8?q?O?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../pr-review/commands/review-pr.md | 72 ++++- apps/claude-code/pr-review/package.json | 2 +- apps/claude-code/pr-review/scripts/pre-pr.mjs | 80 ++++++ .../pr-review/tests/pre-pr.test.mjs | 262 ++++++++++++++++++ .../05-add-pre-pr-mode.md | 16 +- 5 files changed, 421 insertions(+), 11 deletions(-) create mode 100644 apps/claude-code/pr-review/scripts/pre-pr.mjs create mode 100644 apps/claude-code/pr-review/tests/pre-pr.test.mjs diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index b18cb09..d8ec3df 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -186,9 +186,77 @@ Agent( ## Pre-PR mode -> **Pre-PR mode is not yet implemented.** Re-run this command with an ADO PR URL to perform a full review. +**Pre-PR mode active** — no PR URL provided. Reviewing local branch diff; no ADO calls will be made. -Exit cleanly. +### Step A — Detect default branch and compute diff + +```bash +# Detect the default remote branch (main or develop) +DEFAULT_BRANCH=$(git remote show origin 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}' || echo "main") + +RAW_DIFF=$(git diff "origin/${DEFAULT_BRANCH}...HEAD") +``` + +If `git diff` fails (e.g. no upstream remote), inform the user and stop. + +### Step B — Parse changed files + +```bash +PRE_PR_CONTEXT=$( + RAW_DIFF_STR="$RAW_DIFF" \ + PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ + node --input-type=module << 'EOJS' +import { buildPrePrContext } from 'file://' + process.env.PLUGIN_R + '/scripts/pre-pr.mjs' +const ctx = buildPrePrContext(process.env.RAW_DIFF_STR) +process.stdout.write(JSON.stringify(ctx)) +EOJS +) + +FILTERED_FILES=$(printf '%s' "$PRE_PR_CONTEXT" | node -e " +const chunks = [] +process.stdin.on('data', c => chunks.push(c)) +process.stdin.on('end', () => { + const ctx = JSON.parse(Buffer.concat(chunks).toString()) + process.stdout.write(ctx.filteredFiles.join('\n')) +})") +``` + +Read the contents of each file in `FILTERED_FILES` (skip any that are deleted or unavailable). + +### Step C — Resolve aspect filter + +Parse `$ARGUMENTS` for aspect filter (`code`/`errors`/`tests`/`comments`/`types`/`all`); default `all`. +Use the same selection logic as ADO modes: always run `pr-review-toolkit:code-reviewer` and `pr-review-toolkit:silent-failure-hunter`. Also run `pr-review-toolkit:pr-test-analyzer` if test files changed, `pr-review-toolkit:comment-analyzer` if docs/comments added, `pr-review-toolkit:type-design-analyzer` if new types introduced. + +### Step D — Run review aspect agents + +Doc Context is skipped (no PR URL means no work items to fetch). + +Launch all applicable review aspect agents in a single message, passing: + +- The raw diff (`RAW_DIFF`) +- Changed file contents +- No preamble (Doc Context is empty in pre-PR mode) + +For each agent provide: full diff, filtered changed file contents. Collect findings and assign for each: `severity` (`critical`/`important`/`minor`), `filePath` (leading `/`, forward slashes), `startLine`, `endLine`, `title`, `body`. Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. + +### Step E — Present findings + +Present all findings directly in the Claude interface as a structured list — no ADO write-back occurs in pre-PR mode. + +For each finding print: + +``` +[{severity}] {filePath} L{startLine}–{endLine} +{title} +{body} +``` + +Group by severity: `critical` first, then `important`, then `minor`. Print a summary count at the end. + +If no findings, print: `✅ Pre-PR review complete — no issues found.` + +Otherwise, print: `✅ Pre-PR review complete — {N} finding(s). Open a PR to post these as inline ADO comments.` --- diff --git a/apps/claude-code/pr-review/package.json b/apps/claude-code/pr-review/package.json index 51792f1..39b7350 100644 --- a/apps/claude-code/pr-review/package.json +++ b/apps/claude-code/pr-review/package.json @@ -10,7 +10,7 @@ "pnpm": ">=10" }, "scripts": { - "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs tests/ado-fetcher.test.mjs tests/ado-writer.test.mjs", + "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs tests/ado-fetcher.test.mjs tests/ado-writer.test.mjs tests/pre-pr.test.mjs", "bump": "unic-bump", "sync-version": "unic-sync-version", "tag": "unic-tag", diff --git a/apps/claude-code/pr-review/scripts/pre-pr.mjs b/apps/claude-code/pr-review/scripts/pre-pr.mjs new file mode 100644 index 0000000..cf0303e --- /dev/null +++ b/apps/claude-code/pr-review/scripts/pre-pr.mjs @@ -0,0 +1,80 @@ +// @ts-check + +/** + * @typedef {{ changedFiles: string[], filteredFiles: string[], rawDiff: string }} PrePrContext + */ + +/** + * Skip-list patterns for files that should not be passed to review agents. + * Matches: + * - C# source-generated files (*.g.cs) + * - swagger.md / swagger.json + * - Serialization YAML files (*.serialization.yaml / *.serialization.yml) + * - Files named generated-types.* or under a generated/ directory + * + * @param {string} filePath - Leading-slash forward-slash path, e.g. /src/foo.ts + * @returns {boolean} true if the file should be excluded from review + */ +export function shouldSkipFile(filePath) { + const lower = filePath.toLowerCase() + + // C# source-generated files + if (lower.endsWith('.g.cs')) return true + + // swagger + if (lower.endsWith('swagger.md') || lower.endsWith('swagger.json')) return true + + // Serialization YAMLs + if (lower.endsWith('.serialization.yaml') || lower.endsWith('.serialization.yml')) return true + + // generated-types.* (e.g. generated-types.ts, generated-types.d.ts) + const basename = filePath.split('/').pop() ?? '' + if (basename.toLowerCase().startsWith('generated-types.')) return true + + // files under a generated/ directory segment + if (filePath.includes('/generated/')) return true + + return false +} + +/** + * Parses the file paths touched by a `git diff` output. + * Extracts the `b/` path from each `diff --git` header and returns unique + * paths with a leading slash, matching the ADO path format. + * + * @param {string} diffText - Raw output of `git diff` + * @returns {string[]} Unique file paths with leading slash + */ +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) + } + } + } + + return paths +} + +/** + * Builds the Pre-PR context object from a raw git diff string. + * Returns all changed files, the subset that should be reviewed (filtered), + * and the raw diff text. + * + * @param {string} diffText - Raw output of `git diff origin/...HEAD` + * @returns {PrePrContext} + */ +export function buildPrePrContext(diffText) { + const changedFiles = parseChangedFilesFromDiff(diffText) + const filteredFiles = changedFiles.filter((f) => !shouldSkipFile(f)) + return { changedFiles, filteredFiles, rawDiff: diffText } +} diff --git a/apps/claude-code/pr-review/tests/pre-pr.test.mjs b/apps/claude-code/pr-review/tests/pre-pr.test.mjs new file mode 100644 index 0000000..7ed063a --- /dev/null +++ b/apps/claude-code/pr-review/tests/pre-pr.test.mjs @@ -0,0 +1,262 @@ +// @ts-check + +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { describe, it } from 'node:test' +import { buildPrePrContext, parseChangedFilesFromDiff, shouldSkipFile } from '../scripts/pre-pr.mjs' + +/** Reads the review-pr command for content assertions */ +const commandContent = readFileSync(new URL('../commands/review-pr.md', import.meta.url), 'utf8') + +// --------------------------------------------------------------------------- +// parseChangedFilesFromDiff +// --------------------------------------------------------------------------- + +describe('parseChangedFilesFromDiff', () => { + it('empty diff → returns empty array', () => { + assert.deepEqual(parseChangedFilesFromDiff(''), []) + }) + + it('single changed file → returns one path with leading slash', () => { + const diff = `diff --git a/src/api.ts b/src/api.ts +index 1234567..abcdefg 100644 +--- a/src/api.ts ++++ b/src/api.ts +@@ -1,3 +1,4 @@ + unchanged ++added line +` + const result = parseChangedFilesFromDiff(diff) + assert.deepEqual(result, ['/src/api.ts']) + }) + + it('multiple changed files → returns all paths', () => { + const diff = `diff --git a/src/api.ts b/src/api.ts +index 1234567..abcdefg 100644 +--- a/src/api.ts ++++ b/src/api.ts +@@ -1,1 +1,2 @@ ++added +diff --git a/tests/api.test.ts b/tests/api.test.ts +index 1111111..2222222 100644 +--- a/tests/api.test.ts ++++ b/tests/api.test.ts +@@ -1,1 +1,2 @@ ++test added +` + const result = parseChangedFilesFromDiff(diff) + assert.deepEqual(result, ['/src/api.ts', '/tests/api.test.ts']) + }) + + it('renamed file uses b/ path (new name)', () => { + const diff = `diff --git a/old/name.ts b/new/name.ts +similarity index 90% +rename from old/name.ts +rename to new/name.ts +` + const result = parseChangedFilesFromDiff(diff) + assert.deepEqual(result, ['/new/name.ts']) + }) + + it('deduplicates identical paths from multiple hunks', () => { + const diff = `diff --git a/src/index.ts b/src/index.ts +index 111..222 100644 +--- a/src/index.ts ++++ b/src/index.ts +@@ -1,2 +1,3 @@ ++first hunk +diff --git a/src/index.ts b/src/index.ts +@@ -10,2 +11,3 @@ ++second hunk +` + const result = parseChangedFilesFromDiff(diff) + assert.deepEqual(result, ['/src/index.ts']) + }) + + it('nested directory paths preserved', () => { + const diff = `diff --git a/a/b/c/deep.ts b/a/b/c/deep.ts +index 000..111 100644 +` + const result = parseChangedFilesFromDiff(diff) + assert.deepEqual(result, ['/a/b/c/deep.ts']) + }) +}) + +// --------------------------------------------------------------------------- +// shouldSkipFile +// --------------------------------------------------------------------------- + +describe('shouldSkipFile', () => { + it('non-generated .ts file → false (keep)', () => { + assert.equal(shouldSkipFile('/src/api.ts'), false) + }) + + it('*.g.cs file → true (skip)', () => { + assert.equal(shouldSkipFile('/Generated/Models/UserModel.g.cs'), true) + }) + + it('swagger.md → true (skip)', () => { + assert.equal(shouldSkipFile('/docs/swagger.md'), true) + }) + + it('swagger.json → true (skip)', () => { + assert.equal(shouldSkipFile('/api/swagger.json'), true) + }) + + it('serialization YAML ending in .serialization.yaml → true (skip)', () => { + assert.equal(shouldSkipFile('/config/types.serialization.yaml'), true) + }) + + it('regular .yaml file → false (keep)', () => { + assert.equal(shouldSkipFile('/config/pipeline.yaml'), false) + }) + + it('regular .yml file → false (keep)', () => { + assert.equal(shouldSkipFile('/config/ci.yml'), false) + }) + + it('file named generated-types.ts → true (skip)', () => { + assert.equal(shouldSkipFile('/src/generated-types.ts'), true) + }) + + it('file under a generated/ directory → true (skip)', () => { + assert.equal(shouldSkipFile('/src/generated/api-client.ts'), true) + }) + + it('normal source file with no skip pattern → false (keep)', () => { + assert.equal(shouldSkipFile('/src/services/user.service.ts'), false) + }) +}) + +// --------------------------------------------------------------------------- +// buildPrePrContext +// --------------------------------------------------------------------------- + +describe('buildPrePrContext', () => { + it('returns rawDiff unchanged', () => { + const diff = `diff --git a/src/foo.ts b/src/foo.ts\nindex 000..111 100644\n` + const ctx = buildPrePrContext(diff) + assert.equal(ctx.rawDiff, diff) + }) + + it('changedFiles contains all parsed paths', () => { + const diff = `diff --git a/src/foo.ts b/src/foo.ts\nindex 000..111 100644\n` + const ctx = buildPrePrContext(diff) + assert.deepEqual(ctx.changedFiles, ['/src/foo.ts']) + }) + + it('filteredFiles excludes skipped files', () => { + const diff = [ + 'diff --git a/src/api.ts b/src/api.ts', + 'index 000..111 100644', + 'diff --git a/Generated/Foo.g.cs b/Generated/Foo.g.cs', + 'index 222..333 100644', + ].join('\n') + const ctx = buildPrePrContext(diff) + assert.deepEqual(ctx.changedFiles, ['/src/api.ts', '/Generated/Foo.g.cs']) + assert.deepEqual(ctx.filteredFiles, ['/src/api.ts']) + }) + + it('empty diff → all arrays empty', () => { + const ctx = buildPrePrContext('') + assert.deepEqual(ctx.changedFiles, []) + assert.deepEqual(ctx.filteredFiles, []) + assert.equal(ctx.rawDiff, '') + }) +}) + +// --------------------------------------------------------------------------- +// review-pr.md command content — Pre-PR mode section +// --------------------------------------------------------------------------- + +describe('review-pr command — Pre-PR mode', () => { + it('no longer contains "not yet implemented" stub', () => { + assert.ok( + !commandContent.includes('not yet implemented'), + 'Pre-PR mode stub must be replaced with real implementation' + ) + }) + + it('prints a console message confirming Pre-PR mode', () => { + assert.ok( + commandContent.includes('Pre-PR mode') || commandContent.includes('pre-PR mode'), + 'Command must print a Pre-PR mode confirmation message' + ) + }) + + it('uses git diff against upstream to get the local branch diff', () => { + assert.ok( + commandContent.includes('git diff') && commandContent.includes('origin/'), + 'Command must use git diff origin/...HEAD for Pre-PR mode' + ) + }) + + it('uses pre-pr.mjs helpers for diff parsing', () => { + assert.ok(commandContent.includes('pre-pr.mjs'), 'Command must import from pre-pr.mjs in Pre-PR mode') + }) + + it('launches review aspect agents in Pre-PR mode', () => { + assert.ok( + commandContent.includes('pr-review-toolkit:code-reviewer') && + commandContent.includes('pr-review-toolkit:silent-failure-hunter'), + 'Command must launch pr-review-toolkit review agents in Pre-PR mode' + ) + }) + + it('presents findings with severity, filePath, line range, title, body', () => { + const preprSection = commandContent.slice(commandContent.indexOf('## Pre-PR mode')) + assert.ok(preprSection.includes('severity'), 'Findings must include severity') + assert.ok(preprSection.includes('filePath'), 'Findings must include filePath') + assert.ok( + preprSection.includes('startLine') || preprSection.includes('line range') || preprSection.includes('line'), + 'Findings must include line range' + ) + assert.ok(preprSection.includes('title'), 'Findings must include title') + assert.ok(preprSection.includes('body'), 'Findings must include body') + }) + + it('contains no ADO API calls (az devops invoke / az repos)', () => { + const preprSection = commandContent.slice(commandContent.indexOf('## Pre-PR mode')) + assert.ok( + !preprSection.includes('az devops invoke') && !preprSection.includes('az repos'), + 'Pre-PR mode must not make ADO API calls' + ) + }) + + it('respects aspect filter from $ARGUMENTS', () => { + const preprSection = commandContent.slice(commandContent.indexOf('## Pre-PR mode')) + assert.ok( + preprSection.includes('aspect') || preprSection.includes('ARGUMENTS') || preprSection.includes('filter'), + 'Pre-PR mode must respect the aspect filter from $ARGUMENTS' + ) + }) + + it('does not invoke ADO Fetcher agent in Pre-PR mode', () => { + const preprSection = commandContent.slice(commandContent.indexOf('## Pre-PR mode')) + assert.ok(!preprSection.includes('ado-fetcher'), 'Pre-PR mode must not invoke the ado-fetcher agent') + }) + + it('does not invoke ADO Writer agent in Pre-PR mode', () => { + const preprSection = commandContent.slice(commandContent.indexOf('## Pre-PR mode')) + assert.ok(!preprSection.includes('ado-writer'), 'Pre-PR mode must not invoke the ado-writer agent') + }) + + it('does not invoke Re-review Coordinator in Pre-PR mode', () => { + const preprSection = commandContent.slice(commandContent.indexOf('## Pre-PR mode')) + assert.ok( + !preprSection.includes('re-review-coordinator'), + 'Pre-PR mode must not invoke the re-review-coordinator agent' + ) + }) + + it('prints a clear completion message when done', () => { + const preprSection = commandContent.slice(commandContent.indexOf('## Pre-PR mode')) + assert.ok( + preprSection.includes('complete') || + preprSection.includes('done') || + preprSection.includes('finished') || + preprSection.includes('✅'), + 'Pre-PR mode must print a completion message' + ) + }) +}) diff --git a/docs/issues/pr-review-orchestrator-split/05-add-pre-pr-mode.md b/docs/issues/pr-review-orchestrator-split/05-add-pre-pr-mode.md index edcf484..318c345 100644 --- a/docs/issues/pr-review-orchestrator-split/05-add-pre-pr-mode.md +++ b/docs/issues/pr-review-orchestrator-split/05-add-pre-pr-mode.md @@ -1,6 +1,6 @@ # Add Pre-PR mode -**Status:** ready-for-agent +**Status:** resolved **Category:** enhancement ## Parent @@ -21,13 +21,13 @@ No ADO credentials are required and no ADO calls are made in this mode. The pre- ## Acceptance criteria -- [ ] Running the command without a URL enters Pre-PR mode with a console message confirming the mode -- [ ] The diff used is the local branch diff against its upstream target -- [ ] Review aspect agents receive the local diff and changed file contents -- [ ] Findings are presented in the Claude interface with severity, file path, line range, title, and body -- [ ] No ADO API calls are made in this mode -- [ ] The aspect filter argument (e.g. `code`, `errors`, `all`) is respected in pre-PR mode -- [ ] `pnpm test` passes; `pnpm format` produces no diff +- [x] Running the command without a URL enters Pre-PR mode with a console message confirming the mode +- [x] The diff used is the local branch diff against its upstream target +- [x] Review aspect agents receive the local diff and changed file contents +- [x] Findings are presented in the Claude interface with severity, file path, line range, title, and body +- [x] No ADO API calls are made in this mode +- [x] The aspect filter argument (e.g. `code`, `errors`, `all`) is respected in pre-PR mode +- [x] `pnpm test` passes; `pnpm format` produces no diff ## Blocked by From 0a62b47a668f8b3d2c6e3fb9c4e34baa0dccf5ed Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 12 May 2026 15:11:00 +0200 Subject: [PATCH 03/16] feat(pr-review): add compact structured output contract to sub-agent prompts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes issue 06 — compact sub-agent output guidance. Co-Authored-By: Claude Sonnet 4.6 --- .../pr-review/commands/review-pr.md | 34 +++++- .../pr-review/tests/pre-pr.test.mjs | 112 ++++++++++++++++++ .../06-compact-subagent-output.md | 2 +- 3 files changed, 145 insertions(+), 3 deletions(-) diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index d8ec3df..d17dcd8 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -120,7 +120,22 @@ Store output as `DOC_CONTEXT`. For each agent provide: PR title + description, full diff, changed file contents. Prepend `DOC_CONTEXT` as preamble if non-empty. -Collect findings. For each assign: severity (`critical`/`important`/`minor`), `filePath` (leading `/`, forward slashes matching ADO), `startLine`, `endLine`, `title`, `body`. Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. +Each agent prompt **must** end with the following output contract: + +``` +Return your findings as a JSON array. Each element must have exactly these six fields: +- severity: "critical" | "important" | "minor" +- filePath: string — leading /, forward slashes, matching ADO format (e.g. /src/foo.ts) +- startLine: integer — first line of the relevant range +- endLine: integer — last line of the relevant range (same as startLine for single-line findings) +- title: string — one line, ≤ 80 chars +- body: string — one paragraph; the exact text to post as the ADO comment or local-interface comment + +Keep your reasoning, analysis, and supporting evidence inside your own context. +Do not include code quotes, prose reasoning, or any text outside the JSON array in your return value. +``` + +Collect the JSON arrays returned by all agents. Deduplicate and sort by severity (`critical` first). Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. --- @@ -238,7 +253,22 @@ Launch all applicable review aspect agents in a single message, passing: - Changed file contents - No preamble (Doc Context is empty in pre-PR mode) -For each agent provide: full diff, filtered changed file contents. Collect findings and assign for each: `severity` (`critical`/`important`/`minor`), `filePath` (leading `/`, forward slashes), `startLine`, `endLine`, `title`, `body`. Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. +Each agent prompt **must** end with the same output contract used in ADO modes: + +``` +Return your findings as a JSON array. Each element must have exactly these six fields: +- severity: "critical" | "important" | "minor" +- filePath: string — leading /, forward slashes (e.g. /src/foo.ts) +- startLine: integer — first line of the relevant range +- endLine: integer — last line of the relevant range (same as startLine for single-line findings) +- title: string — one line, ≤ 80 chars +- body: string — one paragraph; the exact text to post as the comment + +Keep your reasoning, analysis, and supporting evidence inside your own context. +Do not include code quotes, prose reasoning, or any text outside the JSON array in your return value. +``` + +Collect the JSON arrays returned by all agents. Deduplicate and sort by severity (`critical` first). Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. ### Step E — Present findings diff --git a/apps/claude-code/pr-review/tests/pre-pr.test.mjs b/apps/claude-code/pr-review/tests/pre-pr.test.mjs index 7ed063a..fe145c7 100644 --- a/apps/claude-code/pr-review/tests/pre-pr.test.mjs +++ b/apps/claude-code/pr-review/tests/pre-pr.test.mjs @@ -165,6 +165,118 @@ describe('buildPrePrContext', () => { }) }) +// --------------------------------------------------------------------------- +// review-pr.md command content — compact sub-agent output guidance +// --------------------------------------------------------------------------- + +describe('review-pr command — compact sub-agent output guidance', () => { + /** Slice of Step 6 — the review-agent launch step in ADO modes */ + const step6Section = commandContent.slice( + commandContent.indexOf('## Step 6'), + commandContent.indexOf('## Step 7'), + ) + + /** Pre-PR step D — the review-agent launch step in pre-PR mode */ + const stepDSection = commandContent.slice( + commandContent.indexOf('### Step D'), + commandContent.indexOf('### Step E'), + ) + + it('Step 6 instructs agents to return a JSON array of findings', () => { + assert.ok( + step6Section.includes('JSON') && step6Section.includes('array'), + 'Step 6 must instruct review agents to return a JSON array of findings', + ) + }) + + it('Step 6 requires all six finding fields in agent prompt', () => { + const requiredFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] + for (const field of requiredFields) { + assert.ok( + step6Section.includes(field), + `Step 6 agent prompt must mention required finding field: ${field}`, + ) + } + }) + + it('Step 6 instructs agents to omit code quotes from return value', () => { + assert.ok( + step6Section.includes('no code quote') || + step6Section.includes('omit code quote') || + step6Section.includes('no code quotes') || + step6Section.includes('omit code quotes') || + step6Section.includes('without code quote') || + step6Section.includes('code quotes') || + step6Section.toLowerCase().includes('code quote'), + 'Step 6 must instruct agents to omit code quotes from the return value', + ) + }) + + it('Step 6 instructs agents to omit prose reasoning from return value', () => { + assert.ok( + step6Section.toLowerCase().includes('reasoning') || + step6Section.toLowerCase().includes('prose') || + step6Section.toLowerCase().includes('analysis') || + step6Section.toLowerCase().includes('supporting'), + 'Step 6 must instruct agents to keep reasoning inside their own context, not in return value', + ) + }) + + it('Step 6 severity values are exactly critical / important / minor', () => { + assert.ok(step6Section.includes('critical'), 'Step 6 must specify "critical" as a severity value') + assert.ok(step6Section.includes('important'), 'Step 6 must specify "important" as a severity value') + assert.ok(step6Section.includes('minor'), 'Step 6 must specify "minor" as a severity value') + }) + + it('Step 6 requires filePath to use leading slash and forward slashes', () => { + assert.ok( + step6Section.includes('leading') || step6Section.includes('forward slash') || step6Section.includes('leading /'), + 'Step 6 must require filePath with leading slash and forward slashes matching ADO format', + ) + }) + + it('Step 6 requires title to be one line capped at 80 chars', () => { + assert.ok( + step6Section.includes('80') || step6Section.includes('one line') || step6Section.includes('≤ 80'), + 'Step 6 must require title to be one line, at most 80 characters', + ) + }) + + it('Step 6 requires body to be exactly the text to post as comment (no code quotes)', () => { + assert.ok( + step6Section.includes('body') && (step6Section.includes('post') || step6Section.includes('comment')), + 'Step 6 must describe body as the exact text to post as the ADO or local-interface comment', + ) + }) + + it('Step D instructs agents to return structured JSON findings (same schema as ADO modes)', () => { + assert.ok( + stepDSection.includes('JSON') || stepDSection.includes('structured'), + 'Step D must instruct review agents to return structured JSON findings', + ) + }) + + it('Step D requires same six finding fields as Step 6', () => { + const requiredFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] + for (const field of requiredFields) { + assert.ok( + stepDSection.includes(field), + `Step D agent prompt must mention required finding field: ${field}`, + ) + } + }) + + it('Step D instructs agents to omit code quotes and prose reasoning from return value', () => { + assert.ok( + stepDSection.toLowerCase().includes('code quote') || + stepDSection.toLowerCase().includes('reasoning') || + stepDSection.toLowerCase().includes('prose') || + stepDSection.toLowerCase().includes('analysis'), + 'Step D must instruct agents to keep reasoning inside their own context, not in return value', + ) + }) +}) + // --------------------------------------------------------------------------- // review-pr.md command content — Pre-PR mode section // --------------------------------------------------------------------------- diff --git a/docs/issues/pr-review-orchestrator-split/06-compact-subagent-output.md b/docs/issues/pr-review-orchestrator-split/06-compact-subagent-output.md index 06293bf..7eb4292 100644 --- a/docs/issues/pr-review-orchestrator-split/06-compact-subagent-output.md +++ b/docs/issues/pr-review-orchestrator-split/06-compact-subagent-output.md @@ -1,6 +1,6 @@ # Add compact sub-agent output guidance to the review-agent launch step -**Status:** ready-for-agent +**Status:** resolved **Category:** enhancement ## Parent From 95de3f38b0a5daeebd4728f24bdb2b185c2031b1 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 12 May 2026 15:11:27 +0200 Subject: [PATCH 04/16] chore(pr-review): populate [Unreleased] CHANGELOG entries for orchestrator split Co-Authored-By: Claude Sonnet 4.6 --- apps/claude-code/pr-review/CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index 810bfd5..28720fb 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -6,7 +6,12 @@ - (none) ### Added -- (none) +- Orchestrator split: `review-pr.md` refactored from a monolithic command to a thin orchestrator (~199 lines) that delegates ADO API calls and coordination logic to three focused agents +- ADO Fetcher agent: handles all Azure DevOps REST API fetches (diff, threads, iterations) in a single dedicated context window +- Re-review Coordinator agent: classifies prior bot threads, computes incremental diffs, and decides per-thread reply actions +- ADO Writer agent: posts all inline thread comments and the summary comment back to ADO, keeping write operations isolated from analysis +- Pre-PR mode: invoke `/pr-review:review-pr` without an ADO URL to review a local branch diff before the PR is created; findings are printed to the terminal instead of posted to ADO +- Compact sub-agent output: all review-aspect agent prompts now include an explicit JSON output contract, keeping reasoning inside each agent's context window and returning only structured `{ severity, filePath, startLine, endLine, title, body }[]` arrays to the orchestrator ### Fixed - (none) From f61529752c1e7f9492eaa8c897426e36a7b103ec Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 12 May 2026 15:12:40 +0200 Subject: [PATCH 05/16] docs(pr-review): update CLAUDE.md for orchestrator split architecture 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 --- apps/claude-code/pr-review/CLAUDE.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/apps/claude-code/pr-review/CLAUDE.md b/apps/claude-code/pr-review/CLAUDE.md index 008ff1a..96eb0bd 100644 --- a/apps/claude-code/pr-review/CLAUDE.md +++ b/apps/claude-code/pr-review/CLAUDE.md @@ -18,10 +18,16 @@ A Claude Code plugin (`pr-review`) that adds a `/pr-review:review-pr` command. W plugin.json # Plugin manifest (name, version, description) marketplace.json # Marketplace listing metadata commands/ - review-pr.md # The slash command definition — this is the core logic + review-pr.md # The slash command definition — thin orchestrator +.agents/ + ado-fetcher.md # ADO Fetcher — all Azure DevOps REST API fetches + re-review-coordinator.md # Re-review Coordinator — thread classification + incremental diff + ado-writer.md # ADO Writer — posts inline threads and summary comment to ADO + doc-context-orchestrator.md # Doc Context Orchestrator — work item + Confluence fetching + doc-context-synthesizer.md # Doc Context Synthesizer — produces business-context narrative ``` -The entire behaviour of the plugin lives in `commands/review-pr.md`. There are no build steps, no transpilation, no dependencies to install. +`commands/review-pr.md` is a thin orchestrator (~199 lines). It delegates ADO API calls and coordination logic to the focused agents in `.agents/`. There are no build steps, no transpilation, no dependencies to install. ## Plugin metadata @@ -35,6 +41,8 @@ When bumping the version, update it in **both** files: - YAML frontmatter declares `allowed-tools` — add any new tools the command needs there - Auto-generated files are explicitly skipped in Step 6 (serialization YAMLs, `*.g.cs`, generated types output, `swagger.md`) - All comments posted to ADO **must** end with the exact signature: `---\n🤖 *Reviewed by Claude Code* — Iteration N` (where N = LATEST_ITERATION_ID) +- ADO REST calls (`pullRequestThreads`, thread replies, iteration fetches) are handled by the focused agents in `.agents/`, not inline in the orchestrator command +- ADO Fetcher (`ado-fetcher.md`) owns all read operations; ADO Writer (`ado-writer.md`) owns all write operations; Re-review Coordinator (`re-review-coordinator.md`) owns thread classification and incremental diff logic - Inline threads use ADO REST `pullRequestThreads` via `az devops invoke`; file paths must match ADO format (leading `/`, forward slashes) - Always use the latest iteration of the PR. `iterationId=1` is never used. Re-reviews additionally compute `PRIOR_ITERATION_ID` from the prior review's signature — see spec 02. - If `az devops invoke` returns a `threadContext` error, fall back to posting without `threadContext` (general comment) From 73ac451942b0c54d6d866693e8b34441610ffdd2 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 12 May 2026 15:14:25 +0200 Subject: [PATCH 06/16] =?UTF-8?q?chore(pr-review):=20bump=20to=20v1.0.0=20?= =?UTF-8?q?=E2=80=94=20orchestrator=20split=20major=20release?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/claude-code/pr-review/.agents/ado-fetcher.md | 1 + .../pr-review/.claude-plugin/marketplace.json | 2 +- apps/claude-code/pr-review/.claude-plugin/plugin.json | 2 +- apps/claude-code/pr-review/CHANGELOG.md | 11 +++++++++++ apps/claude-code/pr-review/package.json | 2 +- 5 files changed, 15 insertions(+), 3 deletions(-) diff --git a/apps/claude-code/pr-review/.agents/ado-fetcher.md b/apps/claude-code/pr-review/.agents/ado-fetcher.md index 43b8de3..87c674c 100644 --- a/apps/claude-code/pr-review/.agents/ado-fetcher.md +++ b/apps/claude-code/pr-review/.agents/ado-fetcher.md @@ -235,6 +235,7 @@ ADO_FETCHER_RESULT_END ``` Where: + - `WORK_ITEM_IDS` is the JSON array from Step 5, e.g. `[42, 7]` or `[]` - `CHANGED_FILES` is the newline-separated list from Step 3, e.g. `edit: /src/api.ts` - `RAW_DIFF` is the full diff text from Step 4 (may be empty if no new commits) diff --git a/apps/claude-code/pr-review/.claude-plugin/marketplace.json b/apps/claude-code/pr-review/.claude-plugin/marketplace.json index cd2173a..f28550f 100644 --- a/apps/claude-code/pr-review/.claude-plugin/marketplace.json +++ b/apps/claude-code/pr-review/.claude-plugin/marketplace.json @@ -21,7 +21,7 @@ "name": "pr-review", "source": "./", "tags": ["code-quality", "azure-devops"], - "version": "0.9.1" + "version": "1.0.0" } ] } diff --git a/apps/claude-code/pr-review/.claude-plugin/plugin.json b/apps/claude-code/pr-review/.claude-plugin/plugin.json index 1d22639..8aa040c 100644 --- a/apps/claude-code/pr-review/.claude-plugin/plugin.json +++ b/apps/claude-code/pr-review/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "pr-review", - "version": "0.9.1", + "version": "1.0.0", "description": "Review Azure DevOps pull requests with multi-agent analysis and post threaded comments back to the PR.", "author": { "name": "Unic AG", diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index 28720fb..c98e6c6 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -5,6 +5,17 @@ ### Breaking - (none) +### Added +- (none) + +### Fixed +- (none) + +## [1.0.0] — 2026-05-12 + +### Breaking +- (none) + ### Added - Orchestrator split: `review-pr.md` refactored from a monolithic command to a thin orchestrator (~199 lines) that delegates ADO API calls and coordination logic to three focused agents - ADO Fetcher agent: handles all Azure DevOps REST API fetches (diff, threads, iterations) in a single dedicated context window diff --git a/apps/claude-code/pr-review/package.json b/apps/claude-code/pr-review/package.json index 39b7350..fb070d9 100644 --- a/apps/claude-code/pr-review/package.json +++ b/apps/claude-code/pr-review/package.json @@ -1,6 +1,6 @@ { "name": "pr-review", - "version": "0.9.1", + "version": "1.0.0", "private": true, "license": "LGPL-3.0-or-later", "type": "module", From 9c332cc0f49790bcada89128b17c57debb35dfab Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 12 May 2026 15:14:47 +0200 Subject: [PATCH 07/16] =?UTF-8?q?chore(triage):=20resolve=20issue=2007=20?= =?UTF-8?q?=E2=80=94=20version=20bump=20and=20CHANGELOG?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- .../pr-review-orchestrator-split/07-version-bump-and-release.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/issues/pr-review-orchestrator-split/07-version-bump-and-release.md b/docs/issues/pr-review-orchestrator-split/07-version-bump-and-release.md index e25ce91..44a5b8b 100644 --- a/docs/issues/pr-review-orchestrator-split/07-version-bump-and-release.md +++ b/docs/issues/pr-review-orchestrator-split/07-version-bump-and-release.md @@ -1,6 +1,6 @@ # Version bump and CHANGELOG -**Status:** ready-for-agent +**Status:** resolved **Category:** enhancement ## Parent From c23638b3b4664b602c8f73e6f6e48adef1bc2090 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 12:58:44 +0200 Subject: [PATCH 08/16] fix: formatting --- .../pr-review/tests/ado-fetcher.test.mjs | 23 ++++-------- .../pr-review/tests/pre-pr.test.mjs | 36 +++++++------------ .../inbox/adapt-release-package-to-gitflow.md | 1 + docs/inbox/add-github-support-to-pr-review.md | 1 + ...alternative-doc-sources-for-doc-context.md | 1 + ...ative-work-item-sources-for-doc-context.md | 1 + docs/inbox/automate-qa-in-github.md | 1 + ...review-request-user-confirmation-before.md | 1 + ...ing-pr-review-on-active-ado-prs-wrongly.md | 1 + docs/issues/ci-node24-upgrade/PRD.md | 2 +- .../issues/conventional-commits-scopes/PRD.md | 16 ++++----- docs/issues/github-copilot-config/PRD.md | 2 +- docs/issues/plugin-unic-prefix/PRD.md | 10 +++--- 13 files changed, 41 insertions(+), 55 deletions(-) diff --git a/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs b/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs index 4cc62dc..0d5e558 100644 --- a/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs +++ b/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs @@ -6,10 +6,7 @@ import { describe, it } from 'node:test' import { parseIterations, parseWorkItemIds } from '../scripts/ado-fetcher.mjs' /** Reads the ado-fetcher agent markdown for content assertions */ -const agentContent = readFileSync( - new URL('../.agents/ado-fetcher.md', import.meta.url), - 'utf8', -) +const agentContent = readFileSync(new URL('../.agents/ado-fetcher.md', import.meta.url), 'utf8') describe('ado-fetcher agent content', () => { it('contains no ADO write HTTP methods (POST/PATCH/DELETE)', () => { @@ -25,11 +22,7 @@ describe('ado-fetcher agent content', () => { // Flag --http-method POST/PATCH/DELETE return /--http-method\s+(POST|PATCH|DELETE)/i.test(trimmed) }) - assert.deepEqual( - suspectLines, - [], - `Agent contains write operations: ${suspectLines.join(' | ')}`, - ) + assert.deepEqual(suspectLines, [], `Agent contains write operations: ${suspectLines.join(' | ')}`) }) it('declares allowed-tools in frontmatter', () => { @@ -59,7 +52,7 @@ describe('ado-fetcher agent content', () => { agentContent.includes('no iterations returned') || agentContent.includes('zero-iteration') || agentContent.includes('defaulting to iteration 1'), - 'Agent must document zero-iteration fallback behaviour', + 'Agent must document zero-iteration fallback behaviour' ) }) @@ -68,21 +61,21 @@ describe('ado-fetcher agent content', () => { agentContent.includes('already merged') || agentContent.includes('mergeStatus') || agentContent.includes('continue without error'), - 'Agent must document handling of already-merged PRs', + 'Agent must document handling of already-merged PRs' ) }) it('invokes the parseIterations helper from ado-fetcher.mjs', () => { assert.ok( agentContent.includes('parseIterations'), - 'Agent must delegate iteration parsing to parseIterations helper', + 'Agent must delegate iteration parsing to parseIterations helper' ) }) it('invokes the parseWorkItemIds helper from ado-fetcher.mjs', () => { assert.ok( agentContent.includes('parseWorkItemIds'), - 'Agent must delegate work-item ID parsing to parseWorkItemIds helper', + 'Agent must delegate work-item ID parsing to parseWorkItemIds helper' ) }) }) @@ -95,9 +88,7 @@ describe('parseIterations', () => { }) it('single iteration → returns its id and commit SHA', () => { - const iterations = [ - { id: 1, sourceRefCommit: { commitId: 'abc123' } }, - ] + const iterations = [{ id: 1, sourceRefCommit: { commitId: 'abc123' } }] const result = parseIterations(iterations) assert.equal(result.latestIterationId, 1) assert.equal(result.latestCommitSha, 'abc123') diff --git a/apps/claude-code/pr-review/tests/pre-pr.test.mjs b/apps/claude-code/pr-review/tests/pre-pr.test.mjs index fe145c7..18dc996 100644 --- a/apps/claude-code/pr-review/tests/pre-pr.test.mjs +++ b/apps/claude-code/pr-review/tests/pre-pr.test.mjs @@ -171,31 +171,22 @@ describe('buildPrePrContext', () => { describe('review-pr command — compact sub-agent output guidance', () => { /** Slice of Step 6 — the review-agent launch step in ADO modes */ - const step6Section = commandContent.slice( - commandContent.indexOf('## Step 6'), - commandContent.indexOf('## Step 7'), - ) + const step6Section = commandContent.slice(commandContent.indexOf('## Step 6'), commandContent.indexOf('## Step 7')) /** Pre-PR step D — the review-agent launch step in pre-PR mode */ - const stepDSection = commandContent.slice( - commandContent.indexOf('### Step D'), - commandContent.indexOf('### Step E'), - ) + const stepDSection = commandContent.slice(commandContent.indexOf('### Step D'), commandContent.indexOf('### Step E')) it('Step 6 instructs agents to return a JSON array of findings', () => { assert.ok( step6Section.includes('JSON') && step6Section.includes('array'), - 'Step 6 must instruct review agents to return a JSON array of findings', + 'Step 6 must instruct review agents to return a JSON array of findings' ) }) it('Step 6 requires all six finding fields in agent prompt', () => { const requiredFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] for (const field of requiredFields) { - assert.ok( - step6Section.includes(field), - `Step 6 agent prompt must mention required finding field: ${field}`, - ) + assert.ok(step6Section.includes(field), `Step 6 agent prompt must mention required finding field: ${field}`) } }) @@ -208,7 +199,7 @@ describe('review-pr command — compact sub-agent output guidance', () => { step6Section.includes('without code quote') || step6Section.includes('code quotes') || step6Section.toLowerCase().includes('code quote'), - 'Step 6 must instruct agents to omit code quotes from the return value', + 'Step 6 must instruct agents to omit code quotes from the return value' ) }) @@ -218,7 +209,7 @@ describe('review-pr command — compact sub-agent output guidance', () => { step6Section.toLowerCase().includes('prose') || step6Section.toLowerCase().includes('analysis') || step6Section.toLowerCase().includes('supporting'), - 'Step 6 must instruct agents to keep reasoning inside their own context, not in return value', + 'Step 6 must instruct agents to keep reasoning inside their own context, not in return value' ) }) @@ -231,38 +222,35 @@ describe('review-pr command — compact sub-agent output guidance', () => { it('Step 6 requires filePath to use leading slash and forward slashes', () => { assert.ok( step6Section.includes('leading') || step6Section.includes('forward slash') || step6Section.includes('leading /'), - 'Step 6 must require filePath with leading slash and forward slashes matching ADO format', + 'Step 6 must require filePath with leading slash and forward slashes matching ADO format' ) }) it('Step 6 requires title to be one line capped at 80 chars', () => { assert.ok( step6Section.includes('80') || step6Section.includes('one line') || step6Section.includes('≤ 80'), - 'Step 6 must require title to be one line, at most 80 characters', + 'Step 6 must require title to be one line, at most 80 characters' ) }) it('Step 6 requires body to be exactly the text to post as comment (no code quotes)', () => { assert.ok( step6Section.includes('body') && (step6Section.includes('post') || step6Section.includes('comment')), - 'Step 6 must describe body as the exact text to post as the ADO or local-interface comment', + 'Step 6 must describe body as the exact text to post as the ADO or local-interface comment' ) }) it('Step D instructs agents to return structured JSON findings (same schema as ADO modes)', () => { assert.ok( stepDSection.includes('JSON') || stepDSection.includes('structured'), - 'Step D must instruct review agents to return structured JSON findings', + 'Step D must instruct review agents to return structured JSON findings' ) }) it('Step D requires same six finding fields as Step 6', () => { const requiredFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] for (const field of requiredFields) { - assert.ok( - stepDSection.includes(field), - `Step D agent prompt must mention required finding field: ${field}`, - ) + assert.ok(stepDSection.includes(field), `Step D agent prompt must mention required finding field: ${field}`) } }) @@ -272,7 +260,7 @@ describe('review-pr command — compact sub-agent output guidance', () => { stepDSection.toLowerCase().includes('reasoning') || stepDSection.toLowerCase().includes('prose') || stepDSection.toLowerCase().includes('analysis'), - 'Step D must instruct agents to keep reasoning inside their own context, not in return value', + 'Step D must instruct agents to keep reasoning inside their own context, not in return value' ) }) }) diff --git a/docs/inbox/adapt-release-package-to-gitflow.md b/docs/inbox/adapt-release-package-to-gitflow.md index 99b1030..1b3cda9 100644 --- a/docs/inbox/adapt-release-package-to-gitflow.md +++ b/docs/inbox/adapt-release-package-to-gitflow.md @@ -19,6 +19,7 @@ If I'm using Git-flow, shouldn't the release process be adapted? And CI? Now I n The pain point is that after a hotfix or release merges to `main`, `develop` falls behind and the developer must remember to backfill it manually. Both `packages/release-tools/` and the CI workflows in `.github/workflows/` may need adjusting. **What grilling needs to resolve:** + - Which scenarios create the drift? Hotfix merges? Release PRs from `develop` → `main`? - Should the fix be a GitHub Actions workflow step (auto-merge `main` back into `develop` after a release merges), a documented manual step, or a `release-tools` script? - Are there edge cases where auto-backfill would be dangerous (e.g. `main` has a hotfix that conflicts with in-flight feature work on `develop`)? diff --git a/docs/inbox/add-github-support-to-pr-review.md b/docs/inbox/add-github-support-to-pr-review.md index d6fa617..6859644 100644 --- a/docs/inbox/add-github-support-to-pr-review.md +++ b/docs/inbox/add-github-support-to-pr-review.md @@ -17,6 +17,7 @@ Add GitHub support to pr-review The plugin communicates with ADO via `ado-fetcher` and `ado-writer` sub-agents. Supporting GitHub PRs would require equivalent `github-fetcher` and `github-writer` agents using the GitHub REST API (or `gh` CLI), plus a top-level dispatch in the orchestrator to route based on the detected remote. **What grilling needs to resolve:** + - Does GitHub support run alongside ADO (auto-detect remote from `git remote`), or is it configured explicitly? - Authentication: `gh` CLI token, `GITHUB_TOKEN` env var, or a stored PAT? - Thread model mapping: GitHub uses inline review comments and PR-level comments — how does the existing classification logic (`addressed`, `pending`, `disputed`, `obsolete`) map onto GitHub's review state machine? diff --git a/docs/inbox/alternative-doc-sources-for-doc-context.md b/docs/inbox/alternative-doc-sources-for-doc-context.md index 1532a2f..eeaa3ab 100644 --- a/docs/inbox/alternative-doc-sources-for-doc-context.md +++ b/docs/inbox/alternative-doc-sources-for-doc-context.md @@ -33,6 +33,7 @@ dimension, different axis). **Nature:** Multi-source doc client design — additive extension to the doc context enrichment layer. Blocked on two open design questions that need grilling before a spec can be written: + 1. **Dispatch strategy** — URL-pattern auto-detection (simpler UX, fragile for private URLs) vs. explicit config listing active doc sources (more setup, more predictable). 2. **Credential handling** — each source (Notion, SharePoint, GitHub Wiki) has a different auth model; needs a consistent discovery pattern (env vars? config file? per-source entry?). diff --git a/docs/inbox/alternative-work-item-sources-for-doc-context.md b/docs/inbox/alternative-work-item-sources-for-doc-context.md index fab1e8f..832a6c9 100644 --- a/docs/inbox/alternative-work-item-sources-for-doc-context.md +++ b/docs/inbox/alternative-work-item-sources-for-doc-context.md @@ -32,6 +32,7 @@ item trackers are active for a given install. Needs grilling before implementati **Nature:** Multi-source work item client design — additive extension to the doc context enrichment layer. Blocked on design decisions that need grilling: + 1. **Source discovery** — how does the plugin know which tracker a linked URL belongs to? URL pattern matching, or explicit config? 2. **Credential handling** — Jira uses API tokens + Basic auth; GitHub Issues uses `gh` CLI or a PAT. Needs a consistent abstraction across clients. 3. **Config file shape** — the architecture note suggests a declarative config; grilling should nail down the exact format before implementation. diff --git a/docs/inbox/automate-qa-in-github.md b/docs/inbox/automate-qa-in-github.md index ab6a7d7..2d7cb89 100644 --- a/docs/inbox/automate-qa-in-github.md +++ b/docs/inbox/automate-qa-in-github.md @@ -50,6 +50,7 @@ Is this worth for this repo only? Or for `pr-review` app too? Closely related to `review-pr-review-command-process.md` — both describe the same loop from different angles. Strong candidate for merging into a single PRD. Should be grilled together. **What grilling needs to resolve:** + - One feature or two? If merged, what is the slug? - Target scope: this repo only (monorepo-specific) or a general skill usable across any repo? - Delivery vehicle: a new slash command, an extension to `/pr-review-toolkit:review-pr`, or a CLAUDE.md prompt template? diff --git a/docs/inbox/pr-review-request-user-confirmation-before.md b/docs/inbox/pr-review-request-user-confirmation-before.md index 7322142..c9c217a 100644 --- a/docs/inbox/pr-review-request-user-confirmation-before.md +++ b/docs/inbox/pr-review-request-user-confirmation-before.md @@ -17,6 +17,7 @@ pr-review request user confirmation before proceeding to checkout the branch to The plugin currently checks out the PR branch automatically as part of the review flow. If the user has uncommitted work or is on a different branch, this can be disruptive with no warning. **What grilling needs to resolve:** + - Exact trigger: confirmation before any checkout, or only when the working tree is dirty / a branch switch is needed? - UX: a yes/no prompt via `AskUserQuestion`, or a `--no-checkout` flag that reviews from the remote diff only? - Should the plugin stash/restore working changes automatically, or just warn and abort? diff --git a/docs/inbox/using-pr-review-on-active-ado-prs-wrongly.md b/docs/inbox/using-pr-review-on-active-ado-prs-wrongly.md index 805f805..0c031c6 100644 --- a/docs/inbox/using-pr-review-on-active-ado-prs-wrongly.md +++ b/docs/inbox/using-pr-review-on-active-ado-prs-wrongly.md @@ -17,6 +17,7 @@ using pr-review on active ADO PRs wrongly identified as merged Very sparse report; no repro steps or error output provided. **What we still need to reproduce and fix:** + - What is the ADO PR status at the time of the wrong identification? (Active, Draft, something else?) - Which code path reads the PR status — `ado-fetcher`? Which field on the ADO PR object is being checked (`status`, `mergeStatus`, something else)? - Does this happen on all PRs or only under specific conditions (e.g. auto-complete enabled, a specific branch naming pattern, a particular reviewer state)? diff --git a/docs/issues/ci-node24-upgrade/PRD.md b/docs/issues/ci-node24-upgrade/PRD.md index e3f690a..9b0e9c3 100644 --- a/docs/issues/ci-node24-upgrade/PRD.md +++ b/docs/issues/ci-node24-upgrade/PRD.md @@ -6,7 +6,7 @@ created: 2026-05-12 **Status:** ready-for-agent **Category:** bug -> *This was generated by AI during triage.* +> _This was generated by AI during triage._ ## Problem Statement diff --git a/docs/issues/conventional-commits-scopes/PRD.md b/docs/issues/conventional-commits-scopes/PRD.md index eb46042..eabb5dd 100644 --- a/docs/issues/conventional-commits-scopes/PRD.md +++ b/docs/issues/conventional-commits-scopes/PRD.md @@ -6,7 +6,7 @@ created: 2026-05-12 **Status:** ready-for-agent **Category:** enhancement -> *This was generated by AI during triage.* +> _This was generated by AI during triage._ ## Problem Statement @@ -20,13 +20,13 @@ generate, and PR reviews harder to reason about. Define a canonical scope vocabulary and document it in `CLAUDE.md` (or a dedicated `docs/conventions/commits.md` linked from `CLAUDE.md`). The proposed taxonomy: -| Type | Scope examples | Notes | -|---|---|---| -| `feat`, `fix` | `pr-review`, `auto-format`, `unic-confluence`, `release-tools`, `biome-config`, `tsconfig` | One scope per app or package | -| `chore` | `ci`, `deps`, `biome`, `prettier`, `eslint`, `ts`, `vscode` | Per tooling concern | -| `docs` | `pr-review`, `auto-format`, `unic-agents-plugins` | Per plugin or repo-wide | -| `chore(release)` | _(no scope, or plugin name)_ | Version bumps, tags | -| `test` | plugin name or package name | Matches `feat`/`fix` scope | +| Type | Scope examples | Notes | +| ---------------- | ------------------------------------------------------------------------------------------ | ---------------------------- | +| `feat`, `fix` | `pr-review`, `auto-format`, `unic-confluence`, `release-tools`, `biome-config`, `tsconfig` | One scope per app or package | +| `chore` | `ci`, `deps`, `biome`, `prettier`, `eslint`, `ts`, `vscode` | Per tooling concern | +| `docs` | `pr-review`, `auto-format`, `unic-agents-plugins` | Per plugin or repo-wide | +| `chore(release)` | _(no scope, or plugin name)_ | Version bumps, tags | +| `test` | plugin name or package name | Matches `feat`/`fix` scope | Once the vocabulary is agreed, optionally add a `commitlint` config (`commitlint.config.mjs`) enforcing it via the existing `commit-msg` hook slot in diff --git a/docs/issues/github-copilot-config/PRD.md b/docs/issues/github-copilot-config/PRD.md index f7a747e..8df2e3c 100644 --- a/docs/issues/github-copilot-config/PRD.md +++ b/docs/issues/github-copilot-config/PRD.md @@ -6,7 +6,7 @@ created: 2026-05-12 **Status:** ready-for-agent **Category:** enhancement -> *This was generated by AI during triage.* +> _This was generated by AI during triage._ ## Problem Statement diff --git a/docs/issues/plugin-unic-prefix/PRD.md b/docs/issues/plugin-unic-prefix/PRD.md index 94b652f..3f873fe 100644 --- a/docs/issues/plugin-unic-prefix/PRD.md +++ b/docs/issues/plugin-unic-prefix/PRD.md @@ -6,17 +6,17 @@ created: 2026-05-12 **Status:** ready-for-agent **Category:** enhancement -> *This was generated by AI during triage.* +> _This was generated by AI during triage._ ## Problem Statement Plugin names are inconsistent across the monorepo: -| Plugin | Current name | Command prefix | -|---|---|---| +| Plugin | Current name | Command prefix | +| ---------------------------------- | ----------------- | ---------------------- | | `apps/claude-code/unic-confluence` | `unic-confluence` | `/unic-confluence:…` ✓ | -| `apps/claude-code/pr-review` | `pr-review` | `/pr-review:…` ✗ | -| `apps/claude-code/auto-format` | `auto-format` | `/auto-format:…` ✗ | +| `apps/claude-code/pr-review` | `pr-review` | `/pr-review:…` ✗ | +| `apps/claude-code/auto-format` | `auto-format` | `/auto-format:…` ✗ | The `unic-confluence` plugin already follows the desired `unic-` pattern. `pr-review` and `auto-format` do not, making it visually ambiguous whether a command From e843e7843b35020e4f0a0822a17b646aaf5e3269 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 13:14:35 +0200 Subject: [PATCH 09/16] fix(pr-review): convert static imports to dynamic in agent prompts and orchestrator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Static `import ... from ` 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. --- apps/claude-code/pr-review/.agents/ado-fetcher.md | 4 ++-- apps/claude-code/pr-review/.agents/ado-writer.md | 10 ++++++++-- apps/claude-code/pr-review/CHANGELOG.md | 2 +- apps/claude-code/pr-review/commands/review-pr.md | 4 ++-- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/apps/claude-code/pr-review/.agents/ado-fetcher.md b/apps/claude-code/pr-review/.agents/ado-fetcher.md index 87c674c..a536508 100644 --- a/apps/claude-code/pr-review/.agents/ado-fetcher.md +++ b/apps/claude-code/pr-review/.agents/ado-fetcher.md @@ -62,7 +62,7 @@ ITER_RESULT=$( ITERATIONS_JSON_STR="$ITERATIONS_JSON" \ PLUGIN_R="$PLUGIN_ROOT" \ node --input-type=module << 'EOJS' -import { parseIterations } from `file://${process.env.PLUGIN_R}/scripts/ado-fetcher.mjs` +const { parseIterations } = await import(`file://${process.env.PLUGIN_R}/scripts/ado-fetcher.mjs`) const value = JSON.parse(process.env.ITERATIONS_JSON_STR).value ?? [] const result = parseIterations(value) process.stdout.write(JSON.stringify(result)) @@ -197,7 +197,7 @@ WORK_ITEM_IDS=$( WI_RESP="$WI_RESPONSE" \ PLUGIN_R="$PLUGIN_ROOT" \ node --input-type=module << 'EOJS' -import { parseWorkItemIds } from `file://${process.env.PLUGIN_R}/scripts/ado-fetcher.mjs` +const { parseWorkItemIds } = await import(`file://${process.env.PLUGIN_R}/scripts/ado-fetcher.mjs`) const response = process.env.WI_RESP ? JSON.parse(process.env.WI_RESP) : null const ids = parseWorkItemIds(response) process.stdout.write(JSON.stringify(ids)) diff --git a/apps/claude-code/pr-review/.agents/ado-writer.md b/apps/claude-code/pr-review/.agents/ado-writer.md index 72ba1da..e353186 100644 --- a/apps/claude-code/pr-review/.agents/ado-writer.md +++ b/apps/claude-code/pr-review/.agents/ado-writer.md @@ -277,7 +277,7 @@ rm -f /tmp/ado_writer_thread_*.json /tmp/ado_writer_thread_*.err /tmp/ado_writer ## Output -Parse the result using the helper script and return the following structured block as your final output. This block is consumed verbatim by the orchestrator: +Emit the structured result block as your final output, validating it round-trips through the `parseAdoWriterResult` helper before printing. This block is consumed verbatim by the orchestrator: ```bash RESULT=$( @@ -285,8 +285,14 @@ RESULT=$( FP="${FINDINGS_POSTED}" \ PLUGIN_R="${PLUGIN_ROOT}" \ node --input-type=module << 'EOJS' -import { parseAdoWriterResult } from `file://${process.env.PLUGIN_R}/scripts/ado-writer.mjs` +const { parseAdoWriterResult } = await import(`file://${process.env.PLUGIN_R}/scripts/ado-writer.mjs`) const output = `ADO_WRITER_RESULT_START\nSUMMARY_THREAD_ID: ${process.env.SID}\nFINDINGS_POSTED: ${process.env.FP}\nADO_WRITER_RESULT_END` +// Round-trip through the helper so any malformed block fails fast here, not downstream. +const parsed = parseAdoWriterResult(output) +if (parsed.summaryThreadId === null || parsed.findingsPosted === null) { + process.stderr.write('ado-writer: result block failed to parse\n') + process.exit(1) +} process.stdout.write(output) EOJS ) diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index c98e6c6..2e4f13b 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -9,7 +9,7 @@ - (none) ### Fixed -- (none) +- Convert static imports of helper modules to `await import(...)` in agent prompts — static `import` does not accept dynamic specifiers. ## [1.0.0] — 2026-05-12 diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index d17dcd8..da8485e 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -54,7 +54,7 @@ SIGNATURE_PREFIX="🤖 *Reviewed by Claude Code*" DETECT_JSON=$( RAW_T="$RAW_THREADS_JSON" SIG_P="$SIGNATURE_PREFIX" PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ node --input-type=module << 'EOJS' -import { detectPriorReview } from 'file://' + process.env.PLUGIN_R + '/scripts/re-review/detect-prior-review.mjs' +const { detectPriorReview } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/detect-prior-review.mjs') const r = detectPriorReview({ threads: JSON.parse(process.env.RAW_T || '[]'), signaturePrefix: process.env.SIG_P }) process.stdout.write(JSON.stringify({ isRereview: r.isRereview, @@ -221,7 +221,7 @@ PRE_PR_CONTEXT=$( RAW_DIFF_STR="$RAW_DIFF" \ PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ node --input-type=module << 'EOJS' -import { buildPrePrContext } from 'file://' + process.env.PLUGIN_R + '/scripts/pre-pr.mjs' +const { buildPrePrContext } = await import('file://' + process.env.PLUGIN_R + '/scripts/pre-pr.mjs') const ctx = buildPrePrContext(process.env.RAW_DIFF_STR) process.stdout.write(JSON.stringify(ctx)) EOJS From 3119400857bf7a96d558684cfe743fe2227ca9de Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 13:26:36 +0200 Subject: [PATCH 10/16] feat(pr-review): port re-review hunk parser to Node and use TMPDIR for 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 --- .../pr-review/.agents/ado-writer.md | 26 +++--- .../.agents/re-review-coordinator.md | 54 ++++++------- apps/claude-code/pr-review/CHANGELOG.md | 5 +- apps/claude-code/pr-review/package.json | 2 +- .../scripts/re-review/parse-diff-hunks.mjs | 50 ++++++++++++ .../pr-review/tests/parse-diff-hunks.test.mjs | 80 +++++++++++++++++++ 6 files changed, 171 insertions(+), 46 deletions(-) create mode 100644 apps/claude-code/pr-review/scripts/re-review/parse-diff-hunks.mjs create mode 100644 apps/claude-code/pr-review/tests/parse-diff-hunks.test.mjs diff --git a/apps/claude-code/pr-review/.agents/ado-writer.md b/apps/claude-code/pr-review/.agents/ado-writer.md index e353186..05fc09f 100644 --- a/apps/claude-code/pr-review/.agents/ado-writer.md +++ b/apps/claude-code/pr-review/.agents/ado-writer.md @@ -48,10 +48,10 @@ Every comment posted — inline or summary — **must** end with this trailer: For each finding in `FINDINGS`, post one new Inline Comment thread to ADO at the correct file path and line range. -Use a unique temp file per comment (e.g. `/tmp/ado_writer_thread_1.json`, `_2.json`, etc.). +Use a unique temp file per comment (e.g. `${TMPDIR:-/tmp}/ado_writer_thread_1.json`, `_2.json`, etc.). ```bash -cat > /tmp/ado_writer_thread_N.json << 'ENDJSON' +cat > "${TMPDIR:-/tmp}/ado_writer_thread_N.json" << 'ENDJSON' { "comments": [ { @@ -74,9 +74,9 @@ THREAD_RESPONSE=$(az devops invoke \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/ado_writer_thread_N.json \ + --in-file "${TMPDIR:-/tmp}/ado_writer_thread_N.json" \ --api-version "7.1" \ - --output json 2>/tmp/ado_writer_thread_N.err) + --output json 2>"${TMPDIR:-/tmp}/ado_writer_thread_N.err") THREAD_EXIT=$? ``` @@ -93,7 +93,7 @@ If the `az devops invoke` call fails (non-zero exit) or the response contains an ```bash if [ $THREAD_EXIT -ne 0 ] || echo "$THREAD_RESPONSE" | grep -qi '"message"'; then - cat > /tmp/ado_writer_thread_N_fallback.json << 'ENDJSON' + cat > "${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.json" << 'ENDJSON' { "comments": [ { @@ -111,7 +111,7 @@ ENDJSON --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/ado_writer_thread_N_fallback.json \ + --in-file "${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.json" \ --api-version "7.1" \ --output json) fi @@ -137,7 +137,7 @@ Branch on `MODE` and the `SUMMARY_THREAD_ID` value. Post one general thread **without** `threadContext`: ```bash -cat > /tmp/ado_writer_summary.json << 'ENDJSON' +cat > "${TMPDIR:-/tmp}/ado_writer_summary.json" << 'ENDJSON' { "comments": [ { @@ -155,7 +155,7 @@ SUMMARY_RESPONSE=$(az devops invoke \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/ado_writer_summary.json \ + --in-file "${TMPDIR:-/tmp}/ado_writer_summary.json" \ --api-version "7.1" \ --output json) @@ -206,7 +206,7 @@ If `FINDINGS_POSTED > 0`: Reply to the existing summary thread via `pullRequestThreadComments`: ```bash -cat > /tmp/ado_writer_delta.json << 'ENDJSON' +cat > "${TMPDIR:-/tmp}/ado_writer_delta.json" << 'ENDJSON' { "content": "🔄 Re-review delta — Iteration {LATEST_ITERATION_ID}\n\n{FINDINGS_POSTED} new finding(s).\n\n{BULLET_LIST_OF_NEW_FINDING_TITLES}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", "commentType": 1 @@ -219,7 +219,7 @@ az devops invoke \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${SUMMARY_THREAD_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/ado_writer_delta.json \ + --in-file "${TMPDIR:-/tmp}/ado_writer_delta.json" \ --api-version "7.1" \ --output json | node -e "process.stdout.write('Delta reply posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" ``` @@ -242,7 +242,7 @@ After Step 2 completes, post one final reply to the summary thread. This is the ```bash if [ -n "${SUMMARY_THREAD_ID}" ]; then - cat > /tmp/ado_writer_completion.json << 'ENDJSON' + cat > "${TMPDIR:-/tmp}/ado_writer_completion.json" << 'ENDJSON' { "content": "✅ Review complete — Iteration {LATEST_ITERATION_ID} ({FINDINGS_POSTED} findings posted)\n\n---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}", "commentType": 1 @@ -255,7 +255,7 @@ ENDJSON --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${SUMMARY_THREAD_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/ado_writer_completion.json \ + --in-file "${TMPDIR:-/tmp}/ado_writer_completion.json" \ --api-version "7.1" \ --output json | node -e "process.stdout.write('Completion marker posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" else @@ -270,7 +270,7 @@ The absence of this marker for `LATEST_ITERATION_ID` on the next run signals a p ## Step 4 — Clean up ```bash -rm -f /tmp/ado_writer_thread_*.json /tmp/ado_writer_thread_*.err /tmp/ado_writer_summary.json /tmp/ado_writer_delta.json /tmp/ado_writer_completion.json +rm -f "${TMPDIR:-/tmp}"/ado_writer_thread_*.json "${TMPDIR:-/tmp}"/ado_writer_thread_*.err "${TMPDIR:-/tmp}/ado_writer_summary.json" "${TMPDIR:-/tmp}/ado_writer_delta.json" "${TMPDIR:-/tmp}/ado_writer_completion.json" ``` --- diff --git a/apps/claude-code/pr-review/.agents/re-review-coordinator.md b/apps/claude-code/pr-review/.agents/re-review-coordinator.md index 4ccdd84..92a5fe1 100644 --- a/apps/claude-code/pr-review/.agents/re-review-coordinator.md +++ b/apps/claude-code/pr-review/.agents/re-review-coordinator.md @@ -43,30 +43,22 @@ SIGNATURE="🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}" Parse the raw diff text into a JSON array of `{ filePath, startLine, endLine }` objects. Store in a temp file. ```bash -DIFF_HUNKS_FILE="$(mktemp "${TMPDIR:-/tmp}/re_review_hunks_XXXXXX.json")" +DIFF_HUNKS_FILE="$(mktemp "${TMPDIR:-/tmp}/re_review_hunks_XXXXXX")" echo '[]' > "$DIFF_HUNKS_FILE" ``` -Parse hunk boundaries from `RAW_DIFF`: +Parse hunk boundaries from `RAW_DIFF` via the Node helper `parse-diff-hunks.mjs` (cross-platform; no python3 dependency): ```bash -printf '%s' "$RAW_DIFF" | python3 -c " -import sys, json, re -hunks = [] -current_file = None -for line in sys.stdin: - m = re.match(r'^diff --git a/.* b/(.*)', line.rstrip()) - if m: - current_file = '/' + m.group(1) - continue - m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@', line) - if m and current_file: - start = int(m.group(1)) - count = int(m.group(2)) if m.group(2) is not None else 1 - end = start + max(count - 1, 0) - hunks.append({'filePath': current_file, 'startLine': start, 'endLine': end}) -print(json.dumps(hunks)) -" > "$DIFF_HUNKS_FILE" +RAW_DIFF="$RAW_DIFF" \ +HUNKS_OUT_F="$DIFF_HUNKS_FILE" \ +PLUGIN_R="$PLUGIN_ROOT" \ +node --input-type=module << 'EOJS' +import { writeFileSync } from 'node:fs' +const { parseDiffHunks } = await import(`file://${process.env.PLUGIN_R}/scripts/re-review/parse-diff-hunks.mjs`) +const hunks = parseDiffHunks(process.env.RAW_DIFF ?? '') +writeFileSync(process.env.HUNKS_OUT_F, JSON.stringify(hunks)) +EOJS ``` If `RAW_DIFF` is empty, `DIFF_HUNKS_FILE` remains `[]` — this is valid for a no-new-commits path. @@ -78,7 +70,7 @@ If `RAW_DIFF` is empty, `DIFF_HUNKS_FILE` remains `[]` — this is valid for a n Call `detect-prior-review` on the raw threads JSON: ```bash -PRIOR_THREADS_FILE="$(mktemp "${TMPDIR:-/tmp}/re_review_prior_threads_XXXXXX.json")" +PRIOR_THREADS_FILE="$(mktemp "${TMPDIR:-/tmp}/re_review_prior_threads_XXXXXX")" DETECT_JSON=$( RAW_THREADS="$RAW_THREADS_JSON" \ @@ -302,7 +294,7 @@ Read the most recent bot comment from the matched thread (last comment whose con - If **new evidence** (additional analysis, different suggested fix, new code examples): post a new-evidence reply: ```bash -cat > /tmp/re_review_reply_${THREAD_ID}.json << ENDJSON +cat > "${TMPDIR:-/tmp}/re_review_reply_${THREAD_ID}.json" << ENDJSON { "content": "{NEW_EVIDENCE_CONTENT}\n\n---\n🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}", "commentType": 1 @@ -315,7 +307,7 @@ az devops invoke \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/re_review_reply_${THREAD_ID}.json \ + --in-file "${TMPDIR:-/tmp}/re_review_reply_${THREAD_ID}.json" \ --api-version "7.1" \ --output json | node -e "process.stdout.write('New-evidence reply posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" ``` @@ -325,7 +317,7 @@ az devops invoke \ Briefly acknowledge the reviewer's perspective without re-asserting the finding. Always include the ADO nudge before the signature: ```bash -cat > /tmp/re_review_reply_${THREAD_ID}.json << ENDJSON +cat > "${TMPDIR:-/tmp}/re_review_reply_${THREAD_ID}.json" << ENDJSON { "content": "{BRIEF_ACKNOWLEDGEMENT}\n\nIf you consider this resolved, please mark the thread as fixed in Azure DevOps.\n\n---\n🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}", "commentType": 1 @@ -338,7 +330,7 @@ az devops invoke \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/re_review_reply_${THREAD_ID}.json \ + --in-file "${TMPDIR:-/tmp}/re_review_reply_${THREAD_ID}.json" \ --api-version "7.1" \ --output json | node -e "process.stdout.write('Dispute acknowledgement posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" ``` @@ -347,7 +339,7 @@ az devops invoke \ ```bash # 1. Post resolution reply -cat > /tmp/re_review_reply_${THREAD_ID}.json << ENDJSON +cat > "${TMPDIR:-/tmp}/re_review_reply_${THREAD_ID}.json" << ENDJSON { "content": "Resolved as of Iteration ${LATEST_ITERATION_ID} — thanks!\n\n---\n🤖 *Reviewed by Claude Code* — Iteration ${LATEST_ITERATION_ID}", "commentType": 1 @@ -360,12 +352,12 @@ az devops invoke \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ --org "${ORG_URL}" \ --http-method POST \ - --in-file /tmp/re_review_reply_${THREAD_ID}.json \ + --in-file "${TMPDIR:-/tmp}/re_review_reply_${THREAD_ID}.json" \ --api-version "7.1" \ --output json | node -e "process.stdout.write('Resolution reply posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" # 2. PATCH thread status to fixed (2) -cat > /tmp/re_review_patch_${THREAD_ID}.json << ENDJSON +cat > "${TMPDIR:-/tmp}/re_review_patch_${THREAD_ID}.json" << ENDJSON { "status": 2 } ENDJSON @@ -375,15 +367,15 @@ az devops invoke \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${THREAD_ID}" \ --org "${ORG_URL}" \ --http-method PATCH \ - --in-file /tmp/re_review_patch_${THREAD_ID}.json \ + --in-file "${TMPDIR:-/tmp}/re_review_patch_${THREAD_ID}.json" \ --api-version "7.1" \ - --output json 2>/tmp/re_review_patch_${THREAD_ID}.err | \ + --output json 2>"${TMPDIR:-/tmp}/re_review_patch_${THREAD_ID}.err" | \ node -e " try { const d = JSON.parse(require('fs').readFileSync('/dev/stdin', 'utf8')) process.stdout.write('Thread ' + d.id + ' patched to fixed') } catch (e) { - const err = require('fs').readFileSync('/tmp/re_review_patch_${THREAD_ID}.err', 'utf8') + const err = require('fs').readFileSync(\`\${process.env.TMPDIR || '/tmp'}/re_review_patch_${THREAD_ID}.err\`, 'utf8') if (err.includes('409') || err.toLowerCase().includes('conflict')) { process.stdout.write('409 Conflict — thread resolved concurrently. Continuing.') } else { @@ -399,7 +391,7 @@ try { ```bash rm -f "$PRIOR_THREADS_FILE" "$DIFF_HUNKS_FILE" -rm -f /tmp/re_review_reply_*.json /tmp/re_review_patch_*.json /tmp/re_review_patch_*.err +rm -f "${TMPDIR:-/tmp}"/re_review_reply_*.json "${TMPDIR:-/tmp}"/re_review_patch_*.json "${TMPDIR:-/tmp}"/re_review_patch_*.err ``` --- diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index 2e4f13b..2779f21 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -6,10 +6,13 @@ - (none) ### Added -- (none) +- New `scripts/re-review/parse-diff-hunks.mjs` helper module (with 7 unit tests) that parses raw `git diff` text into per-hunk `{ filePath, startLine, endLine }` entries — pure function, no I/O, slash-prefixed file paths. ### Fixed - Convert static imports of helper modules to `await import(...)` in agent prompts — static `import` does not accept dynamic specifiers. +- Port the re-review diff-hunk parser from a `python3` heredoc to a Node helper (`parse-diff-hunks.mjs`) in `re-review-coordinator.md` Step 1 — Windows-native CI and developer machines have no `python3`, breaking the cross-platform rule. +- Replace bare `/tmp/` literals with `${TMPDIR:-/tmp}/` across `re-review-coordinator.md` (reply/patch/error files in Steps 6 and 7) and `ado-writer.md` (thread, fallback, summary, delta, completion files in Steps 1–4) so temp files honour the OS-configured temp directory. +- Drop the `.json` suffix from `mktemp ".../re_review_hunks_XXXXXX"` / `re_review_prior_threads_XXXXXX` patterns — BSD `mktemp` on macOS rejects suffixes after the `X` template. ## [1.0.0] — 2026-05-12 diff --git a/apps/claude-code/pr-review/package.json b/apps/claude-code/pr-review/package.json index fb070d9..f9b5c96 100644 --- a/apps/claude-code/pr-review/package.json +++ b/apps/claude-code/pr-review/package.json @@ -10,7 +10,7 @@ "pnpm": ">=10" }, "scripts": { - "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs tests/ado-fetcher.test.mjs tests/ado-writer.test.mjs tests/pre-pr.test.mjs", + "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs tests/ado-fetcher.test.mjs tests/ado-writer.test.mjs tests/pre-pr.test.mjs tests/parse-diff-hunks.test.mjs", "bump": "unic-bump", "sync-version": "unic-sync-version", "tag": "unic-tag", diff --git a/apps/claude-code/pr-review/scripts/re-review/parse-diff-hunks.mjs b/apps/claude-code/pr-review/scripts/re-review/parse-diff-hunks.mjs new file mode 100644 index 0000000..1e75c08 --- /dev/null +++ b/apps/claude-code/pr-review/scripts/re-review/parse-diff-hunks.mjs @@ -0,0 +1,50 @@ +// @ts-check + +/** + * @typedef {{ filePath: string, startLine: number, endLine: number }} DiffHunk + */ + +const FILE_HEADER_RE = /^diff --git a\/.* b\/(.*)$/ +const HUNK_HEADER_RE = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/ + +/** + * Parses a unified `git diff` text into an array of per-hunk ranges on the +side. + * + * Each entry is `{ filePath, startLine, endLine }`. `filePath` is slash-prefixed + * (e.g. `/src/foo.ts`) to match the format consumed by `classify-thread` and + * `match-finding`. No deduplication is performed — every hunk produces an entry. + * + * Hunk headers without a `+side` (binary diffs, pure deletes) are skipped. + * Hunk headers appearing before any `diff --git` line are ignored. + * CRLF line endings are handled transparently. + * + * Pure function. No I/O. + * + * @param {string} rawDiff + * @returns {DiffHunk[]} + */ +export function parseDiffHunks(rawDiff) { + if (!rawDiff) return [] + /** @type {DiffHunk[]} */ + const hunks = [] + /** @type {string | null} */ + let currentFile = null + + const lines = rawDiff.split(/\r?\n/) + for (const line of lines) { + const fileMatch = line.match(FILE_HEADER_RE) + if (fileMatch) { + currentFile = `/${fileMatch[1]}` + continue + } + const hunkMatch = line.match(HUNK_HEADER_RE) + if (hunkMatch && currentFile) { + const startLine = Number(hunkMatch[1]) + const count = hunkMatch[2] != null ? Number(hunkMatch[2]) : 1 + const endLine = startLine + Math.max(count - 1, 0) + hunks.push({ filePath: currentFile, startLine, endLine }) + } + } + + return hunks +} diff --git a/apps/claude-code/pr-review/tests/parse-diff-hunks.test.mjs b/apps/claude-code/pr-review/tests/parse-diff-hunks.test.mjs new file mode 100644 index 0000000..933d9a3 --- /dev/null +++ b/apps/claude-code/pr-review/tests/parse-diff-hunks.test.mjs @@ -0,0 +1,80 @@ +// @ts-check + +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' +import { parseDiffHunks } from '../scripts/re-review/parse-diff-hunks.mjs' + +describe('parseDiffHunks', () => { + it('returns [] for empty input', () => { + assert.deepEqual(parseDiffHunks(''), []) + }) + + it('parses a single-file single-hunk diff into one slash-prefixed entry', () => { + const diff = [ + 'diff --git a/src/foo.ts b/src/foo.ts', + 'index abc..def 100644', + '--- a/src/foo.ts', + '+++ b/src/foo.ts', + '@@ -10,3 +10,5 @@', + ' context', + '+added', + '+added', + ].join('\n') + const result = parseDiffHunks(diff) + assert.deepEqual(result, [{ filePath: '/src/foo.ts', startLine: 10, endLine: 14 }]) + }) + + it('preserves per-hunk granularity across multi-file diff (no dedup)', () => { + const diff = [ + 'diff --git a/src/a.ts b/src/a.ts', + '@@ -1,2 +1,2 @@', + ' x', + '+y', + '@@ -20,1 +20,3 @@', + '+a', + '+b', + '+c', + 'diff --git a/src/b.ts b/src/b.ts', + '@@ -5,1 +5,1 @@', + '-old', + '+new', + ].join('\n') + const result = parseDiffHunks(diff) + assert.deepEqual(result, [ + { filePath: '/src/a.ts', startLine: 1, endLine: 2 }, + { filePath: '/src/a.ts', startLine: 20, endLine: 22 }, + { filePath: '/src/b.ts', startLine: 5, endLine: 5 }, + ]) + }) + + it('defaults count to 1 when hunk header omits the count (@@ -1 +5 @@)', () => { + const diff = ['diff --git a/x.md b/x.md', '@@ -1 +5 @@', '+only-line'].join('\n') + const result = parseDiffHunks(diff) + assert.deepEqual(result, [{ filePath: '/x.md', startLine: 5, endLine: 5 }]) + }) + + it('skips hunk headers that lack the +side (binary diff or pure delete header)', () => { + const diff = [ + 'diff --git a/bin/blob.png b/bin/blob.png', + 'Binary files a/bin/blob.png and b/bin/blob.png differ', + 'diff --git a/src/keep.ts b/src/keep.ts', + '@@ -3,2 +3,2 @@', + '-old', + '+new', + ].join('\n') + const result = parseDiffHunks(diff) + assert.deepEqual(result, [{ filePath: '/src/keep.ts', startLine: 3, endLine: 4 }]) + }) + + it('is robust to CRLF line endings', () => { + const diff = ['diff --git a/src/foo.ts b/src/foo.ts', '@@ -10,2 +12,3 @@', ' ctx', '+a', '+b'].join('\r\n') + const result = parseDiffHunks(diff) + assert.deepEqual(result, [{ filePath: '/src/foo.ts', startLine: 12, endLine: 14 }]) + }) + + it('ignores hunk headers that appear before any diff --git line (no current file)', () => { + const diff = ['@@ -1,2 +1,2 @@', ' a', '+b'].join('\n') + const result = parseDiffHunks(diff) + assert.deepEqual(result, []) + }) +}) From 0d027f64cb3d9b3e7bd252cc6fb0e92ff1cb4257 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 13:38:17 +0200 Subject: [PATCH 11/16] refactor(pr-review): trim review-pr.md orchestrator to 200 lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/claude-code/pr-review/CHANGELOG.md | 6 +- apps/claude-code/pr-review/CLAUDE.md | 4 +- .../pr-review/commands/review-pr.md | 221 +++++------------- apps/claude-code/pr-review/package.json | 2 +- .../pr-review/scripts/mode-detection.mjs | 60 +++++ .../pr-review/tests/mode-detection.test.mjs | 72 ++++++ .../pr-review/tests/pre-pr.test.mjs | 101 ++++---- 7 files changed, 251 insertions(+), 215 deletions(-) create mode 100644 apps/claude-code/pr-review/scripts/mode-detection.mjs create mode 100644 apps/claude-code/pr-review/tests/mode-detection.test.mjs diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index 2779f21..1961e66 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -7,6 +7,10 @@ ### Added - New `scripts/re-review/parse-diff-hunks.mjs` helper module (with 7 unit tests) that parses raw `git diff` text into per-hunk `{ filePath, startLine, endLine }` entries — pure function, no I/O, slash-prefixed file paths. +- New `scripts/mode-detection.mjs` helper that consolidates `Step 4` re-review detection and exports both `detectMode()` and `formatModeEnv()` used by the orchestrator. + +### Changed +- Trim `commands/review-pr.md` from 297 lines to ≤ 200 lines to meet the PRD acceptance criterion: extracted mode-detection to a helper, factored the duplicated `MODE`/`SUMMARY_THREAD_ID` write-back into a single ADO Writer prompt, consolidated the compact finding schema into one shared block referenced by Step 6 and Pre-PR Step D, and tightened instructional prose. Realigned the compact-output guidance tests to assert against the shared schema block + each section's reference, removing fragile section-slice substring assertions. ### Fixed - Convert static imports of helper modules to `await import(...)` in agent prompts — static `import` does not accept dynamic specifiers. @@ -20,7 +24,7 @@ - (none) ### Added -- Orchestrator split: `review-pr.md` refactored from a monolithic command to a thin orchestrator (~199 lines) that delegates ADO API calls and coordination logic to three focused agents +- Orchestrator split: `review-pr.md` refactored from a monolithic command to a thin orchestrator (≤ 200 lines per PRD acceptance criterion) that delegates ADO API calls and coordination logic to three focused agents - ADO Fetcher agent: handles all Azure DevOps REST API fetches (diff, threads, iterations) in a single dedicated context window - Re-review Coordinator agent: classifies prior bot threads, computes incremental diffs, and decides per-thread reply actions - ADO Writer agent: posts all inline thread comments and the summary comment back to ADO, keeping write operations isolated from analysis diff --git a/apps/claude-code/pr-review/CLAUDE.md b/apps/claude-code/pr-review/CLAUDE.md index 96eb0bd..794f9c6 100644 --- a/apps/claude-code/pr-review/CLAUDE.md +++ b/apps/claude-code/pr-review/CLAUDE.md @@ -27,7 +27,7 @@ commands/ doc-context-synthesizer.md # Doc Context Synthesizer — produces business-context narrative ``` -`commands/review-pr.md` is a thin orchestrator (~199 lines). It delegates ADO API calls and coordination logic to the focused agents in `.agents/`. There are no build steps, no transpilation, no dependencies to install. +`commands/review-pr.md` is a thin orchestrator (≤ 200 lines per PRD acceptance criterion). It delegates ADO API calls and coordination logic to the focused agents in `.agents/`. Pure helpers used by both the orchestrator and the agents live under `scripts/` (`ado-fetcher.mjs`, `ado-writer.mjs`, `pre-pr.mjs`, `mode-detection.mjs`, `confluence-client.mjs`, `re-review/*.mjs`) with tests under `tests/`. There are no build steps, no transpilation, no dependencies to install. ## Plugin metadata @@ -39,7 +39,7 @@ When bumping the version, update it in **both** files: ## Command conventions (`commands/review-pr.md`) - YAML frontmatter declares `allowed-tools` — add any new tools the command needs there -- Auto-generated files are explicitly skipped in Step 6 (serialization YAMLs, `*.g.cs`, generated types output, `swagger.md`) +- Auto-generated files are skipped during file-content reading by the `shouldSkipFile` helper in `scripts/pre-pr.mjs` (serialization YAMLs, `*.g.cs`, generated types output, `swagger.md`) - All comments posted to ADO **must** end with the exact signature: `---\n🤖 *Reviewed by Claude Code* — Iteration N` (where N = LATEST_ITERATION_ID) - ADO REST calls (`pullRequestThreads`, thread replies, iteration fetches) are handled by the focused agents in `.agents/`, not inline in the orchestrator command - ADO Fetcher (`ado-fetcher.md`) owns all read operations; ADO Writer (`ado-writer.md`) owns all write operations; Re-review Coordinator (`re-review-coordinator.md`) owns thread classification and incremental diff logic diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index da8485e..9a8a80b 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -8,74 +8,74 @@ description: 'Review an Azure DevOps pull request: fetch diff, run multi-agent a **Arguments:** "$ARGUMENTS" ---- +Thin orchestrator that detects one of three modes — Pre-PR, First-review, Re-review — and delegates to focused agents. -## Step 1 — Prerequisites (always) +## Constants -Verify `pr-review-toolkit` is available (`pr-review-toolkit:code-reviewer` agent). If missing, stop and tell the user to install and enable it via Claude Code settings → Plugins. +- `SIGNATURE_PREFIX` = `🤖 *Reviewed by Claude Code*` — never alter; re-review detection depends on it. +- ADO Writer appends `---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}` to every posted comment. -Verify `git` is available: `git --version` +### Compact finding schema ---- +Every review aspect agent prompt (Step 6, Step D) ends with this exact contract: -## Step 2 — Parse arguments and detect mode +``` +Return your findings as a JSON array. Each element must have exactly these six fields: +- severity: "critical" | "important" | "minor" +- filePath: string — leading /, forward slashes, matching ADO format (e.g. /src/foo.ts) +- startLine: integer — first line of the relevant range +- endLine: integer — last line of the relevant range (same as startLine for single-line findings) +- title: string — one line, ≤ 80 chars +- body: string — one paragraph; the exact text to post as the ADO comment or local-interface comment -Extract a PR URL from `$ARGUMENTS`. Expected format: -`https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}` +Keep reasoning and supporting evidence inside your own context. Do not include code quotes, prose reasoning, or any text outside the JSON array in your return value. +``` -**GitHub URLs** are not supported — tell the user and stop. +### Aspect-filter selection (used in Step 6 and Pre-PR Step D) -If **no URL** provided → `MODE=pre-pr` → jump to [Pre-PR mode](#pre-pr-mode). +Parse `$ARGUMENTS` for an aspect filter (`code` | `errors` | `tests` | `comments` | `types` | `all`); default `all`. Always run `pr-review-toolkit:code-reviewer` and `pr-review-toolkit:silent-failure-hunter`. Also run `pr-review-toolkit:pr-test-analyzer` if test files changed, `pr-review-toolkit:comment-analyzer` if docs/comments were added, and `pr-review-toolkit:type-design-analyzer` if new types were introduced. -Extract: `ORG_URL=https://dev.azure.com/{org}`, `PROJECT={project}`, `PR_ID={id}` +## Step 1 — Prerequisites ---- +Verify `pr-review-toolkit` is enabled (e.g. the `pr-review-toolkit:code-reviewer` agent exists). If missing, stop with installation instructions. Verify `git --version` succeeds. -## Step 3 — Azure CLI check (PR modes only) +## Step 2 — Parse arguments and detect mode -Run `az --version` and check `az extension list` for `azure-devops`. If missing: `az extension add --name azure-devops` +Extract a PR URL from `$ARGUMENTS`. Expected format: `https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}`. GitHub URLs are not supported. ---- +- **No URL** → `MODE=pre-pr` → jump to [Pre-PR mode](#pre-pr-mode). +- **URL present** → extract `ORG_URL`, `PROJECT`, `PR_ID` and continue. -## Step 4 — Mode detection +## Step 3 — Azure CLI check (PR modes only) -Fetch the full thread list **once** — captured here and passed forward; never re-fetched downstream. +Run `az --version` and `az extension list | grep azure-devops`. If missing: `az extension add --name azure-devops`. -```bash -RAW_THREADS_JSON=$(az repos pr thread list \ - --id "$PR_ID" --org "$ORG_URL" --output json 2>/dev/null) || RAW_THREADS_JSON="[]" -``` +## Step 4 — Re-review detection -Check for a prior Bot Signature: +Fetch the thread list **once**; never re-fetch downstream. ```bash -SIGNATURE_PREFIX="🤖 *Reviewed by Claude Code*" +RAW_THREADS_JSON=$(az repos pr thread list \ + --id "$PR_ID" --org "$ORG_URL" --output json 2>/dev/null) || RAW_THREADS_JSON="[]" -DETECT_JSON=$( - RAW_T="$RAW_THREADS_JSON" SIG_P="$SIGNATURE_PREFIX" PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ +eval "$( + RAW_T="$RAW_THREADS_JSON" SIG_P="🤖 *Reviewed by Claude Code*" PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ node --input-type=module << 'EOJS' -const { detectPriorReview } = await import('file://' + process.env.PLUGIN_R + '/scripts/re-review/detect-prior-review.mjs') -const r = detectPriorReview({ threads: JSON.parse(process.env.RAW_T || '[]'), signaturePrefix: process.env.SIG_P }) -process.stdout.write(JSON.stringify({ - isRereview: r.isRereview, - priorIterationId: r.priorIterationId != null ? String(r.priorIterationId) : '', - summaryThreadId: r.summaryThread != null ? String(r.summaryThread.threadId) : '', -})) +const { detectMode, formatModeEnv } = await import(`file://${process.env.PLUGIN_R}/scripts/mode-detection.mjs`) +const threads = JSON.parse(process.env.RAW_T || '[]') +process.stdout.write(formatModeEnv(detectMode({ threads, signaturePrefix: process.env.SIG_P }))) EOJS -) - -IS_REREVIEW=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).isRereview))") -PRIOR_ITERATION_ID=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).priorIterationId)") -SUMMARY_THREAD_ID=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).summaryThreadId)") +)" -[ "$IS_REREVIEW" = "true" ] && MODE="re-review" || MODE="first-review" echo "Mode detected: $MODE" ``` ---- +After this block: `MODE`, `IS_REREVIEW`, `PRIOR_ITERATION_ID`, and `SUMMARY_THREAD_ID` are set. ## Step 5 — ADO Fetcher +Launch the ADO Fetcher agent and **wait for its result** before launching anything else (the PRD requires the Fetcher to complete before the Doc Context Orchestrator and review aspect agents run). + ```txt Agent( subagent_type: "pr-review:ado-fetcher", @@ -88,15 +88,13 @@ Agent( ) ``` -Store full output as `ADO_FETCHER_RESULT`. Parse `LATEST_ITERATION_ID`, `REPO_ID`, `CHANGED_FILES`, `RAW_DIFF`, `WORK_ITEM_IDS` from the `ADO_FETCHER_RESULT_START/END` block. +Store the full output as `ADO_FETCHER_RESULT`. Parse `LATEST_ITERATION_ID`, `REPO_ID`, `CHANGED_FILES`, `RAW_DIFF`, and `WORK_ITEM_IDS` from the `ADO_FETCHER_RESULT_START`/`ADO_FETCHER_RESULT_END` block. ---- +## Step 6 — Doc Context Orchestrator + review aspect agents (parallel) -## Step 6 — Doc Context Orchestrator + review agents (parallel) +Launch both groups concurrently in a **single message**. -Launch all of the following in a **single message**: - -**Doc Context Orchestrator:** +**Doc Context Orchestrator** — gathers business context. The returned text is stored as `DOC_CONTEXT` and surfaced in the final user-facing summary; it is **not** prepended to review aspect agent prompts (those run in parallel with the orchestrator and cannot block on its output). ```txt Agent( @@ -114,52 +112,13 @@ Agent( ) ``` -Store output as `DOC_CONTEXT`. - -**Review aspect agents** — parse `$ARGUMENTS` for aspect filter (`code`/`errors`/`tests`/`comments`/`types`/`all`); default `all`. Always run `pr-review-toolkit:code-reviewer` and `pr-review-toolkit:silent-failure-hunter`. Also run `pr-review-toolkit:pr-test-analyzer` if test files changed, `pr-review-toolkit:comment-analyzer` if docs/comments added, `pr-review-toolkit:type-design-analyzer` if new types introduced. - -For each agent provide: PR title + description, full diff, changed file contents. Prepend `DOC_CONTEXT` as preamble if non-empty. - -Each agent prompt **must** end with the following output contract: - -``` -Return your findings as a JSON array. Each element must have exactly these six fields: -- severity: "critical" | "important" | "minor" -- filePath: string — leading /, forward slashes, matching ADO format (e.g. /src/foo.ts) -- startLine: integer — first line of the relevant range -- endLine: integer — last line of the relevant range (same as startLine for single-line findings) -- title: string — one line, ≤ 80 chars -- body: string — one paragraph; the exact text to post as the ADO comment or local-interface comment - -Keep your reasoning, analysis, and supporting evidence inside your own context. -Do not include code quotes, prose reasoning, or any text outside the JSON array in your return value. -``` +**Review aspect agents** — apply the [aspect-filter selection](#aspect-filter-selection-used-in-step-6-and-pre-pr-step-d) above. For each selected agent, pass: PR title + description, full diff, and changed file contents. Every prompt **must** end with the [compact finding schema](#compact-finding-schema) block verbatim. Collect the JSON arrays returned by all agents. Deduplicate and sort by severity (`critical` first). Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. ---- - ## Step 7 — Write-back (branch on mode) -### First-review - -```txt -Agent( - subagent_type: "pr-review:ado-writer", - prompt: "Post all ADO comments for this first-review. - ORG_URL: {ORG_URL} - PROJECT: {PROJECT} - REPO_ID: {REPO_ID} - PR_ID: {PR_ID} - LATEST_ITERATION_ID: {LATEST_ITERATION_ID} - SUMMARY_THREAD_ID: - MODE: first-review - PLUGIN_ROOT: {CLAUDE_PLUGIN_ROOT} - FINDINGS: {FINDINGS_JSON}" -) -``` - -### Re-review +**Re-review only** — first run the coordinator, parse `RE_REVIEW_COORDINATOR_RESULT_START`/`_END`, extract `earlyExit` and `freshFindings`. If `earlyExit: true`, stop; otherwise reassign `FINDINGS_JSON` to `freshFindings`. ```txt Agent( @@ -175,106 +134,62 @@ Agent( ) ``` -Parse `RE_REVIEW_COORDINATOR_RESULT_START/END`. Extract `earlyExit` and `freshFindings`. - -If `earlyExit: true` — stop here; do **not** invoke ADO Writer. - -Otherwise: +**Both modes** — invoke ADO Writer. For first-review, `MODE=first-review` and `SUMMARY_THREAD_ID=""`. For re-review, both come from Step 4. ```txt Agent( subagent_type: "pr-review:ado-writer", - prompt: "Post all ADO comments for this re-review. + prompt: "Post all ADO comments for this {MODE} run. ORG_URL: {ORG_URL} PROJECT: {PROJECT} REPO_ID: {REPO_ID} PR_ID: {PR_ID} LATEST_ITERATION_ID: {LATEST_ITERATION_ID} SUMMARY_THREAD_ID: {SUMMARY_THREAD_ID} - MODE: re-review + MODE: {MODE} PLUGIN_ROOT: {CLAUDE_PLUGIN_ROOT} - FINDINGS: {FRESH_FINDINGS_JSON}" + FINDINGS: {FINDINGS_JSON}" ) ``` ---- - ## Pre-PR mode -**Pre-PR mode active** — no PR URL provided. Reviewing local branch diff; no ADO calls will be made. +No PR URL provided — reviewing the local branch diff; no ADO calls are made. -### Step A — Detect default branch and compute diff +### Step A — Compute diff ```bash -# Detect the default remote branch (main or develop) DEFAULT_BRANCH=$(git remote show origin 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}' || echo "main") - -RAW_DIFF=$(git diff "origin/${DEFAULT_BRANCH}...HEAD") +RAW_DIFF=$(git diff "origin/${DEFAULT_BRANCH}...HEAD") || { echo "git diff failed"; exit 1; } ``` -If `git diff` fails (e.g. no upstream remote), inform the user and stop. - ### Step B — Parse changed files ```bash -PRE_PR_CONTEXT=$( - RAW_DIFF_STR="$RAW_DIFF" \ - PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ +FILTERED_FILES=$( + RAW_DIFF_STR="$RAW_DIFF" PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ node --input-type=module << 'EOJS' -const { buildPrePrContext } = await import('file://' + process.env.PLUGIN_R + '/scripts/pre-pr.mjs') -const ctx = buildPrePrContext(process.env.RAW_DIFF_STR) -process.stdout.write(JSON.stringify(ctx)) +const { buildPrePrContext } = await import(`file://${process.env.PLUGIN_R}/scripts/pre-pr.mjs`) +process.stdout.write(buildPrePrContext(process.env.RAW_DIFF_STR).filteredFiles.join('\n')) EOJS ) - -FILTERED_FILES=$(printf '%s' "$PRE_PR_CONTEXT" | node -e " -const chunks = [] -process.stdin.on('data', c => chunks.push(c)) -process.stdin.on('end', () => { - const ctx = JSON.parse(Buffer.concat(chunks).toString()) - process.stdout.write(ctx.filteredFiles.join('\n')) -})") ``` -Read the contents of each file in `FILTERED_FILES` (skip any that are deleted or unavailable). +Read the contents of each file in `FILTERED_FILES`, skipping deleted ones. ### Step C — Resolve aspect filter -Parse `$ARGUMENTS` for aspect filter (`code`/`errors`/`tests`/`comments`/`types`/`all`); default `all`. -Use the same selection logic as ADO modes: always run `pr-review-toolkit:code-reviewer` and `pr-review-toolkit:silent-failure-hunter`. Also run `pr-review-toolkit:pr-test-analyzer` if test files changed, `pr-review-toolkit:comment-analyzer` if docs/comments added, `pr-review-toolkit:type-design-analyzer` if new types introduced. +Apply the [aspect-filter selection](#aspect-filter-selection-used-in-step-6-and-pre-pr-step-d) defined above. ### Step D — Run review aspect agents -Doc Context is skipped (no PR URL means no work items to fetch). - -Launch all applicable review aspect agents in a single message, passing: - -- The raw diff (`RAW_DIFF`) -- Changed file contents -- No preamble (Doc Context is empty in pre-PR mode) - -Each agent prompt **must** end with the same output contract used in ADO modes: - -``` -Return your findings as a JSON array. Each element must have exactly these six fields: -- severity: "critical" | "important" | "minor" -- filePath: string — leading /, forward slashes (e.g. /src/foo.ts) -- startLine: integer — first line of the relevant range -- endLine: integer — last line of the relevant range (same as startLine for single-line findings) -- title: string — one line, ≤ 80 chars -- body: string — one paragraph; the exact text to post as the comment - -Keep your reasoning, analysis, and supporting evidence inside your own context. -Do not include code quotes, prose reasoning, or any text outside the JSON array in your return value. -``` +Doc Context is skipped (no work items without a PR). Launch all selected review aspect agents in a **single message**, passing `RAW_DIFF` and changed file contents. Every prompt **must** end with the [compact finding schema](#compact-finding-schema) verbatim; in Pre-PR mode the `body` field reads "exact text to post as the comment" (rendered in the Claude interface, not written back to ADO). -Collect the JSON arrays returned by all agents. Deduplicate and sort by severity (`critical` first). Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. +Collect, dedupe, and sort returned JSON arrays into `FINDINGS` (`critical` first). ### Step E — Present findings -Present all findings directly in the Claude interface as a structured list — no ADO write-back occurs in pre-PR mode. - -For each finding print: +Print each finding in the Claude interface, grouped by severity (`critical`, `important`, `minor`): ``` [{severity}] {filePath} L{startLine}–{endLine} @@ -282,16 +197,4 @@ For each finding print: {body} ``` -Group by severity: `critical` first, then `important`, then `minor`. Print a summary count at the end. - -If no findings, print: `✅ Pre-PR review complete — no issues found.` - -Otherwise, print: `✅ Pre-PR review complete — {N} finding(s). Open a PR to post these as inline ADO comments.` - ---- - -## Comment signature - -Every comment must end with `---\n🤖 *Reviewed by Claude Code* — Iteration {LATEST_ITERATION_ID}`. - -`SIGNATURE_PREFIX` = `🤖 *Reviewed by Claude Code*` — never alter; re-review detection depends on it. +End with `✅ Pre-PR review complete — {N} finding(s).` (or `no issues found.` when `N == 0`). diff --git a/apps/claude-code/pr-review/package.json b/apps/claude-code/pr-review/package.json index f9b5c96..77685d8 100644 --- a/apps/claude-code/pr-review/package.json +++ b/apps/claude-code/pr-review/package.json @@ -10,7 +10,7 @@ "pnpm": ">=10" }, "scripts": { - "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs tests/ado-fetcher.test.mjs tests/ado-writer.test.mjs tests/pre-pr.test.mjs tests/parse-diff-hunks.test.mjs", + "test": "node --test tests/parse-signature.test.mjs tests/classify-thread.test.mjs tests/match-finding.test.mjs tests/detect-prior-review.test.mjs tests/confluence-client.test.mjs tests/ado-fetcher.test.mjs tests/ado-writer.test.mjs tests/pre-pr.test.mjs tests/parse-diff-hunks.test.mjs tests/mode-detection.test.mjs", "bump": "unic-bump", "sync-version": "unic-sync-version", "tag": "unic-tag", diff --git a/apps/claude-code/pr-review/scripts/mode-detection.mjs b/apps/claude-code/pr-review/scripts/mode-detection.mjs new file mode 100644 index 0000000..d2a166a --- /dev/null +++ b/apps/claude-code/pr-review/scripts/mode-detection.mjs @@ -0,0 +1,60 @@ +// @ts-check + +import { detectPriorReview } from './re-review/detect-prior-review.mjs' + +/** + * @typedef {{ + * mode: 'first-review' | 're-review', + * isRereview: boolean, + * priorIterationId: string, + * summaryThreadId: string, + * }} ModeDetectionResult + */ + +/** + * Classifies a PR as `first-review` or `re-review` from its already-fetched + * thread list. Wraps `detectPriorReview` and stringifies the optional numeric + * fields so the orchestrator can consume them via plain shell. + * + * Pure function. No I/O. + * + * @param {{ threads: unknown[], signaturePrefix: string }} input + * @returns {ModeDetectionResult} + */ +export function detectMode({ threads, signaturePrefix }) { + const r = detectPriorReview({ + // detect-prior-review accepts the raw ADO thread shape; the orchestrator + // passes whatever `az repos pr thread list` returned, untouched. + // eslint-disable-next-line + // @ts-ignore -- runtime-validated by detectPriorReview's own guards + threads: Array.isArray(threads) ? threads : [], + signaturePrefix, + }) + return { + mode: r.isRereview ? 're-review' : 'first-review', + isRereview: r.isRereview, + priorIterationId: r.priorIterationId != null ? String(r.priorIterationId) : '', + summaryThreadId: r.summaryThread != null ? String(r.summaryThread.threadId) : '', + } +} + +/** + * Formats a `ModeDetectionResult` as four newline-separated shell-friendly + * lines, intended to be eval-captured by the orchestrator: + * + * MODE=first-review + * IS_REREVIEW=false + * PRIOR_ITERATION_ID= + * SUMMARY_THREAD_ID= + * + * @param {ModeDetectionResult} result + * @returns {string} + */ +export function formatModeEnv(result) { + return [ + `MODE=${result.mode}`, + `IS_REREVIEW=${result.isRereview ? 'true' : 'false'}`, + `PRIOR_ITERATION_ID=${result.priorIterationId}`, + `SUMMARY_THREAD_ID=${result.summaryThreadId}`, + ].join('\n') +} diff --git a/apps/claude-code/pr-review/tests/mode-detection.test.mjs b/apps/claude-code/pr-review/tests/mode-detection.test.mjs new file mode 100644 index 0000000..aef8671 --- /dev/null +++ b/apps/claude-code/pr-review/tests/mode-detection.test.mjs @@ -0,0 +1,72 @@ +// @ts-check + +import assert from 'node:assert/strict' +import { describe, it } from 'node:test' +import { detectMode, formatModeEnv } from '../scripts/mode-detection.mjs' + +const SIGNATURE_PREFIX = '🤖 *Reviewed by Claude Code*' + +describe('detectMode', () => { + it('no threads → first-review with empty fields', () => { + const r = detectMode({ threads: [], signaturePrefix: SIGNATURE_PREFIX }) + assert.equal(r.mode, 'first-review') + assert.equal(r.isRereview, false) + assert.equal(r.priorIterationId, '') + assert.equal(r.summaryThreadId, '') + }) + + it('non-array threads → first-review (defensive)', () => { + // @ts-expect-error — exercising defensive path + const r = detectMode({ threads: null, signaturePrefix: SIGNATURE_PREFIX }) + assert.equal(r.mode, 'first-review') + assert.equal(r.isRereview, false) + }) + + it('threads without signature → first-review', () => { + const threads = [{ id: 1, comments: [{ content: 'hello from a human' }] }] + const r = detectMode({ threads, signaturePrefix: SIGNATURE_PREFIX }) + assert.equal(r.mode, 'first-review') + assert.equal(r.isRereview, false) + }) + + it('thread with signature and iteration → re-review with stringified fields', () => { + const threads = [ + { + id: 42, + threadContext: null, + comments: [ + { + content: `## PR Review Summary\n\nfoo\n\n---\n${SIGNATURE_PREFIX} — Iteration 3`, + }, + ], + }, + ] + const r = detectMode({ threads, signaturePrefix: SIGNATURE_PREFIX }) + assert.equal(r.mode, 're-review') + assert.equal(r.isRereview, true) + assert.equal(r.priorIterationId, '3') + assert.equal(r.summaryThreadId, '42') + }) +}) + +describe('formatModeEnv', () => { + it('emits four KEY=value lines for first-review', () => { + const r = detectMode({ threads: [], signaturePrefix: SIGNATURE_PREFIX }) + const env = formatModeEnv(r) + assert.equal( + env, + ['MODE=first-review', 'IS_REREVIEW=false', 'PRIOR_ITERATION_ID=', 'SUMMARY_THREAD_ID='].join('\n') + ) + }) + + it('emits stringified IDs for re-review', () => { + const r = { + /** @type {'re-review'} */ mode: /** @type {const} */ ('re-review'), + isRereview: true, + priorIterationId: '3', + summaryThreadId: '42', + } + const env = formatModeEnv(r) + assert.equal(env, ['MODE=re-review', 'IS_REREVIEW=true', 'PRIOR_ITERATION_ID=3', 'SUMMARY_THREAD_ID=42'].join('\n')) + }) +}) diff --git a/apps/claude-code/pr-review/tests/pre-pr.test.mjs b/apps/claude-code/pr-review/tests/pre-pr.test.mjs index 18dc996..4eee8c2 100644 --- a/apps/claude-code/pr-review/tests/pre-pr.test.mjs +++ b/apps/claude-code/pr-review/tests/pre-pr.test.mjs @@ -176,91 +176,88 @@ describe('review-pr command — compact sub-agent output guidance', () => { /** Pre-PR step D — the review-agent launch step in pre-PR mode */ const stepDSection = commandContent.slice(commandContent.indexOf('### Step D'), commandContent.indexOf('### Step E')) - it('Step 6 instructs agents to return a JSON array of findings', () => { - assert.ok( - step6Section.includes('JSON') && step6Section.includes('array'), - 'Step 6 must instruct review agents to return a JSON array of findings' - ) + /** The shared "Compact finding schema" block referenced by both Step 6 and Step D */ + const schemaStart = commandContent.indexOf('### Compact finding schema') + const schemaEnd = commandContent.indexOf('### Aspect-filter selection') + const schemaSection = schemaStart >= 0 && schemaEnd > schemaStart ? commandContent.slice(schemaStart, schemaEnd) : '' + + it('orchestrator defines a single Compact finding schema block', () => { + assert.ok(schemaSection.length > 0, 'review-pr.md must define a "### Compact finding schema" block') }) - it('Step 6 requires all six finding fields in agent prompt', () => { - const requiredFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] - for (const field of requiredFields) { - assert.ok(step6Section.includes(field), `Step 6 agent prompt must mention required finding field: ${field}`) - } + it('Step 6 references the compact finding schema', () => { + assert.ok( + step6Section.toLowerCase().includes('compact finding schema'), + 'Step 6 must reference the shared compact finding schema' + ) }) - it('Step 6 instructs agents to omit code quotes from return value', () => { + it('Step D references the compact finding schema', () => { assert.ok( - step6Section.includes('no code quote') || - step6Section.includes('omit code quote') || - step6Section.includes('no code quotes') || - step6Section.includes('omit code quotes') || - step6Section.includes('without code quote') || - step6Section.includes('code quotes') || - step6Section.toLowerCase().includes('code quote'), - 'Step 6 must instruct agents to omit code quotes from the return value' + stepDSection.toLowerCase().includes('compact finding schema'), + 'Step D must reference the shared compact finding schema' ) }) - it('Step 6 instructs agents to omit prose reasoning from return value', () => { + it('schema instructs agents to return a JSON array of findings', () => { assert.ok( - step6Section.toLowerCase().includes('reasoning') || - step6Section.toLowerCase().includes('prose') || - step6Section.toLowerCase().includes('analysis') || - step6Section.toLowerCase().includes('supporting'), - 'Step 6 must instruct agents to keep reasoning inside their own context, not in return value' + schemaSection.includes('JSON') && schemaSection.includes('array'), + 'Compact finding schema must instruct review agents to return a JSON array of findings' ) }) - it('Step 6 severity values are exactly critical / important / minor', () => { - assert.ok(step6Section.includes('critical'), 'Step 6 must specify "critical" as a severity value') - assert.ok(step6Section.includes('important'), 'Step 6 must specify "important" as a severity value') - assert.ok(step6Section.includes('minor'), 'Step 6 must specify "minor" as a severity value') + it('schema requires all six finding fields', () => { + const requiredFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] + for (const field of requiredFields) { + assert.ok(schemaSection.includes(field), `Compact finding schema must mention required field: ${field}`) + } }) - it('Step 6 requires filePath to use leading slash and forward slashes', () => { + it('schema instructs agents to omit code quotes from return value', () => { assert.ok( - step6Section.includes('leading') || step6Section.includes('forward slash') || step6Section.includes('leading /'), - 'Step 6 must require filePath with leading slash and forward slashes matching ADO format' + schemaSection.toLowerCase().includes('code quote'), + 'Compact finding schema must instruct agents to omit code quotes from the return value' ) }) - it('Step 6 requires title to be one line capped at 80 chars', () => { + it('schema instructs agents to omit prose reasoning from return value', () => { assert.ok( - step6Section.includes('80') || step6Section.includes('one line') || step6Section.includes('≤ 80'), - 'Step 6 must require title to be one line, at most 80 characters' + schemaSection.toLowerCase().includes('reasoning') || + schemaSection.toLowerCase().includes('prose') || + schemaSection.toLowerCase().includes('analysis'), + 'Compact finding schema must instruct agents to keep reasoning inside their own context, not in return value' ) }) - it('Step 6 requires body to be exactly the text to post as comment (no code quotes)', () => { + it('schema severity values are exactly critical / important / minor', () => { + assert.ok(schemaSection.includes('critical'), 'Compact finding schema must specify "critical" as a severity value') assert.ok( - step6Section.includes('body') && (step6Section.includes('post') || step6Section.includes('comment')), - 'Step 6 must describe body as the exact text to post as the ADO or local-interface comment' + schemaSection.includes('important'), + 'Compact finding schema must specify "important" as a severity value' ) + assert.ok(schemaSection.includes('minor'), 'Compact finding schema must specify "minor" as a severity value') }) - it('Step D instructs agents to return structured JSON findings (same schema as ADO modes)', () => { + it('schema requires filePath to use leading slash and forward slashes', () => { assert.ok( - stepDSection.includes('JSON') || stepDSection.includes('structured'), - 'Step D must instruct review agents to return structured JSON findings' + schemaSection.includes('leading') || + schemaSection.includes('forward slash') || + schemaSection.includes('leading /'), + 'Compact finding schema must require filePath with leading slash and forward slashes matching ADO format' ) }) - it('Step D requires same six finding fields as Step 6', () => { - const requiredFields = ['severity', 'filePath', 'startLine', 'endLine', 'title', 'body'] - for (const field of requiredFields) { - assert.ok(stepDSection.includes(field), `Step D agent prompt must mention required finding field: ${field}`) - } + it('schema requires title to be one line capped at 80 chars', () => { + assert.ok( + schemaSection.includes('80') || schemaSection.includes('one line') || schemaSection.includes('≤ 80'), + 'Compact finding schema must require title to be one line, at most 80 characters' + ) }) - it('Step D instructs agents to omit code quotes and prose reasoning from return value', () => { + it('schema describes body as the exact text to post as the ADO or local-interface comment', () => { assert.ok( - stepDSection.toLowerCase().includes('code quote') || - stepDSection.toLowerCase().includes('reasoning') || - stepDSection.toLowerCase().includes('prose') || - stepDSection.toLowerCase().includes('analysis'), - 'Step D must instruct agents to keep reasoning inside their own context, not in return value' + schemaSection.includes('body') && (schemaSection.includes('post') || schemaSection.includes('comment')), + 'Compact finding schema must describe body as the exact text to post as the ADO or local-interface comment' ) }) }) From 27081d9ec979e09927c7a37ca780642d94c9b11d Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 13:44:58 +0200 Subject: [PATCH 12/16] fix(pr-review): surface silent failures in ADO read/write and partial-run paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../pr-review/.agents/ado-fetcher.md | 5 +- .../pr-review/.agents/ado-writer.md | 50 +++++++++++++++---- .../.agents/re-review-coordinator.md | 44 ++++++++++------ apps/claude-code/pr-review/CHANGELOG.md | 5 ++ .../pr-review/commands/review-pr.md | 14 +++--- 5 files changed, 84 insertions(+), 34 deletions(-) diff --git a/apps/claude-code/pr-review/.agents/ado-fetcher.md b/apps/claude-code/pr-review/.agents/ado-fetcher.md index a536508..edb7215 100644 --- a/apps/claude-code/pr-review/.agents/ado-fetcher.md +++ b/apps/claude-code/pr-review/.agents/ado-fetcher.md @@ -129,8 +129,9 @@ git branch --show-current If it does not match, check out the PR branch: ```bash -az repos pr checkout --id "$PR_ID" --org "$ORG_URL" -# fallback: git fetch origin "$SOURCE_BRANCH" && git checkout "$SOURCE_BRANCH" +az repos pr checkout --id "$PR_ID" --org "$ORG_URL" \ + || (git fetch origin "$SOURCE_BRANCH" && git checkout "$SOURCE_BRANCH") \ + || { echo "ERROR: could not check out PR source branch $SOURCE_BRANCH" >&2; exit 1; } ``` If `PRIOR_ITERATION_ID` is non-empty, determine the incremental diff range. Fetch the prior iteration's commit SHA from the iterations list: diff --git a/apps/claude-code/pr-review/.agents/ado-writer.md b/apps/claude-code/pr-review/.agents/ado-writer.md index 05fc09f..8d7158a 100644 --- a/apps/claude-code/pr-review/.agents/ado-writer.md +++ b/apps/claude-code/pr-review/.agents/ado-writer.md @@ -89,10 +89,11 @@ Map severity to emoji before writing the content: ### threadContext rejection fallback -If the `az devops invoke` call fails (non-zero exit) or the response contains an error related to `threadContext` (file not in diff, invalid path), **retry without `threadContext`** to post as a general comment: +Decide whether the primary POST succeeded by parsing the response structurally — exit code zero **and** the response JSON contains a numeric `id`. The old substring `"message"` heuristic produced false positives on any error-shaped response and false negatives when an `id` field appeared alongside a benign `message`. If the structural check fails, **retry without `threadContext`** to post as a general comment: ```bash -if [ $THREAD_EXIT -ne 0 ] || echo "$THREAD_RESPONSE" | grep -qi '"message"'; then +THREAD_ID=$(printf '%s' "$THREAD_RESPONSE" | node -e "try { const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')); process.stdout.write(typeof d.id === 'number' ? String(d.id) : '') } catch (e) { process.stdout.write('') }") +if [ $THREAD_EXIT -ne 0 ] || [ -z "$THREAD_ID" ]; then cat > "${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.json" << 'ENDJSON' { "comments": [ @@ -113,15 +114,25 @@ ENDJSON --http-method POST \ --in-file "${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.json" \ --api-version "7.1" \ - --output json) + --output json 2>"${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.err") + FALLBACK_EXIT=$? + THREAD_ID=$(printf '%s' "$THREAD_RESPONSE" | node -e "try { const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')); process.stdout.write(typeof d.id === 'number' ? String(d.id) : '') } catch (e) { process.stdout.write('') }") fi ``` -After each successful post (primary or fallback): +**Only increment `FINDINGS_POSTED` after confirming the response contains a numeric `id`.** On confirmed failure (no numeric `id` after the fallback), emit a clear stderr message with the captured `*.err` payload and **continue to the next finding** — losing one comment is recoverable; aborting the writer loses every remaining comment. ```bash -FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) -echo "Thread posted: $(echo "$THREAD_RESPONSE" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))")" +if [ -n "$THREAD_ID" ]; then + FINDINGS_POSTED=$((FINDINGS_POSTED + 1)) + echo "Thread posted: $THREAD_ID" +else + { + echo "WARN: failed to post inline thread for finding N — continuing with remaining findings." + [ -s "${TMPDIR:-/tmp}/ado_writer_thread_N.err" ] && cat "${TMPDIR:-/tmp}/ado_writer_thread_N.err" + [ -s "${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.err" ] && cat "${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.err" + } >&2 +fi ``` --- @@ -157,9 +168,17 @@ SUMMARY_RESPONSE=$(az devops invoke \ --http-method POST \ --in-file "${TMPDIR:-/tmp}/ado_writer_summary.json" \ --api-version "7.1" \ - --output json) + --output json 2>"${TMPDIR:-/tmp}/ado_writer_summary.err") +SUMMARY_EXIT=$? -SUMMARY_THREAD_ID=$(echo "$SUMMARY_RESPONSE" | node -e "process.stdout.write(String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))") +SUMMARY_THREAD_ID=$(printf '%s' "$SUMMARY_RESPONSE" | node -e "try { const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')); process.stdout.write(typeof d.id === 'number' ? String(d.id) : '') } catch (e) { process.stdout.write('') }") + +if [ $SUMMARY_EXIT -ne 0 ] || [ -z "$SUMMARY_THREAD_ID" ]; then + echo "ERROR: failed to post review summary; aborting writer. The completion marker depends on a valid SUMMARY_THREAD_ID, and the next re-review depends on it being detectable — silently continuing here would corrupt re-review state forever." >&2 + echo "ADO response: $SUMMARY_RESPONSE" >&2 + [ -s "${TMPDIR:-/tmp}/ado_writer_summary.err" ] && cat "${TMPDIR:-/tmp}/ado_writer_summary.err" >&2 + exit 1 +fi echo "Summary thread posted: ${SUMMARY_THREAD_ID}" ``` @@ -213,7 +232,7 @@ cat > "${TMPDIR:-/tmp}/ado_writer_delta.json" << 'ENDJSON' } ENDJSON -az devops invoke \ +DELTA_RESPONSE=$(az devops invoke \ --area git \ --resource pullRequestThreadComments \ --route-parameters "project=${PROJECT}" "repositoryId=${REPO_ID}" "pullRequestId=${PR_ID}" "threadId=${SUMMARY_THREAD_ID}" \ @@ -221,7 +240,16 @@ az devops invoke \ --http-method POST \ --in-file "${TMPDIR:-/tmp}/ado_writer_delta.json" \ --api-version "7.1" \ - --output json | node -e "process.stdout.write('Delta reply posted, comment ' + String(JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')).id ?? ''))" + --output json 2>"${TMPDIR:-/tmp}/ado_writer_delta.err") +DELTA_EXIT=$? +DELTA_COMMENT_ID=$(printf '%s' "$DELTA_RESPONSE" | node -e "try { const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')); process.stdout.write(typeof d.id === 'number' ? String(d.id) : '') } catch (e) { process.stdout.write('') }") +if [ $DELTA_EXIT -ne 0 ] || [ -z "$DELTA_COMMENT_ID" ]; then + echo "ERROR: failed to post delta reply to summary thread ${SUMMARY_THREAD_ID}; aborting writer. The completion marker depends on this thread being detectable on the next re-review." >&2 + echo "ADO response: $DELTA_RESPONSE" >&2 + [ -s "${TMPDIR:-/tmp}/ado_writer_delta.err" ] && cat "${TMPDIR:-/tmp}/ado_writer_delta.err" >&2 + exit 1 +fi +echo "Delta reply posted, comment ${DELTA_COMMENT_ID}" ``` `{BULLET_LIST_OF_NEW_FINDING_TITLES}` — one bullet per finding posted in Step 1, format: @@ -270,7 +298,7 @@ The absence of this marker for `LATEST_ITERATION_ID` on the next run signals a p ## Step 4 — Clean up ```bash -rm -f "${TMPDIR:-/tmp}"/ado_writer_thread_*.json "${TMPDIR:-/tmp}"/ado_writer_thread_*.err "${TMPDIR:-/tmp}/ado_writer_summary.json" "${TMPDIR:-/tmp}/ado_writer_delta.json" "${TMPDIR:-/tmp}/ado_writer_completion.json" +rm -f "${TMPDIR:-/tmp}"/ado_writer_thread_*.json "${TMPDIR:-/tmp}"/ado_writer_thread_*.err "${TMPDIR:-/tmp}/ado_writer_summary.json" "${TMPDIR:-/tmp}/ado_writer_summary.err" "${TMPDIR:-/tmp}/ado_writer_delta.json" "${TMPDIR:-/tmp}/ado_writer_delta.err" "${TMPDIR:-/tmp}/ado_writer_completion.json" ``` --- diff --git a/apps/claude-code/pr-review/.agents/re-review-coordinator.md b/apps/claude-code/pr-review/.agents/re-review-coordinator.md index 92a5fe1..eb5232d 100644 --- a/apps/claude-code/pr-review/.agents/re-review-coordinator.md +++ b/apps/claude-code/pr-review/.agents/re-review-coordinator.md @@ -116,24 +116,40 @@ fi If `IS_REREVIEW=true`, `SUMMARY_THREAD_ID` is non-empty, and `PRIOR_ITERATION_ID` is not `"null"`, verify the prior review completed. Check the summary thread for the completion marker `✅ Review complete — Iteration {PRIOR_ITERATION_ID}`: +The Node check distinguishes three outcomes via distinct exit codes — this prevents conflating "marker missing" (legitimate partial prior run; downgrade is correct) with "check crashed" (silent downgrade would re-post every prior thread): + +- exit `0` → marker found → `MARKER_FOUND=true` (proceed normally) +- exit `1` → marker not found → `MARKER_FOUND=false` (legitimate partial run; downgrade to first-review mode) +- exit `2` or any other non-zero → the check itself crashed → **abort the coordinator with exit code 3** (do not silently downgrade) + +The orchestrator's Step 7 only treats an `earlyExit: true` block as a non-fatal skip; a non-zero coordinator exit propagates as a fatal failure that surfaces to the user and stops the run — which is the correct behaviour when the partial-run check is itself broken. + ```bash if [ "$IS_REREVIEW" = "true" ] && [ -n "$SUMMARY_THREAD_ID" ] && [ "$PRIOR_ITERATION_ID" != "null" ]; then - MARKER_FOUND=$( - THREADS_F="$PRIOR_THREADS_FILE" SID="$SUMMARY_THREAD_ID" PID="$PRIOR_ITERATION_ID" \ - node --input-type=module << 'EOJS' + THREADS_F="$PRIOR_THREADS_FILE" SID="$SUMMARY_THREAD_ID" PID="$PRIOR_ITERATION_ID" \ + node --input-type=module << 'EOJS' import { readFileSync } from 'node:fs' -const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) -const sid = Number(process.env.SID) -const prefix = '✅ Review complete — Iteration ' + process.env.PID -const found = threads.some(t => t.threadId === sid && (t.comments ?? []).some(c => (c.content ?? '').startsWith(prefix))) -console.log(found ? 'true' : 'false') +try { + const threads = JSON.parse(readFileSync(process.env.THREADS_F, 'utf8')) + const sid = Number(process.env.SID) + const prefix = '✅ Review complete — Iteration ' + process.env.PID + const found = threads.some(t => t.threadId === sid && (t.comments ?? []).some(c => (c.content ?? '').startsWith(prefix))) + process.exit(found ? 0 : 1) +} catch (e) { + process.stderr.write('PARTIAL_RUN_CHECK_ERROR: ' + e.message + '\n') + process.exit(2) +} EOJS - ) || { echo "ERROR: partial-run check failed — falling back to first-review mode."; MARKER_FOUND="false"; } - - if [ "$MARKER_FOUND" != "true" ] && [ "$MARKER_FOUND" != "false" ]; then - echo "ERROR: unexpected MARKER_FOUND value '${MARKER_FOUND}' — falling back to first-review mode." - MARKER_FOUND="false" - fi + PARTIAL_RUN_EXIT=$? + + case "$PARTIAL_RUN_EXIT" in + 0) MARKER_FOUND="true" ;; + 1) MARKER_FOUND="false" ;; + *) + echo "ERROR: partial-run check crashed unexpectedly (exit ${PARTIAL_RUN_EXIT}); refusing to silently downgrade mode." >&2 + exit 3 + ;; + esac if [ "$MARKER_FOUND" = "false" ]; then echo "No completion marker for Iteration $PRIOR_ITERATION_ID — partial prior run. Falling back to first-review mode." diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index 1961e66..d3769ae 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -17,6 +17,11 @@ - Port the re-review diff-hunk parser from a `python3` heredoc to a Node helper (`parse-diff-hunks.mjs`) in `re-review-coordinator.md` Step 1 — Windows-native CI and developer machines have no `python3`, breaking the cross-platform rule. - Replace bare `/tmp/` literals with `${TMPDIR:-/tmp}/` across `re-review-coordinator.md` (reply/patch/error files in Steps 6 and 7) and `ado-writer.md` (thread, fallback, summary, delta, completion files in Steps 1–4) so temp files honour the OS-configured temp directory. - Drop the `.json` suffix from `mktemp ".../re_review_hunks_XXXXXX"` / `re_review_prior_threads_XXXXXX` patterns — BSD `mktemp` on macOS rejects suffixes after the `X` template. +- H1 — ADO Writer Step 1 no longer bumps `FINDINGS_POSTED` unconditionally after the threadContext fallback. The substring `"message"` heuristic is replaced by a structural check (exit code + numeric `id` parsed by Node); on confirmed failure the writer logs the captured stderr from the `*.err` file and continues to the next finding rather than miscounting a missing post as success. +- H2 — ADO Writer Step 2 no longer swallows summary/delta POST failures. The summary POST and the re-review delta-reply POST now capture exit code + parsed numeric `id`; on failure the writer aborts with a non-zero exit and 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 `[]`. The fetch is now captured separately; on non-zero exit the orchestrator emits a clear stderr error ("ERROR: failed to fetch PR threads via Azure CLI. Try `az devops login` to re-authenticate.") and exits `1`, preventing a fetch failure from being mistaken for "no prior threads" and triggering a duplicate-post storm on re-review. +- H4 — Re-review Coordinator Step 3 partial-run check no longer conflates "marker missing" with "check crashed". The Node heredoc now wraps its body in try/catch and exits with distinct codes (`0` = found, `1` = not found, `2` = crash); the bash side 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 `git fetch origin "$SOURCE_BRANCH" && git checkout "$SOURCE_BRANCH"`, and aborts with a clear stderr error if both fail — previously the comment-form fallback never ran and the agent silently continued on the wrong branch. ## [1.0.0] — 2026-05-12 diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index 9a8a80b..2d1f53a 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -55,8 +55,10 @@ Run `az --version` and `az extension list | grep azure-devops`. If missing: `az Fetch the thread list **once**; never re-fetch downstream. ```bash -RAW_THREADS_JSON=$(az repos pr thread list \ - --id "$PR_ID" --org "$ORG_URL" --output json 2>/dev/null) || RAW_THREADS_JSON="[]" +PR_THREADS_ERR="${TMPDIR:-/tmp}/pr_threads.err" +RAW_THREADS_JSON=$(az repos pr thread list --id "$PR_ID" --org "$ORG_URL" --output json 2>"$PR_THREADS_ERR") || { + echo "ERROR: failed to fetch PR threads via Azure CLI. Try \`az devops login\` to re-authenticate." >&2 + cat "$PR_THREADS_ERR" >&2; exit 1; } eval "$( RAW_T="$RAW_THREADS_JSON" SIG_P="🤖 *Reviewed by Claude Code*" PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ @@ -70,11 +72,11 @@ EOJS echo "Mode detected: $MODE" ``` -After this block: `MODE`, `IS_REREVIEW`, `PRIOR_ITERATION_ID`, and `SUMMARY_THREAD_ID` are set. +Sets `MODE`, `IS_REREVIEW`, `PRIOR_ITERATION_ID`, `SUMMARY_THREAD_ID`. ## Step 5 — ADO Fetcher -Launch the ADO Fetcher agent and **wait for its result** before launching anything else (the PRD requires the Fetcher to complete before the Doc Context Orchestrator and review aspect agents run). +Launch the ADO Fetcher agent and **wait for its result** before anything else (the PRD requires the Fetcher to complete before downstream agents run). ```txt Agent( @@ -112,9 +114,7 @@ Agent( ) ``` -**Review aspect agents** — apply the [aspect-filter selection](#aspect-filter-selection-used-in-step-6-and-pre-pr-step-d) above. For each selected agent, pass: PR title + description, full diff, and changed file contents. Every prompt **must** end with the [compact finding schema](#compact-finding-schema) block verbatim. - -Collect the JSON arrays returned by all agents. Deduplicate and sort by severity (`critical` first). Assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. +**Review aspect agents** — apply the [aspect-filter selection](#aspect-filter-selection-used-in-step-6-and-pre-pr-step-d) above. For each selected agent, pass: PR title + description, full diff, and changed file contents. Every prompt **must** end with the [compact finding schema](#compact-finding-schema) block verbatim. Collect returned JSON arrays, deduplicate, sort by severity (`critical` first); assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. ## Step 7 — Write-back (branch on mode) From fc57ae68af5ba269780741dfa677e8d82dbfcb95 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 13:56:32 +0200 Subject: [PATCH 13/16] docs(pr-review): align inline cross-refs and clarify input contracts after orchestrator split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/claude-code/pr-review/.agents/ado-fetcher.md | 1 + apps/claude-code/pr-review/.agents/ado-writer.md | 2 +- .../pr-review/.agents/re-review-coordinator.md | 14 +++++++++----- apps/claude-code/pr-review/CHANGELOG.md | 7 +++++++ apps/claude-code/pr-review/commands/review-pr.md | 2 +- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/apps/claude-code/pr-review/.agents/ado-fetcher.md b/apps/claude-code/pr-review/.agents/ado-fetcher.md index edb7215..ae6bd10 100644 --- a/apps/claude-code/pr-review/.agents/ado-fetcher.md +++ b/apps/claude-code/pr-review/.agents/ado-fetcher.md @@ -240,5 +240,6 @@ Where: - `WORK_ITEM_IDS` is the JSON array from Step 5, e.g. `[42, 7]` or `[]` - `CHANGED_FILES` is the newline-separated list from Step 3, e.g. `edit: /src/api.ts` - `RAW_DIFF` is the full diff text from Step 4 (may be empty if no new commits) +- `LATEST_COMMIT_SHA` is the latest source-branch commit SHA captured in Step 2; reserved for future diff-range debugging and not consumed by any current downstream agent — the diff-range logic that needed it is now self-contained in Step 4 above. **Never add any ADO write operations (POST, PATCH, DELETE) to this agent.** diff --git a/apps/claude-code/pr-review/.agents/ado-writer.md b/apps/claude-code/pr-review/.agents/ado-writer.md index 8d7158a..3c5d516 100644 --- a/apps/claude-code/pr-review/.agents/ado-writer.md +++ b/apps/claude-code/pr-review/.agents/ado-writer.md @@ -212,7 +212,7 @@ If `FINDINGS_POSTED=0` (no new findings were posted in Step 1): echo "Re-review: no new findings — skipping summary reply." ``` -Do not post anything. `SUMMARY_THREAD_ID` remains as provided. +Do not post anything in Step 2. `SUMMARY_THREAD_ID` remains as provided. Step 3 still posts the completion marker on every successful run, even when zero inline findings were posted. --- diff --git a/apps/claude-code/pr-review/.agents/re-review-coordinator.md b/apps/claude-code/pr-review/.agents/re-review-coordinator.md index eb5232d..b070ff2 100644 --- a/apps/claude-code/pr-review/.agents/re-review-coordinator.md +++ b/apps/claude-code/pr-review/.agents/re-review-coordinator.md @@ -27,6 +27,8 @@ You receive: - `SIGNATURE_PREFIX` — always `🤖 *Reviewed by Claude Code*` - `PLUGIN_ROOT` — absolute path to this plugin's directory (for Node.js helper scripts) +`PRIOR_ITERATION_ID` is recomputed internally from `RAW_THREADS_JSON` by `detect-prior-review` (Step 2); the orchestrator's own `PRIOR_ITERATION_ID` is not passed in. + --- ## Constants @@ -98,7 +100,7 @@ SUMMARY_THREAD_ID=$(printf '%s' "$DETECT_JSON" | node -e "process.stdout.write(S PRIOR_ITERATION_ID=$(printf '%s' "$DETECT_JSON" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')); process.stdout.write(d.priorIterationId != null ? String(d.priorIterationId) : 'null')") ``` -If `IS_REREVIEW=false`: no prior bot threads found. Fall back to first-review mode — skip to [Step 7 — Return result](#step-7--return-result) with all counts zero, `freshFindings` = `FINDINGS`, `earlyExit: false`. +If `IS_REREVIEW=false`: no prior bot threads found — return all findings as fresh and exit without classification or replies. Skip to [Step 8 — Return result](#step-8--return-result) with all counts zero, `freshFindings` = `FINDINGS`, `earlyExit: false`. (The coordinator does not switch modes; the orchestrator does not change agent dispatch based on this branch.) Log: @@ -106,7 +108,7 @@ Log: if [ "$IS_REREVIEW" = "true" ]; then echo "Detected $BOT_THREAD_COUNT prior bot threads — re-review mode." else - echo "No prior bot threads detected — first-review mode. Returning all findings as fresh." + echo "No prior bot threads detected — returning all findings as fresh; no classification or replies." fi ``` @@ -119,7 +121,7 @@ If `IS_REREVIEW=true`, `SUMMARY_THREAD_ID` is non-empty, and `PRIOR_ITERATION_ID The Node check distinguishes three outcomes via distinct exit codes — this prevents conflating "marker missing" (legitimate partial prior run; downgrade is correct) with "check crashed" (silent downgrade would re-post every prior thread): - exit `0` → marker found → `MARKER_FOUND=true` (proceed normally) -- exit `1` → marker not found → `MARKER_FOUND=false` (legitimate partial run; downgrade to first-review mode) +- exit `1` → marker not found → `MARKER_FOUND=false` (legitimate partial run; treat prior threads as absent — all findings will be returned as fresh) - exit `2` or any other non-zero → the check itself crashed → **abort the coordinator with exit code 3** (do not silently downgrade) The orchestrator's Step 7 only treats an `earlyExit: true` block as a non-fatal skip; a non-zero coordinator exit propagates as a fatal failure that surfaces to the user and stops the run — which is the correct behaviour when the partial-run check is itself broken. @@ -152,7 +154,7 @@ EOJS esac if [ "$MARKER_FOUND" = "false" ]; then - echo "No completion marker for Iteration $PRIOR_ITERATION_ID — partial prior run. Falling back to first-review mode." + echo "No completion marker for Iteration $PRIOR_ITERATION_ID — partial prior run; treating prior threads as absent and returning all findings as fresh." IS_REREVIEW=false SUMMARY_THREAD_ID="" PRIOR_ITERATION_ID="null" @@ -160,7 +162,7 @@ EOJS fi ``` -If `IS_REREVIEW` is now `false` after the partial-run check: skip to [Step 7 — Return result](#step-7--return-result) with all counts zero, `freshFindings` = `FINDINGS`, `earlyExit: false`. +If `IS_REREVIEW` is now `false` after the partial-run check: no prior bot threads remain valid — return all findings as fresh and exit without classification or replies. Skip to [Step 8 — Return result](#step-8--return-result) with all counts zero, `freshFindings` = `FINDINGS`, `earlyExit: false`. --- @@ -264,6 +266,8 @@ Process each finding one at a time. For each finding: ### 6a — Find matching prior thread +Substitute the `{finding.x}` placeholders below with concrete values from the current `FINDINGS` array element — these are prompt-template tokens, not shell variables. + ```bash MATCH=$( THREADS_F="$PRIOR_THREADS_FILE" \ diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index d3769ae..077999f 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -22,6 +22,13 @@ - H3 — Orchestrator Step 4 no longer coerces `az repos pr thread list` failures to `[]`. The fetch is now captured separately; on non-zero exit the orchestrator emits a clear stderr error ("ERROR: failed to fetch PR threads via Azure CLI. Try `az devops login` to re-authenticate.") and exits `1`, preventing a fetch failure from being mistaken for "no prior threads" and triggering a duplicate-post storm on re-review. - H4 — Re-review Coordinator Step 3 partial-run check no longer conflates "marker missing" with "check crashed". The Node heredoc now wraps its body in try/catch and exits with distinct codes (`0` = found, `1` = not found, `2` = crash); the bash side 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 `git fetch origin "$SOURCE_BRANCH" && git checkout "$SOURCE_BRANCH"`, and aborts with a clear stderr error if both fail — previously the comment-form fallback never ran and the agent silently continued on the wrong branch. +- Re-review Coordinator inline cross-references in Steps 2 and 3 pointed to a non-existent `Step 7 — Return result` section (the actual return-result heading is Step 8, after `Step 7 — Clean up`). Anchors now resolve and use the same numbering as the headings. +- Re-review Coordinator Inputs section now states explicitly that `PRIOR_ITERATION_ID` is recomputed internally by `detect-prior-review` from `RAW_THREADS_JSON`; the orchestrator's own `PRIOR_ITERATION_ID` is not threaded in, preventing redundant input plumbing. +- Re-review Coordinator no-prior-threads and partial-run branches no longer claim to "fall back to first-review mode" — the coordinator does not switch modes, it returns a result block with zero counts and `freshFindings = FINDINGS`, and the orchestrator does not change agent dispatch based on this. Prose corrected in Step 2, Step 3, and the two associated `echo` log lines. +- Re-review Coordinator Step 6a now states up front that `{finding.filePath}` / `{finding.startLine}` / `{finding.endLine}` are prompt-template placeholders to be substituted by the agent for the current `FINDINGS` element, not bash variables. +- ADO Writer Step 2's `MODE=re-review, zero new findings` branch now notes that Step 3 still posts the completion marker on every successful run, resolving the apparent contradiction with the "Do not post anything" line. +- ADO Fetcher output documentation now flags `LATEST_COMMIT_SHA` as reserved for future diff-range debugging and unused by any current downstream agent (the diff-range logic that needed it is self-contained in Step 4) — prevents future contributors from threading it through new agents under the assumption it is consumed. +- Orchestrator Step 6 prose no longer claims the review-aspect-agent prompts receive `PR_TITLE` and `PR_DESCRIPTION`. The Fetcher captures them for downstream use, but the orchestrator does not parse them, so the prose now reads "full diff and changed file contents" only — removing the contradiction with Step 5's parse list. ## [1.0.0] — 2026-05-12 diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index 2d1f53a..393e632 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -114,7 +114,7 @@ Agent( ) ``` -**Review aspect agents** — apply the [aspect-filter selection](#aspect-filter-selection-used-in-step-6-and-pre-pr-step-d) above. For each selected agent, pass: PR title + description, full diff, and changed file contents. Every prompt **must** end with the [compact finding schema](#compact-finding-schema) block verbatim. Collect returned JSON arrays, deduplicate, sort by severity (`critical` first); assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. +**Review aspect agents** — apply the [aspect-filter selection](#aspect-filter-selection-used-in-step-6-and-pre-pr-step-d) above. For each selected agent, pass the full diff and changed file contents (the Fetcher captures PR title and description for downstream use only; they are not parsed by the orchestrator). Every prompt **must** end with the [compact finding schema](#compact-finding-schema) block verbatim. Collect returned JSON arrays, deduplicate, sort by severity (`critical` first); assemble `FINDINGS` as `{ severity, filePath, startLine, endLine, title, body }[]`. ## Step 7 — Write-back (branch on mode) From 82819e4da8dad508f8562f0a83d232e930bc7f74 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 13:58:11 +0200 Subject: [PATCH 14/16] chore(inbox): capture deferred follow-ups from PR #29 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/inbox/ci-test-job-missing-pr-review.md | 40 +++++++++++++++++++ .../pr-review-ado-error-hardening-pass.md | 36 +++++++++++++++++ ...review-prompt-content-tests-brittleness.md | 39 ++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 docs/inbox/ci-test-job-missing-pr-review.md create mode 100644 docs/inbox/pr-review-ado-error-hardening-pass.md create mode 100644 docs/inbox/pr-review-prompt-content-tests-brittleness.md diff --git a/docs/inbox/ci-test-job-missing-pr-review.md b/docs/inbox/ci-test-job-missing-pr-review.md new file mode 100644 index 0000000..d6690b8 --- /dev/null +++ b/docs/inbox/ci-test-job-missing-pr-review.md @@ -0,0 +1,40 @@ +--- +title: CI test job filter omits pr-review +created: 2026-05-13 +--- + +**Status:** needs-triage +**Category:** ci + +> _This was generated by AI during triage of PR #29._ + +`.github/workflows/ci.yml` defines a `test` matrix job that runs `pnpm --filter test` only when the corresponding package changed. The matrix currently includes `auto-format`, `unic-confluence` (under the output name `confluence-publish` — separate naming bug), and `release-tools`. **`pr-review` is not in the matrix.** + +Consequences: + +- Every PR that changes `apps/claude-code/pr-review/**` skips CI tests entirely. The `Detect changed packages` job runs the filter (which already declares `pr-review:` correctly), but the downstream `test` job's `if:` condition (lines 48–51 in `ci.yml`) does not include `pr-review` and the matrix `package:` list does not include it either. +- PR #29 (orchestrator split) added the helpers `scripts/ado-fetcher.mjs`, `scripts/ado-writer.mjs`, `scripts/pre-pr.mjs`, `scripts/mode-detection.mjs`, `scripts/re-review/parse-diff-hunks.mjs` and accompanying tests under `tests/`. Local `pnpm --filter pr-review test` runs 142 tests across all of them. None of these run in CI. + +There's also a latent output-key mismatch on line 28: the filter has key `unic-confluence` but the outputs declare `confluence-publish`. Even though the `if:` clause uses `unic-confluence` (the filter key), the outputs declaration wouldn't expose it — that probably already silently masks confluence tests too. + +## Fix sketch + +In `.github/workflows/ci.yml`: + +1. Add `pr-review: ${{ steps.filter.outputs.pr-review }}` to the `changes` job outputs. +2. Add `needs.changes.outputs.pr-review == 'true'` to the `test` job's `if:` condition. +3. Add the package to the matrix: + ```yaml + - name: pr-review + changed: ${{ needs.changes.outputs.pr-review }} + ``` +4. Fix the `unic-confluence` / `confluence-publish` output-key mismatch while in there. + +## What grilling needs to resolve + +- Does the `pr-review` test suite have any cross-platform-flaky tests that would slow the Windows / macOS matrix? Quick local run suggests no (pure helpers, no fs/network). +- Should this be batched with a broader CI audit (the `confluence-publish` typo, the auto-format / release-tools filter coverage)? + +## Source + +PR #29 review (triage step 2 — CI checks). The `Test ...` job appears as `skipping` on every pr-review PR; root cause is matrix filter omission rather than a check failure. diff --git a/docs/inbox/pr-review-ado-error-hardening-pass.md b/docs/inbox/pr-review-ado-error-hardening-pass.md new file mode 100644 index 0000000..f271aaf --- /dev/null +++ b/docs/inbox/pr-review-ado-error-hardening-pass.md @@ -0,0 +1,36 @@ +--- +title: pr-review ADO error-hardening pass +created: 2026-05-13 +--- + +**Status:** needs-triage +**Category:** enhancement + +> _This was generated by AI during triage of PR #29._ + +PR #29 (pr-review orchestrator split) addressed the highest-stakes silent-failure paths in the ADO Writer and the orchestrator's mode-detection step (H1–H5). A second wave of silent-failure findings from the same review was deferred because the changes span multiple agents and helpers and feel like a feature in themselves — "ADO write reliability" — rather than a fit for the orchestrator-split PR. Capture them here so they aren't lost. + +## Items to harden + +- **ADO Fetcher Step 2 (iterations fetch).** `ITERATIONS_JSON=$(az devops invoke ...)` does not capture the exit code; an `az` failure produces an empty variable, which the subsequent Node parse silently coerces to `LATEST_ITERATION_ID=''` and `LATEST_COMMIT_SHA=''`. Every comment is then signed `Iteration ` (empty) and re-review detection breaks forever afterward because no `PRIOR_ITERATION_ID` can match `""`. Capture the exit code, branch on it, and abort the whole agent with a clear error rather than emitting a signature-less review. +- **ADO Fetcher Step 5 (work-item fetch).** `WI_RESPONSE=$(az devops invoke ... 2>/dev/null) || WI_RESPONSE=""` conflates legitimate "no work items linked" with auth expiry, project rename, 5xx, extension uninstall, network partition. The Doc Context Orchestrator then runs without business context, silently. Capture stderr, log it, and emit a distinct sentinel (e.g. `WORK_ITEM_IDS_ERROR`) so the orchestrator can surface to the user. +- **ADO Fetcher Step 4 (diff-range fallback).** When the prior commit can't be fetched, the agent silently falls back to the full diff. The Re-review Coordinator then classifies prior threads against the wider hunk set and may flip threads from `obsolete` to `pending` or vice versa. Either propagate a `DIFF_RANGE: full|incremental` field through `ADO_FETCHER_RESULT` so the Coordinator can react, or skip classification when the fallback fires. +- **`parseWorkItemIds` discriminated union.** The helper currently maps null/undefined responses to `[]` — the JSDoc even codifies this as intentional. Replace with `{ ok: true, ids } | { ok: false, reason }` so callers must distinguish "no work items" from "fetch failed". +- **`parseAdoWriterResult` discriminated union.** Returns `null` when the result block is missing. The orchestrator has no documented branch for "writer returned null block" — it presumably treats it as 0/empty and proceeds to report success. Either throw on missing block or add a `{ status: 'missing' | 'parsed' }` variant. +- **`parseIterations` empty-input default.** Returns `{ latestIterationId: 1, latestCommitSha: '' }` when `value` is empty — but `iterationId=1` is explicitly the iteration the project never uses (per plugin CLAUDE.md). If a fetch failure produces an empty value array, the agent happily emits `Iteration 1` signatures, silently violating the rule. Either throw, or return a discriminated union. +- **Pre-PR default-branch detection.** `DEFAULT_BRANCH=$(git remote show origin 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}' || echo "main")` silently falls back to `main`. The repo's own Gitflow uses `develop` — so a transient `git remote show` failure would compute the pre-PR diff against the wrong base and review hundreds of unrelated commits with no warning. Surface the fallback with a stderr warning. +- **Inline POST `*.err` files cleanup.** `Step 4 — Clean up` in the Writer runs `rm -f` unconditionally — destroying the only persistent record of stderr from failed inline POSTs. Keep them on failure (`if FINDINGS_POSTED < EXPECTED`) or stream their contents to stderr before cleanup. +- **Re-review Coordinator PATCH-to-fixed catch-all.** The Node catch block only special-cases `409` → continue; everything else (401, 403, 404, 5xx, network) becomes a 200-character "PATCH warning" in stdout that no automated layer inspects. Threads stay active when the user expects them fixed, and the next re-review re-replies to them. Distinguish recoverable (409) from unrecoverable and propagate the latter. +- **Re-review Coordinator per-finding match parse.** `MATCH=$(... 2>/dev/null || echo "")` — on Node parse error, CLASSIFICATION and THREAD_ID both end up empty strings, dispatch falls through to "no match → add to freshFindings", and the reviewer sees the same finding posted twice. Capture the exit code separately and abort/log instead of silently downgrading. +- **`pre-pr.mjs` parseChangedFilesFromDiff edge case.** `[]` is returned for both "empty input" and "diff with no `diff --git` headers" — the latter is suspicious (a real diff always has those headers). Pre-PR mode then prints `✅ Pre-PR review complete — no issues found.` even when the actual reason was a broken pipeline. Log a debug line when non-empty input produces zero files, or have `buildPrePrContext` throw on suspicious shapes. + +## What grilling needs to resolve + +- Is "harden every silent-failure path" a single feature, or should it be split (Writer / Fetcher / Coordinator / Pre-PR)? +- Where is the line between "abort and surface to user" and "log and continue"? Different items currently lean different ways. +- Adding `try/catch` + structured exit codes around every Node heredoc balloons the agent prompts — is that acceptable, or do we extract more helpers (the `mode-detection.mjs` precedent) so the bash side gets simpler? +- Are the discriminated-union refactors of `parseWorkItemIds` / `parseIterations` / `parseAdoWriterResult` a breaking change to the helper API? If they have any external consumers (they shouldn't, but verify), call it out. + +## Source + +PR #29 silent-failure-hunter review. The fixed subset (H1–H5) is documented in `apps/claude-code/pr-review/CHANGELOG.md` under [Unreleased] → Fixed. diff --git a/docs/inbox/pr-review-prompt-content-tests-brittleness.md b/docs/inbox/pr-review-prompt-content-tests-brittleness.md new file mode 100644 index 0000000..4a6e151 --- /dev/null +++ b/docs/inbox/pr-review-prompt-content-tests-brittleness.md @@ -0,0 +1,39 @@ +--- +title: pr-review prompt-content tests are brittle +created: 2026-05-13 +--- + +**Status:** needs-triage +**Category:** tech-debt + +> _This was generated by AI during triage of PR #29._ + +The pr-review test suite includes ~60% of its lines in "prompt-content assertions" against `.agents/*.md` and `commands/review-pr.md`. They string-grep markdown prose for substrings like `"no code quotes"`, `"leading slash"`, `"≤ 80"` etc. These work today but are brittle: a behaviourally-equivalent rewrite of the prompt prose (e.g. "no inline code quotes" instead of "no code quotes") breaks the test, and several assertions OR three to seven substring fallbacks because the author already knew the wording would drift. + +The orchestrator-split PR (#29) realigned some of these tests to read from a single shared `### Compact finding schema` block instead of slicing Step 6 / Step D — already an improvement. But the underlying anti-pattern is still present in: + +- `tests/ado-fetcher.test.mjs` — frontmatter + Step 1/2/3 prose substring assertions, plus a "no ADO write HTTP methods" test that uses a regex over a stripped slice of the markdown (with multiple opt-out clauses). +- `tests/ado-writer.test.mjs` — mirror "GET-forbidden" test on the writer prompt with the same fragility, plus a 5-way OR substring assertion on the zero-findings branch wording. +- `tests/pre-pr.test.mjs` — Pre-PR "absence" assertions (`does not invoke ADO Fetcher` etc.) which are valuable, and the compact-output guidance assertions which are now schema-block focused (good). + +The PRD's own testing decision says "No new unit tests required for the three new agents — their behaviour is best verified by integration against a real ADO PR (smoke test)." The current tests stretch that — and the section-slice approach silently passes when `indexOf` returns `-1` because `slice(-1, ...)` yields an empty string. + +## Proposed direction + +Replace prompt-prose substring assertions with one of: + +- **Structured contract blocks inside the prompts.** E.g. an `` / `` fence in `.agents/ado-fetcher.md` listing the output fields, parsed by a `parseFetcherContract` helper. Tests then assert against the parsed structure, not the prose. +- **Header-level structural assertions.** "ADO Fetcher must declare an Inputs section listing exactly these fields" — parse the markdown headings. +- **A single static snapshot test of the contract block** that updates with an intentional `--update-snapshot` flag. + +For the substring-OR-chain assertions ("zero" || "no new findings" || "FINDINGS_POSTED=0" || "nothing to report" || "skip"), the assertion is so permissive it carries no signal — drop or replace with a structural marker. + +## What grilling needs to resolve + +- Is the right replacement a structured contract block, snapshot tests, or just deleting the noisiest assertions? +- Should the prompts evolve a small "spec frontmatter" convention (parseable by tests) so we can stop string-matching prose? +- How does this interact with the existing 4 re-review module tests (which are good and should stay)? + +## Source + +PR #29 pr-test-analyzer review. Affected tests are unmodified by PR #29 except for the Step 6 / Step D realignment that landed alongside the orchestrator trim. From 4a98883971f3e42d84950170226152f494cc2ed3 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 14:00:37 +0200 Subject: [PATCH 15/16] fix(pr-review): replace @ts-ignore with @ts-expect-error in mode-detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/claude-code/pr-review/scripts/mode-detection.mjs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/claude-code/pr-review/scripts/mode-detection.mjs b/apps/claude-code/pr-review/scripts/mode-detection.mjs index d2a166a..4665b7d 100644 --- a/apps/claude-code/pr-review/scripts/mode-detection.mjs +++ b/apps/claude-code/pr-review/scripts/mode-detection.mjs @@ -25,8 +25,7 @@ export function detectMode({ threads, signaturePrefix }) { const r = detectPriorReview({ // detect-prior-review accepts the raw ADO thread shape; the orchestrator // passes whatever `az repos pr thread list` returned, untouched. - // eslint-disable-next-line - // @ts-ignore -- runtime-validated by detectPriorReview's own guards + // @ts-expect-error -- runtime-validated by detectPriorReview's own guards threads: Array.isArray(threads) ? threads : [], signaturePrefix, }) From 042da644c768064a708c04aa688dd0fa23d25908 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Wed, 13 May 2026 14:15:01 +0200 Subject: [PATCH 16/16] fix(pr-review): address Copilot review comments K1, K2, K4, K5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/claude-code/pr-review/CHANGELOG.md | 33 +++++++++++-------- .../pr-review/commands/review-pr.md | 2 +- apps/claude-code/pr-review/scripts/pre-pr.mjs | 6 ++-- .../pr-review/tests/pre-pr.test.mjs | 11 +++++++ 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/apps/claude-code/pr-review/CHANGELOG.md b/apps/claude-code/pr-review/CHANGELOG.md index 077999f..c9fc044 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -6,6 +6,23 @@ - (none) ### Added +- (none) + +### Fixed +- (none) + +## [1.0.0] — 2026-05-12 + +### Breaking +- (none) + +### Added +- Orchestrator split: `review-pr.md` refactored from a monolithic command to a thin orchestrator (≤ 200 lines per PRD acceptance criterion) that delegates ADO API calls and coordination logic to three focused agents +- ADO Fetcher agent: handles all Azure DevOps REST API fetches (diff, threads, iterations) in a single dedicated context window +- Re-review Coordinator agent: classifies prior bot threads, computes incremental diffs, and decides per-thread reply actions +- ADO Writer agent: posts all inline thread comments and the summary comment back to ADO, keeping write operations isolated from analysis +- Pre-PR mode: invoke `/pr-review:review-pr` without an ADO URL to review a local branch diff before the PR is created; findings are printed to the terminal instead of posted to ADO +- Compact sub-agent output: all review-aspect agent prompts now include an explicit JSON output contract, keeping reasoning inside each agent's context window and returning only structured `{ severity, filePath, startLine, endLine, title, body }[]` arrays to the orchestrator - New `scripts/re-review/parse-diff-hunks.mjs` helper module (with 7 unit tests) that parses raw `git diff` text into per-hunk `{ filePath, startLine, endLine }` entries — pure function, no I/O, slash-prefixed file paths. - New `scripts/mode-detection.mjs` helper that consolidates `Step 4` re-review detection and exports both `detectMode()` and `formatModeEnv()` used by the orchestrator. @@ -29,19 +46,9 @@ - ADO Writer Step 2's `MODE=re-review, zero new findings` branch now notes that Step 3 still posts the completion marker on every successful run, resolving the apparent contradiction with the "Do not post anything" line. - ADO Fetcher output documentation now flags `LATEST_COMMIT_SHA` as reserved for future diff-range debugging and unused by any current downstream agent (the diff-range logic that needed it is self-contained in Step 4) — prevents future contributors from threading it through new agents under the assumption it is consumed. - Orchestrator Step 6 prose no longer claims the review-aspect-agent prompts receive `PR_TITLE` and `PR_DESCRIPTION`. The Fetcher captures them for downstream use, but the orchestrator does not parse them, so the prose now reads "full diff and changed file contents" only — removing the contradiction with Step 5's parse list. - -## [1.0.0] — 2026-05-12 - -### Breaking -- (none) - -### Added -- Orchestrator split: `review-pr.md` refactored from a monolithic command to a thin orchestrator (≤ 200 lines per PRD acceptance criterion) that delegates ADO API calls and coordination logic to three focused agents -- ADO Fetcher agent: handles all Azure DevOps REST API fetches (diff, threads, iterations) in a single dedicated context window -- Re-review Coordinator agent: classifies prior bot threads, computes incremental diffs, and decides per-thread reply actions -- ADO Writer agent: posts all inline thread comments and the summary comment back to ADO, keeping write operations isolated from analysis -- Pre-PR mode: invoke `/pr-review:review-pr` without an ADO URL to review a local branch diff before the PR is created; findings are printed to the terminal instead of posted to ADO -- Compact sub-agent output: all review-aspect agent prompts now include an explicit JSON output contract, keeping reasoning inside each agent's context window and returning only structured `{ severity, filePath, startLine, endLine, title, body }[]` arrays to the orchestrator +- Pre-PR mode default-branch detection no longer silently leaves `DEFAULT_BRANCH` empty when `git remote show origin` produces no `HEAD branch:` line. The pipeline now filters empty awk output through `grep .` so the `|| echo "main"` fallback fires for real, instead of being short-circuited by a still-zero-exit awk. +- `shouldSkipFile` now uses the lower-cased path for the `/generated/` directory check too, so capitalised `.NET`-style paths like `/Source/Generated/ApiClient.cs` are skipped consistently with the other rules. +- `parseChangedFilesFromDiff` now splits the diff text on `/\r?\n/` (matching the sibling `parseDiffHunks` helper), so CRLF-formatted diffs from Windows Git no longer produce paths with a trailing `\r`. ### Fixed - (none) diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index 393e632..3c64d8a 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -159,7 +159,7 @@ No PR URL provided — reviewing the local branch diff; no ADO calls are made. ### Step A — Compute diff ```bash -DEFAULT_BRANCH=$(git remote show origin 2>/dev/null | grep 'HEAD branch' | awk '{print $NF}' || echo "main") +DEFAULT_BRANCH=$(git remote show origin 2>/dev/null | awk '/HEAD branch/{print $NF}' | grep . || echo "main") RAW_DIFF=$(git diff "origin/${DEFAULT_BRANCH}...HEAD") || { echo "git diff failed"; exit 1; } ``` diff --git a/apps/claude-code/pr-review/scripts/pre-pr.mjs b/apps/claude-code/pr-review/scripts/pre-pr.mjs index cf0303e..0a5b1a7 100644 --- a/apps/claude-code/pr-review/scripts/pre-pr.mjs +++ b/apps/claude-code/pr-review/scripts/pre-pr.mjs @@ -31,8 +31,8 @@ export function shouldSkipFile(filePath) { const basename = filePath.split('/').pop() ?? '' if (basename.toLowerCase().startsWith('generated-types.')) return true - // files under a generated/ directory segment - if (filePath.includes('/generated/')) return true + // files under a generated/ directory segment (case-insensitive: e.g. /Generated/ on .NET) + if (lower.includes('/generated/')) return true return false } @@ -51,7 +51,7 @@ export function parseChangedFilesFromDiff(diffText) { const seen = new Set() const paths = [] - for (const line of diffText.split('\n')) { + for (const line of diffText.split(/\r?\n/)) { const m = line.match(/^diff --git a\/.*? b\/(.+)$/) if (m) { const filePath = `/${m[1]}` diff --git a/apps/claude-code/pr-review/tests/pre-pr.test.mjs b/apps/claude-code/pr-review/tests/pre-pr.test.mjs index 4eee8c2..115e989 100644 --- a/apps/claude-code/pr-review/tests/pre-pr.test.mjs +++ b/apps/claude-code/pr-review/tests/pre-pr.test.mjs @@ -80,6 +80,13 @@ index 000..111 100644 const result = parseChangedFilesFromDiff(diff) assert.deepEqual(result, ['/a/b/c/deep.ts']) }) + + it('CRLF-separated diff produces clean paths (no trailing \\r)', () => { + const diff = + 'diff --git a/src/foo.ts b/src/foo.ts\r\nindex 000..111 100644\r\n--- a/src/foo.ts\r\n+++ b/src/foo.ts\r\n' + const result = parseChangedFilesFromDiff(diff) + assert.deepEqual(result, ['/src/foo.ts']) + }) }) // --------------------------------------------------------------------------- @@ -123,6 +130,10 @@ describe('shouldSkipFile', () => { assert.equal(shouldSkipFile('/src/generated/api-client.ts'), true) }) + it('file under a capitalised Generated/ directory (.NET-style) → true (skip)', () => { + assert.equal(shouldSkipFile('/Source/Generated/ApiClient.cs'), true) + }) + it('normal source file with no skip pattern → false (keep)', () => { assert.equal(shouldSkipFile('/src/services/user.service.ts'), false) })