feat(pr-review): orchestrator split + platform failure handling + suppress addressed reply (v1.1.0–v1.2.9)#31
Merged
Conversation
Lands the foundational tracer-bullet slice of PRD A (ADO Fetcher
reliability). Wires the four-tier Notice doctrine end-to-end with one
concrete emission site — the Doc-Context EMPTY-BY-DESIGN info Notice
fired when WORK_ITEM_IDS=[] — and adds the mandatory end-of-run Trailer
in the Claude interface. Subsequent slices (A2/A3/A4 and PRD B) extend
the same surface with DEGRADED + ABORTED emission sites on the Fetcher,
Writer, Coordinator, and Pre-PR paths.
What this slice ships:
- scripts/ado/notices.mjs — pure helpers (createNotice, mergeNotices,
formatNoticesAsSummaryBlock, formatNoticesAsPrePrPreamble,
formatTrailer). 14 unit-test cases pinning the dedup-by-kind merge,
the per-severity emoji rendering (ℹ️ for info, ⚠ for warning, bare
## Notices heading), and the four Trailer shapes (first-review,
re-review, pre-pr, aborted) including pluralisation.
- ADR 0014 — Notice Tier doctrine + failure-classification helper
layer; refines ADR 0013's testing posture (orchestration in agent
prompts, classification in pure-JS helpers).
- ADO Fetcher (.agents/ado-fetcher.md) — new Step 6 builds the NOTICES
array and emits the Doc-Context info Notice when WORK_ITEM_IDS=[].
ADO_FETCHER_RESULT block grows a NOTICES JSON-array field.
- Orchestrator (commands/review-pr.md) — parses NOTICES from the
Fetcher result block, sets NOTICES_JSON via mergeNotices (single
source today; subsequent slices add Coordinator/Writer sources),
threads it into the ADO Writer prompt, and prints one mandatory
formatTrailer line via a new Step 8. Pre-PR Step E's completion
message becomes a formatTrailer call (mode: 'pre-pr'). The 200-line
cap is held exactly by folding the documentation-only Constants
section into the lead paragraph (SIGNATURE_PREFIX value was already
inlined at both call sites; no behavioural change).
- ADO Writer (.agents/ado-writer.md) — accepts NOTICES_JSON input and
renders a ## Notices block above severity-grouped findings in the
Review Summary content. Empty NOTICES_JSON ⇒ no heading.
- Version: pr-review 1.0.0 → 1.1.0 (Added: user-visible Trailer +
Notices block). CHANGELOG dated 2026-05-13; verify:changelog clean.
Key decisions (locked during the 2026-05-13 /grill-with-docs session):
- Four tiers, no fifth ASK tier — AFK runs cannot block on user input.
Failures that would tempt ASK reclassified as ABORTED.
- Doc-Context info-Notice is the one EMPTY-BY-DESIGN carve-out — every
other empty state is silent.
- Notice shape: { severity, kind, message }; kind is an enum, not a
free-form string, so cross-agent dedup is structural rather than
textual.
- Trailer is mandatory for every run (success, abort, pre-pr); designed
for AFK skim — invoker sees outcome status without opening the PR.
- Helper layer refines (not replaces) ADR 0013: orchestration still in
agent prompts; failure classification moves to scripts/ado/.
Files: 12 changed (5 new, 7 modified). Issue
docs/issues/pr-review-ado-fetcher-reliability/01-end-to-end-notice-pipeline.md
flipped to Status: resolved with a Deviations section documenting the
sandbox-driven manual bump (no pnpm — Node 24 unreachable here, Node 20
runs node --test directly), the manual prettier/tsc feedback loops, and
the Biome layer left to CI (only darwin-arm64 cli binary is installed;
sandbox is linux-arm64).
Unblocks: A2 (work-items DEGRADED), A4 (DIFF_RANGE sentinel), B2
(parseAdoWriterResult discriminated-union), B4 (match-finding throw),
B6 (Pre-PR Notice surface). A3 and B-others remain blocked on A2/A4/B1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…for Node Sandboxed AFK runs were failing two ways: pnpm's `useNodeVersion: 24.15.0` triggered an outbound `nodejs.org` fetch the firewall blocked, and Biome's platform binary was only installed for the host arch, so `pnpm check` couldn't run on Linux. Fix both by deferring Node version selection to `.nvmrc` (already consumed by `actions/setup-node` and every local version manager) and by declaring `supportedArchitectures` so all OS/arch pairs of platform-specific packages land in `node_modules` regardless of where `pnpm install` runs. Relaxing the engines range from `>=24` to `>=22` makes the existing CI matrix (Node 22 + 24) honest — `useNodeVersion` was silently forcing Node 24 even on the "Node 22" job. Docs (AGENTS.md, README, CONTRIBUTING, plugin CLAUDE.md/CONTRIBUTING, and the new-plugin scaffolding template) updated to point at `.nvmrc` and the relaxed engine range. Historical artifacts under docs/adr/ and docs/plans/ are left intact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…er + ADR-0015 (v1.2.0)
Wires the DEGRADED tier end-to-end for the work-item fetch path. Two new
pure JS helpers replace the inline parseWorkItemIds helper and introduce
the canonical HTTP-tier mapping used by PRD B's write-side helpers.
New helpers:
- scripts/ado/classify-http-error.mjs — classifyHttpError({ status, body,
exitCode }) → { tier, kind, message }. Canonical mapping: 200/201/404/409
→ ok; 401/403 → aborted (kind: auth); 5xx → degraded (kind: transient);
other 4xx → degraded (kind: malformed-request); non-zero exit + no status
→ degraded (kind: network). 16 unit-test cases.
- scripts/ado/fetch-work-items.mjs — fetchWorkItems({ responseText, exitCode
}) → { ok: true, ids } | { ok: false, reason, message }. Distinguishes
EMPTY-BY-DESIGN (ok: true, ids: []) from fetch failure (ok: false). 9
unit-test cases.
Prompt change (.agents/ado-fetcher.md Step 5): delegates work-item fetch to
fetchWorkItems via await import. On { ok: false }, emits DEGRADED Notice
(kind: work-items) into NOTICES array. On { ok: true, ids: [] }, emits
EMPTY-BY-DESIGN info Notice (kind: doc-context) as before.
Cleanup: parseWorkItemIds removed from scripts/ado-fetcher.mjs and its
tests; ado-fetcher.test.mjs updated to assert fetchWorkItems usage instead.
ADR 0015 (docs/adr/0015-canonical-http-tier-mapping.md): records the
HTTP-tier mapping table, the 401/403 abort rule, and the no-retries-in-v1
stance. Consumed by PRD B's parse-write-response (B1).
Deviations: version bumped manually (unic-bump requires a clean working
tree; sandbox does not support committing mid-step). Version updated in
plugin.json + marketplace.json; CHANGELOG moved [Unreleased] to [1.2.0].
Unblocks: A3 (fetch-iterations ABORTED), B1 (parse-write-response + Writer
HTTP-tier mapping). A3 depends on classify-http-error; B1 depends on both.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removes the invalid `iterationId=1` fallback and wires the ABORTED tier
for the iterations fetch path. An empty or failed iterations response now
aborts the run rather than silently signing comments with the wrong ID.
New helper:
- scripts/ado/fetch-iterations.mjs — fetchIterations({ responseText,
exitCode }) → { ok: true, latestIterationId, latestCommitSha } |
{ ok: false, reason: 'empty-iterations' | 'auth' | 'transient' |
'malformed', message }. Uses classifyHttpError for HTTP failures.
8 unit-test cases covering every branch.
Prompt changes:
- .agents/ado-fetcher.md Step 2: delegates to fetchIterations via
await import. On { ok: false }, exits non-zero with a clear stderr
message ('az devops login' hint for auth; explicit empty-iterations
message for that reason).
- commands/review-pr.md Step 5: detects absent result block (Fetcher
exited non-zero), infers abortKind from output, calls formatTrailer
and stops. Line count held at exactly 200.
Cleanup:
- scripts/ado-fetcher.mjs deleted (parseIterations was its only export;
all callers updated).
- tests/ado-fetcher.test.mjs: parseIterations import + describe block
removed; new assertions check for fetchIterations usage and verify
no iterationId=1 default remains in the agent prompt.
- package.json: tests/fetch-iterations.test.mjs added to test script.
Deviations: version bumped manually (unic-bump requires a clean working
tree; sandbox does not support committing mid-step).
Unblocks: A4 (diff-range sentinel) had no dependency on A3; A3 itself
had no dependents in the current issue queue.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ndment (v1.2.2)
Emits the DIFF_RANGE sentinel in the ADO Fetcher result block and fires
a DEGRADED Notice when the incremental diff fallback triggers.
ADO Fetcher (.agents/ado-fetcher.md):
- Step 4: sets DIFF_RANGE=incremental on successful incremental diff,
DIFF_RANGE=full + DIFF_RANGE_FALLBACK=true when prior commit is
unreachable, DIFF_RANGE=full on first-review, DIFF_RANGE=incremental
on no-new-commits branch.
- Step 6: passes DIFF_RANGE_FB into the Notice-building script; emits
warning Notice (kind: diff-range) when DIFF_RANGE_FALLBACK=true.
Removes the forward-looking A4 comment (now implemented).
- Output block: adds DIFF_RANGE: {DIFF_RANGE} field with Where: docs.
Orchestrator (commands/review-pr.md): adds DIFF_RANGE to the parsed
fields list in Step 5 with a note that B3 will consume it. 200 lines.
ADR 0004: adds "Degraded baseline" subsection documenting the γ-downgrade
rule (remap addressed/obsolete → pending when DIFF_RANGE=full). B3 impls.
Tests: adds 3 new ado-fetcher content assertions (DIFF_RANGE assignments,
DIFF_RANGE_FALLBACK flag, diff-range Notice). 185 tests, all passing.
Deviations: version bumped manually (unic-bump requires a clean working
tree; sandbox does not support committing mid-step).
Unblocks: B3 (Coordinator γ-downgrade consumes DIFF_RANGE from A4).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…(v1.2.3)
Routes every ADO write call site in the ADO Writer through one canonical
helper. Fixes the H1 auth gap (401/403 on inline-POST now aborts the Writer
instead of log-and-continue), wires the DEGRADED Notice surface end-to-end
for write failures, and adds the NOTICES field to the Writer result block.
New helper:
- scripts/ado/parse-write-response.mjs — parseWriteResponse({ httpExit,
responseText, errStream }) → { ok: true, id } | { ok: false, tier, kind,
message }. Composes classifyHttpError with response-id extraction. 404/409
map to { ok: true, id: null } (canonical ok, no resource created); 200
without numeric id → { ok: false, tier: 'degraded', kind: 'malformed-
response' }. 13 unit cases covering all branches.
Prompt changes (.agents/ado-writer.md):
- New "Helper: parse-write-response" section shows the canonical invoke
pattern once; each of the four call sites (inline POST + fallback, summary
POST, delta reply, completion marker) references it.
- On tier: aborted → stream *.err to stderr + exit 1 at every call site.
- On tier: degraded → push typed Warning Notice to NOTICES='[]' and continue.
- ADO_WRITER_RESULT_START/END grows NOTICES: [...] field.
- Cleanup (Step 4) is unconditional (no retention based on counts).
Orchestrator (commands/review-pr.md):
- Step 8 renamed to "Merge Writer notices + Trailer"; parses NOTICES from
ADO_WRITER_RESULT_START/END and merges via mergeNotices before Trailer.
200 lines.
Helper changes (scripts/ado-writer.mjs):
- parseAdoWriterResult return type extended to { summaryThreadId,
findingsPosted, notices: Notice[] }. Legacy blocks without NOTICES return
notices: []. Tests updated (7 parseAdoWriterResult cases + 3 new
agent-content assertions for parse-write-response usage).
Deviations: version bumped manually (unic-bump requires clean working tree).
Unblocks: B5 (Coordinator PATCH-to-fixed mapping depends on parse-write-
response).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…1.2.4)
Distinguishes Writer crash from zero-success: the old `null` return could
not tell "block was never emitted" from "block parsed, zero findings."
Helper change (scripts/ado-writer.mjs):
- parseAdoWriterResult now returns
{ ok: true, summaryThreadId, findingsPosted, notices }
| { ok: false, reason: 'missing-block' | 'malformed' }
- missing-block: ADO_WRITER_RESULT_START/END absent from output
- malformed: block present but FINDINGS_POSTED field absent/non-numeric
- summaryThreadId remains nullable (empty SUMMARY_THREAD_ID is valid)
Tests (tests/ado-writer.test.mjs):
- All valid-block cases assert result.ok === true before field access
- Renamed "returns null for both numeric fields" → "returns { ok: false,
reason: 'missing-block' } when no result block is present"
- New case: "returns { ok: false, reason: 'malformed' } when block is
present but FINDINGS_POSTED is absent"
- 205 tests, all passing
ADO Writer prompt (.agents/ado-writer.md):
- Round-trip validation updated from
`if (parsed.summaryThreadId === null || parsed.findingsPosted === null)`
to `if (!parsed.ok)`, with reason included in the error message.
Orchestrator (commands/review-pr.md):
- Step 8 renamed "Parse Writer result + Trailer"; branches on result.ok.
On { ok: false }, emits ERROR stderr message (with reason) and Trailer
aborted line, then stops. On { ok: true }, extracts result.notices and
merges with fetcher notices as before.
- File remains at exactly 200 lines.
Deviations: version bumped manually (unic-bump requires clean working
tree; sandbox does not support committing mid-step).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… wiring (v1.2.5) Extends classify-thread with the γ-downgrade rule (ADR-0004) and wires DIFF_RANGE from the ADO Fetcher result block into every thread classification call in the Re-review Coordinator. Helper change (scripts/re-review/classify-thread.mjs): - classifyThread now accepts diffRange: 'full' | 'incremental' (default 'incremental') - When diffRange === 'full', outputs 'addressed' and 'obsolete' are remapped to 'pending' via a single post-processing branch; 'disputed' is unaffected (its derivation is reviewer-reply-based, not diff-position-based) Tests (tests/classify-thread.test.mjs): - Three new γ-downgrade cases: full+intersects→pending, full+absent→pending, full+disputed→still disputed; 208 tests, all passing Coordinator (.agents/re-review-coordinator.md): - Inputs: added DIFF_RANGE as a parsed field with description - Step 5: parses DIFF_RANGE from ADO_FETCHER_RESULT (default incremental), passes DIFF_R env var into the classifyThread invocation Orchestrator (commands/review-pr.md): - Step 5 note updated: "not yet consumed" removed; explains Coordinator parses DIFF_RANGE from ADO_FETCHER_RESULT to apply γ-downgrade - File remains at exactly 200 lines Deviations: version bumped manually (unic-bump requires clean working tree). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ress-addressed-reply/01)
Removes the cosmetic "Resolved — thanks!" reply comment from the addressed
branch of the Re-review Coordinator. The thread status PATCH to fixed (status 2),
ADDRESSED_COUNT, and FINDINGS_POSTED counts are all unchanged.
Motivation: the reply generated an ADO notification for every thread participant.
Developers often self-resolve threads before the bot runs, causing the bot to
comment on already-closed threads (notification spam).
Changes:
- .agents/re-review-coordinator.md: removed Reply POST az devops invoke block,
updated section heading ("post resolution confirmation and" removed), updated
Step 8 description for addressed count
- docs/adr/0006-reply-not-duplicate-auto-resolve.md: updated Decision bullet for
addressed threads; added Revised note (2026-05-14) with reason
- docs/issues/done/01-remove-addressed-reply.md: moved from ready-for-agent to resolved
Note: version bump and CHANGELOG covered by suppress-addressed-reply/02.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ed-reply/02) Bumped manually (unic-bump requires a clean working tree; sandbox does not support committing mid-step). Changes: - plugin.json + marketplace.json: 1.2.5 → 1.2.6 - CHANGELOG.md: [Unreleased] promoted to [1.2.6] — 2026-05-14 with the addressed-thread reply removal entry; fresh empty [Unreleased] added - docs/issues/done/02-version-bump.md: moved from ready-for-agent to resolved Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…1.2.7)
Changes match-finding's null-vs-throw contract so parse errors are
distinguishable from legitimate no-match, and wires the Coordinator
to surface a DEGRADED Notice instead of silently posting duplicates.
Helper change (scripts/re-review/match-finding.mjs):
- Throws TypeError when priorThreads is not an array
- Throws TypeError when finding is null or missing typed fields
- Returns null only for legitimate no-match (contract unchanged)
Tests (tests/match-finding.test.mjs):
- Three new throw-path cases: non-array priorThreads, null finding,
wrong field types; 211 tests, all passing
Coordinator (.agents/re-review-coordinator.md):
- Initialises NOTICES='[]' alongside FRESH_FINDINGS_JSON in Step 6
- Step 6a captures Node exit code (MATCH_EXIT); on non-zero, pushes
DEGRADED Notice (kind: thread-match) and falls through to no-match
- Result block gains NOTICES: {NOTICES} field; Step 8 description updated
Orchestrator (commands/review-pr.md):
- Step 7: extracts coordinatorNotices from RE_REVIEW_COORDINATOR_RESULT
- Step 8: merges coordinatorNotices into combined mergeNotices call
- File remains at exactly 200 lines
Deviations: version bumped manually (unic-bump requires clean working tree).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…(v1.2.8) Routes the PATCH-to-fixed call site in the Re-review Coordinator through the canonical parse-write-response helper, replacing the old 409-only catch-all with uniform HTTP-tier handling. Coordinator (.agents/re-review-coordinator.md): - addressed branch: captures PATCH_RESP + PATCH_EXIT separately; routes through parseWriteResponse via Node heredoc (same PWR_JSON pattern as ADO Writer); on ok: true → continue; on tier: aborted → stderr + exit 1; on tier: degraded → push patch-to-fixed Notice + continue - 404/409 map to ok: true (canonical mapping) — silent continue as before - Old 409-only catch-all removed Orchestrator (commands/review-pr.md): - Step 7: added coordinator-abort handling — missing result block infers abortKind from output and calls formatTrailer before stopping - File remains at exactly 200 lines Deviations: version bumped manually (unic-bump requires clean working tree). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… (v1.2.9)
Gives Pre-PR mode the same Notice surface as ADO modes. Detects malformed
diff inputs. Replaces the hardcoded 'main' fallback with a Gitflow-aware
fallback chain that emits visible Notices.
New helper (scripts/pre-pr/detect-default-branch.mjs):
- detectDefaultBranch({ branchExists, remoteHeadBranch })
→ { branch, source, notice? }
- Chain: remote-show → develop-fallback → main-fallback → master-fallback → none
- Warning Notice (kind: default-branch) for every fallback level
- branch: null means no detectable branch → caller aborts
- 7 unit cases (tests/detect-default-branch.test.mjs)
Helper change (scripts/pre-pr.mjs):
- buildPrePrContext returns notices: Notice[] alongside existing fields
- Suspicious-shape detection: non-empty diff with diff --git headers but
zero parsed paths → DEGRADED Notice (kind: diff-parse)
- 4 new test cases in tests/pre-pr.test.mjs
Orchestrator (commands/review-pr.md):
- Pre-PR Step A: calls detectDefaultBranch; on branch: null aborts with Trailer;
any fallback notice pushed to PRE_PR_NOTICES
- Pre-PR Step B: merges buildPrePrContext().notices via mergeNotices
- Pre-PR Step E: formatNoticesAsPrePrPreamble before findings;
formatTrailer receives PRE_PR_NOTICES (notice count reflected in Trailer)
- File is 187 lines (≤ 200)
package.json: added tests/detect-default-branch.test.mjs to test script.
Deviations: version bumped manually (unic-bump requires clean working tree).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
orioltf
force-pushed
the
feature/pr-review/orchestrator-split-03
branch
from
64b9137 to
a341da1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Large refactor of the pr-review plugin that (1) splits the ~1000-line review-pr.md orchestrator into a thin orchestrator plus three focused sub-agents (ado-fetcher, re-review-coordinator, ado-writer); (2) introduces a four-tier Notice doctrine (OK / EMPTY-BY-DESIGN / DEGRADED / ABORTED) with canonical HTTP-tier mapping helpers (classify-http-error, parse-write-response, fetch-iterations, fetch-work-items, notices) and a mandatory end-of-run Trailer; (3) adds a γ-downgrade in classifyThread when the diff range is full, suppresses the reply on addressed threads, and replaces the hard-coded main default-branch fallback with a Gitflow-aware chain. Versions bump 1.0.0 → 1.2.9. Unrelated to the stated scope, several files also relax the workspace's Node requirement from ≥24 to ≥22.
Changes:
- Orchestrator split + Notice pipeline + Trailer formatting helpers, plus matching ADRs 0013/0014/0015.
- Platform-failure handling: discriminated-union helpers for ADO writes and reads; classify-thread γ-downgrade on
DIFF_RANGE=full; default-branch fallback chain. - Suppress reply on addressed threads (silent PATCH-to-fixed) and revise ADR 0006.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/claude-code/pr-review/commands/review-pr.md | Thin orchestrator delegating to sub-agents; parses NOTICES and DIFF_RANGE; calls formatTrailer. |
| apps/claude-code/pr-review/.agents/ado-fetcher.md | Emits DIFF_RANGE field, NOTICES array; routes iterations/work-items through new helpers. |
| apps/claude-code/pr-review/.agents/ado-writer.md | Routes every POST/PATCH through parseWriteResponse; emits NOTICES; uses kinds not in NoticeKind. |
| apps/claude-code/pr-review/.agents/re-review-coordinator.md | Threads DIFF_RANGE into classifyThread; silent PATCH-to-fixed; throws-via-Notice for match-finding. |
| apps/claude-code/pr-review/scripts/ado/notices.mjs | New Notice doctrine helpers (create/merge/format/trailer). |
| apps/claude-code/pr-review/scripts/ado/classify-http-error.mjs | Canonical HTTP status → tier mapping. |
| apps/claude-code/pr-review/scripts/ado/parse-write-response.mjs | Discriminated-union result for ADO writes. |
| apps/claude-code/pr-review/scripts/ado/fetch-iterations.mjs | Replaces parseIterations; ABORTED on empty/auth. |
| apps/claude-code/pr-review/scripts/ado/fetch-work-items.mjs | Replaces parseWorkItemIds; distinguishes EMPTY-BY-DESIGN from failure. |
| apps/claude-code/pr-review/scripts/pre-pr/detect-default-branch.mjs | Gitflow-aware fallback chain (develop → main → master → none). |
| apps/claude-code/pr-review/scripts/pre-pr.mjs | Adds Notice surface for suspicious-shape diffs. |
| apps/claude-code/pr-review/scripts/re-review/classify-thread.mjs | γ-downgrade when diffRange=full. |
| apps/claude-code/pr-review/scripts/re-review/match-finding.mjs | Throws TypeError on malformed inputs. |
| apps/claude-code/pr-review/scripts/ado-writer.mjs | parseAdoWriterResult returns discriminated union with notices. |
| apps/claude-code/pr-review/scripts/ado-fetcher.mjs | Deleted (helpers moved to scripts/ado/). |
| apps/claude-code/pr-review/tests/* | New tests for each helper; existing tests updated for new shapes. |
| apps/claude-code/pr-review/docs/adr/* | ADRs 0014/0015 added; ADRs 0004/0006 amended. |
| apps/claude-code/pr-review/CHANGELOG.md | Adds entries for 1.1.0 → 1.2.9 (missing 1.2.5). |
| apps/claude-code/pr-review/.claude-plugin/{plugin,marketplace}.json | Version bump to 1.2.9. |
| package.json, pnpm-workspace.yaml, README.md, CONTRIBUTING.md, AGENTS.md, apps/.../unic-confluence/, apps/.../auto-format/, .claude/skills/new-plugin/* | Workspace-wide Node ≥24 → ≥22 (out-of-scope). |
| docs/issues/** | Issue PRD/slice files marked resolved or added (some under inconsistent done/ paths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…n malformed-JSON httpExit:0 test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
orioltf
commented
May 14, 2026
Member
Author
orioltf
left a comment
orioltf
left a comment
There was a problem hiding this comment.
Addressing Copilot inline findings:
F2 (NoticeKind missing kinds) — Fixed in 08163e4: added 'delta-reply' and 'completion-marker' to the NoticeKind typedef in notices.mjs and to the enum list in ADR-0014.
F4 (mergeNotices severity-drop risk) — Documented, no code change: diff-range is emitted exclusively by the ADO Fetcher (only when DIFF_RANGE_FALLBACK=true). The Re-review Coordinator consumes DIFF_RANGE but emits no diff-range Notice of its own. First-wins dedup is safe because each kind has exactly one source agent. This single-source invariant is now documented in ADR-0014.
F5 (formatTrailer trailing — ) — Fixed in 08163e4: the aborted branch now conditionally omits the separator when abortReason is absent: ❌ Review aborted: auth (no trailing —). Test updated.
F6 (missing httpExit:0 + malformed JSON test) — Fixed in 08163e4: test case added to parse-write-response.test.mjs pinning the behavior.
F8 (inconsistent done/ paths) — Fixed in 08163e4: all issue files consolidated to docs/issues/<PRD>/done/ layout; old docs/issues/done/ flat structure removed.
Non-actionable findings:
F1 (CHANGELOG v1.2.5 gap): This is a branch-development artifact. Commit 7fcdb5c bumped plugin.json to 1.2.5 but kept the changelog as [Unreleased]; commit f343b68 then promoted it directly to [1.2.6], folding the v1.2.5 content into that entry. No data is lost — the classifyThread diffRange change is documented under [1.2.6]. No tags fire until merge to main, so there is no public contract to honour.
F3 (Node ≥22 out of scope): The change is intentional and predates the PR feature work — commit de533e1 explains the rationale: pnpm useNodeVersion: 24.15.0 was blocked by a firewall in sandboxed AFK runs, and Biome's platform binary wasn't installed for Linux. Relaxing engines to >=22 unblocks cross-platform CI without changing the local dev target (.nvmrc still pins 24.15.0). CI already exercises both Node 22 and 24.
F7 (no env var override for detect-default-branch): Valid nice-to-have, but out of scope of any current PRD. Filed as a follow-on improvement for the pre-PR mode feature set.
… override Tracks F7 from Copilot review of PR #31 — valid improvement but out of scope of any current PRD. Deferred until after the orchestrator-split branch ships. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d + sentinel doc - B3: map malformed-request kind to 'malformed' reason instead of 'transient' - B1: filter null array elements before reduce; treat all-null array as empty-iterations - B4: document latestCommitSha '' sentinel in JSDoc comment - D1: add tests for missing value key and HTTP 400 → malformed paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t guard Route non-zero exit codes through classifyHttpError for consistent tier mapping (auth/transient/malformed) matching fetch-iterations.mjs pattern. Guard against null/non-object elements in ADO value array to prevent TypeError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…c + test coverage Corrects the classify-thread JSDoc to accurately list 5 ordered rules (ADR-0004) instead of collapsing status-check and intersection-check into one; documents minor-findings omission from the trailer in notices.mjs; adds γ-downgrade/status=fixed test, full RESOLVED_STATUSES coverage, re-review trailer test, and mergeNotices null-source tolerance test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…am in classified write failures
- A1: parseAdoWriterResult returns { ok: false, reason: 'malformed' } on malformed NOTICES JSON
instead of silently falling back to empty notices; broadened NOTICES regex to detect
unclosed arrays too
- A3: parseWriteResponse appends errStream detail to classified failure messages (same
pattern already used for malformed-response path)
- D4: add HTTP 400 and 422 test cases to parse-write-response.test.mjs
- D10: update malformed JSON (non-zero exit) test to assert kind: 'network'
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e, add none-notice - Import Notice from notices.mjs instead of local typedef (kind was string, not NoticeKind) - Trim whitespace from remoteHeadBranch before truthy check (whitespace-only input now falls through to fallback chain) - Add warning Notice to source='none' result for consistency with levels 2–4 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…otice assertions Add whitespace-only remoteHeadBranch test (D6) and update source='none' assertions to expect a warning notice (pairs with A4 fix in 3ee5c6a). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <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.
Why
review-pr.mdhad grown to ~1000 lines, creating two compounding problems:This branch delivers the full orchestrator split (ADR-0013) plus three tightly scoped follow-on features.
What changed
Orchestrator split (specs A1–A4, B1–B6)
review-pr.mdrefactored into a thin orchestrator (~200 lines): prerequisites, mode detection, one delegation block per mode..agents/:pr-review:ado-fetcher— fetches PR metadata, iterations, changed files, raw diff.pr-review:re-review-coordinator— thread classification, finding matching, PATCH-to-fixed.pr-review:ado-writer— inline thread posting, summary comment.{ kind, tier, message }objects propagate from every sub-agent into the Trailer so failures are always visible.Platform failure handling
classify-http-error.mjs— canonical HTTP tier mapping (ok / degraded / aborted).parse-write-response.mjs— discriminated union result from ADO Writer.fetch-iterations.mjs/fetch-work-items.mjs—ABORTEDtier on auth failure.classifyThread(ADR-0004 amendment).Suppress addressed reply (v1.2.6)
addressedthreads are PATCHed to fixed (status 2) silently — no more "Resolved — thanks!" reply generating ADO notifications.classifyThreadacceptsdiffRangeparameter; on'full',addressed/obsolete→pending(conservative downgrade).Gitflow-aware default-branch detection (v1.2.9)
detect-default-branch.mjs: fallback chainremote-show → develop → main → master → none; emits a warning Notice at every fallback level;noneaborts pre-PR run.Workspace-wide Node ≥22 (incidental, commit
de533e1)engines.noderelaxed from>=24to>=22across all packages to unblock cross-platform CI sandbox runs:pnpm useNodeVersionwas hitting a firewall, and Biome's platform binary wasn't installed for Linux. The local dev target (.nvmrcpins24.15.0) is unchanged. CI already exercises both Node 22 and 24.Test plan
tests/*.test.mjs) —pnpm testpasses (223 tests)pnpm typecheckpassespnpm check(Biome + Prettier) passesaddressedthreads no longer generate reply notifications🤖 Generated with Claude Code