-
Notifications
You must be signed in to change notification settings - Fork 3k
ci(autofix): fan out review targets and stop route-scan starvation #7127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
28c3e23
6607548
8d391ca
13676f6
d26e2e0
1373512
a35245e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,9 @@ env: | |
| # Hard cap on automated review-address rounds per PR. After this the bot stops | ||
| # and leaves the PR for a human. | ||
| MAX_ROUNDS: '5' | ||
| # Upper bound on review targets emitted per scan (fan-out defense-in-depth; | ||
| # excess is logged and deferred to the next scan). | ||
| MAX_TARGETS_PER_SCAN: '10' | ||
| # Do not claim more issues when too many existing autofix PRs are still open. | ||
| MAX_OPEN_AUTOFIX_PRS: '5' | ||
|
|
||
|
|
@@ -90,8 +93,24 @@ jobs: | |
| runs-on: 'ubuntu-latest' | ||
| timeout-minutes: 5 | ||
| concurrency: | ||
| group: 'qwen-autofix-route' | ||
| cancel-in-progress: true | ||
| # Concurrency is keyed by TARGET, not shared and not fully unique: | ||
| # • cron ticks share one group (a newer tick supersedes a queued one) | ||
| # • review events coalesce PER PR (two reviews on the same PR seconds | ||
| # apart route once — the old shared group's one useful side effect, | ||
| # kept, without letting events on OTHER PRs cancel this one) | ||
| # • issue events coalesce PER issue | ||
| # • dispatches are unique per run and are never cancelled | ||
| # The old single shared cancel-in-progress group let ANY newer event kill | ||
| # pending full scans while route jobs sat queued behind runner backlog — | ||
| # observed as hours of scan starvation during review-event storms. | ||
| # Four cases: schedule → one shared cron group (newer tick supersedes); | ||
| # pull_request_review → per-PR; issues → per-issue (same-target events | ||
| # coalesce, unrelated targets never collide); anything else (dispatch) | ||
| # → unique per run_id, and cancel-in-progress false below means manual | ||
| # dispatches are never cancelled at all. | ||
| group: "${{ github.event_name == 'schedule' && 'qwen-autofix-route-cron' || (github.event_name == 'pull_request_review' && format('qwen-autofix-route-pr-{0}', github.event.pull_request.number)) || (github.event_name == 'issues' && format('qwen-autofix-route-issue-{0}', github.event.issue.number)) || format('qwen-autofix-route-{0}', github.run_id) }}" | ||
| cancel-in-progress: |- | ||
| ${{ github.event_name != 'workflow_dispatch' }} | ||
| permissions: | ||
| contents: 'read' | ||
| outputs: | ||
|
|
@@ -1004,8 +1023,46 @@ jobs: | |
| # check and double-process the feedback). | ||
| PENDING_STALE_MIN=240 | ||
| PENDING_CUTOFF="$(date -u -d "${PENDING_STALE_MIN} minutes ago" +%Y-%m-%dT%H:%M:%SZ)" | ||
|
|
||
| # PRs whose review-address is already RUNNING OR QUEUED in any live | ||
| # autofix run must not be re-targeted. Schedule/dispatch runs execute | ||
| # against main's SHA, so their matrix jobs never appear in the PR's | ||
| # statusCheckRollup — and a fanned-out matrix holds queued jobs well | ||
| # past a 10-minute tick, so without this the next scan re-emits the | ||
| # same PRs and the per-PR address groups accumulate duplicates that | ||
| # later replay stale watermarks. The status filter is SERVER-side: a | ||
| # client-side filter over the N newest runs loses a long-lived | ||
| # fanned-out run once cron traffic pushes it past the window, and | ||
| # its queued PRs silently stop looking busy. Filtered this way the | ||
| # limit applies to LIVE runs only (at most a handful), and one | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: server-side status filtering still truncates each live status to the newest 50 runs. Under a larger runner backlog, an older live matrix and its queued PR can become invisible, causing duplicate queue/build amplification. Paginate live runs or detect a full page and use a fallback that cannot declare omitted PRs idle. — Codex GPT-5 via Qwen Code /review |
||
| # jobs-view per live run stays cheap. | ||
| BUSY_PRS=' ' | ||
| while IFS= read -r LIVE_RUN; do | ||
| [[ -z "${LIVE_RUN}" ]] && continue | ||
| while IFS= read -r BUSY; do | ||
| [[ -n "${BUSY}" ]] && BUSY_PRS="${BUSY_PRS}${BUSY} " | ||
| done < <(gh run view "${LIVE_RUN}" --repo "${REPO}" --json jobs \ | ||
| --jq '.jobs[] | select(.status != "completed") | .name | capture("^review-address \\((?<pr>[0-9]+),") | .pr' 2> /dev/null) | ||
| done < <( | ||
| for LIVE_STATUS in in_progress queued; do | ||
| # || true: one status query failing must not hide the other. A | ||
| # DOUBLE failure yields an empty set — deliberately fail-open: | ||
| # this skip is an optimization, and the address-side live-marker | ||
| # revalidation is the correctness gate. If that revalidation is | ||
| # ever removed, this read must become fail-closed instead. | ||
| gh run list --repo "${REPO}" --workflow qwen-autofix.yml \ | ||
| --status "${LIVE_STATUS}" --limit 50 --json databaseId \ | ||
| --jq '.[].databaseId' 2> /dev/null || true | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: list/view failures are silently suppressed, so outage or rate-limit mode is indistinguishable from no live runs even though it can enqueue expensive duplicates. Keep the documented fail-open behavior, but record failed status/run queries and emit an explicit degraded-mode warning. — Codex GPT-5 via Qwen Code /review |
||
| done | sort -u | ||
| ) | ||
| [[ "${BUSY_PRS}" != ' ' ]] && echo "🚧 address in flight/queued for PR(s):${BUSY_PRS}" | ||
|
|
||
| TARGETS='[]' | ||
| for PR in ${CANDIDATES}; do | ||
| if [[ "${BUSY_PRS}" == *" ${PR} "* ]]; then | ||
| echo "⏳ #${PR}: review-address already in flight or queued — skipping" | ||
| continue | ||
| fi | ||
| # One PR fetch for the branch name, check rollup, and creation time (the | ||
| # watermark floor below) — avoids extra round-trips per candidate PR. | ||
| PR_META="$(gh pr view "${PR}" --repo "${REPO}" \ | ||
|
|
@@ -1152,7 +1209,21 @@ jobs: | |
| --arg round "${ROUND}" --arg wm "${EFF_WM}" \ | ||
| '. + [{pr: $pr, branch: $branch, issue: $issue, round: $round, watermark: $wm}]' \ | ||
| <<< "${TARGETS}")" | ||
| break # one PR per scheduled scan | ||
| # Fan out: emit EVERY eligible PR up to the per-scan budget. The | ||
| # address matrix bounds simultaneity (max-parallel) and the per-PR | ||
| # concurrency groups plus the busy-PR skip above prevent duplicate | ||
| # same-PR runs, so one scan drains the whole backlog instead of | ||
| # serving a single newest-first target per tick (which starved | ||
| # older PRs for hours when cron ticks were sparse). The budget | ||
| # break bounds this loop's RUNTIME and API usage too — each | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: this target-count guard does not bound candidate-loop work when fewer than ten PRs are actionable. Nine actionable plus ninety-one quiet candidates still perform all serial reads, so the claimed runtime/API bound is absent. Add a separate fair candidate-inspection budget or batching; otherwise narrow the comment to the matrix-size guarantee. — Codex GPT-5 via Qwen Code /review |
||
| # candidate costs several serial API reads, so scanning past a | ||
| # full budget would spend hundreds of calls for nothing. Never a | ||
| # silent cap: the deferral is logged and the next scan picks up | ||
| # the remainder (their signals persist). | ||
| if [[ "$(jq 'length' <<< "${TARGETS}")" -ge "${MAX_TARGETS_PER_SCAN}" ]]; then | ||
| echo "⚠️ target budget (${MAX_TARGETS_PER_SCAN}) reached; deferring the remaining candidates to the next scan" | ||
| break | ||
| fi | ||
| done | ||
|
|
||
| COUNT="$(jq 'length' <<< "${TARGETS}")" | ||
|
|
@@ -1342,6 +1413,54 @@ jobs: | |
| [[ -z "${NEWEST}" ]] && NEWEST="${WATERMARK}" | ||
| echo "newest=${NEWEST}" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| # Live-watermark revalidation: two near-simultaneous triggers for the | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Fan-out can leave later matrix targets queued behind long-running jobs, but address-time revalidation never checks whether the PR is still open and eligible. A PR closed while queued can still run the secret-bearing agent, push its retained branch, and receive a comment; a deleted branch can instead trigger a terminal handoff on the closed PR. Before checkout or setup, re-fetch and require open state, expected bot author, same-repo head, — Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in follow-up #7163: an eligibility recheck runs before the PR branch checkout — open state, bot author, in-repo head, base main, unchanged head branch, with a failed fetch treated as ineligible (fail closed) — and the discard path publishes every output the later gates read. Will resolve once #7163 merges. |
||
| # SAME PR can both pass their (per-target, route-level) gates and both | ||
| # scan before either has emitted a matrix job, so both emit this PR | ||
| # with the same stale watermark. The per-PR address concurrency group | ||
| # QUEUES the duplicate rather than discarding it — but that queueing | ||
| # is exactly what makes this check sound: address jobs for one PR run | ||
| # strictly one at a time, so by the time the duplicate runs here, the | ||
| # first job's eval marker is posted and visible. Two duplicate | ||
| # signatures: (a) a sibling evaluated through a NEWER live ts than | ||
| # our matrix watermark; (b) a conflict-only sibling resolved and | ||
| # marked at the SAME ts — with no newer feedback its marker keeps | ||
| # ts=watermark while its ROUND advances past ours (ours is the max | ||
| # round observed at scan time). Either way, if there is no live | ||
| # conflict left and nothing newer than the live watermark, this run | ||
| # is a stale duplicate and discards itself. | ||
| STALE='false' | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: queued duplicates are not discarded until after checkout, Node/tmux setup, — Codex GPT-5 via Qwen Code /review |
||
| LIVE_MARKS="$(jq -r --arg ab "${AUTOFIX_BOT}" ' | ||
| [ .[] | select((.user.login // "") == $ab) | (.body // "") | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: the eval-marker grammar is duplicated in the primary parser, this live parser, and the test fixture without mechanical coupling. A marker format change can leave the stale parser and its private fixture mutually green but disconnected from production writers. Define the grammar once and validate every emitted marker shape against it. — Codex GPT-5 via Qwen Code /review |
||
| | [ scan("<!-- autofix-eval ts=([^ ]+) acted=([^ ]+) round=([0-9]+) -->") ] | .[] ]' "${WORKDIR}/ic.json")" | ||
| LIVE_EVAL_WM="$(jq -r 'map(.[0]) | max // ""' <<< "${LIVE_MARKS}")" | ||
| LIVE_MAX_ROUND="$(jq -r 'map(.[2] | tonumber) | max // 0' <<< "${LIVE_MARKS}")" | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] A queued job may observe — Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| if [[ -n "${LIVE_EVAL_WM}" && "${CONFLICT}" != "true" ]] \ | ||
| && { [[ "${LIVE_EVAL_WM}" > "${WATERMARK}" ]] || [[ "${LIVE_MAX_ROUND}" -gt "${ROUND}" ]]; }; then | ||
| LIVE_NEW="$(jq -rs \ | ||
| --arg wm "${LIVE_EVAL_WM}" --arg rb "${REVIEW_BOT}" --arg ab "${AUTOFIX_BOT}" \ | ||
| --argjson trust "${TRUSTED_ASSOC}" ' | ||
| (.[0] | map(select((.submitted_at // "") > $wm) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Follow-up only: the actionable-feedback predicate is independently implemented in — Codex GPT-5 via Qwen Code /review |
||
| | select((.user.login // "") != $ab) | ||
| | select(((.author_association // "") | IN($trust[])) or (.user.login // "") == $rb) | ||
| | select((.state // "") | IN("CHANGES_REQUESTED", "COMMENTED"))) | length) | ||
| + (.[1] | map(select((.created_at // "") > $wm) | ||
| | select((.user.login // "") != $ab) | ||
| | select(((.author_association // "") | IN($trust[])) or (.user.login // "") == $rb)) | length) | ||
| + (.[2] | map(select((.created_at // "") > $wm) | ||
| | select((.user.login // "") != $ab) | ||
| | select(((.author_association // "") | IN($trust[])) or (.user.login // "") == $rb) | ||
| | select((.body // "") | test("<!-- (autofix-eval|qwen-triage|qwen-review-suggestion-summary|pr-force-push|qwen-review-ack) ") | not)) | length) | ||
| + (.[3] | map(select((.conclusion // .state // "") | IN("FAILURE", "FAILED", "ERROR", "TIMED_OUT", "ACTION_REQUIRED", "CANCELLED")) | ||
| | select(((.workflowName // "") != "Qwen Autofix") or (((.name // "") | startswith("review-address")))) | ||
| | select((.completedAt // .updatedAt // "") > $wm)) | length)' \ | ||
| "${WORKDIR}/rv.json" "${WORKDIR}/rc.json" "${WORKDIR}/ic.json" "${WORKDIR}/checks.json")" | ||
| if [[ "${LIVE_NEW}" == "0" ]]; then | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] When a queued duplicate finds newer feedback, it proceeds with the original matrix watermark. If the sibling handled F1 through T1 and F2 arrives after T1, the renderers still filter from W and send both F1 and F2 to the agent, replaying already-addressed work and potentially producing duplicate or contradictory changes. In the non-stale branch, advance the effective watermark to — Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| STALE='true' | ||
| echo "🫥 stale duplicate target: a sibling run already evaluated through ${LIVE_EVAL_WM} (round ${LIVE_MAX_ROUND}) and nothing is newer — discarding without action or marker" | ||
| fi | ||
| fi | ||
| echo "stale=${STALE}" >> "${GITHUB_OUTPUT}" | ||
|
|
||
| # Render the actionable feedback into one prompt-ready file. | ||
| { | ||
| ISSUE_REF="" | ||
|
|
@@ -1401,6 +1520,10 @@ jobs: | |
|
|
||
| - name: 'Triage and address' | ||
| id: 'address' | ||
| # Skipped entirely for a stale duplicate target (see the live-watermark | ||
| # revalidation in prepare) — no agent run, no marker, no comment. | ||
| if: |- | ||
| ${{ steps.prepare.outputs.stale != 'true' }} | ||
|
Comment on lines
+1524
to
+1526
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] The stale guard covers address and verification, but not the later failure reporter. If an always-run artifact step fails after — Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| # Bound the agent well below the 120-minute job timeout so a runaway agent | ||
| # fails THIS step (not the whole job), leaving the always() verify and | ||
| # report steps time to run and post a handoff. A job-level timeout would | ||
|
|
@@ -1467,7 +1590,7 @@ jobs: | |
| - name: 'Verification gate' | ||
| id: 'verify' | ||
| if: |- | ||
| ${{ always() }} | ||
| ${{ always() && steps.prepare.outputs.stale != 'true' }} | ||
| run: |- | ||
| if [[ -f "${WORKDIR}/failure.md" && -n "$(git status --porcelain)" ]]; then | ||
| echo "❌ Agent wrote failure.md after leaving a dirty workspace:" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical] This concurrency group is entered before the reviewer trust check. On a public PR, an untrusted submitted review can cancel or replace a trusted per-PR route; the replacement then fails authorization in
Decide phases, so the legitimate real-time scan is lost until an independent trigger recovers it. Authenticate in a prerequisite job before entering this target group, or ensure untrusted review payloads use an isolated run-specific group while retaining the live permission check.— Codex GPT-5 via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in follow-up #7163: the per-PR group is granted only when the review payload already looks trusted (
OWNER/MEMBER/COLLABORATORassociation, or the review bot); anything else gets a run-unique group — cancels nothing, still fully authorized insideDecide phases. Will resolve once #7163 merges.