[bawoodruff]

Addresses items 3, 4, and 5 of #41 — one theme, as the reporter grouped them. Items 1–2 are in #45.

The shepherd assumed a reviewer that posts inline threads and answers APPROVED-or-nothing. This repo's reviewer posts a single review body and answers with three verdicts. Three failures compounded from that one mismatch.

Item 3 — findings were invisible

The state machine keyed entirely on THREAD_COUNT from reviewThreads, but gh pr review --body-file submits one top-level body and cannot attach line comments. THREAD_COUNT was permanently 0, the fix route never fired, and Steps 5–8 were dead code against this reviewer.

Step 2 now also reads the review body — from the REST payload it already fetches for the verdict, so no extra call — and routing keys on ACTIONABLE: threads, a body, or both.

An APPROVED body is deliberately not actionable. Approval is terminal success; reopening it restarts the treadmill the skill already warns about.

Item 4 — a by-design failure was reported as broken CI

The review gate check fails deliberately when the reviewer requests changes. It counted toward CI_FAILING, so Route D fired "CI is failing — investigate CI failures" on a PR whose builds were all green and whose real signal was a review finding.

CI_FAILING now excludes it by name via REVIEW_GATE_CHECK — overridable, which #30's consumer repos will need. It still counts toward CI_PENDING, where "the review is running" is exactly what it means.

Item 5 — a concluded review was polled as if it might change

Route A terminated only on APPROVED, so a COMMENTED head fell to the wait route and polled 30 × 90s ≈ 45 minutes before escalating as "reviewer appears stuck" — with the findings sitting unread in the body.

Route E now waits only on PENDING. COMMENTED and CHANGES_REQUESTED are terminal: they route to the fix path when they carry findings, and to a new Route G ("Reviewed, Not Approved") when they don't — which reports the verdict, quotes the body, and stops without a wakeup, because no further automated event is coming.

Verified against this repo's own PRs

Case	Old	New
#42 — COMMENTED, 0 threads, 4,161-char body	Route E → poll ~45 min → "stuck"	Route B → fix findings
#39 / #40 / #43 — APPROVED with a body	Route A (done)	Route A (done) — unchanged
verdict gate FAILURE, all builds green	CI_FAILING=1 → "investigate CI"	CI_FAILING=0 → routes to B
genuine build failure alongside the gate	caught	still caught — no regression
verdict gate PENDING	pending	CI_PENDING=1 — still waits

The middle three rows are the regressions I most needed to rule out: that approvals still terminate, that a real build failure isn't masked by the exclusion, and that a running review still counts as pending.

Also

Steps 5/7/8 no longer assume every finding lives in a resolvable thread — a body-posting reviewer gets one disposition comment per round, listing declined findings with reasoning, since it offers nothing to reply to or resolve. plugin/agents/pr-shepherd.md carries the same corrections so the agent and skill variants can't drift on what counts as done.

Bumped to 3.4.0 with a CHANGELOG entry (behavior change under plugin/).

🤖 Generated with Claude Code

https://claude.ai/code/session_01WDPKxAK1dJvyn3gXaGfsFc

[github-actions]

Agent QA — reviewer audit score

Defect	Technique	Result
D1	diff parallel implementations	FOUND
D2	falsify doc comments	FOUND
D3	what would this test still pass with	FOUND
D4	language traps (float equality)	FOUND
D5	language traps / semantic (truncation before filter)	FOUND
D6	state/failure paths (error after commit + retry)	FOUND

SCORE: 6/6 (gate threshold: 4; recorded baseline: 6/6 (100%))

Full review output: workflow run artifact agent-qa-review-output

Planted-defect harness tests/agent-qa/; policy: docs/adr/011-agent-qa-in-ci.md.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: plugin/agents/pr-shepherd.md, plugin/skills/shepherd-pr/SKILL.md, CHANGELOG.md, VERSION, plugin/.claude-plugin/plugin.json
• Overall verdict: COMMENT — one major correctness gap; the SKILL variant is solid

---

Major Issues (should fix)

plugin/agents/pr-shepherd.md:391 — Route G is absent from the agent; the CHANGELOG's parity claim is false

