fix(ci): surface blocked autofix takeover admission - #8410

Code review

Reviewed ea34938 (gh pr diff 8410) against the PR head of .github/workflows/qwen-autofix.yml. I replayed the new classifier and the retry helpers locally rather than reading them only.

What I verified independently

• forced_admission_reason is behaviourally equivalent to the old OK predicate. I extracted it verbatim and ran it: skip at label index 0 → skip_label (jq treats 0 as truthy, so the old index($skip) | not was already correct and the new != null matches), bot in-repo → eligible, missing isCrossRepository → cross_repo_state_missing, fork without allow-edits → maintainer_edits_disabled. No admission widening.
• The whole review-scan run block is syntactically valid — extracted and dedented, bash -n is clean.
• The [[ cond ]] && sleep idiom in both retry loops is safe under -eo pipefail (defaults.run.shell: bash). I confirmed the final iteration's non-zero &&-list does not trip errexit and the loop falls through to return 1.
• FORCED_PR never reaches this step without a PAT — the fork pull_request:labeled path is already guarded at the route (L516-L526), so the new exit 1 cannot produce the "red run for a label that never engaged anything" failure that comment warns about.

Nice touches: the explicit status_lookup_ok flag is strictly better than the existing status writer at L3466, which conflates "lookup failed" with "no comment exists" via || STATUS_ID=''. And the two new fleet_row calls in the scheduled fork loop close a genuine visibility hole.

Suggestions

1. The blocked comment tells maintainers to wait, for blockers that will never clear (L1779-L1785)

Only maintainer_edits_disabled gets specific guidance. author_permission_read / author_permission_triage and cross_repo_state_missing fall into the else branch and render "A later scheduled scan will retry without advancing the feedback watermark." But the scheduled scan applies the same write+ gate (L1923-L1933), so it will skip the PR on every tick forever. The comment then sits on the PR indefinitely promising a retry that structurally cannot succeed — which is the same class of misleading-status problem this PR sets out to fix.

Worth a third branch for author_permission_*: grant the fork author write+, or drop autofix/takeover.

2. The bot's own fork PRs get no blocked status at all (L1769)

[[ "$(jq -r --arg take "${TAKEOVER_LABEL}" '[.labels[]?.name] | index($take) != null' <<< "${META}")" == 'true' ]] || return 0

The workflow deliberately manages the bot's own fork PRs without a label (header L24-L29: "The bot's OWN fork PRs … are auto-managed WITHOUT a label when allow-edits is on"), and bot-prs.json feeds the fork loop at L1917 for exactly that reason. So when a bot-authored fork PR has allow-edits turned off, or its permission read fails, admission still stops silently — the original bug, unfixed for that class. Gating on "managed" (author == AUTOFIX_BOT or takeover label) rather than the label alone would cover both, and matches how forced_admission_reason itself defines unmanaged_author.

3. Terminal API answers are retried and then reported as transient (L1740-L1752, L1717-L1738)

read_live_permission treats every outcome outside admin|maintain|write|triage|read as retryable. That folds in answers that are terminal, not transient:

• "permission": "none" — returned for a user with no access (private repos, blocked users). Verified: on this public repo a non-collaborator returns read, so the common case is fine, but none is a documented value of that field.
• HTTP 404 — verified live: gh api repos/QwenLM/qwen-code/collaborators/<gone>/permission → "… is not a user" (HTTP 404), non-zero exit. A deleted or renamed fork author hits this.

Either produces 3 sleeps, ADMISSION_REASON=permission_lookup_failed, the "will retry" comment from #1 above, and exit 1 (red run) — where the old code produced a clean green terminal rejection. read_forced_pr_meta's schema gate has the same shape: a successful response that fails the structure check (e.g. author: null for a ghost author) is not transient, yet it retries three times and hard-exits.

Splitting "the call failed" (retry) from "the call answered and the answer is terminal" (classify, exit 0) would keep the fail-loud property where it belongs.

4. Both readers discard the API error (2> /dev/null, L1721/L1730/L1743)

The stated goal is making blockers visible, but the only trace left is attempt N/3 — a maintainer debugging a red forced scan cannot tell 404 from 403 from a secondary-rate-limit. This file already has the better pattern twice, in the route job (L470-L490): capture stderr to a temp file and echo it in the warning (::warning::Permission API call failed for ${SENDER_LOGIN}: ${api_error}). Copying it here costs three lines.

5. cross_repo_state_missing is unreachable in production (L1761)

read_forced_pr_meta already rejects a non-boolean .isCrossRepository (L1728), so a forced dispatch with that field missing dies as metadata_fetch_failed before the classifier runs. The new test asserts the branch, which reads as coverage of the documented jq //-trap but exercises a path the workflow can no longer reach. Either drop the field from the schema check (so malformed metadata classifies cleanly instead of erroring) or drop the branch — keeping both makes the fail-closed guarantee look like it lives somewhere it doesn't.

6. jq program interpolation deviates from this file's own convention (L1791)

--jq ".[] | select(.user.login == \"${AUTOFIX_BOT}\") | select(.body | contains(\"<!-- autofix-status -->\")) | .id"

The identical lookup at L3466-L3470 uses --arg ab "${AUTOFIX_BOT}". AUTOFIX_BOT is vars.AUTOFIX_BOT_LOGIN || 'qwen-code-dev-bot', so this is repo-var controlled and low risk, but a " or \ in that var silently rewrites the filter. --arg is free.

7. Dead ${FPERM:-none} fallbacks (L1861, L1931)

After read_live_permission, FPERM is one of the five regex values on success and the empty case has already exited/continued, so author_permission_${FPERM:-none} can never render none at either call site. The reason vocabulary is effectively author_permission_read|author_permission_triage. Harmless, but it reads as if the none case were handled — see #3.

8. Outputs written immediately before exit 1 (L1827-L1828, L1852-L1853, L1869/L1874)

