From f51482fb9438a60b45d8ffd4eac016a2db18a05d Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Fri, 14 Aug 2026 18:49:26 +0000 Subject: [PATCH 01/18] feat(ci): route non-functional PRs to a triage-only review path (#7411) Small behavior-neutral maintenance PRs (internal renames, formatting, comment/JSDoc fixes) currently go through the full multi-stage triage, review, and verification pipeline. Add a conservative Stage 1f route in the triage skill that classifies such PRs (100%-certainty bar, risky surfaces excluded, unattended runs only) and marks them with the existing status/on-hold label, and make the automatic PR-review lane check the live label before invoking the model. Explicit /review asks and review_requested always bypass the skip. --- .github/workflows/qwen-code-pr-review.yml | 22 +++ .qwen/skills/triage/references/pr-workflow.md | 93 ++++++++++++ scripts/tests/qwen-pr-review-workflow.test.js | 137 ++++++++++++++++++ scripts/tests/qwen-triage-workflow.test.js | 68 +++++++++ 4 files changed, 320 insertions(+) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 0370d0024a8..b77d37ad9b9 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -456,6 +456,8 @@ jobs: id: 'context' env: TRIGGER_BODY: "${{ github.event.comment.body || github.event.review.body || '' }}" + # Read-only label lookup for the triage-only skip below. + GH_TOKEN: '${{ secrets.GITHUB_TOKEN }}' run: |- set -euo pipefail DEFAULT_TIMEOUT_MINUTES=180 @@ -510,6 +512,26 @@ jobs: exit 1 fi + # Triage-only route (#7411): the triage skill routes PRs it + # classifies as behavior-neutral maintenance (renames, formatting, + # comment/JSDoc-only) to a triage summary instead of the full + # review, marking them with `status/on-hold` (pr-workflow.md, + # Stage 1f). The automatic lane honours that mark; check the LIVE + # labels, not the event payload — the label lands after the event + # fires, and this step runs again after the review delay. Explicit + # asks (/review comments, review_requested, dispatch) never reach + # this check, and removing the label re-enables the automatic lane + # on the next push. A failed label read skips the optimisation and + # lets the review run (fail open: the worst case is one extra full + # review, never a silently skipped one). + if [ "$AUTO_REVIEW" = "true" ]; then + if gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels?per_page=100" --paginate --jq '.[].name' | grep -Fxq 'status/on-hold'; then + echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" + echo "should_run=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + fi + if [ -n "$TRIGGER_COMMAND" ]; then set -f for token in $TRIGGER_COMMAND; do diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index cd122a165e1..9e6f60c56c1 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -250,6 +250,98 @@ If any file matches (the strongest triage-time signal — 10 of 31 reverted PRs This signal is NOT a terminal gate — it does not stop the review or close the PR. It escalates review depth and flags risk so the reviewer knows where to focus. A PR that touches high-risk paths but passes full review with clean E2E verification can still be approved. +**1f. Non-functional change routing (triage-only outcome):** + +Behavior-neutral maintenance PRs — internal renames, formatting, comment/JSDoc +fixes — burn the full multi-stage review while carrying no observable risk +(#7411). Route them to a lightweight triage-only outcome instead, so review +capacity goes to PRs that can break something. The classifier must be +conservative: **when in doubt, take the full path.** A false skip (a risky +change classified as neutral) silently loses review coverage — that is the +dangerous failure mode here. + +**Applies only to unattended CI runs** (`GITHUB_EVENT_NAME=issues` or +`pull_request_target`). Explicit re-runs (`issue_comment`, +`workflow_dispatch`) and local invocations always take the full path — a +human asked for a review, so give them one. + +**Preconditions — every one must hold before this route is even considered:** + +- Stages 0 and 1a–1e pass without escalation, and 1e matched no high-risk path; +- the title is not a `fix` type — a fix claims a behaviour change by definition; +- no core module paths (Stage 0 list), no `.github/workflows/**`, no package + manifests or lockfiles, no schema/generated files, no prompts/system + instructions or other model-visible text, no user-facing documentation or + CLI help strings, no broken-link changes; +- at most 100 production logic lines (Stage 0 size calculation). + +**Classification — 100% certainty required.** Every hunk must be exactly one of: + +1. a source-comment or JSDoc-only edit; +2. a whitespace/formatting change with no token-level semantic change; +3. an internal identifier rename (variable, field, local function) where every + occurrence is updated consistently within the diff and no external contract + changes — no exported symbol, CLI flag, config key, serialized field, or + snapshot/test expectation. + +A hunk that touches a string literal, a number, control flow, a dependency, +configuration, or anything you cannot certify as behaviour-neutral sends the +whole PR to the full path. Title, description, and line count are not +classification evidence — read the diff. + +**Triage-only outcome (terminal — no review submitted):** + +1. Apply the existing label (never create one): + +```bash +gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' +``` + +2. Post the Stage 1 comment using the triage-only variant below. +3. Stop — no Stage 2, no Stage 3, no approval, no CHANGES_REQUESTED. + +The automated review workflow checks the live `status/on-hold` label before +invoking the model and skips the automatic lane while it is present +(`qwen-code-pr-review.yml`, 'Resolve PR context'). Maintainers pull a full +review at any time with `@qwen-code /review` — explicit triggers bypass the +label check — and removing the label re-enables the automatic lane on the next +push. Nothing here blocks merging or closes the PR. + +```markdown + + +Thanks for the PR! + +Template looks good ✓ + +This looks like a behaviour-neutral maintenance change (), so it takes the lightweight +triage-only path instead of the full automated review: `status/on-hold` is +applied and no review verdict will post automatically. Nothing is blocked — a +maintainer can start a full review at any time with `@qwen-code /review`, and +removing the label re-enables the automatic lane. If this cleanup pairs +naturally with a substantive change, consider folding it in there. + +
+中文说明 + +感谢贡献！ + +模板完整 ✓ + +这是一个行为中性的维护性改动（<一句话证据，如"仅 N 个文件的注释/JSDoc 修正">）， +因此走轻量 triage-only 路径，不再自动进入完整评审：已添加 `status/on-hold` +标签，不会自动发布评审结论。合并不受阻塞——maintainer 可随时用 +`@qwen-code /review` 发起完整评审；移除该标签后，下次推送会恢复自动评审。 +如果这项清理适合随某个实质性改动一起提交，也可以考虑合并进去。 + +
+ +— _Qwen Code · qwen3.7-max_ + +Reviewed at `` · re-run with `@qwen-code /triage` +``` + Post a single Stage 1 comment. Be direct — say what you actually think, not what's polite: ```markdown @@ -305,6 +397,7 @@ Save this comment's ID. Terminal exits — stop here if any applies: - Template failure (Stage 1a) → stopped. - Problem does not exist (Stage 1b) → request changes, do not proceed to Stage 2. - Direction escalated (Stage 1c) → stop here. +- Non-functional triage-only route (Stage 1f) → `status/on-hold` applied and triage summary posted, do not proceed to Stage 2. ### Stage 2: Review + Test diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index db4e46297dc..10fc26a1e46 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2517,3 +2517,140 @@ describe('bot comment markers', () => { expect(ackLine).toContain('"$RUN_URL"'); }); }); + +describe('qwen pr review triage-only skip (#7411)', () => { + // Executes the real 'Resolve PR context' bash with the expression + // placeholders resolved for one trigger shape and a stub `gh` whose label + // answer is scripted — the skip must come from LIVE labels, never from the + // event payload, and only on the automatic lane. + function runContextStep({ + eventName, + action = 'opened', + labels = [], + ghFails = false, + triggerBody = '', + }) { + const doc = parse(workflow); + const step = doc.jobs['review-pr'].steps.find( + (s) => s.name === 'Resolve PR context', + ); + let run = step.run; + const subs = { + '${{ github.event_name }}': eventName, + '${{ github.event.action }}': action, + '${{ github.event.pull_request.number }}': '4242', + '${{ github.event.issue.number }}': '4242', + '${{ github.event.inputs.pr_number }}': '4242', + '${{ github.event.inputs.review_mode }}': 'dry-run', + "${{ github.event.inputs.timeout_minutes || '180' }}": '180', + }; + for (const [expr, value] of Object.entries(subs)) { + run = run.split(expr).join(value); + } + expect(run).not.toContain('${{'); + + const dir = mkdtempSync(join(tmpdir(), 'pr-review-context-')); + const callsFile = join(dir, 'gh-calls'); + const ghPath = join(dir, 'gh'); + writeFileSync( + ghPath, + ghFails + ? ['#!/usr/bin/env bash', 'echo "API error" >&2', 'exit 1', ''].join( + '\n', + ) + : [ + '#!/usr/bin/env bash', + 'echo "$@" >> "$CONTEXT_GH_CALLS"', + "printf '%s\\n' $CONTEXT_LABELS", + '', + ].join('\n'), + ); + chmodSync(ghPath, 0o755); + const outputFile = join(dir, 'output'); + const summaryFile = join(dir, 'summary'); + writeFileSync(outputFile, ''); + writeFileSync(summaryFile, ''); + execFileSync('bash', ['-c', run], { + env: { + PATH: [dir, process.env.PATH].join(':'), + GITHUB_OUTPUT: outputFile, + GITHUB_STEP_SUMMARY: summaryFile, + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + GH_TOKEN: 'test-token', + TRIGGER_BODY: triggerBody, + CONTEXT_LABELS: labels.join(' '), + CONTEXT_GH_CALLS: callsFile, + }, + }); + return { + output: readFileSync(outputFile, 'utf8'), + summary: readFileSync(summaryFile, 'utf8'), + ghCalls: existsSync(callsFile) ? readFileSync(callsFile, 'utf8') : '', + dir, + }; + } + + it('skips the automatic review when the LIVE labels carry status/on-hold', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['type/bug', 'status/on-hold'], + }); + expect(r.output).toContain('should_run=false'); + expect(r.output).not.toContain('should_run=true'); + // The check must query the labels endpoint, not trust the event payload: + // the label is applied AFTER the triggering event fires. + expect(r.ghCalls).toContain('issues/4242/labels'); + expect(r.summary).toContain('status/on-hold'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('matches the label exactly — a lookalike label must not skip', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold-extended'], + }); + expect(r.output).toContain('should_run=true'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('still reviews automatically when the label is absent', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['type/bug'], + }); + expect(r.output).toContain('should_run=true'); + expect(r.output).toContain('auto_review=true'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('fails open: an unreadable label list never silently skips the review', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + ghFails: true, + }); + expect(r.output).toContain('should_run=true'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('never skips an explicit /review ask, whatever the labels say', () => { + const r = runContextStep({ + eventName: 'issue_comment', + labels: ['status/on-hold'], + triggerBody: '@qwen-code /review', + }); + expect(r.output).toContain('should_run=true'); + expect(r.ghCalls).not.toContain('labels'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('never skips a maintainer review_requested, whatever the labels say', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + action: 'review_requested', + labels: ['status/on-hold'], + }); + expect(r.output).toContain('should_run=true'); + expect(r.ghCalls).not.toContain('labels'); + rmSync(r.dir, { recursive: true, force: true }); + }); +}); diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 9f121ad0191..11e855c783f 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -5982,3 +5982,71 @@ describe('triage job budget', () => { } }); }); + +describe('triage skill non-functional routing (#7411)', () => { + // Stage 1f routes behavior-neutral maintenance PRs to a triage-only + // outcome. A false skip silently loses review coverage, so the contract + // words the conservative fallback, the unattended-only scope, the + // exclusions, and the maintainer override — pin all of them. + function stage1f() { + const start = prSkill.indexOf('**1f. Non-functional change routing'); + expect(start).toBeGreaterThan(-1); + const end = prSkill.indexOf('Post a single Stage 1 comment', start); + expect(end).toBeGreaterThan(start); + // The prose wraps mid-sentence; normalise whitespace so assertions are + // robust to rewrapping. + return prSkill.slice(start, end).replace(/\s+/g, ' '); + } + + it('fails closed: doubt always takes the full path', () => { + const section = stage1f(); + expect(section).toContain('when in doubt, take the full path'); + expect(section).toContain('100% certainty required'); + // Classification evidence is the diff, never the metadata. + expect(section).toContain('Title, description, and line count are not'); + }); + + it('applies only to unattended runs; explicit asks always get a review', () => { + const section = stage1f(); + expect(section).toContain('Applies only to unattended CI runs'); + expect(section).toContain('GITHUB_EVENT_NAME=issues'); + expect(section).toContain('always take the full path'); + }); + + it('keeps risky surfaces out of the triage-only route', () => { + const section = stage1f(); + for (const excluded of [ + 'fix', + '.github/workflows/**', + 'lockfiles', + 'schema/generated files', + 'model-visible text', + 'CLI help strings', + 'broken-link changes', + 'high-risk path', + ]) { + expect(section, excluded).toContain(excluded); + } + expect(section).toContain('at most 100 production logic lines'); + }); + + it('reuses the existing on-hold label and stops before Stage 2', () => { + const section = stage1f(); + expect(section).toContain( + 'gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label \'status/on-hold\'', + ); + expect(section).toContain('never create one'); + expect(section).toContain('no Stage 2, no Stage 3, no approval'); + // Maintainer override stays available and is named explicitly. + expect(section).toContain('@qwen-code /review'); + expect(section).toContain('explicit triggers bypass the label check'); + }); + + it('lists the triage-only route as a Stage 1 terminal exit', () => { + const exits = prSkill.slice( + prSkill.indexOf('Terminal exits — stop here if any applies'), + prSkill.indexOf('### Stage 2: Review + Test'), + ); + expect(exits).toContain('Non-functional triage-only route (Stage 1f)'); + }); +}); From 75ced85c6a76e2661dbe39e0b173635eb64e4d0e Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 15 Aug 2026 10:18:52 +0800 Subject: [PATCH 02/18] chore(ci): retrigger failed checks From b80a1a97924db5e08f58f67484fb6b5391e8bb6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Sat, 15 Aug 2026 10:26:32 +0000 Subject: [PATCH 03/18] fix(ci): pin the triage-only skip to the triaged head SHA (#9219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The status/on-hold label is applied once at open time by Stage 1f and is never invalidated, so a PR earning it with a benign v1 would silently skip the automatic review lane for every later push of arbitrary content — a fork contributor could smuggle behaviour changes past review with zero classifier error. - Stage 1f now also posts a marker comment pinning the skip to the triaged head SHA (``) - the skip fires only while that pin matches the LIVE head: a push after triage moves the head (triage does not re-run on synchronize), so the new diff goes to the review lane; a manually applied parked label has no marker and never skips; an unreadable head fails open like the label read - pin the new behaviour with workflow step tests (matching pin skips, stale pin reviews, marker-less label reviews, unreadable head fails open) and a Stage 1f doc pin for the marker instruction --- .github/workflows/qwen-code-pr-review.yml | 20 +++++-- .qwen/skills/triage/references/pr-workflow.md | 25 ++++++--- scripts/tests/qwen-pr-review-workflow.test.js | 54 +++++++++++++++++-- scripts/tests/qwen-triage-workflow.test.js | 14 +++++ 4 files changed, 100 insertions(+), 13 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index b77d37ad9b9..bab81c2fdf6 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -524,11 +524,25 @@ jobs: # on the next push. A failed label read skips the optimisation and # lets the review run (fail open: the worst case is one extra full # review, never a silently skipped one). + # + # The label alone is NOT sufficient (#9219): Stage 1f applies it + # once at open time and nothing ever removes it, so a PR that + # earns it with a benign v1 would silently skip review for every + # later push of arbitrary content. Skip only while the triage run's + # marker comment is pinned to the LIVE head SHA; a push after + # triage moves the head (triage does not re-run on synchronize), + # stale pin -> the new diff goes to the review lane. A manually + # applied "parked" label has no marker and never skips. Unreadable + # head fails open like the label read. if [ "$AUTO_REVIEW" = "true" ]; then if gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels?per_page=100" --paginate --jq '.[].name' | grep -Fxq 'status/on-hold'; then - echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" - echo "should_run=false" >> "$GITHUB_OUTPUT" - exit 0 + HEAD_SHA="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true)" + if [ -n "$HEAD_SHA" ] && gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[].body' | grep -Fq "qwen-triage on-hold sha=${HEAD_SHA}"; then + echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold pinned to head ${HEAD_SHA} (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" + echo "should_run=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + echo "status/on-hold present but not pinned to the live head (${HEAD_SHA:-unreadable}) — running the review lane (#9219)." >> "$GITHUB_STEP_SUMMARY" fi fi diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 9e6f60c56c1..00c0effcde2 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -291,21 +291,32 @@ classification evidence — read the diff. **Triage-only outcome (terminal — no review submitted):** -1. Apply the existing label (never create one): +1. Apply the existing label (never create one) AND pin the skip to the + triaged head SHA with a marker comment — the review lane honours the + label only while this pin matches the live head, so a later push + re-enables review automatically (#9219): ```bash gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' +gh pr comment "$PR_NUMBER" --repo "$REPO" --body '' ``` + `` is the head commit you triaged — the same SHA quoted in the + Stage 1 "Reviewed at" footer. Post the marker exactly once per triage run. + 2. Post the Stage 1 comment using the triage-only variant below. 3. Stop — no Stage 2, no Stage 3, no approval, no CHANGES_REQUESTED. -The automated review workflow checks the live `status/on-hold` label before -invoking the model and skips the automatic lane while it is present -(`qwen-code-pr-review.yml`, 'Resolve PR context'). Maintainers pull a full -review at any time with `@qwen-code /review` — explicit triggers bypass the -label check — and removing the label re-enables the automatic lane on the next -push. Nothing here blocks merging or closes the PR. +The automated review workflow checks the live `status/on-hold` label AND the +marker pin before invoking the model and skips the automatic lane only while +the marker's SHA matches the live head (`qwen-code-pr-review.yml`, 'Resolve +PR context'). Triage does not re-run on `synchronize`, so a push after the +triage-only outcome invalidates the pin and the new diff receives the full +review — a benign v1 must not grant later pushes a silent bypass. Maintainers +pull a full review at any time with `@qwen-code /review` — explicit triggers +bypass the label check — and removing the label re-enables the automatic lane +on the next push. A manually applied `status/on-hold` carries no marker and +never skips. Nothing here blocks merging or closes the PR. ```markdown diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 10fc26a1e46..07b0ae56808 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2529,6 +2529,8 @@ describe('qwen pr review triage-only skip (#7411)', () => { labels = [], ghFails = false, triggerBody = '', + headSha = 'abc123def456', + onHoldMarkers = [], }) { const doc = parse(workflow); const step = doc.jobs['review-pr'].steps.find( @@ -2561,7 +2563,12 @@ describe('qwen pr review triage-only skip (#7411)', () => { : [ '#!/usr/bin/env bash', 'echo "$@" >> "$CONTEXT_GH_CALLS"', - "printf '%s\\n' $CONTEXT_LABELS", + 'case "$*" in', + ' */labels*) printf \'%s\\n\' $CONTEXT_LABELS ;;', + ' */comments*) printf \'%s\\n\' "$CONTEXT_MARKERS" ;;', + ' *pulls/*) printf \'%s\\n\' "$CONTEXT_HEAD_SHA" ;;', + ' *) ;;', + 'esac', '', ].join('\n'), ); @@ -2579,6 +2586,8 @@ describe('qwen pr review triage-only skip (#7411)', () => { GH_TOKEN: 'test-token', TRIGGER_BODY: triggerBody, CONTEXT_LABELS: labels.join(' '), + CONTEXT_MARKERS: onHoldMarkers.join('\n'), + CONTEXT_HEAD_SHA: headSha, CONTEXT_GH_CALLS: callsFile, }, }); @@ -2590,20 +2599,59 @@ describe('qwen pr review triage-only skip (#7411)', () => { }; } - it('skips the automatic review when the LIVE labels carry status/on-hold', () => { + it('skips the automatic review when status/on-hold is pinned to the live head', () => { const r = runContextStep({ eventName: 'pull_request_target', labels: ['type/bug', 'status/on-hold'], + headSha: 'abc123def456', + onHoldMarkers: [''], }); expect(r.output).toContain('should_run=false'); expect(r.output).not.toContain('should_run=true'); // The check must query the labels endpoint, not trust the event payload: - // the label is applied AFTER the triggering event fires. + // the label is applied AFTER the triggering event fires. It must also + // read the live head SHA and the marker comments (#9219). expect(r.ghCalls).toContain('issues/4242/labels'); + expect(r.ghCalls).toContain('pulls/4242'); + expect(r.ghCalls).toContain('issues/4242/comments'); expect(r.summary).toContain('status/on-hold'); rmSync(r.dir, { recursive: true, force: true }); }); + it('a push after triage invalidates the pin — the new diff is reviewed (#9219)', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: 'newhead999', + onHoldMarkers: [''], + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain('not pinned to the live head'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('a manually applied on-hold label without a marker never skips (#9219)', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + onHoldMarkers: [], + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain('not pinned to the live head'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('fails open when the live head SHA is unreadable (#9219)', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: '', + onHoldMarkers: [''], + }); + expect(r.output).toContain('should_run=true'); + rmSync(r.dir, { recursive: true, force: true }); + }); + it('matches the label exactly — a lookalike label must not skip', () => { const r = runContextStep({ eventName: 'pull_request_target', diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 11e855c783f..94ce4496dd3 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6042,6 +6042,20 @@ describe('triage skill non-functional routing (#7411)', () => { expect(section).toContain('explicit triggers bypass the label check'); }); + it('pins the skip to the triaged head SHA with a marker comment (#9219)', () => { + const section = stage1f(); + // The label alone would skip review for every future push; the marker + // pins the skip to the triaged head so a later push re-enables review. + expect(section).toContain( + 'gh pr comment "$PR_NUMBER" --repo "$REPO" --body \'\'', + ); + expect(section).toContain( + "skips the automatic lane only while the marker's SHA matches the live head", + ); + expect(section).toContain('Triage does not re-run on `synchronize`'); + expect(section).toContain('carries no marker and never skips'); + }); + it('lists the triage-only route as a Stage 1 terminal exit', () => { const exits = prSkill.slice( prSkill.indexOf('Terminal exits — stop here if any applies'), From 102cefb9297708d92c52cb64520c4e264c77380f Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 15 Aug 2026 21:39:01 +0800 Subject: [PATCH 04/18] fix(ci): authenticate triage-only markers --- .github/workflows/qwen-code-pr-review.yml | 6 ++-- scripts/tests/qwen-pr-review-workflow.test.js | 28 +++++++++++++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 70cc123babe..6a840173f95 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -551,9 +551,11 @@ jobs: # applied "parked" label has no marker and never skips. Unreadable # head fails open like the label read. if [ "$AUTO_REVIEW" = "true" ]; then - if gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels?per_page=100" --paginate --jq '.[].name' | grep -Fxq 'status/on-hold'; then + LABELS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels?per_page=100" --paginate --jq '.[].name' 2>/dev/null || true)" + if printf '%s\n' "$LABELS" | grep -Fxq 'status/on-hold'; then HEAD_SHA="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true)" - if [ -n "$HEAD_SHA" ] && gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[].body' | grep -Fq "qwen-triage on-hold sha=${HEAD_SHA}"; then + MARKERS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[] | select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot") | .body' 2>/dev/null || true)" + if [ -n "$HEAD_SHA" ] && printf '%s\n' "$MARKERS" | grep -Fq "qwen-triage on-hold sha=${HEAD_SHA}"; then echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold pinned to head ${HEAD_SHA} (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" echo "should_run=false" >> "$GITHUB_OUTPUT" exit 0 diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 30c99864c1a..bac226e4d12 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2552,6 +2552,11 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(run).not.toContain('${{'); const dir = mkdtempSync(join(tmpdir(), 'pr-review-context-')); + const markerRows = onHoldMarkers.map((marker) => + typeof marker === 'string' + ? { author: 'qwen-code-ci-bot', body: marker } + : marker, + ); const callsFile = join(dir, 'gh-calls'); const ghPath = join(dir, 'gh'); writeFileSync( @@ -2565,7 +2570,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { 'echo "$@" >> "$CONTEXT_GH_CALLS"', 'case "$*" in', " */labels*) printf '%s\\n' $CONTEXT_LABELS ;;", - ' */comments*) printf \'%s\\n\' "$CONTEXT_MARKERS" ;;', + ' */comments*) node -e \'const rows = JSON.parse(process.env.CONTEXT_MARKERS || "[]"); for (const row of rows) if (["github-actions[bot]", "qwen-code-ci-bot"].includes(row.author)) console.log(row.body);\' ;;', ' *pulls/*) printf \'%s\\n\' "$CONTEXT_HEAD_SHA" ;;', ' *) ;;', 'esac', @@ -2580,13 +2585,14 @@ describe('qwen pr review triage-only skip (#7411)', () => { execFileSync('bash', ['-c', run], { env: { PATH: [dir, process.env.PATH].join(':'), + HOME: dir, GITHUB_OUTPUT: outputFile, GITHUB_STEP_SUMMARY: summaryFile, GITHUB_REPOSITORY: 'QwenLM/qwen-code', GH_TOKEN: 'test-token', TRIGGER_BODY: triggerBody, CONTEXT_LABELS: labels.join(' '), - CONTEXT_MARKERS: onHoldMarkers.join('\n'), + CONTEXT_MARKERS: JSON.stringify(markerRows), CONTEXT_HEAD_SHA: headSha, CONTEXT_GH_CALLS: callsFile, }, @@ -2641,6 +2647,24 @@ describe('qwen pr review triage-only skip (#7411)', () => { rmSync(r.dir, { recursive: true, force: true }); }); + it('ignores forged on-hold markers from non-bot comments (#9219)', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: 'abc123def456', + onHoldMarkers: [ + { + author: 'contributor', + body: '', + }, + ], + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain('not pinned to the live head'); + expect(r.ghCalls).toContain('.user.login'); + rmSync(r.dir, { recursive: true, force: true }); + }); + it('fails open when the live head SHA is unreadable (#9219)', () => { const r = runContextStep({ eventName: 'pull_request_target', From 0760f41d0326c716f6c9debe1943ad5309e36ab7 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 15 Aug 2026 15:34:48 +0000 Subject: [PATCH 05/18] fix(ci): drop the printf|grep pipes from the triage-only skip gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review of the triage-only route (#7411): - The marker match no longer pipes `printf '%s\n' "$MARKERS"` into `grep -Fq`: under `set -o pipefail`, grep -q exits at the first match and closes the pipe, printf dies on EPIPE with 141, and pipefail surfaces the producer failure as FALSE — with >64 KB of bot comment bodies (busy PRs) the skip silently never fired while the summary claimed the opposite. The marker check is now a pipe-free `[[ "$MARKERS" == *"..."* ]]` pattern match; the label check keeps exact-line semantics via a herestring (`grep -Fxq <<< "$LABELS"`, no producer pipe). Regression test: the stub emits a 300 KB bot comment body alongside the marker and the skip must still fire — mutation-verified red against the old shape. - Pin the author-filter jq expression verbatim in the positive and forged-marker tests (the stub does its own filtering, so the '.user.login' substring alone let select(.user.login != "") and dropped-login mutants survive). - Stage 1f posts its pin marker through the author-scoped upsert-bot-comment.sh (lookup marker is the SHA-less prefix) instead of a bare `gh pr comment`, so a re-run PATCHes the existing marker instead of minting a byte-identical duplicate. - The triage-only Stage 1 wording qualifies the "no review verdict will post" promise for the ready_for_review race (the review lane starts immediately on that action while triage takes minutes). --- .github/workflows/qwen-code-pr-review.yml | 11 +++++- .qwen/skills/triage/references/pr-workflow.md | 20 +++++++--- scripts/tests/qwen-pr-review-workflow.test.js | 38 ++++++++++++++++++- 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 6a840173f95..6ec548234c3 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -551,11 +551,18 @@ jobs: # applied "parked" label has no marker and never skips. Unreadable # head fails open like the label read. if [ "$AUTO_REVIEW" = "true" ]; then + # No `printf | grep -q` pipes below: under `set -o pipefail` + # the -q consumer exits at the first match and closes the + # pipe, so the producer (printf builtin or gh) dies on EPIPE + # with 141 and pipefail surfaces that as FALSE even when the + # marker matches — on busy PRs (>64 KB of bot comment bodies) + # the skip would silently never fire. Herestrings and [[ == ]] + # pattern matches have no pipe and no early-exit producer. LABELS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels?per_page=100" --paginate --jq '.[].name' 2>/dev/null || true)" - if printf '%s\n' "$LABELS" | grep -Fxq 'status/on-hold'; then + if grep -Fxq 'status/on-hold' <<< "$LABELS"; then HEAD_SHA="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true)" MARKERS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[] | select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot") | .body' 2>/dev/null || true)" - if [ -n "$HEAD_SHA" ] && printf '%s\n' "$MARKERS" | grep -Fq "qwen-triage on-hold sha=${HEAD_SHA}"; then + if [ -n "$HEAD_SHA" ] && [[ "$MARKERS" == *"qwen-triage on-hold sha=${HEAD_SHA}"* ]]; then echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold pinned to head ${HEAD_SHA} (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" echo "should_run=false" >> "$GITHUB_OUTPUT" exit 0 diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 00c0effcde2..5c4fb99748f 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -298,11 +298,17 @@ classification evidence — read the diff. ```bash gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' -gh pr comment "$PR_NUMBER" --repo "$REPO" --body '' +printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md +.github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" 'qwen-triage on-hold sha=' /tmp/qwen-triage-on-hold-marker.md ``` `` is the head commit you triaged — the same SHA quoted in the - Stage 1 "Reviewed at" footer. Post the marker exactly once per triage run. + Stage 1 "Reviewed at" footer. Post the marker through the author-scoped + upsert helper (never a bare `gh pr comment`): a re-run of the same + triage then PATCHes the existing marker in place instead of minting a + byte-identical duplicate, and a later triage of a new head updates the + same comment to the new pin. The lookup marker is the SHA-less prefix + so the existing comment is found across heads. 2. Post the Stage 1 comment using the triage-only variant below. 3. Stop — no Stage 2, no Stage 3, no approval, no CHANGES_REQUESTED. @@ -328,9 +334,13 @@ Template looks good ✓ This looks like a behaviour-neutral maintenance change (), so it takes the lightweight triage-only path instead of the full automated review: `status/on-hold` is -applied and no review verdict will post automatically. Nothing is blocked — a -maintainer can start a full review at any time with `@qwen-code /review`, and -removing the label re-enables the automatic lane. If this cleanup pairs +applied and no review verdict will post automatically. Nothing is blocked — +a maintainer can start a full review at any time with `@qwen-code /review`, +and removing the label re-enables the automatic lane. If this cleanup pairs naturally with a substantive change, consider folding it in there.
diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index bac226e4d12..e7184740de7 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2531,6 +2531,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { triggerBody = '', headSha = 'abc123def456', onHoldMarkers = [], + markerPadBytes = 0, }) { const doc = parse(workflow); const step = doc.jobs['review-pr'].steps.find( @@ -2570,7 +2571,9 @@ describe('qwen pr review triage-only skip (#7411)', () => { 'echo "$@" >> "$CONTEXT_GH_CALLS"', 'case "$*" in', " */labels*) printf '%s\\n' $CONTEXT_LABELS ;;", - ' */comments*) node -e \'const rows = JSON.parse(process.env.CONTEXT_MARKERS || "[]"); for (const row of rows) if (["github-actions[bot]", "qwen-code-ci-bot"].includes(row.author)) console.log(row.body);\' ;;', + ' */comments*)', + ' node -e \'const rows = JSON.parse(process.env.CONTEXT_MARKERS || "[]"); for (const row of rows) if (["github-actions[bot]", "qwen-code-ci-bot"].includes(row.author)) console.log(row.body);\'', + ' if [ "${CONTEXT_MARKER_PAD:-0}" != "0" ]; then head -c "$CONTEXT_MARKER_PAD" /dev/zero | tr "\\\\0" "x"; echo; fi ;;', ' *pulls/*) printf \'%s\\n\' "$CONTEXT_HEAD_SHA" ;;', ' *) ;;', 'esac', @@ -2593,6 +2596,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { TRIGGER_BODY: triggerBody, CONTEXT_LABELS: labels.join(' '), CONTEXT_MARKERS: JSON.stringify(markerRows), + CONTEXT_MARKER_PAD: String(markerPadBytes), CONTEXT_HEAD_SHA: headSha, CONTEXT_GH_CALLS: callsFile, }, @@ -2620,10 +2624,38 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(r.ghCalls).toContain('issues/4242/labels'); expect(r.ghCalls).toContain('pulls/4242'); expect(r.ghCalls).toContain('issues/4242/comments'); + // Pin the filter EXPRESSION, not just its substring: the stub does its + // own author filtering, so a mutated jq that merely contains + // '.user.login' (e.g. select(.user.login != "")) would otherwise pass + // here while silently accepting forged markers in production (#9219). + expect(r.ghCalls).toContain( + 'select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot")', + ); expect(r.summary).toContain('status/on-hold'); rmSync(r.dir, { recursive: true, force: true }); }); + it('still skips when bot comment bodies exceed the pipe buffer', () => { + // Under `set -o pipefail` the old `printf '%s\n' "$MARKERS" | grep -Fq` + // shape read FALSE once $MARKERS crossed the 64 KB pipe buffer: grep + // -q exited at the first match, printf died on EPIPE with 141, and + // pipefail surfaced the producer failure — on busy PRs the skip + // silently never fired while the summary claimed the opposite (#9219). + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: 'abc123def456', + onHoldMarkers: [''], + // The stub emits a further 300 KB bot comment body (the round-3 + // probe size), generated inside the stub so the step's $MARKERS can + // exceed the env-string limit the harness itself is bound by. + markerPadBytes: 300_000, + }); + expect(r.output).toContain('should_run=false'); + expect(r.output).not.toContain('should_run=true'); + rmSync(r.dir, { recursive: true, force: true }); + }); + it('a push after triage invalidates the pin — the new diff is reviewed (#9219)', () => { const r = runContextStep({ eventName: 'pull_request_target', @@ -2661,7 +2693,9 @@ describe('qwen pr review triage-only skip (#7411)', () => { }); expect(r.output).toContain('should_run=true'); expect(r.summary).toContain('not pinned to the live head'); - expect(r.ghCalls).toContain('.user.login'); + expect(r.ghCalls).toContain( + 'select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot")', + ); rmSync(r.dir, { recursive: true, force: true }); }); From 1430f84af584039a8b1dbc30d28ab5ff5ef045ce Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 00:16:14 +0800 Subject: [PATCH 06/18] test(ci): align triage marker pin assertion --- scripts/tests/qwen-triage-workflow.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 94ce4496dd3..03da5618985 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6047,7 +6047,10 @@ describe('triage skill non-functional routing (#7411)', () => { // The label alone would skip review for every future push; the marker // pins the skip to the triaged head so a later push re-enables review. expect(section).toContain( - 'gh pr comment "$PR_NUMBER" --repo "$REPO" --body \'\'', + "printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md", + ); + expect(section).toContain( + '.github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" \'qwen-triage on-hold sha=\' /tmp/qwen-triage-on-hold-marker.md', ); expect(section).toContain( "skips the automatic lane only while the marker's SHA matches the live head", From 5c02b55a08e08cbf3165ca5c568a834100243e95 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 15 Aug 2026 20:34:30 +0000 Subject: [PATCH 07/18] fix(ci): close triage-route review round (#9193) - skill doc: post the on-hold marker BEFORE applying the label and fail closed if the upsert fails (a label without the pin would skip review for every future push) - skill doc: mirror the ready_for_review race hedge into the ZH half of the Stage 1f comment (both halves post in one comment) - retag the misplaced (#9219) provenance citations to (#9193) at the remaining sites (workflow, step summary, skill doc, tests); the marker-pin assertion itself was already aligned in 1430f84af - tests: supply genuine bot markers in the lookalike-label and label-absent skip-gate tests so the exact-label and label gates are each the only failing gate (kills the relaxed-match / dropped-gate mutants); pin the step-level GH_TOKEN env on 'Resolve PR context' --- .github/workflows/qwen-code-pr-review.yml | 4 +-- .qwen/skills/triage/references/pr-workflow.md | 14 ++++++-- scripts/tests/qwen-pr-review-workflow.test.js | 36 +++++++++++++++---- scripts/tests/qwen-triage-workflow.test.js | 4 ++- 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 6ec548234c3..f4cafd56359 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -541,7 +541,7 @@ jobs: # lets the review run (fail open: the worst case is one extra full # review, never a silently skipped one). # - # The label alone is NOT sufficient (#9219): Stage 1f applies it + # The label alone is NOT sufficient (#9193): Stage 1f applies it # once at open time and nothing ever removes it, so a PR that # earns it with a benign v1 would silently skip review for every # later push of arbitrary content. Skip only while the triage run's @@ -567,7 +567,7 @@ jobs: echo "should_run=false" >> "$GITHUB_OUTPUT" exit 0 fi - echo "status/on-hold present but not pinned to the live head (${HEAD_SHA:-unreadable}) — running the review lane (#9219)." >> "$GITHUB_STEP_SUMMARY" + echo "status/on-hold present but not pinned to the live head (${HEAD_SHA:-unreadable}) — running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" fi fi diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 5c4fb99748f..25ba05c833c 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -294,14 +294,20 @@ classification evidence — read the diff. 1. Apply the existing label (never create one) AND pin the skip to the triaged head SHA with a marker comment — the review lane honours the label only while this pin matches the live head, so a later push - re-enables review automatically (#9219): + re-enables review automatically (#9193): ```bash -gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md .github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" 'qwen-triage on-hold sha=' /tmp/qwen-triage-on-hold-marker.md +gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' ``` + Fail closed, in this order: post the marker FIRST. If the upsert fails + (retries exhausted), stop — do not apply the label and do not post the + triage-only outcome. A label without the pin would skip review for every + future push, and the outcome comment's promise would post over a skip + that is no longer bounded to the triaged head. + `` is the head commit you triaged — the same SHA quoted in the Stage 1 "Reviewed at" footer. Post the marker through the author-scoped upsert helper (never a bare `gh pr comment`): a re-run of the same @@ -352,7 +358,9 @@ naturally with a substantive change, consider folding it in there. 这是一个行为中性的维护性改动（<一句话证据，如"仅 N 个文件的注释/JSDoc 修正">）， 因此走轻量 triage-only 路径，不再自动进入完整评审：已添加 `status/on-hold` -标签，不会自动发布评审结论。合并不受阻塞——maintainer 可随时用 +标签，不会自动发布评审结论<例外：当触发动作是 `ready_for_review`（PR 刚被标记为就绪）时， +可能已有一个评审运行在路上——两条通道无法互相同步，此时改说"与该 pin 竞争的这次运行 +仍可能发布评审结论；pin 对后续运行生效"，不要给出绝对承诺>。合并不受阻塞——maintainer 可随时用 `@qwen-code /review` 发起完整评审；移除该标签后，下次推送会恢复自动评审。 如果这项清理适合随某个实质性改动一起提交，也可以考虑合并进去。 diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index e7184740de7..16cd01ee8c7 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2620,14 +2620,14 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(r.output).not.toContain('should_run=true'); // The check must query the labels endpoint, not trust the event payload: // the label is applied AFTER the triggering event fires. It must also - // read the live head SHA and the marker comments (#9219). + // read the live head SHA and the marker comments (#9193). expect(r.ghCalls).toContain('issues/4242/labels'); expect(r.ghCalls).toContain('pulls/4242'); expect(r.ghCalls).toContain('issues/4242/comments'); // Pin the filter EXPRESSION, not just its substring: the stub does its // own author filtering, so a mutated jq that merely contains // '.user.login' (e.g. select(.user.login != "")) would otherwise pass - // here while silently accepting forged markers in production (#9219). + // here while silently accepting forged markers in production (#9193). expect(r.ghCalls).toContain( 'select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot")', ); @@ -2640,7 +2640,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { // shape read FALSE once $MARKERS crossed the 64 KB pipe buffer: grep // -q exited at the first match, printf died on EPIPE with 141, and // pipefail surfaced the producer failure — on busy PRs the skip - // silently never fired while the summary claimed the opposite (#9219). + // silently never fired while the summary claimed the opposite (#9193). const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold'], @@ -2656,7 +2656,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { rmSync(r.dir, { recursive: true, force: true }); }); - it('a push after triage invalidates the pin — the new diff is reviewed (#9219)', () => { + it('a push after triage invalidates the pin — the new diff is reviewed (#9193)', () => { const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold'], @@ -2668,7 +2668,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { rmSync(r.dir, { recursive: true, force: true }); }); - it('a manually applied on-hold label without a marker never skips (#9219)', () => { + it('a manually applied on-hold label without a marker never skips (#9193)', () => { const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold'], @@ -2679,7 +2679,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { rmSync(r.dir, { recursive: true, force: true }); }); - it('ignores forged on-hold markers from non-bot comments (#9219)', () => { + it('ignores forged on-hold markers from non-bot comments (#9193)', () => { const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold'], @@ -2699,7 +2699,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { rmSync(r.dir, { recursive: true, force: true }); }); - it('fails open when the live head SHA is unreadable (#9219)', () => { + it('fails open when the live head SHA is unreadable (#9193)', () => { const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold'], @@ -2711,18 +2711,28 @@ describe('qwen pr review triage-only skip (#7411)', () => { }); it('matches the label exactly — a lookalike label must not skip', () => { + // A genuine bot marker matching the live head is supplied so the + // exact-label match is the ONLY gate failing: relaxing it to substring + // matching would let the lookalike label + marker skip the review. const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold-extended'], + headSha: 'abc123def456', + onHoldMarkers: [''], }); expect(r.output).toContain('should_run=true'); rmSync(r.dir, { recursive: true, force: true }); }); it('still reviews automatically when the label is absent', () => { + // A genuine bot marker matching the live head is supplied so the label + // gate is the ONLY gate failing: dropping it would let the marker alone + // skip the review. const r = runContextStep({ eventName: 'pull_request_target', labels: ['type/bug'], + headSha: 'abc123def456', + onHoldMarkers: [''], }); expect(r.output).toContain('should_run=true'); expect(r.output).toContain('auto_review=true'); @@ -2738,6 +2748,18 @@ describe('qwen pr review triage-only skip (#7411)', () => { rmSync(r.dir, { recursive: true, force: true }); }); + it('pins the GH_TOKEN env on the Resolve PR context step (#9193)', () => { + // The skip gate's `gh api` reads authenticate through this step-level + // env; the `run` block never references it textually, so deleting the + // line passes every behavioural test (the harness supplies its own + // GH_TOKEN) while the reads fail unauthenticated in production. + const doc = parse(workflow); + const step = doc.jobs['review-pr'].steps.find( + (s) => s.name === 'Resolve PR context', + ); + expect(step.env?.GH_TOKEN).toBe('${{ secrets.GITHUB_TOKEN }}'); + }); + it('never skips an explicit /review ask, whatever the labels say', () => { const r = runContextStep({ eventName: 'issue_comment', diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 03da5618985..9a7019579f0 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6042,10 +6042,12 @@ describe('triage skill non-functional routing (#7411)', () => { expect(section).toContain('explicit triggers bypass the label check'); }); - it('pins the skip to the triaged head SHA with a marker comment (#9219)', () => { + it('pins the skip to the triaged head SHA with a marker comment (#9193)', () => { const section = stage1f(); // The label alone would skip review for every future push; the marker // pins the skip to the triaged head so a later push re-enables review. + // The marker posts through the author-scoped upsert helper (never a bare + // `gh pr comment`) so a re-run PATCHes the existing pin in place. expect(section).toContain( "printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md", ); From a577d64437a7087e573c0e6aade5cb134286f75c Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 02:34:15 +0000 Subject: [PATCH 08/18] fix(ci): make skip-gate read failures diagnosable + correct doc rationale (#9193) - R5-4: the skip gate's fail-open paths now write distinct summary lines per failed read (label read failed / head SHA unreadable / marker comments unreadable) instead of silence or a false stale-pin claim; a failed marker read can no longer report 'not pinned to the live head' for a mismatch never observed. Tests pin all three diagnostics, with a new harness knob that fails only the comments read. - R5-2: the fail-closed rationale in the Stage 1f doc described the pre-pin label-only behaviour ('a label without the pin would skip every future push'), contradicting the shipped gate (label AND pin required; label-without-pin runs the review lane). Rewritten to the actual harm of a partial state: the outcome promise broken while the unpinned label leaves the automatic lane running. - R4-4: cross-artifact identity test tying the skill-doc marker writer (upsert prefix + template) to the skip-gate reader literal, so a one-sided rename of the shared prefix fails the suite. --- .github/workflows/qwen-code-pr-review.yml | 27 +++++++-- .qwen/skills/triage/references/pr-workflow.md | 11 +++- scripts/tests/qwen-pr-review-workflow.test.js | 57 +++++++++++++++++++ 3 files changed, 87 insertions(+), 8 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index f4cafd56359..7b7a1d4aab6 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -558,16 +558,33 @@ jobs: # marker matches — on busy PRs (>64 KB of bot comment bodies) # the skip would silently never fire. Herestrings and [[ == ]] # pattern matches have no pipe and no early-exit producer. - LABELS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels?per_page=100" --paginate --jq '.[].name' 2>/dev/null || true)" - if grep -Fxq 'status/on-hold' <<< "$LABELS"; then + # + # Read failures stay fail-open but are never silent: each + # failed read writes its own summary line, so a skip disabled + # by an API failure, rate-limit window, or token regression is + # diagnosable from the run artifacts — and a failed read is + # never reported as a stale pin, which would assert a mismatch + # that was never observed (R5-4, #9193 review). + LABELS_READ_OK=true + LABELS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels?per_page=100" --paginate --jq '.[].name' 2>/dev/null)" || LABELS_READ_OK=false + if [ "$LABELS_READ_OK" = false ]; then + echo "status/on-hold skip gate: label read failed — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + elif grep -Fxq 'status/on-hold' <<< "$LABELS"; then HEAD_SHA="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true)" - MARKERS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[] | select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot") | .body' 2>/dev/null || true)" - if [ -n "$HEAD_SHA" ] && [[ "$MARKERS" == *"qwen-triage on-hold sha=${HEAD_SHA}"* ]]; then + MARKERS_READ_OK=true + MARKERS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[] | select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot") | .body' 2>/dev/null)" || MARKERS_READ_OK=false + if [ -n "$HEAD_SHA" ] && [ "$MARKERS_READ_OK" = true ] && [[ "$MARKERS" == *"qwen-triage on-hold sha=${HEAD_SHA}"* ]]; then echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold pinned to head ${HEAD_SHA} (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" echo "should_run=false" >> "$GITHUB_OUTPUT" exit 0 fi - echo "status/on-hold present but not pinned to the live head (${HEAD_SHA:-unreadable}) — running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + if [ -z "$HEAD_SHA" ]; then + echo "status/on-hold present but the live head SHA was unreadable — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + elif [ "$MARKERS_READ_OK" = false ]; then + echo "status/on-hold present but the marker comments were unreadable — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + else + echo "status/on-hold present but not pinned to the live head (${HEAD_SHA}) — running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + fi fi fi diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 25ba05c833c..a0d7a3453ea 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -304,9 +304,14 @@ gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' Fail closed, in this order: post the marker FIRST. If the upsert fails (retries exhausted), stop — do not apply the label and do not post the - triage-only outcome. A label without the pin would skip review for every - future push, and the outcome comment's promise would post over a skip - that is no longer bounded to the triaged head. + triage-only outcome. The review-lane gate requires the label AND a pin + matching the live head (a label without the pin runs the review lane), + so the harm of a partial state is not a silent skip — it is the inverse: + the outcome comment promises that no review verdict will post + automatically while the unpinned label leaves the automatic lane + running on every push, and the label alone reads as "parked" with no + pin to explain why. All three — pin, label, outcome — post together or + not at all. `` is the head commit you triaged — the same SHA quoted in the Stage 1 "Reviewed at" footer. Post the marker through the author-scoped diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 16cd01ee8c7..5ef69cabbe4 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2528,6 +2528,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { action = 'opened', labels = [], ghFails = false, + markersFail = false, triggerBody = '', headSha = 'abc123def456', onHoldMarkers = [], @@ -2572,6 +2573,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { 'case "$*" in', " */labels*) printf '%s\\n' $CONTEXT_LABELS ;;", ' */comments*)', + ' if [ "${CONTEXT_MARKERS_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi', ' node -e \'const rows = JSON.parse(process.env.CONTEXT_MARKERS || "[]"); for (const row of rows) if (["github-actions[bot]", "qwen-code-ci-bot"].includes(row.author)) console.log(row.body);\'', ' if [ "${CONTEXT_MARKER_PAD:-0}" != "0" ]; then head -c "$CONTEXT_MARKER_PAD" /dev/zero | tr "\\\\0" "x"; echo; fi ;;', ' *pulls/*) printf \'%s\\n\' "$CONTEXT_HEAD_SHA" ;;', @@ -2596,6 +2598,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { TRIGGER_BODY: triggerBody, CONTEXT_LABELS: labels.join(' '), CONTEXT_MARKERS: JSON.stringify(markerRows), + CONTEXT_MARKERS_FAIL: markersFail ? '1' : '0', CONTEXT_MARKER_PAD: String(markerPadBytes), CONTEXT_HEAD_SHA: headSha, CONTEXT_GH_CALLS: callsFile, @@ -2707,6 +2710,10 @@ describe('qwen pr review triage-only skip (#7411)', () => { onHoldMarkers: [''], }); expect(r.output).toContain('should_run=true'); + // An unreadable head is reported as such — never as a stale pin, which + // would assert a mismatch that was never observed (R5-4, #9193 review). + expect(r.summary).toContain('live head SHA was unreadable'); + expect(r.summary).not.toContain('not pinned to the live head'); rmSync(r.dir, { recursive: true, force: true }); }); @@ -2745,6 +2752,27 @@ describe('qwen pr review triage-only skip (#7411)', () => { ghFails: true, }); expect(r.output).toContain('should_run=true'); + // The fail-open must be DIAGNOSABLE: a skip disabled by an API failure + // writes its own summary line instead of vanishing silently — the only + // symptom used to be the continued capacity burn this PR eliminates + // (R5-4, #9193 review). + expect(r.summary).toContain('label read failed'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('fails open: unreadable marker comments never skip and never masquerade as a stale pin (#9193)', () => { + // Labels read fine but the comments read fails: before R5-4 this + // reported "not pinned to the live head" — a mismatch never observed. + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: 'abc123def456', + onHoldMarkers: [''], + markersFail: true, + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain('marker comments were unreadable'); + expect(r.summary).not.toContain('not pinned to the live head'); rmSync(r.dir, { recursive: true, force: true }); }); @@ -2760,6 +2788,35 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(step.env?.GH_TOKEN).toBe('${{ secrets.GITHUB_TOKEN }}'); }); + it('ties the marker writer side to the skip-gate reader (#9193)', () => { + // R4-4: the marker grammar lives in two artifacts — the skill doc's + // upsert call (writer) and the skip gate's substring match (reader). + // A one-sided rename of the shared prefix makes the gate silently + // never fire (the label stays applied, every push burns a full + // review), so pin both sides to the same literal (#9193 review). + const skillDoc = readFileSync( + join('.qwen', 'skills', 'triage', 'references', 'pr-workflow.md'), + 'utf8', + ); + const writerMatch = skillDoc.match( + /upsert-bot-comment\.sh "\$REPO" "\$PR_NUMBER" '([^']+)'/, + ); + expect(writerMatch).not.toBeNull(); + const writerPrefix = writerMatch[1]; + // The marker template in the same doc leads with the same prefix. + expect(skillDoc).toContain( + '', + ); + // The gate matches that exact prefix, keyed to the live head. + const doc = parse(workflow); + const step = doc.jobs['review-pr'].steps.find( + (s) => s.name === 'Resolve PR context', + ); + expect(step.run).toContain( + '*"' + writerPrefix + '${HEAD_SHA}"*', + ); + }); + it('never skips an explicit /review ask, whatever the labels say', () => { const r = runContextStep({ eventName: 'issue_comment', From f9e2e1e4e53125b61731328ecf5fdbc2956915ce Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 05:27:59 +0000 Subject: [PATCH 09/18] test(ci): pin --paginate on the skip gate's list reads (#9193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R6-7: the recorded gh-call assertions matched only the bare endpoint substrings, so deleting --paginate from the labels/comments reads kept the suite green — a pin beyond page 1 (a re-triage after 100+ comments) would be invisible to the gate, which would then misreport 'not pinned' and burn a full review on every event: silent, fail-open, undiagnosable. Pin both list reads including the flag, the same shape the sibling triage/autofix suites use. Mutation-verified: dropping --paginate from the comments read now fails exactly the skip test. --- scripts/tests/qwen-pr-review-workflow.test.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 5ef69cabbe4..bbf72c58cc0 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2623,10 +2623,17 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(r.output).not.toContain('should_run=true'); // The check must query the labels endpoint, not trust the event payload: // the label is applied AFTER the triggering event fires. It must also - // read the live head SHA and the marker comments (#9193). - expect(r.ghCalls).toContain('issues/4242/labels'); + // read the live head SHA and the marker comments (#9193). Both list + // reads carry `--paginate`: without it a pin sitting beyond page 1 + // (a re-triage after 100+ comments accumulated) is invisible to the + // gate, which then misreports "not pinned" and burns a full review on + // every event — silent, fail-open, and green in CI (R6-7, #9193 + // review; pinned the same way as the sibling triage/autofix reads). + expect(r.ghCalls).toContain('issues/4242/labels?per_page=100 --paginate'); expect(r.ghCalls).toContain('pulls/4242'); - expect(r.ghCalls).toContain('issues/4242/comments'); + expect(r.ghCalls).toContain( + 'issues/4242/comments?per_page=100 --paginate', + ); // Pin the filter EXPRESSION, not just its substring: the stub does its // own author filtering, so a mutated jq that merely contains // '.user.login' (e.g. select(.user.login != "")) would otherwise pass From ef9b6f4deb2ba85c10c2330f7f264e5291fc28ec Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 07:28:20 +0000 Subject: [PATCH 10/18] fix(ci): carve whitespace-is-syntax files out of the triage-only route (#9193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-9 critical: clause 2 ('whitespace/formatting change with no token-level semantic change') is worded for tokenized languages — in YAML/Makefile/shell/Dockerfile a pure re-indent hunk changes the parse tree while every line's token sequence stays identical, yet passed every written precondition (only .github/workflows/** was excluded) and would skip the review lane: the exact false skip the section names as the dangerous failure mode. - clause 2 now forbids the certification in files where whitespace IS syntax (YAML indentation, Makefile recipe tabs, shell/Dockerfile line continuations) - the precondition exclusions extend to non-workflow YAML anywhere, Makefile, Dockerfile, and .github/scripts/** - the Stage 1f test block pins both the new exclusions and the carve-out wording --- .qwen/skills/triage/references/pr-workflow.md | 16 +++++++++++----- scripts/tests/qwen-triage-workflow.test.js | 11 +++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index a0d7a3453ea..ece446950bd 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -269,16 +269,22 @@ human asked for a review, so give them one. - Stages 0 and 1a–1e pass without escalation, and 1e matched no high-risk path; - the title is not a `fix` type — a fix claims a behaviour change by definition; -- no core module paths (Stage 0 list), no `.github/workflows/**`, no package - manifests or lockfiles, no schema/generated files, no prompts/system - instructions or other model-visible text, no user-facing documentation or - CLI help strings, no broken-link changes; +- no core module paths (Stage 0 list), no `.github/workflows/**`, no other + YAML (`*.yml`/`*.yaml` anywhere, e.g. `.yamllint.yml`, + `.github/dependabot.yml`, `.github/release.yml`), no `Makefile`, + `Dockerfile`, or `.github/scripts/**`, no package manifests or lockfiles, + no schema/generated files, no prompts/system instructions or other + model-visible text, no user-facing documentation or CLI help strings, no + broken-link changes; - at most 100 production logic lines (Stage 0 size calculation). **Classification — 100% certainty required.** Every hunk must be exactly one of: 1. a source-comment or JSDoc-only edit; -2. a whitespace/formatting change with no token-level semantic change; +2. a whitespace/formatting change with no token-level semantic change — never + in files where whitespace IS syntax (YAML indentation, Makefile recipe + tabs, shell and Dockerfile line continuations); such edits send the whole + PR to the full path; 3. an internal identifier rename (variable, field, local function) where every occurrence is updated consistently within the diff and no external contract changes — no exported symbol, CLI flag, config key, serialized field, or diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 9a7019579f0..115df7bc903 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6018,6 +6018,10 @@ describe('triage skill non-functional routing (#7411)', () => { for (const excluded of [ 'fix', '.github/workflows/**', + 'no other YAML', + 'Makefile', + 'Dockerfile', + '.github/scripts/**', 'lockfiles', 'schema/generated files', 'model-visible text', @@ -6028,6 +6032,13 @@ describe('triage skill non-functional routing (#7411)', () => { expect(section, excluded).toContain(excluded); } expect(section).toContain('at most 100 production logic lines'); + // Whitespace IS syntax in YAML/Makefile/shell/Dockerfile: clause 2 must + // carve those files out, otherwise a pure re-indent hunk is certified + // behaviour-neutral and skips the review lane — the exact false skip + // this section names as dangerous (round-9 critical). + expect(section).toContain('whitespace IS syntax'); + expect(section).toContain('YAML indentation, Makefile recipe'); + expect(section).toContain('shell and Dockerfile line continuations'); }); it('reuses the existing on-hold label and stops before Stage 2', () => { From 5516a2aa76cf33440bef3fc436ef3d44f8486c2f Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 07:30:50 +0000 Subject: [PATCH 11/18] docs(ci): cover the label-failure branch in the Stage 1f fail-closed order (#9193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fail-closed paragraph prescribed only the upsert-failure branch while the invariant below it demands all-three-or-none: spell out the label-application-failure branch too — stop before the outcome; an orphaned pin alone cannot skip and a re-run upserts the same pin and retries the label. --- .qwen/skills/triage/references/pr-workflow.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index ece446950bd..a1403d5b2c0 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -310,7 +310,12 @@ gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' Fail closed, in this order: post the marker FIRST. If the upsert fails (retries exhausted), stop — do not apply the label and do not post the - triage-only outcome. The review-lane gate requires the label AND a pin + triage-only outcome. If the label application fails AFTER the pin + posted, stop the same way — do not post the outcome: an orphaned pin + alone cannot skip (the gate requires the label too), and a re-run of + the triage upserts the same pin in place and retries the label; but an + outcome posted without the label would promise a skip the gate does + not grant. The review-lane gate requires the label AND a pin matching the live head (a label without the pin runs the review lane), so the harm of a partial state is not a silent skip — it is the inverse: the outcome comment promises that no review verdict will post From 742ed718e88ca2aa555227fa65966a3d85d54409 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 22:36:17 +0800 Subject: [PATCH 12/18] fix(ci): pin triage-only skips to base --- .github/workflows/qwen-code-pr-review.yml | 13 ++-- .qwen/skills/triage/references/pr-workflow.md | 77 ++++++++++--------- scripts/tests/qwen-pr-review-workflow.test.js | 77 ++++++++++++++----- scripts/tests/qwen-triage-workflow.test.js | 7 +- 4 files changed, 107 insertions(+), 67 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 3af7436bd59..e8b42342366 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -583,20 +583,21 @@ jobs: if [ "$LABELS_READ_OK" = false ]; then echo "status/on-hold skip gate: label read failed — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" elif grep -Fxq 'status/on-hold' <<< "$LABELS"; then - HEAD_SHA="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/dev/null || true)" + PR_REFS="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '[.head.sha, .base.sha] | @tsv' 2>/dev/null || true)" + IFS=$'\t' read -r HEAD_SHA BASE_SHA <<< "$PR_REFS" MARKERS_READ_OK=true MARKERS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[] | select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot") | .body' 2>/dev/null)" || MARKERS_READ_OK=false - if [ -n "$HEAD_SHA" ] && [ "$MARKERS_READ_OK" = true ] && [[ "$MARKERS" == *"qwen-triage on-hold sha=${HEAD_SHA}"* ]]; then - echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold pinned to head ${HEAD_SHA} (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" + if [ -n "$HEAD_SHA" ] && [ -n "$BASE_SHA" ] && [ "$MARKERS_READ_OK" = true ] && [[ "$MARKERS" == *"qwen-triage on-hold sha=${HEAD_SHA} base=${BASE_SHA}"* ]]; then + echo "Skipping automatic review: PR #${PR_NUMBER} carries status/on-hold pinned to head ${HEAD_SHA} and base ${BASE_SHA} (triage-only route). A maintainer can run @qwen-code /review explicitly." >> "$GITHUB_STEP_SUMMARY" echo "should_run=false" >> "$GITHUB_OUTPUT" exit 0 fi - if [ -z "$HEAD_SHA" ]; then - echo "status/on-hold present but the live head SHA was unreadable — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + if [ -z "$HEAD_SHA" ] || [ -z "$BASE_SHA" ]; then + echo "status/on-hold present but the live head/base SHA was unreadable — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" elif [ "$MARKERS_READ_OK" = false ]; then echo "status/on-hold present but the marker comments were unreadable — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" else - echo "status/on-hold present but not pinned to the live head (${HEAD_SHA}) — running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + echo "status/on-hold present but not pinned to the live head/base (${HEAD_SHA}/${BASE_SHA}) — running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" fi fi fi diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index a1403d5b2c0..6ac50a2aa90 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -298,53 +298,56 @@ classification evidence — read the diff. **Triage-only outcome (terminal — no review submitted):** 1. Apply the existing label (never create one) AND pin the skip to the - triaged head SHA with a marker comment — the review lane honours the - label only while this pin matches the live head, so a later push + triaged head/base SHA pair with a marker comment — the review lane honours the + label only while this pin matches the live head and base, so a later push or retarget re-enables review automatically (#9193): ```bash -printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md +BASE_SHA=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefOid --jq '.baseRefOid') || exit 1 +[ -n "$BASE_SHA" ] || { echo 'empty base SHA — fail closed'; exit 1; } +printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md .github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" 'qwen-triage on-hold sha=' /tmp/qwen-triage-on-hold-marker.md -gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label 'status/on-hold' -``` - - Fail closed, in this order: post the marker FIRST. If the upsert fails - (retries exhausted), stop — do not apply the label and do not post the - triage-only outcome. If the label application fails AFTER the pin - posted, stop the same way — do not post the outcome: an orphaned pin - alone cannot skip (the gate requires the label too), and a re-run of - the triage upserts the same pin in place and retries the label; but an - outcome posted without the label would promise a skip the gate does - not grant. The review-lane gate requires the label AND a pin - matching the live head (a label without the pin runs the review lane), - so the harm of a partial state is not a silent skip — it is the inverse: - the outcome comment promises that no review verdict will post - automatically while the unpinned label leaves the automatic lane - running on every push, and the label alone reads as "parked" with no - pin to explain why. All three — pin, label, outcome — post together or - not at all. - - `` is the head commit you triaged — the same SHA quoted in the - Stage 1 "Reviewed at" footer. Post the marker through the author-scoped - upsert helper (never a bare `gh pr comment`): a re-run of the same - triage then PATCHes the existing marker in place instead of minting a - byte-identical duplicate, and a later triage of a new head updates the - same comment to the new pin. The lookup marker is the SHA-less prefix - so the existing comment is found across heads. +gh api --method POST "repos/$REPO/issues/$PR_NUMBER/labels" -f labels[]='status/on-hold' +``` + +Fail closed, in this order: post the marker FIRST. If the upsert fails +(retries exhausted), stop — do not apply the label and do not post the +triage-only outcome. If the label application fails AFTER the pin +posted, stop the same way — do not post the outcome: an orphaned pin +alone cannot skip (the gate requires the label too), and a re-run of +the triage upserts the same pin in place and retries the label; but an +outcome posted without the label would promise a skip the gate does +not grant. The review-lane gate requires the label AND a pin +matching the live head (a label without the pin runs the review lane), +so the harm of a partial state is not a silent skip — it is the inverse: +the outcome comment promises that no review verdict will post +automatically while the unpinned label leaves the automatic lane +running on every push, and the label alone reads as "parked" with no +pin to explain why. All three — pin, label, outcome — post together or +not at all. + +`` is the head commit you triaged — the same SHA quoted in the +Stage 1 "Reviewed at" footer. Post the marker through the author-scoped +upsert helper (never a bare `gh pr comment`): a re-run of the same +triage then PATCHes the existing marker in place instead of minting a +byte-identical duplicate, and a later triage of a new head updates the +same comment to the new pin. The lookup marker is the SHA-less prefix +so the existing comment is found across heads. 2. Post the Stage 1 comment using the triage-only variant below. 3. Stop — no Stage 2, no Stage 3, no approval, no CHANGES_REQUESTED. The automated review workflow checks the live `status/on-hold` label AND the marker pin before invoking the model and skips the automatic lane only while -the marker's SHA matches the live head (`qwen-code-pr-review.yml`, 'Resolve -PR context'). Triage does not re-run on `synchronize`, so a push after the -triage-only outcome invalidates the pin and the new diff receives the full -review — a benign v1 must not grant later pushes a silent bypass. Maintainers -pull a full review at any time with `@qwen-code /review` — explicit triggers -bypass the label check — and removing the label re-enables the automatic lane -on the next push. A manually applied `status/on-hold` carries no marker and -never skips. Nothing here blocks merging or closes the PR. +the marker's SHA pair matches the live head and base (`qwen-code-pr-review.yml`, +'Resolve PR context'). Triage does not re-run on `synchronize` or base retarget, +so either change after the triage-only outcome invalidates the pin and the new +diff receives the full review — a benign v1 must not grant later content or +base changes a silent bypass. Maintainers pull a full review at any time with +`@qwen-code /review` — explicit triggers bypass the label check — and removing +the label re-enables the automatic lane on the next push. A manually applied +`status/on-hold` carries no marker and never skips. Nothing here blocks merging +or closes the PR. ```markdown diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index e9138278c5c..abe05b42e26 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2538,6 +2538,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { markersFail = false, triggerBody = '', headSha = 'abc123def456', + baseSha = 'base777', onHoldMarkers = [], markerPadBytes = 0, }) { @@ -2583,7 +2584,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { ' if [ "${CONTEXT_MARKERS_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi', ' node -e \'const rows = JSON.parse(process.env.CONTEXT_MARKERS || "[]"); for (const row of rows) if (["github-actions[bot]", "qwen-code-ci-bot"].includes(row.author)) console.log(row.body);\'', ' if [ "${CONTEXT_MARKER_PAD:-0}" != "0" ]; then head -c "$CONTEXT_MARKER_PAD" /dev/zero | tr "\\\\0" "x"; echo; fi ;;', - ' *pulls/*) printf \'%s\\n\' "$CONTEXT_HEAD_SHA" ;;', + ' *pulls/*) printf \'%s\\t%s\\n\' "$CONTEXT_HEAD_SHA" "$CONTEXT_BASE_SHA" ;;', ' *) ;;', 'esac', '', @@ -2608,6 +2609,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { CONTEXT_MARKERS_FAIL: markersFail ? '1' : '0', CONTEXT_MARKER_PAD: String(markerPadBytes), CONTEXT_HEAD_SHA: headSha, + CONTEXT_BASE_SHA: baseSha, CONTEXT_GH_CALLS: callsFile, }, }); @@ -2624,7 +2626,9 @@ describe('qwen pr review triage-only skip (#7411)', () => { eventName: 'pull_request_target', labels: ['type/bug', 'status/on-hold'], headSha: 'abc123def456', - onHoldMarkers: [''], + onHoldMarkers: [ + '', + ], }); expect(r.output).toContain('should_run=false'); expect(r.output).not.toContain('should_run=true'); @@ -2660,7 +2664,9 @@ describe('qwen pr review triage-only skip (#7411)', () => { eventName: 'pull_request_target', labels: ['status/on-hold'], headSha: 'abc123def456', - onHoldMarkers: [''], + onHoldMarkers: [ + '', + ], // The stub emits a further 300 KB bot comment body (the round-3 // probe size), generated inside the stub so the step's $MARKERS can // exceed the env-string limit the harness itself is bound by. @@ -2676,10 +2682,27 @@ describe('qwen pr review triage-only skip (#7411)', () => { eventName: 'pull_request_target', labels: ['status/on-hold'], headSha: 'newhead999', - onHoldMarkers: [''], + onHoldMarkers: [ + '', + ], }); expect(r.output).toContain('should_run=true'); - expect(r.summary).toContain('not pinned to the live head'); + expect(r.summary).toContain('not pinned to the live head/base'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('a base retarget after triage invalidates the pin (#9193)', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: 'abc123def456', + baseSha: 'newbase888', + onHoldMarkers: [ + '', + ], + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain('not pinned to the live head/base'); rmSync(r.dir, { recursive: true, force: true }); }); @@ -2690,7 +2713,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { onHoldMarkers: [], }); expect(r.output).toContain('should_run=true'); - expect(r.summary).toContain('not pinned to the live head'); + expect(r.summary).toContain('not pinned to the live head/base'); rmSync(r.dir, { recursive: true, force: true }); }); @@ -2702,30 +2725,32 @@ describe('qwen pr review triage-only skip (#7411)', () => { onHoldMarkers: [ { author: 'contributor', - body: '', + body: '', }, ], }); expect(r.output).toContain('should_run=true'); - expect(r.summary).toContain('not pinned to the live head'); + expect(r.summary).toContain('not pinned to the live head/base'); expect(r.ghCalls).toContain( 'select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot")', ); rmSync(r.dir, { recursive: true, force: true }); }); - it('fails open when the live head SHA is unreadable (#9193)', () => { + it('fails open when the live head/base SHA is unreadable (#9193)', () => { const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold'], headSha: '', - onHoldMarkers: [''], + onHoldMarkers: [ + '', + ], }); expect(r.output).toContain('should_run=true'); - // An unreadable head is reported as such — never as a stale pin, which + // An unreadable head/base is reported as such — never as a stale pin, which // would assert a mismatch that was never observed (R5-4, #9193 review). - expect(r.summary).toContain('live head SHA was unreadable'); - expect(r.summary).not.toContain('not pinned to the live head'); + expect(r.summary).toContain('live head/base SHA was unreadable'); + expect(r.summary).not.toContain('not pinned to the live head/base'); rmSync(r.dir, { recursive: true, force: true }); }); @@ -2737,7 +2762,9 @@ describe('qwen pr review triage-only skip (#7411)', () => { eventName: 'pull_request_target', labels: ['status/on-hold-extended'], headSha: 'abc123def456', - onHoldMarkers: [''], + onHoldMarkers: [ + '', + ], }); expect(r.output).toContain('should_run=true'); rmSync(r.dir, { recursive: true, force: true }); @@ -2751,7 +2778,9 @@ describe('qwen pr review triage-only skip (#7411)', () => { eventName: 'pull_request_target', labels: ['type/bug'], headSha: 'abc123def456', - onHoldMarkers: [''], + onHoldMarkers: [ + '', + ], }); expect(r.output).toContain('should_run=true'); expect(r.output).toContain('auto_review=true'); @@ -2774,17 +2803,19 @@ describe('qwen pr review triage-only skip (#7411)', () => { it('fails open: unreadable marker comments never skip and never masquerade as a stale pin (#9193)', () => { // Labels read fine but the comments read fails: before R5-4 this - // reported "not pinned to the live head" — a mismatch never observed. + // reported "not pinned to the live head/base" — a mismatch never observed. const r = runContextStep({ eventName: 'pull_request_target', labels: ['status/on-hold'], headSha: 'abc123def456', - onHoldMarkers: [''], + onHoldMarkers: [ + '', + ], markersFail: true, }); expect(r.output).toContain('should_run=true'); expect(r.summary).toContain('marker comments were unreadable'); - expect(r.summary).not.toContain('not pinned to the live head'); + expect(r.summary).not.toContain('not pinned to the live head/base'); rmSync(r.dir, { recursive: true, force: true }); }); @@ -2816,13 +2847,17 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(writerMatch).not.toBeNull(); const writerPrefix = writerMatch[1]; // The marker template in the same doc leads with the same prefix. - expect(skillDoc).toContain(''); - // The gate matches that exact prefix, keyed to the live head. + expect(skillDoc).toContain( + '', + ); + // The gate matches that exact prefix, keyed to the live head/base pair. const doc = parse(workflow); const step = doc.jobs['review-pr'].steps.find( (s) => s.name === 'Resolve PR context', ); - expect(step.run).toContain('*"' + writerPrefix + '${HEAD_SHA}"*'); + expect(step.run).toContain( + '*"' + writerPrefix + '${HEAD_SHA} base=${BASE_SHA}"*', + ); }); it('never skips an explicit /review ask, whatever the labels say', () => { diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 115df7bc903..37fec07a3c0 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6044,7 +6044,7 @@ describe('triage skill non-functional routing (#7411)', () => { it('reuses the existing on-hold label and stops before Stage 2', () => { const section = stage1f(); expect(section).toContain( - 'gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label \'status/on-hold\'', + 'gh api --method POST "repos/$REPO/issues/$PR_NUMBER/labels" -f labels[]=\'status/on-hold\'', ); expect(section).toContain('never create one'); expect(section).toContain('no Stage 2, no Stage 3, no approval'); @@ -6060,15 +6060,16 @@ describe('triage skill non-functional routing (#7411)', () => { // The marker posts through the author-scoped upsert helper (never a bare // `gh pr comment`) so a re-run PATCHes the existing pin in place. expect(section).toContain( - "printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md", + "printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md", ); expect(section).toContain( '.github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" \'qwen-triage on-hold sha=\' /tmp/qwen-triage-on-hold-marker.md', ); expect(section).toContain( - "skips the automatic lane only while the marker's SHA matches the live head", + "skips the automatic lane only while the marker's SHA pair matches the live head and base", ); expect(section).toContain('Triage does not re-run on `synchronize`'); + expect(section).toContain('or base retarget'); expect(section).toContain('carries no marker and never skips'); }); From ee43e1cba9e327e448db2fb5048b2ab8f7e030eb Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 17 Aug 2026 00:10:38 +0800 Subject: [PATCH 13/18] fix(ci): strip stale status/on-hold when the triage pin no longer matches (#9193) When the review lane runs because status/on-hold is not pinned to the live head/base, remove the label too. It sits in the stale.yml exempt-pr-labels and Stage 1f applies it only once at open time, so without this a PR would skip review and never age out via the stale bot. --- .github/workflows/qwen-code-pr-review.yml | 10 ++++++++++ scripts/tests/qwen-pr-review-workflow.test.js | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index e8b42342366..ac11cee4d6b 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -598,6 +598,16 @@ jobs: echo "status/on-hold present but the marker comments were unreadable — failing open, running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" else echo "status/on-hold present but not pinned to the live head/base (${HEAD_SHA}/${BASE_SHA}) — running the review lane (#9193)." >> "$GITHUB_STEP_SUMMARY" + # The stale pin means the triage-only exemption no longer + # matches the live diff, so strip the label too. It sits in + # stale.yml's exempt-pr-labels and Stage 1f applies it only + # once at open time — without this the PR would skip the + # review lane AND never age out via the stale bot. + if gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels/status%2Fon-hold" -X DELETE 2>/dev/null; then + echo "Removed stale status/on-hold so the stale bot can age this PR (#9193)." >> "$GITHUB_STEP_SUMMARY" + else + echo "Failed to remove stale status/on-hold — leaving it in place (#9193)." >> "$GITHUB_STEP_SUMMARY" + fi fi fi fi diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index abe05b42e26..8af728ca104 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2688,6 +2688,10 @@ describe('qwen pr review triage-only skip (#7411)', () => { }); expect(r.output).toContain('should_run=true'); expect(r.summary).toContain('not pinned to the live head/base'); + // The stale exemption must not linger in stale.yml's exempt list. + expect(r.summary).toContain('Removed stale status/on-hold'); + expect(r.ghCalls).toContain('labels/status%2Fon-hold'); + expect(r.ghCalls).toContain('-X DELETE'); rmSync(r.dir, { recursive: true, force: true }); }); From 0966c62b126350490ca16ec39e224924f2de04ef Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 17 Aug 2026 00:21:58 +0800 Subject: [PATCH 14/18] fix(ci): pin the on-hold marker to a dedicated comment and harden Stage 1f (#9193) - require the marker to START a bot comment body (startswith), so a forged pin embedded in a larger bot comment cannot skip the lane - write the marker to a per-run mktemp file instead of a fixed /tmp path - document the head/base pair requirement consistently - pin the base-SHA fetch/empty guard, the mktemp+rm, and the marker-before-label ordering in the triage test --- .github/workflows/qwen-code-pr-review.yml | 19 ++++++----- .qwen/skills/triage/references/pr-workflow.md | 8 +++-- scripts/tests/qwen-pr-review-workflow.test.js | 34 ++++++++++++++++++- scripts/tests/qwen-triage-workflow.test.js | 17 ++++++++-- 4 files changed, 63 insertions(+), 15 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index ac11cee4d6b..c206fa1e0cf 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -555,14 +555,15 @@ jobs: # review, never a silently skipped one). # # The label alone is NOT sufficient (#9193): Stage 1f applies it - # once at open time and nothing ever removes it, so a PR that - # earns it with a benign v1 would silently skip review for every - # later push of arbitrary content. Skip only while the triage run's - # marker comment is pinned to the LIVE head SHA; a push after - # triage moves the head (triage does not re-run on synchronize), - # stale pin -> the new diff goes to the review lane. A manually - # applied "parked" label has no marker and never skips. Unreadable - # head fails open like the label read. + # once at open time and, until a stale pin strips it, it never + # leaves the exempt list — so a PR that earns it with a benign v1 + # could silently skip review for every later push of arbitrary + # content. Skip only while the triage run's marker comment is + # pinned to the LIVE head/base SHA pair; a push or base retarget + # after triage moves them (triage re-runs on neither), stale pin -> + # the new diff goes to the review lane, which also strips the + # label. A manually applied "parked" label has no marker and never + # skips. Unreadable head/base fails open like the label read. if [ "$AUTO_REVIEW" = "true" ]; then # No `printf | grep -q` pipes below: under `set -o pipefail` # the -q consumer exits at the first match and closes the @@ -586,7 +587,7 @@ jobs: PR_REFS="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '[.head.sha, .base.sha] | @tsv' 2>/dev/null || true)" IFS=$'\t' read -r HEAD_SHA BASE_SHA <<< "$PR_REFS" MARKERS_READ_OK=true - MARKERS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[] | select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot") | .body' 2>/dev/null)" || MARKERS_READ_OK=false + MARKERS="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments?per_page=100" --paginate --jq '.[] | select(.user.login == "github-actions[bot]" or .user.login == "qwen-code-ci-bot") | select(.body | startswith("' > /tmp/qwen-triage-on-hold-marker.md -.github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" 'qwen-triage on-hold sha=' /tmp/qwen-triage-on-hold-marker.md +MARKER_FILE="$(mktemp "${RUNNER_TEMP:-/tmp}/qwen-triage-on-hold-marker.XXXXXX")" +printf '%s' '' > "$MARKER_FILE" +.github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" 'qwen-triage on-hold sha=' "$MARKER_FILE" +rm -f "$MARKER_FILE" gh api --method POST "repos/$REPO/issues/$PR_NUMBER/labels" -f labels[]='status/on-hold' ``` @@ -318,7 +320,7 @@ alone cannot skip (the gate requires the label too), and a re-run of the triage upserts the same pin in place and retries the label; but an outcome posted without the label would promise a skip the gate does not grant. The review-lane gate requires the label AND a pin -matching the live head (a label without the pin runs the review lane), +matching the live head and base (a label without the pin runs the review lane), so the harm of a partial state is not a silent skip — it is the inverse: the outcome comment promises that no review verdict will post automatically while the unpinned label leaves the automatic lane diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 8af728ca104..793cd37babf 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2582,7 +2582,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { " */labels*) printf '%s\\n' $CONTEXT_LABELS ;;", ' */comments*)', ' if [ "${CONTEXT_MARKERS_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi', - ' node -e \'const rows = JSON.parse(process.env.CONTEXT_MARKERS || "[]"); for (const row of rows) if (["github-actions[bot]", "qwen-code-ci-bot"].includes(row.author)) console.log(row.body);\'', + ' node -e \'const rows = JSON.parse(process.env.CONTEXT_MARKERS || "[]"); for (const row of rows) if (["github-actions[bot]", "qwen-code-ci-bot"].includes(row.author) && row.body.startsWith("', + ], + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain('live head/base SHA was unreadable'); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it('a marker embedded in a larger bot comment does not skip (#9193)', () => { + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: 'abc123def456', + onHoldMarkers: [ + { + author: 'qwen-code-ci-bot', + body: '
\nqwen-triage on-hold sha=abc123def456 base=base777\n
', + }, + ], + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain('not pinned to the live head/base'); + rmSync(r.dir, { recursive: true, force: true }); + }); + it('a manually applied on-hold label without a marker never skips (#9193)', () => { const r = runContextStep({ eventName: 'pull_request_target', diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 37fec07a3c0..9b2207eac1c 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6060,10 +6060,23 @@ describe('triage skill non-functional routing (#7411)', () => { // The marker posts through the author-scoped upsert helper (never a bare // `gh pr comment`) so a re-run PATCHes the existing pin in place. expect(section).toContain( - "printf '%s' '' > /tmp/qwen-triage-on-hold-marker.md", + 'gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefOid', ); + expect(section).toContain('empty base SHA — fail closed'); expect(section).toContain( - '.github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" \'qwen-triage on-hold sha=\' /tmp/qwen-triage-on-hold-marker.md', + 'MARKER_FILE="$(mktemp "${RUNNER_TEMP:-/tmp}/qwen-triage-on-hold-marker.XXXXXX")"', + ); + expect(section).toContain( + "printf '%s' '' > \"$MARKER_FILE\"", + ); + expect(section).toContain( + '.github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" \'qwen-triage on-hold sha=\' "$MARKER_FILE"', + ); + expect(section).toContain('rm -f "$MARKER_FILE"'); + // Fail closed, in this order: the marker upsert must precede the label + // so a failed upsert never leaves a bare "parked" label behind. + expect(section.indexOf('upsert-bot-comment.sh')).toBeLessThan( + section.indexOf("labels[]='status/on-hold'"), ); expect(section).toContain( "skips the automatic lane only while the marker's SHA pair matches the live head and base", From cc37df5208b9425c9627c343b759e102e9f8f120 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 17 Aug 2026 02:37:27 +0800 Subject: [PATCH 15/18] test(ci): pin triage skip label retention --- scripts/tests/qwen-pr-review-workflow.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 6249e2161dd..0b20e7d5367 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2647,6 +2647,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(r.ghCalls).toContain('issues/4242/labels?per_page=100 --paginate'); expect(r.ghCalls).toContain('pulls/4242'); expect(r.ghCalls).toContain('issues/4242/comments?per_page=100 --paginate'); + expect(r.ghCalls).not.toContain('-X DELETE'); // Pin the filter EXPRESSION, not just its substring: the stub does its // own author filtering, so a mutated jq that merely contains // '.user.login' (e.g. select(.user.login != "")) would otherwise pass From 3354b1a5ffc682534d03f074d2f3be15a643d217 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 18:45:53 +0000 Subject: [PATCH 16/18] test(ci): make the stale-label-removal failure branch reachable (#9193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R2-6 follow-up: the gate's "Failed to remove stale status/on-hold" branch was unreachable in the harness — the stub's */labels* case answered the `-X DELETE` call first and always exited 0, so no test could ever fail the delete. Consequence: unwrapping the `if` around the DELETE survived the whole suite, yet under the step's `set -euo pipefail` a transient API error or rate limit would then abort the entire Resolve PR context step before `should_run=true` is written, hard-failing the review workflow. Add a deleteFails stub mode (a quoted `*"-X DELETE"*` case ordered before the labels answer, failing when CONTEXT_DELETE_FAIL=1) and a test asserting the failure summary plus that the gate still writes should_run=true. The existing negative pins already cover manual-park/matching-pin no-DELETE behaviour. --- scripts/tests/qwen-pr-review-workflow.test.js | 38 ++++++++++++++++++- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 0b20e7d5367..5167d50c8c5 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2543,6 +2543,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { onHoldMarkers = [], markerPadBytes = 0, editedBaseChanged = false, + deleteFails = false, }) { const doc = parse(workflow); const step = doc.jobs['review-pr'].steps.find( @@ -2553,8 +2554,9 @@ describe('qwen pr review triage-only skip (#7411)', () => { '${{ github.event_name }}': eventName, '${{ github.event.action }}': action, '${{ github.event.pull_request.number }}': '4242', - "${{ github.event.changes.base && 'true' || 'false' }}": - editedBaseChanged ? 'true' : 'false', + "${{ github.event.changes.base && 'true' || 'false' }}": editedBaseChanged + ? 'true' + : 'false', '${{ github.event.issue.number }}': '4242', '${{ github.event.inputs.pr_number }}': '4242', '${{ github.event.inputs.review_mode }}': 'dry-run', @@ -2583,6 +2585,12 @@ describe('qwen pr review triage-only skip (#7411)', () => { '#!/usr/bin/env bash', 'echo "$@" >> "$CONTEXT_GH_CALLS"', 'case "$*" in', + // Ordered BEFORE the labels answer: the stale-label strip is a + // `-X DELETE` on the same */labels* URL, and only this branch + // can fail it (exercising the gate's failure branch). The + // pattern is quoted: an unquoted space would split it into two + // case words and bash rejects the line outright. + ' *"-X DELETE"*) if [ "${CONTEXT_DELETE_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi ;;', " */labels*) printf '%s\\n' $CONTEXT_LABELS ;;", ' */comments*)', ' if [ "${CONTEXT_MARKERS_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi', @@ -2611,6 +2619,7 @@ describe('qwen pr review triage-only skip (#7411)', () => { CONTEXT_LABELS: labels.join(' '), CONTEXT_MARKERS: JSON.stringify(markerRows), CONTEXT_MARKERS_FAIL: markersFail ? '1' : '0', + CONTEXT_DELETE_FAIL: deleteFails ? '1' : '0', CONTEXT_MARKER_PAD: String(markerPadBytes), CONTEXT_HEAD_SHA: headSha, CONTEXT_BASE_SHA: baseSha, @@ -2700,6 +2709,31 @@ describe('qwen pr review triage-only skip (#7411)', () => { rmSync(r.dir, { recursive: true, force: true }); }); + it('a failed label removal never aborts the gate — it reports and leaves the label (#9193)', () => { + // The DELETE sits behind `[ -n "$MARKERS" ] &&` inside an `if` on + // purpose: under `set -euo pipefail` a bare DELETE would abort the + // whole Resolve PR context step on a transient API error or rate + // limit, before `should_run=true` is written — hard-failing the + // review workflow. Before this stub mode existed the failure branch + // was unreachable (the */labels* case answered the DELETE first and + // always exited 0), so unwrapping the `if` survived every test. + const r = runContextStep({ + eventName: 'pull_request_target', + labels: ['status/on-hold'], + headSha: 'newhead999', + deleteFails: true, + onHoldMarkers: [ + '', + ], + }); + expect(r.output).toContain('should_run=true'); + expect(r.summary).toContain( + 'Failed to remove stale status/on-hold — leaving it in place', + ); + expect(r.summary).not.toContain('Removed stale status/on-hold'); + rmSync(r.dir, { recursive: true, force: true }); + }); + it('a base retarget after triage invalidates the pin (#9193)', () => { const r = runContextStep({ eventName: 'pull_request_target', From 66645bc2fc1e80ea8b33b952a003de3cf7836bda Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 21:40:59 +0000 Subject: [PATCH 17/18] fix(ci): close the edited-trigger race and the base TOCTOU (#9193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-13 review, two Criticals plus three hardenings: - CRITICAL (#9091-class race): an edited run without a base change is a guaranteed no-op, yet joined the shared qwen-pr-review-pr-{N} concurrency group — a routine title/body edit could supersede a PENDING synchronize run (pending runs are replaced by the newest queued run, cancel-in-progress notwithstanding) and the pushed head would silently never review. Carve edited-without-changes.base out of the PR group into the per-run group, mirroring review_requested; base-retarget edits keep the shared group (superseding a stale pending run there reviews live state). Verbatim group pin updated. - CRITICAL (TOCTOU): Stage 1f read BASE_SHA fresh at outcome-post time while HEAD_SHA follows the capture-once/re-read-before-post discipline — a retarget landing mid-run minted a pin matching the NEW base pair, skipping a diff the triage never certified. Capture BASE_SHA once at review start alongside HEAD_SHA and re-read/compare before the outcome post, bailing on a moved base (exact mirror of the head guard). Triage-suite pins updated to the compare shape. - Reclaim the wasted preamble: the same edited clause now gates precheck-pr, authorize and review-pr at the job level (the in-step early exit stays as the tie-breaker). - Invalidate the stale marker comment when the stale-pin strip succeeds: markers are upserted but never deleted, so a later manually re-applied park label hit the stale-strip again instead of the manual-park carve-out. The strip now rewrites the marker via upsert-bot-comment.sh --update-only with a non-marker body. - Pin the '- edited' trigger subscription itself (the behavioural harness bypasses the on: block; deleting the line kept the suite green). Suites: triage 140/140, pr-review 174 green + 2 pre-existing root-user env failures unrelated to this change. eslint + prettier clean. --- .github/workflows/qwen-code-pr-review.yml | 33 +++++++++++++++- .qwen/skills/triage/references/pr-workflow.md | 15 +++++-- scripts/tests/qwen-pr-review-workflow.test.js | 39 ++++++++++++++++++- scripts/tests/qwen-triage-workflow.test.js | 9 ++++- 4 files changed, 89 insertions(+), 7 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 843931946b1..e1810e3b83f 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -63,11 +63,18 @@ concurrency: # anyone who can request the bot without write permission. The per-run group # costs only an occasional duplicate review when an authorized bot request # 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. + # never a lost review. The SAME carve-out applies to `edited` runs without a + # base change: they exit should_run=false at the step's early gate, so in + # the shared group a routine title/body edit could supersede a pending + # synchronize run and the pushed head would silently never review. Only + # base-retarget edits (changes.base) join the PR group — superseding a + # stale pending run there reviews live state: compute, never a lost + # review. Comment/review events use per-run groups to avoid cancelling + # active reviews. group: >- ${{ github.event_name == 'pull_request_target' && github.event.action != 'review_requested' && + (github.event.action != 'edited' || github.event.changes.base) && 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') }}" @@ -84,6 +91,7 @@ jobs: if: |- github.event_name == 'pull_request_target' && github.event.action != 'closed' && + (github.event.action != 'edited' || github.event.changes.base) && github.event.pull_request.head.repo.full_name != github.repository && (github.event.action != 'review_requested' || github.event.requested_reviewer.login == 'qwen-code-ci-bot') @@ -263,6 +271,9 @@ jobs: !cancelled() && (github.event_name != 'pull_request_target' || github.event.action != 'closed') && + (github.event_name != 'pull_request_target' || + github.event.action != 'edited' || + github.event.changes.base) && (github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.full_name == github.repository || needs.precheck-pr.outputs.decision == 'allow_triage') && @@ -378,6 +389,7 @@ jobs: (github.event_name == 'pull_request_target' && github.event.pull_request.state == 'open' && !github.event.pull_request.draft && + (github.event.action != 'edited' || github.event.changes.base) && needs.authorize.outputs.should_review == 'true' && ((github.event.action == 'review_requested' && github.event.requested_reviewer.login == needs.review-config.outputs.bot_login) || @@ -740,6 +752,23 @@ jobs: # review lane AND never age out via the stale bot. if [ -n "$MARKERS" ] && gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/labels/status%2Fon-hold" -X DELETE 2>/dev/null; then echo "Removed stale status/on-hold so the stale bot can age this PR (#9193)." >> "$GITHUB_STEP_SUMMARY" + # Invalidate the stale marker comment in the same pass: + # markers are upserted, never deleted, so leaving it in + # place keeps MARKERS non-empty forever and a maintainer + # who later re-applies status/on-hold to park the PR + # (documented workflow; the label is in stale.yml's + # exempt-pr-labels) would hit this stale-strip again + # instead of the manual-park carve-out above. Rewrite it + # with a body that no longer starts with the marker + # prefix; --update-only never mints a comment + # (#9193 review). A failed invalidation only warns: the + # worst case is one more strip of a re-applied label. + invalidated_marker_file="$(mktemp "${RUNNER_TEMP:-/tmp}/qwen-triage-marker-invalidated.XXXXXX")" + printf '%s\n' "" > "$invalidated_marker_file" + if ! .github/scripts/upsert-bot-comment.sh "${GITHUB_REPOSITORY}" "${PR_NUMBER}" 'qwen-triage on-hold sha=' "$invalidated_marker_file" --update-only; then + echo "::warning::could not invalidate the stale triage marker comment (#9193)." + fi + rm -f "$invalidated_marker_file" elif [ -z "$MARKERS" ]; then echo "status/on-hold present without a triage marker — leaving the manual label in place (#9193)." >> "$GITHUB_STEP_SUMMARY" else diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index 995ec9eb4b7..345354f213e 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -57,14 +57,18 @@ EXISTING=$(gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" --method GET --paginate if [ "$EXISTING" -eq 0 ]; then gh pr review ... ; fi ``` -**Signature & footer:** capture the reviewed commit's **full** OID **once, when you begin inspecting the code** — the SHA the worktree/diff actually reflects. Not a 7-char prefix (28 bits; a fork author can force-push a colliding prefix), and **not** a fresh read at post time (that would attest to code you never reviewed). Reuse this `HEAD_SHA` for every stage's footer, and before each post — and again before `--approve` — re-read the head and bail if it moved: +**Signature & footer:** capture the reviewed commit's **full** OID **once, when you begin inspecting the code** — the SHA the worktree/diff actually reflects. Not a 7-char prefix (28 bits; a fork author can force-push a colliding prefix), and **not** a fresh read at post time (that would attest to code you never reviewed). Capture the **base** alongside it under the same discipline: the classification reads the diff against the base it saw at start, so a retarget landing mid-run invalidates the verdict exactly like a push does (a pin posted with the NEW base would match the live pair and skip a diff the triage never certified). Reuse these SHAs for every stage's footer, and before each post — and again before `--approve` — re-read head and base and bail if either moved: ```bash HEAD_SHA=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefOid --jq '.headRefOid') || exit 1 [ -n "$HEAD_SHA" ] || { echo 'empty head SHA — fail closed'; exit 1; } # once, at review start +BASE_SHA=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefOid --jq '.baseRefOid') || exit 1 +[ -n "$BASE_SHA" ] || { echo 'empty base SHA — fail closed'; exit 1; } # once, alongside the head # before any post or approval — refuse to attest to code you didn't review: NOW=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefOid --jq '.headRefOid') || exit 1 [ -n "$NOW" ] && [ "$NOW" = "$HEAD_SHA" ] || { echo 'head moved or unreadable — restart or defer'; exit 1; } +NOW_BASE=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefOid --jq '.baseRefOid') || exit 1 +[ -n "$NOW_BASE" ] && [ "$NOW_BASE" = "$BASE_SHA" ] || { echo 'base moved or unreadable — restart or defer'; exit 1; } ``` Every staged comment (Stage 1 gate-pass, Stage 2, Stage 3) ends with the signature line, then a footer recording the commit this pass reflects. Because comments are updated in place on re-run, the SHA lets a maintainer tell at a glance whether new commits landed since the last review: @@ -303,8 +307,13 @@ classification evidence — read the diff. re-enables review automatically (#9193): ```bash -BASE_SHA=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefOid --jq '.baseRefOid') || exit 1 -[ -n "$BASE_SHA" ] || { echo 'empty base SHA — fail closed'; exit 1; } +# Re-read the base and compare against the SHA captured at review start — +# the exact mirror of the head guard above. A retarget landing between +# classification and this post must ABORT: a pin stamped with the new +# base would match the live head/base pair and skip a diff the triage +# never certified (#9193 review). +NOW_BASE=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefOid --jq '.baseRefOid') || exit 1 +[ -n "$NOW_BASE" ] && [ "$NOW_BASE" = "$BASE_SHA" ] || { echo 'base moved or unreadable since classification — restart triage'; exit 1; } MARKER_FILE="$(mktemp "${RUNNER_TEMP:-/tmp}/qwen-triage-on-hold-marker.XXXXXX")" printf '%s' '' > "$MARKER_FILE" .github/scripts/upsert-bot-comment.sh "$REPO" "$PR_NUMBER" 'qwen-triage on-hold sha=' "$MARKER_FILE" || { echo 'marker upsert failed — fail closed'; exit 1; } diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 5167d50c8c5..0ef55c30e7f 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2591,6 +2591,14 @@ describe('qwen pr review triage-only skip (#7411)', () => { // pattern is quoted: an unquoted space would split it into two // case words and bash rejects the line outright. ' *"-X DELETE"*) if [ "${CONTEXT_DELETE_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi ;;', + // Fast answers for the marker-invalidation upsert the strip + // branch now runs (upsert-bot-comment.sh): a bot login and an + // EMPTY listing, so --update-only no-ops instead of retrying + // its way through the stub's silent branches (3x sleep 10). + // `[]`, not `[[]]`: the upsert jq runs with -s (slurp), which + // wraps the document once itself. + ' "api user"*) echo "qwen-code-ci-bot" ;;', + ' *"--method GET"*) echo "[]" ;;', " */labels*) printf '%s\\n' $CONTEXT_LABELS ;;", ' */comments*)', ' if [ "${CONTEXT_MARKERS_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi', @@ -3019,15 +3027,44 @@ describe('qwen pr review concurrency routing', () => { // lifecycle actions reach the PR group and every review_requested (bot // included) falls through to the per-run group. Any requested_reviewer // clause here re-admits the unauthorized-requester no-op and silently - // re-ships the race. + // re-ships the race. The edited carve-out is the same protection for + // the other guaranteed no-op shape: a title/body edit exits + // should_run=false at the step's early gate, so in the shared group it + // could supersede a PENDING synchronize run and the pushed head would + // silently never review (#9193 review); only base-retarget edits join + // the PR group. expect(group).toBe( "${{ github.event_name == 'pull_request_target' && " + "github.event.action != 'review_requested' && " + + "(github.event.action != 'edited' || github.event.changes.base) && " + "format('qwen-pr-review-pr-{0}', github.event.pull_request.number) || " + "format('qwen-pr-review-run-{0}', github.run_id) }}", ); }); + it('subscribes the edited trigger so base retargets fire the gate (#9193)', () => { + // The behavioural harness injects action: 'edited' straight into the + // step bash, bypassing the on: block — deleting '- edited' from the + // subscription kept the whole suite green while production base + // retargets would never fire this workflow again. Sibling suites pin + // trigger types the same way. + expect(parse(workflow).on.pull_request_target.types).toContain('edited'); + }); + + it('keeps base-less edited runs out of the heavy jobs (#9193)', () => { + // Title/body edits are guaranteed no-ops; without these job-level + // clauses each one wakes precheck-pr (fork permission API + diff + // fetch), authorize, and review-pr's checkout preamble on the shared + // self-hosted pool. The in-step early exit stays as the tie-breaker. + const noBaselessEdited = + "github.event.action != 'edited' || github.event.changes.base"; + for (const job of ['precheck-pr', 'authorize', 'review-pr']) { + expect(parse(workflow).jobs[job].if.replace(/\s+/g, ' ')).toContain( + noBaselessEdited, + ); + } + }); + it('gates the review_requested jobs on the published bot login', () => { // The group expression no longer names the requested reviewer; the // job-level gates still must. Pin the surviving literal against the diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index d86d30c9034..092c6eddbf2 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6062,7 +6062,14 @@ describe('triage skill non-functional routing (#7411)', () => { expect(section).toContain( 'gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefOid', ); - expect(section).toContain('empty base SHA — fail closed'); + // The base is captured ONCE at review start (signature block) and only + // re-read here to compare — a fresh read at post time would let a + // mid-run retarget mint a pin matching the NEW base pair, skipping a + // diff the triage never certified (TOCTOU, #9193 review). + expect(section).toContain('NOW_BASE=$(gh pr view'); + expect(section).toContain( + '[ "$NOW_BASE" = "$BASE_SHA" ] || { echo \'base moved or unreadable since classification — restart triage\'; exit 1; }', + ); expect(section).toContain( 'MARKER_FILE="$(mktemp "${RUNNER_TEMP:-/tmp}/qwen-triage-on-hold-marker.XXXXXX")"', ); From 3b87dae8bb4203ff370395c9b6046129d4342659 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 23:30:36 +0000 Subject: [PATCH 18/18] fix(ci): authenticate the stale-marker invalidation as the marker author MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R13-1: the invalidation upsert ran under the step's GITHUB_TOKEN (github-actions[bot]), but upsert-bot-comment.sh scopes its lookup to the AUTHENTICATED login and the marker is authored by the triage agent under QWEN_CODE_BOT_TOKEN/CI_BOT_PAT (qwen-code-ci-bot) — so --update-only exited 0 with "no existing comment; nothing to update" and never PATCHed. The marker kept its prefix forever, the manual-park carve-out stayed unreachable on every Stage-1f-routed PR, and a later manually re-applied park label got stripped again on the next event. Bind the marker author's token inline on the invalidation call (mirroring the sibling upsert sites' CI_BOT_PAT binding) and extend the harness: the stub answers `gh api user` per token (only the bot token resolves to the marker author), lists a marker authored by that bot, and the stale-strip test now pins that the PATCH lands with the invalidated body. Mutation-verified: dropping the inline override removes the PATCH and fails the new pins. Suites green (2 pre-existing root-env failures unrelated); eslint + prettier clean. --- .github/workflows/qwen-code-pr-review.yml | 11 +++++- scripts/tests/qwen-pr-review-workflow.test.js | 35 +++++++++++++++---- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index e1810e3b83f..3150d7e7af8 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -763,9 +763,18 @@ jobs: # prefix; --update-only never mints a comment # (#9193 review). A failed invalidation only warns: the # worst case is one more strip of a re-applied label. + # + # GH_TOKEN must bind the MARKER AUTHOR's identity: the + # upsert lookup is author-scoped, the marker is posted by + # the triage agent under QWEN_CODE_BOT_TOKEN/CI_BOT_PAT, + # and this step's GITHUB_TOKEN resolves to + # github-actions[bot] — authenticating as anyone else + # makes --update-only a SILENT no-op (exit 0, no PATCH), + # leaving the marker live. Mirrors the sibling upsert + # sites' CI_BOT_PAT binding (docs-only relay/supersede). invalidated_marker_file="$(mktemp "${RUNNER_TEMP:-/tmp}/qwen-triage-marker-invalidated.XXXXXX")" printf '%s\n' "" > "$invalidated_marker_file" - if ! .github/scripts/upsert-bot-comment.sh "${GITHUB_REPOSITORY}" "${PR_NUMBER}" 'qwen-triage on-hold sha=' "$invalidated_marker_file" --update-only; then + if ! GH_TOKEN="${{ secrets.QWEN_CODE_BOT_TOKEN || secrets.CI_BOT_PAT }}" .github/scripts/upsert-bot-comment.sh "${GITHUB_REPOSITORY}" "${PR_NUMBER}" 'qwen-triage on-hold sha=' "$invalidated_marker_file" --update-only; then echo "::warning::could not invalidate the stale triage marker comment (#9193)." fi rm -f "$invalidated_marker_file" diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 0ef55c30e7f..447633f3cff 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2561,6 +2561,11 @@ describe('qwen pr review triage-only skip (#7411)', () => { '${{ github.event.inputs.pr_number }}': '4242', '${{ github.event.inputs.review_mode }}': 'dry-run', "${{ github.event.inputs.timeout_minutes || '180' }}": '180', + // The stale-marker invalidation binds the marker AUTHOR's token + // inline (the upsert lookup is author-scoped); give it a distinct + // stub token so the stub's `gh api user` can answer per identity. + '${{ secrets.QWEN_CODE_BOT_TOKEN || secrets.CI_BOT_PAT }}': + 'stub-triage-bot-pat', }; for (const [expr, value] of Object.entries(subs)) { run = run.split(expr).join(value); @@ -2592,13 +2597,17 @@ describe('qwen pr review triage-only skip (#7411)', () => { // case words and bash rejects the line outright. ' *"-X DELETE"*) if [ "${CONTEXT_DELETE_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi ;;', // Fast answers for the marker-invalidation upsert the strip - // branch now runs (upsert-bot-comment.sh): a bot login and an - // EMPTY listing, so --update-only no-ops instead of retrying - // its way through the stub's silent branches (3x sleep 10). - // `[]`, not `[[]]`: the upsert jq runs with -s (slurp), which - // wraps the document once itself. - ' "api user"*) echo "qwen-code-ci-bot" ;;', - ' *"--method GET"*) echo "[]" ;;', + // branch now runs (upsert-bot-comment.sh). The user answer is + // IDENTITY-SCOPED: only the marker author's token resolves to + // the marker author — dropping the workflow's inline GH_TOKEN + // override leaves the upsert authenticated as + // github-actions[bot], the author filter matches nothing, and + // no PATCH is issued (the R13-1 regression this pins). The + // listing carries one marker comment authored by that bot so + // the matching run reaches the PATCH. Slurp note: the upsert + // jq runs with -s, which wraps the document once itself. + ' "api user"*) if [ "$GH_TOKEN" = "stub-triage-bot-pat" ]; then echo "qwen-code-ci-bot"; else echo "github-actions[bot]"; fi ;;', + ' *"--method GET"*) echo "[{\\"id\\":123,\\"user\\":{\\"login\\":\\"qwen-code-ci-bot\\"},\\"body\\":\\"\\"}]" ;;', " */labels*) printf '%s\\n' $CONTEXT_LABELS ;;", ' */comments*)', ' if [ "${CONTEXT_MARKERS_FAIL:-0}" = "1" ]; then echo "API error" >&2; exit 1; fi', @@ -2714,6 +2723,18 @@ describe('qwen pr review triage-only skip (#7411)', () => { expect(r.summary).toContain('Removed stale status/on-hold'); expect(r.ghCalls).toContain('labels/status%2Fon-hold'); expect(r.ghCalls).toContain('-X DELETE'); + // The marker invalidation must actually PATCH: it runs under the + // marker AUTHOR's token (inline GH_TOKEN override — the upsert lookup + // is author-scoped; this step's own GITHUB_TOKEN is + // github-actions[bot], which would make --update-only a silent no-op + // and leave the marker live, R13-1). The stub answers `gh api user` + // per token and only lists the marker to the author identity, so + // dropping the override removes the PATCH and fails these pins. + expect(r.ghCalls).toContain('--method PATCH'); + expect(r.ghCalls).toContain('qwen-triage on-hold invalidated'); + expect(r.summary).not.toContain( + 'could not invalidate the stale triage marker', + ); rmSync(r.dir, { recursive: true, force: true }); });