SKILL.md adds explicit Route G:

IF ACTIONABLE == 0 AND … AND VERDICT is COMMENTED or CHANGES_REQUESTED
  → print REVIEWED-NOT-APPROVED REPORT, stop (no ScheduleWakeup)

The agent has no equivalent. Its Step 7 else-branch adds a comment ("COMMENTED/CHANGES_REQUESTED is TERMINAL — never wait for it to change on its own") but then says "Act on its findings" — when ACTIONABLE is 0 there are none, so the instruction gives no exit signal, and the loop continues to Step 1 and repeats. That cycle runs until MAX_WAIT_ITER before escalating as "reviewer appears stuck": the exact bad behaviour item 5 was written to fix, still present in the agent variant.

The CHANGELOG's claim — "plugin/agents/pr-shepherd.md carries the same corrections, so the agent and skill variants cannot drift apart on what counts as done" — is demonstrably false for this specific fix.

Fix: add an explicit guard in the pr-shepherd.md loop structure (after the Done Check block) mirroring Route G's exit condition: when ACTIONABLE is 0 and VERDICT is COMMENTED or CHANGES_REQUESTED, write the Reviewed-Not-Approved report and exit without looping.

---

Minor Issues (optional)

plugin/agents/pr-shepherd.md:257 — redundant gh api call for REVIEW_BODY

Lines 248–250 fetch the reviews payload to extract VERDICT; lines 257–259 make a second, identical gh api "repos/${REPO}/pulls/${PR}/reviews" --paginate call for REVIEW_BODY. SKILL.md correctly captures this in $VERDICT_RAW and derives both values from the same snapshot. The double call wastes a round-trip and opens a race window: if a new review lands between the two calls, VERDICT and REVIEW_BODY reflect different reviews and BODY_FINDINGS can be computed against mismatched state. Fix: capture the payload in VERDICT_RAW and derive both variables from it, as SKILL.md does.

plugin/agents/pr-shepherd.md:370 — Done Check uses stale $BODY_FINDINGS from Step 3

Step 7 re-fetches OPEN_COUNT and VERDICT live (correct), but evaluates the $BODY_FINDINGS value set in Step 3 — before the push. If the push triggers a fast automated approval between Steps 5–6 and the Done Check, $BODY_FINDINGS still reflects the pre-fix CHANGES_REQUESTED state and fails the [ "$BODY_FINDINGS" -eq 0 ] guard despite the reviewer having approved. The consequence is one wasted extra loop rather than a wrong decision, but it's a silent inconsistency. Fix: re-derive $BODY_FINDINGS from the freshly-fetched $VERDICT and $REVIEW_BODY at the top of Step 7 (or accept the extra iteration as acceptable debt, and document it).

plugin/skills/shepherd-pr/SKILL.md:158 — changelog and code comment overstate when the gate check appears in CI_PENDING

The comment (and the CHANGELOG entry for item 4) says the gate check "still counts toward CI_PENDING, where 'the review is running' is exactly what it means." That is true only while the gate is IN_PROGRESS. When the reviewer posts CHANGES_REQUESTED, the gate check transitions to FAILURE — which the CI_PENDING query excludes by state, so it is counted in neither CI_FAILING nor CI_PENDING. The behavior is correct and harmless (VERDICT carries the semantics), but the comment will mislead someone debugging a FAILURE gate. Fix: tighten the wording — "counts toward CI_PENDING while it is running (IN_PROGRESS/QUEUED); once it reaches FAILURE the gate's signal is carried entirely by VERDICT."

---

Security Findings

None.

---

Positive Observations

• The SKILL.md routing logic is clean and complete: Route G correctly terminates the loop on terminal verdicts with nothing actionable, and the CI_FAILING exclusion is well-guarded with $REVIEW_GATE_CHECK being overridable for consumer repos.
• Reusing $VERDICT_RAW for both VERDICT and REVIEW_BODY in SKILL.md is the right design — one call, one consistent snapshot.
• The 40-char body threshold for BODY_FINDINGS is conservative and the decision to leave APPROVED bodies non-actionable (avoiding the "treadmill" re-open) is well-reasoned.
• The PR's verification table (test cases against real PRs #39, #40, #42, #43) is exactly the right evidence to accompany a routing change.