targets=[] / has_targets=false are appended and then the step exits non-zero. review-address requires has_targets == 'true' and the issue-phase job requires needs.review-scan.result == 'success', so neither consumer can observe them on a failed job. Not a bug — but if this is deliberate belt-and-braces it's worth one line of comment, otherwise it's noise in three places.

Test coverage

The behavioural harness is a real step up from string assertions, and neutralising sleep in the fixtures is the right call. Gaps worth closing, given the PR leans on "behaviorally verifies":

• No end-to-end case for a successful-but-below-write permission. The author_permission_* branch (L1860-L1863) and the blocked-comment text it produces — the most likely real rejection — are never exercised. This is also the case that would surface pre-release: fix ci #1.
• No case for permission: "none" or a 404 from the permission endpoint (see 如何自定义密钥文件 .env可能与其他文件冲突 #3).
• The reporter's two early return 0 paths are untested: non-takeover-label (the silent path, Where is the config saved? #2) and a non-matching reason. A regression there re-introduces silence with green tests.
• actor != AUTOFIX_BOT → return 1 is untested, though it's one of only two ways the terminal path turns red.
• The PATCH-failure path is untested — the fixture only fails the lookup (FAIL_STATUS_LOOKUP); Failed to update blocked takeover status never runs.
• expect(reviewScanJob).toContain('exit 1') is vacuous — exit 1 appears throughout a 1000-line script. Either anchor it to the surrounding block or drop it.
• The fake gh interpolates successOutput into a single-quoted bash string; this only works because the fixture JSON happens to contain no '. A heredoc or an env var would remove the trap for the next person editing the fixture.

Summary

No Critical. The classifier rewrite is sound and I could not find an admission-widening path. The substantive concerns are all on the new reporting surface rather than the gate: #1 and #2 mean the "make terminal blockers visible" promise is only partly delivered (wrong guidance for permission blockers, no coverage for the bot's own fork PRs), and #3/#4 mean some terminal conditions are laundered into transient-looking red runs with no diagnostic in the log. Those three plus the author_permission_* test case are what I'd fix before landing; the rest are nits.

Review: fix(ci): surface blocked autofix takeover admission

Overview

The forced (FORCED_PR) branch of review-scan is refactored from a single boolean OK predicate into three shell helpers plus a jq classifier:

• read_forced_pr_meta — gh pr view with response-shape validation and 3 bounded retries (replaces || echo '{}').
• read_live_permission — collaborator permission read with the same retry shape; also reused by the scheduled fork-candidate loop.
• forced_admission_reason — replaces the boolean with stable reason codes (not_open, wrong_base, skip_label, unmanaged_author, maintainer_edits_disabled, cross_repo_state_missing, eligible).
• report_forced_takeover_blocked — PATCHes (or posts) the <!-- autofix-status --> comment with a bilingual ⛔ blocked notice for terminal reasons on managed PRs.

Plus a shared qwen-pr-head-write-<pr> concurrency group with review-address, a fleet_row for blocked fork candidates, and ~310 lines of behavioural regression coverage.

The direction is right, and the test approach — extracting the shell VERBATIM and replaying it against a stub gh on PATH — is genuinely strong. Three things I'd want addressed before merge.

🔴 1. permission: "none" is misclassified as a transient lookup failure

read_live_permission accepts only admin|maintain|write|triage|read:

&& [[ "${permission}" =~ ^(admin|maintain|write|triage|read)$ ]]; then

But GET /repos/{owner}/{repo}/collaborators/{username}/permission returns 200 with permission: "none" for a user who is not a collaborator — which is precisely the common case for a fork author on a autofix/takeover PR. "none" fails the regex, so the helper retries 3× and returns 1.

Confirmed by replaying the extracted function against a stub gh that prints none:

::warning::Permission lookup failed for outsider (attempt 1/3)
::warning::Permission lookup failed for outsider (attempt 2/3)
::warning::Permission lookup failed for outsider (attempt 3/3)
RC=1

Consequences, all regressions against main:

• Forced path (L1874-1882): a fork author with no write access now yields permission_lookup_failed → exit 1 (red run), and the posted notice says "A later scheduled scan will retry without advancing the feedback watermark" — wrong guidance, the condition is terminal. On main this printed permission='none' below write and exited 0.
• The author_permission_${FPERM:-none} branch at L1888 and its correct "Grant the fork author write access, or remove the autofix/takeover label" message become unreachable — FPERM can now only be triage or read when that case is reached.
• Scheduled path (L1946): same misclassification, so every non-privileged takeover fork reports blocked / permission_lookup_failed instead of author_permission_none, and costs 3 API calls + 3s of sleep on every scan tick. ${FPERM:-none} at L1958 is likewise dead.

Suggested fix — separate "the call failed" from "the call answered":

if permission="$(gh api "repos/${REPO}/collaborators/${login}/permission" --jq '.permission // ""' 2> /dev/null)" \
  && [[ -n "${permission}" ]]; then
  printf '%s' "${permission}"
  return 0
fi

Any non-empty documented value (including none) is a real answer; only a non-zero gh exit or an empty/malformed payload is transient. Please add a none case to the regression test — its absence is exactly why this slipped through.

🟠 2. Sharing the head-write lock can queue or silently cancel a forced scan

concurrency:
  group: 'qwen-pr-head-write-${{ needs.route.outputs.pr_number || github.run_id }}'
  cancel-in-progress: false

review-address holds this same group with timeout-minutes: 300 and max-parallel legs. Two consequences:

• review-scan's own timeout-minutes: 15 does not cover queue time, so a forced dispatch on a PR with an address run in flight can now sit pending for up to 5 hours instead of running immediately. On main it ran, hit the busy check, and emitted fleet_row … 'busy' 'address run in flight' — a fast, visible answer.
• GitHub keeps at most one pending entry per concurrency group; a newer entrant cancels the previously-pending one even with cancel-in-progress: false. So a second forced dispatch (or the next review-address matrix leg entering the group) can cancel the queued review-scan → job cancelled, review-address skipped, no comment, no error. That is the same silent no-op failure class this PR exists to eliminate.

The race being defended against is narrow: the blocked-notice PATCH is the only new write, and it only happens on terminal admission failure. Options worth considering instead of a job-wide lock:

1. Accept the race and make the write best-effort (consistent with review-address's own status writes — see 如何自定义密钥文件 .env可能与其他文件冲突 #3), or
2. Re-read the comment immediately before PATCHing and skip if it now carries an in-flight 🔄 AutoFix is working marker from a newer run.

If the job-level group is kept, please at least note in the comment that the scan may queue behind a 300-minute address job.

🟠 3. Fail-closed severity is inconsistent with the rest of the status pipeline

review-address's status writes are explicitly best-effort — the existing comment says "a status post that fails warns and continues — it must never cost the round" — and every failure path there is || echo "::warning::…; continuing.".

The new reporter is the opposite: any failure to publish the notice fails the whole scan. The sharpest edge is the identity gate:

if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then
  echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor:-unknown}'"
  return 1
fi

If CI_DEV_BOT_PAT is unset, rotated, or unavailable (fork-originated runs), gh api user fails 3× → actor='' → return 1 → the caller exit 1s. Every terminal takeover admission becomes a red run with no comment written and only a ::warning:: explaining why. Forks and secret rotation are the exact situations where this fires, and there's nothing an operator can do about it from the PR side.

Suggestion: treat "cannot authenticate as the bot" as not applicable (warn, return 0) rather than failed to publish, and reserve return 1 for the cases where the bot identity is confirmed but the write itself failed. At minimum give it a distinct reason code so the red run is diagnosable from the summary.

🟡 Smaller notes

• cross_repo_state_missing is dead code. read_forced_pr_meta now validates (.isCrossRepository | type == "boolean") before returning, and GraphQL types both isCrossRepository and maintainerCanModify as Boolean! — so META can never reach the classifier with the field missing. Harmless defence-in-depth, but the unit test drives the classifier directly, so it reads as live coverage of a guard that can't fire. Worth a one-line comment saying the branch is unreachable-by-construction and kept deliberately.
• Warning streams are inconsistent. read_forced_pr_meta / read_live_permission send warnings to >&2 (correct — they're inside command substitution); report_forced_takeover_blocked sends its four warnings to stdout. Not a bug today, but it's a trap if the reporter is ever wrapped in $( ). Make them all >&2.
• New-comment fallback isn't in the PR description. When no <!-- autofix-status --> comment exists, the reporter posts a fresh one via gh pr comment. The description says takeover PRs "update their existing AutoFix status"; the create path deserves a mention. (Also: review-address posts via gh api "repos/${REPO}/issues/${PR}/comments" — same effect, but the two writers now use different mechanisms for the same comment.)
• ✅ -f body= (not -F) is the right choice for a body that could contain a leading @; tail -1 matches review-address's | last; schema-validating gh pr view output is a real improvement over || echo '{}'.

Test coverage

The replay harness is good. Gaps, roughly in priority order:

1. No permission: "none" case — see pre-release: fix ci #1.
2. No test that a non-managed META returns 0 with zero gh calls. That META gate is the guard preventing AutoFix from commenting on unrelated PRs; it's the one branch worth asserting on call count, and it's untested.
3. No test for actor != AUTOFIX_BOT → return 1. The stub always answers qwen-code-dev-bot, so the riskiest new failure path (如何自定义密钥文件 .env可能与其他文件冲突 #3) is uncovered.
4. expect(reviewScanJob).toContain('exit 1') asserts nothing — a 900-line job contains exit 1. Either drop it or anchor it to the surrounding line.
5. reviewAddressJob = workflow.match(/\n {2}review-address:[\s\S]*$/) is greedy to EOF. It works only because review-address is currently last; a job appended after it makes both toContain assertions silently match the wrong job. Use the (?=\n {2}# ==========) lookahead the other extractors use.
6. The extraction regexes (\n {10}\}) and .replace(/\n {10}/g, '\n') hard-code YAML indentation. toBeTruthy() catches a total miss, but a partial reindent would de-indent into subtly different shell. A shared helper (extractShellFn(job, name)) with an explicit assertion on the closing brace would be sturdier.
7. The new it() is ~230 lines covering six distinct behaviours (transient meta, transient perm, terminal perm, terminal meta, six reporter scenarios, bash syntax). Splitting into focused cases would localize failures.

Verdict

Fix #1 (correctness regression, confirmed by repro) before merge. #2 and #3 are design calls where I'd like the author's reasoning — both trade a previously-fast visible no-op for a new way to be slow or red. The rest are polish.

Review round 2 — e49aadef

Re-reviewed against the current head (e49aadef, "treat terminal permission answers as a routine rejection"), diffing the forced-admission block and scripts/tests/qwen-autofix-workflow.test.js rather than re-reading the PR description.

✅ Round-1 #1 is fixed and I verified it. read_live_permission now accepts none in the regex (L1770) and short-circuits HTTP 404 and the empty-login case to a terminal none (L1763-1776). The comment at L1748-1759 states the invariant precisely — "none and HTTP 404 are DEFINITIVE answers, not lookup failures" — and the author_permission_* branch it used to strand is reachable again. Good fix, and the reasoning left in the comment is the right thing to leave behind.

Round-1 #2 and #3 are unchanged, and I think both are still worth resolving. One new issue landed with this commit.

🟠 1. Shared qwen-pr-head-write-<pr> lock — still open, and the failure mode is worse than I described last time

# .github/workflows/qwen-autofix.yml:1696-1698
concurrency:
  group: 'qwen-pr-head-write-${{ needs.route.outputs.pr_number || github.run_id }}'
  cancel-in-progress: false

review-address holds the same group (L2910) with timeout-minutes: 300 (L2870). Scheduled scans are unaffected — pr_number is empty, so the group falls back to github.run_id and is run-unique — so this only bites the forced/real-time path, which narrows it. But on that path:

• The 15-minute budget does not cover queue time. A review event on PR N while an address run for N is in flight now sits pending for up to 5 hours. The pre-existing BUSY_PRS guard already handled this in seconds, and I confirmed a forced PR reaches it: CANDIDATES="${FORCED_PR}" (L1931) flows into for PR in ${CANDIDATES} and hits the busy skip at L2130-2133 → fleet_row … 'busy' 'address run in flight'.
• The single pending slot is now contended by two different jobs. GitHub keeps at most one pending entry per concurrency group and cancels the previously-pending one when a newer entrant arrives — independent of cancel-in-progress: false. Before this PR only review-address legs were in the group. Now a queued review-scan and a queued review-address leg for the same PR compete for that one slot, in either order: a second review event on PR N can cancel a pending address leg (the round never runs, no report comment), and a newly-queued address leg can cancel a pending scan (the review event is dropped silently). build-cli and review-address both needs: review-scan with no always(), so a cancelled scan takes the whole run with it.

That silent-drop class is exactly what this PR exists to eliminate, so trading it in here feels like the wrong direction. The write being protected is one comment PATCH on a rarely-taken terminal path — scoping the lock to that write (or re-reading the status comment immediately before PATCHing and skipping if it carries a newer 🔄 AutoFix is working marker) buys the same safety without putting a 15-minute job behind a 300-minute one. If the job-level group stays, please at least say so in the concurrency comment — "share its per-PR lock so neither writer can erase the other's state" reads as free.

🟠 2. PAT-identity mismatch still reds the run — and it is the only site in this step that does

# .github/workflows/qwen-autofix.yml:1818-1821
if [[ "${actor}" != "${AUTOFIX_BOT}" ]]; then
  echo "::warning::Blocked takeover status skipped: PAT authenticates as '${actor:-unknown}'"
  return 1
fi

The caller turns that into exit 1 (L1920-1924). The same condition is handled four times in this same step — L2370, L2474, L2520, L2590 — and every one of them warns and continues green, using a memoized SCAN_BOT_ACTOR with || echo 'unknown':

if [[ -z "${SCAN_BOT_ACTOR:-}" ]]; then
  SCAN_BOT_ACTOR="$(gh api user --jq '.login' 2> /dev/null || echo 'unknown')"
fi
if [[ "${SCAN_BOT_ACTOR}" != "${AUTOFIX_BOT}" ]]; then
  echo "::warning::cap-refused notice skipped: PAT authenticates as '${SCAN_BOT_ACTOR}', expected ${AUTOFIX_BOT}"

So a PAT rotation, or AUTOFIX_BOT_LOGIN being set to something the PAT does not match, turns every review event on every managed fork PR into a red run, while the four neighbouring writers in the same shell stay green. "The PAT is not the bot" is a not-applicable condition, not a failed write — I'd warn and return 0, and reserve return 1 for "identity confirmed, write failed". Reusing SCAN_BOT_ACTOR instead of a fresh local actor also drops the extra gh api user calls (up to 3 more per run) for free.

🟠 3. NEW — metadata_fetch_failed reds a run that main exited 0 on, and is the one blocked reason with no PR-visible explanation

# .github/workflows/qwen-autofix.yml:1878-1882
if ! META="$(read_forced_pr_meta)"; then
  echo "::error::Forced PR #${FORCED_PR} admission blocked: metadata_fetch_failed"
  echo "targets=[]" >> "${GITHUB_OUTPUT}"
  echo "has_targets=false" >> "${GITHUB_OUTPUT}"
  exit 1
fi

On main this was META="$(gh pr view … || echo '{}')" → OK=false → a descriptive ❌ #N is not an open main-targeting PR … line → exit 0. Now any gh pr view that fails three times costs 3 calls plus 3 seconds and reds the run. That includes ordinary operator error — a mistyped pr_number on a workflow_dispatch, a number that is an issue rather than a PR, a deleted or transferred PR — none of which is a system fault.

report_forced_takeover_blocked is deliberately not called here (META doesn't exist yet, so the managed-PR gate at L1803-1805 can't run), which is understandable, but it makes metadata_fetch_failed the only blocked reason whose sole trace is a log line inside a red run. That inverts the PR's stated goal. Two options: keep exit 1 but only for a genuinely transient signature, and take exit 0 + the old descriptive line for a 404; or fall back to gh pr view --json number alone and, if that succeeds, post the blocked notice.

While here: the targets=[] / has_targets=false writes immediately before exit 1 (L1880-1881 and again L1905-1906) are dead — a failed job's outputs are never read, and build-cli/review-address gate on needs.review-scan.outputs.has_targets with no always(), so both are skipped outright. Dropping them removes the implication that there is a fall-through contract.

🟡 4. The status-lookup jq diverges from the sibling upsert it is duplicating

# .github/workflows/qwen-autofix.yml:1836-1837
if status_ids="$(gh api "repos/${REPO}/issues/${FORCED_PR}/comments" --paginate \
  --jq ".[] | select(.user.login == \"${AUTOFIX_BOT}\") | select(.body | contains(\"<!-- autofix-status -->\")) | .id" 2> /dev/null)"; then

versus the 'Post autofix status comment' step (~L3691), which does the same read on the same comment:

jq -rs --arg m "${MARKER}" --arg ab "${AUTOFIX_BOT}" \
  '[ .[][] | select((.user.login // "") == $ab) | select((.body // "") | contains($m)) ] | last | .id // empty'

Three deliberate differences in the sibling are dropped here:

• No // "" guards. A null body aborts the whole filter — real jq, not hypothetical:

  jq: error (at <stdin>:1): null (null) and string ("<!-- autof...) cannot have their containment checked
  rc=5
  

  gh then exits non-zero, all three attempts fail, status_lookup_ok stays false, return 1 → exit 1. A red run that never posts the blocked status it exists to post.
• ${AUTOFIX_BOT} interpolated into the jq program text instead of --arg. It comes from vars.AUTOFIX_BOT_LOGIN (L97), so it is repo-configurable — a " or \ in that var makes every lookup a jq parse error rather than a mismatch.
• gh pr comment for the create path where the sibling uses gh api …/comments -f body. Same effect, but two writers on one comment with two mechanisms.

Extracting a single upsert_status_comment helper would collapse this and remove three independent chances to drift.

🟡 Smaller notes

• cross_repo_state_missing is now provably unreachable, not just probably. read_forced_pr_meta validates (.isCrossRepository | type == "boolean") (L1736) before returning, so the classifier's (((.isCrossRepository == true) or (.isCrossRepository == false)) | not) (L1795) is always false. Only the unit test can produce that string, by feeding META directly and bypassing the reader — so expect(reason(missing)).toBe('cross_repo_state_missing') certifies a branch that cannot fire, while the real malformed-metadata case takes metadata_fetch_failed → exit 1 with no comment. Either drop the branch or comment it as unreachable-by-construction.
• Hardcoded https://github.com/… in the blocked body (L1832). Every other status writer passes RUN_URL: '${{ github.server_url }}/…' (L3675, L4937) and the report step uses ${GITHUB_SERVER_URL} (L4900). On a proxied/GHES host, the one link this message exists to surface is the broken one, sitting next to correctly-resolved links in the same comment.
• Warning streams still split. read_forced_pr_meta / read_live_permission write to >&2; all four warnings in report_forced_takeover_blocked (L1815, L1841, L1854, L1862) go to stdout. Raised last round; still a trap if the reporter is ever wrapped in $( ).
• The status-lookup failure discards gh's stderr (2> /dev/null, L1836) — directly against the rule this same diff establishes 60 lines above: "Surface gh's own diagnosis instead of discarding it: a rate limit, an expired PAT and a 5xx all look identical otherwise." (L1778-1779). On a red run the operator sees only attempt 3/3.
• Back-off is well below GitHub's. Secondary rate-limit responses carry Retry-After, typically 60s; the sleep 1 / sleep 2 ladder neither reads nor approximates it, so a rate-limited scan burns its three attempts inside 3 seconds and gives up.
• Six copies of the same retry skeleton landed in one diff (read_forced_pr_meta, read_live_permission, and four loops in the reporter), each for attempt in 1 2 3 / warn / [[ "${attempt}" -lt 3 ]] && sleep "${attempt}". One retry3 "<what>" <cmd…> helper collapses all six and makes a Retry-After change a one-line edit.
• ${FPERM:-none} (L1914, and L1984 in the scheduled loop) can no longer expand to anything but ${FPERM} — the new reader either fails (handled above) or returns a regex-matched value. Harmless, but it hides the invariant the reader now guarantees.
• The rejection log lost the author. 🧭 forced fork PR #N rejected: author_permission_read no longer names the login, and neither does the blocked comment; read_live_permission prints it only on the failure path. A maintainer told to "Grant the fork author write access" has to go find who that is, and can't distinguish an unexpected FORK_AUTHOR (bot login, transferred account) from a genuine permission gap. The scheduled loop still logs author ${FAUTHOR}=${FPERM}, so the two paths now disagree.

Tests

The verbatim-extract + stub-gh approach remains the right one, and the reporter stub covering transient actor / PATCH / comment failures is good coverage of the retry paths.

1. The harnesses run under the wrong shell. defaults.run.shell: 'bash' (L89) makes GitHub invoke bash --noprofile --norc -eo pipefail {0}; every new harness (runReader, runReporter, and the failing-gh blocks) spawns bash -c with no flags. I don't believe anything currently misbehaves under errexit — each helper is called from an if ! or || context, which disables it — but that is a property of the call sites, not of the functions, and the tests are what would catch it changing. set -eo pipefail as the first line of each harness script costs nothing and makes the suite reproduce production semantics.
2. Still no test for actor != AUTOFIX_BOT → return 1. The stub answers qwen-code-dev-bot and the harness sets AUTOFIX_BOT: 'qwen-code-dev-bot', so the branch in Where is the config saved? #2 above — the riskiest new failure path — has no coverage in either direction.
3. No null body case for the status lookup, which is Are you interested in AI Terminal? #4 above; the stub returns a bare 123, so the filter itself is never exercised against a realistic comments payload.
4. Round-1 notes 4-7 (toContain('exit 1') asserting nothing, the greedy reviewAddressJob regex, hard-coded indentation in the extractors, the ~230-line single it()) still stand.

Verdict

Nothing here is a correctness regression on the scale of round-1 #1 — that one is properly fixed. What's left is #1 and #2, where I'd like your reasoning before merge because both convert a previously fast-and-green outcome into a slow or red one, and #3, which is new and cheap to fix. The rest is polish.

已修复（commit 5d45d9f32），针对 review round 2 的 #4 与部分 smaller notes；#1 / #2 / #3 未改动，等你的取舍。

Fixed in 5d45d9f32 — round-2 item #4 and three of the smaller notes. #1, #2 and #3 are deliberately untouched, see below.

Fixed

• Are you interested in AI Terminal? #4 — the status-lookup jq diverged from the sibling upsert. The lookup now runs the same filter as Post autofix status comment: // "" guards on both .user.login and .body, and --arg for the bot login and the marker instead of interpolating them into the program text. Reproduced your rc=5 first — the old filter against a page whose first bot comment has body: null:

  jq: error (at <stdin>:0): null (null) and string ("<!-- autof...) cannot have their containment checked
  rc=5
  

  The new filter returns 123 on the same input, 456 on two concatenated pages (the pre-2.31 gh shape), and exits 0 on an empty stream. Kept as an inline id stream into tail -1, so the WORKDIR page normalizer still does not apply and the toBe(9) pin is unchanged; --paginate site count stays 14.
• Hardcoded https://github.com/… in the blocked body → ${GITHUB_SERVER_URL}, matching every other status writer.
• 2> /dev/null on the status lookup → gh's own stderr now rides along in the per-attempt warning, the rule stated 60 lines above for read_live_permission.
• Split warning streams → all four reporter warnings now go to >&2 like the two reader helpers.

Tests

• Harnesses now run under production shell options. Every harness (runReader, runReporter, runGate, and the failing-gh blocks) starts with set -eo pipefail and calls through || exit $?, so errexit is suspended inside the helper exactly as the if ! / || call sites suspend it — both halves of the production semantics, not just one.
• actor != AUTOFIX_BOT → return 1 now has a case in both directions (status 1, warning names the actor, no comment written).
• The reporter stub returns a realistic comments page — objects rather than a bare 123, including a null-bodied bot comment — so the filter itself is exercised.
• runGate gained the two missing scenarios: in-repo pass-through (admitted by label alone, asserting no collaborator call) and a 502-on-every-attempt permission lookup (status 1, blocked comment posted first).

Each new case kills a mutant that previously survived — I re-ran the suite against all three:

Verification

• npm run test:scripts — 946 passed, 9 skipped, 46 files, 0 failed (qwen-autofix-workflow.test.js 116/116).
• npx prettier --check on both changed files — clean.
• npx eslint scripts/tests/qwen-autofix-workflow.test.js — clean.
• git diff --check — clean.

Not changed, on purpose

#1 (shared qwen-pr-head-write-<pr> lock), #2 (PAT-identity mismatch reds the run) and #3 (metadata_fetch_failed reds a run main exited 0 on) all change which outcome a run reports, and you asked for reasoning before merge on #1 and #2 while #3 offers two designs. I have not picked one unilaterally — leaving those to the author/maintainer decision rather than guessing. The remaining polish notes (the retry3 extraction, cross_repo_state_missing reachability, Retry-After-aware backoff, the dead targets=[] writes before exit 1, the rejection log dropping the author) are untouched this round.

@qwen-code /triage

已修复 round 9 的两条 Suggestion（commit bf6d79656）。三条 Critical 仍按上轮说明保留，等待维护者取舍。

Fixed both round-9 suggestions in bf6d79656. The three [Critical] items remain deliberately unchanged — as the review itself notes, they are pending maintainer judgment calls, not code defects.

R9-1 — .github/workflows/qwen-autofix.yml:1698. Confirmed the mechanism: route computes ROUTE_PR from the dispatch input and emits pr_number unconditionally (line 641), while phase: issue leaves DO_REVIEW=false (the forced-routing block that would flip it only applies in auto/empty phase). So phase: issue + pr_number: N did resolve the group to qwen-pr-head-write-N for a job that only skips, and issue-autofix (needs: ['route', 'review-scan']) idled behind it. The group now carries the same do_review == 'true' conjunct, so a skipping run falls to github.run_id. Serialization against review-address on the real forced path is unchanged.

R9-2 — scripts/tests/qwen-autofix-workflow.test.js. The pin now extracts both prefixes and asserts equality, mirroring groupOf in qwen-resolve-workflow.test.js, plus a new case pinning the group predicate against the job's own if: block.

Verification

Note on style: the group is now double-quoted. Its expression embeds 'true' and contains no double quotes, so Prettier rewrites the escaped-'' single-quoted form; the sibling runs-on lines keep '' only because they also contain ".

@qwen-code /triage

Maintainer verification — local real-stack run of review-scan

I rebuilt this PR's admission path locally and ran it for real rather than reading the diff. Summary: the behaviour this PR claims is real and verified, and the harness also turned up one genuine defect in the new blocked-status writer, plus two smaller notes worth a look before merge.

Confirmed working

Edge probes that also passed: a null-body comment in the list does not abort the jq filter (the // "" guard holds); an AUTOFIX_BOT_LOGIN containing " and \ is a mismatch, not a jq parse error (the --arg guard holds); a status comment living on page 2 is found correctly through --paginate + .[][]; the run link is built from GITHUB_SERVER_URL, and the bilingual body renders as intended (screenshot 1).

The PR's own suite: 117/117 pass on the head commit.

🐞 Defect found — connection-level failure on the status read is neither retried nor surfaced

report_forced_takeover_blocked reads the status comments through a pipeline:

if status_ids="$(gh api ".../comments" --paginate 2> "${err}" |
  jq -rs --arg ab "${AUTOFIX_BOT}" --arg m '<!-- autofix-status -->' '…')"; then
  status_lookup_ok=true

The review-scan step does not set pipefail (the sibling Post autofix status comment step does — it starts with set -uo pipefail; this one only mentions pipefail in a comment). So the if tests jq's exit status, not gh's. When gh fails with an HTTP status it prints the error body to stdout and jq errors out, so the retry works — that path is fine. But when gh fails at the connection level (TCP reset / TLS abort / DNS blip) stdout is empty, jq -rs happily produces nothing and exits 0.

Verified end-to-end (scenario: terminal author_permission_none, status read answered with a TCP RST):

• 1 status read instead of 3 — the documented bounded retry never happens
• status_lookup_ok=true although nothing was read
• a duplicate ⛔ AutoFix blocked comment is posted while the stale ✅ round 3 pushed comment stays — the exact two-status state this PR exists to remove
• the run exits 0, green

Re-running the identical scenario with the one-line change below gives 3 bounded retries, gh's real diagnosis in the warning, Failed to read takeover status comments, and exit 1 — i.e. the contract the PR description states:

if status_ids="$(set -o pipefail; gh api ".../comments" --paginate 2> "${err}" |
  jq -rs …)"; then

Smaller notes (non-blocking)

1. npm run test:scripts exits 1 on macOS. On this head, scripts/tests/qwen-autofix-workflow.test.js reports 117/117 passed but the process still exits 1 on an unhandled [vitest-worker]: Timeout calling "onTaskUpdate". Deterministic here — 3/3 runs on macOS (file runtime 62–86 s); the same file on the merge-base is exit 0, 112 passed, 55 s. Linux CI is green, and the Test (macos-latest) job was skipped in that run, so this is currently unobserved by CI rather than proven safe there. Worth a look, since the file is not excluded on darwin (only on win32).

2. A 404 on the forced PR lookup is a terminal answer treated as transient. read_forced_pr_meta retries any gh pr view failure three times, so a mistyped pr_number in a manual dispatch burns 3 calls + back-off and then reds out with metadata_fetch_failed — a transient-sounding code for a permanent condition. This is the same distinction the PR already draws for permissions (e49aade); mirroring it here (grep -q 'HTTP 404' → pr_not_found) would be consistent.

3. The jq -e schema gate on the metadata is effectively unreachable through real gh. gh pr view --json always emits every requested field, so the "malformed metadata" branch cannot fire in practice — a deleted-account author comes through as login: "app/" (still a string), and the run then correctly ends at author_permission_none. Harmless belt-and-braces, but the transient-vs-malformed coverage it implies is theoretical.

Not covered

• The concurrency: group change in bf6d7965 is a job-level expression, evaluated by the Actions engine before the step runs — it is outside this harness. I only reviewed it by reading; its semantics (do_review != 'true' → github.run_id) look right, but it is untested here.
• Everything downstream of admission (review-address, the push path, the agent round itself).
• Real GitHub rate-limit / secondary-limit behaviour.

Verdict: the admission changes do what the description says, before/after is clearly demonstrable, and the reason codes + status write behave correctly under every failure mode I could inject. I'd like the pipefail fix in before merge; the three notes can follow up.

if status_ids="$(gh api ".../comments" --paginate 2> "${err}" |
  jq -rs --arg ab "${AUTOFIX_BOT}" --arg m '<!-- autofix-status -->' '…')"; then
  status_lookup_ok=true

if status_ids="$(set -o pipefail; gh api ".../comments" --paginate 2> "${err}" |
  jq -rs …)"; then

已修复（commit d8d66b5b0）：report_forced_takeover_blocked 的状态评论读取现在自带 set -o pipefail。三条小问题本轮未动。

Fixed the blocking pipefail item in d8d66b5b0. The three smaller notes are untouched this round.

The change

.github/workflows/qwen-autofix.yml — exactly the form you proposed:

if status_ids="$(set -o pipefail; gh api "repos/${REPO}/issues/${FORCED_PR}/comments" --paginate 2> "${err}" |
  jq -rs --arg ab "${AUTOFIX_BOT}" --arg m '<!-- autofix-status -->' \
    '.[][] | select((.user.login // "") == $ab) | select((.body // "") | contains($m)) | .id')"; then

One factual note on severity, since it changes what the fix is for rather than whether to take it: this file sets defaults.run.shell: 'bash' (line 87), which GitHub expands to bash --noprofile --norc -eo pipefail {0} — so review-scan does run with pipefail on a real runner, and the duplicate-comment path your harness reproduced needs the harness's plain bash to appear. Your reading of the mechanism is exactly right either way, and the option is now carried locally, so it holds regardless of that default or of the helper moving to a step that sets its own options. The # pipefail comment you spotted in this step now says which of the two it means.

Verification

Both halves of the failure are now replayed against the extracted helper, and I checked each new assertion bites by mutating the source:

The second row is the new one: it is the case that fails without the local option. Mutation checks —

• remove set -o pipefail; → that row reports exit 0 instead of 1 (1 failed | 116 passed), i.e. exactly the silent-green duplicate you demonstrated
• remove the defaults: block → the new textual pin fails (1 failed | 116 passed)

The third row is there so the jq-chokes-on-the-body path stays pinned separately and the local option cannot later be read as redundant with it.

Commands run on d8d66b5b0:

• npx vitest run --config scripts/tests/vitest.config.ts scripts/tests/qwen-autofix-workflow.test.js scripts/tests/qwen-resolve-workflow.test.js scripts/tests/qwen-fleet-shepherd-workflow.test.js scripts/tests/package-scripts.test.js → 173/173 pass (autofix suite 117/117)
• npm run build → ok; npm run typecheck → ok
• npx prettier --check + npx eslint on both changed files → clean
• YAML re-parsed after the edit (8 jobs, defaults.run.shell: bash), and the suite's own bash -n over the whole review-scan run block still passes

Note 1 (macOS test:scripts exit 1) I could not reproduce — this is a Linux box, where the file is exit 0 at 117/117 in ~7s. Notes 2 and 3 I've left as follow-ups per your "non-blocking".

已修复（merge commit dfc6ec551）：沙箱验证报告的那条 Critical（reviewAddressJob 重复声明）以及 Test (ubuntu-latest, Node 22.x) 的红灯，根因都是本分支落后 main 17 个 commit。

Fixed in merge commit dfc6ec551. Both the sandboxed verification's Critical and the red Test (ubuntu-latest, Node 22.x) had the same root cause — this branch was 17 commits behind main. Neither was caused by a change in this PR.

1. Test (ubuntu-latest, Node 22.x) — npm error Missing script: "check:voice-guard-sync"

job 92688890346 died at the Check voice guard mirror sync step, exit 1.

The asymmetry is the cause: for pull_request, the workflow definition comes from the base branch, but ci.yml:242 checks out refs/pull/8410/head. So main's step run: 'npm run check:voice-guard-sync' (ci.yml:358) ran against this branch's package.json, which predates that script — it landed in main with #8350 (732f4d8a2), after this branch's merge-base 89b3d5ea8. Nothing on the PR side to fix; the branch just had to catch up.

After the merge, locally: npm run check:voice-guard-sync → Voice guard mirror check passed. (exit 0).

2. The sandboxed verification Critical — duplicate const reviewAddressJob

Confirmed and fixed, with one correction to the attribution: the duplicate does not exist on the PR head. On d8d66b5b0 (and on bf6d79656c, the sha you verified) scripts/tests/qwen-autofix-workflow.test.js declares reviewAddressJob exactly once. main also declares it exactly once. The duplicate is created by the merge: both sides added the declaration in different hunks, so git took both textually without raising a conflict. That is why it only shows up on the committed/merged tree — which is precisely the tree that matters for merging, so the finding stands.

Resolution — kept main's slice, dropped this branch's older one:

// dropped (this branch, older — assumes review-address is the last job):
const reviewAddressJob =
  workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? '';

// kept (main, bounded on the generic next-job shape, still allows EOF):
const reviewAddressJob =
  workflow.match(
    /\n {2}review-address:[\s\S]*?(?=\n {2}[a-z][a-z0-9-]*:\n|$)/,
  )?.[0] ?? '';

main's form is strictly more general: the |$ alternative keeps it correct while review-address is last, and it shrinks correctly once a job is appended after it. No assertion changed.

Verification on the merged tree (dfc6ec551)

• npm run test:scripts → 47 files, 961 passed / 9 skipped, exit 0 (qwen-autofix-workflow.test.js alone: 120/120, up from 117 — main added 3)
• npm run build → ok · npm run typecheck → ok
• npm run check:voice-guard-sync → pass · npm run check:lockfile → pass · npm run check:desktop-isolation → pass
• npx prettier --check and npx eslint on scripts/tests/qwen-autofix-workflow.test.js → both clean (the no-redeclare you reported is gone)

The merge is a fast-forward on top of d8d66b5b0; the only manual resolution in it is the two-line deletion above (git show dfc6ec551 shows nothing else).

The three smaller notes from the maintainer verification round remain untouched this round, as stated previously.

Re-verification after d8d66b5 — and a correction to my own report

Re-ran the whole harness against dfc6ec5 (merge-base now 037b4d9). First, the correction, because it is mine to make.

⚠️ My severity call on the pipefail finding was wrong

d8d66b5's commit message is right and my report was not: this workflow sets defaults.run.shell: 'bash', which GitHub expands to bash --noprofile --norc -eo pipefail. My harness launched the extracted step with plain bash. That single infidelity is what made the status-read failure look reachable in production. It isn't — the ambient pipefail already covered it on every real runner.

I fixed the harness to launch the step exactly as the runner does, and re-ran the scenario as a 2×2:

The failure lives only in the bottom-left cell. So: not a production bug, and I should not have called it one. The fix is still worth keeping for the reason the commit gives — it is the only guard that does not depend on a workflow-level default staying put or on this helper never moving to a step that sets its own options — and the two new tests (connection-level vs HTTP-level failure, with ambient pipefail dropped) pin exactly the right distinction. Pinning defaults.run.shell: 'bash' textually is a good call too: the other gh … | jq … writers in this file silently depend on it.

Also worth noting for anyone else building a harness against this file: -e is part of that same default, and I re-checked it — it changes nothing in this step, because every call site of the new helpers is an if ! / || context, which suspends errexit inside the call.

Full matrix re-run under the runner shell

All 31 scenarios × {merge-base 037b4d9, head dfc6ec5}, now with bash --noprofile --norc -eo pipefail. Every admission outcome, reason code, retry count and status write is byte-identical to my first run — so the whole "Confirmed working" table in my earlier comment stands as written, only now on a faithful shell:

• transient permission 500 → write: admitted after one retry
• permission 500 ×3: permission_lookup_failed, existing status comment PATCHed to the blocked body, exit 1
• permission 404 / none / read: terminal, 1 API call, author_permission_*, routine exit 0
• maintainer_edits_disabled, metadata_fetch_failed, PAT-identity refusal, DRY_RUN writes nothing, distinct reason codes for not_open / wrong_base / skip_label / unmanaged_author, scheduled-scan blocked fleet rows — all unchanged
• edge probes unchanged: null comment body, AUTOFIX_BOT_LOGIN containing " and \, status comment on page 2, GITHUB_SERVER_URL-derived run link

The PR's own suite on dfc6ec5: 120/120 pass.

Status of my other three notes

1. test:scripts exit 1 on macOS — still there, but I have to soften how I described it. On this head it is intermittent: 2 of 6 runs exit 1 with the unhandled [vitest-worker]: Timeout calling "onTaskUpdate", always on the slower runs (~78 s file runtime vs ~58 s when green); tests are 120/120 either way. On the earlier head I saw 3/3, so "deterministic" was an artefact of a small sample. Still worth a look — the file is only excluded on win32, and this is a process-level non-zero exit, not a test failure.
2. 404 on the forced PR lookup treated as transient — open. read_forced_pr_meta still retries a nonexistent pr_number three times and reports metadata_fetch_failed. Verified again on dfc6ec5: 3 pr_view calls, exit 1.
3. jq -e metadata schema gate unreachable through real gh — open, and harmless. Informational only.

Verdict

Unchanged apart from the retraction: the admission changes do what the description says, and I could not find a production-reachable failure in the forced or scheduled path across 31 injected conditions. My earlier "I'd like the pipefail fix in before merge" was based on my harness bug, not on the code — d8d66b5 is a good defensive addition rather than a required fix. From my side this is ready to merge; notes 1–3 are follow-up material.

@qwen-code /triage