Skip to content

fix(autofix): hold autofix rounds while review-pr is in flight #8899

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.

Sign up for GitHub

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

Merged
yiliang114 merged 6 commits into from
Aug 14, 2026
Merged

fix(autofix): hold autofix rounds while review-pr is in flight #8899

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c9302d
fix(autofix): hold rounds while review-pr is in flight (#8888)
yiliang114 Aug 10, 2026
ede52a0
fix(ci): harden autofix review-in-flight gate
yiliang114 Aug 13, 2026
97532d0
fix(ci): ack deferred review fallback runs
yiliang114 Aug 13, 2026
ee7f327
fix(ci): hold only cancelable automatic reviews in the gate (R2-1)
yiliang114 Aug 13, 2026
527da11
fix(ci): bound review run fallback
yiliang114 Aug 13, 2026
801e61e
fix(ci): gate infra reruns behind review liveness
yiliang114 Aug 13, 2026
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
101 changes: 101 additions & 0 deletions .github/workflows/qwen-autofix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -2398,6 +2400,16 @@ 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 '')"

# 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
Expand Down Expand Up @@ -2607,6 +2619,92 @@ jobs:
fleet_row "${PR}" 'waiting' 'active checks in flight'
continue
fi
# Review-in-flight gate (#8888): NON_BLOCKING_CHECKS keeps an
# in-flight review-pr from blocking the FEEDBACK gate (its
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
# conclusion carries nothing the loop acts on — #7416), but every
# head mutation this scan can make (a stale-base update-branch
# below, an 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"))) ]

@yiliang114 yiliang114 Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This needs a design decision before code changes. Distinguishing cancelable automatic review check-runs from explicit-trigger review check-runs in statusCheckRollup requires mapping the check back to workflow-run provenance, which is broader than the bounded runs-fallback fix.

| length > 0
' <<< "${CHECKS_JSON}")"
REVIEW_RUN_STARTED_AT=""
if [[ "${REVIEW_PR_LIVE}" != "true" && -n "${REVIEW_WF_ID}" && -n "${PR_HEAD_OID}" && -n "${BRANCH}" ]]; 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. The runs API catches both pull_request_target and
# explicit-trigger review runs by PR/head identity. One paginated
# read per candidate, and only when the rollup showed nothing live.
REVIEW_RUNS_JSON="$(gh api --paginate "repos/${REPO}/actions/workflows/${REVIEW_WF_ID}/runs?per_page=100" 2> /dev/null || printf '{"workflow_runs":[]}')"
REVIEW_RUN_STARTED_AT="$(jq -r --arg wf "${REVIEW_WF_ID}" --arg pr "${PR}" --arg head "${PR_HEAD_OID}" --arg branch "${BRANCH}" '
[ .workflow_runs[]?
| select((.workflow_id | tostring) == $wf)
| select((.status // "") | IN("queued", "waiting", "pending", "requested", "in_progress"))
| select(((.head_sha // "") == $head) or ((.head_branch // "") == $branch) 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
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'
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
# 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
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
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")))
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
| (.startedAt // "")
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
| select(. != "") ] | first // ""' <<< "${CHECKS_JSON}")"
[[ -z "${REVIEW_STARTED_AT}" ]] && REVIEW_STARTED_AT="${REVIEW_RUN_STARTED_AT}"
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
# 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)"
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
if grep -qF "<!-- autofix-review-deferred ${REVIEW_STARTED_AT} -->" <<< "${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<details>\n<summary>中文说明</summary>\n\n🕐 已收到评审 —— 当前 head 上仍有一轮自动 review 在运行，本轮暂缓（现在推送会取消该 review 并丢弃其工作，#8888）。反馈保持排队，等待下一次可运行的轮次处理。\n\n</details>\n\n<!-- autofix-review-deferred %s -->' "${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
Expand Down Expand Up @@ -2906,6 +3004,9 @@ jobs:
fi
continue
fi
if [[ "${REVIEW_PR_LIVE}" == "true" ]]; then
continue
fi
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
# Auto-update a PR that is red ONLY because of a stale base (see the
Comment on lines +3016 to 3019

@yiliang114 yiliang114 Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This needs a scheduling-boundary decision before code changes. Re-checking review liveness at prepare/push/report time would add a second live gate across multiple mutation sites, not a bounded patch to the scan gate.

Comment thread
yiliang114 marked this conversation as resolved.
Resolved
# MAIN_GREEN_CHECKS rationale above). The gate: the failing check also
# passed for the PR that produced current main (a necessary-but-NOT-
Expand Down
135 changes: 132 additions & 3 deletions scripts/tests/qwen-autofix-workflow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,11 @@
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)
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
);
});
expect(guardlessSelectors).toEqual([]);
Expand Down Expand Up @@ -590,6 +594,129 @@
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-update a PR that is red ONLY'),
);
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
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).toContain('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[]?');

// 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}"');
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
expect(reviewScanJob).toContain('autofix-review-deferred');
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
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 }}'",
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
);
// 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
Expand Down Expand Up @@ -2678,14 +2805,16 @@
// 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);

Check failure on line 2808 in scripts/tests/qwen-autofix-workflow.test.js

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest, Node 22.x)

scripts/tests/qwen-autofix-workflow.test.js > qwen-autofix workflow > normalizes every paginated WORKDIR fetch to one flat array (>100-item PRs) 

AssertionError: expected 16 to be 15 // Object.is equality

- Expected
+ Received

- 15
+ 16

 ❯ scripts/tests/qwen-autofix-workflow.test.js:2808:53
// 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' —
Expand Down
Loading