diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 3c81ac64bc6..e7299ea36b6 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -308,6 +308,7 @@ jobs: takeover_cmd: '${{ steps.decide.outputs.takeover_cmd }}' retry_pr: '${{ steps.decide.outputs.retry_pr }}' cmd_pr: '${{ steps.decide.outputs.cmd_pr }}' + review_sender: '${{ github.event.review.user.login }}' steps: - name: 'Decide phases' id: 'decide' @@ -2003,6 +2004,7 @@ jobs: FORCED_PR: '${{ needs.route.outputs.pr_number }}' DRY_RUN: '${{ needs.route.outputs.dry_run }}' EVENT_NAME: '${{ github.event_name }}' + REVIEW_SENDER: '${{ needs.route.outputs.review_sender }}' DISPATCH_SOURCE: "${{ github.event_name == 'workflow_dispatch' && inputs.source || '' }}" run: |- # Every lane that reaches this scan is supposed to hold the PAT: @@ -2398,6 +2400,21 @@ jobs: fi fi + # Review-workflow id, resolved ONCE per scan for the review-in-flight + # gate below (#8888): during qwen-code-pr-review.yml's 10-minute + # delay-automatic-review wait the review-pr JOB (and thus its + # check-run in statusCheckRollup) does not exist yet, so the rollup + # alone misses a just-triggered review; the runs API sees the run + # by head SHA before its job starts. Empty on lookup failure โ€” the + # gate then degrades to the rollup check only (fail-open, like + # BUSY_PRS). + REVIEW_WF_ID="$(gh api "repos/${REPO}/actions/workflows/qwen-code-pr-review.yml" --jq '.id' 2> /dev/null || echo '')" + REVIEW_RUNS_JSON='{"workflow_runs":[]}' + if [[ -n "${REVIEW_WF_ID}" ]] \ + && ! REVIEW_RUNS_JSON="$(gh api "repos/${REPO}/actions/workflows/${REVIEW_WF_ID}/runs?per_page=100" 2> /dev/null)"; then + REVIEW_RUNS_JSON='{"workflow_runs":[]}' + fi + # 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 @@ -2542,6 +2559,52 @@ jobs: CHECKS_JSON="$(jq -c '.statusCheckRollup // []' <<< "${PR_META}")" PR_HEAD_OID="$(jq -r '.headRefOid // ""' <<< "${PR_META}")" + # Review-in-flight gate (#8888): NON_BLOCKING_CHECKS keeps an + # in-flight review-pr from blocking the FEEDBACK gate (its + # conclusion carries nothing the loop acts on โ€” #7416), but every + # head mutation this scan can make (a stale-base update-branch, + # infra rerun, or address push later) is a synchronize event that + # cancels the in-flight review via qwen-code-pr-review.yml's + # cancel-in-progress, discarding up to ~3h of review work โ€” the + # self-reinforcing cancellation loop of #8830 (three killed runs + # in one PR, two by merge-main). Its findings are also the very + # feedback the next round should batch with, so deferring the + # WHOLE round until the review lands loses nothing: the watermark + # is not advanced on a skip, so the feedback stays visible. This + # is deliberately SEPARATE from HAS_PENDING_CHECKS rather than a + # NON_BLOCKING_CHECKS revert: that gate ages checks out after + # PENDING_STALE_MIN and would also re-block on the review's + # conclusion, reintroducing #7416's median-49-minute wait. + REVIEW_PR_LIVE="$(jq -r ' + [ .[] + | select((((.status // .state // "") | IN("QUEUED", "IN_PROGRESS", "PENDING", "WAITING", "REQUESTED")) and ((.name // "") == "review-pr") and ((.workflowName // "") == "๐Ÿง Qwen Pull Request Review"))) ] + | length > 0 + ' <<< "${CHECKS_JSON}")" + REVIEW_RUN_STARTED_AT="" + if [[ "${REVIEW_PR_LIVE}" != "true" && -n "${REVIEW_WF_ID}" && -n "${PR_HEAD_OID}" ]]; then + # Delay-window fallback: a review run parked BEFORE its job + # starts (the 10-minute environment wait) has no review-pr + # check-run yet, but a push now would still cancel it via + # synchronize. Only pull_request_target runs are cancelable โ€” + # comment/review-triggered runs use per-run concurrency groups + # that a synchronize never cancels, so holding the round for + # one would defer autofix for nothing (R2-1). The scan fetched + # the newest run page once above; match by immutable head SHA or + # PR number, never by fork-controlled bare branch name. + REVIEW_RUN_STARTED_AT="$(jq -r --arg wf "${REVIEW_WF_ID}" --arg pr "${PR}" --arg head "${PR_HEAD_OID}" ' + [ .workflow_runs[]? + | select((.workflow_id | tostring) == $wf) + | select((.event // "") == "pull_request_target") + | select((.status // "") | IN("queued", "waiting", "pending", "requested", "in_progress")) + | select(((.head_sha // "") == $head) or any(.pull_requests[]?; (.number | tostring) == $pr)) + | (.run_started_at // .created_at // "") ] + | map(select(. != "")) | sort | last // "" + ' <<< "${REVIEW_RUNS_JSON}")" + if [[ -n "${REVIEW_RUN_STARTED_AT}" ]]; then + REVIEW_PR_LIVE="true" + fi + fi + # Auto-rerun a check that died on INFRASTRUCTURE, not the code (see # INFRA_FAILURE_SIGNATURES). Only reached when the PR has a FAILED # check; then, for each, we read its annotations and โ€” if they carry @@ -2551,7 +2614,7 @@ jobs: # marker needed; the attempt counter is the guard, and after a rerun # the attempt increments so the next scan skips it. Any API failure # here is fail-safe: it just means no rerun. - if [[ -n "${PR_HEAD_OID}" ]] && jq -e 'any(.[]; ((.conclusion // .state // "") | IN("FAILURE","FAILED","ERROR","TIMED_OUT","ACTION_REQUIRED")) and (((.workflowName // "") != "Qwen Autofix") or ((.name // "") | startswith("review-address"))))' <<< "${CHECKS_JSON}" > /dev/null 2>&1; then + if [[ -n "${PR_HEAD_OID}" && "${REVIEW_PR_LIVE}" != "true" ]] && jq -e 'any(.[]; ((.conclusion // .state // "") | IN("FAILURE","FAILED","ERROR","TIMED_OUT","ACTION_REQUIRED")) and (((.workflowName // "") != "Qwen Autofix") or ((.name // "") | startswith("review-address"))))' <<< "${CHECKS_JSON}" > /dev/null 2>&1; then RERAN_INFRA=false # Failed check-runs on this head, with their run id and annotation # count โ€” fetched once. External statuses (no check-run) are absent @@ -2607,6 +2670,50 @@ jobs: fleet_row "${PR}" 'waiting' 'active checks in flight' continue fi + if [[ "${REVIEW_PR_LIVE}" == "true" ]]; then + echo "๐Ÿ” #${PR}: review-pr in flight on this head โ€” holding this round so the push cannot cancel it (#8888)" + fleet_row "${PR}" 'review-in-flight' 'review-pr live on head; round deferred' + # Ack-on-defer (#8888): a real-time human review routed this + # scan straight here, but the gate holds every mutation โ€” from + # the human's seat the bot read their review and then did + # nothing. Say so once per in-flight review (the marker embeds + # the review-pr check's startedAt, so a NEW review re-arms the + # ack). The feedback itself needs no ack: the watermark is not + # advanced on this skip, so the next scan after the review + # lands still sees and addresses it. Cron scans stay silent โ€” + # nothing arrived in them that a human is waiting on, and the + # fleet table already shows the deferral. + if [[ "${EVENT_NAME}" == 'pull_request_review' && "${DRY_RUN}" != "true" && "${REVIEW_SENDER}" != "${REVIEW_BOT}" ]]; then + REVIEW_STARTED_AT="$(jq -r ' + [ .[] + | select((((.status // .state // "") | IN("QUEUED", "IN_PROGRESS", "PENDING", "WAITING", "REQUESTED")) and ((.name // "") == "review-pr") and ((.workflowName // "") == "๐Ÿง Qwen Pull Request Review"))) + | (.startedAt // "") + | select(. != "") ] | first // ""' <<< "${CHECKS_JSON}")" + [[ -z "${REVIEW_STARTED_AT}" ]] && REVIEW_STARTED_AT="${REVIEW_RUN_STARTED_AT}" + # An empty key (a queued check with no startedAt yet) would + # make the marker match EVERY future deferral โ€” skip the ack + # this scan rather than arm a permanently-dead dedup. + if [[ -z "${REVIEW_STARTED_AT}" ]]; then + echo "๐Ÿ• #${PR}: deferred-review ack skipped: live review-pr check has no startedAt yet (queued); a later scan acks once it starts" + else + DEFER_ACKS="$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ + | jq -r --arg ab "${AUTOFIX_BOT}" '.[] | select((.user.login // "") == $ab) | .body // ""' 2> /dev/null || true)" + if grep -qF "" <<< "${DEFER_ACKS}"; then + echo "๐Ÿ• #${PR}: deferred-review ack already posted for this review run" + else + if [[ -z "${SCAN_BOT_ACTOR:-}" ]]; then + SCAN_BOT_ACTOR="$(gh api user --jq '.login' 2> /dev/null || echo 'unknown')" + fi + if [[ "${SCAN_BOT_ACTOR}" != "${AUTOFIX_BOT}" ]]; then + echo "::warning::#${PR}: deferred-review ack skipped: PAT authenticates as '${SCAN_BOT_ACTOR}', expected ${AUTOFIX_BOT}" + else + gh pr comment "${PR}" --repo "${REPO}" --body "$(printf '๐Ÿ• Review received โ€” an automatic review of the current head is still running, so this round is held until it lands (a push now would cancel it and discard its work, #8888). Your feedback stays queued for the next eligible round.\n\n
\nไธญๆ–‡่ฏดๆ˜Ž\n\n๐Ÿ• ๅทฒๆ”ถๅˆฐ่ฏ„ๅฎก โ€”โ€” ๅฝ“ๅ‰ head ไธŠไปๆœ‰ไธ€่ฝฎ่‡ชๅŠจ review ๅœจ่ฟ่กŒ๏ผŒๆœฌ่ฝฎๆš‚็ผ“๏ผˆ็ŽฐๅœจๆŽจ้€ไผšๅ–ๆถˆ่ฏฅ review ๅนถไธขๅผƒๅ…ถๅทฅไฝœ๏ผŒ#8888๏ผ‰ใ€‚ๅ้ฆˆไฟๆŒๆŽ’้˜Ÿ๏ผŒ็ญ‰ๅพ…ไธ‹ไธ€ๆฌกๅฏ่ฟ่กŒ็š„่ฝฎๆฌกๅค„็†ใ€‚\n\n
\n\n' "${REVIEW_STARTED_AT}")" > /dev/null 2>&1 \ + || echo "::warning::#${PR}: deferred-review ack failed โ€” the dedup marker is NOT posted (a later scan may ack again)" + fi + fi + fi + fi + fi # Pre-first-eval floor: the PR's IMMUTABLE creation time. Feedback # cannot predate the PR, and unlike the head commit date this never # advances when the branch is synced with main ("Update branch"/base @@ -2906,6 +3013,9 @@ jobs: fi continue fi + if [[ "${REVIEW_PR_LIVE}" == "true" ]]; then + continue + fi # Auto-update a PR that is red ONLY because of a stale base (see the # MAIN_GREEN_CHECKS rationale above). The gate: the failing check also # passed for the PR that produced current main (a necessary-but-NOT- diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 0e7bbb738e7..b3a10beb5bb 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -488,7 +488,11 @@ describe('qwen-autofix workflow', () => { const sel = seg.slice(0, 400); return ( !/startswith\("review-address"\)/.test(sel) && - !/!= "Qwen Autofix"/.test(sel) + !/!= "Qwen Autofix"/.test(sel) && + // The review-in-flight gate (#8888) selects BY NAME for the LLM + // review check โ€” a liveness probe, not a feedback selector, so it + // needs neither the review-address carve-out nor the workflow guard. + !/== "review-pr"/.test(sel) ); }); expect(guardlessSelectors).toEqual([]); @@ -542,6 +546,9 @@ describe('qwen-autofix workflow', () => { '.github/workflows/qwen-code-pr-review.yml', 'utf8', ); + expect(reviewWorkflow.split('\n')[0]).toBe( + "name: '๐Ÿง Qwen Pull Request Review'", + ); for (const name of nonBlocking) { expect(reviewWorkflow).toContain(`\n ${name}:\n`); } @@ -590,6 +597,212 @@ describe('qwen-autofix workflow', () => { expect(run([{ ...llm, name: 'resolve-pr' }])).toBe('true'); }); + it('holds a round while review-pr is in flight on the head (#8888)', () => { + // Every head mutation the scan can make (a stale-base update-branch, an + // address push) is a synchronize event that cancels the in-flight review + // via qwen-code-pr-review.yml's cancel-in-progress, discarding up to ~3h + // of review work โ€” the self-reinforcing cancellation loop of PR #8830. + // The gate skips the PR entirely while review-pr is live on its head; the + // watermark is not advanced on the skip, so the feedback stays visible. + // It is deliberately separate from HAS_PENDING_CHECKS (no aging out, no + // NON_BLOCKING_CHECKS revert โ€” that would re-block on the conclusion and + // reintroduce #7416's wait). + expect(reviewScanJob).toContain('REVIEW_PR_LIVE='); + expect(reviewScanJob).toContain( + 'review-pr in flight on this head โ€” holding this round', + ); + expect(reviewScanJob).toContain('fleet_row "${PR}" \'review-in-flight\''); + // The gate must sit BEFORE the stale-base update (a merge-main is exactly + // the push that killed two reviews on #8830) and the feedback dispatch. + expect(reviewScanJob.indexOf('REVIEW_PR_LIVE=')).toBeLessThan( + reviewScanJob.indexOf('Auto-rerun a check that died on INFRASTRUCTURE'), + ); + expect(reviewScanJob.indexOf('REVIEW_PR_LIVE=')).toBeLessThan( + reviewScanJob.indexOf('Auto-update a PR that is red ONLY'), + ); + expect(reviewScanJob.indexOf('REVIEW_PR_LIVE=')).toBeLessThan( + reviewScanJob.indexOf('N_FAILED_CHECKS='), + ); + expect( + reviewScanJob.lastIndexOf('if [[ "${REVIEW_PR_LIVE}" == "true" ]]'), + ).toBeGreaterThan(reviewScanJob.indexOf('if [[ "${ROUND}" -ge')); + + // Replay the REAL extracted liveness filter over rollup fixtures. + const filter = reviewScanJob.match( + /REVIEW_PR_LIVE="\$\(jq -r[\s\S]*?<<< "\$\{CHECKS_JSON\}"\)"/, + )?.[0]; + expect(filter).toBeTruthy(); + const run = (checks) => + execFileSync( + 'bash', + [ + '-c', + `CHECKS_JSON='${JSON.stringify(checks)}'\n${filter}\nprintf '%s' "$REVIEW_PR_LIVE"`, + ], + { env: { ...process.env }, encoding: 'utf8' }, + ); + const started = '2026-08-10T10:05:49Z'; + // A live review-pr blocks โ€” in every pending-ish status the rollup uses. + for (const status of [ + 'QUEUED', + 'IN_PROGRESS', + 'PENDING', + 'WAITING', + 'REQUESTED', + ]) { + expect( + run([ + { + name: 'review-pr', + workflowName: '๐Ÿง Qwen Pull Request Review', + status, + startedAt: started, + }, + ]), + ).toBe('true'); + } + expect( + run([ + { + name: 'review-pr', + workflowName: 'Other', + status: 'IN_PROGRESS', + }, + ]), + ).toBe('false'); + // A concluded review does NOT block (that would reintroduce #7416's wait). + expect( + run([ + { + name: 'review-pr', + workflowName: '๐Ÿง Qwen Pull Request Review', + status: 'COMPLETED', + conclusion: 'SUCCESS', + }, + ]), + ).toBe('false'); + // Other checks in flight are this gate's business as usual โ€” not live. + expect( + run([{ name: 'Test (ubuntu-latest, Node 22.x)', status: 'IN_PROGRESS' }]), + ).toBe('false'); + + // Delay-window fallback: during the review workflow's 10-minute delay the + // review-pr check-run does not exist yet, so the rollup alone misses it; + // the scan falls back to queued runs of the review workflow by head SHA. + expect(reviewScanJob).toContain('REVIEW_WF_ID='); + expect(reviewScanJob).toContain( + 'actions/workflows/${REVIEW_WF_ID}/runs?per_page=100', + ); + expect(reviewScanJob).not.toContain( + 'REVIEW_RUNS_JSON="$(gh api --paginate', + ); + expect(reviewScanJob).toContain( + 'IN("queued", "waiting", "pending", "requested", "in_progress")', + ); + expect(reviewScanJob).not.toContain( + "grep -qE '^(queued|waiting|pending)$'", + ); + expect(reviewScanJob).toContain('REVIEW_RUN_STARTED_AT='); + expect(reviewScanJob).toContain('.run_started_at // .created_at'); + expect(reviewScanJob).toContain('any(.pull_requests[]?'); + expect(reviewScanJob).toContain( + 'select((.event // "") == "pull_request_target")', + ); + + // Replay the REAL runs-API fallback filter over fixtures (R1-8): the + // toContain pins above would still pass if the jq body were dead. + const runsFilter = reviewScanJob.match( + /REVIEW_RUN_STARTED_AT="\$\(jq -r[\s\S]*?<<< "\$\{REVIEW_RUNS_JSON\}"\)"/, + )?.[0]; + expect(runsFilter).toBeTruthy(); + const runRuns = (runs) => + execFileSync( + 'bash', + [ + '-c', + `REVIEW_WF_ID='77' PR='42' PR_HEAD_OID='abc123'\nREVIEW_RUNS_JSON='${JSON.stringify(runs)}'\n${runsFilter}\nprintf '%s' "$REVIEW_RUN_STARTED_AT"`, + ], + { env: { ...process.env }, encoding: 'utf8' }, + ); + const runs = (...overrides) => ({ + workflow_runs: overrides.map((o) => ({ + workflow_id: 77, + event: 'pull_request_target', + status: 'in_progress', + head_sha: 'abc123', + head_branch: 'feat/x', + run_started_at: '2026-08-13T01:00:00Z', + pull_requests: [], + ...o, + })), + }); + // A live automatic review on the head blocks โ€” every pending-ish status + // the runs API uses, including requested/in_progress (R2-2). + for (const status of [ + 'queued', + 'waiting', + 'pending', + 'requested', + 'in_progress', + ]) { + expect(runRuns(runs({ status }))).toBe('2026-08-13T01:00:00Z'); + } + // An explicit-trigger run is NOT cancelable by synchronize โ€” no hold (R2-1). + expect(runRuns(runs({ event: 'issue_comment' }))).toBe(''); + // A run of another workflow id never blocks (R2-1 binding). + expect(runRuns(runs({ workflow_id: 99 }))).toBe(''); + // A concluded run does not block. + expect(runRuns(runs({ status: 'completed' }))).toBe(''); + // A fork-controlled bare branch name alone is not identity. + expect( + runRuns( + runs({ + head_sha: 'other', + head_branch: 'feat/x', + pull_requests: [], + }), + ), + ).toBe(''); + // Immutable head SHA alone is still enough. + expect( + runRuns( + runs({ + head_sha: 'abc123', + head_branch: 'other', + pull_requests: [], + }), + ), + ).toBe('2026-08-13T01:00:00Z'); + // Matching also works via pull_requests association, not only head SHA. + expect( + runRuns( + runs({ + head_sha: 'other', + head_branch: 'other', + pull_requests: [{ number: 42 }], + }), + ), + ).toBe('2026-08-13T01:00:00Z'); + + // Ack-on-defer: a real-time HUMAN review that the gate defers gets one + // visible acknowledgment per in-flight review run (marker keyed on the + // review-pr check's startedAt); the review bot's own findings never ack. + expect(reviewScanJob).toContain('"${REVIEW_SENDER}" != "${REVIEW_BOT}"'); + expect(reviewScanJob).toContain('autofix-review-deferred'); + expect(reviewScanJob).toContain('select((.user.login // "") == $ab)'); + expect(reviewScanJob).toContain( + '[[ -z "${REVIEW_STARTED_AT}" ]] && REVIEW_STARTED_AT="${REVIEW_RUN_STARTED_AT}"', + ); + expect(workflow).toContain( + "review_sender: '${{ github.event.review.user.login }}'", + ); + // An empty startedAt must skip the ack, not arm an always-matching marker. + expect(reviewScanJob).toContain('select(. != "") ] | first // ""'); + expect(reviewScanJob).toContain( + 'has no startedAt yet (queued); a later scan acks once it starts', + ); + }); + it('auto-updates a PR red only from a stale base, gated on green-on-main', () => { // A PR can be red purely because it merged a main that was broken then and // is fixed now (a web-shell TS break, an agent-registry test โ€” both stranded @@ -1016,6 +1229,7 @@ describe('qwen-autofix workflow', () => { rerunOk = true, crName = 'E2E', wfName = 'CI', + reviewLive = false, }) => { const dir = mkdtempSync(join(tmpdir(), 'infra-')); const bin = join(dir, 'bin'); @@ -1062,6 +1276,7 @@ describe('qwen-autofix workflow', () => { PR: '1', PR_META: JSON.stringify({ headRefOid: 'headSHA' }), PR_HEAD_OID: 'headSHA', + REVIEW_PR_LIVE: reviewLive ? 'true' : 'false', CHECKS_JSON: JSON.stringify(checks), INFRA_FAILURE_SIGNATURES: INFRA_SIGNATURES, PATH: `${bin}:${process.env.PATH}`, @@ -1097,6 +1312,16 @@ describe('qwen-autofix workflow', () => { annotations: 'Expected 1 argument but got 2 โ€” src/foo.ts:10', }), ).toEqual({ reran: false, continued: false }); + // A live review-pr must win: rerunning review-address can push and cancel + // that review, so the infra recovery waits for the next scan. + expect( + run({ + checks: [FAIL], + annotations: + 'The self-hosted runner lost communication with the server', + reviewLive: true, + }), + ).toEqual({ reran: false, continued: false }); // Already reran once (attempt 2) and still infra-failing โ†’ persistent, do // not loop. expect( @@ -2678,14 +2903,16 @@ describe('qwen-autofix workflow', () => { // forces a deliberate test update, however it is spaced or line-wrapped: // bump this count AND pipe the new site through the normalizer (bumping // the count below too) โ€” bumping this pin alone leaves toBe(9) green. - expect(workflow.split('--paginate').length - 1).toBe(14); + expect(workflow.split('--paginate').length - 1).toBe(15); // scan ic + pr-events + ic re-fetch + scan rv/rc + prepare rv/rc/ic + // report COMMENTS_JSON fallback = nine normalized fetch sites. The // blocked-takeover status lookup is deliberately NOT among them: like the // sibling STATUS_ID read, it consumes the page stream inline via // `--jq ... | .id` into `tail -1` and never lands in a WORKDIR json file, // so piping it through `jq -s 'add // []'` would wrap the id stream in an - // array and break the tail-1 consumer. + // array and break the tail-1 consumer. The #8888 deferred-review ack + // dedup is the same class: `--jq '.[].body'` feeds a grep, never a + // WORKDIR file, so it bumps the total pin but not the normalizer count. expect(workflow.split("jq -s 'add // []'").length - 1).toBe(9); // Empty-input semantics: a total gh failure feeds the fallback an EMPTY // stream, where the normalizer filter must yield '[]' and not 'null' โ€”