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..ae6bd10 --- /dev/null +++ b/apps/claude-code/pr-review/.agents/ado-fetcher.md @@ -0,0 +1,245 @@ +--- +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' +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)) +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" \ + || (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: + +```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' +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)) +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) +- `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 new file mode 100644 index 0000000..3c5d516 --- /dev/null +++ b/apps/claude-code/pr-review/.agents/ado-writer.md @@ -0,0 +1,342 @@ +--- +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. `${TMPDIR:-/tmp}/ado_writer_thread_1.json`, `_2.json`, etc.). + +```bash +cat > "${TMPDIR:-/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 "${TMPDIR:-/tmp}/ado_writer_thread_N.json" \ + --api-version "7.1" \ + --output json 2>"${TMPDIR:-/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 + +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 +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": [ + { + "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 "${TMPDIR:-/tmp}/ado_writer_thread_N_fallback.json" \ + --api-version "7.1" \ + --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 +``` + +**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 +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 +``` + +--- + +## 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 > "${TMPDIR:-/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 "${TMPDIR:-/tmp}/ado_writer_summary.json" \ + --api-version "7.1" \ + --output json 2>"${TMPDIR:-/tmp}/ado_writer_summary.err") +SUMMARY_EXIT=$? + +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}" +``` + +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 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. + +--- + +### 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 > "${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 +} +ENDJSON + +DELTA_RESPONSE=$(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 "${TMPDIR:-/tmp}/ado_writer_delta.json" \ + --api-version "7.1" \ + --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: + +``` +- **[{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 > "${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 + } +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 "${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 + 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 "${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" +``` + +--- + +## Output + +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=$( + SID="${SUMMARY_THREAD_ID}" \ + FP="${FINDINGS_POSTED}" \ + PLUGIN_R="${PLUGIN_ROOT}" \ + node --input-type=module << 'EOJS' +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 +) +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..b070ff2 --- /dev/null +++ b/apps/claude-code/pr-review/.agents/re-review-coordinator.md @@ -0,0 +1,456 @@ +--- +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) + +`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 + +```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")" +echo '[]' > "$DIFF_HUNKS_FILE" +``` + +Parse hunk boundaries from `RAW_DIFF` via the Node helper `parse-diff-hunks.mjs` (cross-platform; no python3 dependency): + +```bash +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. + +--- + +## 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")" + +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 — 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: + +```bash +if [ "$IS_REREVIEW" = "true" ]; then + echo "Detected $BOT_THREAD_COUNT prior bot threads — re-review mode." +else + echo "No prior bot threads detected — returning all findings as fresh; no classification or replies." +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}`: + +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; 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. + +```bash +if [ "$IS_REREVIEW" = "true" ] && [ -n "$SUMMARY_THREAD_ID" ] && [ "$PRIOR_ITERATION_ID" != "null" ]; then + THREADS_F="$PRIOR_THREADS_FILE" SID="$SUMMARY_THREAD_ID" PID="$PRIOR_ITERATION_ID" \ + node --input-type=module << 'EOJS' +import { readFileSync } from 'node:fs' +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 + 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; treating prior threads as absent and returning all findings as fresh." + IS_REREVIEW=false + SUMMARY_THREAD_ID="" + PRIOR_ITERATION_ID="null" + fi +fi +``` + +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`. + +--- + +## 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 + +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" \ + 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 > "${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 +} +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 "${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 ?? ''))" +``` + +**`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 > "${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 +} +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 "${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 ?? ''))" +``` + +**`addressed` → post resolution confirmation and PATCH thread status to fixed** + +```bash +# 1. Post resolution reply +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 +} +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 "${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 > "${TMPDIR:-/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 "${TMPDIR:-/tmp}/re_review_patch_${THREAD_ID}.json" \ + --api-version "7.1" \ + --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(\`\${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 { + 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 "${TMPDIR:-/tmp}"/re_review_reply_*.json "${TMPDIR:-/tmp}"/re_review_patch_*.json "${TMPDIR:-/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/.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 810bfd5..c9fc044 100644 --- a/apps/claude-code/pr-review/CHANGELOG.md +++ b/apps/claude-code/pr-review/CHANGELOG.md @@ -11,6 +11,48 @@ ### 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. + +### 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. +- 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. +- 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. +- 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) + ## [0.9.1] — 2026-05-08 ### Breaking diff --git a/apps/claude-code/pr-review/CLAUDE.md b/apps/claude-code/pr-review/CLAUDE.md index 008ff1a..794f9c6 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 (≤ 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 @@ -33,8 +39,10 @@ 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 - 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) diff --git a/apps/claude-code/pr-review/commands/review-pr.md b/apps/claude-code/pr-review/commands/review-pr.md index a297a48..3c64d8a 100644 --- a/apps/claude-code/pr-review/commands/review-pr.md +++ b/apps/claude-code/pr-review/commands/review-pr.md @@ -6,990 +6,195 @@ 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 -``` - -If `azure-devops` extension is missing: `az extension add --name azure-devops` - -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. - ---- - -## 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: +Thin orchestrator that detects one of three modes — Pre-PR, First-review, Re-review — and delegates to focused agents. -- `ORG_URL` = `https://dev.azure.com/{org}` -- `PROJECT` = `{project}` -- `PR_ID` = `{id}` +## Constants -**GitHub URLs** (`https://github.com/...`) are not supported — tell the user and stop. +- `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. -If no URL provided, run `az repos pr list --status active --output table` to help them pick one. +### Compact finding schema ---- - -## Step 2 — Check the default `az` org +Every review aspect agent prompt (Step 6, Step D) ends with this exact contract: -```bash -az devops configure --list ``` +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 -Note the configured `organization`. If it differs from `ORG_URL`, pass `--org {ORG_URL}` explicitly in every `az` command below. - ---- - -## Step 3 — Fetch PR metadata - -```bash -az repos pr show --id {PR_ID} --org {ORG_URL} --output json +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. ``` -Capture and remember: +### Aspect-filter selection (used in Step 6 and Pre-PR Step D) -- `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` +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. -Strip `refs/heads/` prefix to get plain branch names for git commands. +## Step 1 — Prerequisites -Capture additionally: +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. -- `repository.project.name` → `PROJECT` +## Step 2 — Parse arguments and detect mode ---- +Extract a PR URL from `$ARGUMENTS`. Expected format: `https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}`. GitHub URLs are not supported. -## Step 3.5 — Detect prior review +- **No URL** → `MODE=pre-pr` → jump to [Pre-PR mode](#pre-pr-mode). +- **URL present** → extract `ORG_URL`, `PROJECT`, `PR_ID` and continue. -Fetch all existing PR threads and check for prior Claude Code comments. This step runs **unconditionally** and performs **no write actions**. +## Step 3 — Azure CLI check (PR modes only) -### Variables exported by this step +Run `az --version` and `az extension list | grep azure-devops`. If missing: `az extension add --name azure-devops`. -| 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 — Re-review detection -### Fetch all threads (paginated) +Fetch the thread list **once**; never re-fetch 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" -``` - -### Parse bot threads +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; } -```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}" \ +eval "$( + RAW_T="$RAW_THREADS_JSON" SIG_P="🤖 *Reviewed by Claude Code*" 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)) -process.stdout.write(JSON.stringify({ - isRereview: r.isRereview, - summaryThreadId: r.summaryThread != null ? r.summaryThread.threadId : '', - priorIterationId: r.priorIterationId, - count: r.priorThreads.length, -})) +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 -) -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 +echo "Mode detected: $MODE" ``` -### Partial-prior-run check +Sets `MODE`, `IS_REREVIEW`, `PRIOR_ITERATION_ID`, `SUMMARY_THREAD_ID`. -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. +## Step 5 — ADO Fetcher -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. +Launch the ADO Fetcher agent and **wait for its result** before anything else (the PRD requires the Fetcher to complete before downstream agents run). -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 -``` - ---- - -## 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})" -``` - -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 -``` - ---- - -## 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}') -" -``` - ---- - -## Step 4a — Gather Doc Context (work items + Confluence pages) - -```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="" +```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}" +) ``` -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. +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. -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). +## Step 6 — Doc Context Orchestrator + review aspect agents (parallel) -Resolve the plugin path: +Launch both groups concurrently in a **single message**. -```bash -CONFLUENCE_CLIENT_PATH="${CLAUDE_PLUGIN_ROOT}/scripts/confluence-client.mjs" -``` - -Delegate to the Doc Context Orchestrator agent: +**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( 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). - -#### Path B2 — Re-review, no latest commit (`IS_REREVIEW=true`, `LATEST_COMMIT_ID` empty) - -```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). - -#### 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. - ---- - -## 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: - -- `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} +**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 }[]`. -## Changed files -{file contents} -``` +## Step 7 — Write-back (branch on mode) -**Example agent invocations (parallel):** +**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( - 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: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}" ) ``` ---- - -## 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 +**Both modes** — invoke ADO Writer. For first-review, `MODE=first-review` and `SUMMARY_THREAD_ID=""`. For re-review, both come from Step 4. -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 +```txt +Agent( + subagent_type: "pr-review:ado-writer", + 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: {MODE} + PLUGIN_ROOT: {CLAUDE_PLUGIN_ROOT} + FINDINGS: {FINDINGS_JSON}" +) ``` -Branch on `IS_REREVIEW`. - ---- +## Pre-PR mode -### Path A — IS_REREVIEW=false (first-review flow) +No PR URL provided — reviewing the local branch diff; no ADO calls are made. -For each finding with a known file and line, post a PR thread: +### Step A — Compute diff ```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)) +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; } ``` -**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: +### Step B — Parse changed files ```bash -MATCH=$( - THREADS_F="$PRIOR_THREADS_FILE" \ - FINDING_F="{FINDING_FILE}" \ - FINDING_S="{FINDING_START}" \ - FINDING_E="{FINDING_END}" \ - PLUGIN_R="${CLAUDE_PLUGIN_ROOT}" \ +FILTERED_FILES=$( + RAW_DIFF_STR="$RAW_DIFF" 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) : '') +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 ) - -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 +Read the contents of each file in `FILTERED_FILES`, skipping deleted ones. -Reply to confirm the fix, then PATCH the thread status to `fixed` (`status: 2`). Log 409 and continue: +### Step C — Resolve aspect filter -```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) +Apply the [aspect-filter selection](#aspect-filter-selection-used-in-step-6-and-pre-pr-step-d) defined above. -- **[file:line]** Issue description +### Step D — Run review aspect agents -### 🟡 Minor / Suggestions +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). -- Suggestion +Collect, dedupe, and sort returned JSON arrays into `FINDINGS` (`critical` first). -### ✅ What's good +### Step E — Present findings -- Positive observation +Print each finding in the Claude interface, grouped by severity (`critical`, `important`, `minor`): ---- - -🤖 _Reviewed by Claude Code_ — Iteration {N} -``` - ---- - -### IS_REREVIEW=true, all counters zero — skip - -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. - ---- - -### IS_REREVIEW=true, at least one counter > 0 — delta reply or fallback - -#### 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'))" +[{severity}] {filePath} L{startLine}–{endLine} +{title} +{body} ``` -`{BULLET_LIST_OF_NEW_FINDING_TITLES}` — one bullet per new thread posted in Step 10, format: - -``` -- **[{filePath}:{startLine}]** {one-line finding title} -``` - -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 - -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: - -```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" -``` - ---- - -## 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 - -- 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. +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 05b6354..77685d8 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", @@ -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 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/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/scripts/mode-detection.mjs b/apps/claude-code/pr-review/scripts/mode-detection.mjs new file mode 100644 index 0000000..4665b7d --- /dev/null +++ b/apps/claude-code/pr-review/scripts/mode-detection.mjs @@ -0,0 +1,59 @@ +// @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. + // @ts-expect-error -- 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/scripts/pre-pr.mjs b/apps/claude-code/pr-review/scripts/pre-pr.mjs new file mode 100644 index 0000000..0a5b1a7 --- /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 (case-insensitive: e.g. /Generated/ on .NET) + if (lower.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(/\r?\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/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/ado-fetcher.test.mjs b/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs new file mode 100644 index 0000000..0d5e558 --- /dev/null +++ b/apps/claude-code/pr-review/tests/ado-fetcher.test.mjs @@ -0,0 +1,143 @@ +// @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/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/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, []) + }) +}) 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..115e989 --- /dev/null +++ b/apps/claude-code/pr-review/tests/pre-pr.test.mjs @@ -0,0 +1,370 @@ +// @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']) + }) + + 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']) + }) +}) + +// --------------------------------------------------------------------------- +// 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('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) + }) +}) + +// --------------------------------------------------------------------------- +// 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 — 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')) + + /** 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 references the compact finding schema', () => { + assert.ok( + step6Section.toLowerCase().includes('compact finding schema'), + 'Step 6 must reference the shared compact finding schema' + ) + }) + + it('Step D references the compact finding schema', () => { + assert.ok( + stepDSection.toLowerCase().includes('compact finding schema'), + 'Step D must reference the shared compact finding schema' + ) + }) + + it('schema instructs agents to return a JSON array of findings', () => { + assert.ok( + schemaSection.includes('JSON') && schemaSection.includes('array'), + 'Compact finding schema must instruct review agents to return a JSON array of findings' + ) + }) + + 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('schema instructs agents to omit code quotes from return value', () => { + assert.ok( + schemaSection.toLowerCase().includes('code quote'), + 'Compact finding schema must instruct agents to omit code quotes from the return value' + ) + }) + + it('schema instructs agents to omit prose reasoning from return value', () => { + assert.ok( + 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('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( + 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('schema requires filePath to use leading slash and forward slashes', () => { + assert.ok( + 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('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('schema describes body as the exact text to post as the ADO or local-interface comment', () => { + assert.ok( + 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' + ) + }) +}) + +// --------------------------------------------------------------------------- +// 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/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/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. 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 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 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 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 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