fix(pr-review): repair Step 4 ADO mode detection and add CLI guards#133
Merged
orioltf merged 8 commits intoMay 27, 2026
Merged
Conversation
Local mirror of issue #120 aligned with ADR 0016 as merged. The original GitHub issue body diverged from the ADR on three points (5xx → DEGRADED not ABORTED, ADR 0015 amended by 0018 not 0016, new kind: thread-fetch IS added); the local PRD reflects the ADR. Two AFK slices ready for an agent to pick up: - 01-fold-thread-fetch-into-fetcher.md (P0, no blockers): move thread fetch + mode detection from the orchestrator's broken Step 4 into the ADO Fetcher; export SIGNATURE_PREFIX; emit MODE / IS_REREVIEW / PRIOR_ITERATION_ID / SUMMARY_THREAD_ID / RAW_THREADS_JSON from the Fetcher result block. - 02-ado-cli-inventory-and-smoke-test.md (P1, blocked by 01): add the ADO CLI inventory + allowlist, scripts/ado/cli-completeness.mjs, inventory-completeness test (everywhere) + CLI smoke test (gated on az on PATH), Step 3 preflight assertion, single-cell CI install. First plugin-scoped PRD under apps/claude-code/<plugin>/docs/issues/ per the multi-context doctrine; root docs/issues/ stays monorepo-only going forward (see #132 for the retroactive relocation of prior plugin PRDs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The orchestrator's Step 4 was calling a non-existent
`az repos pr thread list` subcommand and failing fatally on every ADO
PR review. Thread fetching and mode detection now live inside the ADO
Fetcher, which calls `az devops invoke --resource pullRequestThreads`
and applies the ADR 0015 HTTP-tier classification (401/403 → abort with
`az devops login` hint; 404 → empty `{value:[]}`; 5xx/network →
`thread-fetch` warning Notice). The Fetcher's result block now emits
RAW_THREADS_JSON, MODE, IS_REREVIEW, PRIOR_ITERATION_ID and
SUMMARY_THREAD_ID; the orchestrator's Step 4 captures PR metadata via
`az repos pr show` and forwards it to the Fetcher. Per ADR 0016.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Guards the failure class behind Slice 01 (a phantom `az repos pr thread list` shipping for months) by adding offline checks for ADO CLI drift: - `tests/fixtures/ado-cli-inventory.mjs` — single source of truth for every `az` call the plugin makes; allowlist exempts preflights and error-message hints. - `scripts/ado/cli-completeness.mjs` — pure `findUninventoriedCommands`; handles multi-line bash, backslash continuations, split `--area`/ `--resource` flags, skips shell comments + markdown inline code + quoted strings. - `tests/ado-cli-smoke.test.mjs` — completeness against real source files (runs everywhere) + `<cmd> --help` per inventory entry (skips cleanly when `az` is absent). - `commands/review-pr.md` Step 3 — preflight asserts `az devops invoke` itself is callable; orchestrator stays at 192 lines. - `.github/workflows/ci.yml` — adds pr-review to the test matrix (was filter-detected but never actually tested) and installs `azure-cli` + `azure-devops` on ubuntu/Node 24 so the smoke test exercises `--help` for real on one cell. - `tests/pre-pr.test.mjs` — ADR-0016 invariant test now skips inline- code / quoted-string mentions and allows the `--help` probe (still bans REST calls). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the pr-review plugin’s Azure DevOps Step 4 failure by moving thread fetch + mode detection into the ADO Fetcher, and adds guardrails (CLI inventory + smoke tests + CI wiring) to prevent future “phantom az subcommand” regressions.
Changes:
- Replace orchestrator Step 4 thread listing with
az repos pr showmetadata capture; Fetcher now ownspullRequestThreads+ mode detection and returns mode/thread fields in its result block. - Add ADO CLI inventory + allowlist, plus completeness and
--helpsmoke tests (CI installsazon one Linux/Node 24 cell to run the real smoke). - Add
pr-reviewto the CI matrix and update supporting docs/ADRs/CHANGELOG/tests.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/claude-code/pr-review/commands/review-pr.md | Orchestrator Step 4 now fetches PR metadata via az repos pr show; forwards metadata to Fetcher; parses mode/thread fields from Fetcher output; adds az devops invoke --help preflight. |
| apps/claude-code/pr-review/agents/ado-fetcher.md | Fetcher now fetches threads via az devops invoke --resource pullRequestThreads, runs mode detection, and emits thread/mode fields + degraded notice on thread-fetch failure. |
| apps/claude-code/pr-review/agents/re-review-coordinator.md | Updates RAW_THREADS_JSON contract to match pullRequestThreads response shape and avoids referring to the removed subcommand. |
| apps/claude-code/pr-review/scripts/mode-detection.mjs | Exports canonical SIGNATURE_PREFIX and updates thread-shape comments to .value array semantics. |
| apps/claude-code/pr-review/scripts/ado/notices.mjs | Extends NoticeKind union with thread-fetch. |
| apps/claude-code/pr-review/scripts/ado/cli-completeness.mjs | New helper that extracts az invocation “shapes” and reports any not in inventory/allowlist. |
| apps/claude-code/pr-review/tests/fixtures/ado-cli-inventory.mjs | New single source of truth listing the plugin’s az command surface (including invoke area/resource). |
| apps/claude-code/pr-review/tests/fixtures/ado-cli-allowlist.mjs | New allowlist for intentional non-inventory az references (preflight + user-hint strings). |
| apps/claude-code/pr-review/tests/ado-cli-completeness.test.mjs | New unit tests for findUninventoriedCommands. |
| apps/claude-code/pr-review/tests/ado-cli-smoke.test.mjs | New integration-ish test: scans sources for uninventoried az invocations; runs az … --help per inventory entry (skips when az absent). |
| apps/claude-code/pr-review/tests/ado-fetcher.test.mjs | Adds assertions for new Fetcher responsibilities (threads endpoint, mode detection, new result fields, failure mapping). |
| apps/claude-code/pr-review/tests/pre-pr.test.mjs | Adds assertions that orchestrator no longer uses the phantom thread subcommand and delegates REST calls to Fetcher. |
| apps/claude-code/pr-review/tests/mode-detection.test.mjs | Asserts SIGNATURE_PREFIX is exported and matches the canonical string. |
| apps/claude-code/pr-review/tests/notices.test.mjs | Adds coverage that thread-fetch kind is accepted. |
| apps/claude-code/pr-review/package.json | Adds the new CLI completeness + smoke tests to the test script. |
| apps/claude-code/pr-review/CHANGELOG.md | Documents the Step 4 fix and the new CLI guardrails. |
| apps/claude-code/pr-review/AGENTS.md | Documents the CLI inventory as the authoritative source for az usage. |
| apps/claude-code/pr-review/docs/adr/0013-orchestrator-split-for-review-pr.md | Marks ADR 0013 as amended by ADR 0016. |
| apps/claude-code/pr-review/docs/issues/pr-review-ado-fetcher-step4-fix/PRD.md | New PRD capturing the bug + guardrail slices and acceptance criteria. |
| apps/claude-code/pr-review/docs/issues/pr-review-ado-fetcher-step4-fix/01-fold-thread-fetch-into-fetcher.md | New slice doc for folding thread fetch + mode detection into Fetcher. |
| apps/claude-code/pr-review/docs/issues/pr-review-ado-fetcher-step4-fix/02-ado-cli-inventory-and-smoke-test.md | New slice doc for CLI inventory + completeness/smoke tests + preflight + CI install. |
| .github/workflows/ci.yml | Adds pr-review to the test matrix and conditionally installs Azure CLI + extension on ubuntu/node24 for the smoke test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The agent frontmatter regex assumed LF line endings, which broke on Windows runners where git checks out files with CRLF. Surfaced once pr-review joined the CI test matrix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- cli-completeness now emits `az <flag>` for root-flag invocations like `az --version`, so the allowlist actually covers them. - ado-cli-smoke normalises path separators before SKIP_FRAGMENTS, so scratchpad/ and node_modules/ are skipped on Windows too. Adds a unit test for the new shape extraction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- ado-cli-smoke walk() only swallows ENOENT; other read errors propagate so a broken scan can't silently produce a green "no uninventoried" pass. - ado-fetcher Step 2 logs a WARN when HTTP status parsing fails so a 404 with an unparseable error body isn't silently routed to DEGRADED. - ado-fetcher test asserts the new fields appear inside the ADO_FETCHER_RESULT_START/_END block, not just anywhere in the markdown. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Same CRLF-vs-LF mismatch as the earlier plugin-structure fix. Windows checkouts have CRLF line endings; the new regex must use `\r?\n` rather than `\n` for the block delimiters. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements both slices of the
pr-review-ado-fetcher-step4-fixPRD (apps/claude-code/pr-review/docs/issues/pr-review-ado-fetcher-step4-fix/PRD.md):az repos pr thread listsubcommand and failing fatally on every ADO PR review. Thread fetching now lives in the ADO Fetcher (per ADR-0016) and usesaz devops invoke --resource pullRequestThreads. The orchestrator's Step 4 captures PR metadata viaaz repos pr show; the Fetcher's result block emitsRAW_THREADS_JSON,MODE,IS_REREVIEW,PRIOR_ITERATION_ID,SUMMARY_THREAD_ID.azcommand the plugin invokes is enumerated intests/fixtures/ado-cli-inventory.mjs; the new smoke test asserts (a) everyazinvocation inagents//commands//scripts/has a matching inventory entry, and (b)<cmd> --helpexits 0 against the real CLI. Step 3 preflight now verifiesaz devops invokeitself is callable. CI installsazure-cli+azure-devopson Linux/Node 24 so the smoke test runs for real on one cell; other cellst.skipcleanly.Adds
pr-reviewto the CI test matrix (was filter-detected but never actually tested).Test plan
pr-reviewmatrix packagepnpm --filter pr-review testpasses locally withazinstalled (smoke test executes) and withoutaz(smoke skips, completeness still runs) — both verifiedpnpm --filter pr-review verify:changelogpassespnpm checkclean (Biome + Prettier)/pr-review:review-pr <ADO-PR-URL>reaches the Fetcher without the Step 4 abortaz repos pr thread list) → CLI smoke test surfaces it on the Linux/Node 24 cell🤖 Generated with Claude Code