ci(autofix): harden the address path against stale targets and untrusted route events - #7163
Merged
Conversation
added 2 commits
July 18, 2026 20:59
…ted route events Follow-up to #7127 addressing the five Critical findings of its post-merge review (4728499913): - Route group trust prefilter: the per-PR concurrency group is entered before any step runs, so an arbitrary commenter's review could cancel a queued legitimate route and then die in Decide phases. Reviews whose payload does not already look trusted (repo association or the review bot) now get a run-unique group — cancel nothing, still fully authorized inside. Decide phases remains the real permission gate. - Address-time eligibility recheck: a matrix job can start hours after its scan; a PR closed/merged meanwhile (or with changed author/repo/base/branch) is discarded BEFORE the PR branch checkout — no secret-bearing agent run, no push, no comment, no marker. A failed fetch discards too (unknown is not eligible). - Non-stale duplicates adopt the live watermark and round: a sibling may have evaluated F1 through T1 while this job carried watermark W; rendering from W would replay handled feedback to the agent, and reusing the matrix round would double-write a marker round. Both reporters consume the effective round. - Live round cap: when live markers already sit at MAX_ROUNDS the run discards (the scan itself skips capped PRs before conflict checks) — no round MAX+1 work, no second capped marker. - Stale discard suppresses the failure-path handoff: a late always() step failure after stale=true no longer converts a deliberate no-comment/no-marker discard into a handoff that consumes a round. Contract tests 53/53: W/T1/T2 replay proving only-F2 rendering via the adopted watermark, live-round adoption and cap-discard replays, sentinel-ts non-adoption, stale-suppressed handoff replay, and eligibility ordering/coverage pins.
…gate Forward-compatible with the incoming label-takeover feature so the two changes commute in any merge order: the eligibility recheck now reads labels live — autofix/takeover exempts a human-authored PR from the bot-author requirement, and autofix/skip (which wins over takeover) withdraws consent even if applied while the job sat queued. Both are enforced at the moment the secret-bearing run starts, not at scan time. Inert until the labels exist.
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: The five findings were identified as Critical in a specific post-merge review (4728499913) of #7127 — documented issues from code review of the live autofix pipeline, not theoretical concerns. Three subsequent commits add engagement label honor ( Direction: CI workflow hardening for the autofix loop — security (route-group trust prefilter), correctness (watermark/round adoption, round cap enforcement), and safety (eligibility recheck, stale-handoff suppression, scan-side skip). Solidly in scope. No CHANGELOG reference expected for internal CI infrastructure. Size: Not applicable — changes are in Approach: Scope feels right — each of the five fixes maps directly to a Critical finding, and the follow-up commits (engagement labels, behavioral replay tests, scan-side skip, API failure guard) tighten the implementation without bloat. The test suite pins workflow literals against test expectations, so drift between workflow and tests fails the suite. No unrelated changes or drive-by refactors. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 五项发现来自 #7127 合入后评审（4728499913）中标记为 Critical 的问题——是线上 autofix 流水线评审中记录的实质问题，不是理论性担忧。后续三个 commit 增加了 engagement label 支持（ 方向： autofix 回路的 CI 工作流加固——安全（路由组信任预过滤）、正确性（水印/轮次采纳、轮次上限强制）、安全保护（资格复核、stale-handoff 抑制、scan 侧 skip）。完全在项目范围内。内部 CI 基础设施不需要 CHANGELOG 条目。 规模： 不适用——改动在 方案： 范围合理——五项修复各对应一个 Critical 发现，后续 commit（engagement labels、行为回放测试、scan 侧 skip、API 失败守卫）收紧实现而无膨胀。测试套件将工作流字面量与测试期望钉住，工作流与测试间的漂移会导致套件失败。没有无关改动或顺手重构。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code reviewRe-read the full diff at the current HEAD ( Route-group trust prefilter — The Address-time eligibility recheck — Placed correctly before Live watermark adoption — The terminal sentinel ( Round cap enforcement — Stale-handoff suppression — Scan-side skip filter — The skip-labeled PR exclusion at candidate selection prevents an infinite re-emission loop: the address-gate discard writes no marker, so without this filter, every scan tick would re-emit a skip-labeled PR through full checkout/install. The Engagement labels — Static checks
TestsVerified via CI: Test (ubuntu-latest, Node 22.x) — pass (33m43s). Could not reproduce locally (vitest not installed in this environment; Real-scenario testingN/A — this PR modifies GitHub Actions workflow YAML, not CLI behavior. The autofix pipeline runs on GitHub Actions runners and cannot be exercised locally via tmux. The verification path is the contract test suite (behavioral replays extracted verbatim from the workflow) plus static validation. 中文说明代码审查在当前 HEAD（ 路由组信任预过滤 — GitHub Actions 表达式中 address 时资格复核 — 正确位于 live 水印采纳 — 终止哨兵通过显式字符串检查被正确排除。水印和轮次独立采纳，均仅对非 stale 运行生效。后续轮次上限检查设置 轮次上限强制 — 两个报告器中的 stale-handoff 抑制 — scan 侧 skip 过滤 — 候选选择时排除 skip 标记的 PR 防止无限重发循环：address 门丢弃不写标记，没有此过滤则每次扫描都会对 skip 标记的 PR 执行完整 checkout/install。 Engagement labels — 声明为工作流级 env 变量，address 时实时重新读取——排队期间应用的 skip label 在带 secret 的运行开始前撤回同意。 静态检查
测试通过 CI 验证：Test (ubuntu-latest, Node 22.x) — pass（33m43s）。本地环境无法复现（vitest 未安装； 真实场景测试不适用——本 PR 修改 GitHub Actions 工作流 YAML，不是 CLI 行为。验证路径是契约测试套件加静态校验。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage; the five fixes are tightly scoped to documented Critical findings, well-tested, and correct. The three follow-up commits strengthen the implementation further. This is a focused hardening PR that does exactly what it says — five specific race conditions and bookkeeping corruptions in the autofix loop, each with a clear fix, each with a test that pins the workflow against the test suite. The follow-up commits add meaningful depth: behavioral replay tests that actually exercise the discard path (not just string-match it), a scan-side skip filter that closes an infinite re-emission loop, and a fail-closed API error guard that distinguishes infra outages from state changes. The code is well-commented with the why, the fail-closed defaults are in the right places, and the output propagation to downstream reporters is correct. Would merge without hesitation. 中文说明信心度：5/5 — 各阶段均干净；五项修复紧密对应已记录的 Critical 发现，测试充分，实现正确。后续三个 commit 进一步加强了实现。 这是一个聚焦的加固 PR，完全如其所述——autofix 回路中的五个具体竞态条件和记账污染，每项有清晰的修复，每项有将工作流与测试套件钉住的测试。后续 commit 增加了有意义的深度：实际执行丢弃路径（而非仅字符串匹配）的行为回放测试、关闭无限重发循环的 scan 侧 skip 过滤、以及区分基础设施故障与状态变更的 fail-closed API 错误守卫。代码注释说明了 为什么，fail-closed 默认值位于正确位置，output 传播到下游报告器正确无误。可以毫不犹豫地合入。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
pushed a commit
that referenced
this pull request
Jul 18, 2026
Large managed PRs routinely need dozens of feedback rounds — that is the point of takeover — so the unattended MAX_ROUNDS=5 would strangle exactly the PRs the label exists for. While TAKEOVER_LABEL is present the effective cap is TAKEOVER_MAX_ROUNDS=50: the circuit breaker stays (a bot/review-bot ping-pong is still bounded and every round still requires new trusted feedback or a conflict), it is just sized for explicitly delegated work. Removing the label restores the strict cap on the next scan. The scan computes the effective cap from the candidate's live labels and stamps it into the matrix target; the address job shadows the workflow-level MAX_ROUNDS with the matrix value, so every round message, marker, and cap gate uses the same number consistently (including #7163's address-time cap discard once both merge). 57/57 with a verbatim cap-selection replay (labeled → 50, plain → 5).
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Review suggestion, adopted: the discard path was only string-pinned — a future edit dropping the stale=true echo would leave every toContain green while STALE arrived empty downstream, letting a late always() failure post a spurious handoff for a discarded job. The recheck now runs VERBATIM under a PATH-stubbed gh across nine states: healthy bot PR proceeds writing nothing; closed-while-queued discards AND writes all four outputs later gates read; live takeover label exempts a human author; live skip label withdraws consent even for the bot's own PR; fork head, renamed branch, and a failed fetch (unknown is not eligible) all discard. 54/54.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
Review — PR #7163 @
|
wenshao
commented
Jul 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
added 2 commits
July 19, 2026 00:17
…tinct discard Review round on the hardening PR (six Suggestions, all adopted): - The scan candidate list now excludes skip-labeled PRs: the address-gate discard writes no marker, so an unfiltered scan would re-emit a skip-labeled PR into a full address job (checkout, npm ci, build) every tick forever. - The engagement-labels env comment no longer overpromises: in THIS change the labels are honored at the eligibility gate and scan filter; the scan-side widening that makes takeover summon human PRs ships with the takeover feature PR (the two commute either way). - A failed eligibility fetch now discards with an infra-distinct message (metadata fetch failed (API error) — fail-closed) instead of masquerading as state='unknown', mirroring the scan-side wording. - Tests: the non-main-base discard is now exercised behaviorally (not just pinned); the fetch-failure case asserts its distinct message; the full parenthesized route trust expression is pinned as ONE string (Actions binds && tighter than ||, so dropped parens would invert the grouping); and the terminal-sentinel adoption guard is exercised on a path that actually reaches the adoption block (live conflict skips the stale gate) instead of passing via the discard. 54/54.
Collaborator
Author
|
Re: the review at
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
Note on the 17:39 auto-review's "Downgraded from Approve to Comment: CI failing: route": the sampled |
wenshao
commented
Jul 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— qwen-latest-series-invite-beta-v92 via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
ytahdn
reviewed
Jul 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
ytahdn
reviewed
Jul 19, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Not reviewed: coverage — the plan could not be used (ENOENT: no such file or directory, open '.qwen/tmp/qwen-review-pr-7163-fetch.json'), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (ENOENT: no such file or directory, open '.qwen/tmp/qwen-review-pr-7163-fetch.json').
— qwen3.7-max via Qwen Code /review
ytahdn
approved these changes
Jul 19, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! ✅
— qwen3.7-max via Qwen Code /review
pull Bot
pushed a commit
to Stars1233/qwen-code
that referenced
this pull request
Jul 19, 2026
… green no-op (QwenLM#7165) * feat(autofix): label-driven takeover and release; fix forced-dispatch green no-op Takeover, exactly as designed: applying autofix/takeover (GitHub triage+ only — the permission gate is GitHub's own) summons the loop onto a PR, human-authored included; removing it releases the PR. The autofix/skip label opts any PR out at every engagement path — the autofix scan, the forced path, and the Fleet Shepherd walk — and wins when both labels are present. Every toggle gets a bilingual ack from the PAT-verified bot identity. The comment-command surface stays CLOSED: the pinned no-comment-commands contract test is untouched. Plumbing: pull_request labeled/unlabeled triggers; label events share the per-PR route group (the event class is triage-gated); the group expression also carries the QwenLM#7163 payload trust prefilter so the two open PRs converge on the same final text in either merge order; scan candidates become bot PRs ∪ takeover PRs minus skip PRs (deduped); fork label events are logged and dropped (no secrets to even post a rejection). The forced-validation replay immediately caught a latent bug live since QwenLM#6528: '(.isCrossRepository // true) | not' is false for EVERY input (jq's // treats false as empty), so every workflow_dispatch pr_number=N run — including the shepherd's conflict dispatches — validated to targets=[] and exited GREEN having done nothing. Fixed to '.isCrossRepository == false' (fail-closed on a missing field) with a replay case pinning the trap. Tests: autofix 55/55 (label engage/release pins, candidate-selection replay with skip-wins and fork cases, forced-validation replay across author/takeover/skip/closed/fork/missing-field); shepherd 12/12 (skip-filter replay). * feat(autofix): '@qwen-code /takeover' comment sugar over the takeover label Maintainer-mandated reopening of the comment surface, in the narrowest possible form: '@qwen-code /takeover' applies TAKEOVER_LABEL and '@qwen-code /takeover stop' removes it — nothing else. The label stays the single source of truth: engagement and release happen exclusively via the pull_request label events, so a manual label edit and the command are one mechanism with two entry points, and the command's whole blast radius is one label toggle. Gates: an expression-level startsWith prefilter keeps ordinary comments from ever starting a job; the body must match the constant EXACTLY after trimming (no parsing, no arguments); allowed senders are the PR author (who may lack label access — this is who the sugar is FOR) or a write+ collaborator via the same permission API used by review routing; closed PRs, non-PR comments, and the bot itself are ignored. The toggle job is PAT-verified and presence-aware (no-op toggles are explicit, since they fire no label event). The 'does not expose comment-triggered autofix commands' contract test is REWRITTEN into pinning this gated design, documenting the deliberate reversal. New behavioral replay drives the extracted command branch with a PATH-stubbed permission API across eight author/write/read/ exact-match/non-PR/closed/self cases. 56/56. * feat(autofix): raise the round cap to 50 while a PR is under takeover Large managed PRs routinely need dozens of feedback rounds — that is the point of takeover — so the unattended MAX_ROUNDS=5 would strangle exactly the PRs the label exists for. While TAKEOVER_LABEL is present the effective cap is TAKEOVER_MAX_ROUNDS=50: the circuit breaker stays (a bot/review-bot ping-pong is still bounded and every round still requires new trusted feedback or a conflict), it is just sized for explicitly delegated work. Removing the label restores the strict cap on the next scan. The scan computes the effective cap from the candidate's live labels and stamps it into the matrix target; the address job shadows the workflow-level MAX_ROUNDS with the matrix value, so every round message, marker, and cap gate uses the same number consistently (including QwenLM#7163's address-time cap discard once both merge). 57/57 with a verbatim cap-selection replay (labeled → 50, plain → 5). * feat(autofix): re-armable round windows, cap raised to 100, visible cap pause The round counter is DERIVED state, stored nowhere but in the bot's eval-marker comments on the PR — and counting is now windowed by the latest '<!-- takeover-ack engaged -->' comment. Re-engaging (label off→on, or repeating the takeover command on an already-managed PR, which now posts a re-arm ack instead of a silent no-op) starts a fresh window: a PR that exhausted its rounds continues under management with one human action, auditable in the PR timeline. The WATERMARK stays global across windows — feedback already addressed is never replayed — and a PR never taken over has no ack, so strict lifetime counting is unchanged. The prepare-side live round is windowed identically, so pre-reset markers can neither trip the cap nor look like same-ts round-advance duplicates (replay-proven). TAKEOVER_MAX_ROUNDS rises to 100 per maintainer sizing, and pausing at the cap is now VISIBLE on managed PRs: a bilingual notice with re-arm guidance, once per counting window (marker-deduped past the latest re-arm; a failed post retries next scan). 58/58: rearm windowing replay (no ack → lifetime; ack → round 0 with watermark preserved; new rounds count from 1; latest ack wins), the stale-gate re-arm interplay case, cap-selection at 100, and cap-notice dedup pins. * feat(autofix): collapsed-Chinese bilingual takeover comments; fix ESLint regex-spaces Every takeover-flow comment — engage ack, release ack, re-arm ack, and the cap-pause notice — now follows the project convention: English body plus Chinese collapsed under <details><summary>中文说明</summary> (pinned at exactly four sites). Bodies are built via printf so no workflow indentation leaks into the markdown: the previous literal multi-line strings embedded 10 leading spaces, which would have rendered the trailing marker comment as a visible code block. Also fixes the CI failure at 1497a7e: three extraction regexes used literal space runs, tripping ESLint no-regex-spaces — now {n} quantifiers. Lesson applied: prettier alone is not the lint gate. 58/58 + 12/12. * fix(autofix): fork-safe takeover release, verified cap-notice write, doc drift Review round (issue comment on head c2d8a89), all three findings: - F1 (bug): 'unlabeled' on a fork PR emitted the release ack unconditionally; fork pull_request runs carry no secrets, so takeover-ack failed its PAT identity check — a red run, reachable self-serve (the comment sugar accepts the fork PR's author, sticking the label; any later unlabel fired the red run). The unlabeled branch now mirrors the fork log-and-drop, and takeover-command — which runs in issue_comment context WITH secrets — refuses fork PRs up front with a bilingual explanation (adoption guidance), so the label can no longer stick to forks via the command at all. - F2 (convention): the scan's cap notice now verifies the PAT identity before writing (memoized per run): a rotated PAT would post under a foreign login that the AUTOFIX_BOT-scoped dedup can never see, reposting every scan. Scan header updated — its single write is identity-verified. - F3 (doc drift): two leftover '50' references updated to speak of TAKEOVER_MAX_ROUNDS. - Note adopted: the leading-whitespace prefilter/trim nuance is now documented at the prefilter. 58/58 + 12/12; collapsed-Chinese sites now pinned at five (the fork-refusal joins the four acks). * fix(autofix): window-keyed rounds, ordered commands, skip closure, marker scrub Second review round on the takeover feature (dev-bot GPT-5 review at c2d8a89) — all seven Criticals and three Suggestions: - Round windows are now keyed, not timestamped: every eval marker records the window key it was produced under (win=…, legacy counts as 'none'), the current key is the latest engage ack's created_at, and only current-key markers count toward the cap. An in-flight address job selected before a re-arm can no longer re-cap the fresh window with a late marker — and prepare discards any job whose key a re-arm superseded while it sat queued (conflicts stay actionable). - Takeover commands are serialized per PR (queued concurrency group): an older /takeover cannot land after a newer /takeover stop read the unlabeled state. - Skip closure at every remaining gap: the command refuses engage and re-arm on a skip-labeled PR (bilingual skip-blocked ack, no bogus window anchor); the label-event ack does the same; the scan's fresh per-PR fetch re-checks skip mid-scan; the shepherd re-checks the LIVE label immediately before its two mutating levers (fail closed — unreadable labels count as skipped). - Honest release on bot-authored PRs: removing takeover now says standard bot management continues (strict cap); only takeover mode ends. - Model files posted verbatim as PR comments (address-summary, no-action) are scrubbed of HTML comments — the rule the handoff DETAIL already applied — so induced output cannot forge control markers the scanners would trust. - Test hardening per the Suggestions: every bilingual body is asserted individually (8 printf bodies), and the gh pr list producers must request 'labels' in both workflows (fixture-driven consumers alone went green without it). 58/58 + 12/12; the rearm replay now proves the race the key model closes (an old-window round-50 marker landing after the ack counts 0). * fix(autofix): sever PR hooks from PAT pushes; scope label routing; fork-author command gate Third review round (yiliang114 P0-P3 + a second inline batch): - P0: the address/publish push steps carry the PAT while the branch carries PR-controlled .husky hooks (hooksPath was pointed there so the agent's commits get checked) — a pre-push hook would execute that code with the PAT in env. Both push sites now sever hooks (core.hooksPath=/dev/null + git push --no-verify). The force-push guard regex also false-positived on --no-verify ('-…f') and is now precise (-f word / +refspec). - P1: only the takeover label itself shares the per-PR route group; an unrelated label changed in the same batch operation can no longer cancel a queued takeover route. - P2: candidate INSPECTION is bounded (MAX_CANDIDATE_INSPECTIONS=60), not just emitted targets — idle candidates burn serial API calls and takeover widens the pool. Excess rotates to the next scan. - Fork-author command gate: author privilege is in-repo only — a fork-PR author can no longer summon even PAT-authored refusal comments onto their own PR (silent drop); write+ maintainers still reach the explanatory fork refusal. Replay-proven both ways. - Candidates jq now fails closed on the fork field (== false), matching the forced path and the //-trap NOTE. - The unlabeled path mirrors the labeled-path state guards: releasing a closed/non-main/fork PR acks nothing (it was never engaged). - Command-contract docs updated to the real side-effect set, and the four-path toggle (plus skip/fork refusals) now has a full gh-recording behavioral replay. 59/59 + 12/12. * fix(autofix): live cap-notice dedup key, line-proof marker scrub, checkout hook severing Round-3 review (issue comment at 427301c), all four findings: - R1 (regression): the window-key rename left the cap-notice dedup on a dangling REARM_TS — empty rt made every historical notice count, silently turning per-window dedup into per-lifetime, so a re-armed PR hitting the cap again paused with no reminder (the feature's headline case). Now NOTICE_RT=REARM_KEY with the 'none' corner falling back to lifetime dedup (created_at > 'none' is never true lexically, which would have flipped it into posting every scan). The extracted-jq replay proves all three regimes. - R2: the HTML-comment strip was line-oriented while jq scan() matches across newlines — a marker split over two lines survived the scrub and still parsed. All three publish sites now neutralize the opening token itself (sed 's/<!--/<!\-\-/g' — line-independent, and the backslashes render away in markdown). Proven end-to-end on a split forged marker, with the sed extracted verbatim. - R3 (pre-existing, folded in as the natural home): prepare's checkout -B ran PR-controlled post-checkout hooks with the PAT in env. Prepare now severs hooks like the push steps; the agent step — no PAT, sandboxed tools — re-points .husky itself so its commits still get checked. - R4: candidates now drain NEWEST-first (unique_by sorts ascending; sort_by(-.number) added), the comment states the real semantics instead of a rotation that never existed, and the free busy skip no longer consumes inspection budget. 59/59 + 12/12. * fix(autofix): takeover-ack state read fails closed Review suggestion, adopted verbatim: the ack's gh pr view fell back to '{}' on failure, defaulting HAS_SKIP to false — a transient API failure could post a wrong 'engaged' ack on a skip-labeled PR. It now exits like the sibling takeover-command job (a red ack job posts nothing; engagement is scan-driven and unaffected). Pinned both ways. * fix(autofix): command-style comments are instructions, not review feedback Motivating case: a maintainer posted '@qwen-code /triage' on a bot PR and the loop burned a full agent cycle (checkout, install, build, agent) to publish a no-action report explaining that the command is not feedback. The takeover command introduced here would self-trigger the same round on every engagement. Comments matching '^\s*@qwen-code /' (any author) are now excluded at all four feedback-decision sites — the scan's issue-comment count, the NEWEST computation, the LIVE_NEW recount, and the prompt renderer — alongside the existing bot-marker filter. Behaviorally proven: a trusted takeover-command comment newer than the live watermark no longer rescues a stale duplicate into an agent round, and the raw jq counts real feedback while dropping the command. 59/59 + 12/12. * fix(shepherd): fork check fails closed, matching the autofix convention Review suggestion, adopted verbatim: the fleet filter kept '.isCrossRepository != true' (fail-open on a missing field) while the autofix candidates migrated to '== false' in the same PR. Aligned; the filter replay now includes a missing-field row and proves it is rejected. 12/12. * fix(autofix): trusted runner staging, live author privilege, rotation, sentinel watermark Auto-review of the merged head (16 findings; 11 adopted, 3 rebutted in replies, 1 rename, 1 already-covered): - The address agent step invoked run-agent.mjs from the CHECKED-OUT PR branch with the model key in env — branch-controlled code on the host (takeover targets human branches). The runner is now staged from the trusted base into RUNNER_TEMP (same pattern as the schema gate) and invoked from there. - Author command privilege is LIVE, not durable: authors must hold triage+ today, so an ex-member's authorship no longer summons secret-bearing runs (the sugar's audience — members below write — still qualifies). - The terminal-handoff sentinel ts is excluded from watermark computation (scan and prepare): it is a flag, not an evaluation time, and it previously made a re-arm after a terminal handoff dead on arrival by filtering all future feedback forever. Terminal skipping stays round-based and thus window-scoped. - Candidate inspection gains a rotating start offset (fixed newest-first plus the budget starved the oldest tail FOREVER once the pool exceeded the budget). - issue_comment events get their own per-PR command group: bursts coalesce away from review routes, and pending-slot replacement is exactly latest-intent. - The queued toggle re-verifies OPEN + base=main; the cap notice honors dry-run and re-verifies live consent before posting; the release ack on a skip-labeled bot PR now says skip governs. - Shepherd: live_skip is reason-aware (an API outage is reported as fail-closed, never as consent withdrawn) and both levers check their budgets BEFORE the PAT-backed live read. - Tests: the supersede fixture is now discriminating (old-window marker lands AFTER the ack — timestamp-windowing would have counted it); the force-push guard catches combined short options (-uf); the sentinel-watermark rule and rotation are replayed; the labels test is renamed to stop claiming the comment surface is closed. 60/60 + 12/12. * test(autofix): restore recheck-before-checkout ordering pin; pin all win-marker sites Two review suggestions, both fallout from the merge-resolution test unification: the eligibility recheck's BEFORE-checkout ordering assertion (lost with the replaced pin test) is back inside the replay test, and all three eval-marker producers now carry per-site win= format pins (the global count-3 catches removal but not a lose-one-gain-a-duplicate swap). 60/60. * fix(autofix): trusted-commenter cmd groups; hooks severed at every host checkout; honest model-key framing - The issue_comment command group now requires a trusted-looking payload association (same prefilter pattern as reviews): an arbitrary commenter's rejected command can no longer cancel a maintainer's queued command out of the shared per-PR group. - Both verification checkouts (review gate and issue publish) sever hooks like every other host checkout — no secret sits in those steps, but branch post-checkout code on the host broke the convention (five severing sites now pinned). - The agent-step comment and the PR risk section now state the model key's real exposure explicitly: the CLI forwards OPENAI_API_KEY into the sandbox and the agent's job is to build/test the branch, so a taken-over branch's scripts can read AUTOFIX_OPENAI_API_KEY — an accepted, documented consequence of takeover (PAT remains fully severed via trusted staging + hook severing); the key should be low-privilege and rotatable. 60/60 + 12/12. * fix(autofix): label events get their own route group; non-takeover labels never start jobs Two review suggestions, adopted verbatim: pull_request label events now group as route-label-{N} (distinct from the review group — a simultaneous review and label toggle on the same PR can no longer cancel each other), and the route job gate filters pull_request events to the takeover label, so a triage labeling session across dozens of PRs burns zero runner slots. 60/60. --------- Co-authored-by: wenshao <wenshao@example.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Hardens the review-address pipeline against stale targets and untrusted route events — a follow-up fixing the five Critical findings from #7127's post-merge review (4728499913):
Decide phases— losing the real-time scan. Reviews whose payload does not already look trusted (repo associationOWNER/MEMBER/COLLABORATOR, or the review bot) now get a run-unique group: they cancel nothing and still go through the full permission gate inside the job. The payload check is a cheap pre-filter for group assignment only; a trusted sender it misses merely loses coalescing.main, unchanged head branch); anything else — including a failed fetch (unknown ≠ eligible) — discards silently with all downstream outputs published.MAX_ROUNDSyet proceed on newer feedback, doing round-6 work and double-writing a capped marker. The job now adopts the live round (both reporters consume the effective round) and discards when it is already at the cap — mirroring the scan, which skips capped PRs before even looking at conflicts.stale=truediscard, a latealways()step failure (e.g. artifact upload) flipped the job to failure and the failure reporter posted a handoff + eval marker — converting a deliberate no-comment/no-marker discard into a consumed round. ThePOST_HANDOFFdecision now short-circuits on stale.Why it's needed
All five are reachable on current main and three of them waste or corrupt the loop's core bookkeeping: replayed feedback produces duplicate or contradictory changes (3), a concealed round cap breaks the "5 rounds then a human" contract (4), and a handoff on a discarded duplicate consumes a round for work that never happened (5). (1) is a griefing vector on a public repository — any account could suppress real-time routing for a targeted PR with junk reviews. (2) runs the secret-bearing agent against closed PRs, pushing branches and commenting where nobody asked.
Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 53/53. New/extended behavioral replays (all extracted VERBATIM from the workflow, drift fails the suite):9999-…is never adopted as a watermark.STALE=true+ empty outcome + failed job → no handoff (previous cases unchanged).git checkout -B, covers state/author/fork/base/branch, and the discard path publishes every output later gates read.contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) || github.event.review.user.login == 'qwen-code-ci-bot'before granting the per-PR group; the literal list mirrorsTRUSTED_ASSOCand the login mirrorsREVIEW_BOT(pinned against each other in the tests).run:block passesbash -n.Evidence (Before & After)
Tested on
Environment (optional)
Contract suite via vitest on a worktree checkout of main; YAML/
bash -nstatic validation. The workflow itself runs onubuntu-latest.Risk & Scope
Decide phasesstill authorizes them.Linked Issues
Follow-up to #7127 (post-merge review 4728499913).
中文说明
本 PR 做了什么
加固 review-address 流水线,修复 #7127 合入后评审(4728499913)的五个 Critical:
Decide phases内因鉴权失败而死 —— 实时扫描就此丢失。现在载荷看起来不可信的 review(非OWNER/MEMBER/COLLABORATOR关联、也非 review bot)拿到 run 级唯一组:什么都取消不了,进入 job 后仍走完整权限门。载荷检查只是分组用的廉价预过滤;漏判的可信发送者只损失合并去重,不损失路由。main、head 分支未变);不符合 —— 包括拉取失败(未知 ≠ 合格)—— 一律静默丢弃,且丢弃路径发布所有下游 outputs。MAX_ROUNDS却因有新反馈继续,做第 6 轮工作并重复写 capped 标记。现在任务采纳 live 轮次(两个报告步骤均消费生效轮次),已达上限即丢弃 —— 与扫描一致(扫描在看冲突之前就跳过 capped PR)。stale=true丢弃后,若靠后的always()步骤(如工件上传)失败把 job 翻红,失败报告器会发 handoff + eval 标记 —— 把刻意的"无评论无标记"丢弃变成消耗一轮。POST_HANDOFF判定现在对 stale 短路。为什么需要
五项在当前 main 上全部可达,其中三项直接浪费或污染回路的核心记账:重放反馈产生重复或矛盾的更改(3);隐藏轮次上限破坏"5 轮后交人"契约(4);对已丢弃任务发 handoff 白白消耗一轮(5)。(1) 是公共仓库上的滋扰向量 —— 任何账号都能用垃圾 review 压制目标 PR 的实时路由。(2) 会对已关闭的 PR 运行带 secret 的 agent、推分支、发评论。
评审验证方案
如何验证
npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 53/53。新增/扩展的行为回放(全部从工作流逐字提取,漂移即测试失败):9999-…绝不被采纳为水印。STALE=true+ 空 outcome + job 失败 → 不发 handoff(原有用例不变)。git checkout -B之前,覆盖 state/作者/fork/base/分支,丢弃路径发布全部下游 outputs。contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.review.author_association) || github.event.review.user.login == 'qwen-code-ci-bot'才授予 per-PR 组;字面量列表与TRUSTED_ASSOC、登录名与REVIEW_BOT在测试中互相钉住。run:块通过bash -n。证据(前后对比)
测试情况
环境(可选)
在 main 的 worktree 检出上运行 vitest 契约套件;YAML/
bash -n静态校验。工作流本身运行于ubuntu-latest。风险与范围
Decide phases仍会为其放行。关联 Issue
#7127 合入后评审(4728499913)的后续。