diff --git a/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json b/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json index 281c8749..208ee0af 100644 --- a/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json +++ b/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json @@ -21,7 +21,7 @@ "name": "unic-pr-review", "source": "./", "tags": ["productivity", "code-review", "azure-devops"], - "version": "2.1.9" + "version": "2.1.10" } ] } diff --git a/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json b/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json index dbe8bc73..d677f75b 100644 --- a/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json +++ b/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json @@ -15,5 +15,5 @@ "keywords": ["pr-review", "azure-devops", "jira", "confluence", "code-review", "unic"], "license": "LGPL-3.0-or-later", "name": "unic-pr-review", - "version": "2.1.9" + "version": "2.1.10" } diff --git a/apps/claude-code/unic-pr-review/CHANGELOG.md b/apps/claude-code/unic-pr-review/CHANGELOG.md index 38656549..fa414a8f 100644 --- a/apps/claude-code/unic-pr-review/CHANGELOG.md +++ b/apps/claude-code/unic-pr-review/CHANGELOG.md @@ -16,6 +16,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - (none) +## [2.1.10] — 2026-06-16 + +### Breaking +- (none) + +### Added +- (none) + +### Fixed +- ADO Fetcher now fetches linked Work Items via `pullrequestworkitems` endpoint (Step 1.5) and populates `prMetadata.workItemRefs`; previously the fetcher never called this endpoint so `discoverWorkItems()` always returned `[]` and the Intent Check was silently skipped on every PR with linked Work Items (#247). Step 1.5 discriminates three fetch outcomes so a zero-exit-but-malformed response can no longer masquerade as "no Work Items linked" + ## [2.1.9] — 2026-06-09 ### Breaking diff --git a/apps/claude-code/unic-pr-review/agents/ado-fetcher.md b/apps/claude-code/unic-pr-review/agents/ado-fetcher.md index 127d6ec6..3a5b052c 100644 --- a/apps/claude-code/unic-pr-review/agents/ado-fetcher.md +++ b/apps/claude-code/unic-pr-review/agents/ado-fetcher.md @@ -38,6 +38,30 @@ If the command exits non-zero, emit this error and stop: { "error": "fetch-failed", "step": 1, "resource": "pullrequests", "message": "" } ``` +### Step 1.5 — Fetch linked Work Items + +The `pullrequests` endpoint does not include Work Item links. Fetch them from the dedicated endpoint: + +```sh +az devops invoke --area git --resource pullrequestworkitems \ + --route-parameters organization="" project="" repositoryId="" pullRequestId="" \ + --http-method GET --api-version 7.0 --output json +``` + +Store stdout as `WI_LIST` and capture the exit code. Distinguish three outcomes — never let a failure masquerade as the legitimate "no Work Items linked" case (the silent false negative this step exists to prevent): + +- **Exit zero and stdout parses to an object with a `value` array** → set `PR_METADATA.workItemRefs = WI_LIST.value`. An empty `value` legitimately means no Work Items are linked — no warning. +- **Exit non-zero** → set `PR_METADATA.workItemRefs = []` and add the warning below (`` = `Error: `). +- **Exit zero but stdout is empty, unparseable, or has no `value` array** → treat as a fetch failure, not zero Work Items: set `PR_METADATA.workItemRefs = []` and add the warning below (`` = `unexpected response shape — do not assume zero Work Items`). + +In both failure branches add this warning (it belongs to the same `warnings` array emitted in Step 6): + +``` +"pullrequestworkitems fetch failed — Work Item discovery skipped, Intent Check will be omitted. " +``` + +Do not stop; continue to Step 2. + ### Step 2 — Fetch Revisions (iterations) ```sh @@ -232,7 +256,10 @@ Emit exactly one JSON object — no prose, no markdown, no footer (replace each ```json { - "prMetadata": "", + "prMetadata": { + "pullRequestId": 42, + "workItemRefs": [{ "id": "101", "url": "https://dev.azure.com/org/project/_apis/wit/workitems/101" }] + }, "revisions": "", "threads": "", "changedFiles": ["path/to/file.ts"], @@ -247,4 +274,6 @@ Emit exactly one JSON object — no prose, no markdown, no footer (replace each } ``` +`prMetadata` is the raw ADO `pullrequests` response enriched with `workItemRefs` from Step 1.5. `workItemRefs` is always an array (empty `[]` when no Work Items are linked or the fetch failed). + `mode` is one of `"first-review"`, `"re-review"`, `"first-review-fallback"`. `priorRevisionId` and `priorIteration` are `null` except in `re-review` mode (where they carry `PRIOR_SIG.priorRevisionId` / `PRIOR_SIG.priorIteration`). `deltaRawDiff` is the delta diff string (empty in first-review modes). `priorFindings` is an array of `{ threadId, filePath, startLine, severity, title }` objects (empty except in `re-review` mode), where `threadId` is the number id of the ADO Thread carrying that prior finding's bot comment — it is what the Re-review Coordinator keys all thread mapping on. `diffUnavailable` is `false` when a real diff was computed (re-review always, first-review/first-review-fallback when inside a matching clone and the git diff succeeds) and `true` when a diff could not be obtained (no matching clone, missing commonRefCommit or sourceRefCommit, git diff failure, or an empty diff despite a non-empty changedFiles). `warnings` is an array of strings for any non-fatal issues. Never emit `hardStop` — the orchestrator handles all write decisions. diff --git a/apps/claude-code/unic-pr-review/docs/adr/0001-multi-source-intent-with-shared-atlassian-credentials.md b/apps/claude-code/unic-pr-review/docs/adr/0001-multi-source-intent-with-shared-atlassian-credentials.md index d2de9d9f..c14bbc71 100644 --- a/apps/claude-code/unic-pr-review/docs/adr/0001-multi-source-intent-with-shared-atlassian-credentials.md +++ b/apps/claude-code/unic-pr-review/docs/adr/0001-multi-source-intent-with-shared-atlassian-credentials.md @@ -24,9 +24,11 @@ Reuse `~/.unic-confluence.json` as the shared Atlassian Credential File for both ## Amendment (2026-06) Work-item discovery is a **Provider contract**. Each Source Platform Provider exposes -`discoverWorkItems(prMetadata) → [{ id, type, url, raw }]`. For ADO, this reads the -PR's native `workItemRefs` field (not regex-scraping the description); for future -GitHub/GitLab Providers it will use their respective native linkages. The Intent Checker -stays Source-Platform-agnostic: it consumes the normalised list regardless of origin. -This separates "where did the Work Items come from?" (Provider contract) from "what -intent do they express?" (Intent Checker responsibility). +`discoverWorkItems(prMetadata) → [{ id, type, url, raw }]`. For ADO, the Fetcher +populates `prMetadata.workItemRefs` from the dedicated `pullrequestworkitems` endpoint +(Step 1.5 of the ADO Fetcher — the `pullrequests` response does not include Work Item +links); `discoverWorkItems` then reads that field (never regex-scraping the description). +For future GitHub/GitLab Providers it will use their respective native linkage endpoints. +The Intent Checker stays Source-Platform-agnostic: it consumes the normalised list +regardless of origin. This separates "where did the Work Items come from?" (Provider +contract) from "what intent do they express?" (Intent Checker responsibility). diff --git a/apps/claude-code/unic-pr-review/docs/adr/0016-human-threads-as-read-only-context.md b/apps/claude-code/unic-pr-review/docs/adr/0016-human-threads-as-read-only-context.md index 21c9844d..33e44ef5 100644 --- a/apps/claude-code/unic-pr-review/docs/adr/0016-human-threads-as-read-only-context.md +++ b/apps/claude-code/unic-pr-review/docs/adr/0016-human-threads-as-read-only-context.md @@ -6,15 +6,15 @@ The ADO Fetcher fetches every PR Thread but classifies them on a single axis: does `comments[0].content` carry the Iteration Marker (a Bot Thread, ADR-0006) or not. In `first-review` mode the fetched Threads are then dropped — no step reads them — and in `re-review` only Bot Threads feed the Re-review Coordinator's `priorFindings`. Human review discussion is invisible to a Review in every Mode. -This surfaced in a head-to-head against the legacy `pr-review` plugin on ADO PR #5570. That plugin read the five Human Threads on the PR, cross-referenced a Finding to a still-open one, and noted a Thread marked *fixed* whose issue was in fact still present ("re-verify before merge"). `unic-pr-review` could surface none of this — a structural gap, not a fetch failure. +This surfaced in a head-to-head against the legacy `pr-review` plugin on ADO PR #5570. That plugin read the five Human Threads on the PR, cross-referenced a Finding to a still-open one, and noted a Thread marked _fixed_ whose issue was in fact still present ("re-verify before merge"). `unic-pr-review` could surface none of this — a structural gap, not a fetch failure. -The tempting fix — let a resolved Human Thread suppress or down-rank a matching Finding — is the mirror image of the work-item-discovery false negative (issue #247): where that bug made the Plugin *under-gather intent*, suppression would make it *under-report code issues*. A wrongly-resolved Thread would silently hide a real defect. +The tempting fix — let a resolved Human Thread suppress or down-rank a matching Finding — is the mirror image of the work-item-discovery false negative (issue #247): where that bug made the Plugin _under-gather intent_, suppression would make it _under-report code issues_. A wrongly-resolved Thread would silently hide a real defect. ## Decision Human Threads are **read-only context**, surfaced after the aspect fan-out and never used to suppress a Finding. -1. **Classification.** The Fetcher gains a human/system split alongside the existing bot detection: *Bot Thread* = Iteration Marker (ADR-0006); *System Thread* = `comments[0].commentType === "system"` (ref updates, votes, policy); *Human Thread* = neither. Only Human Threads are surfaced; System Threads are never shown to the Reviewer. +1. **Classification.** The Fetcher gains a human/system split alongside the existing bot detection: _Bot Thread_ = Iteration Marker (ADR-0006); _System Thread_ = `comments[0].commentType === "system"` (ref updates, votes, policy); _Human Thread_ = neither. Only Human Threads are surfaced; System Threads are never shown to the Reviewer. 2. **Context-only, never suppress.** A Finding is never dropped or down-ranked because a Human Thread exists. Confidence < 60 remains the sole filter (ADR-0002). diff --git a/apps/claude-code/unic-pr-review/package.json b/apps/claude-code/unic-pr-review/package.json index bcde4233..5cc2bc3e 100644 --- a/apps/claude-code/unic-pr-review/package.json +++ b/apps/claude-code/unic-pr-review/package.json @@ -22,5 +22,5 @@ "verify:changelog": "unic-verify-changelog" }, "type": "module", - "version": "2.1.9" + "version": "2.1.10" } diff --git a/apps/claude-code/unic-pr-review/providers/azure_devops/README.md b/apps/claude-code/unic-pr-review/providers/azure_devops/README.md index a828ec64..34f0b022 100644 --- a/apps/claude-code/unic-pr-review/providers/azure_devops/README.md +++ b/apps/claude-code/unic-pr-review/providers/azure_devops/README.md @@ -25,7 +25,9 @@ Throws `Not an ADO PR URL: ` when the URL does not match `prUrlPattern`. ## `discoverWorkItems(prMetadata)` -Reads the PR's **native** `workItemRefs` field and normalises each entry to +Reads the `workItemRefs` that the ADO Fetcher populates on `prMetadata` from the +dedicated `pullrequestworkitems` endpoint (Step 1.5 — the `pullrequests` response +does not carry Work Item links), and normalises each entry to `{ id, type: "ado-work-item", url, raw }`. It never regex-scrapes the PR description — work-item discovery is a Provider contract (ADR-0001 amendment). Returns `[]` when `workItemRefs` is empty or absent. @@ -39,8 +41,9 @@ Returns `[]` when `workItemRefs` is empty or absent. ## Adding fixtures -PR-metadata fixtures live in `fixtures/`. They mirror the shape returned by -`az devops invoke --area git --resource pullrequests`. Add a new fixture file and +PR-metadata fixtures live in `fixtures/`. They mirror the enriched `prMetadata` +shape — the `az devops invoke --area git --resource pullrequests` response with +`workItemRefs` grafted on by the Fetcher (Step 1.5). Add a new fixture file and reference it from `tests/provider.test.mjs` via the `fixture(name)` helper. The `fixtures/ado-cli-inventory.json` file catalogues every `az devops invoke` call the ADO Fetcher agent emits; the root `tests/ado-cli-smoke.test.mjs` asserts the diff --git a/apps/claude-code/unic-pr-review/providers/azure_devops/fixtures/ado-cli-inventory.json b/apps/claude-code/unic-pr-review/providers/azure_devops/fixtures/ado-cli-inventory.json index 422270f0..f03c10e7 100644 --- a/apps/claude-code/unic-pr-review/providers/azure_devops/fixtures/ado-cli-inventory.json +++ b/apps/claude-code/unic-pr-review/providers/azure_devops/fixtures/ado-cli-inventory.json @@ -1,6 +1,12 @@ { "invokeCommands": [ { "area": "git", "resource": "pullrequests", "httpMethod": "GET", "description": "PR metadata" }, + { + "area": "git", + "resource": "pullrequestworkitems", + "httpMethod": "GET", + "description": "Work Items linked to the PR (Step 1.5 — separate endpoint, not in pullrequests response)" + }, { "area": "git", "resource": "pullrequestiterations", diff --git a/apps/claude-code/unic-pr-review/providers/azure_devops/provider.mjs b/apps/claude-code/unic-pr-review/providers/azure_devops/provider.mjs index d8c6298e..50739433 100644 --- a/apps/claude-code/unic-pr-review/providers/azure_devops/provider.mjs +++ b/apps/claude-code/unic-pr-review/providers/azure_devops/provider.mjs @@ -41,10 +41,11 @@ export const agents = { } /** - * Read the PR's native Work Item field (`workItemRefs`). Never regex-scrapes the - * description — this is the ADR-0001 amendment contract. + * Read the Work Item refs that the ADO Fetcher populated on `prMetadata` from the + * PR's linked work-item endpoint (`pullrequestworkitems`, Step 1.5). Never + * regex-scrapes the description — this is the ADR-0001 amendment contract. * - * @param {{ workItemRefs?: Array<{ id: string, url: string }> }} prMetadata + * @param {{ workItemRefs?: Array<{ id: string | number, url: string }> }} prMetadata * @returns {Array<{ id: string, type: string, url: string, raw: object }>} * @throws {Error} when `prMetadata` is not a PR-metadata object (guards against a * malformed stdin payload silently yielding zero Work Items) diff --git a/apps/claude-code/unic-pr-review/providers/azure_devops/tests/provider.test.mjs b/apps/claude-code/unic-pr-review/providers/azure_devops/tests/provider.test.mjs index 4351fbe8..41db6cb0 100644 --- a/apps/claude-code/unic-pr-review/providers/azure_devops/tests/provider.test.mjs +++ b/apps/claude-code/unic-pr-review/providers/azure_devops/tests/provider.test.mjs @@ -83,4 +83,49 @@ describe('discoverWorkItems', () => { assert.equal(item.type, 'ado-work-item') } }) + it('normalises integer wire-format ids to strings', () => { + // The live pullrequestworkitems endpoint returns integer ids; fixtures use strings. + // discoverWorkItems must coerce via String(ref.id) so downstream id handling is stable + // regardless of wire shape — this exercises the integer branch the JSDoc `id: string | number` + // widening opened, which the fixture-shape tests below cannot reach. + const items = discoverWorkItems({ + workItemRefs: [{ id: 42622, url: 'https://dev.azure.com/FZAG/_apis/wit/workitems/42622' }], + }) + assert.equal(items.length, 1) + assert.equal(items[0].id, '42622') + assert.equal(typeof items[0].id, 'string') + }) +}) + +// Fetcher contract: the ADO Fetcher (Step 1.5) must populate workItemRefs on prMetadata +// from the pullrequestworkitems endpoint. These tests assert that the fixtures reflect +// real fetcher output shape — workItemRefs is always present (never absent) because the +// fetcher sets it explicitly (to [] when no WIs are linked or the fetch fails). +describe('fetcher contract — prMetadata.workItemRefs is always fetcher-populated', () => { + it('pr-with-work-items fixture has workItemRefs with at least one entry', () => { + const meta = fixture('pr-with-work-items.json') + assert.ok(Array.isArray(meta.workItemRefs), 'workItemRefs must be an array (fetcher-populated)') + assert.ok(meta.workItemRefs.length > 0, 'fixture must have at least one work item ref') + }) + it('pr-with-multiple-work-items fixture has workItemRefs with multiple entries', () => { + const meta = fixture('pr-with-multiple-work-items.json') + assert.ok(Array.isArray(meta.workItemRefs), 'workItemRefs must be an array (fetcher-populated)') + assert.ok(meta.workItemRefs.length > 1, 'fixture must have more than one work item ref') + }) + it('pr-without-work-items fixture has workItemRefs as empty array (not absent)', () => { + const meta = fixture('pr-without-work-items.json') + assert.ok(Array.isArray(meta.workItemRefs), 'workItemRefs must be present even when empty') + assert.equal(meta.workItemRefs.length, 0) + }) + it('each workItemRef has string id and url', () => { + // Note: fixtures use synthetic string ids ("101") for simplicity. + // The real ADO pullrequestworkitems endpoint returns integer ids (101). + // discoverWorkItems normalises both via String(ref.id), so runtime is safe; + // this assertion matches the fixture shape, not the wire format. + const meta = fixture('pr-with-work-items.json') + for (const ref of meta.workItemRefs) { + assert.equal(typeof ref.id, 'string', 'workItemRef.id must be a string') + assert.equal(typeof ref.url, 'string', 'workItemRef.url must be a string') + } + }) })