diff --git a/.github/workflows/impl-review.yml b/.github/workflows/impl-review.yml index 18f2c83d63..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 @@ -337,23 +345,67 @@ jobs: env: 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 + # 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, 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 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 + + # 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 +463,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 +492,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 +532,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 +878,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..93078aff09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,27 @@ 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 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