fix(impl-review): stop treating a legitimate score of 0 as a crashed review

[MarkusNeusinger]

The bug

impl-review.yml used quality score 0 as its sentinel for "the AI review produced no output". But 0 is also a score the review prompt mandates: the Stage 1 auto-reject gates in prompts/workflow-prompts/ai-quality-review.md require exactly Score = 0, verdict = REJECTED for AR-08 (clipped element) and AR-09 (mandatory title not visible).

Prompt and workflow therefore contradicted each other, and the pipeline was guaranteed to dead-end on exactly the plots it is designed to reject hardest.

The deadlock chain

1. Plot renders without a visible title → AR-09 → reviewer returns Score: 0/100, Verdict: REJECTED — this is correct behaviour
2. Extract quality score normalised it: ::warning::Invalid quality score '0', defaulting to 0
3. Validate review output fired on score == '0' → ::error::AI Review did not produce valid output files → ai-review-failed → exit 1
4. exit 1 skipped Add verdict label and take action — which the file itself documents as "the pipeline's only hand-off point: every downstream workflow (merge, repair) starts from a call made right here"
5. So impl-repair was never dispatched and the missing title was never fixed
6. impl-review-retry.yml rescued once → the re-review scored 0 again (deterministically — the plot was unchanged) → ai-review-failed re-applied
7. ai-review-failed + ai-review-rescued matches no watchdog case (watchdog-stuck-jobs.yml:116 only emits a ::warning:: and defers to a human) → PR stranded permanently

Evidence

Six open PRs sit in exactly that state, each with an AR-09 verdict already in hand:

PR	Library / spec	Reported score	Verdict	Gate
#10152	seaborn windrose-basic	0	REJECTED	AR-09
#10130	muix streamgraph-basic	0	REJECTED	AR-09
#10009	matplotlib wireframe-3d-basic	0	REJECTED	AR-09
#10003	d3 ternary-basic	0	REJECTED	AR-09
#9968	plotnine treemap-basic	0	REJECTED	AR-09
#9776	matplotlib polar-basic	0	REJECTED	AR-09

This was never an infrastructure failure. In run 31035492709 the Claude action reported "subtype": "success", "is_error": false, 15 turns, permission_denials_count: 0 — and posted a complete review ending in ### Score: 0/100 / ### Verdict: REJECTED. 94 of the last 100 impl-review runs are green; the 6 failures are these gate-tripping plots.

The fix

Output presence becomes its own signal, decoupled from the score value:

• Extract quality score now emits has_output alongside score. 0 is accepted as a valid score; only a non-numeric or out-of-range value marks output as missing.
• The six gates that keyed off score != '0' / score == '0' now key off has_output.
• A score of 0 therefore flows into the normal ai-rejected → impl-repair path (threshold floor is 50, so 0 < 50 → rejected → repair dispatched), and only genuinely absent output raises ai-review-failed.

Second, latent bug fixed in the same step: the comment fallback read .comments[-1].body, but on a retry the workflow's own "auto-retrying" notice is posted after the review — so the fallback searched the notice and found no score. It now selects the last claude[bot] comment.

Deliberately not changed: watchdog-stuck-jobs.yml. With the root cause fixed, "review produced no output twice in a row" (PRs #9953/#9952/#9951, which have no claude[bot] comment at all) is a genuine failure that should escalate to a human rather than loop forever.

Verification

GitHub Actions changes have no verification loop in this repo, so the step's shell body was tested directly: a harness extracts the Extract quality score run: block verbatim from the YAML and exercises it with a stubbed gh.

--- the regression that caused the deadlock ---
PASS  file score 0 (AR-09 auto-reject)               score=0    has_output=true
PASS  comment fallback, score 0                      score=0    has_output=true
--- normal operation must be unchanged ---
PASS  file score 87 / 100 / 1 / trailing-newline 73  score=...  has_output=true
PASS  comment fallback, score 87                     score=87   has_output=true
--- genuine 'no output' must still be detected ---
PASS  no file, no review comment                     score=0    has_output=false
PASS  no file, comment without a score line          score=0    has_output=false
PASS  file with non-numeric garbage                  score=0    has_output=false
PASS  file with out-of-range score                   score=0    has_output=false
PASS  empty file                                     score=0    has_output=false

ALL CASES PASS

YAML validity re-checked after the edit (yaml.safe_load, 20 steps parsed).

Residual risk: the if: expression rewrites and the REPOSITORY env addition are only observable on a real pipeline run. Recovery path for the six stranded PRs after merge: re-dispatch impl-review.yml -f pr_number=<n>, which will now score them 0, label ai-rejected, and hand them to impl-repair to fix the titles.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes a workflow logic conflict in the implementation review pipeline by decoupling “review produced output” from the numeric quality score, allowing a legitimate score of 0 (mandatory for specific auto-reject gates) to proceed through the normal ai-rejected → impl-repair path instead of being treated as a crashed review.

Changes:

• Update .github/workflows/impl-review.yml to emit a has_output signal alongside score, and gate downstream steps on has_output instead of score != 0.
• Improve the score recovery fallback to read from the last claude[bot] comment (instead of the last comment overall).
• Add a detailed [Unreleased] changelog entry describing the deadlock chain and the fix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
CHANGELOG.md	Adds an [Unreleased] fix entry documenting the pipeline deadlock and resolution.
.github/workflows/impl-review.yml	Treats 0 as a valid score and introduces has_output to detect genuinely missing review output; adjusts step gating accordingly.

[MarkusNeusinger]

Good catch, and it was accurate — for the version you reviewed. That commit used gh api --paginate --jq '...', where --jq is applied per page, so --paginate emitted one filtered result per page and head -1 took the first (oldest) page's match.

The follow-up commit (5c6d24d, pushed while this review was being generated) already replaced that form while closing a separate staleness bug:

gh api --paginate "repos/${REPOSITORY}/issues/${PR_NUM}/comments?per_page=100" 2>/dev/null \
  | jq -s --arg since "$REVIEW_STARTED_AT" -r \
      'add // []
       | [.[] | select(.user.login == "claude[bot]")
              | select(.created_at >= $since)]
       | last | .body // ""'

jq -s slurps every page into one array, add concatenates them, and last then picks the newest match across all pages — not per page. The remaining head -1 operates on a single comment body, where it selects the one ### Score: N/100 heading in that body.

Verified rather than asserted: the test harness stubs gh to emit two concatenated arrays (exactly what --paginate produces), with the older match on page 1 and the current one on page 2.

--- pagination: --paginate emits one array per page (Copilot #10179) ---
PASS  two pages, newest match wins                     score=91   has_output=true
PASS  two pages, all stale                             score=0    has_output=false

The pre-fix form returns 55 (page 1) on that fixture; the current form returns 91. Full suite is 19 cases, all passing.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.