From a102276637a1ef49aed56087138f7ee7c5f5e2d9 Mon Sep 17 00:00:00 2001 From: Markus Neusinger <2921697+MarkusNeusinger@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:02:18 +0200 Subject: [PATCH 1/3] fix(impl-review): stop treating a legitimate score of 0 as a crashed review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workflow 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). So every plot that correctly tripped those gates was reported as a crashed review: `Validate review output` raised "AI Review did not produce valid output files", applied `ai-review-failed` and exited 1 — skipping the verdict step, which the file itself documents as "the pipeline's only hand-off point". impl-repair was therefore never dispatched and the missing title never fixed. The retry listener rescued once, the re-review scored 0 again (deterministically: the plot had not changed), and the resulting `ai-review-failed` + `ai-review-rescued` pair matches no watchdog case, stranding the PR permanently. Six PRs sat in exactly that state (#10152, #10130, #10009, #10003, #9968, #9776), each with an AR-09 verdict already in hand. Output presence is now its own `has_output` step output, decoupled from the score value. A score of 0 flows into the normal rejected -> repair path; only genuinely absent or malformed output raises `ai-review-failed`. The six gates that keyed off `score != '0'` now key off `has_output`. The comment fallback had a second, latent bug: it 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. Verified with a harness that extracts the step body verbatim from the YAML and exercises 12 cases: score 0 via file and via comment fallback, normal scores (1/73/87/100), and the four genuine no-output shapes (no file + no comment, comment without a score line, non-numeric file, out-of-range file). All pass. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/impl-review.yml | 48 ++++++++++++++++++++++++------- CHANGELOG.md | 19 ++++++++++++ 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/.github/workflows/impl-review.yml b/.github/workflows/impl-review.yml index 18f2c83d63..ee3e0c4492 100644 --- a/.github/workflows/impl-review.yml +++ b/.github/workflows/impl-review.yml @@ -337,23 +337,51 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUM: ${{ steps.pr.outputs.pr_number }} + REPOSITORY: ${{ github.repository }} run: | + # `0` is a LEGITIMATE score, not a sentinel: the Stage 1 auto-reject + # gates in prompts/workflow-prompts/ai-quality-review.md mandate + # exactly "Score = 0, verdict = REJECTED" for AR-08 (clipped element) + # and AR-09 (mandatory title not visible). Conflating that with "the + # action produced nothing" is what deadlocked PRs #10152/#10130/ + # #10009/#10003/#9968/#9776: a correctly-rejected plot was reported as + # a crashed review, so the repair that would have fixed the title was + # never dispatched. Whether output exists is therefore tracked + # separately from what the score is. + HAS_OUTPUT=false if [ -f "quality_score.txt" ]; then SCORE=$(cat quality_score.txt | tr -d '[:space:]') + HAS_OUTPUT=true else - SCORE=$(gh pr view "$PR_NUM" --json comments -q '.comments[-1].body' | grep -oP 'Score: \K\d+' | head -1 || echo "0") + # Fall back to the review comment. Select the last *claude[bot]* + # comment rather than the last comment overall — on a retry the + # github-actions[bot] "auto-retrying" notice lands after it, and + # `.comments[-1]` would then read that notice and find no score. + SCORE=$(gh api --paginate "repos/${REPOSITORY}/issues/${PR_NUM}/comments?per_page=100" \ + --jq '[.[] | select(.user.login == "claude[bot]")] | last | .body // ""' 2>/dev/null \ + | grep -oP '^###\s+Score:\s+\K\d+' | head -1 || true) + if [ -n "$SCORE" ]; then + HAS_OUTPUT=true + echo "::notice::quality_score.txt missing; recovered score ${SCORE} from the review comment" + fi + fi + + # Only a non-numeric or out-of-range value is invalid. 0 passes. + if [ "$HAS_OUTPUT" = "true" ] && { ! [[ "$SCORE" =~ ^[0-9]+$ ]] || [ "$SCORE" -gt 100 ]; }; then + echo "::warning::Invalid quality score '$SCORE' — treating as missing review output" + HAS_OUTPUT=false fi - # Validate score is a number between 1-100, default to 0 if invalid - if ! [[ "$SCORE" =~ ^[0-9]+$ ]] || [ "$SCORE" -lt 1 ] || [ "$SCORE" -gt 100 ]; then - echo "::warning::Invalid quality score '$SCORE', defaulting to 0" + if [ "$HAS_OUTPUT" != "true" ]; then SCORE="0" fi echo "score=$SCORE" >> $GITHUB_OUTPUT + echo "has_output=$HAS_OUTPUT" >> $GITHUB_OUTPUT + echo "::notice::score=${SCORE} has_output=${HAS_OUTPUT}" - name: Validate review output - if: steps.review.conclusion == 'success' && steps.score.outputs.score == '0' + if: steps.review.conclusion == 'success' && steps.score.outputs.has_output != 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUM: ${{ steps.pr.outputs.pr_number }} @@ -411,7 +439,7 @@ jobs: exit 1 - name: Add quality score label - if: steps.review.conclusion == 'success' && steps.score.outputs.score != '0' + if: steps.review.conclusion == 'success' && steps.score.outputs.has_output == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUM: ${{ steps.pr.outputs.pr_number }} @@ -440,7 +468,7 @@ jobs: } - name: Add preliminary verdict label (early) - if: steps.review.conclusion == 'success' && steps.score.outputs.score != '0' + if: steps.review.conclusion == 'success' && steps.score.outputs.has_output == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUM: ${{ steps.pr.outputs.pr_number }} @@ -480,11 +508,11 @@ jobs: fi - name: Install Python dependencies for metadata update - if: steps.review.conclusion == 'success' && steps.score.outputs.score != '0' + if: steps.review.conclusion == 'success' && steps.score.outputs.has_output == 'true' run: pip install pyyaml - name: Update metadata and implementation header - if: steps.review.conclusion == 'success' && steps.score.outputs.score != '0' + if: steps.review.conclusion == 'success' && steps.score.outputs.has_output == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} SPEC_ID: ${{ steps.pr.outputs.specification_id }} @@ -826,7 +854,7 @@ jobs: exit 1 - name: Add verdict label and take action - if: steps.review.conclusion == 'success' && steps.score.outputs.score != '0' + if: steps.review.conclusion == 'success' && steps.score.outputs.has_output == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUM: ${{ steps.pr.outputs.pr_number }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 209eb8cf7b..24eaf5e84f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,25 @@ aggregate instead: an italic *Catalog* line at the end of the version section an ### Fixed +- **A correctly rejected plot was reported as a crashed review, deadlocking the PR** — + `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 + (`AR-08` clipped element, `AR-09` mandatory title not visible) require exactly + "Score = 0, verdict = REJECTED". Every plot that tripped those gates therefore hit + `Validate review output` → `::error::AI Review did not produce valid output files` → + `ai-review-failed` → `exit 1`, which skipped the verdict step — the pipeline's *only* + hand-off point — so `impl-repair` was never dispatched and the missing title was never fixed. + The retry listener rescued once, the re-review scored `0` again (deterministically: the plot + was unchanged), and the resulting `ai-review-failed` + `ai-review-rescued` pair matches no + watchdog case, stranding the PR for good. Six PRs sat in exactly that state (#10152, #10130, + #10009, #10003, #9968, #9776), all with an `AR-09` verdict in hand. Output presence is now + tracked as its own `has_output` step output, so a score of `0` flows into the normal + rejected → repair path and only genuinely absent output raises `ai-review-failed`. The + comment fallback also now reads the last **`claude[bot]`** comment instead of the last comment + overall — on a retry the bot's own "auto-retrying" notice landed last and shadowed the score + it was looking for. Verified with a harness that extracts the step body verbatim from the YAML + and exercises 12 cases (score 0 via file and via comment fallback, normal scores, and the four + genuine no-output shapes) (#PRNUM). - **CI lint went red on every PR after a ruff minor bump** — `pyproject.toml` pins `ruff>=0.15.21` without an upper bound, CI resolved **0.16.0**, and that version started formatting Python code blocks inside Markdown: 20 tracked `.md` files suddenly "would be From 02cd9e1fed0d12fd4c0361770e95b4fe7463ae01 Mon Sep 17 00:00:00 2001 From: Markus Neusinger <2921697+MarkusNeusinger@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:03:06 +0200 Subject: [PATCH 2/3] docs(changelog): reference PR #10179 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24eaf5e84f..ad3be7f113 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -156,7 +156,7 @@ aggregate instead: an italic *Catalog* line at the end of the version section an overall — on a retry the bot's own "auto-retrying" notice landed last and shadowed the score it was looking for. Verified with a harness that extracts the step body verbatim from the YAML and exercises 12 cases (score 0 via file and via comment fallback, normal scores, and the four - genuine no-output shapes) (#PRNUM). + genuine no-output shapes) (#10179). - **CI lint went red on every PR after a ruff minor bump** — `pyproject.toml` pins `ruff>=0.15.21` without an upper bound, CI resolved **0.16.0**, and that version started formatting Python code blocks inside Markdown: 20 tracked `.md` files suddenly "would be From 5c6d24db673884223cc5dabf5e3542341d97dd23 Mon Sep 17 00:00:00 2001 From: Markus Neusinger <2921697+MarkusNeusinger@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:05:52 +0200 Subject: [PATCH 3/3] fix(impl-review): scope the score fallback to the current run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment fallback added in the previous commit selected the last claude[bot] comment, which fixed the ordering bug (the workflow's own "auto-retrying" notice is posted after the review and shadowed the score) but introduced a staleness bug: a PR that has already been reviewed carries older review comments with a score the current implementation never earned. PR #9948 is exactly that shape — its `quality:89` comes from its first review, and a later review that produced no output at all would now have silently inherited 89. A new `Record review start time` step stamps the boundary immediately before the Claude action runs, and the fallback only accepts claude[bot] comments created at or after it. RFC 3339 UTC timestamps compare correctly as strings here: GitHub and `date -u` emit the same fixed-width `...Z` format. One second of slack absorbs clock skew. Test harness extended from 12 to 17 cases, covering both the fresh comment followed by the notice, and the #9948 stale-comment shape. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/impl-review.yml | 38 +++++++++++++++++++++++++------ CHANGELOG.md | 12 ++++++---- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/.github/workflows/impl-review.yml b/.github/workflows/impl-review.yml index ee3e0c4492..f35cfac228 100644 --- a/.github/workflows/impl-review.yml +++ b/.github/workflows/impl-review.yml @@ -306,6 +306,14 @@ jobs: run: | gh api "repos/$REPOSITORY/issues/$PR_NUMBER/reactions" -f content=eyes + # Marks the boundary between "review comments that already existed" and + # "the comment this run produced", so the score fallback below can never + # pick up a previous attempt's score. One second of slack absorbs clock + # skew between the runner and GitHub's comment timestamps. + - name: Record review start time + id: t0 + run: echo "at=$(date -u -d '1 second ago' +%Y-%m-%dT%H:%M:%SZ)" >> $GITHUB_OUTPUT + - name: Run AI Quality Review id: review continue-on-error: true @@ -338,6 +346,7 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUM: ${{ steps.pr.outputs.pr_number }} REPOSITORY: ${{ github.repository }} + REVIEW_STARTED_AT: ${{ steps.t0.outputs.at }} run: | # `0` is a LEGITIMATE score, not a sentinel: the Stage 1 auto-reject # gates in prompts/workflow-prompts/ai-quality-review.md mandate @@ -353,16 +362,31 @@ jobs: SCORE=$(cat quality_score.txt | tr -d '[:space:]') HAS_OUTPUT=true else - # Fall back to the review comment. Select the last *claude[bot]* - # comment rather than the last comment overall — on a retry the - # github-actions[bot] "auto-retrying" notice lands after it, and - # `.comments[-1]` would then read that notice and find no score. - SCORE=$(gh api --paginate "repos/${REPOSITORY}/issues/${PR_NUM}/comments?per_page=100" \ - --jq '[.[] | select(.user.login == "claude[bot]")] | last | .body // ""' 2>/dev/null \ + # Fall back to the review comment, under two constraints: + # + # 1. Select the last *claude[bot]* comment, not the last comment + # overall — on a retry the github-actions[bot] "auto-retrying" + # notice lands after the review, and `.comments[-1]` would read + # that notice and find no score. + # 2. Only accept a comment posted by THIS run. A PR that has + # already been reviewed carries older review comments with a + # stale score (PR #9948 still shows `Score: 89/100` from its + # first review), and reading one of those would attach a score + # the current implementation never earned. Comparing RFC 3339 + # UTC timestamps lexicographically is exact here: GitHub and + # `date -u` emit the same fixed-width `...Z` format. + SCORE=$(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 // ""' \ | grep -oP '^###\s+Score:\s+\K\d+' | head -1 || true) if [ -n "$SCORE" ]; then HAS_OUTPUT=true - echo "::notice::quality_score.txt missing; recovered score ${SCORE} from the review comment" + echo "::notice::quality_score.txt missing; recovered score ${SCORE} from this run's review comment" + else + echo "::notice::quality_score.txt missing and no review comment from this run (since ${REVIEW_STARTED_AT})" fi fi diff --git a/CHANGELOG.md b/CHANGELOG.md index ad3be7f113..93078aff09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -152,11 +152,13 @@ aggregate instead: an italic *Catalog* line at the end of the version section an #10009, #10003, #9968, #9776), all with an `AR-09` verdict in hand. Output presence is now tracked as its own `has_output` step output, so a score of `0` flows into the normal rejected → repair path and only genuinely absent output raises `ai-review-failed`. The - comment fallback also now reads the last **`claude[bot]`** comment instead of the last comment - overall — on a retry the bot's own "auto-retrying" notice landed last and shadowed the score - it was looking for. Verified with a harness that extracts the step body verbatim from the YAML - and exercises 12 cases (score 0 via file and via comment fallback, normal scores, and the four - genuine no-output shapes) (#10179). + comment fallback also now reads the last **`claude[bot]`** comment posted by the *current* run, + instead of the last comment overall — on a retry the bot's own "auto-retrying" notice landed + last and shadowed the score, while an unscoped author filter would have reused a previous + attempt's score (#9948 still shows `Score: 89/100` from its first review). Verified with a + harness that extracts the step body verbatim from the YAML and exercises 17 cases (score 0 via + file and via comment fallback, stale-score reuse, normal scores, and the five genuine + no-output shapes) (#10179). - **CI lint went red on every PR after a ruff minor bump** — `pyproject.toml` pins `ruff>=0.15.21` without an upper bound, CI resolved **0.16.0**, and that version started formatting Python code blocks inside Markdown: 20 tracked `.md` files suddenly "would be