diff --git a/.github/workflows/impl-review.yml b/.github/workflows/impl-review.yml index f35cfac228..43870ac3ec 100644 --- a/.github/workflows/impl-review.yml +++ b/.github/workflows/impl-review.yml @@ -468,6 +468,7 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUM: ${{ steps.pr.outputs.pr_number }} SCORE: ${{ steps.score.outputs.score }} + REPOSITORY: ${{ github.repository }} run: | LABEL="quality:${SCORE}" gh label create "$LABEL" --color "0e8a16" --description "Quality score ${SCORE}/100" 2>/dev/null || true @@ -482,6 +483,24 @@ jobs: gh pr edit "$PR_NUM" --remove-label "$STALE" 2>/dev/null || true fi + # Reaching this step means the review produced real output, so the + # previous failure streak is over. `ai-review-rescued` is the + # one-shot token both impl-review-retry.yml and watchdog Case 1 + # check before rescuing; nothing ever cleared it, so it meant "this + # PR was rescued once, ever" instead of "this failure streak was + # already rescued once". A PR that failed review twice at any point + # in its life was therefore permanently outside automation — 9 of + # the 11 PRs stuck on 2026-08-05 carried this pair. Clearing both + # labels on a successful review restores the intended semantics. + # Deleted via the REST API: `gh pr edit --remove-label` fails on + # this repo with a GraphQL "Projects (classic) is being deprecated" + # error on repository.pullRequest.projectCards. + for OLD in ai-review-failed ai-review-rescued; do + if gh api -X DELETE "repos/${REPOSITORY}/issues/${PR_NUM}/labels/${OLD}" >/dev/null 2>&1; then + echo "::notice::Cleared ${OLD} — review succeeded, failure streak reset" + fi + done + # Retry on transient GitHub API failures (e.g. 504) — without this, # a single 5xx leaves the PR stuck with no quality label and the # downstream verdict step is gated on this step's success. diff --git a/.github/workflows/watchdog-stuck-jobs.yml b/.github/workflows/watchdog-stuck-jobs.yml index 75ecdb5372..558c398904 100644 --- a/.github/workflows/watchdog-stuck-jobs.yml +++ b/.github/workflows/watchdog-stuck-jobs.yml @@ -64,14 +64,54 @@ jobs: STALE_SEC=$(( STALE_HOURS * 3600 )) NOW=$(date -u +%s) + DISPATCH_FAILURES=0 + + # A failed dispatch must never abort the scan. This function used to + # call `gh workflow run` bare under `set -e`, so a single unroutable + # dispatch killed the whole run and every PR after it went unscanned + # — the safety net failing silently, in the one place where nothing + # else is watching. That is what happened on 2026-08-02..04: five of + # fourteen scheduled scans died on HTTP 422 "Cannot trigger a + # 'workflow_dispatch' on a disabled workflow" because daily-regen.yml + # was disabled manually, while the log line above the failure had + # already announced "→ dispatching". The announcement now follows the + # attempt instead of preceding it, so the log cannot claim a dispatch + # that did not happen. dispatch() { local label="$1"; shift if [[ "$DRY_RUN" == "true" ]]; then echo "::notice::[dry-run] $label → gh workflow run $*" + return 0 + fi + + local err + if err=$(gh workflow run "$@" 2>&1); then + echo "::notice::$label → dispatched" + return 0 + fi + + # Truncate with parameter expansion, never a pipeline. The obvious + # `printf '%s' "$err" | head -c 300 | tr '\n' ' '` reintroduces the + # very bug this function exists to fix: once the message exceeds + # the pipe buffer, `head` exits after its 300 bytes, `printf` takes + # SIGPIPE and returns 141, `pipefail` propagates that to the + # assignment, and `set -e` kills the scan — in the error path, so + # only a pathological message would ever expose it. Measured on + # this runner: 60 000 bytes survives, 70 000 exits 141. Parameter + # expansion forks nothing and cannot fail. `::warning::` is + # line-oriented, so newlines must go or everything after the first + # one leaks into the raw log. + err=${err//$'\n'/ } + err=${err:0:300} + if [[ "$err" == *"disabled workflow"* ]]; then + # Not transient and not something a retry fixes — a human turned + # the workflow off. Report it plainly and keep scanning. + echo "::warning::${label} → SKIPPED, target workflow is disabled: ${err}" else - echo "::notice::$label → dispatching" - gh workflow run "$@" + echo "::warning::${label} → dispatch FAILED: ${err}" fi + DISPATCH_FAILURES=$(( DISPATCH_FAILURES + 1 )) + return 0 } ensure_label() { @@ -125,11 +165,23 @@ jobs: fi # Case 2: stalled repair handoff - # has ai-attempt-N + quality:M, no ai-approved/ai-rejected, + # has ai-attempt-N + quality:M, not ai-approved, # PR untouched for stale_hours → re-dispatch impl-repair + # + # `ai-rejected` used to be excluded here, which left + # `ai-rejected` + `ai-attempt-N` matching NO case at all: Case 2 + # rejected it for having a verdict, Case 4 for having an attempt + # label. That combination is the exact state impl-review.yml + # leaves behind when its impl-repair dispatch fails — its own + # comment says so and tries to drop `ai-rejected` to escape into + # this case, which only works when that removal also succeeds. + # PR #9949 sat in it for ten days. Excluding only `ai-approved` + # (Case 3's business) closes the hole; the age guard keeps a + # normal in-flight repair from being re-dispatched, and the + # `watchdog:repair-rescued-N` marker keeps it one-shot. if echo " $labels " | grep -qE " ai-attempt-[0-9]+ " \ && echo " $labels " | grep -qE " quality:[0-9]+ " \ - && ! echo " $labels " | grep -qE " (ai-approved|ai-rejected) " \ + && ! echo " $labels " | grep -qE " ai-approved " \ && (( age > STALE_SEC )); then attempt=$(echo " $labels " | grep -oP "ai-attempt-\K[0-9]+" | sort -nr | head -1) marker="watchdog:repair-rescued-$attempt" @@ -310,9 +362,29 @@ jobs: # immediately after dispatch and would spill into it). Like the # cron, the window is UTC-fixed: it matches Berlin evening under # CEST and sits an hour earlier in local terms under CET. + # + # A manually disabled daily-regen is an INTENTIONAL operator state, + # not a starved schedule: the maintainer switches it off and on to + # manage the monthly token budget. Reviving it would override that + # decision, and "starved, re-dispatching" is simply a false report. + # Without this check the rescue also cannot succeed — a disabled + # workflow rejects workflow_dispatch with HTTP 422 — so every scan + # would log a warning and burn a dispatch failure for as long as the + # maintainer keeps it off, which is exactly the noise that hid the + # real breakage before. `disabled_inactivity` is GitHub switching + # schedules off after 60 days of repo inactivity; that one is worth + # flagging loudly, but it is equally undispatchable, so both states + # skip the rescue and only the message differs. LIVENESS_HOURS=10 HOUR=$(date -u +%-H) - if (( HOUR >= 17 && HOUR <= 21 )); then + REGEN_STATE=$(gh api "repos/${GITHUB_REPOSITORY}/actions/workflows/daily-regen.yml" \ + --jq '.state' 2>/dev/null || echo "unknown") + + if [[ "$REGEN_STATE" == "disabled_manually" ]]; then + echo "::notice::daily-regen liveness: workflow is disabled manually — intentional, rescue skipped" + elif [[ "$REGEN_STATE" == disabled_* ]]; then + echo "::warning::daily-regen liveness: workflow state is '${REGEN_STATE}' — cannot be dispatched, rescue skipped" + elif (( HOUR >= 17 && HOUR <= 21 )); then echo "::notice::daily-regen liveness: inside/adjacent to quiet window (UTC hour $HOUR) — check skipped" else # --branch main: schedule runs (and rescue dispatches) live on the @@ -339,4 +411,15 @@ jobs: fi fi - echo "::notice::Watchdog scan complete" + # Surface the tally. Individual failures are warned about inline, but + # a scan that dispatched nothing successfully now looks identical to + # a quiet, healthy one unless the total is stated — and an + # always-green watchdog that quietly rescues nothing is the failure + # mode this whole PR is about. Deliberately not `exit 1`: aborting is + # what took the scan down in the first place, and a red run would + # hide the summary behind a failed step. + if (( DISPATCH_FAILURES > 0 )); then + echo "::warning::Watchdog scan complete — ${DISPATCH_FAILURES} dispatch(es) failed (see warnings above)" + else + echo "::notice::Watchdog scan complete — all dispatches succeeded" + fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 93078aff09..1ce39d5a4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,40 @@ aggregate instead: an italic *Catalog* line at the end of the version section an ### Fixed +- **The pipeline's safety net had three holes, and one of them was armed** — follow-up to #10179, + which stopped PRs *entering* the dead-end; this stops them *staying* there. + (1) `watchdog-stuck-jobs.yml` called `gh workflow run` bare under `set -euo pipefail`, so a + single unroutable dispatch aborted the entire scan and every PR after it went unscanned — the + safety net failing silently, where nothing else is watching. Five of fourteen scheduled scans + died this way on 2026-08-02..04 with HTTP 422 `Cannot trigger a 'workflow_dispatch' on a + disabled workflow`, because `daily-regen.yml` is disabled manually while the watchdog's + cron-liveness rescue keeps trying to revive it — and the log announced `→ dispatching` *before* + attempting, so the run reported rescues it never performed. Dispatch failures are now counted + and reported, never fatal, with a disabled target called out as the non-transient case it is; + the log line now follows the attempt. This was live: `daily-regen` last ran 16:51 UTC against a + 10 h liveness threshold, so the next scan after 02:51 UTC would have died again. + (2) `ai-rejected` + `ai-attempt-N` matched **no** watchdog case — Case 2 excluded any verdict + label, Case 4 excluded any attempt label — which is exactly the state `impl-review.yml` leaves + behind when its `impl-repair` dispatch fails, as its own comment says. PR #9949 sat in it for + ten days. Case 2 now excludes only `ai-approved` (Case 3's business); the existing age guard + and `watchdog:repair-rescued-N` marker keep it from racing an in-flight repair. + (3) `ai-review-rescued` was written once and cleared by nothing, so it meant "this PR was ever + rescued" rather than "this failure streak was already rescued" — any PR that failed review + twice in its life was permanently outside automation, which described 9 of the 11 PRs stuck on + 2026-08-05. A successful review now clears it along with `ai-review-failed`. + (4) The cron-liveness rescue treated a manually disabled `daily-regen` as a starved schedule. + It is not: the maintainer switches that workflow off and on to manage the monthly token budget, + so reviving it would override a deliberate decision, and "starved, re-dispatching" was simply a + false report — one that could never succeed anyway, since a disabled workflow rejects + `workflow_dispatch`. Section C now reads the workflow state first and skips the rescue for any + disabled state, quietly for `disabled_manually` and loudly for GitHub's 60-day + `disabled_inactivity`. Verified with harnesses that extract `dispatch()` verbatim from the YAML + and replicate the Case 2 and Section C guards exactly: 25 cases covering a disabled target, + transient failure and success, a 100 KiB error message, counter propagation through the real + `while ... done < <(...)` scan loop, the six Case 2 label shapes, and the full workflow-state × + quiet-window × gap matrix — each regression case checked to actually fail against the + pre-fix code (#10180). + - **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