From 4a4afb961d95991ed727e1de7ac586b98b1db35a Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 26 Aug 2026 17:24:51 +0800 Subject: [PATCH 01/11] fix(ci): salvage superseded review runs and hold the loop's report-time base refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A synchronize used to cancel-in-progress the in-flight review run no matter how close to done it was: on PR #9729 a push cancelled run 32726618419 4h06m in, minutes from posting, and cancelled review runs wasted ~7h41m of runner time across that one PR. A declarative cancel cannot weigh the run's progress, so the supersede decision moves into the run: cancel-in-progress now fires on closed only, a synchronize run queues pending in the PR-scoped group, and the in-flight run's new supersede watcher either ends the attempt early (pre-threshold: the queued replacement reviews the new head from scratch, within a poll interval) or, past the salvage threshold (compose artifact present, or elapsed >= 50% of the size-aware budget, tunable via QWEN_REVIEW_SALVAGE_ELAPSED_PERCENT), finishes and posts against the head it reviewed. The gh-wrapper write guard gains a salvage escape pinned to the reviewed head; the CLI's existing presubmit head-drift gate annotates and caps the verdict, and a new step posts a historical-head note naming the anchor the next incremental review starts from. The delay job now also skips a queued run whose event head went stale while it waited. The other half covers the one loop-owned head move outside the #8888/#8899 dispatch hold: the autofix report step's stale-base update-branch. It now probes for a live review-pr first (same probe pair as the scan gate, fail-open on probe errors) and defers the refresh with the retry branch's sentinel semantics instead of superseding the review; the deferred round joins the consecutive-failure exemptions. Rationale recorded as qwen-autofix.md#af-148, and the af-099 delay-window entry is updated for the new supersede semantics. Both workflows' .size-baseline entries are bumped in this PR per the growth ratchet: the review workflow gains the watcher/salvage machinery (+15 KB) and the autofix report step gains the probe pair (+6 KB) — real feature growth, with the long-form rationale kept in the design record. Fixes #10110 --- .github/workflows/.size-baseline | 4 +- .github/workflows/qwen-autofix.md | 53 +++- .github/workflows/qwen-autofix.yml | 49 +++- .github/workflows/qwen-code-pr-review.yml | 260 +++++++++++++++++- scripts/tests/qwen-autofix-workflow.test.js | 136 ++++++++- scripts/tests/qwen-pr-review-workflow.test.js | 200 +++++++++++++- scripts/tests/qwen-resolve-workflow.test.js | 71 ++++- 7 files changed, 742 insertions(+), 31 deletions(-) diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 3664883e371..d007b317dca 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -34,9 +34,9 @@ 6495 pr-self-report-label.yml 9646 qwen-autofix-fork-bridge.yml 5942 qwen-autofix-fork-signal.yml -397656 qwen-autofix.yml +403688 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -158010 qwen-code-pr-review.yml +173247 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 qwen-issue-followup-bot.yml 5760 qwen-pr-safety-precheck.yml diff --git a/.github/workflows/qwen-autofix.md b/.github/workflows/qwen-autofix.md index 3e014680dd6..b845534e62f 100644 --- a/.github/workflows/qwen-autofix.md +++ b/.github/workflows/qwen-autofix.md @@ -249,6 +249,7 @@ task-oriented guides — what a maintainer types and what happens next — see: - [145. review-address · Report dry-run / failure — CUMULATIVE timeout breaker — the sibling of the consecutive one above, for the…](#af-145) - [146. review-address · Report dry-run / failure — The agent committed (verify recorded committed=true before any gate could fail),…](#af-146) - [147. review-address · Report dry-run / failure — Same byte-budget hygiene as the English excerpt above. 3000 bytes ≈ 1000 CJK…](#af-147) +- [148. review-address · Report dry-run / failure — Hold the stale-base refresh while a review-pr is in flight on the PR.…](#af-148) --- @@ -2764,10 +2765,14 @@ In `review-scan` · `Scan for PRs with new feedback`. ```text 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 — +check-run yet, but a push now would still supersede it (the +review workflow's own supersede handling, #10110: a parked or +pre-threshold run yields to the push — its work is discarded +exactly as the old synchronize cancel did — and only a +past-threshold run salvages; the hold keeps the loop from +forcing either). Only pull_request_target runs are affected — comment/review-triggered runs use per-run concurrency groups -that a synchronize never cancels, so holding the round for +that a synchronize never touches, so holding the round for one would defer autofix for nothing (R2-1). Match against the scan's REVIEW_RUNS_JSON fetch — one page of the review workflow's runs, empty on lookup failure — by immutable head @@ -3727,3 +3732,45 @@ forbids HTML in failure.zh.md), but must not be able to open or close a
/ that swallows the closing tag the workflow emits below. ``` + + + +### 148. review-address · Report dry-run / failure — Hold the stale-base refresh while a review-pr is in flight on the PR. + +In `review-address` · `Report dry-run / failure`. + +```text +The scan's dispatch gate (#8888/#8899) already refuses to start a round +while review-pr is live, but the loop had one more head-moving write +outside that hold: this step's stale-base retry calls update-branch at +REPORT time, hours after the dispatch gate last looked. A review can +start in that window — a human /review comment, a bot re-request, or a +run the scan's fail-open probe missed — and the merge push would then +supersede a lifecycle review run mid-flight (#10110; before the salvage +threshold that discards its work exactly as the old cancel did), or +invalidate a command run's posting: every review pins the head it +reviews (QWEN_CI_REVIEW_EXPECTED_HEAD_SHA) and its guard blocks the +final post when the head moved, so even the uncancellable per-run-group +reviews lose their whole run to a head move. + +So the retry probes for a live review first, with the scan gate's exact +probe pair: the statusCheckRollup filter (any live review-pr check from +the review workflow, trigger-independent), then the runs-API fallback +for pull_request_target runs still parked in the 10-minute delay window +with no check-run yet. On a live review the update is DEFERRED, not +skipped: the same 9999 sentinel MARK_TS the retry branch uses keeps the +feedback live, the next scan re-runs the round (itself held while the +review is still in flight), and that round's report step performs the +refresh once the review has landed. One extra round of latency, bounded +by MAX_ROUNDS, against hours of discarded review work. + +Fail-open on probe errors, deliberately: the probe is an optimization, +and failing closed would wedge stale-base recovery — the path that +un-sticks red PRs — on any transient API error. A probe error therefore +reads as "no review live" and the update proceeds, which is exactly the +pre-#10110 behavior. The deferred headline joins CONSEC_FAIL's +streak-reset needles ("deferred a stale-base refresh"): like the +updated-a-stale-base round it defers to, the round's failure is not +evidence about the PR, and counting it toward the cap would park a PR +for having been reviewed at the wrong moment. +``` diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 513d30ef94c..052c4c452e2 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -2568,8 +2568,9 @@ jobs: 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. + # check-run yet, but a push now would still supersede it + # (#10110: a parked or pre-threshold run yields to the push — + # its work is discarded exactly as a cancel did). # Full rationale → qwen-autofix.md#af-099 REVIEW_RUN_STARTED_AT="$(jq -r --arg wf "${REVIEW_WF_ID}" --arg pr "${PR}" --arg head "${PR_HEAD_OID}" ' [ .workflow_runs[]? @@ -5684,13 +5685,40 @@ jobs: # generated checks complete after the conflict marker this # same report posts — waking the very park it establishes. # Full rationale → qwen-autofix.md#af-143 + STALE_BASE_DEFERRED=false if [[ "${AUDIT_VERDICT:-}" != 'conflict' ]]; then MAIN_HEAD_R="$(gh api "repos/${REPO}/commits/${DEFAULT_BRANCH:-main}" --jq '.sha' 2> /dev/null || echo '')" if [[ -n "${MAIN_HEAD_R}" && -n "${REPORT_HEAD}" ]]; then CMP_R="$(gh api "repos/${REPO}/compare/${MAIN_HEAD_R}...${REPORT_HEAD}" --jq '.status' 2> /dev/null || echo '')" - if [[ "${CMP_R}" == 'behind' || "${CMP_R}" == 'diverged' ]] \ - && gh api -X PUT "repos/${REPO}/pulls/${PR}/update-branch" -f expected_head_sha="${REPORT_HEAD}" > /dev/null 2>&1; then - STALE_BASE_RETRY=true + if [[ "${CMP_R}" == 'behind' || "${CMP_R}" == 'diverged' ]]; then + # Review-in-flight hold for the loop's OWN report-time + # head move (#10110): same probe pair as the scan's + # dispatch gate (#8888), fail-open on probe errors. + # Full rationale → qwen-autofix.md#af-148 + ROLLUP_R="$(gh pr view "${PR}" --repo "${REPO}" --json statusCheckRollup --jq '.statusCheckRollup // []' 2> /dev/null || echo '[]')" + REVIEW_LIVE_R="$(jq -r ' + [ .[] + | select((((.status // .state // "") | IN("QUEUED", "IN_PROGRESS", "PENDING", "WAITING", "REQUESTED")) and ((.name // "") == "review-pr") and ((.workflowName // "") == "🧐 Qwen Pull Request Review"))) ] + | length > 0 + ' <<< "${ROLLUP_R}" 2> /dev/null || echo '')" + if [[ "${REVIEW_LIVE_R}" != 'true' ]]; then + RWID_R="$(gh api "repos/${REPO}/actions/workflows/qwen-code-pr-review.yml" --jq '.id' 2> /dev/null || echo '')" + LIVE_HEAD_R="$(jq -r '.headRefOid // ""' <<< "$(gh pr view "${PR}" --repo "${REPO}" --json headRefOid 2> /dev/null || echo '{}')")" + if [[ -n "${RWID_R}" ]]; then + REVIEW_LIVE_R="$(gh api "repos/${REPO}/actions/workflows/${RWID_R}/runs?per_page=100" 2> /dev/null | jq -r --arg pr "${PR}" --arg head "${LIVE_HEAD_R}" ' + [ .workflow_runs[]? + | select((.event // "") == "pull_request_target") + | select((.status // "") | IN("queued", "waiting", "pending", "requested", "in_progress")) + | select((($head != "") and ((.head_sha // "") == $head)) or any(.pull_requests[]?; (.number | tostring) == $pr)) ] + | length > 0 + ' 2> /dev/null || echo '')" + fi + fi + if [[ "${REVIEW_LIVE_R}" == 'true' ]]; then + STALE_BASE_DEFERRED=true + elif gh api -X PUT "repos/${REPO}/pulls/${PR}/update-branch" -f expected_head_sha="${REPORT_HEAD}" > /dev/null 2>&1; then + STALE_BASE_RETRY=true + fi fi fi fi @@ -5702,6 +5730,13 @@ jobs: MARK_TS='9999-12-31T23:59:59Z' HEADLINE="🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind \`${DEFAULT_BRANCH:-main}\`, so it merged current main in via update-branch and will retry on the next scan. A stale base (a dependency or symbol main already changed) can fail the build without being the fix's fault; if it still fails once current, it hands off to a human." HEADLINE_ZH="🤖 AutoFix 更新了一个过期的 base —— 修复未通过验证，但本 PR 落后于 \`${DEFAULT_BRANCH:-main}\`，因此已通过 update-branch 合入当前 main，并将在下次扫描时重试。过期的 base（main 已改动的依赖或符号）可能让构建失败而并非修复本身的错；若 base 更新后仍然失败，将移交人工处理。" + elif [[ "${STALE_BASE_DEFERRED}" == 'true' ]]; then + # Same sentinel semantics as the retry branch: feedback stays + # live, the next round re-runs, and ITS report step performs + # the base refresh once the review has landed. + MARK_TS='9999-12-31T23:59:59Z' + HEADLINE="🤖 AutoFix deferred a stale-base refresh — the fix did not pass verification and this PR is behind \`${DEFAULT_BRANCH:-main}\`, but a review of this PR is still in flight, and merging main now would cancel it or invalidate its posting (#10110). The refresh and this item's retry move to the next scan." + HEADLINE_ZH="🤖 AutoFix 暂缓了一次过期 base 刷新 —— 修复未通过验证且本 PR 落后于 \`${DEFAULT_BRANCH:-main}\`，但该 PR 上仍有一轮评审在运行，现在合入 main 会取消该评审或使其发布失效（#10110）。base 刷新与此项的重试推迟到下次扫描。" else # Say what actually happens next. The old "A human should # take over this PR" read as a full release, but the loop @@ -5758,7 +5793,7 @@ jobs: # Consecutive-failure circuit breaker, distinct from the round cap. # Full rationale → qwen-autofix.md#af-069 - if [[ "${MARK_ROUND}" != "${MAX_ROUNDS}" ]] && [[ "${PREPARE_OUTCOME}" == 'success' || "${PREPARE_OUTCOME}" == 'failure' ]] && [[ "${STALE_BASE_RETRY:-false}" != 'true' ]] && { [[ -z "${API_ERROR_DETAIL}" ]] || [[ "${API_ERROR_KIND}" == 'auth' ]]; }; then + if [[ "${MARK_ROUND}" != "${MAX_ROUNDS}" ]] && [[ "${PREPARE_OUTCOME}" == 'success' || "${PREPARE_OUTCOME}" == 'failure' ]] && [[ "${STALE_BASE_RETRY:-false}" != 'true' ]] && [[ "${STALE_BASE_DEFERRED:-false}" != 'true' ]] && { [[ -z "${API_ERROR_DETAIL}" ]] || [[ "${API_ERROR_KIND}" == 'auth' ]]; }; then CONSEC_FAIL=1 if [[ -f "${WORKDIR}/ic.json" ]]; then COMMENTS_JSON="$(cat "${WORKDIR}/ic.json")" @@ -5780,7 +5815,7 @@ jobs: | (.body | gsub("\r"; "") | split("\n")[0])' <<< "${COMMENTS_JSON}" 2> /dev/null || true)" while IFS= read -r H; do [[ -n "${H}" ]] || continue - if [[ "${H}" == *"Addressed the latest review feedback"* || "${H}" == *"no changes needed"* || "${H}" == *"AutoFix could not start —"* || "${H}" == *"updated a stale base"* ]]; then + if [[ "${H}" == *"Addressed the latest review feedback"* || "${H}" == *"no changes needed"* || "${H}" == *"AutoFix could not start —"* || "${H}" == *"updated a stale base"* || "${H}" == *"deferred a stale-base refresh"* ]]; then CONSEC_FAIL=1 else CONSEC_FAIL=$(( CONSEC_FAIL + 1 )) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 1e437bf94fe..151c4f3af0c 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -49,8 +49,9 @@ on: type: 'boolean' concurrency: - # PR lifecycle events share a PR-scoped group so new pushes restart the delay - # and closed PRs stop any in-flight lifecycle review. Every review_requested + # PR lifecycle events share a PR-scoped group so a closed PR stops any + # in-flight lifecycle review and a new push queues behind one instead of + # racing it. Every review_requested # run — the bot-directed one included — gets a per-run group: membership is # decided here, before `authorize` runs, but whether a bot request reviews # anything is `authorize`'s call on the REQUESTER's write permission. A @@ -64,12 +65,29 @@ concurrency: # lands while the lifecycle run for the same head still queues: compute, # never a lost review. Comment/review events use per-run groups to avoid # cancelling active reviews. + # + # cancel-in-progress fires on `closed` ONLY (#10110). It used to fire on + # `synchronize` too, and that discarded whole reviews no matter how close + # to done they were: on PR #9729 a push cancelled run 32726618419 4h06m in, + # minutes from posting, and the replacement started the same work from + # zero (~7h41m of cancelled review time across that one PR). A declarative + # cancel cannot weigh the in-flight run's progress, so the supersede + # decision moves INTO the run: a synchronize run now queues PENDING in this + # group (the latest push replaces the pending slot) while the in-flight + # run's supersede watcher in "Run review" notices the head move and either + # ends the attempt early — little invested; the queued run reviews the new + # head from scratch — or, past the salvage threshold, finishes and posts + # against the head it reviewed as a historical-head review. Accepted cost: + # rapid pushes during the delay window now wait out the superseded run's + # remaining delay (~10 minutes) plus teardown instead of cancelling it + # instantly; the superseded run's review-pr exits on its stale-head guard + # without spending review budget. group: >- ${{ github.event_name == 'pull_request_target' && github.event.action != 'review_requested' && format('qwen-pr-review-pr-{0}', github.event.pull_request.number) || format('qwen-pr-review-run-{0}', github.run_id) }} - cancel-in-progress: "${{ github.event_name == 'pull_request_target' && (github.event.action == 'synchronize' || github.event.action == 'closed') }}" + cancel-in-progress: "${{ github.event_name == 'pull_request_target' && github.event.action == 'closed' }}" env: # Dedup marker for the review-failure fallback comments. The in-job step @@ -227,10 +245,11 @@ jobs: env: GH_TOKEN: '${{ secrets.GITHUB_TOKEN }}' PR_NUMBER: '${{ github.event.pull_request.number }}' + EVENT_HEAD_SHA: '${{ github.event.pull_request.head.sha }}' run: |- set -euo pipefail - pr_data="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,isDraft --jq '[.state, .isDraft] | @tsv')" - IFS=$'\t' read -r state is_draft <<< "$pr_data" + pr_data="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,isDraft,headRefOid --jq '[.state, .isDraft, .headRefOid] | @tsv')" + IFS=$'\t' read -r state is_draft current_head <<< "$pr_data" if [ "$state" != "OPEN" ]; then echo "Skipping delayed review: PR #${PR_NUMBER} is ${state}." >> "$GITHUB_STEP_SUMMARY" @@ -242,6 +261,17 @@ jobs: echo "should_review=false" >> "$GITHUB_OUTPUT" exit 0 fi + # A lifecycle run can outlive its own head: with cancel-in-progress + # scoped to `closed` (#10110) a synchronize run waits PENDING in the + # PR group behind an in-flight review, and by the time it runs a + # newer push may have superseded its event head. Its successor run + # owns that newer head; skip here, before review-pr spends runner + # setup to reach the same conclusion at its own stale-head guard. + if [ -n "$EVENT_HEAD_SHA" ] && [ -n "$current_head" ] && [ "$current_head" != "$EVENT_HEAD_SHA" ]; then + echo "Skipping delayed review: PR #${PR_NUMBER} moved from ${EVENT_HEAD_SHA} to ${current_head} while this run queued." >> "$GITHUB_STEP_SUMMARY" + echo "should_review=false" >> "$GITHUB_OUTPUT" + exit 0 + fi echo "should_review=true" >> "$GITHUB_OUTPUT" authorize: @@ -923,6 +953,13 @@ jobs: EVENT_NAME: '${{ github.event_name }}' EVENT_HEAD_SHA: '${{ github.event.pull_request.head.sha }}' MAX_TIMEOUT_MINUTES_VAR: '${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}' + # Supersede-salvage tunable (#10110): percentage of the size-aware + # review budget an automatic review must have spent before a head + # move salvages (finishes and posts against the reviewed head) + # instead of ceding to the replacement run. Empty/non-numeric falls + # back to 50 in the step; 0 salvages always, 100 effectively only + # on the compose-artifact signal. + SALVAGE_ELAPSED_PERCENT_VAR: '${{ vars.QWEN_REVIEW_SALVAGE_ELAPSED_PERCENT }}' run: |- set -euo pipefail fail() { @@ -943,7 +980,11 @@ jobs: LOG_PATH="${RUNNER_TEMP:-/tmp}/qwen-review-pr-${PR_NUMBER}.jsonl" # Set by configure_qwen_network once the wrapper dir exists. PROXY_BIN="" - trap 'rm -f "$LOG_PATH"; [ -z "$PROXY_BIN" ] || rm -rf "$PROXY_BIN"' EXIT + # WATCHER_PID/SALVAGE_DIR are set once the supersede watcher exists. + # The trap must reap the watcher on EVERY exit path (fail() included): + # a background watcher outliving this step on the reused self-hosted + # runner could later kill a NEXT job's review of the same PR. + trap 'rm -f "$LOG_PATH"; [ -z "$PROXY_BIN" ] || rm -rf "$PROXY_BIN"; [ -z "${WATCHER_PID:-}" ] || kill "${WATCHER_PID}" 2>/dev/null; [ -z "${SALVAGE_DIR:-}" ] || rm -rf "${SALVAGE_DIR}"' EXIT if [ -z "${GH_TOKEN:-}" ]; then fail "CI_BOT_PAT secret is required for Qwen PR review." @@ -1019,8 +1060,25 @@ jobs: exit 90 fi if [ -n "$expected_head" ] && [ "$current_head" != "$expected_head" ]; then - echo "Blocked PR write: PR #${pr_number} moved from ${expected_head} to ${current_head}." >&2 - exit 90 + # Salvage escape (#10110): the supersede watcher arms this marker + # only past the salvage threshold (compose done, or the elapsed + # fraction crossed), pinning the head this run reviewed; content + # must equal that pin so a stale marker from another run never + # matches. The review then posts against the historical head — + # the CLI's own presubmit head-drift gate (classifyHeadDrift) + # annotates the body and caps the verdict — instead of + # discarding hours of finished work. This escape is a + # correctness gate, not a security boundary: the agent can read + # the marker path from its own environment, but forging it only + # lets it post to an old head — nothing it could not already + # post to the live one. The OPEN check above still applies. + if [ -n "${QWEN_CI_REVIEW_SALVAGE_OK_FILE:-}" ] \ + && [ "$(cat "${QWEN_CI_REVIEW_SALVAGE_OK_FILE}" 2>/dev/null)" = "$expected_head" ]; then + echo "PR write allowed (salvage): PR #${pr_number} moved from ${expected_head} to ${current_head} past the salvage threshold; posting against the reviewed head." >&2 + else + echo "Blocked PR write: PR #${pr_number} moved from ${expected_head} to ${current_head}." >&2 + exit 90 + fi fi } guard_api_write() { @@ -1280,6 +1338,45 @@ jobs: echo "effective_timeout_minutes=$EFFECTIVE_TIMEOUT_MINUTES" } >> "$GITHUB_OUTPUT" + # Supersede-salvage state (#10110). A private per-run dir, like + # PROXY_BIN above: RUNNER_TEMP survives across jobs on the shared + # self-hosted runner, so a fixed path could be pre-seeded or left + # over. The watcher writes; guard_pr_write and the paths below read. + SALVAGE_DIR="$(mktemp -d "${RUNNER_TEMP:-/tmp}/qwen-review-salvage.XXXXXX")" + export QWEN_CI_REVIEW_SALVAGE_OK_FILE="${SALVAGE_DIR}/salvage-ok" + SUPERSEDE_FILE="${SALVAGE_DIR}/superseded" + # The composed-verdict artifact the review skill writes when its + # compose step completes (`.qwen/tmp/qwen-review-pr--composed.json`, + # see composedNameFor in packages/cli/src/commands/review/run.ts — + # keep the two in sync). Its existence means the findings are final + # and posting is minutes out; the run's Step 9 cleanup sweeps it, so + # the watcher LATCHES the first sighting. + COMPOSED_ARTIFACT="${GITHUB_WORKSPACE}/.qwen/tmp/qwen-review-pr-${PR_NUMBER}-composed.json" + SALVAGE_ELAPSED_PERCENT="${SALVAGE_ELAPSED_PERCENT_VAR:-}" + case "$SALVAGE_ELAPSED_PERCENT" in + ''|*[!0-9]*) SALVAGE_ELAPSED_PERCENT=50 ;; + esac + if [ "$SALVAGE_ELAPSED_PERCENT" -gt 100 ]; then + SALVAGE_ELAPSED_PERCENT=100 + fi + # KEEP vs CEDE for a superseded in-flight review, extracted for the + # workflow tests. KEEP (salvage) when compose already completed — + # posting is minutes out regardless of elapsed time — or when the + # run spent at least SALVAGE_ELAPSED_PERCENT of its size-aware + # budget. The default 50% approximates the issue's "80% of typical + # wall time": measured reviews land around 55-70% of their budget + # tier, so half the budget is ~80% of a typical run — e.g. the + # 4h06m run cancelled minutes from posting on PR #9729 (360-minute + # budget) crosses at 180 minutes, while a 90-minute-in run of the + # same budget restarts cheaply on the new head. + salvage_eligible() { + local elapsed="$1" budget="$2" compose_seen="$3" pct="$4" + if [ "$compose_seen" = "true" ]; then + return 0 + fi + [ "$(( elapsed * 100 ))" -ge "$(( budget * pct ))" ] + } + PROMPT="/review ${REVIEW_URL}" if [ "$DOCS_ONLY_MEDIUM" = "true" ]; then PROMPT="${PROMPT} --effort medium" @@ -1476,6 +1573,62 @@ jobs: RETRY_MIN_SECONDS=600 MAX_ATTEMPTS=2 START_TS="$(date +%s)" + + # Supersede watcher (#10110): only automatic lifecycle reviews — the + # runs cancel-in-progress used to cancel on synchronize. It polls + # the live head; on a move it either arms the salvage marker (past + # salvage_eligible — the run finishes and posts against the head it + # reviewed) or writes SUPERSEDE_FILE and kills the review tree so + # the replacement run queued in the concurrency group starts within + # a poll interval instead of after the full budget. The kill matches + # this PR's review URL in the command line: the step shell and this + # watcher run the runner's script file (the URL is not in their + # argv), the runner executes one job at a time, and -U scopes to the + # runner user — so the pattern reaches exactly the qwen tree (the + # CLI relaunch wrapper included, which a recorded PID would miss). + # Explicit runs (/review, review_requested, dispatch) keep today's + # behavior: a human asked for this head's review, and no synchronize + # ever cancelled them. Self-bounded past the budget so a watcher + # leaked through a hard step kill cannot act on a later job. + SALVAGE_POLL_SECONDS=60 + supersede_watcher() { + local live_head compose_seen elapsed + while sleep "$SALVAGE_POLL_SECONDS"; do + elapsed=$(( $(date +%s) - START_TS )) + if [ "$elapsed" -gt "$(( BUDGET_SECONDS + 1800 ))" ]; then + return 0 + fi + if [ ! -f "${SALVAGE_DIR}/compose-seen" ] && [ -e "$COMPOSED_ARTIFACT" ]; then + : > "${SALVAGE_DIR}/compose-seen" + fi + live_head="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefOid --jq '.headRefOid' 2>/dev/null)" || continue + if [ -z "$live_head" ] || [ "$live_head" = "$EXPECTED_HEAD_SHA" ]; then + continue + fi + compose_seen=false + if [ -f "${SALVAGE_DIR}/compose-seen" ]; then + compose_seen=true + fi + if salvage_eligible "$elapsed" "$BUDGET_SECONDS" "$compose_seen" "$SALVAGE_ELAPSED_PERCENT"; then + printf '%s' "$live_head" > "${SALVAGE_DIR}/moved-to" + printf '%s' "$EXPECTED_HEAD_SHA" > "$QWEN_CI_REVIEW_SALVAGE_OK_FILE" + echo "qwen-review-salvage: head moved to ${live_head} ${elapsed}s in (compose_seen=${compose_seen}) — finishing and posting against reviewed head ${EXPECTED_HEAD_SHA} (#10110)" + return 0 + fi + printf '%s' "$live_head" > "$SUPERSEDE_FILE" + echo "qwen-review-salvage: head moved to ${live_head} ${elapsed}s in, before the salvage threshold — ending this review so the queued replacement covers the new head (#10110)" + pkill -U "$(id -u)" -TERM -f "${REVIEW_URL}($|[^0-9])" 2>/dev/null || true + sleep 15 + pkill -U "$(id -u)" -KILL -f "${REVIEW_URL}($|[^0-9])" 2>/dev/null || true + return 0 + done + } + WATCHER_PID="" + if [ "${AUTO_REVIEW:-false}" = "true" ]; then + supersede_watcher & + WATCHER_PID=$! + fi + attempt=1 while :; do attempt_timeout=$(( BUDGET_SECONDS - ($(date +%s) - START_TS) )) @@ -1483,6 +1636,28 @@ jobs: fail "${REASON:-Qwen review ran out of time budget before it could complete.}" 1 "$KIND" fi run_review_once "$attempt_timeout" "$PROMPT" + # Checked BEFORE the outcome classification: a watcher kill + # surfaces as a non-zero qwen status that would otherwise read as + # a fatal (or retryable — and a retry would re-review the + # superseded head from scratch) failure. Ending clean here keeps + # the failure fallbacks silent; the queued replacement run owns + # the new head. + # ${VAR:-} defaults: the retry-loop replay in + # scripts/tests/qwen-pr-review-workflow.test.js extracts this loop + # verbatim and runs it under set -u without the salvage setup. + if [ -f "${SUPERSEDE_FILE:-}" ]; then + echo "Superseded early: PR #${PR_NUMBER:-} moved from ${EXPECTED_HEAD_SHA:-} to $(cat "$SUPERSEDE_FILE" 2>/dev/null || echo unknown) before the salvage threshold; ceding to the replacement run (#10110)." | tee -a "$GITHUB_STEP_SUMMARY" + exit 0 + fi + if [ "$OUTCOME" != "success" ] && [ -f "${QWEN_CI_REVIEW_SALVAGE_OK_FILE:-}" ]; then + # Salvage was armed but the attempt still died (API error, + # timeout). Retrying would re-review a head that is no longer + # current — the replacement run queued behind this one already + # covers the live head, so cede instead of retrying or + # announcing a failure the fallback guards would suppress anyway. + echo "Salvage-armed review attempt did not complete (${REASON:-unknown}); PR #${PR_NUMBER:-} head already moved — ceding to the replacement run (#10110)." | tee -a "$GITHUB_STEP_SUMMARY" + exit 0 + fi if [ "$OUTCOME" = "success" ]; then break fi @@ -1496,6 +1671,21 @@ jobs: fail "$REASON" 1 "$KIND" done + # The review is done; the watcher must not fire past this point (a + # head move now is the NEXT run's business). The EXIT trap also + # reaps it on every early-exit path above. + if [ -n "$WATCHER_PID" ]; then + kill "$WATCHER_PID" 2>/dev/null || true + wait "$WATCHER_PID" 2>/dev/null || true + WATCHER_PID="" + fi + if [ -f "$QWEN_CI_REVIEW_SALVAGE_OK_FILE" ]; then + { + echo "salvaged=true" + echo "salvage_moved_to=$(cat "${SALVAGE_DIR}/moved-to" 2>/dev/null || echo unknown)" + } >> "$GITHUB_OUTPUT" + fi + if [ "$DOCS_ONLY_MEDIUM" = "true" ]; then # The review CLI's machine-readable completion contract — batch # drivers detect completion by this exact line, and it is the one @@ -1527,6 +1717,45 @@ jobs: # relay must not announce a review that never ran. echo "review_completed=true" >> "$GITHUB_OUTPUT" + # A salvaged review just posted against a head that is no longer + # current (#10110). The review body itself carries the CLI's head-drift + # annotation and verdict cap (classifyHeadDrift), but that gate samples + # the live head at presubmit — a push landing between that sample and + # the submit is only known here. This note names the anchor either way + # so a reader (and the autofix loop) knows why a review of an older + # head just appeared and where the next delta review starts from. Never + # fails the job: the review itself succeeded. + - name: 'Report salvaged historical-head review' + if: |- + steps.context.outputs.should_run == 'true' && + steps.review.outcome == 'success' && + steps.review.outputs.review_completed == 'true' && + steps.review.outputs.salvaged == 'true' && + steps.review.outputs.docs_only_medium != 'true' && + steps.context.outputs.pr_number != '' + env: + GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' + PR_NUMBER: '${{ steps.context.outputs.pr_number }}' + EXPECTED_HEAD_SHA: '${{ steps.review.outputs.expected_head_sha }}' + MOVED_TO: '${{ steps.review.outputs.salvage_moved_to }}' + RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' + run: |- + set -euo pipefail + BODY="$(printf '%s\n' \ + "" \ + '' \ + "⏳ **Historical-head review** — head moved to \`${MOVED_TO}\` while this review was in flight (past the salvage threshold), so the run finished and posted against the head it reviewed: \`${EXPECTED_HEAD_SHA}\` (#10110). The next automatic review covers the delta from that anchor. Full log in the [workflow run](${RUN_URL})." \ + '' \ + '
中文说明' \ + '' \ + "⏳ **历史 head 评审** —— 本次评审进行中 head 移动到了 \`${MOVED_TO}\`（已过 salvage 阈值），因此评审跑完并针对其实际评审的 head \`${EXPECTED_HEAD_SHA}\` 发布（#10110）。下一次自动评审将从该锚点起评审增量。完整日志见 [workflow 运行](${RUN_URL})。" \ + '' \ + '
')" + gh pr comment "$PR_NUMBER" \ + --repo "$GITHUB_REPOSITORY" \ + --body "$BODY" \ + || echo "::warning::salvage note could not be posted; the salvaged review itself is on the PR (reviewed head ${EXPECTED_HEAD_SHA})." + - name: 'Report docs-only medium outcome' if: |- steps.context.outputs.should_run == 'true' && @@ -2001,12 +2230,15 @@ jobs: # `needs`: a job-level timeout cancels review-pr ALONE — authorize and # delay-automatic-review completed long before — while a run-level cancel # sweeps the whole chain, so 'cancelled' opens the gate only when neither - # upstream job was itself cancelled. A run-level cancel landing AFTER the - # chain finished (mid-review) still opens the gate: the push-supersede - # flavor is then suppressed by the in-step head-moved guard, the close - # flavor (a `closed`-action run joining the PR-scoped group hours in, the - # head unchanged) by the in-step PR-state check, and a same-head twin - # cannot land that late — its cancel fires at run creation, seconds in. + # upstream job was itself cancelled. Run-level cancels are rarer since + # cancel-in-progress dropped `synchronize` (#10110: a push now supersedes + # in-run — the superseded step exits 0, never opening this gate); the ones + # left can still land AFTER the chain finished (mid-review) and still open + # the gate: the close flavor (a `closed`-action run joining the PR-scoped + # group hours in, the head unchanged) is suppressed by the in-step PR-state + # check, a manual push-adjacent cancel by the in-step head-moved guard, and + # a same-head twin cannot land that late — its cancel fires at run + # creation, seconds in. # A manual run-cancel during the delay window goes # silent under this rule (the person who cancelled does not need retry # guidance); a manual cancel of review-pr alone mid-review still posts, diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 4edbd3fb952..90a2c94de9c 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -16256,7 +16256,7 @@ exit 1 expect(timeoutCap).toBeLessThan(takeoverCap); const block = reviewAddressReportStep.match( - /if \[\[ "\$\{MARK_ROUND\}" != "\$\{MAX_ROUNDS\}" \]\] && \[\[ "\$\{PREPARE_OUTCOME\}" == 'success' \|\| "\$\{PREPARE_OUTCOME\}" == 'failure' \]\] && \[\[ "\$\{STALE_BASE_RETRY:-false\}" != 'true' \]\] && \{ \[\[ -z "\$\{API_ERROR_DETAIL\}" \]\] \|\| \[\[ "\$\{API_ERROR_KIND\}" == 'auth' \]\]; \}; then\n {14}CONSEC_FAIL=1\n[\s\S]*?\n {14}fi\n {12}fi\n/, + /if \[\[ "\$\{MARK_ROUND\}" != "\$\{MAX_ROUNDS\}" \]\] && \[\[ "\$\{PREPARE_OUTCOME\}" == 'success' \|\| "\$\{PREPARE_OUTCOME\}" == 'failure' \]\] && \[\[ "\$\{STALE_BASE_RETRY:-false\}" != 'true' \]\] && \[\[ "\$\{STALE_BASE_DEFERRED:-false\}" != 'true' \]\] && \{ \[\[ -z "\$\{API_ERROR_DETAIL\}" \]\] \|\| \[\[ "\$\{API_ERROR_KIND\}" == 'auth' \]\]; \}; then\n {14}CONSEC_FAIL=1\n[\s\S]*?\n {14}fi\n {12}fi\n/, )?.[0]; expect(block).toBeTruthy(); const script = block.replace(/^ {12}/gm, ''); @@ -16274,6 +16274,8 @@ exit 1 '🤖 AutoFix could not start evaluation — it crashed or timed out before reading the feedback.'; const STALE_BASE = '🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind `main`, so it merged current main in via update-branch and will retry on the next scan.'; + const STALE_BASE_DEFER = + '🤖 AutoFix deferred a stale-base refresh — the fix did not pass verification and this PR is behind `main`, but a review of this PR is still in flight (#10110).'; const run = ( priorHeadlines, @@ -16284,6 +16286,7 @@ exit 1 apiErrorKind = '', prepareOutcome = 'success', staleBaseRetry = false, + staleBaseDeferred = false, agentTimeout = '', } = {}, ) => { @@ -16313,7 +16316,7 @@ exit 1 'bash', [ '-c', - `set -uo pipefail\nWORKDIR='${dir}'\nMARK_ROUND=${markRound}\nMAX_ROUNDS=100\nCONSECUTIVE_FAILURE_CAP=${cap}\nTIMEOUT_WINDOW_CAP=${timeoutCap}\nAGENT_TIMEOUT='${agentTimeout}'\nCONSEC_FAIL=0\nREPO=o/r\nPR=1\nAUTOFIX_BOT=qwen-code-dev-bot\nRETRY_COMMAND='@qwen-code /retry'\nAPI_ERROR_DETAIL='${apiErrorDetail}'\nAPI_ERROR_KIND='${apiErrorKind}'\nPREPARE_OUTCOME='${prepareOutcome}'\nSTALE_BASE_RETRY='${staleBaseRetry}'\n${window !== undefined ? `WINDOW='${window}'\n` : ''}HEADLINE=orig\nHEADLINE_ZH=orig\n${script}\nprintf '\\n@@R@@%s|%s|%s|%s' "$MARK_ROUND" "${'${CONSEC_FAIL}'}" "$HEADLINE" "$HEADLINE_ZH"`, + `set -uo pipefail\nWORKDIR='${dir}'\nMARK_ROUND=${markRound}\nMAX_ROUNDS=100\nCONSECUTIVE_FAILURE_CAP=${cap}\nTIMEOUT_WINDOW_CAP=${timeoutCap}\nAGENT_TIMEOUT='${agentTimeout}'\nCONSEC_FAIL=0\nREPO=o/r\nPR=1\nAUTOFIX_BOT=qwen-code-dev-bot\nRETRY_COMMAND='@qwen-code /retry'\nAPI_ERROR_DETAIL='${apiErrorDetail}'\nAPI_ERROR_KIND='${apiErrorKind}'\nPREPARE_OUTCOME='${prepareOutcome}'\nSTALE_BASE_RETRY='${staleBaseRetry}'\nSTALE_BASE_DEFERRED='${staleBaseDeferred}'\n${window !== undefined ? `WINDOW='${window}'\n` : ''}HEADLINE=orig\nHEADLINE_ZH=orig\n${script}\nprintf '\\n@@R@@%s|%s|%s|%s' "$MARK_ROUND" "${'${CONSEC_FAIL}'}" "$HEADLINE" "$HEADLINE_ZH"`, ], { env: { ...process.env, PATH: `${bin}:${process.env.PATH}` }, @@ -16427,6 +16430,18 @@ exit 1 expect( run(Array(cap - 1).fill(FAIL), { staleBaseRetry: true }), ).toMatchObject({ terminal: false, headline: 'orig' }); + // Same for a DEFERRED stale-base refresh (#10110): the round failed + // because the base is stale AND a review was in flight — the fix was + // never re-attempted on a fresh base, so the failure is not evidence + // about the PR. Current-round exemption and prior-headline streak reset + // both mirror the retry branch. + expect( + run(Array(cap - 1).fill(FAIL), { staleBaseDeferred: true }), + ).toMatchObject({ terminal: false, headline: 'orig' }); + expect(run([FAIL, FAIL, STALE_BASE_DEFER, FAIL, FAIL])).toMatchObject({ + consec: 3, + terminal: false, + }); // Already-terminal rounds skip the circuit breaker entirely. expect(run(Array(cap).fill(FAIL), { markRound: 100 })).toMatchObject({ terminal: true, @@ -21533,3 +21548,120 @@ describe('stale sandbox container cleanup', () => { expect(r.calls.split('\n')).toEqual(['rm -f -- qwen-code-9.9.9-9']); }); }); + +describe('report-step stale-base hold while review-pr is in flight (#10110)', () => { + // The scan's dispatch gate (#8888) covers every push the SCAN can make, + // but the report step's stale-base retry calls update-branch hours after + // that gate last looked — the one loop-owned head move outside the hold. + // Full rationale → qwen-autofix.md#af-148. + + it('probes for a live review before the report-step update-branch', () => { + const probeAt = reviewAddressReportStep.indexOf('REVIEW_LIVE_R='); + const updateAt = reviewAddressReportStep.indexOf( + 'gh api -X PUT "repos/${REPO}/pulls/${PR}/update-branch" -f expected_head_sha="${REPORT_HEAD}"', + ); + expect(probeAt).toBeGreaterThan(-1); + expect(updateAt).toBeGreaterThan(probeAt); + // Both probe layers, mirroring the scan gate: the rollup filter (any + // started review-pr check, trigger-independent) and the runs-API + // fallback for lifecycle runs still parked in the delay window with no + // check-run yet. + expect(reviewAddressReportStep).toContain('<<< "${ROLLUP_R}"'); + expect(reviewAddressReportStep).toContain( + 'actions/workflows/qwen-code-pr-review.yml', + ); + // The update runs only on a falsy probe — fail-open: a probe error must + // not wedge stale-base recovery, so errors read as "no review live". + expect(reviewAddressReportStep).toContain( + 'if [[ "${REVIEW_LIVE_R}" == \'true\' ]]; then\n STALE_BASE_DEFERRED=true\n elif gh api -X PUT', + ); + }); + + it('defers with the retry branch sentinel and an exempted headline', () => { + // Deferred keeps the exact semantics of the retry branch minus the + // update: the 9999 sentinel keeps feedback live so the next round + // re-runs and ITS report step refreshes the base once the review landed. + const deferred = reviewAddressReportStep.match( + /elif \[\[ "\$\{STALE_BASE_DEFERRED\}" == 'true' \]\]; then[\s\S]*?(?=\n {16}else)/, + )?.[0]; + expect(deferred).toBeTruthy(); + expect(deferred).toContain("MARK_TS='9999-12-31T23:59:59Z'"); + expect(deferred).toContain('deferred a stale-base refresh'); + expect(deferred).toContain('#10110'); + // The deferred headline joins the streak-reset needles: a round that + // failed because it was reviewed at the wrong moment is not evidence + // about the PR. + expect(reviewAddressReportStep).toContain( + '|| "${H}" == *"deferred a stale-base refresh"*', + ); + }); + + it('detects a live review through the replayed rollup filter', () => { + const filter = reviewAddressReportStep.match( + /REVIEW_LIVE_R="\$\(jq -r '([\s\S]*?)' <<< "\$\{ROLLUP_R\}"/, + )?.[1]; + expect(filter).toBeTruthy(); + const probe = (checks) => + execFileSync('jq', ['-r', filter], { + input: JSON.stringify(checks), + encoding: 'utf8', + }).trim(); + const live = { + name: 'review-pr', + workflowName: '🧐 Qwen Pull Request Review', + status: 'IN_PROGRESS', + }; + expect(probe([live])).toBe('true'); + expect(probe([{ ...live, status: 'QUEUED' }])).toBe('true'); + // A finished review does not hold, and neither does a live check that + // is not the review workflow's review-pr. + expect( + probe([{ ...live, status: 'COMPLETED', conclusion: 'SUCCESS' }]), + ).toBe('false'); + expect(probe([{ ...live, workflowName: 'CI' }])).toBe('false'); + expect(probe([{ ...live, name: 'lint' }])).toBe('false'); + expect(probe([])).toBe('false'); + }); + + it('detects a delay-parked lifecycle run through the replayed runs filter', () => { + const filter = reviewAddressReportStep.match( + /jq -r --arg pr "\$\{PR\}" --arg head "\$\{LIVE_HEAD_R\}" '([\s\S]*?)' 2> \/dev\/null \|\| echo/, + )?.[1]; + expect(filter).toBeTruthy(); + const probe = (runs, pr = '9729', head = 'sha-live') => + execFileSync( + 'jq', + ['-r', '--arg', 'pr', pr, '--arg', 'head', head, filter], + { + input: JSON.stringify({ workflow_runs: runs }), + encoding: 'utf8', + }, + ).trim(); + const parked = { + event: 'pull_request_target', + status: 'waiting', + head_sha: 'sha-live', + pull_requests: [], + }; + expect(probe([parked])).toBe('true'); + // Matches by PR number too (fork heads may not equal the fetched head). + expect( + probe([ + { ...parked, head_sha: 'other', pull_requests: [{ number: 9729 }] }, + ]), + ).toBe('true'); + // Completed runs, non-lifecycle events, and unrelated runs do not hold. + expect(probe([{ ...parked, status: 'completed' }])).toBe('false'); + expect(probe([{ ...parked, event: 'issue_comment' }])).toBe('false'); + expect( + probe([{ ...parked, head_sha: 'other', pull_requests: [{ number: 1 }] }]), + ).toBe('false'); + // An empty fetched head must not wildcard-match every run. + expect(probe([{ ...parked, head_sha: '' }], '9729', '')).toBe('false'); + }); + + it('documents the hold in the design doc', () => { + expect(designDoc).toContain(''); + expect(designDoc).toContain('deferred a stale-base refresh'); + }); +}); diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 6fcd4c67c4e..b8165a0ae49 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -66,7 +66,10 @@ function retryLoopSource() { // Drive the extracted loop with a stub qwen whose stream-json `result` event is // scripted per attempt, plus stub timeout/sleep so the test is instant. -function runScenario(scenario, { timeoutMinutes = 180, logPath } = {}) { +function runScenario( + scenario, + { timeoutMinutes = 180, logPath, extraEnv = {} } = {}, +) { const dir = mkdtempSync(join(tmpdir(), 'review-retry-')); try { const bin = join(dir, 'bin'); @@ -163,6 +166,7 @@ function runScenario(scenario, { timeoutMinutes = 180, logPath } = {}) { ATT: attemptFile, DUR: durationFile, PRM: promptFile, + ...extraEnv, }, }); } catch (e) { @@ -3855,3 +3859,197 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { expect(r.summary).toContain('deferring to the fallback-comment job'); }); }); + +describe('review supersede salvage (#10110)', () => { + // cancel-in-progress no longer fires on synchronize (that pin lives in the + // resolve suite next to the wrapper-guard replays); the supersede decision + // moved into the in-flight run. These tests pin the three moving parts: + // the watcher that decides KEEP vs CEDE, the retry loop's early exits, and + // the posting path for a salvaged run. + const doc = parse(workflow); + const run = runReviewStep(); + + it('arms the watcher only for automatic lifecycle reviews', () => { + // Explicit runs (/review, review_requested, dispatch) were never + // synchronize-cancellable; killing a review a human just asked for over + // a push would be a regression, so the watcher must stay AUTO-gated. + expect(run).toContain('supersede_watcher()'); + expect(run).toContain( + 'if [ "${AUTO_REVIEW:-false}" = "true" ]; then\n supersede_watcher &', + ); + // Defined and armed before the retry loop starts. + expect(run.indexOf('supersede_watcher()')).toBeLessThan( + run.indexOf('attempt=1'), + ); + }); + + it('checks supersede and salvage-cede before classifying the attempt outcome', () => { + // A watcher kill surfaces as a non-zero qwen status; classified first it + // would read as fatal (job red, fallback machinery engaged) or retryable + // (a from-scratch re-review of a superseded head). + const call = run.indexOf('run_review_once "$attempt_timeout" "$PROMPT"'); + const supersede = run.indexOf('if [ -f "${SUPERSEDE_FILE:-}" ]; then'); + const cede = run.indexOf( + 'if [ "$OUTCOME" != "success" ] && [ -f "${QWEN_CI_REVIEW_SALVAGE_OK_FILE:-}" ]; then', + ); + const success = run.indexOf('if [ "$OUTCOME" = "success" ]; then'); + expect(call).toBeGreaterThan(-1); + expect(supersede).toBeGreaterThan(call); + expect(cede).toBeGreaterThan(supersede); + expect(success).toBeGreaterThan(cede); + }); + + it('decides KEEP vs CEDE with the extracted salvage_eligible', () => { + const fn = run.match(/salvage_eligible\(\) \{[\s\S]*?\n\}/)?.[0]; + expect(fn).toBeTruthy(); + const eligible = (elapsed, budget, composeSeen, pct) => { + try { + execFileSync( + 'bash', + [ + '-c', + `set -euo pipefail\n${fn}\nsalvage_eligible ${elapsed} ${budget} ${composeSeen} ${pct}`, + ], + { encoding: 'utf8' }, + ); + return true; + } catch { + return false; + } + }; + // Compose done → always keep, however early. + expect(eligible(60, 21600, 'true', 50)).toBe(true); + // The motivating incident: PR #9729's 4h06m (14760s) review on a + // 360-minute (21600s) budget crosses the default 50% threshold. + expect(eligible(14760, 21600, 'false', 50)).toBe(true); + // Exactly at the threshold keeps; just below cedes. + expect(eligible(10800, 21600, 'false', 50)).toBe(true); + expect(eligible(10799, 21600, 'false', 50)).toBe(false); + // pct=100 keeps only a run that spent its whole budget (in practice: + // compose-signal only); pct=0 keeps always. + expect(eligible(21599, 21600, 'false', 100)).toBe(false); + expect(eligible(1, 21600, 'false', 0)).toBe(true); + }); + + it('ends a superseded attempt clean without retrying (replayed loop)', () => { + const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); + try { + const supersedeFile = join(dir, 'superseded'); + writeFileSync(supersedeFile, 'head-b'); + const r = runScenario('success', { + extraEnv: { SUPERSEDE_FILE: supersedeFile }, + }); + expect(r.attempts).toBe(1); + expect(r.raw).toContain('Superseded early:'); + expect(r.raw).toContain('ceding to the replacement run (#10110)'); + expect(r.raw).not.toContain('FAIL '); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('cedes instead of retrying when salvage armed but the attempt died (replayed loop)', () => { + const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); + try { + const salvageFile = join(dir, 'salvage-ok'); + writeFileSync(salvageFile, 'head-a'); + // transient_persist normally burns the retry (2 attempts) and FAILs; + // with salvage armed the head has moved, so a retry would re-review a + // superseded head from scratch — one attempt, clean exit. + const r = runScenario('transient_persist', { + extraEnv: { QWEN_CI_REVIEW_SALVAGE_OK_FILE: salvageFile }, + }); + expect(r.attempts).toBe(1); + expect(r.raw).toContain('Salvage-armed review attempt did not complete'); + expect(r.raw).not.toContain('FAIL '); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('completes normally when salvage armed and the attempt succeeds (replayed loop)', () => { + const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); + try { + const salvageFile = join(dir, 'salvage-ok'); + writeFileSync(salvageFile, 'head-a'); + const r = runScenario('success', { + extraEnv: { QWEN_CI_REVIEW_SALVAGE_OK_FILE: salvageFile }, + }); + expect(r.line).toBe('OK outcome=success'); + expect(r.attempts).toBe(1); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('pins the compose-artifact path to the CLI that writes it', () => { + // The watcher's strongest KEEP signal is the composed-verdict artifact; + // its name comes from composedNameFor in the review CLI. If either side + // renames, this cross-pin fails instead of the signal silently dying + // (the elapsed threshold would still salvage, but later than intended). + expect(run).toContain( + 'COMPOSED_ARTIFACT="${GITHUB_WORKSPACE}/.qwen/tmp/qwen-review-pr-${PR_NUMBER}-composed.json"', + ); + const cli = readFileSync('packages/cli/src/commands/review/run.ts', 'utf8'); + expect(cli).toContain('`qwen-review-pr-${cls.number}-composed.json`'); + }); + + it('bounds and scopes the watcher kill', () => { + // -U scopes to the runner user; the trailing ($|[^0-9]) keeps PR 123 + // from matching PR 1234's URL; TERM first, KILL after a grace period. + expect(run).toContain( + 'pkill -U "$(id -u)" -TERM -f "${REVIEW_URL}($|[^0-9])"', + ); + expect(run).toContain( + 'pkill -U "$(id -u)" -KILL -f "${REVIEW_URL}($|[^0-9])"', + ); + // Self-bounded past the budget, and reaped on every exit path — a + // watcher outliving the step on a reused self-hosted runner could kill + // a later job's review of the same PR. + expect(run).toContain('BUDGET_SECONDS + 1800'); + expect(run).toContain('[ -z "${WATCHER_PID:-}" ] || kill "${WATCHER_PID}"'); + }); + + it('wires the salvage outputs into the historical-head note step', () => { + expect(run).toContain('echo "salvaged=true"'); + expect(run).toContain('salvage_moved_to='); + const note = doc.jobs['review-pr'].steps.find( + (s) => s.name === 'Report salvaged historical-head review', + ); + expect(note).toBeTruthy(); + expect(note.if).toContain("steps.review.outputs.salvaged == 'true'"); + expect(note.if).toContain( + "steps.review.outputs.review_completed == 'true'", + ); + // Docs-only medium never posts, so a "posted against" note would be + // false there. + expect(note.if).toContain( + "steps.review.outputs.docs_only_medium != 'true'", + ); + expect(note.run).toContain('\n\n' + + '⏳ **Historical-head review** — head moved while this review was in flight.', + }; + const human = { + user: { login: 'maintainer' }, + author_association: 'MEMBER', + created_at: '2026-08-02T01:00:00Z', + body: 'please address the remaining findings', + }; + const countComments = (comments) => + execFileSync( + 'jq', + [ + '--arg', + 'wm', + '2026-08-01T00:00:00Z', + '--arg', + 'rb', + 'qwen-code-ci-bot', + '--arg', + 'ab', + 'qwen-code-dev-bot', + '--argjson', + 'trust', + '["OWNER","MEMBER","COLLABORATOR"]', + '--arg', + 'bf', + filterDef, + '--arg', + 'cf', + '^\\s*@qwen-code /', + jqProgram, + ], + { input: JSON.stringify(comments), encoding: 'utf8' }, + ).trim(); + // The note alone adds nothing actionable; the human comment still + // counts beside it. + expect(countComments([note])).toBe('0'); + expect(countComments([note, human])).toBe('1'); + expect(countComments([human])).toBe('1'); + // Shape pin: the marker rides the shared alternation at the four + // sibling sites that count or render REVIEW_BOT issue comments + // (NEWEST, LIVE_NEW, and both feedback renderers) plus the scan + // filter's BOT_COMMENT_FILTER — and at no other site (the + // conflict-wake and over-budget filters already exclude REVIEW_BOT + // by login). + expect( + workflow.match(/qwen-review-ack\|qwen-review-salvaged/g) ?? [], + ).toHaveLength(5); + }); + it('keeps forced issue routing bounded to open issues', () => { expect(workflow).toContain( '--json number,title,body,labels,createdAt,url,state', diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 29cba7c44d3..1b085f11555 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -124,7 +124,12 @@ function runScenario( 'if [ -n "${SUPERSEDE_DURING_BACKOFF:-}" ]; then', ' printf "head-b" > "$SUPERSEDE_DURING_BACKOFF"', 'fi', - 'if [ -n "${BACKOFF_OBS:-}" ]; then', + // The observation stands in for the watcher's poll THROUGH the + // backoff, so it must see the RESET state — gate it on the marker + // the scenario's attempt-1 stub drops at its exit: no observation + // during attempt 1 (a watcher's early poll there would race the + // compose writes), every observation after is post-reset. + 'if [ -n "${BACKOFF_OBS:-}" ] && [ -f "${BACKOFF_OBS}.ready" ]; then', ' if [ -f "$SALVAGE_DIR/compose-seen" ] || [ -e "$COMPOSED_ARTIFACT" ]; then', ' echo present >> "$BACKOFF_OBS"', ' else', @@ -141,6 +146,18 @@ function runScenario( ' echo "$n" > "$SLC"', ' [ "$n" -le "$SLEEP_FAIL_AFTER" ] || exit 1', 'fi', + // The inverse of SLEEP_FAIL_AFTER: ONLY the first sleep fails. The + // watcher is launched before the loop, so its first poll-sleep is + // the first sleep in the window — it ends without ever polling gh, + // while the retry backoff (and a relaunched watcher) still sleep + // normally. That gives a backoff-cede replay an AUTO_REVIEW=true + // run whose watcher deterministically misses the attempt. + 'if [ -n "${SLEEP_FAIL_ONLY_FIRST:-}" ]; then', + ' SLC="$ATT.sleep-count"', + ' n=$(( $(cat "$SLC" 2>/dev/null || echo 0) + 1 ))', + ' echo "$n" > "$SLC"', + ' [ "$n" -gt 1 ] || exit 1', + 'fi', 'exit 0', ].join('\n') + '\n', ); @@ -241,9 +258,14 @@ function runScenario( ' salvage_armed_then_die) printf "head-a" > "$QWEN_CI_REVIEW_SALVAGE_OK_FILE"; r success false "[API Error: 503 upstream overloaded]" ;;', ' supersede_forge_mid_attempt) printf "head-b" > "$SUPERSEDE_FILE"; r success false "Reviewed." ;;', ' supersede_during_backoff) if [ "$n" -eq 1 ]; then r success false "[API Error: 503 upstream overloaded]"; else r success false "attempt 2 must not run"; fi ;;', - ' compose_then_backoff_supersede) if [ "$n" -eq 1 ]; then : > "$SALVAGE_DIR/compose-seen"; printf "{}" > "$COMPOSED_ARTIFACT"; r success false "[API Error: 503 upstream overloaded]"; else r success false "attempt 2 must not run"; fi ;;', + ' compose_then_backoff_supersede) if [ "$n" -eq 1 ]; then : > "$SALVAGE_DIR/compose-seen"; printf "{}" > "$COMPOSED_ARTIFACT"; r success false "[API Error: 503 upstream overloaded]"; [ -z "${BACKOFF_OBS:-}" ] || touch "${BACKOFF_OBS}.ready"; else r success false "attempt 2 must not run"; fi ;;', ' supersede_after_success) printf "head-b" > "$SUPERSEDE_FILE"; r success false "Reviewed." ;;', ' cede_revert_kill) exit 143 ;;', + // The FF-departure shape of a supersede-then-revert: attempt 1 + // waits for the watcher's cede (the SUPERSEDE_FILE write) and then + // dies as killed — the watcher's local kill record is the only + // departure witness, since a normal push emits no force-push event. + ' cede_revert_ff_kill) if [ "$n" -eq 1 ]; then i=0; until [ -f "$SUPERSEDE_FILE" ] || [ "$i" -ge 400 ]; do /bin/sleep 0.05; i=$((i+1)); done; exit 143; else r success false "attempt 2 must not run"; fi ;;', ' retry_watcher_relaunch) if [ "$n" -eq 1 ]; then r success false "[API Error: 503 upstream overloaded]"; else i=0; until [ -f "$SUPERSEDE_FILE" ] || [ "$i" -ge 200 ]; do /bin/sleep 0.05; i=$((i+1)); done; r success false "[API Error: 503 upstream overloaded]"; fi ;;', ' retry_clears_stale_signals) if [ "$n" -eq 1 ]; then r success false "[API Error: 503 upstream overloaded]"; else { [ -f "$SUPERSEDE_FILE" ] && echo present || echo absent; } >> "$OBS"; r success false "ok"; fi ;;', ' compose_latch_reset) if [ "$n" -eq 1 ]; then r success false "[API Error: 503 upstream overloaded]"; : > "$SALVAGE_DIR/compose-seen"; else { [ -f "$SALVAGE_DIR/compose-seen" ] && echo present || echo absent; } >> "$OBS"; r success false "[API Error: 503 upstream overloaded]"; fi ;;', @@ -4068,9 +4090,13 @@ describe('review supersede salvage (#10110)', () => { // synchronize-cancellable; killing a review a human just asked for over // a push would be a regression, so the watcher must stay AUTO-gated. expect(run).toContain('supersede_watcher()'); - expect(run).toContain( - 'if [ "${AUTO_REVIEW:-false}" = "true" ]; then\n supersede_watcher &', - ); + expect(run).toContain('if [ "${AUTO_REVIEW:-false}" = "true" ]; then'); + expect(run).toContain('supersede_watcher &'); + // The revert cede's kill-record dir is minted at arming and NEVER + // exported — an exported path would be agent-derivable like every + // SALVAGE_DIR signal (R4-1). + expect(run).toContain('SUPERSEDE_KILL_DIR="$(mktemp -d'); + expect(run).not.toContain('export SUPERSEDE_KILL_DIR'); // Defined and armed before the retry loop starts. expect(run.indexOf('supersede_watcher()')).toBeLessThan( run.indexOf('attempt=1'), @@ -4096,9 +4122,10 @@ describe('review supersede salvage (#10110)', () => { // also runs at the TOP of the loop: a cede landing during the retry // backoff (pkill matched nothing — qwen not running) must stop the next // attempt before it re-reviews the dead head. - const check = 'if [ -f "${SUPERSEDE_FILE:-}" ] && live_head_moved; then'; + const check = + 'if [ "${AUTO_REVIEW:-false}" = "true" ] && [ -f "${SUPERSEDE_FILE:-}" ] && live_head_moved; then'; const guardedCheck = - 'if [ "$OUTCOME" != "success" ] && [ -f "${SUPERSEDE_FILE:-}" ] && live_head_moved; then'; + 'if [ "$OUTCOME" != "success" ] && [ "${AUTO_REVIEW:-false}" = "true" ] && [ -f "${SUPERSEDE_FILE:-}" ] && live_head_moved; then'; const call = run.indexOf('run_review_once "$attempt_timeout" "$PROMPT"'); const preAttempt = run.indexOf(check); const supersede = run.indexOf(guardedCheck, call); @@ -4177,6 +4204,7 @@ describe('review supersede salvage (#10110)', () => { composedArtifact = null, docsOnly = false, pct = 50, + swapArtifactOnRead = false, plant = {}, failFirstPoll = false, emptyFirstPoll = false, @@ -4238,6 +4266,26 @@ describe('review supersede salvage (#10110)', () => { : `#!/bin/bash\necho "${liveHead}"\n`, ); write('pkill', `#!/bin/bash\necho "$*" >> "${pkillLog}"\n`); + if (swapArtifactOnRead) { + // The R6-3 witness: the latch's [ -f ] passes the real artifact, + // and the node invocation swaps a FIFO in at the reopen moment — + // the exact check-then-open window a statically planted FIFO + // cannot reach. readFileSync blocks forever on it (no writer), so + // without the timeout bound the watcher never decides again and + // this harness dies on its own timeout instead. + write( + 'node', + [ + '#!/bin/bash', + 'target="$3"', + 'if [ -n "$target" ] && [ -f "$target" ]; then', + ' rm -f "$target"', + ' mkfifo "$target"', + 'fi', + `exec "${process.execPath}" "$@"`, + ].join('\n') + '\n', + ); + } const eligible = run.match(/salvage_eligible\(\) \{[\s\S]*?\n\}/)?.[0] ?? ''; const harness = [ @@ -4384,6 +4432,25 @@ describe('review supersede salvage (#10110)', () => { expect(fifo.superseded).toBe('head-b'); }); + it('bounds the compose-artifact reopen against a rename-swapped FIFO (replayed watcher)', () => { + // R6-3: [ -f ] refuses a statically planted FIFO, but the artifact + // path is agent-derivable — one rename-swapped in during the + // check-then-open window blocks readFileSync forever (no writer, no + // timeout) and kills the supersede watcher permanently: every later + // head move goes unwatched. The node stub performs the swap at open + // time; the timeout bound keeps the poll bounded and the one-shot + // decision alive (removing the bound turns this test into a harness + // timeout). Takes the ~5s of the production bound. + const r = runWatcher({ + composedArtifact: '{"downgraded":false}', + attemptElapsed: 30, + swapArtifactOnRead: true, + }); + expect(r.marker).toBeNull(); + expect(r.superseded).toBe('head-b'); + expect(r.pkilled).toBe(true); + }); + it('still decides when SALVAGE_DIR vanished before the head move (replayed watcher)', () => { // The deletion dual of the planted-FIFO hardening: SALVAGE_DIR is // agent-derivable and can be rm -rf'd (or chmod'd, or ENOSPC) mid-run. @@ -4461,7 +4528,11 @@ describe('review supersede salvage (#10110)', () => { const supersedeFile = join(dir, 'superseded'); // The watcher cedes mid-attempt (its pkill kills the qwen tree); // the attempt's post-run check sees the file and cedes clean. + // R7-1: the cede sites are AUTO_REVIEW-gated, so the lifecycle shape + // runs with the watcher armed; the explicit-run forgery dual below + // keeps AUTO_REVIEW=false and must fail loudly instead. const r = runScenario('supersede_mid_attempt', { + armWatcher: true, extraEnv: { SUPERSEDE_FILE: supersedeFile, EXPECTED_HEAD_SHA: 'head-a', @@ -4486,6 +4557,58 @@ describe('review supersede salvage (#10110)', () => { } }); + it('fails loudly when an explicit run meets a forged supersede file and a moved head (replayed loop)', () => { + const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); + try { + const supersedeFile = join(dir, 'superseded'); + // R7-1: an explicit run (/review, review_requested, dispatch) has no + // watcher and no queued replacement, so SUPERSEDE_FILE can only + // exist by forgery — the path derives from the exported marker's + // dirname. The exact shape that cedes green in a lifecycle run must + // burn its retry and fail red here: a silent green exit would + // suppress the maintainer-requested review with no replacement. + const r = runScenario('supersede_mid_attempt', { + extraEnv: { + SUPERSEDE_FILE: supersedeFile, + EXPECTED_HEAD_SHA: 'head-a', + STUB_LIVE_HEAD: 'head-b', + }, + }); + expect(r.attempts).toBe(2); + expect(r.status).toBe(1); + expect(r.raw).toContain('FAIL '); + expect(r.raw).not.toContain('Superseded early:'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('does not cede an explicit run to a supersede file forged during the retry backoff (replayed loop)', () => { + const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); + try { + const supersedeFile = join(dir, 'superseded'); + // R7-1's top-of-loop dual: the file lands in the backoff (the + // stubbed stand-in for a watcher cede) and the head reads moved, + // but an explicit run has no watcher and no queued replacement — + // the file can only be a forgery there, and attempt 2 must run + // instead of the loop exiting green on it. + const r = runScenario('supersede_during_backoff', { + extraEnv: { + SUPERSEDE_FILE: supersedeFile, + SUPERSEDE_DURING_BACKOFF: supersedeFile, + EXPECTED_HEAD_SHA: 'head-a', + STUB_LIVE_HEAD: 'head-b', + }, + }); + expect(r.attempts).toBe(2); + expect(r.status).toBe(0); + expect(r.line).toBe('OK outcome=success'); + expect(r.raw).not.toContain('Superseded early:'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('cedes instead of retrying when salvage armed but the attempt died (replayed loop)', () => { const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); try { @@ -4568,12 +4691,21 @@ describe('review supersede salvage (#10110)', () => { // the window where the watcher cedes against an empty process table // (pkill matches nothing — qwen is not running). The top-of-loop // re-check must stop attempt 2 re-reviewing the dead head. + // R7-1 gated the top-of-loop cede on AUTO_REVIEW. The armed watcher + // must stay deterministic: SLEEP_FAIL_ONLY_FIRST ends it before its + // first poll, the stubbed backoff remains the cede's stand-in, and + // the first gh call (the post-attempt re-check) answers the UNMOVED + // head so that check cannot preempt the top-of-loop site under test. const r = runScenario('supersede_during_backoff', { + armWatcher: true, extraEnv: { SUPERSEDE_FILE: supersedeFile, SUPERSEDE_DURING_BACKOFF: supersedeFile, EXPECTED_HEAD_SHA: 'head-a', + STUB_GH_COUNT: join(dir, 'gh-count'), + STUB_LIVE_HEAD_A1: 'head-a', STUB_LIVE_HEAD: 'head-b', + SLEEP_FAIL_ONLY_FIRST: '1', }, }); expect(r.attempts).toBe(1); @@ -4597,16 +4729,31 @@ describe('review supersede salvage (#10110)', () => { // watcher's cede. Without the pre-backoff reset the latch/artifact // survive the backoff (observed `present`) and a real watcher arms // the salvage marker instead of ceding. + // R7-1 gating: the same deterministic AUTO_REVIEW shape as the + // sibling backoff replay above — the watcher's first sleep fails, + // the first gh call answers the unmoved head, and the top-of-loop + // re-check takes the cede after the backoff reset is observed. const r = runScenario('compose_then_backoff_supersede', { + armWatcher: true, extraEnv: { SUPERSEDE_FILE: supersedeFile, SUPERSEDE_DURING_BACKOFF: supersedeFile, BACKOFF_OBS: obs, EXPECTED_HEAD_SHA: 'head-a', + STUB_GH_COUNT: join(dir, 'gh-count'), + STUB_LIVE_HEAD_A1: 'head-a', STUB_LIVE_HEAD: 'head-b', + SLEEP_FAIL_ONLY_FIRST: '1', }, }); - expect(readFileSync(obs, 'utf8').trim()).toBe('absent'); + // Every observation — the backoff itself and any watcher poll past + // it — must meet the reset state; a surviving latch or artifact + // would surface `present` on one of these lines. + const observations = readFileSync(obs, 'utf8') + .split('\n') + .filter(Boolean); + expect(observations.length).toBeGreaterThan(0); + expect(observations.every((l) => l === 'absent')).toBe(true); expect(r.attempts).toBe(1); expect(r.status).toBe(0); expect(r.raw).toContain('Superseded early:'); @@ -4871,6 +5018,39 @@ describe('review supersede salvage (#10110)', () => { } }); + it('cedes a killed attempt whose superseding departure was a normal push (replayed loop)', () => { + const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); + try { + const supersedeFile = join(dir, 'superseded'); + // R4-1: a normal fast-forward departure emits no force-push event, + // so the timeline carries ONLY the corrective back-push — the lone + // back-push the timeline witness rightly rejects (the loneBack arm + // above pins that refusal for a watcher-less run). Here the watcher + // saw the move itself: its kill record, written into the unexported + // minted dir, is the primary witness, and the killed attempt cedes + // clean instead of going red on a genuine supersede. + const now = new Date().toISOString(); + const r = runScenario('cede_revert_ff_kill', { + armWatcher: true, + extraEnv: { + SUPERSEDE_FILE: supersedeFile, + EXPECTED_HEAD_SHA: 'head-a', + STUB_GH_COUNT: join(dir, 'gh-count'), + STUB_LIVE_HEAD_A1: 'head-b', + STUB_LIVE_HEAD: 'head-a', + REPO: 'o/r', + STUB_TIMELINE: `head-x head-a ${now}`, + }, + }); + expect(r.attempts).toBe(1); + expect(r.status).toBe(0); + expect(r.raw).toContain('Superseded early:'); + expect(r.raw).not.toContain('FAIL '); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('relaunches a spent watcher and clears stale signals for a retry (replayed loop)', () => { const dir = mkdtempSync(join(tmpdir(), 'review-salvage-')); try { @@ -5235,4 +5415,132 @@ describe('review supersede salvage (#10110)', () => { 'if [ -n "$EVENT_HEAD_SHA" ] && [ -n "$current_head" ] && [ "$current_head" != "$EVENT_HEAD_SHA" ]; then', ); }); + + it('skips a delayed run whose live head already carries a posted bot review (replayed delay step)', () => { + // R5-4: with cancel-in-progress scoped to `closed`, an away-and-back + // push inside the watcher's poll gap leaves the in-flight run and the + // queued replacement both owing the SAME head — two full reviews of + // one commit. The delay job's re-check dedups on the machine-ledger + // anchor of an already-posted bot review. The bot login is pinned to + // the review-config constant, and every lookup failure must fail OPEN + // (a missed dedup costs one duplicate review; a false skip loses one). + const delay = doc.jobs['delay-automatic-review'].steps.find( + (s) => s.id === 'pr_state', + ); + expect(delay.run).toContain(`--arg bot "${botLogin}"`); + const H = 'a'.repeat(40); + const OTHER = 'b'.repeat(40); + const ledgerFor = (sha) => + `findings posted `; + const runDelayStep = ({ + currentHead = H, + eventHead = H, + reviews = [], + apiStatus = 0, + prState = 'OPEN', + }) => { + const dir = mkdtempSync(join(tmpdir(), 'review-delay-')); + try { + const bin = join(dir, 'bin'); + mkdirSync(bin); + // gh pr view answers the state/draft/head tsv; gh api runs the + // step's OWN --jq program (with the step's --arg values passed + // through) over the fixture reviews, so the replay exercises the + // real filter, not a paraphrase. + writeFileSync( + join(bin, 'gh'), + [ + '#!/bin/bash', + 'if [ "${1:-}" = "pr" ]; then', + ` printf '%s\\tfalse\\t%s\\n' "$STUB_PR_STATE" "$STUB_CURRENT_HEAD"`, + ' exit 0', + 'fi', + 'if [ "${1:-}" = "api" ]; then', + ' filter=""; args=()', + ' shift', + ' while [ $# -gt 0 ]; do', + ' case "$1" in', + ' --jq) filter="$2"; shift 2 ;;', + ' --arg) args+=(--arg "$2" "$3"); shift 3 ;;', + ' *) shift ;;', + ' esac', + ' done', + ` printf '%s' "$STUB_REVIEWS" | jq -r "\${args[@]}" "$filter"`, + ' exit "${STUB_API_STATUS}"', + 'fi', + 'exit 1', + ].join('\n') + '\n', + ); + chmodSync(join(bin, 'gh'), 0o755); + const out = join(dir, 'gho'); + const summary = join(dir, 'gss'); + writeFileSync(out, ''); + writeFileSync(summary, ''); + execFileSync('bash', ['-c', delay.run], { + encoding: 'utf8', + timeout: 30_000, + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + GITHUB_REPOSITORY: 'o/r', + PR_NUMBER: '7', + EVENT_HEAD_SHA: eventHead, + GITHUB_OUTPUT: out, + GITHUB_STEP_SUMMARY: summary, + STUB_PR_STATE: prState, + STUB_CURRENT_HEAD: currentHead, + STUB_REVIEWS: JSON.stringify(reviews), + STUB_API_STATUS: String(apiStatus), + }, + }); + return { + outputs: readFileSync(out, 'utf8'), + summary: readFileSync(summary, 'utf8'), + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + // A posted bot review anchored on the live head: nothing to do. + const deduped = runDelayStep({ + reviews: [{ user: { login: botLogin }, body: ledgerFor(H) }], + }); + expect(deduped.outputs).toContain('should_review=false'); + expect(deduped.summary).toContain( + 'already carries a posted automatic review', + ); + // An anchor on a DIFFERENT head (e.g. a salvaged historical-head + // review whose delta this run must still cover): proceed. + const otherAnchor = runDelayStep({ + reviews: [{ user: { login: botLogin }, body: ledgerFor(OTHER) }], + }); + expect(otherAnchor.outputs).toContain('should_review=true'); + // A ledger-shaped marker on a NON-bot review (anyone can submit a + // review on a public PR) must not suppress the automatic one. + const forged = runDelayStep({ + reviews: [{ user: { login: 'malicious-actor' }, body: ledgerFor(H) }], + }); + expect(forged.outputs).toContain('should_review=true'); + // A ledger whose sha did not survive (truncation, fail-closed round): + // no anchor, no skip. + const noSha = runDelayStep({ + reviews: [ + { + user: { login: botLogin }, + body: '', + }, + ], + }); + expect(noSha.outputs).toContain('should_review=true'); + // Lookup failure fails OPEN — the review runs. + const apiDown = runDelayStep({ apiStatus: 1 }); + expect(apiDown.outputs).toContain('should_review=true'); + // Controls: the pre-existing guards keep their shape. + expect( + runDelayStep({ currentHead: OTHER, eventHead: H }).outputs, + ).toContain('should_review=false'); + expect(runDelayStep({ prState: 'MERGED' }).outputs).toContain( + 'should_review=false', + ); + }); }); From c87061393351c591970ffa0a35e4691be614d273 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 28 Aug 2026 17:40:09 +0000 Subject: [PATCH 09/11] fix(ci): bound salvage-signal reads and authenticate the delay-step dedup (#10123) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the round-8/9 review Criticals: - The delay-step head-level dedup called `gh api --arg`, a flag gh does not have, so the lookup died at flag parse and the dedup was dead code. Drop the flag and interpolate the constant bot login into the --jq filter. - The dedup parsed the FIRST `"sha":"…"` hit out of model-authored review bodies, a forgeable anchor. Parse only the LAST ledger marker per body (the CLI's parseLedger convention) and authenticate the sha against the review's server-recorded commit_id before skipping; every lookup trouble still fails open. - The revert-cede accepted the watcher's kill record alone, but the record's dir is enumerable under RUNNER_TEMP — non-export stops derivation, not discovery by the same-uid agent. Accept the record only corroborated by a server-side back-push onto the expected head; the timeline pair stays the fallback for a lost record. - The salvage-signal reads gated on [ -f ] and then reopened agent- derivable paths with unbounded cats: a FIFO rename-swapped into the check-then-open window wedged the watcher, the posting path, or the cede forever, and a huge plant could be slurped whole. Each read is now one `timeout 5 head -c N` open; the posting path fails closed. - The replay harnesses supply a bound-enforcing timeout(1) stub (macOS lanes ship none), and the gh stub rejects unknown flags like real gh so the --arg fiction cannot be re-emulated. New replays witness each fix (forged kill record, forged/first markers, commit_id mismatch, rename-swapped FIFOs, huge plants); mutation probes confirmed every witness turns red when its guard is removed. The workflow's .size-baseline ratchet is bumped for the real growth. --- .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-code-pr-review.yml | 87 ++- scripts/tests/qwen-pr-review-workflow.test.js | 579 +++++++++++++----- scripts/tests/qwen-resolve-workflow.test.js | 101 ++- 4 files changed, 595 insertions(+), 174 deletions(-) diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index 4b5c766e831..2de8c07daf7 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -36,7 +36,7 @@ 5942 qwen-autofix-fork-signal.yml 432155 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml -197929 qwen-code-pr-review.yml +203437 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml 20525 qwen-issue-followup-bot.yml 5760 qwen-pr-safety-precheck.yml diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index af53ebfa080..7139f43868c 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -282,9 +282,18 @@ jobs: # run has nothing to do. Fail-open: any lookup trouble falls # through to the review — a missed dedup costs one duplicate # review, a false skip would lose one. + # The marker sha is body text the reviewed agent authors, so it + # never dedups alone: only the LAST ledger marker per body + # counts (compose appends the genuine one last — the CLI's + # parseLedger reads the last marker for the same reason), and it + # must equal the review's server-recorded commit_id. A salvaged + # historical-head review posts under a NEWER commit_id, so it + # never dedups the live head here. gh's --jq accepts a single + # expression only (no jq argument flags — fleet-shepherd + # documents the same limit), so the constant bot login is + # interpolated. reviewed_shas="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate \ - --arg bot "qwen-code-ci-bot" \ - --jq '.[] | select((.user.login // "") == $bot) | (.body // "") | select(contains("`; - const runDelayStep = ({ - currentHead = H, - eventHead = H, - reviews = [], - apiStatus = 0, - prState = 'OPEN', - }) => { - const dir = mkdtempSync(join(tmpdir(), 'review-delay-')); - try { - const bin = join(dir, 'bin'); - mkdirSync(bin); - // gh pr view answers the state/draft/head tsv; gh api runs the - // step's OWN --jq program (with the step's --arg values passed - // through) over the fixture reviews, so the replay exercises the - // real filter, not a paraphrase. - writeFileSync( - join(bin, 'gh'), - [ - '#!/bin/bash', - 'if [ "${1:-}" = "pr" ]; then', - ` printf '%s\\tfalse\\t%s\\n' "$STUB_PR_STATE" "$STUB_CURRENT_HEAD"`, - ' exit 0', - 'fi', - 'if [ "${1:-}" = "api" ]; then', - ' filter=""; args=()', - ' shift', - ' while [ $# -gt 0 ]; do', - ' case "$1" in', - ' --jq) filter="$2"; shift 2 ;;', - ' --arg) args+=(--arg "$2" "$3"); shift 3 ;;', - ' *) shift ;;', - ' esac', - ' done', - ` printf '%s' "$STUB_REVIEWS" | jq -r "\${args[@]}" "$filter"`, - ' exit "${STUB_API_STATUS}"', - 'fi', - 'exit 1', - ].join('\n') + '\n', - ); - chmodSync(join(bin, 'gh'), 0o755); - const out = join(dir, 'gho'); - const summary = join(dir, 'gss'); - writeFileSync(out, ''); - writeFileSync(summary, ''); - execFileSync('bash', ['-c', delay.run], { - encoding: 'utf8', - timeout: 30_000, - env: { - ...process.env, - PATH: `${bin}:${process.env.PATH}`, - GITHUB_REPOSITORY: 'o/r', - PR_NUMBER: '7', - EVENT_HEAD_SHA: eventHead, - GITHUB_OUTPUT: out, - GITHUB_STEP_SUMMARY: summary, - STUB_PR_STATE: prState, - STUB_CURRENT_HEAD: currentHead, - STUB_REVIEWS: JSON.stringify(reviews), - STUB_API_STATUS: String(apiStatus), + // The stub below answers the dedup lookup by running the step's OWN --jq + // program with real jq — the filter IS the thing under test — so skip + // honestly on hosts without jq (the suite's hasJq convention) instead of + // reporting the dedup broken there. + const delayStepHasJq = (() => { + try { + execFileSync('jq', ['--version'], { stdio: 'ignore' }); + return true; + } catch { + return false; + } + })(); + + it.skipIf(!delayStepHasJq)( + 'skips a delayed run whose live head already carries a posted bot review (replayed delay step)', + () => { + // R5-4: with cancel-in-progress scoped to `closed`, an away-and-back + // push inside the watcher's poll gap leaves the in-flight run and the + // queued replacement both owing the SAME head — two full reviews of + // one commit. The delay job's re-check dedups on the machine-ledger + // anchor of an already-posted bot review. The bot login is pinned to + // the review-config constant, and every lookup failure must fail OPEN + // (a missed dedup costs one duplicate review; a false skip loses one). + const delay = doc.jobs['delay-automatic-review'].steps.find( + (s) => s.id === 'pr_state', + ); + // gh's --jq takes no --arg (fleet-shepherd documents the same + // limit), so the constant bot login is interpolated — and the stub + // below rejects any unknown flag exactly like real gh. + expect(delay.run).toContain(`== "${botLogin}"`); + expect(delay.run).not.toContain('--arg'); + const H = 'a'.repeat(40); + const OTHER = 'b'.repeat(40); + const ledgerFor = (sha) => + `findings posted `; + const runDelayStep = ({ + currentHead = H, + eventHead = H, + reviews = [], + apiStatus = 0, + prState = 'OPEN', + }) => { + const dir = mkdtempSync(join(tmpdir(), 'review-delay-')); + try { + const bin = join(dir, 'bin'); + mkdirSync(bin); + // gh pr view answers the state/draft/head tsv; gh api runs the + // step's OWN --jq program over the fixture reviews, so the replay + // exercises the real filter, not a paraphrase. Faithful to real gh + // on flags: --arg is a jq flag, not a gh flag — real gh dies on it + // at flag parse, before any request, and a stub that emulated it + // let the dead dedup ship green for a round (R9-1). + writeFileSync( + join(bin, 'gh'), + [ + '#!/bin/bash', + 'if [ "${1:-}" = "pr" ]; then', + ` printf '%s\\tfalse\\t%s\\n' "$STUB_PR_STATE" "$STUB_CURRENT_HEAD"`, + ' exit 0', + 'fi', + 'if [ "${1:-}" = "api" ]; then', + ' shift', + ' filter=""', + ' while [ $# -gt 0 ]; do', + ' case "$1" in', + ' --jq) filter="$2"; shift 2 ;;', + ' --paginate) shift ;;', + ' --*) echo "unknown flag: $1" >&2; exit 1 ;;', + ' *) shift ;;', + ' esac', + ' done', + ` printf '%s' "$STUB_REVIEWS" | jq -r "$filter"`, + ' exit "${STUB_API_STATUS}"', + 'fi', + 'exit 1', + ].join('\n') + '\n', + ); + chmodSync(join(bin, 'gh'), 0o755); + const out = join(dir, 'gho'); + const summary = join(dir, 'gss'); + writeFileSync(out, ''); + writeFileSync(summary, ''); + execFileSync('bash', ['-c', delay.run], { + encoding: 'utf8', + timeout: 30_000, + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + GITHUB_REPOSITORY: 'o/r', + PR_NUMBER: '7', + EVENT_HEAD_SHA: eventHead, + GITHUB_OUTPUT: out, + GITHUB_STEP_SUMMARY: summary, + STUB_PR_STATE: prState, + STUB_CURRENT_HEAD: currentHead, + STUB_REVIEWS: JSON.stringify(reviews), + STUB_API_STATUS: String(apiStatus), + }, + }); + return { + outputs: readFileSync(out, 'utf8'), + summary: readFileSync(summary, 'utf8'), + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + // A posted bot review anchored on the live head: nothing to do. The + // marker sha must also match the review's server-recorded commit_id — + // the body is model-authored text and never dedups alone. + const deduped = runDelayStep({ + reviews: [ + { user: { login: botLogin }, body: ledgerFor(H), commit_id: H }, + ], + }); + expect(deduped.outputs).toContain('should_review=false'); + expect(deduped.summary).toContain( + 'already carries a posted automatic review', + ); + // An anchor on a DIFFERENT head (e.g. a salvaged historical-head + // review whose delta this run must still cover): proceed — marker sha + // and commit_id agree with each other, but not with the live head. + const otherAnchor = runDelayStep({ + reviews: [ + { + user: { login: botLogin }, + body: ledgerFor(OTHER), + commit_id: OTHER, }, - }); - return { - outputs: readFileSync(out, 'utf8'), - summary: readFileSync(summary, 'utf8'), - }; - } finally { - rmSync(dir, { recursive: true, force: true }); - } - }; - // A posted bot review anchored on the live head: nothing to do. - const deduped = runDelayStep({ - reviews: [{ user: { login: botLogin }, body: ledgerFor(H) }], - }); - expect(deduped.outputs).toContain('should_review=false'); - expect(deduped.summary).toContain( - 'already carries a posted automatic review', - ); - // An anchor on a DIFFERENT head (e.g. a salvaged historical-head - // review whose delta this run must still cover): proceed. - const otherAnchor = runDelayStep({ - reviews: [{ user: { login: botLogin }, body: ledgerFor(OTHER) }], - }); - expect(otherAnchor.outputs).toContain('should_review=true'); - // A ledger-shaped marker on a NON-bot review (anyone can submit a - // review on a public PR) must not suppress the automatic one. - const forged = runDelayStep({ - reviews: [{ user: { login: 'malicious-actor' }, body: ledgerFor(H) }], - }); - expect(forged.outputs).toContain('should_review=true'); - // A ledger whose sha did not survive (truncation, fail-closed round): - // no anchor, no skip. - const noSha = runDelayStep({ - reviews: [ - { - user: { login: botLogin }, - body: '', - }, - ], - }); - expect(noSha.outputs).toContain('should_review=true'); - // Lookup failure fails OPEN — the review runs. - const apiDown = runDelayStep({ apiStatus: 1 }); - expect(apiDown.outputs).toContain('should_review=true'); - // Controls: the pre-existing guards keep their shape. - expect( - runDelayStep({ currentHead: OTHER, eventHead: H }).outputs, - ).toContain('should_review=false'); - expect(runDelayStep({ prState: 'MERGED' }).outputs).toContain( - 'should_review=false', - ); - }); + ], + }); + expect(otherAnchor.outputs).toContain('should_review=true'); + // A ledger-shaped marker on a NON-bot review (anyone can submit a + // review on a public PR) must not suppress the automatic one. + const forged = runDelayStep({ + reviews: [{ user: { login: 'malicious-actor' }, body: ledgerFor(H) }], + }); + expect(forged.outputs).toContain('should_review=true'); + // A forged marker sha naming the live head must not manufacture the + // false skip this gate exists to prevent: the sha is authenticated + // against the review's server-recorded commit_id (R5-4). + const mismatchedCommit = runDelayStep({ + reviews: [ + { user: { login: botLogin }, body: ledgerFor(H), commit_id: OTHER }, + ], + }); + expect(mismatchedCommit.outputs).toContain('should_review=true'); + // Only the LAST ledger marker per body counts (compose appends the + // genuine one last — the CLI's parseLedger reads the last for the same + // reason): a forged marker PLACED BEFORE the genuine one must not win + // the capture and skip the live head's review. + const forgedFirstMarker = runDelayStep({ + reviews: [ + { + user: { login: botLogin }, + body: `${ledgerFor(H)}\n\n${ledgerFor(OTHER)}`, + commit_id: OTHER, + }, + ], + }); + expect(forgedFirstMarker.outputs).toContain('should_review=true'); + // Control for the direction above: genuine marker last, authenticated — + // the dedup still skips. + const genuineLast = runDelayStep({ + reviews: [ + { + user: { login: botLogin }, + body: `${ledgerFor(OTHER)}\n\n${ledgerFor(H)}`, + commit_id: H, + }, + ], + }); + expect(genuineLast.outputs).toContain('should_review=false'); + // A ledger whose sha did not survive (truncation, fail-closed round): + // no anchor, no skip. + const noSha = runDelayStep({ + reviews: [ + { + user: { login: botLogin }, + body: '', + commit_id: H, + }, + ], + }); + expect(noSha.outputs).toContain('should_review=true'); + // Lookup failure fails OPEN — the review runs. + const apiDown = runDelayStep({ apiStatus: 1 }); + expect(apiDown.outputs).toContain('should_review=true'); + // Controls: the pre-existing guards keep their shape. + expect( + runDelayStep({ currentHead: OTHER, eventHead: H }).outputs, + ).toContain('should_review=false'); + expect(runDelayStep({ prState: 'MERGED' }).outputs).toContain( + 'should_review=false', + ); + }, + ); }); diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 23b4a08ee79..eaaa0e1629d 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -7,6 +7,7 @@ import { chmodSync, existsSync, + mkdirSync, mkdtempSync, readFileSync, rmSync, @@ -55,6 +56,40 @@ function reviewGhWrapper(runStep) { return runStep.slice(bodyStart, end).replace(/^ {10}/gm, ''); } +// A timeout(1) stub that ENFORCES the bound: the wrapper's salvage marker +// read is a `timeout 5 head -c 128` open, and a lane without GNU coreutils +// (macOS) ships no timeout(1). A bare pass-through is not sufficient: a +// rename-swapped FIFO then blocks the open forever (R8-10). +function boundedTimeoutStub() { + const js = + 'const [dur, ...cmd] = process.argv.slice(1);' + + 'const ms = Math.max(0, Number.parseFloat(dur) || 0) * 1000;' + + 'const child = require("child_process").spawn(cmd[0], cmd.slice(1), { stdio: "inherit" });' + + 'let killed = false;' + + 'const timer = setTimeout(() => { killed = true; try { child.kill("SIGKILL"); } catch (e) {} }, ms);' + + 'child.on("exit", (code, signal) => { clearTimeout(timer); process.exit(killed ? 124 : code === null ? (signal ? 137 : 1) : code); });'; + return `#!/bin/bash\nexec "${process.execPath}" -e '${js}' "$@"\n`; +} + +// A reader stub (installed as head AND cat) that rename-swaps a FIFO onto +// its target at open time — the window [ -f ] cannot refuse — then blocks +// like a real open (no writer). Only a timeout bound resolves the read; +// shimming both readers keeps the wedge red for a regression back to a +// bare `cat`. +function swapAtOpenStub() { + return ( + [ + '#!/bin/bash', + 'for last in "$@"; do :; done', + 'if [ -n "$last" ] && [ -f "$last" ]; then', + ' rm -f "$last"', + ' mkfifo "$last"', + 'fi', + 'exec cat "$last"', + ].join('\n') + '\n' + ); +} + function runReviewGhWrapper( runStep, args, @@ -63,8 +98,10 @@ function runReviewGhWrapper( expectedHead = 'head-a', // salvageContent: when set, a salvage marker file with that content is // created and exported as QWEN_CI_REVIEW_SALVAGE_OK_FILE — the supersede - // watcher's past-threshold pin (#10110). - { salvageContent } = {}, + // watcher's past-threshold pin (#10110). salvageFifo plants a static + // FIFO at the marker; swapSalvageOnRead rename-swaps one in at the + // read's open — the window [ -f ] cannot refuse (R8-10). + { salvageContent, salvageFifo = false, swapSalvageOnRead = false } = {}, ) { const tempDir = mkdtempSync(path.join(tmpdir(), 'qwen-review-gh-')); try { @@ -72,7 +109,10 @@ function runReviewGhWrapper( const realGhPath = path.join(tempDir, 'real-gh'); const ghLogPath = path.join(tempDir, 'gh.log'); let salvagePath = ''; - if (salvageContent !== undefined) { + if (salvageFifo) { + salvagePath = path.join(tempDir, 'salvage-ok'); + spawnSync('mkfifo', [salvagePath]); + } else if (salvageContent !== undefined) { salvagePath = path.join(tempDir, 'salvage-ok'); writeFileSync(salvagePath, salvageContent); } @@ -92,11 +132,29 @@ function runReviewGhWrapper( writeFileSync(ghLogPath, ''); chmodSync(wrapperPath, 0o755); chmodSync(realGhPath, 0o755); + // The marker read is a bounded `timeout 5 head -c 128` open: give the + // wrapper a bound-enforcing timeout(1) on every lane (macOS ships + // none) — and the swap head stub when the wedge arm is requested. + const binDir = path.join(tempDir, 'bin'); + mkdirSync(binDir); + writeFileSync(path.join(binDir, 'timeout'), boundedTimeoutStub()); + chmodSync(path.join(binDir, 'timeout'), 0o755); + if (swapSalvageOnRead) { + for (const name of ['head', 'cat']) { + writeFileSync(path.join(binDir, name), swapAtOpenStub()); + chmodSync(path.join(binDir, name), 0o755); + } + } const result = spawnSync(wrapperPath, args, { encoding: 'utf8', + // A regression that unbounds the marker read must turn the suite + // RED on the harness bound, not hang it: spawnSync kills the child + // at 30s and the status assertions fail on the missing exit. + timeout: 30_000, env: { ...process.env, + PATH: `${binDir}:${process.env.PATH}`, FAKE_GH_LOG: ghLogPath, FAKE_HEAD_SHA: currentHead, FAKE_PR_STATE: prState, @@ -704,6 +762,43 @@ describe('qwen resolve workflow', () => { expect(closedSalvage.ghLog).toBe(''); }); + it('bounds the salvage marker read on the posting path (#10110)', () => { + const runStep = step(reviewJob, 'Run review'); + // R8-10: the escape's marker read is one timeout-bounded, size-capped + // open. A FIFO rename-swapped in at open time — or planted statically — + // must fail CLOSED to the block inside the bound: an unbounded open + // wedges the posting path forever, the attempt budget bleeds out, and + // the salvage-armed cede then discards a finished review with no + // failure signal at all. + const wedged = runReviewGhWrapper( + runStep, + ['api', 'repos/owner/repo/pulls/123/reviews', '--input', 'review.json'], + 'OPEN', + 'head-b', + 'head-a', + { salvageContent: 'head-a', swapSalvageOnRead: true }, + ); + expect(wedged.status).toBe(90); + expect(wedged.stderr).toContain( + 'Blocked PR write: PR #123 moved from head-a to head-b', + ); + expect(wedged.ghLog).toBe(''); + + const fifo = runReviewGhWrapper( + runStep, + ['api', 'repos/owner/repo/pulls/123/reviews', '--input', 'review.json'], + 'OPEN', + 'head-b', + 'head-a', + { salvageFifo: true }, + ); + expect(fifo.status).toBe(90); + expect(fifo.stderr).toContain( + 'Blocked PR write: PR #123 moved from head-a to head-b', + ); + expect(fifo.ghLog).toBe(''); + }); + it('allows wrapped gh review writes when the PR is still current', () => { const runStep = step(reviewJob, 'Run review'); const currentSummary = runReviewGhWrapper( From b7162c8e5f94ba89ec67245b0ad5ce1b758d6391 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 29 Aug 2026 04:13:00 +0000 Subject: [PATCH 10/11] fix(ci): cap the compose-latch read, poison-proof cleanups, and authenticate the cede/skip witnesses (#10123) --- .github/workflows/qwen-code-pr-review.yml | 59 ++- scripts/tests/qwen-pr-review-workflow.test.js | 394 +++++++++++++----- scripts/tests/qwen-resolve-workflow.test.js | 92 ++-- 3 files changed, 390 insertions(+), 155 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 9e756a4b886..4cc1cde62b2 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -261,6 +261,7 @@ jobs: name: 'qwen-pr-review-delay' deployment: false permissions: + actions: 'read' contents: 'read' pull-requests: 'read' outputs: @@ -312,18 +313,29 @@ jobs: # never dedups alone: only the LAST ledger marker per body # counts (compose appends the genuine one last — the CLI's # parseLedger reads the last marker for the same reason), and it - # must equal the review's server-recorded commit_id. A salvaged - # historical-head review posts under a NEWER commit_id, so it - # never dedups the live head here. gh's --jq accepts a single - # expression only (no jq argument flags — fleet-shepherd - # documents the same limit), so the constant bot login is - # interpolated. + # must equal the review's commit_id. Both are CALLER-SUPPLIED on + # POST reviews, and the reviewed agent holds the run's bot token, + # so a matching pair dedups only corroborated by a state the + # agent cannot author — a successful run of THIS workflow, + # server-recorded with the candidate as its event head (workflow + # runs have no create API). A salvaged historical-head review + # posts under a NEWER commit_id, so it never dedups the live head + # here. Fail-open like the lookup above: a lost corroboration + # costs one duplicate review, a false skip loses one. gh's --jq + # accepts a single expression only (no jq argument flags — + # fleet-shepherd documents the same limit), so the constant bot + # login is interpolated. reviewed_shas="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate \ --jq '.[] | select((.user.login // "") == "qwen-code-ci-bot") | ((.body // "") | [match("', - commit_id: H, - }, - ], - }); - expect(noSha.outputs).toContain('should_review=true'); - // Lookup failure fails OPEN — the review runs. - const apiDown = runDelayStep({ apiStatus: 1 }); - expect(apiDown.outputs).toContain('should_review=true'); - // R5-4 strengthened: the reviewed agent holds the run's bot token - // and both compared values are caller-supplied on POST reviews, so - // an agent-forged bot review can pair marker sha and commit_id on - // the live head. The pair alone must never skip: without a - // corroborating successful run at that head the review runs - // (removing the corroboration turns this arm red). - const uncorroborated = runDelayStep({ - reviews: [ - { user: { login: botLogin }, body: ledgerFor(H), commit_id: H }, - ], - }); - expect(uncorroborated.outputs).toContain('should_review=true'); - // A successful run of some OTHER workflow at the head proves - // nothing about this workflow's review. - const wrongWorkflow = runDelayStep({ - reviews: [ - { user: { login: botLogin }, body: ledgerFor(H), commit_id: H }, - ], - runPaths: ['.github/workflows/ci.yml'], - }); - expect(wrongWorkflow.outputs).toContain('should_review=true'); - // A failed corroboration lookup fails OPEN like the reviews lookup. - const runsDown = runDelayStep({ + // The attack shape: an anchored bot review (marker sha == commit_id + // == live head) corroborated by a green run of this workflow — the + // exact shape a ceded, delay-skipped, or docs-only run records + // without any posting. The pre-removal step skipped here + // (should_review=false) — the permanent suppression R13-1 proved; + // the head is reviewed instead, at the cost of one possible + // duplicate review. + const attackShape = runDelayStep({ reviews: [ { user: { login: botLogin }, body: ledgerFor(H), commit_id: H }, ], runPaths: ['.github/workflows/qwen-code-pr-review.yml'], - runsStatus: 1, }); - expect(runsDown.outputs).toContain('should_review=true'); - // Controls: the pre-existing guards keep their shape. + expect(attackShape.outputs).toContain('should_review=true'); + expect(attackShape.summary).not.toContain('already carries'); + // Controls: the state guards keep their shape. expect( runDelayStep({ currentHead: OTHER, eventHead: H }).outputs, ).toContain('should_review=false');