ci(autofix): fan out review targets and stop route-scan starvation - #7127
Merged
Conversation
Two throughput fixes for the review loop, both observed live:
- review-scan emitted ONE newest-first target per scan ("single-target
worker"). With sparse cron ticks this starves older armed PRs for hours —
an armed PR sat unprocessed for 16h while newer PRs took every tick. Emit
EVERY eligible target instead: the address matrix's max-parallel (3) bounds
simultaneity and the per-PR concurrency groups already prevent duplicate
same-PR runs, so one surviving scan drains the whole backlog.
- route used a single shared concurrency group with cancel-in-progress. Under
runner backlog a route job sits QUEUED for minutes, and any newer event
(review submissions arrive constantly) cancelled it — five consecutive
dispatched scans died this way; during event storms no full scan survived
at all. Cron ticks keep deduping through a shared 'route-cron' group, but
dispatches and review/issue events now get unique per-run groups: route is
a seconds-long job, so never cancelling it costs nothing and every trigger
is guaranteed to route.
Contract test updated: fan-out asserted (no single-target break, matrix
max-parallel), new route concurrency expression pinned. 50/50.
Collaborator
|
Thanks for the PR, @wenshao! Template looks good ✓ — all required sections present, bilingual, with before/after evidence. Problem: observed and well-documented. The 16-hour starvation of an armed PR and five consecutive route cancellations are concrete, live-observed failures — not theoretical hardening. The before/after evidence in the PR body makes the impact clear. Direction: aligned. The autofix review loop is project infrastructure that directly affects PR turnaround time. Making it drain its backlog in one scan instead of serially over hours, and preventing route cancellation during event storms, are clear wins. Size: not applicable — changes are confined to Approach: the scope is tight and each piece earns its place. The fan-out (remove single-target break, add budget cap) is the core fix. The route concurrency re-keying (cron coalesces, dispatches never cancelled, per-PR/issue events coalesce independently) directly addresses the observed cancellation storms. The live-watermark revalidation in review-address is a necessary correctness guard for the fan-out case where two near-simultaneous scans emit the same PR. Busy-PR detection prevents re-emitting PRs whose matrix jobs are still queued. Two files, focused diff, no drive-by changes. Moving on to code review and testing. 🔍 中文说明感谢贡献，@wenshao！ 模板完整 ✓ — 所有必需章节齐全，双语，附有 before/after 证据。 问题： 已观测且有据可查。已武装 PR 的 16 小时饥饿和连续五次 route 取消是真实的线上故障——不是理论性加固。PR 正文中的 before/after 证据清楚地展示了影响。 方向： 对齐。autofix review 回路是影响 PR 周转时间的项目基础设施。让它在一次扫描中清空积压而非串行数小时，并防止事件风暴期间的 route 取消，都是明确的改进。 规模： 不适用 — 改动限于 方案： 范围紧凑，每个部分都物有所值。fan-out（移除单目标 break，添加预算上限）是核心修复。route 并发键重设（cron 合并、dispatch 永不取消、per-PR/issue 事件独立合并）直接针对观测到的取消风暴。review-address 中的 live-watermark 再验证是 fan-out 场景下的必要正确性保障——两次近乎同时的扫描可能发出同一个 PR。busy-PR 检测防止重新发出 matrix job 仍在排队中的 PR。两个文件，聚焦 diff，无顺手改动。 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code reviewThis is a CI workflow change — no user-visible CLI behavior is affected, so tmux testing doesn't apply. The contract tests and static validation are the right verification here. Independent baseline: before reading the diff, I'd solve these two starvation problems by (1) removing the single-target break and adding a budget cap, (2) re-keying route concurrency to separate cron from other triggers, and (3) adding some form of dedup guard for the fan-out race. The PR matches this and goes further in well-chosen ways. What the implementation adds beyond the minimum: The route concurrency key is 4-way rather than 2-way: cron coalesces with itself, review events coalesce per-PR (two reviews seconds apart route once), issue events per-issue, and dispatches are unique and never cancelled. This is more granular than just "cron vs. everything else" and directly preserves the one useful behavior of the old shared group (same-target coalescing) while eliminating the harmful cross-target cancellation. The busy-PR detection queries live autofix runs server-side (in_progress ∪ queued) and extracts PR numbers from in-flight matrix jobs. This prevents re-emitting PRs whose address jobs are still queued — important because a fanned-out matrix holds queued jobs well past the next 10-minute tick. The fail-open design (API failure → proceed anyway) is correct given that the address-side live-watermark revalidation is the actual correctness gate. The live-watermark revalidation catches the race where two near-simultaneous scans both emit the same PR before either's matrix job starts. The stale-duplicate detection handles two signatures: (a) a sibling evaluated through a newer watermark, (b) a conflict-only sibling at the same watermark whose round advanced. The behavioral replay test for case (b) is particularly thorough — it exercises the exact transition where the conflict clears but the round number reveals the duplicate. No blockers found. No correctness bugs, security issues, or AGENTS.md violations. Comments are extensive but all explain why for non-obvious decisions — appropriate for a GitHub Actions workflow where the execution model (concurrency groups, matrix queuing, cancel semantics) is subtle and the cost of misunderstanding is hours of silent starvation. TestingStatic validation:
中文说明代码审查这是 CI 工作流改动 — 不影响用户可见的 CLI 行为，因此 tmux 测试不适用。契约测试和静态验证是正确的验证方式。 独立基线： 在阅读 diff 之前，我会通过以下方式解决这两个饥饿问题：(1) 移除单目标 break 并添加预算上限，(2) 重设 route 并发键以区分 cron 和其他触发器，(3) 为 fan-out 竞态添加某种去重守卫。PR 匹配了这些方案并在恰当的地方更进一步。 实现在最低要求之外的补充： route 并发键是 4 路而非 2 路：cron 自我合并、review 事件按 PR 合并（相隔数秒的两个 review 只路由一次）、issue 事件按 issue 合并、dispatch 唯一且永不取消。这比简单的 "cron vs. 其他" 更精细，直接保留了旧共享组的唯一有用行为（同目标合并），同时消除了有害的跨目标取消。 busy-PR 检测在服务端查询活跃的 autofix 运行（in_progress ∪ queued），并从在途的 matrix job 中提取 PR 号。这防止了重新发出地址 job 仍在排队中的 PR — 很重要，因为扇出的 matrix 会在下一个 10 分钟 tick 之后仍持有排队中的 job。fail-open 设计（API 失败 → 照常继续）是正确的，因为地址侧的 live-watermark 再验证才是真正的正确性保障。 live-watermark 再验证捕获了两次近乎同时的扫描在各自 matrix job 启动前都发出同一个 PR 的竞态。stale 重复检测处理两种签名：(a) 兄弟运行已评估到更新的 watermark，(b) 同一 watermark 的仅冲突兄弟其 round 已推进。针对情况 (b) 的行为重放测试特别彻底 — 它演练了冲突清除但 round 号揭示重复的确切转换。 未发现阻塞问题。 无正确性 bug、安全问题或 AGENTS.md 违规。注释较多，但都是解释非显而易见决策的 why — 对于 GitHub Actions 工作流来说是恰当的，因为其执行模型（并发组、matrix 排队、取消语义）微妙，误解的代价是数小时的静默饥饿。 测试静态验证：
— Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. Gate (Stage 1): Template complete, problem observed with live evidence, direction aligned, scope tight. No concerns. Review (Stage 2): 51/51 contract tests pass, YAML and bash syntax validated, no correctness bugs or security issues. The implementation's defensive layers (4-way concurrency key, busy-PR detection with fail-open, live-watermark revalidation with conflict-only duplicate handling) are well-engineered and well-tested. Summary: Two real, observed starvation problems in the autofix review loop — single-target selection starving older PRs, and shared route concurrency cancelling scans during event storms. The fix fans out all eligible PRs per scan (bounded by 中文说明信心度: 5/5 — 各阶段均无问题，毫不犹豫可合并。 门禁 (Stage 1)： 模板完整，问题有线上证据，方向对齐，范围紧凑。无顾虑。 审查 (Stage 2)： 51/51 契约测试通过，YAML 和 bash 语法验证通过，无正确性 bug 或安全问题。实现的防御层（4 路并发键、fail-open 的 busy-PR 检测、含仅冲突重复处理的 live-watermark 再验证）设计精良且测试充分。 总结： autofix review 回路中两个真实的、已观测到的饥饿问题 — 单目标选择令旧 PR 挨饿，共享 route 并发在事件风暴中取消扫描。修复方案每次扫描扇出所有合格 PR（受 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 17, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
Author
Code Review —
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
previously approved these changes
Jul 17, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
yiliang114
reviewed
Jul 18, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the workflow and test changes. The concurrency group separation (cron vs dispatch/review events) is solid and the rationale is clear.
One concern — route job timeout-minutes: 5 may be too short for fan-out:
Before this change, the scan loop breaks after the first eligible PR — so the route job only processes 1 candidate per tick. After removing break, it scans all eligible PRs in the backlog. For a repo with many open PRs, evaluating each one (checking isCrossRepository, statusCheckRollup, pending checks, etc.) involves multiple gh api calls per PR. If the backlog grows large, 5 minutes may not be enough.
Suggestion: Either bump timeout-minutes to 10-15, or add a cap on the number of eligible PRs processed per scan (e.g. | head -20 after the eligibility filter) to bound the route job's runtime.
The max-parallel: 3 on the address matrix and per-PR concurrency groups look correct for preventing duplicate runs. Tests are properly updated.
Review note on the fan-out: bound the scan's output for a pathological backlog. Clarifications recorded in-thread — the loop lives in review-scan (timeout 15m), not route (5m), and the pre-change worst case already walked the full candidate list (break fired on the first ELIGIBLE PR, not the first candidate) — but an explicit bound is good hygiene: emit at most MAX_TARGETS_PER_SCAN (10) targets, LOG the deferral (never a silent cap), and let the next scan pick up the remainder since their signals persist. Contract test pins the cap, the deferral log, and the slice.
Collaborator
Author
|
Thanks — applied the bound in
That said, an explicit bound is good hygiene, so: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
doudouOUC
requested changes
Jul 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the exact head 66075483. The 50 workflow contract tests pass, but the new fan-out/concurrency behavior can enqueue duplicate address jobs and the cap does not bound the newly expanded scan cost, so I am requesting changes.
…ip, in-loop budget Both criticals and the suggestion from review, each verified against live campaign observations: - Route concurrency is now keyed by TARGET: cron ticks still coalesce with each other; review events coalesce PER PR (near-simultaneous reviews on one PR route once — the one useful side effect of the old shared group, restored — without events on other PRs cancelling this one); issue events coalesce per issue; dispatches stay unique and are never cancelled. This keeps the starvation fix while closing the duplicate-forced-scan window the per-run_id grouping had opened. - The scan now skips any PR whose review-address job is RUNNING OR QUEUED in a live autofix run (one runs-list plus a jobs-view per live run). A fanned-out matrix holds queued jobs past a 10-minute tick and schedule/dispatch runs never surface in the PR's checks, so without this the next scan re-emitted the same PRs and per-PR groups accumulated duplicates that later replayed stale watermarks — the exact duplicate-round behavior observed live on the fleet. - The per-scan target budget now BREAKS the candidate loop instead of slicing after it, so it genuinely bounds scan runtime and API usage (each candidate costs several serial reads); the deferral is logged and the remainder keeps its signals for the next scan. Contract test updated for all three (route expression per target, busy-skip message + capture regex, in-loop budget break). 50/50.
Collaborator
Author
|
All three addressed in
Contract test pins all three (per-target route expression, busy-skip + capture regex, in-loop break). 50/50, YAML/ |
qwen-code-ci-bot
previously approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
doudouOUC
requested changes
Jul 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the exact head 8d391ca. The 50 workflow contract tests pass, and the prior queued-job visibility and in-loop budget findings are addressed. However, the per-target route key plus the live-run busy set still leaves a blocking scan-selection race before a matrix job exists, so I am requesting changes.
…idation Review: the busy-set closes the queued-matrix window but not the pre-matrix one — two near-simultaneous same-PR triggers can both scan before either has emitted a matrix job, so both emit the PR with the same stale watermark, and the per-PR address group QUEUES (not discards) the duplicate. That queueing is exactly what makes revalidation sound: address jobs for one PR run strictly one at a time, so when the duplicate reaches prepare, the first job's eval marker is already posted. Prepare now recomputes the watermark from LIVE markers; if it advanced past the matrix watermark and nothing (reviews / inline / issue comments / failed checks) is newer — and there is no conflict — the run marks itself stale and the address + verify steps are skipped entirely: no agent run, no marker, no comment, no push. Contract test pins the revalidation, both step gates, and the now-three shared address-carve-out sites. 50/50.
A client-side status filter over the 15 newest runs loses a long-lived fanned-out run once cron traffic (~6 runs/hr) pushes it past the window — its queued review-address PRs silently stop looking busy and the next scan re-emits them with a stale watermark. Query in_progress and queued server-side instead, so the limit applies to LIVE runs only (at most a handful) and the window cannot be starved by completed runs. One status query failing does not hide the other (|| true per query); an empty set stays fail-open by design — the address-side live-marker revalidation is the second line of defense.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
yiliang114
reviewed
Jul 18, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
The fan-out + per-target route coalescing is well-reasoned — both starvation modes (single-target selection and shared-group cancellation) are addressed with the right bounds (max-parallel, per-PR groups, budget break). The live-watermark revalidation in prepare is the correct second line of defense for the pre-matrix race.
One non-blocking observation on the busy-set listing:
for LIVE_STATUS in in_progress queued; do
gh run list ... --status "${LIVE_STATUS}" --limit 50 ... || true
done | sort -uThe || true per status query means a double-failure (both in_progress AND queued reads fail) produces an empty busy-set — fail-open. The PR body acknowledges this and the address-side revalidation catches it, so it's defense-in-depth working as designed. Just noting that if the revalidation is ever relaxed or removed, this fail-open path becomes the primary gate. The current layering is correct.
The route concurrency expression is a chained ternary that's getting long — might be worth a one-line comment above it summarizing the four cases for the next reader who hits it in a blame. Non-blocking.
Review notes (non-blocking, adopted): a four-case summary above the chained route-concurrency ternary for the next reader hitting it in a blame, and an explicit contract at the busy-set listing — a double status-query failure is deliberately fail-open because the 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.
Collaborator
Author
|
Re: review 4728239895 — both notes adopted in 1373512:
|
qwen-code-ci-bot
approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
previously approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
doudouOUC
requested changes
Jul 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the exact head 1373512. The scan-side busy-set and address-side revalidation fix the ordinary feedback duplicate, and all 50 workflow contract tests pass. The timestamp-only revalidation still misses a conflict-only duplicate, so I am requesting changes.
yiliang114
previously approved these changes
Jul 18, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
LGTM. Solid fix for the scan starvation problem.
What's good
- Per-target concurrency keying — cron ticks coalesce (newer supersedes), review events coalesce per-PR, issue events per-issue, dispatches are unique and never cancelled. The nested ternary is long but well-commented and each case is tested.
- Fan-out with budget — removes the single-target
break, emits every eligible PR, caps atMAX_TARGETS_PER_SCANwith the break on the LOOP (bounding API usage, not just matrix size), and logs the deferral. - Busy-PR skip with server-side status filter —
for LIVE_STATUS in in_progress queuedwith--statusavoids the client-side window problem where a long-lived fanned-out run gets pushed past the--limit. - Live-watermark revalidation — handles the race where two near-simultaneous scans both emit the same PR before either produces a matrix job. The per-PR address group serializes them, so the later one sees the first's eval marker and discards itself. The
staleoutput correctly gates both the address step AND the verify step. - Test coverage — behavioral tests for the concurrency expression, fan-out budget, busy-set, and stale detection all pin the real patterns.
— Qwen Code review
…e stale gate
Review round: the ts-only revalidation missed a conflict-only
duplicate. Two overlapping scans emit the same conflicted PR with
watermark W; the first serialized job resolves the conflict and — with
no newer feedback — its marker keeps ts=W while its round advances.
The second job then sees CONFLICT=false live but LIVE_EVAL_WM == W, so
the strict > gate never fired and the agent re-ran against resolved
work. The gate now also extracts LIVE_MAX_ROUND and treats
same-ts-with-newer-round (conflict cleared) as a duplicate signature,
still subject to the nothing-newer recount.
The behavioral replay the reviewer asked for immediately caught a
latent bug in the previous fix: the recount jq opened with '((' and
never closed it, so it failed to compile, LIVE_NEW stayed empty, and
the whole stale gate was dead code in production. Fixed to a single
paren; the replay now proves five transitions (conflict-only duplicate
discards, first conflict job proceeds, live conflict always proceeds,
ts-advanced duplicate discards, round-advanced-with-new-feedback
proceeds).
wenshao
dismissed stale reviews from yiliang114 and qwen-code-ci-bot
via
July 18, 2026 11:04
a35245e
doudouOUC
approved these changes
Jul 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed the exact head a35245e6.
My earlier CHANGES_REQUESTED was about the pre-matrix scan-selection race — two near-simultaneous scans emitting the same PR before either has a matrix job. That's now closed by the live-watermark revalidation in prepare: the per-PR address group (cancel-in-progress: false) serializes the duplicates, so the later job reliably sees the first job's eval marker and self-discards. I confirmed the discard path posts nothing — address and verify are stale-gated, so OUTCOME is empty and both report steps no-op (no agent run, no marker, no comment).
I also checked the interim a35245e6 fix closely, since the stale gate is not exercised by the string-matching contract tests:
- Dead recount fix — confirmed real. The prior
LIVE_NEWexpression had an unbalanced leading(and failedjqcompilation; at runtime the recount would have crashed the prepare step (underset -eo pipefail) and posted a false failure handoff instead of silently discarding a stale duplicate. The rewritten sum compiles and returns the correct count (verified against sample review / comment / check inputs). - Conflict-only dedup — correct. Splitting the marker scan into
LIVE_EVAL_WM(max ts) +LIVE_MAX_ROUND(max round) and gating onts > watermark OR round > our roundcorrectly catches the same-ts / advanced-round duplicate a conflict resolution leaves behind, while a live conflict, or trusted feedback after the watermark, still proceeds. - The new behavioral test is a genuine guard. It extracts the stale gate verbatim and replays it over fixtures. I ran it against the pre-fix workflow — it fails (
expected false to be true, the crashed jq leftstale=false) — and it passes on this head. 51/51.
Route concurrency (per-target keys), fan-out + max-parallel: 3, and the busy-set skip all still look right. YAML parses; the concurrency expressions evaluate correctly for all four triggers (schedule / pull_request_review / issues / workflow_dispatch).
Two non-blocking notes:
- The PR description still says "two coordinated changes" and that "the stale-snapshot duplicate-round race across runs is pre-existing and unchanged" — but the PR now adds the busy-set skip and the live-watermark revalidation specifically for that race. Worth updating the body so it matches what shipped.
- The busy-set PR-skip relies on the matrix job-name format
review-address (<pr>, …). It has not run on a live multi-target tick yet (the loop has been starved), and it is backstopped by the now-working stale gate, so a naming miss only costs a redundant self-discarding job. Worth eyeballing the first post-merge fan-out tick to confirm the skip fires.
LGTM — approving.
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
doudouOUC
approved these changes
Jul 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex GPT-5 via Qwen Code /review
| # 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) }}" |
Collaborator
Author
There was a problem hiding this comment.
[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
Collaborator
Author
There was a problem hiding this comment.
| | 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.
[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 LIVE_EVAL_WM before rendering, and add a W/T1/T2 replay test that asserts only F2 is rendered.
— Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Comment on lines
+1524
to
+1526
| # revalidation in prepare) — no agent run, no marker, no comment. | ||
| if: |- | ||
| ${{ steps.prepare.outputs.stale != 'true' }} |
Collaborator
Author
There was a problem hiding this comment.
[Critical] The stale guard covers address and verification, but not the later failure reporter. If an always-run artifact step fails after stale=true, the empty verification outcome lets the reporter post a handoff and eval marker, consuming a round despite the stated no-comment/no-marker contract. Add the same stale guard to the failure-report step or POST_HANDOFF decision, and test an artifact failure after a stale skip.
— Codex GPT-5 via Qwen Code /review
Collaborator
Author
| [ .[] | select((.user.login // "") == $ab) | (.body // "") | ||
| | [ 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.
[Critical] A queued job may observe LIVE_MAX_ROUND=5 yet continue because newer feedback exists while its matrix ROUND remains 4. A successful run then computes 4+1 again, performs a sixth automated fix, and writes another round-5 marker, bypassing and concealing the hard cap. Stop immediately when the live round is already capped; otherwise carry the live round forward as the effective round for all later checks and marker writes.
— Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
| [[ -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.
[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, main base, and the snapshotted branch, then suppress all side effects for an inactive target.
— Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
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.
Comment on lines
+330
to
+331
| writeFileSync(join(dir, 'rc.json'), '[]'); | ||
| writeFileSync(join(dir, 'checks.json'), '[]'); |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] Follow-up only: the stale-gate replay varies reviews, but keeps inline comments and checks empty and uses issue comments only for eval markers. Regressions in the three positive LIVE_NEW branches could discard real feedback while all cases pass. Add trusted inline-comment, actionable issue-comment, marker-exclusion, and failed-check fixtures.
— Codex GPT-5 via Qwen Code /review
| # 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.
[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
| # is a stale duplicate and discards itself. | ||
| STALE='false' | ||
| LIVE_MARKS="$(jq -r --arg ab "${AUTOFIX_BOT}" ' | ||
| [ .[] | select((.user.login // "") == $ab) | (.body // "") |
Collaborator
Author
There was a problem hiding this comment.
[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
| # 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.
[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
| # 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.
[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
This was referenced Jul 18, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jul 19, 2026
…ted route events (#7163) * ci(autofix): harden the address path against stale targets and untrusted route events Follow-up to #7127 addressing the five Critical findings of its post-merge review (4728499913): - Route group trust prefilter: the per-PR concurrency group is entered before any step runs, so an arbitrary commenter's review could cancel a queued legitimate route and then die in Decide phases. Reviews whose payload does not already look trusted (repo association or the review bot) now get a run-unique group — cancel nothing, still fully authorized inside. Decide phases remains the real permission gate. - Address-time eligibility recheck: a matrix job can start hours after its scan; a PR closed/merged meanwhile (or with changed author/repo/base/branch) is discarded BEFORE the PR branch checkout — no secret-bearing agent run, no push, no comment, no marker. A failed fetch discards too (unknown is not eligible). - Non-stale duplicates adopt the live watermark and round: a sibling may have evaluated F1 through T1 while this job carried watermark W; rendering from W would replay handled feedback to the agent, and reusing the matrix round would double-write a marker round. Both reporters consume the effective round. - Live round cap: when live markers already sit at MAX_ROUNDS the run discards (the scan itself skips capped PRs before conflict checks) — no round MAX+1 work, no second capped marker. - Stale discard suppresses the failure-path handoff: a late always() step failure after stale=true no longer converts a deliberate no-comment/no-marker discard into a handoff that consumes a round. Contract tests 53/53: W/T1/T2 replay proving only-F2 rendering via the adopted watermark, live-round adoption and cap-discard replays, sentinel-ts non-adoption, stale-suppressed handoff replay, and eligibility ordering/coverage pins. * ci(autofix): honor engagement labels in the address-time eligibility gate Forward-compatible with the incoming label-takeover feature so the two changes commute in any merge order: the eligibility recheck now reads labels live — autofix/takeover exempts a human-authored PR from the bot-author requirement, and autofix/skip (which wins over takeover) withdraws consent even if applied while the job sat queued. Both are enforced at the moment the secret-bearing run starts, not at scan time. Inert until the labels exist. * test(autofix): behaviorally replay the eligibility recheck Review suggestion, adopted: the discard path was only string-pinned — a future edit dropping the stale=true echo would leave every toContain green while STALE arrived empty downstream, letting a late always() failure post a spurious handoff for a discarded job. The recheck now runs VERBATIM under a PATH-stubbed gh across nine states: healthy bot PR proceeds writing nothing; closed-while-queued discards AND writes all four outputs later gates read; live takeover label exempts a human author; live skip label withdraws consent even for the bot's own PR; fork head, renamed branch, and a failed fetch (unknown is not eligible) all discard. 54/54. * fix(autofix): scan-side skip filter, honest labels comment, infra-distinct discard Review round on the hardening PR (six Suggestions, all adopted): - The scan candidate list now excludes skip-labeled PRs: the address-gate discard writes no marker, so an unfiltered scan would re-emit a skip-labeled PR into a full address job (checkout, npm ci, build) every tick forever. - The engagement-labels env comment no longer overpromises: in THIS change the labels are honored at the eligibility gate and scan filter; the scan-side widening that makes takeover summon human PRs ships with the takeover feature PR (the two commute either way). - A failed eligibility fetch now discards with an infra-distinct message (metadata fetch failed (API error) — fail-closed) instead of masquerading as state='unknown', mirroring the scan-side wording. - Tests: the non-main-base discard is now exercised behaviorally (not just pinned); the fetch-failure case asserts its distinct message; the full parenthesized route trust expression is pinned as ONE string (Actions binds && tighter than ||, so dropped parens would invert the grouping); and the terminal-sentinel adoption guard is exercised on a path that actually reaches the adoption block (live conflict skips the stale gate) instead of passing via the discard. 54/54. --------- Co-authored-by: wenshao <wenshao@example.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes the autofix review loop process its backlog concurrently instead of serially, and makes scan scheduling survive busy repositories. Two coordinated changes: (1)
review-scannow emits every eligible PR as a matrix target instead of a single newest-first target per scan — the address matrix'smax-parallel: 3bounds simultaneity and the existing per-PR concurrency groups prevent duplicate same-PR runs, so one surviving scan drains the whole backlog; (2) theroutejob's concurrency is re-keyed so cron ticks still supersede each other, but manual dispatches and review/issue events each route in their own group and are never cancelled.Why it's needed
Both problems were observed live while driving the loop across all open bot PRs. Starvation by single-target selection: an armed PR (fresh failed checks above its watermark) sat unprocessed for ~16 hours because each rare surviving scan handed its one slot to a newer PR, and overnight cron ticks did not fire at all. Starvation by route cancellation: the route job shared one
cancel-in-progressconcurrency group across all triggers; under runner backlog a route sits queued for minutes, and review-submission events arrive constantly — five consecutive dispatched scans were cancelled this way, and during event storms no full scan survived at all. Route is a seconds-long job, so unique groups for non-cron triggers cost nothing while guaranteeing that every trigger actually routes.Reviewer Test Plan
How to verify
review-scan's per-PR loop no longer ends withbreak # one PR per scheduled scan; the eligible-target accumulation runs to the end of the candidate list, and the fan-out rationale is documented inline. Thereview-addressmatrix (max-parallel: 3,concurrency: qwen-autofix-review-<pr>withcancel-in-progress: false) is unchanged — it already supported multiple targets.route.concurrency.groupis nowschedule → 'qwen-autofix-route-cron'(ticks supersede each other, preserving the original dedupe) andformat('qwen-autofix-route-{0}', github.run_id)otherwise (unique → never cancelled);cancel-in-progressis true only for the cron group.npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 50/50, including the updated assertions: the single-target break is asserted ABSENT, the fan-out comment andmax-parallel: 3are asserted present, and the new concurrency expressions are pinned.run:block passesbash -n.Behavioral expectation after merge: the first scan (cron tick or one dispatch) targets ALL armed bot PRs at once — the matrix shows one
review-address (…)job per PR, up to 3 running concurrently — and a dispatched scan is no longer cancelled by unrelated review events.Evidence (Before & After)
Tested on
Environment (optional)
Contract test via vitest on a worktree checkout of main; YAML parse +
bash -nstatic checks locally. The behavior itself runs onubuntu-latestin the autofix workflow.Risk & Scope
max-parallel(3) agent runs at once — cost scales with open bot PRs, whichMAX_OPEN_AUTOFIX_PRS(5) already bounds; previously the same total work happened, just serialized over many hours. Never-cancelled routes mean each event spends a few runner-seconds even in bursts.Linked Issues
Follow-up to #6998/#7094, prompted by live fleet-driving: a 16-hour starvation of an armed PR and five consecutive route cancellations.
中文说明
本 PR 做了什么
让 autofix review 回路并发处理积压而非串行,并让扫描调度在繁忙仓库中存活。两处协同改动:(1)
review-scan现在把每一个合格 PR 都作为 matrix 目标发出,而不是每次扫描只发一个 newest-first 目标 —— address matrix 的max-parallel: 3约束并发度,既有的每 PR 并发组防止同 PR 重复运行,因此一次存活的扫描即可清空整个积压;(2)routejob 的并发键重设:cron tick 之间仍互相顶替,但手动 dispatch 与 review/issue 事件各自独立路由、永不被取消。为什么需要
两个问题都是在驱动全部 open bot PR 的实战中观察到的。单目标选择导致的饥饿:一个已武装的 PR(新鲜失败检查高于水位线)等了约 16 小时未被处理 —— 稀少的存活扫描每次都把唯一名额给了更新的 PR,而夜间 cron 完全没有触发。route 取消导致的饥饿:route job 全触发器共享一个
cancel-in-progress并发组;runner 积压时 route 排队数分钟,而 review 提交事件源源不断 —— 连续五次 dispatch 的扫描被这样取消,事件风暴期间没有任何全量扫描存活。route 是秒级 job,非 cron 触发用唯一组零成本,且保证每次触发必然完成路由。评审验证方案
如何验证
review-scan的按 PR 循环不再以break # one PR per scheduled scan结尾;合格目标累积会跑完整个候选列表,fan-out 理由内联注释。review-addressmatrix(max-parallel: 3、concurrency: qwen-autofix-review-<pr>、cancel-in-progress: false)未动 —— 它本就支持多目标。route.concurrency.group现为:schedule → 'qwen-autofix-route-cron'(tick 互相顶替,保留原有去重),否则format('qwen-autofix-route-{0}', github.run_id)(唯一 → 永不取消);cancel-in-progress仅对 cron 组为 true。npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 50/50,含更新断言:单目标 break 断言不存在、fan-out 注释与max-parallel: 3断言存在、新并发表达式被钉住。run:块通过bash -n。合并后的行为预期:第一次扫描(cron 或一次 dispatch)同时锁定所有已武装的 bot PR —— matrix 中每个 PR 一个
review-address (…)job,最多 3 个并发 —— 且 dispatch 的扫描不再被无关 review 事件取消。证据(前后对比)
测试情况
环境(可选)
在 main 的 worktree 检出上用 vitest 跑契约测试;本地 YAML 解析 +
bash -n静态检查。行为本身运行于 autofix 工作流的ubuntu-latest。风险与范围
max-parallel(3)个 agent 运行 —— 成本随 open bot PR 数扩展,而MAX_OPEN_AUTOFIX_PRS(5)已有上限;此前同样的总工作量只是被串行摊到数小时。route 永不取消意味着突发事件各花几秒 runner 时间。关联 Issue
#6998/#7094 的后续,由实战驱动舰队时发现:一个已武装 PR 的 16 小时饥饿与连续五次 route 取消。