[bawoodruff]

Fixed in 4b084ef (rebased onto main now that #45 has merged).

Major — correct, and the sharper point is that my CHANGELOG asserted the opposite. I claimed the agent "carries the same corrections" and that the two variants "cannot drift apart on what counts as done." That was false for the one fix that mattered most: SKILL.md got Route G's terminal exit, the agent got only a comment saying the verdict is terminal followed by "act on its findings" — which, when there are none, gives no exit signal at all. It looped to Step 1, re-read the same finished review, and ran to MAX_WAIT_ITER before escalating as "reviewer appears stuck". Item 5's exact behavior, left intact in half the stack, under a claim that it wasn't.

The agent now has an explicit guard mirroring Route G, and the CHANGELOG says what is actually true — including why the agent needed it. Exercised as real shell across seven states:

State	Result
green, no findings, COMMENTED	exit — reviewed, not approved
green, no findings, CHANGES_REQUESTED	exit
green, body findings, COMMENTED	continue → fix
green, threads open, COMMENTED	continue → fix
green, no findings, PENDING	continue → wait
CI not green, no findings, COMMENTED	continue
green, no findings, APPROVED	never reaches the guard — the done branch catches it

Minor (double fetch) — fixed. Worth noting the race you identified is the more serious half: a review landing between the two calls leaves VERDICT and REVIEW_BODY describing different reviews, so BODY_FINDINGS is computed from mismatched state. One VERDICT_RAW snapshot now feeds both, matching SKILL.md.

Minor (stale BODY_FINDINGS) — fixed rather than documented as debt. The Done Check re-derives verdict and body from one fresh snapshot.

Minor (CI_PENDING wording) — fixed in both places. You're right that on FAILURE the gate falls out of both counts. The comment now says so and explains why that's deliberate: a check whose only meaning is "the reviewer said no" must not also read as a broken build.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: plugin/agents/pr-shepherd.md, plugin/skills/shepherd-pr/SKILL.md, CHANGELOG.md, VERSION, plugin/.claude-plugin/plugin.json
• Overall verdict: REQUEST CHANGES — Route G is structurally non-functional in the agent for its primary target verdict; item 4's fix was applied to the skill but not ported to the agent, and the two gaps compound.

---

Critical Issues (must fix before merge)

plugin/agents/pr-shepherd.md:344–345, 413 — Route G can never fire for CHANGES_REQUESTED

ALL_OK is computed by checking that every CI check is SUCCESS/SKIPPED/NEUTRAL/STALE/CANCELLED. When VERDICT=CHANGES_REQUESTED the "Claude verdict" gate check is in FAILURE state (that is the gate's designed behaviour), so ALL_OK=false. Route G's guard is [ "$ALL_OK" = "true" ], which therefore fails for every CHANGES_REQUESTED verdict. Route G — the fix for item 5, "concluded review was polled as if it might change" — cannot fire for the primary verdict it was written to handle; the agent falls through to the KG write-and-loop path and exhausts MAX_ITER before escalating, reproducing the original bug.

The gate check exclusion (REVIEW_GATE_CHECK / select(.name != $gate)) was added to SKILL.md's CI_FAILING computation but was not applied to the agent's ALL_OK query in Step 7. Fix: mirror the SKILL.md exclusion in Step 7's ALL_OK jq expression — exclude the gate check by name before the all(…) test, e.g.:

REVIEW_GATE_CHECK="${REVIEW_GATE_CHECK:-Claude verdict}"
ALL_OK=$(echo "$CHECKS_JSON" | jq -r --arg gate "$REVIEW_GATE_CHECK" \
  '(length > 0) and ([.[] | select(.name != $gate) | .state]
   | all(. == "SUCCESS" or . == "SKIPPED" or . == "NEUTRAL" or . == "STALE" or . == "CANCELLED"))')

---

Major Issues (should fix)

plugin/agents/pr-shepherd.md:170–171 — item 4's fix (gate check exclusion from FAILURES) is absent from the agent

SKILL.md adds --arg gate "$REVIEW_GATE_CHECK" and select(.name != $gate) to the CI_FAILING query so the gate check's designed FAILURE is not treated as broken CI. The agent's Step 2 FAILURES query has no equivalent filter. When the reviewer requests changes, "Claude verdict" appears in FAILURES and Step 2 attempts to diagnose and fix it as a CI failure — consuming an iteration on a check that is working correctly. The CHANGELOG's claim that "plugin/agents/pr-shepherd.md carries all three corrections" is false for item 4. Fix: add the name filter to the Step 2 FAILURES query, matching the SKILL.md pattern.

---

Minor Issues (optional)

CHANGELOG.md:33 — parity claim is false for item 4

"plugin/agents/pr-shepherd.md carries all three corrections" — items 3 and 5 are present; item 4 is not. Update after the code is fixed to avoid a future reviewer trusting the claim to skip a cross-check.

plugin/skills/shepherd-pr/SKILL.md:~307 — DISMISSED and other non-standard verdicts fall through all routes

Route E now matches only PENDING; Route G matches only COMMENTED or CHANGES_REQUESTED. A DISMISSED verdict (or any API response not in that set) matches no route, exits with no wakeup and no report. Document the intended behaviour or add a final catch-all for unknown verdicts.

---

Positive Observations

• The single-fetch pattern for VERDICT_RAW (derive both VERDICT and REVIEW_BODY from one API call) is the correct fix for the race window; consistently applied across both Step 3 and the Step 7 re-derive in both files.
• SKILL.md's ACTIONABLE abstraction cleanly unifies thread and body findings across all routing branches; the inline comments explaining why THREAD_COUNT-only routing was blind to body-posting reviewers are clear and correct.
• Route G's report template and the "do not schedule a wakeup" directive are both present and correctly placed in SKILL.md.

[bawoodruff]

Shepherd round 1 — disposition for review on 4b084ef4

All four findings accepted and fixed in 1db77a2e. No disputes this round.

Critical — Route G can never fire for CHANGES_REQUESTED (pr-shepherd.md:344–345, 413)

Fixed. Confirmed on the merits, and this PR's own CI was the reproduction: gh pr checks 46 on 4b084ef4 reported Claude verdict fail with every other check green, against a CHANGES_REQUESTED verdict.

Empirical check of the two expressions against that exact payload:

$ echo '[{"name":"Claude verdict","state":"FAILURE"},{"name":"Docs site builds","state":"SUCCESS"}]' \
  | jq -r '(length > 0) and ([.[].state] | all(. == "SUCCESS" or . == "SKIPPED" or . == "NEUTRAL" or . == "STALE" or . == "CANCELLED"))'
false        # old — ALL_OK pinned false, Route G guard unreachable

$ ... | jq -r --arg gate "Claude verdict" '(length > 0) and ([.[] | select(.name != $gate) | .state] | all(...))'
true         # new — Route G fires

Regression cases both hold: [] still yields false (the vacuous-true guard survives the rewrite), and Claude verdict=FAILURE alongside Docs site builds=FAILURE still yields false, so a genuine build failure is not masked.

Major — item 4's exclusion absent from the agent's Step 2 (pr-shepherd.md:170–171)

Fixed. FAILURES now carries --arg gate / select(.name != $gate). Verified the gate drops out while a real failure survives: the query over [Claude verdict=FAILURE, Docs site builds=FAILURE, gate=SUCCESS] returns exactly Docs site builds.

REVIEW_GATE_CHECK is defined once in the startup variable block (defaulting to Claude verdict, overridable per #30) and re-defaulted idempotently at each use, since Step 7 can run without Step 2 having executed.

Minor — CHANGELOG parity claim false for item 4 (CHANGELOG.md:33)

Fixed. The claim is now true, and rather than leave it as a bare assertion a future reviewer has to cross-check, the entry spells out both queries the agent was missing and why the ALL_OK one was the load-bearing failure.

Minor — DISMISSED falls through all routes (SKILL.md:~307)

Fixed, and in both files rather than only the skill — the agent's terminal-verdict exit had the identical enumeration. Route G's guard is now the complement of the two handled states (not APPROVED and not PENDING) instead of a COMMENTED/CHANGES_REQUESTED list. APPROVED exits at Route A, PENDING waits at Route E, so every other value — DISMISSED or anything unrecognised — is terminal-with-nothing-to-do and now reports and stops. A catch-all was the alternative; the complement is strictly less to keep in sync and removes the failure mode rather than adding a branch for it.

Validation

No workflow YAML changed, so actionlint was not applicable. The edited shell constructs pass bash -n, and the jq expressions were exercised against both live PR data and synthetic edge cases as shown above. Versions stay consistent — the new notes land under the existing [3.4.0] section; VERSION and plugin.json are untouched at 3.4.0.

[cortexa-llc-reviewer]

Review Summary

• Files reviewed: plugin/agents/pr-shepherd.md, plugin/skills/shepherd-pr/SKILL.md, CHANGELOG.md, VERSION, plugin/.claude-plugin/plugin.json
• Overall verdict: APPROVE — all previously-raised Critical and Major issues are fixed; no new critical or major defects found.

---

Previously-Raised Issues — Verified Fixed

Critical (last review): Route G could never fire for CHANGES_REQUESTED because ALL_OK counted the gate check (which is in FAILURE by design) and was always false.

Fixed. ALL_OK in Step 7 of the agent now excludes $REVIEW_GATE_CHECK via select(.name != $gate) before the all() test. With the gate excluded, a CHANGES_REQUESTED head where all real builds pass produces ALL_OK=true, and Route G can fire as intended.

Major (last review): Item 4's gate-check exclusion was applied to SKILL.md's CI_FAILING but not to the agent's FAILURES query in Step 2.

Fixed. The agent's Step 2 now passes --arg gate "$REVIEW_GATE_CHECK" and prepends select(.name != $gate) before the failure-state filter, matching the skill.

Major (last review, COMMENTED): Route G was absent from the agent entirely.

Fixed. Lines 431–438 of the agent add the explicit Route G guard (ALL_OK=true && OPEN_COUNT==0 && BODY_FINDINGS==0 && VERDICT != APPROVED && VERDICT != PENDING → exit 0), mirroring SKILL.md Route G.

Minor (last review, COMMENTED): Redundant gh api call for REVIEW_BODY.

Fixed. Both Step 3 and the Done Check (Step 7) now fetch VERDICT_RAW once and derive both VERDICT and REVIEW_BODY from a single snapshot, eliminating the race window.

---

Minor Observations (no action required)

plugin/agents/pr-shepherd.md — triple declaration of REVIEW_GATE_CHECK
REVIEW_GATE_CHECK="${REVIEW_GATE_CHECK:-Claude verdict}" appears at initialisation (~line 48), in Step 2 (~line 175), and in Step 7 (~line 356). The :- default syntax makes each subsequent declaration a no-op when the variable is already set, so this is harmless — and the per-site comments make each block self-documenting. Purely cosmetic.

plugin/agents/pr-shepherd.md:358 — vacuous-truth comment now slightly misleading
The (length > 0) and … guard was originally written to catch an entirely empty checks array. After the select(.name != $gate) filter, the downstream all() may operate on an empty array even when checks exist (if the gate is the only check), and that is intentional behaviour. The comment still says "Guard against vacuous-true when checks array is empty" — a one-line note that the vacuous case also covers "only the gate remains after filtering" would clarify the intent for future editors.

---

Positive Observations

• The three bug fixes are tightly scoped and address exactly the failure modes documented in #41.
• The single-snapshot pattern for VERDICT_RAW (derive both VERDICT and REVIEW_BODY from one call, documented with the race-window rationale) is the right design and consistent across both variants.
• REVIEW_GATE_CHECK is overridable at each use site, matching the consumer-repo requirement from #30.
• Route G uses the complement of {APPROVED, PENDING} rather than enumerating COMMENTED/CHANGES_REQUESTED, correctly catching DISMISSED and any future unrecognised verdict without a silent exit.
• The verification table in the PR description covers the most dangerous regressions (approvals still terminate, real build failures still surface, running reviews still count as pending) — useful evidence that the exclusion is scoped correctly.