Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/workflows/impl-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
95 changes: 89 additions & 6 deletions .github/workflows/watchdog-stuck-jobs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +113 to +114

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and the gap was real: the counter was incremented and never read, so "counted and reported" only held for the inline per-failure warnings. Fixed in 5a8f0e4 — the scan's closing line now states the tally.

This matters beyond tidiness. Without it, a scan that dispatched nothing successfully reads exactly like a quiet, healthy one — and an always-green watchdog that silently rescues nothing is the precise failure mode this PR exists to fix.

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

Deliberately not exit 1 on a non-zero tally: aborting the scan is what took it down in the first place, and a red run would bury the summary behind a failed step.

Your comment also surfaced an adjacent risk worth pinning down rather than reasoning about. The counter only works if the increments happen in the current shell. The real scans are while read ... done < <(jq -c '.[]') — process substitution, so the body does run in the current shell — but had they been jq | while read pipelines, every increment would have landed in a subshell and the tally would have printed 0 forever, which is worse than no tally at all. Two cases now assert it against the real loop form:

--- the counter must survive the scan loop and be reported ---
PASS  counter survives while/process-substitution    3 failures reported
PASS  healthy scan reports success, not silence      ok

Suite is 25 cases, all passing.

}

ensure_label() {
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
34 changes: 34 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading