From 3537b255af49f38e44e409376749d814c3ba3e4e Mon Sep 17 00:00:00 2001 From: verify Date: Fri, 7 Aug 2026 00:30:33 +0800 Subject: [PATCH 1/7] perf(ci): run docs-only automatic reviews at medium effort A 1-line docs PR costs the same 57-180 minute high-effort review as a code change, and on a diff with zero source lines the passes medium drops - the adversarial personas and the reverse audit - have no failure mode to hunt. Counterfactual analysis over six dissected CI runs showed the one case where those passes caught a real Critical was a source PR, which this gate never touches: classification reuses the Test workflow's conservative classify-profile.mjs (docs/**.md(x) + root prose only; markdown under any src/ tree stays full, matching the review skill's own source rule), and any fetch or classifier failure falls back to the full review. Only the automatic pull_request_target review downgrades; every explicit request (workflow_dispatch, @qwen-code /review) keeps full high effort. Because an effective --comment forces high and medium never posts, the downgrade drops --comment and a new step relays the review CLI's verbatim "Review complete:" line - its machine-readable completion contract - as a single PR comment, with a pointer for requesting the full review. The docs-only budget is the size-aware timeout halved with a 90-minute floor. --- .github/workflows/qwen-code-pr-review.yml | 93 ++++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index f6de121b033..022f1fe7644 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -443,6 +443,10 @@ jobs: # input or a /review --timeout=N comment). When false, "Run review" # replaces this default with a PR-size-aware tier instead. TIMEOUT_EXPLICIT=false + # True only for the automatic pull_request_target review — every + # other path is a human asking for a review explicitly, and an + # explicit ask always gets the full high-effort run. + AUTO_REVIEW=false TRIGGER_COMMAND="${TRIGGER_BODY%%$'\n'*}" if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then @@ -467,6 +471,9 @@ jobs: fi PR_NUMBER="${{ github.event.pull_request.number }}" REVIEW_MODE="comment" + if [ "${{ github.event_name }}" = "pull_request_target" ]; then + AUTO_REVIEW=true + fi else echo "Unsupported event: ${{ github.event_name }}" >&2 exit 1 @@ -495,6 +502,7 @@ jobs: echo "review_mode=$REVIEW_MODE" echo "timeout_minutes=$TIMEOUT_MINUTES" echo "timeout_explicit=$TIMEOUT_EXPLICIT" + echo "auto_review=$AUTO_REVIEW" } >> "$GITHUB_OUTPUT" - name: 'Setup Node.js for hosted review' @@ -684,6 +692,7 @@ jobs: OPENAI_MODEL: '${{ vars.QWEN_PR_REVIEW_MODEL }}' PR_NUMBER: '${{ steps.context.outputs.pr_number }}' REVIEW_MODE: '${{ steps.context.outputs.review_mode }}' + AUTO_REVIEW: '${{ steps.context.outputs.auto_review }}' TIMEOUT_MINUTES: '${{ steps.context.outputs.timeout_minutes }}' TIMEOUT_EXPLICIT: '${{ steps.context.outputs.timeout_explicit }}' # Evidence-image destination for `qwen review publish-assets` — @@ -950,6 +959,42 @@ jobs: fi fi + # Docs-only automatic reviews run at medium effort. The classifier is + # the same one the Test workflow's CI-profile gate already trusts + # (.github/scripts/ci/classify-profile.mjs) and it is conservative by + # construction: only docs/**.md(x) and root-level prose files classify + # docs_only — markdown under any src/ tree stays `full`, matching the + # review skill's own "markdown inside a source tree counts as source" + # rule — and any fetch or classifier failure falls back to the full + # review. On a diff with zero source lines, the passes medium drops + # (adversarial personas, reverse audit) have no failure mode to hunt; + # medium keeps the verified finder fan-out. `--comment` is dropped + # with the downgrade — an effective --comment forces high + # (parse-args), and medium never posts — so the "Report docs-only + # medium outcome" step relays the review's completion line instead. + # Explicit requests (dispatch, @qwen-code /review) never downgrade. + DOCS_ONLY_MEDIUM=false + if [ "${AUTO_REVIEW:-false}" = "true" ]; then + changed_files="${RUNNER_TEMP}/review-changed-files.jsonl" + if gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/files" \ + --jq '.[] | {filename, status, previous_filename}' > "$changed_files" 2>/dev/null; then + if profile="$(node .github/scripts/ci/classify-profile.mjs "$changed_files")" \ + && [ "$profile" = "docs_only" ]; then + DOCS_ONLY_MEDIUM=true + # Medium measures at one-third to one-half of high, so halve + # the size-aware budget with a 90-minute floor. + EFFECTIVE_TIMEOUT_MINUTES=$(( EFFECTIVE_TIMEOUT_MINUTES / 2 )) + if [ "$EFFECTIVE_TIMEOUT_MINUTES" -lt 90 ]; then + EFFECTIVE_TIMEOUT_MINUTES=90 + fi + echo "PR #${PR_NUMBER} is docs-only; automatic review runs at --effort medium (${EFFECTIVE_TIMEOUT_MINUTES}-minute budget)." + fi + else + echo "::warning::Could not list changed files for the docs-only gate; running the full review." + fi + fi + echo "docs_only_medium=$DOCS_ONLY_MEDIUM" >> "$GITHUB_OUTPUT" + if ! PR_DATA="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')"; then fail "Failed to determine state for PR #${PR_NUMBER}." fi @@ -976,7 +1021,9 @@ jobs: } >> "$GITHUB_OUTPUT" PROMPT="/review ${REVIEW_URL}" - if [ "$REVIEW_MODE" = "comment" ]; then + if [ "$DOCS_ONLY_MEDIUM" = "true" ]; then + PROMPT="${PROMPT} --effort medium" + elif [ "$REVIEW_MODE" = "comment" ]; then PROMPT="${PROMPT} --comment" fi @@ -1158,6 +1205,50 @@ jobs: fail "$REASON" 1 "$KIND" done + if [ "$DOCS_ONLY_MEDIUM" = "true" ]; then + # The review CLI's machine-readable completion contract — batch + # drivers detect completion by this exact line, and it is the one + # verdict statement the relay step may quote (asserting a verdict + # the run did not print is the failure the review skill measures). + COMPLETION_LINE="$(printf '%s\n' "$RESULT_TEXT" | grep -E '^Review complete: ' | tail -n1 || true)" + echo "completion_line=${COMPLETION_LINE:-Review complete: pr-${PR_NUMBER} — (completion line not found; see the run log)}" >> "$GITHUB_OUTPUT" + fi + + - name: 'Report docs-only medium outcome' + if: |- + steps.context.outputs.should_run == 'true' && + steps.review.outcome == 'success' && + steps.review.outputs.docs_only_medium == 'true' && + steps.context.outputs.pr_number != '' + env: + GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' + PR_NUMBER: '${{ steps.context.outputs.pr_number }}' + COMPLETION_LINE: "${{ steps.review.outputs.completion_line || 'Review complete: (see the run log)' }}" + RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' + run: |- + set -euo pipefail + # Medium never posts inline comments and --comment would force high + # effort back on (parse-args), so a docs-only automatic review + # reports through this single relay instead. The quoted line is the + # CLI's own completion contract, relayed verbatim — this step asserts + # nothing the run did not print. + BODY="$(printf '%s\n' \ + '' \ + '' \ + "📄 **Docs-only change** — the automatic review ran at \`--effort medium\` (verified findings, no reverse audit; medium posts no inline comments). Outcome:" \ + '' \ + "> ${COMPLETION_LINE}" \ + '' \ + "Full report in the [workflow run](${RUN_URL}). For a full high-effort review with inline comments, comment \`@qwen-code /review\`." \ + '' \ + '
中文说明' \ + '' \ + "📄 **纯文档变更** —— 自动评审以 \`--effort medium\` 运行（发现已验证、无反向审计；medium 不发行内评论），结果见上方引用行。完整报告见 [workflow 运行](${RUN_URL})；如需带行内评论的完整高档评审，请评论 \`@qwen-code /review\`。" \ + '' \ + '
')" + gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" -f body="$BODY" >/dev/null + echo "docs-only medium outcome relayed to PR #${PR_NUMBER}." + - name: 'Post fallback comment on failure' if: |- failure() && From f178af5ec7ac0bc7bb51e20750725efaaab8dfab Mon Sep 17 00:00:00 2001 From: verify Date: Fri, 7 Aug 2026 06:32:39 +0800 Subject: [PATCH 2/7] perf(ci): address review feedback on the docs-only medium gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All nine review suggestions, each verified before fixing: - review_requested is an explicit ask: the AUTO_REVIEW flag now excludes that action (authorize write-permission-checks its requester), so a maintainer requesting the bot's review gets the full high-effort run. - The fetch-and-classify wrapper is extracted to .github/scripts/ci/classify-pr-profile.sh and consumed by both ci.yml and the review gate, so the classifier's input contract lives in one place; distinct exit codes preserve each caller's fallback messages. - Neither completion-line fallback mints the reserved "Review complete: " prefix anymore, and the relayed line passes a strict not-posted disposition allowlist - on this never-posts path any posted-form disposition is false by definition (the measured phantom APPROVE posted), so it falls back to a neutral non-scrapable form. - The relay upserts by its marker (mirroring the queued-acknowledgement step) instead of stacking a comment per push, retries the POST/PATCH three times, and never fails the job - a failed relay after a successful review must not trip the failure fallback into announcing a review failure that never happened. - The Chinese relay copy no longer parses as "发行" and renders high-effort as 高强度 rather than 高档. - The qwen-review docs-only-medium marker is registered in all six BOT_COMMENT_FILTER sites in qwen-autofix.yml, so clean docs-only relays cannot select PRs into autofix rounds as actionable feedback. - The gate's behavioral invariants are pinned in scripts/tests/qwen-pr-review-workflow.test.js by executing the extracted bash: prompt-branch order (--effort medium instead of --comment), the halve-with-90-minute-floor arithmetic, the completion-line allowlist including the phantom shapes, AUTO_REVIEW exclusivity, the six-site marker registration, and the shared-wrapper routing in both workflows. --- .github/scripts/ci/classify-pr-profile.sh | 28 +++ .github/workflows/ci.yml | 20 +- .github/workflows/qwen-autofix.yml | 12 +- .github/workflows/qwen-code-pr-review.yml | 100 +++++++--- scripts/tests/qwen-pr-review-workflow.test.js | 172 ++++++++++++++++++ 5 files changed, 294 insertions(+), 38 deletions(-) create mode 100755 .github/scripts/ci/classify-pr-profile.sh diff --git a/.github/scripts/ci/classify-pr-profile.sh b/.github/scripts/ci/classify-pr-profile.sh new file mode 100755 index 00000000000..d872d96ae2d --- /dev/null +++ b/.github/scripts/ci/classify-pr-profile.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# Fetch a PR's changed files and classify its CI profile in one step. +# +# The jq projection below is the input contract of classify-profile.mjs +# (it reads `filename`, `status`, `previous_filename` per JSONL entry). +# Both ci.yml's profile gate and qwen-code-pr-review.yml's docs-only +# downgrade consume the classification through THIS script, so the contract +# lives in exactly one place — a divergence between the two call sites once +# meant the same PR could classify differently in each workflow, silently, +# because both fall back to `full` on their own errors. +# +# Usage: classify-pr-profile.sh +# Prints the profile (docs_only | github_ci_only | full) on stdout. +# Exit codes: 0 classified; 2 file listing failed; 3 classifier failed. +set -euo pipefail + +repo="${1:?usage: classify-pr-profile.sh }" +pr="${2:?usage: classify-pr-profile.sh }" + +tmp="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" +files="${tmp}/classify-pr-${pr}-files.jsonl" + +if ! gh api --paginate "repos/${repo}/pulls/${pr}/files" \ + --jq '.[] | {filename, status, previous_filename}' > "${files}"; then + exit 2 +fi + +node "$(dirname "$0")/classify-profile.mjs" "${files}" || exit 3 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1d64329d472..b69d588e5fb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -267,14 +267,20 @@ jobs: profile=full if [[ "${GITHUB_EVENT_NAME}" == "pull_request" && -n "${PR_NUMBER}" ]]; then if [[ "${IS_SAME_REPO_PR}" == "true" ]]; then - changed_files="${RUNNER_TEMP}/changed-files.jsonl" - if gh api --paginate "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/files" --jq '.[] | {filename, status, previous_filename}' > "${changed_files}"; then - if ! profile="$(node .github/scripts/ci/classify-profile.mjs "${changed_files}")"; then - echo "::error::CI profile classifier exited non-zero; running full CI." - profile=full - fi - else + # Fetch + classify through the shared wrapper (also used by the + # review workflow's docs-only gate) so the classifier's input + # contract lives in one place. Exit 2 = listing failed, + # 3 = classifier failed. + set +e + profile="$(.github/scripts/ci/classify-pr-profile.sh "${GITHUB_REPOSITORY}" "${PR_NUMBER}")" + classify_rc=$? + set -e + if [ "$classify_rc" -eq 2 ]; then echo "::warning::Unable to list PR changed files; running full CI." + profile=full + elif [ "$classify_rc" -ne 0 ]; then + echo "::error::CI profile classifier exited non-zero; running full CI." + profile=full fi else echo "Fork PR detected; running full CI." diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index ffa84855409..4d69a85335c 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -2701,7 +2701,7 @@ jobs: # bot's own eval markers, and known non-actionable bot comments # (triage stages, coverage reports, legacy suggestion summaries, # force-push reminders). - BOT_COMMENT_FILTER='' \ '' \ @@ -1243,11 +1262,42 @@ jobs: '' \ '
中文说明' \ '' \ - "📄 **纯文档变更** —— 自动评审以 \`--effort medium\` 运行（发现已验证、无反向审计；medium 不发行内评论），结果见上方引用行。完整报告见 [workflow 运行](${RUN_URL})；如需带行内评论的完整高档评审，请评论 \`@qwen-code /review\`。" \ + "📄 **纯文档变更** —— 自动评审以 \`--effort medium\` 运行（发现已验证、无反向审计；medium 不发布行内评论），结果见上方引用行。完整报告见 [workflow 运行](${RUN_URL})；如需带行内评论的完整高强度（high-effort）评审，请评论 \`@qwen-code /review\`。" \ '' \ '
')" - gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" -f body="$BODY" >/dev/null - echo "docs-only medium outcome relayed to PR #${PR_NUMBER}." + # Upsert by marker, mirroring the queued-acknowledgement step: each + # push of a docs-only PR re-runs the automatic review, and without + # the PATCH branch every run would stack another near-identical + # relay comment on the PR page. + EXISTING_ID="$( + gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --method GET \ + --paginate \ + -F per_page=100 \ + | jq -sr '[.[][] | select(.body | contains(""))] | last | .id // empty' + )" || EXISTING_ID="" + # Bounded retry, and never fail the job over the relay: the review + # itself succeeded, and a failing step here would trip the + # post-failure fallback into announcing a review failure that never + # happened. Losing the relay costs a comment; the outcome line below + # keeps it recoverable from the job log. + posted=false + for attempt in 1 2 3; do + if [ -n "$EXISTING_ID" ]; then + gh api --method PATCH \ + "repos/${GITHUB_REPOSITORY}/issues/comments/${EXISTING_ID}" \ + -f body="$BODY" >/dev/null && posted=true && break + else + gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + -f body="$BODY" >/dev/null && posted=true && break + fi + sleep 10 + done + if [ "$posted" = "true" ]; then + echo "docs-only medium outcome relayed to PR #${PR_NUMBER}." + else + echo "::warning::Docs-only relay comment could not be posted after 3 attempts; the review itself succeeded. Outcome: ${COMPLETION_LINE}" + fi - name: 'Post fallback comment on failure' if: |- diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 18c608b1f98..ce0c356b3c5 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1232,3 +1232,175 @@ describe('capture-tools step wiring', () => { ).toBe('${{ vars.QWEN_REVIEW_ASSETS_REPO }}'); }); }); + +describe('docs-only medium gate', () => { + // The downgrade logic is inline bash in two steps; these tests extract and + // EXECUTE the load-bearing fragments (prompt branch, timeout floor, the + // completion-line allowlist) rather than asserting on their text, because + // the surviving mutations are behavioral: swapping the if/elif order makes + // parse-args force high effort back on AND post inline comments while the + // relay still claims medium posted nothing; flipping the floor comparison + // caps every size-tiered docs run at 90 minutes. + const run = (() => { + const doc = parse(workflow); + return doc.jobs['review-pr'].steps.find((s) => s.name === 'Run review').run; + })(); + + function promptBranchSource() { + const start = run.indexOf('PROMPT="/review ${REVIEW_URL}"'); + expect(start).toBeGreaterThan(-1); + const end = run.indexOf('\nfi', start) + '\nfi'.length; + return run.slice(start, end); + } + + function buildPrompt({ docsOnlyMedium, reviewMode }) { + const script = [ + 'set -euo pipefail', + 'REVIEW_URL="https://x/pull/1"', + `DOCS_ONLY_MEDIUM=${docsOnlyMedium}`, + `REVIEW_MODE=${reviewMode}`, + promptBranchSource(), + 'printf "%s" "$PROMPT"', + ].join('\n'); + return execFileSync('bash', ['-c', script], { encoding: 'utf8' }); + } + + it('emits --effort medium INSTEAD OF --comment on the docs-only path', () => { + const prompt = buildPrompt({ + docsOnlyMedium: 'true', + reviewMode: 'comment', + }); + expect(prompt).toContain('--effort medium'); + expect(prompt).not.toContain('--comment'); + }); + + it('keeps --comment on the non-docs comment path', () => { + const prompt = buildPrompt({ + docsOnlyMedium: 'false', + reviewMode: 'comment', + }); + expect(prompt).toContain('--comment'); + expect(prompt).not.toContain('--effort'); + }); + + function floorSource() { + const anchor = run.indexOf('# Medium measures at one-third to one-half'); + expect(anchor).toBeGreaterThan(-1); + const start = run.indexOf('EFFECTIVE_TIMEOUT_MINUTES=$((', anchor); + // The YAML parser strips the block scalar's base indentation, so the + // floor's closing `fi` sits at four spaces in the parsed text. + const end = run.indexOf('\n fi', start) + '\n fi'.length; + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + return run.slice(start, end); + } + + it.each([ + [360, 180], + [180, 90], + [100, 90], + ])( + 'halves the size-aware budget with a 90-minute floor (%i → %i)', + (input, want) => { + const script = [ + 'set -euo pipefail', + `EFFECTIVE_TIMEOUT_MINUTES=${input}`, + floorSource(), + 'printf "%s" "$EFFECTIVE_TIMEOUT_MINUTES"', + ].join('\n'); + expect(execFileSync('bash', ['-c', script], { encoding: 'utf8' })).toBe( + String(want), + ); + }, + ); + + function completionBlockSource() { + const anchor = run.indexOf('machine-readable completion contract'); + expect(anchor).toBeGreaterThan(-1); + const start = run.lastIndexOf( + 'if [ "$DOCS_ONLY_MEDIUM" = "true" ]; then', + anchor, + ); + // Base indentation is stripped by the YAML parser: the block's outer `fi` + // sits at column 0, its inner allowlist `fi` at two spaces — so `\nfi` + // uniquely anchors the outer close. + const end = run.indexOf('\nfi', anchor) + '\nfi'.length; + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + return run.slice(start, end); + } + + function relayLine(resultText) { + const dir = mkdtempSync(join(tmpdir(), 'review-completion-')); + try { + const gho = join(dir, 'gho'); + writeFileSync(gho, ''); + const script = [ + 'set -euo pipefail', + 'DOCS_ONLY_MEDIUM=true', + 'PR_NUMBER=123', + `GITHUB_OUTPUT="${gho}"`, + `RESULT_TEXT=$(cat "${join(dir, 'result')}")`, + completionBlockSource(), + ].join('\n'); + writeFileSync(join(dir, 'result'), resultText); + execFileSync('bash', ['-c', script], { encoding: 'utf8' }); + return readFileSync(gho, 'utf8').trim(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + + it('relays only the not-posted disposition shape', () => { + expect( + relayLine( + 'prose...\nReview complete: pr-123 — Comment, not posted (0 Critical, 2 Suggestion)', + ), + ).toBe( + 'completion_line=Review complete: pr-123 — Comment, not posted (0 Critical, 2 Suggestion)', + ); + }); + + it.each([ + // The measured phantom: a posted-form disposition on a path that never posts. + 'Review complete: pr-123 — APPROVE posted', + 'Review complete: pr-123 — COMMENT posted (0 Critical, 1 Suggestion inline)', + // Reworded/missing completion lines. + 'The review finished fine, trust me.', + '', + ])('falls back to the neutral non-scrapable form for %j', (text) => { + const line = relayLine(text); + expect(line.startsWith('completion_line=(no relayable')).toBe(true); + // The fallback must never mint the reserved machine prefix. + expect(line).not.toContain('completion_line=Review complete:'); + }); + + it('classifies review_requested as an explicit ask, never automatic', () => { + const doc = parse(workflow); + const context = doc.jobs['review-pr'].steps.find((s) => s.id === 'context'); + // One assignment site, guarded on both the event and the action. + expect(context.run.match(/AUTO_REVIEW=true/g)).toHaveLength(1); + expect(context.run).toMatch( + /!= "review_requested" \]; then\s*\n\s*AUTO_REVIEW=true/, + ); + }); + + it('registers the relay marker in every autofix bot-comment filter site', () => { + const autofix = readFileSync('.github/workflows/qwen-autofix.yml', 'utf8'); + // Definition + five inline jq copies; the trailing space is part of the + // pattern (marker text is always followed by a space before `-->`). + const hits = autofix.match(/\|qwen-review docs-only-medium\) /g) ?? []; + expect(hits.length).toBeGreaterThanOrEqual(6); + }); + + it('routes classification through the shared classify-pr-profile wrapper', () => { + // Both this gate and ci.yml must consume the classifier via the shared + // script so its input contract lives in one place. + expect(run).toContain('.github/scripts/ci/classify-pr-profile.sh'); + const ci = readFileSync('.github/workflows/ci.yml', 'utf8'); + expect(ci).toContain('.github/scripts/ci/classify-pr-profile.sh'); + expect(ci).not.toContain( + "--jq '.[] | {filename, status, previous_filename}'", + ); + }); +}); From 119845d38ce47fd4740e678f97c8fe9651d892b7 Mon Sep 17 00:00:00 2001 From: verify Date: Fri, 7 Aug 2026 09:06:29 +0800 Subject: [PATCH 3/7] perf(ci): harden the docs-only gate against round-2 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thirteen findings across two review passes; every fix is executed by a test rather than asserted as text where the finding was behavioral. - The relay marker exclusion in qwen-autofix.yml is author-scoped at all six filter sites: only the relay bot's own marker comment is filtered, so a human quoting the marker stays actionable feedback. - classify-pr-profile.sh guards the 3,000-file listing cap (any mismatch against the PR's declared changed_files classifies full), uses mktemp+trap instead of a fixed path on the shared persistent pool, and ships its own node:test suite (renamed source→docs pins the projection contract; exit codes 2/3 pinned) registered in HELPER_TESTS. - classify-profile.mjs restricts reserved root prose basenames to inert extensions - README.js / SECURITY.ts / LICENSE.sh classify full. - The completion-line allowlist binds to pr- and to the only verdict a medium run can produce (Comment, not posted) - a stale line for another PR or an Approve-shaped injection falls back to neutral. - A dedicated review_completed output gates the relay: the state/head guards exit 0 without running the review, and outcome==success alone would have announced a review that never ran. - The relay upsert filters by the authenticated bot login, re-resolves the comment id on every attempt, and falls back to POST when the PATCH target is gone - a participant posting the marker can no longer capture the upsert, a transient listing failure no longer mints duplicates. - The gate and relay are now executed under stubbed executables in qwen-pr-review-workflow.test.js (docs_only/full/failure/explicit scenarios; POST/PATCH/never-fail branches), the AUTO_REVIEW pin covers both guard halves, and the marker contract is pinned producer-side and filter-side. --- .github/scripts/ci/classify-pr-profile.sh | 18 +- .../scripts/ci/classify-pr-profile.test.mjs | 109 ++++++++++ .github/scripts/ci/classify-profile.mjs | 6 +- .github/scripts/ci/classify-profile.test.mjs | 11 + .github/workflows/ci.yml | 2 +- .github/workflows/qwen-autofix.yml | 18 +- .github/workflows/qwen-code-pr-review.yml | 60 ++++-- scripts/tests/qwen-pr-review-workflow.test.js | 191 +++++++++++++++++- 8 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 .github/scripts/ci/classify-pr-profile.test.mjs diff --git a/.github/scripts/ci/classify-pr-profile.sh b/.github/scripts/ci/classify-pr-profile.sh index d872d96ae2d..4b188476fb5 100755 --- a/.github/scripts/ci/classify-pr-profile.sh +++ b/.github/scripts/ci/classify-pr-profile.sh @@ -17,12 +17,28 @@ set -euo pipefail repo="${1:?usage: classify-pr-profile.sh }" pr="${2:?usage: classify-pr-profile.sh }" +# mktemp + trap, not a fixed name: the self-hosted pool is persistent and +# shared, so a predictable path is a leftover-file landmine, and ci.yml's +# gate and the review gate can run concurrently for the same PR — two +# writers interleaving one JSONL would classify one job against the other +# job's file list. tmp="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" -files="${tmp}/classify-pr-${pr}-files.jsonl" +files="$(mktemp "${tmp}/classify-pr-${pr}-files.XXXXXX")" +trap 'rm -f "$files"' EXIT if ! gh api --paginate "repos/${repo}/pulls/${pr}/files" \ --jq '.[] | {filename, status, previous_filename}' > "${files}"; then exit 2 fi +# The list-files endpoint caps at 3,000 entries. A truncated listing can be +# all docs while an omitted later entry is source, so any mismatch against +# the PR's own changed-file count conservatively classifies as `full`. +declared="$(gh api "repos/${repo}/pulls/${pr}" --jq '.changed_files')" || exit 2 +retrieved="$(wc -l < "${files}")" +if [ "${retrieved}" -ne "${declared}" ]; then + echo "full" + exit 0 +fi + node "$(dirname "$0")/classify-profile.mjs" "${files}" || exit 3 diff --git a/.github/scripts/ci/classify-pr-profile.test.mjs b/.github/scripts/ci/classify-pr-profile.test.mjs new file mode 100644 index 00000000000..70cb8301867 --- /dev/null +++ b/.github/scripts/ci/classify-pr-profile.test.mjs @@ -0,0 +1,109 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Executes the real classify-pr-profile.sh with a stubbed `gh` (and, for the +// classifier-failure case, a stubbed `node`) on PATH. The wrapper's own +// comment declares its jq projection the single home of the classifier's +// input contract — these tests are what make that claim enforceable: dropping +// `status` from the projection turns a renamed source→docs file into a plain +// docs path (classifyFileEntry consults `previous_filename` only when +// `status === "renamed"`), which downgraded a source PR in the probe that +// motivated this file. The exit-code contract (2 listing / 3 classifier) is +// consumed by both ci.yml and the review workflow's docs-only gate. + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const here = dirname(fileURLToPath(import.meta.url)); +const wrapper = join(here, 'classify-pr-profile.sh'); + +function run(scenario, { stubNodeFailure = false } = {}) { + const dir = mkdtempSync(join(tmpdir(), 'classify-pr-profile-')); + const bin = join(dir, 'bin'); + mkdirSync(bin); + const write = (name, body) => { + const p = join(bin, name); + writeFileSync(p, body); + chmodSync(p, 0o755); + }; + // The gh stub serves the two calls the wrapper makes: the paginated file + // listing (JSONL via --jq) and the PR object's changed_files count. + write( + 'gh', + [ + '#!/bin/bash', + 'case "$*" in', + ' *"/files"*)', + ' case "$SCENARIO" in', + ' list-fail) exit 1 ;;', + ' docs-only) printf \'%s\\n\' \'{"filename":"docs/users/a.md","status":"modified","previous_filename":null}\' \'{"filename":"README.md","status":"modified","previous_filename":null}\' ;;', + ' renamed-source) printf \'%s\\n\' \'{"filename":"docs/new.md","status":"renamed","previous_filename":"packages/core/src/runtime.ts"}\' ;;', + ' truncated) printf \'%s\\n\' \'{"filename":"docs/users/a.md","status":"modified","previous_filename":null}\' ;;', + ' *) exit 9 ;;', + ' esac ;;', + ' *"repos/"*)', + ' case "$SCENARIO" in', + ' truncated) echo 5 ;;', + ' docs-only) echo 2 ;;', + ' renamed-source) echo 1 ;;', + ' *) exit 9 ;;', + ' esac ;;', + ' *) exit 9 ;;', + 'esac', + 'exit 0', + ].join('\n') + '\n', + ); + if (stubNodeFailure) { + write('node', '#!/bin/bash\nexit 1\n'); + } + try { + const stdout = execFileSync('bash', [wrapper, 'o/r', '42'], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + SCENARIO: scenario, + RUNNER_TEMP: dir, + }, + }); + return { code: 0, stdout: stdout.trim() }; + } catch (e) { + return { code: e.status, stdout: `${e.stdout ?? ''}`.trim() }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +test('classifies a docs-only listing as docs_only', () => { + assert.deepEqual(run('docs-only'), { code: 0, stdout: 'docs_only' }); +}); + +test('a renamed source→docs file classifies full (the projection carries status/previous_filename)', () => { + assert.deepEqual(run('renamed-source'), { code: 0, stdout: 'full' }); +}); + +test('a listing shorter than the PR-declared changed_files classifies full (3,000-file cap)', () => { + assert.deepEqual(run('truncated'), { code: 0, stdout: 'full' }); +}); + +test('exit 2 when the file listing fails', () => { + assert.equal(run('list-fail').code, 2); +}); + +test('exit 3 when the classifier fails', () => { + assert.equal(run('docs-only', { stubNodeFailure: true }).code, 3); +}); diff --git a/.github/scripts/ci/classify-profile.mjs b/.github/scripts/ci/classify-profile.mjs index 4188e9d10cc..bdccc000f1b 100644 --- a/.github/scripts/ci/classify-profile.mjs +++ b/.github/scripts/ci/classify-profile.mjs @@ -17,7 +17,11 @@ function isDocsOnlyFile(file) { const normalized = file.replace(/\\/g, '/'); return ( /^docs\/.+\.(?:md|mdx)$/i.test(normalized) || - /^(?:README|CHANGELOG|CONTRIBUTING|CODE_OF_CONDUCT|SECURITY|SUPPORT|LICENSE|NOTICE)(?:\.[^/]*)?$/i.test( + // Extensionless or known-inert documentation extensions ONLY: the open + // `(?:\.[^/]*)?` form classified executable files named after reserved + // prose basenames (README.js, SECURITY.ts, LICENSE.sh) as docs, which + // would downgrade an automatic review over runnable code. + /^(?:README|CHANGELOG|CONTRIBUTING|CODE_OF_CONDUCT|SECURITY|SUPPORT|LICENSE|NOTICE)(?:\.(?:md|mdx|txt|rst))?$/i.test( normalized, ) ); diff --git a/.github/scripts/ci/classify-profile.test.mjs b/.github/scripts/ci/classify-profile.test.mjs index 99e6721d9d4..c2626f99548 100644 --- a/.github/scripts/ci/classify-profile.test.mjs +++ b/.github/scripts/ci/classify-profile.test.mjs @@ -114,3 +114,14 @@ test('falls back to full for runtime markdown assets and instruction files', () ); assert.equal(classifyChangedFiles(['AGENTS.md']), 'full'); }); + +test('reserved prose basenames classify docs_only only with inert extensions', () => { + assert.equal(classifyChangedFiles(['README.md']), 'docs_only'); + assert.equal(classifyChangedFiles(['LICENSE']), 'docs_only'); + assert.equal(classifyChangedFiles(['SECURITY.txt']), 'docs_only'); + // Executable files named after reserved basenames must never downgrade a + // review: the open-extension form classified all of these as docs. + assert.equal(classifyChangedFiles(['README.js']), 'full'); + assert.equal(classifyChangedFiles(['SECURITY.ts']), 'full'); + assert.equal(classifyChangedFiles(['LICENSE.sh']), 'full'); +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b69d588e5fb..567eb3e0f77 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ env: # BOTH the github_ci_only helper step and the full-profile Test step, so a # new helper test can't be added to one path and silently dropped from the # other. - HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs' + HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs' jobs: classify_pr: diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 4d69a85335c..0b14a9ac957 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -2701,7 +2701,7 @@ jobs: # bot's own eval markers, and known non-actionable bot comments # (triage stages, coverage reports, legacy suggestion summaries, # force-push reminders). - BOT_COMMENT_FILTER='"))] | last | .id // empty' - )" || EXISTING_ID="" + # Upsert by marker AND author, mirroring the queued-acknowledgement + # step: each push of a docs-only PR re-runs the automatic review, and + # without the PATCH branch every run would stack another + # near-identical relay comment on the PR page. The author filter is + # load-bearing — a PR participant can post the marker themselves, and + # an author-blind `last` match would then PATCH user-owned content + # (or fail on it forever, suppressing the authoritative relay). + BOT_LOGIN="$(gh api user --jq '.login')" || BOT_LOGIN="" # Bounded retry, and never fail the job over the relay: the review # itself succeeded, and a failing step here would trip the # post-failure fallback into announcing a review failure that never # happened. Losing the relay costs a comment; the outcome line below - # keeps it recoverable from the job log. + # keeps it recoverable from the job log. The lookup is re-resolved + # on every attempt — a transient failure on the listing GET must not + # settle the loop into the POST branch (a permanent duplicate), and + # a comment deleted between attempts must fall back to POST rather + # than PATCHing a 404 three times. posted=false for attempt in 1 2 3; do + EXISTING_ID="$( + gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --method GET \ + --paginate \ + -F per_page=100 \ + | jq -sr --arg bot "$BOT_LOGIN" '[.[][] + | select((.user.login // "") == $bot) + | select(.body | contains(""))] + | last | .id // empty' + )" || EXISTING_ID="" if [ -n "$EXISTING_ID" ]; then - gh api --method PATCH \ + if gh api --method PATCH \ "repos/${GITHUB_REPOSITORY}/issues/comments/${EXISTING_ID}" \ - -f body="$BODY" >/dev/null && posted=true && break - else - gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ - -f body="$BODY" >/dev/null && posted=true && break + -f body="$BODY" >/dev/null; then + posted=true + break + fi + elif gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + -f body="$BODY" >/dev/null; then + posted=true + break fi sleep 10 done diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index ce0c356b3c5..7c45c141592 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1380,17 +1380,35 @@ describe('docs-only medium gate', () => { const context = doc.jobs['review-pr'].steps.find((s) => s.id === 'context'); // One assignment site, guarded on both the event and the action. expect(context.run.match(/AUTO_REVIEW=true/g)).toHaveLength(1); + // Both halves of the guard: the event must be pull_request_target AND the + // action must not be review_requested. Pinning only the action half let a + // deleted event condition survive — the branch is shared with + // pull_request_review(_comment), whose actions are never review_requested, + // so a review-body `@qwen-code /review` would have downgraded silently. expect(context.run).toMatch( - /!= "review_requested" \]; then\s*\n\s*AUTO_REVIEW=true/, + /= "pull_request_target" \] &&\s*\n\s*\[ "\$\{\{ github\.event\.action \}\}" != "review_requested" \]; then\s*\n\s*AUTO_REVIEW=true/, ); }); - it('registers the relay marker in every autofix bot-comment filter site', () => { + it('pins the relay marker producer↔filter contract, author-scoped', () => { + // Producer side: the marker literal as the relay step actually posts it. + const doc = parse(workflow); + const relay = doc.jobs['review-pr'].steps.find( + (s) => s.name === 'Report docs-only medium outcome', + ); + const m = relay.run.match(//); + expect(m).not.toBeNull(); + // Filter side: every autofix exclusion of that marker must carry the + // author scope ($rb) — a human quoting the marker stays actionable — + // and all six sites (definition + five inline copies) must be present. const autofix = readFileSync('.github/workflows/qwen-autofix.yml', 'utf8'); - // Definition + five inline jq copies; the trailing space is part of the - // pattern (marker text is always followed by a space before `-->`). - const hits = autofix.match(/\|qwen-review docs-only-medium\) /g) ?? []; - expect(hits.length).toBeGreaterThanOrEqual(6); + const scoped = + autofix.match( + /\(\.user\.login \/\/ ""\) == \$rb\)\) and \(\(\.body \/\/ ""\) \| test\(" old"}]\'', + ' else echo "[]"; fi ;;', + ' *) : ;;', + 'esac', + 'exit 0', + ].join('\n') + '\n', + ); + const script = [ + 'set -euo pipefail', + 'GITHUB_REPOSITORY=o/r', + 'PR_NUMBER=42', + 'COMPLETION_LINE="Review complete: pr-42 — Comment, not posted (0 Critical, 1 Suggestion)"', + 'RUN_URL=https://x', + relayRun, + ].join('\n'); + const stdout = execFileSync('bash', ['-c', script], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + SCENARIO: scenario, + CALLS: calls, + }, + }); + return { stdout, calls: readFileSync(calls, 'utf8') }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + + it('POSTs a fresh relay comment when none exists', () => { + const r = runRelay({ scenario: 'fresh' }); + expect(r.stdout).toContain('relayed to PR #42'); + expect(r.calls).toContain('api repos/o/r/issues/42/comments -f'); + expect(r.calls).not.toContain('--method PATCH'); + }); + + it('PATCHes the existing bot-authored relay comment', () => { + const r = runRelay({ scenario: 'existing' }); + expect(r.stdout).toContain('relayed to PR #42'); + expect(r.calls).toContain( + 'api --method PATCH repos/o/r/issues/comments/777', + ); + }); + + it('warns and exits 0 when every attempt fails', () => { + const r = runRelay({ scenario: 'all-fail' }); + expect(r.stdout).toContain('::warning::'); + expect(r.stdout).toContain('the review itself succeeded'); + }); +}); From f87dec50afb3f508a0e3f29f5bb2d578bf170f0d Mon Sep 17 00:00:00 2001 From: verify Date: Fri, 7 Aug 2026 12:10:10 +0800 Subject: [PATCH 4/7] perf(ci): fix the medium Request-changes swallow and the stale docs badge Round-3 review findings (2 Critical, 8 test-gap Suggestions), each fix executed by a test where the finding was behavioral: - The completion-line allowlist accepts `Request changes, not posted` - compose-review caps only Approve at medium, so a docs-only run that verifies a Critical legitimately emits Request changes, and the old Comment-only allowlist swallowed exactly the blocker-finding outcome into the neutral fallback. Target binding to pr- is unchanged and now pinned by a test, as is the last-line selection over a stale or injected earlier completion line. - A stale docs-only badge can no longer outlive its revision: the full automatic review path now supersedes the bot-authored marker comment (strikethrough + superseded note) via a new --update-only mode that never mints a badge where none existed. - The marker+author upsert protocol is extracted to .github/scripts/upsert-bot-comment.sh - one implementation shared by the relay and the supersede step (the per-step copies had already drifted), with its own node:test suite covering the author scope, the per-attempt re-resolution (deleted-mid-retry falls back to POST), and the --update-only no-op; registered in HELPER_TESTS. - The classify-pr-profile gh stub now applies the wrapper's own --jq argument with real jq over API-shaped fixtures, so the projection contract is genuinely under test (negative control: dropping `status` turns the renamed-source scenario red). - New pins: review_completed wiring end to end (run-step emit + both consumers' if clauses), the auto_review output->env wiring at both links, and both AUTO_REVIEW guard halves. --- .../scripts/ci/classify-pr-profile.test.mjs | 19 ++- .github/scripts/upsert-bot-comment.sh | 60 ++++++++ .github/scripts/upsert-bot-comment.test.mjs | 139 ++++++++++++++++++ .github/workflows/ci.yml | 2 +- .github/workflows/qwen-code-pr-review.yml | 109 ++++++++------ scripts/tests/qwen-pr-review-workflow.test.js | 60 ++++++++ 6 files changed, 334 insertions(+), 55 deletions(-) create mode 100755 .github/scripts/upsert-bot-comment.sh create mode 100644 .github/scripts/upsert-bot-comment.test.mjs diff --git a/.github/scripts/ci/classify-pr-profile.test.mjs b/.github/scripts/ci/classify-pr-profile.test.mjs index 70cb8301867..d753304774d 100644 --- a/.github/scripts/ci/classify-pr-profile.test.mjs +++ b/.github/scripts/ci/classify-pr-profile.test.mjs @@ -41,20 +41,29 @@ function run(scenario, { stubNodeFailure = false } = {}) { chmodSync(p, 0o755); }; // The gh stub serves the two calls the wrapper makes: the paginated file - // listing (JSONL via --jq) and the PR object's changed_files count. + // listing and the PR object's changed_files count. For the listing it + // applies the wrapper's OWN `--jq` argument to a full API-shaped fixture + // with real jq — so the projection (the input contract this wrapper exists + // to be the single home of) is genuinely under test: dropping `status` or + // `previous_filename` from it changes what the classifier sees and turns + // the renamed-source scenario red, instead of the stub hardcoding the + // projected output and keeping every projection mutant green. write( 'gh', [ '#!/bin/bash', + 'jqfilter=""; prev=""', + 'for a in "$@"; do if [ "$prev" = "--jq" ]; then jqfilter="$a"; fi; prev="$a"; done', 'case "$*" in', ' *"/files"*)', ' case "$SCENARIO" in', ' list-fail) exit 1 ;;', - ' docs-only) printf \'%s\\n\' \'{"filename":"docs/users/a.md","status":"modified","previous_filename":null}\' \'{"filename":"README.md","status":"modified","previous_filename":null}\' ;;', - ' renamed-source) printf \'%s\\n\' \'{"filename":"docs/new.md","status":"renamed","previous_filename":"packages/core/src/runtime.ts"}\' ;;', - ' truncated) printf \'%s\\n\' \'{"filename":"docs/users/a.md","status":"modified","previous_filename":null}\' ;;', + ' docs-only) FIXTURE=\'[{"filename":"docs/users/a.md","status":"modified","previous_filename":null,"sha":"x","additions":1},{"filename":"README.md","status":"modified","previous_filename":null,"sha":"y","additions":1}]\' ;;', + ' renamed-source) FIXTURE=\'[{"filename":"docs/new.md","status":"renamed","previous_filename":"packages/core/src/runtime.ts","sha":"z","additions":0}]\' ;;', + ' truncated) FIXTURE=\'[{"filename":"docs/users/a.md","status":"modified","previous_filename":null,"sha":"x","additions":1}]\' ;;', ' *) exit 9 ;;', - ' esac ;;', + ' esac', + ' printf \'%s\' "$FIXTURE" | jq -c "$jqfilter" ;;', ' *"repos/"*)', ' case "$SCENARIO" in', ' truncated) echo 5 ;;', diff --git a/.github/scripts/upsert-bot-comment.sh b/.github/scripts/upsert-bot-comment.sh new file mode 100755 index 00000000000..97db6a187c2 --- /dev/null +++ b/.github/scripts/upsert-bot-comment.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# Upsert a marker-identified bot comment on a PR/issue. +# +# One implementation of the marker+author upsert protocol, shared by the +# docs-only relay and the stale-badge supersede step in +# qwen-code-pr-review.yml (the previous per-step copies had already drifted: +# one had retry/null-guards/dynamic login, the other none). The lookup is +# author-scoped — only comments by the authenticated login are upsert +# targets, so a participant posting the marker can never capture the upsert — +# and is re-resolved on every attempt: a transient listing failure must not +# settle the loop into the POST branch (a permanent duplicate), and a +# comment deleted mid-retry must fall back to POST rather than PATCHing a +# stale id repeatedly. +# +# Usage: upsert-bot-comment.sh [--update-only] +# --update-only: PATCH an existing bot-authored marker comment if present; +# succeed as a no-op when none exists (never POSTs). For +# superseding a badge without minting one where none was. +# Exit codes: 0 posted/updated/no-op; 1 all attempts failed. +set -euo pipefail + +repo="${1:?usage: upsert-bot-comment.sh [--update-only]}" +number="${2:?missing issue number}" +marker="${3:?missing marker}" +body_file="${4:?missing body file}" +update_only="${5:-}" + +body="$(cat "${body_file}")" +bot_login="$(gh api user --jq '.login')" || bot_login="" + +for _attempt in 1 2 3; do + existing_id="$( + gh api "repos/${repo}/issues/${number}/comments" \ + --method GET \ + --paginate \ + -F per_page=100 \ + | jq -sr --arg bot "${bot_login}" --arg marker "${marker}" '[.[][] + | select((.user.login // "") == $bot) + | select((.body // "") | contains($marker))] + | last | .id // empty' + )" || existing_id="" + if [ -n "${existing_id}" ]; then + if gh api --method PATCH \ + "repos/${repo}/issues/comments/${existing_id}" \ + -f body="${body}" >/dev/null; then + echo "updated comment ${existing_id}" + exit 0 + fi + elif [ "${update_only}" = "--update-only" ]; then + echo "no existing comment; nothing to update" + exit 0 + elif gh api "repos/${repo}/issues/${number}/comments" \ + -f body="${body}" >/dev/null; then + echo "posted new comment" + exit 0 + fi + sleep 10 +done +echo "all attempts failed" >&2 +exit 1 diff --git a/.github/scripts/upsert-bot-comment.test.mjs b/.github/scripts/upsert-bot-comment.test.mjs new file mode 100644 index 00000000000..fe005b9f0a1 --- /dev/null +++ b/.github/scripts/upsert-bot-comment.test.mjs @@ -0,0 +1,139 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Executes the real upsert-bot-comment.sh with a stubbed `gh` + `sleep` on +// PATH. The scenarios pin the protocol's load-bearing properties, each of +// which survived as a green mutant when it lived untested inside a workflow +// step: the author scope (a user-posted marker must not capture the upsert), +// the per-attempt re-resolution (a comment deleted mid-retry falls back to +// POST instead of PATCHing a stale id), and the --update-only no-op (a +// supersede must never mint a badge where none existed). + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const here = dirname(fileURLToPath(import.meta.url)); +const script = join(here, 'upsert-bot-comment.sh'); +const MARKER = ''; + +function run(scenario, { updateOnly = false } = {}) { + const dir = mkdtempSync(join(tmpdir(), 'upsert-bot-comment-')); + const bin = join(dir, 'bin'); + mkdirSync(bin); + const calls = join(dir, 'calls'); + writeFileSync(calls, ''); + const bodyFile = join(dir, 'body'); + writeFileSync(bodyFile, `${MARKER}\nhello`); + const write = (name, body) => { + writeFileSync(join(bin, name), body); + chmodSync(join(bin, name), 0o755); + }; + write('sleep', '#!/bin/bash\nexit 0\n'); + write( + 'gh', + [ + '#!/bin/bash', + 'echo "$*" >> "$CALLS"', + 'n=$(grep -c "method GET" "$CALLS" || true)', + 'case "$*" in', + ' "api user"*) echo bot ;;', + ' *"--method GET"*)', + ' case "$SCENARIO" in', + ' fresh) echo "[]" ;;', + ' existing-bot) echo \'[{"id":7,"user":{"login":"bot"},"body":" old"}]\' ;;', + ' existing-user) echo \'[{"id":8,"user":{"login":"alice"},"body":" mine"}]\' ;;', + ' deleted-mid-retry)', + ' if [ "$n" -le 1 ]; then echo \'[{"id":9,"user":{"login":"bot"},"body":" old"}]\'; else echo "[]"; fi ;;', + ' esac ;;', + ' *"--method PATCH"*)', + ' if [ "$SCENARIO" = "deleted-mid-retry" ]; then exit 1; fi ;;', + ' *) : ;;', + 'esac', + 'exit 0', + ].join('\n') + '\n', + ); + let code = 0; + let stdout = ''; + try { + stdout = execFileSync( + 'bash', + [ + script, + 'o/r', + '42', + MARKER, + bodyFile, + ...(updateOnly ? ['--update-only'] : []), + ], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + SCENARIO: scenario, + CALLS: calls, + }, + }, + ); + } catch (e) { + code = e.status; + stdout = `${e.stdout ?? ''}`; + } + const recorded = readFileSync(calls, 'utf8'); + rmSync(dir, { recursive: true, force: true }); + return { code, stdout, calls: recorded }; +} + +test('POSTs a fresh comment when no bot-authored marker exists', () => { + const r = run('fresh'); + assert.equal(r.code, 0); + assert.match(r.stdout, /posted new comment/); + assert.doesNotMatch(r.calls, /--method PATCH/); +}); + +test('PATCHes the existing bot-authored marker comment', () => { + const r = run('existing-bot'); + assert.equal(r.code, 0); + assert.match(r.stdout, /updated comment 7/); + assert.match(r.calls, /--method PATCH repos\/o\/r\/issues\/comments\/7/); +}); + +test('a user-authored marker comment never captures the upsert', () => { + const r = run('existing-user'); + assert.equal(r.code, 0); + assert.match(r.stdout, /posted new comment/); + assert.doesNotMatch(r.calls, /--method PATCH/); +}); + +test('falls back to POST when the target vanishes mid-retry (re-resolution)', () => { + const r = run('deleted-mid-retry'); + assert.equal(r.code, 0); + assert.match(r.stdout, /posted new comment/); + // Attempt 1 PATCHed the stale id and failed; attempt 2 re-resolved to + // empty and POSTed — a hoisted lookup would PATCH id 9 three times. + assert.match(r.calls, /--method PATCH repos\/o\/r\/issues\/comments\/9/); +}); + +test('--update-only is a no-op success when nothing exists', () => { + const r = run('fresh', { updateOnly: true }); + assert.equal(r.code, 0); + assert.match(r.stdout, /nothing to update/); + assert.doesNotMatch(r.calls, /--method PATCH/); + // And no POST either: the only api writes would be comment creation. + assert.doesNotMatch(r.calls, /issues\/42\/comments -f/); +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 567eb3e0f77..4ce19b8b22b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ env: # BOTH the github_ci_only helper step and the full-profile Test step, so a # new helper test can't be added to one path and silently dropped from the # other. - HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs' + HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/upsert-bot-comment.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs' jobs: classify_pr: diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 456d06dd45e..883b19c685c 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1226,12 +1226,14 @@ jobs: # form, which deliberately does NOT start with the reserved # `Review complete: ` prefix so log scrapers never parse it. COMPLETION_LINE="$(printf '%s\n' "$RESULT_TEXT" | grep -E '^Review complete: ' | tail -n1 || true)" - # Bound to THIS PR's target and to the only verdict a medium run - # can produce (medium caps at Comment): a stale line for another - # PR, or an injection-steered Approve/Request-changes shape, - # must not be republished under the bot's name. + # Bound to THIS PR's target and to the verdicts a medium run can + # produce: Comment, or Request changes when it verified a Critical + # (compose-review caps only Approve at medium — a docs-only run + # that found a blocker is exactly the outcome the relay must not + # swallow). A stale line for another PR, or an Approve-shaped + # injection, must not be republished under the bot's name. if ! printf '%s' "$COMPLETION_LINE" \ - | grep -qE "^Review complete: pr-${PR_NUMBER} — Comment, not posted \([0-9]+ Critical, [0-9]+ Suggestion\)$"; then + | grep -qE "^Review complete: pr-${PR_NUMBER} — (Comment|Request changes), not posted \([0-9]+ Critical, [0-9]+ Suggestion\)$"; then COMPLETION_LINE="" fi echo "completion_line=${COMPLETION_LINE:-(no relayable \"Review complete:\" line in the run output — see the run log)}" >> "$GITHUB_OUTPUT" @@ -1274,55 +1276,64 @@ jobs: "📄 **纯文档变更** —— 自动评审以 \`--effort medium\` 运行（发现已验证、无反向审计；medium 不发布行内评论），结果见上方引用行。完整报告见 [workflow 运行](${RUN_URL})；如需带行内评论的完整高强度（high-effort）评审，请评论 \`@qwen-code /review\`。" \ '' \ '')" - # Upsert by marker AND author, mirroring the queued-acknowledgement - # step: each push of a docs-only PR re-runs the automatic review, and - # without the PATCH branch every run would stack another - # near-identical relay comment on the PR page. The author filter is - # load-bearing — a PR participant can post the marker themselves, and - # an author-blind `last` match would then PATCH user-owned content - # (or fail on it forever, suppressing the authoritative relay). - BOT_LOGIN="$(gh api user --jq '.login')" || BOT_LOGIN="" - # Bounded retry, and never fail the job over the relay: the review - # itself succeeded, and a failing step here would trip the - # post-failure fallback into announcing a review failure that never - # happened. Losing the relay costs a comment; the outcome line below - # keeps it recoverable from the job log. The lookup is re-resolved - # on every attempt — a transient failure on the listing GET must not - # settle the loop into the POST branch (a permanent duplicate), and - # a comment deleted between attempts must fall back to POST rather - # than PATCHing a 404 three times. - posted=false - for attempt in 1 2 3; do - EXISTING_ID="$( - gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ - --method GET \ - --paginate \ - -F per_page=100 \ - | jq -sr --arg bot "$BOT_LOGIN" '[.[][] - | select((.user.login // "") == $bot) - | select(.body | contains(""))] - | last | .id // empty' - )" || EXISTING_ID="" - if [ -n "$EXISTING_ID" ]; then - if gh api --method PATCH \ - "repos/${GITHUB_REPOSITORY}/issues/comments/${EXISTING_ID}" \ - -f body="$BODY" >/dev/null; then - posted=true - break - fi - elif gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ - -f body="$BODY" >/dev/null; then - posted=true - break - fi - sleep 10 - done - if [ "$posted" = "true" ]; then + # The shared upsert protocol (.github/scripts/upsert-bot-comment.sh) + # carries the load-bearing properties: author-scoped lookup (a PR + # participant posting the marker can never capture the upsert), + # per-attempt re-resolution, and bounded retry. Never fail the job + # over the relay: the review itself succeeded, and a failing step + # here would trip the post-failure fallback into announcing a + # review failure that never happened — losing the relay costs a + # comment, and the outcome line below keeps it recoverable from + # the job log. + printf '%s' "$BODY" > "${RUNNER_TEMP}/docs-only-relay-body.md" + if .github/scripts/upsert-bot-comment.sh \ + "${GITHUB_REPOSITORY}" "${PR_NUMBER}" \ + '' \ + "${RUNNER_TEMP}/docs-only-relay-body.md"; then echo "docs-only medium outcome relayed to PR #${PR_NUMBER}." else echo "::warning::Docs-only relay comment could not be posted after 3 attempts; the review itself succeeded. Outcome: ${COMPLETION_LINE}" fi + - name: 'Supersede stale docs-only badge' + if: |- + steps.context.outputs.should_run == 'true' && + steps.review.outcome == 'success' && + steps.review.outputs.review_completed == 'true' && + steps.review.outputs.docs_only_medium == 'false' && + steps.context.outputs.auto_review == 'true' && + steps.context.outputs.pr_number != '' + env: + GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' + PR_NUMBER: '${{ steps.context.outputs.pr_number }}' + run: |- + set -euo pipefail + # A docs-only badge is upserted only by docs-only runs; when a later + # push makes the PR non-docs-only, the full automatic review runs + # here — and without this step the stale badge would keep asserting + # that the automatic review ran at medium with no reverse audit, a + # misrepresentation of the current head's review state that nothing + # ever corrects. --update-only makes this a strict no-op on PRs + # that never carried the badge; failure is best-effort (the badge + # is cosmetic next to the posted full review). + BODY="$(printf '%s\n' \ + '' \ + '' \ + '📄 ~~Docs-only change~~ **(superseded)** — a later push made this PR no longer docs-only; the automatic review for the current head ran the full high-effort pipeline. See the posted review on this PR.' \ + '' \ + '
中文说明' \ + '' \ + '📄 ~~纯文档变更~~ **（已失效）** —— 后续推送使本 PR 不再是纯文档变更；当前 head 的自动评审已运行完整 high-effort 流水线，结论见本 PR 上发布的评审。' \ + '' \ + '
')" + printf '%s' "$BODY" > "${RUNNER_TEMP}/docs-only-supersede-body.md" + .github/scripts/upsert-bot-comment.sh \ + "${GITHUB_REPOSITORY}" "${PR_NUMBER}" \ + '' \ + "${RUNNER_TEMP}/docs-only-supersede-body.md" \ + --update-only \ + || echo "::warning::Could not supersede the stale docs-only badge." + - name: 'Post fallback comment on failure' if: |- failure() && diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 7c45c141592..0b5adc1bb90 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1375,6 +1375,32 @@ describe('docs-only medium gate', () => { expect(line).not.toContain('completion_line=Review complete:'); }); + it('accepts the Request-changes disposition a Critical-finding medium run emits', () => { + // compose-review caps only Approve at medium: a verified Critical still + // yields Request changes, and that is exactly the outcome the relay must + // not swallow into the neutral fallback. + const line = + 'Review complete: pr-123 — Request changes, not posted (1 Critical, 0 Suggestion)'; + expect(relayLine(`prose...\n${line}`)).toBe(`completion_line=${line}`); + }); + + it('relays the LAST completion line, not a stale or injected earlier one', () => { + const stale = + 'Review complete: pr-123 — Comment, not posted (9 Critical, 9 Suggestion)'; + const valid = + 'Review complete: pr-123 — Comment, not posted (0 Critical, 2 Suggestion)'; + expect(relayLine(`${stale}\nmore prose\n${valid}`)).toBe( + `completion_line=${valid}`, + ); + }); + + it("rejects another PR's completion line (target binding)", () => { + const line = relayLine( + 'Review complete: pr-999 — Comment, not posted (0 Critical, 2 Suggestion)', + ); + expect(line.startsWith('completion_line=(no relayable')).toBe(true); + }); + it('classifies review_requested as an explicit ask, never automatic', () => { const doc = parse(workflow); const context = doc.jobs['review-pr'].steps.find((s) => s.id === 'context'); @@ -1543,6 +1569,7 @@ describe('docs-only gate and relay, executed', () => { 'set -euo pipefail', 'GITHUB_REPOSITORY=o/r', 'PR_NUMBER=42', + `RUNNER_TEMP="${dir}"`, 'COMPLETION_LINE="Review complete: pr-42 — Comment, not posted (0 Critical, 1 Suggestion)"', 'RUN_URL=https://x', relayRun, @@ -1582,4 +1609,37 @@ describe('docs-only gate and relay, executed', () => { expect(r.stdout).toContain('::warning::'); expect(r.stdout).toContain('the review itself succeeded'); }); + + it('pins the review_completed wiring end to end', () => { + // The state/head guards exit 0 without running the review; the relay and + // the badge-supersede step must both require the dedicated output, and + // the run step must emit it after the retry loop. + expect(runStep).toContain( + 'echo "review_completed=true" >> "$GITHUB_OUTPUT"', + ); + const doc2 = parse(workflow); + for (const name of [ + 'Report docs-only medium outcome', + 'Supersede stale docs-only badge', + ]) { + const step = doc2.jobs['review-pr'].steps.find((s) => s.name === name); + expect(step.if).toContain( + "steps.review.outputs.review_completed == 'true'", + ); + } + }); + + it('pins the auto_review output→env wiring at both links', () => { + const doc2 = parse(workflow); + const context = doc2.jobs['review-pr'].steps.find( + (s) => s.id === 'context', + ); + expect(context.run).toContain('echo "auto_review=$AUTO_REVIEW"'); + const review = doc2.jobs['review-pr'].steps.find( + (s) => s.name === 'Run review', + ); + expect(review.env.AUTO_REVIEW).toBe( + '${{ steps.context.outputs.auto_review }}', + ); + }); }); From 2c648b13b8c67d3db2c45988aa52c27ac7bd716b Mon Sep 17 00:00:00 2001 From: verify Date: Fri, 7 Aug 2026 17:26:24 +0800 Subject: [PATCH 5/7] perf(ci): never let a failed lookup mint or keep a stale docs badge Round-4 review findings (1 Critical, 7 Suggestions): - The upsert script no longer conflates failed lookups with empty results: the authenticated login, the listing, and the jq extraction are all resolved inside the retry loop as one prerequisite chain, an attempt whose prerequisites failed retries instead of falling through to POST (the shape that minted a permanent duplicate badge off one transient 5xx), and --update-only exits 1 on a failed lookup so the supersede warning fires instead of a false no-op success. New tests pin the failed-listing-then-PATCH path, the persistent identity failure, the update-only failure exit, and the update-only PATCH. - Supersede now covers every path that owes the correction: a FAILED full review and an EXPLICIT requested review (the badge's own CTA) both retire the badge, gated only on docs_only_medium == 'false' - empty on runs that failed before classifying, so a badge is never superseded on ignorance. The body is cause-neutral: it asserts only that the badge described an earlier revision. - The marker literal is defined once per step (MARKER variable, the qwen-triage convention) and shared between body and lookup argument; a pin requires the definition and --update-only on the supersede invocation. - New behavioral pins: the Approve verdict stays rejected by the allowlist, github_ci_only never downgrades (CI helpers are executable), and review_completed's emit position is asserted AFTER the closed-PR and stale-head guards (the hoist mutant survived position-independent contains checks). --- .github/scripts/upsert-bot-comment.sh | 50 +++++++------- .github/scripts/upsert-bot-comment.test.mjs | 38 ++++++++++- .github/workflows/qwen-code-pr-review.yml | 45 ++++++++----- scripts/tests/qwen-pr-review-workflow.test.js | 65 +++++++++++++++++-- 4 files changed, 150 insertions(+), 48 deletions(-) diff --git a/.github/scripts/upsert-bot-comment.sh b/.github/scripts/upsert-bot-comment.sh index 97db6a187c2..965eda4e962 100755 --- a/.github/scripts/upsert-bot-comment.sh +++ b/.github/scripts/upsert-bot-comment.sh @@ -6,11 +6,16 @@ # qwen-code-pr-review.yml (the previous per-step copies had already drifted: # one had retry/null-guards/dynamic login, the other none). The lookup is # author-scoped — only comments by the authenticated login are upsert -# targets, so a participant posting the marker can never capture the upsert — -# and is re-resolved on every attempt: a transient listing failure must not -# settle the loop into the POST branch (a permanent duplicate), and a -# comment deleted mid-retry must fall back to POST rather than PATCHing a -# stale id repeatedly. +# targets, so a participant posting the marker can never capture the upsert. +# +# A FAILED lookup is never treated as an EMPTY result: posting on a failed +# listing is how a transient 5xx mints a permanent duplicate (later runs +# PATCH only the `last` match), so every prerequisite — the authenticated +# login and the listing — is re-resolved inside the retry loop, and an +# attempt whose prerequisites failed retries instead of falling through to +# POST. On --update-only, a failed lookup exits 1 (the caller's warning +# path), never the no-op success reserved for a lookup that genuinely +# found nothing. # # Usage: upsert-bot-comment.sh [--update-only] # --update-only: PATCH an existing bot-authored marker comment if present; @@ -26,33 +31,34 @@ body_file="${4:?missing body file}" update_only="${5:-}" body="$(cat "${body_file}")" -bot_login="$(gh api user --jq '.login')" || bot_login="" for _attempt in 1 2 3; do - existing_id="$( - gh api "repos/${repo}/issues/${number}/comments" \ + if bot_login="$(gh api user --jq '.login')" \ + && [ -n "${bot_login}" ] \ + && listing="$(gh api "repos/${repo}/issues/${number}/comments" \ --method GET \ --paginate \ - -F per_page=100 \ + -F per_page=100)" \ + && existing_id="$(printf '%s' "${listing}" \ | jq -sr --arg bot "${bot_login}" --arg marker "${marker}" '[.[][] | select((.user.login // "") == $bot) | select((.body // "") | contains($marker))] - | last | .id // empty' - )" || existing_id="" - if [ -n "${existing_id}" ]; then - if gh api --method PATCH \ - "repos/${repo}/issues/comments/${existing_id}" \ + | last | .id // empty')"; then + if [ -n "${existing_id}" ]; then + if gh api --method PATCH \ + "repos/${repo}/issues/comments/${existing_id}" \ + -f body="${body}" >/dev/null; then + echo "updated comment ${existing_id}" + exit 0 + fi + elif [ "${update_only}" = "--update-only" ]; then + echo "no existing comment; nothing to update" + exit 0 + elif gh api "repos/${repo}/issues/${number}/comments" \ -f body="${body}" >/dev/null; then - echo "updated comment ${existing_id}" + echo "posted new comment" exit 0 fi - elif [ "${update_only}" = "--update-only" ]; then - echo "no existing comment; nothing to update" - exit 0 - elif gh api "repos/${repo}/issues/${number}/comments" \ - -f body="${body}" >/dev/null; then - echo "posted new comment" - exit 0 fi sleep 10 done diff --git a/.github/scripts/upsert-bot-comment.test.mjs b/.github/scripts/upsert-bot-comment.test.mjs index fe005b9f0a1..687380220c5 100644 --- a/.github/scripts/upsert-bot-comment.test.mjs +++ b/.github/scripts/upsert-bot-comment.test.mjs @@ -51,9 +51,14 @@ function run(scenario, { updateOnly = false } = {}) { 'echo "$*" >> "$CALLS"', 'n=$(grep -c "method GET" "$CALLS" || true)', 'case "$*" in', - ' "api user"*) echo bot ;;', + ' "api user"*)', + ' if [ "$SCENARIO" = "user-fails" ]; then exit 1; fi', + ' echo bot ;;', ' *"--method GET"*)', ' case "$SCENARIO" in', + ' listing-always-fails) exit 1 ;;', + ' listing-fails-once)', + ' if [ "$n" -le 1 ]; then exit 1; else echo \'[{"id":7,"user":{"login":"bot"},"body":" old"}]\'; fi ;;', ' fresh) echo "[]" ;;', ' existing-bot) echo \'[{"id":7,"user":{"login":"bot"},"body":" old"}]\' ;;', ' existing-user) echo \'[{"id":8,"user":{"login":"alice"},"body":" mine"}]\' ;;', @@ -137,3 +142,34 @@ test('--update-only is a no-op success when nothing exists', () => { // And no POST either: the only api writes would be comment creation. assert.doesNotMatch(r.calls, /issues\/42\/comments -f/); }); + +test('a failed listing NEVER falls through to POST (retries, then PATCHes)', () => { + // The critical shape: the badge already exists, the first listing GET hits + // a transient failure. Conflating that failure with "no match" would POST + // a permanent duplicate; the fix retries and PATCHes the real comment. + const r = run('listing-fails-once'); + assert.equal(r.code, 0); + assert.match(r.stdout, /updated comment 7/); + assert.doesNotMatch(r.calls, /issues\/42\/comments -f/); +}); + +test('a persistently failing identity lookup exits 1 without writing', () => { + const r = run('user-fails'); + assert.equal(r.code, 1); + assert.doesNotMatch(r.calls, /--method PATCH/); + assert.doesNotMatch(r.calls, /issues\/42\/comments -f/); +}); + +test('--update-only with a failing lookup exits 1, not the no-op success', () => { + // The supersede caller's ::warning:: path depends on this: a failed lookup + // must not masquerade as "nothing to update". + const r = run('listing-always-fails', { updateOnly: true }); + assert.equal(r.code, 1); +}); + +test('--update-only PATCHes an existing bot-authored badge', () => { + const r = run('existing-bot', { updateOnly: true }); + assert.equal(r.code, 0); + assert.match(r.stdout, /updated comment 7/); + assert.match(r.calls, /--method PATCH repos\/o\/r\/issues\/comments\/7/); +}); diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 883b19c685c..8c5215a3de6 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1261,9 +1261,12 @@ jobs: # effort back on (parse-args), so a docs-only automatic review # reports through this single relay instead. The quoted line arrives # allowlisted by "Run review" (not-posted disposition shape only) — - # this step asserts nothing the run did not print. + # this step asserts nothing the run did not print. The marker is + # defined ONCE and used for both the body and the upsert lookup — + # the two must be byte-identical or the upsert posts duplicates. + MARKER='' BODY="$(printf '%s\n' \ - '' \ + "$MARKER" \ '' \ "📄 **Docs-only change** — the automatic review ran at \`--effort medium\` (verified findings, no reverse audit; medium posts no inline comments). Outcome:" \ '' \ @@ -1288,7 +1291,7 @@ jobs: printf '%s' "$BODY" > "${RUNNER_TEMP}/docs-only-relay-body.md" if .github/scripts/upsert-bot-comment.sh \ "${GITHUB_REPOSITORY}" "${PR_NUMBER}" \ - '' \ + "$MARKER" \ "${RUNNER_TEMP}/docs-only-relay-body.md"; then echo "docs-only medium outcome relayed to PR #${PR_NUMBER}." else @@ -1296,40 +1299,46 @@ jobs: fi - name: 'Supersede stale docs-only badge' + # Runs whenever this run determined the PR is NOT (or no longer) + # docs-only — including a FAILED full review and an EXPLICIT + # requested review: the stale badge misrepresents the PR whether or + # not this particular run completed (a failed full review posts its + # own failure fallback; an explicit run is the badge's own CTA and + # must retire it). `docs_only_medium == 'false'` is the load-bearing + # gate: a run that failed before classifying emits nothing (empty != + # 'false'), so a badge is never superseded on ignorance — and the + # supersede body is cause-neutral, asserting only what is true on + # every path here: the badge described an earlier revision. if: |- + !cancelled() && steps.context.outputs.should_run == 'true' && - steps.review.outcome == 'success' && - steps.review.outputs.review_completed == 'true' && steps.review.outputs.docs_only_medium == 'false' && - steps.context.outputs.auto_review == 'true' && steps.context.outputs.pr_number != '' env: GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' PR_NUMBER: '${{ steps.context.outputs.pr_number }}' run: |- set -euo pipefail - # A docs-only badge is upserted only by docs-only runs; when a later - # push makes the PR non-docs-only, the full automatic review runs - # here — and without this step the stale badge would keep asserting - # that the automatic review ran at medium with no reverse audit, a - # misrepresentation of the current head's review state that nothing - # ever corrects. --update-only makes this a strict no-op on PRs - # that never carried the badge; failure is best-effort (the badge - # is cosmetic next to the posted full review). + # --update-only makes this a strict no-op on PRs that never + # carried the badge; a failed lookup exits 1 (never the no-op), so + # the warning below fires instead of silently keeping a stale + # badge. The marker is defined once and used for both the body and + # the lookup — they must be byte-identical. + MARKER='' BODY="$(printf '%s\n' \ - '' \ + "$MARKER" \ '' \ - '📄 ~~Docs-only change~~ **(superseded)** — a later push made this PR no longer docs-only; the automatic review for the current head ran the full high-effort pipeline. See the posted review on this PR.' \ + '📄 ~~Docs-only change~~ **(superseded)** — this badge described an earlier docs-only revision of this PR and no longer reflects the current head. See the latest review activity on this PR.' \ '' \ '
中文说明' \ '' \ - '📄 ~~纯文档变更~~ **（已失效）** —— 后续推送使本 PR 不再是纯文档变更；当前 head 的自动评审已运行完整 high-effort 流水线，结论见本 PR 上发布的评审。' \ + '📄 ~~纯文档变更~~ **（已失效）** —— 该徽章描述的是本 PR 更早的纯文档修订，已不再反映当前 head。请以本 PR 上最新的评审动态为准。' \ '' \ '
')" printf '%s' "$BODY" > "${RUNNER_TEMP}/docs-only-supersede-body.md" .github/scripts/upsert-bot-comment.sh \ "${GITHUB_REPOSITORY}" "${PR_NUMBER}" \ - '' \ + "$MARKER" \ "${RUNNER_TEMP}/docs-only-supersede-body.md" \ --update-only \ || echo "::warning::Could not supersede the stale docs-only badge." diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 0b5adc1bb90..db85114a0d8 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1394,6 +1394,15 @@ describe('docs-only medium gate', () => { ); }); + it('rejects the Approve verdict medium can never produce', () => { + // Widening the alternation to include Approve must turn this red: an + // injection-steered approval must not be republished under the bot's name. + const line = relayLine( + 'Review complete: pr-123 — Approve, not posted (0 Critical, 0 Suggestion)', + ); + expect(line.startsWith('completion_line=(no relayable')).toBe(true); + }); + it("rejects another PR's completion line (target binding)", () => { const line = relayLine( 'Review complete: pr-999 — Comment, not posted (0 Critical, 2 Suggestion)', @@ -1527,6 +1536,15 @@ describe('docs-only gate and relay, executed', () => { expect(r.stdout).toContain('timeout=360'); }); + it('keeps the full review for github_ci_only (CI helpers are executable)', () => { + const r = runGate({ + autoReview: 'true', + wrapper: '#!/bin/bash\necho github_ci_only\n', + }); + expect(r.output).toBe('docs_only_medium=false'); + expect(r.stdout).toContain('timeout=360'); + }); + it('never classifies on an explicit (non-automatic) run', () => { const r = runGate({ autoReview: 'false', @@ -1611,22 +1629,55 @@ describe('docs-only gate and relay, executed', () => { }); it('pins the review_completed wiring end to end', () => { - // The state/head guards exit 0 without running the review; the relay and - // the badge-supersede step must both require the dedicated output, and - // the run step must emit it after the retry loop. - expect(runStep).toContain( - 'echo "review_completed=true" >> "$GITHUB_OUTPUT"', + // The state/head guards exit 0 without running the review; the relay + // must require the dedicated output, and the run step must emit it + // AFTER those guards — a hoisted emit would open the relay gate for a + // closed/stale PR whose review never ran (position pinned below). The + // supersede step deliberately does NOT require it: a failed full review + // still owes the badge correction, gated on docs_only_medium == 'false' + // (empty on runs that failed before classifying). + const emitAt = runStep.indexOf('echo "review_completed=true"'); + expect(emitAt).toBeGreaterThan(-1); + expect(emitAt).toBeGreaterThan( + runStep.indexOf('if [ "$PR_STATE" != "OPEN" ]'), + ); + expect(emitAt).toBeGreaterThan( + runStep.indexOf('Skipping stale review run'), ); + const doc2 = parse(workflow); + const relay = doc2.jobs['review-pr'].steps.find( + (s) => s.name === 'Report docs-only medium outcome', + ); + expect(relay.if).toContain( + "steps.review.outputs.review_completed == 'true'", + ); + const supersede = doc2.jobs['review-pr'].steps.find( + (s) => s.name === 'Supersede stale docs-only badge', + ); + expect(supersede.if).not.toContain('review_completed'); + expect(supersede.if).toContain( + "steps.review.outputs.docs_only_medium == 'false'", + ); + expect(supersede.if).toContain('!cancelled()'); + }); + + it('pins the supersede invocation shape (update-only, shared marker)', () => { const doc2 = parse(workflow); for (const name of [ 'Report docs-only medium outcome', 'Supersede stale docs-only badge', ]) { const step = doc2.jobs['review-pr'].steps.find((s) => s.name === name); - expect(step.if).toContain( - "steps.review.outputs.review_completed == 'true'", + // One marker definition serving both the body and the lookup argument. + expect(step.run).toContain( + "MARKER=''", ); + expect(step.run).toContain('"$MARKER"'); } + const supersede = doc2.jobs['review-pr'].steps.find( + (s) => s.name === 'Supersede stale docs-only badge', + ); + expect(supersede.run).toContain('--update-only'); }); it('pins the auto_review output→env wiring at both links', () => { From e3a20ecbc065782cc7e2a422d968994e15ac06e6 Mon Sep 17 00:00:00 2001 From: verify Date: Fri, 7 Aug 2026 21:32:51 +0800 Subject: [PATCH 6/7] perf(ci): make docs_only_medium three-valued and pin the untested guards Round-5 review findings (1 Critical, 6 Suggestions): - docs_only_medium no longer conflates "determined not docs-only" with "never determined": the output is three-valued ('' when the classification failed or never ran), so a transient classifier failure or a dispatch dry-run can no longer retire a still-accurate badge. The supersede condition names its two licensed paths explicitly - a POSITIVE not-docs-only determination (without requiring review success), or an explicit comment-mode review that completed (the badge's CTA; report-mode dry runs retire nothing). - The count-mismatch fallback in classify-pr-profile.sh logs to stderr, so a systematic divergence is distinguishable from every PR genuinely classifying full. - Six probed surviving mutants now each turn a test red: the supersede body is executed (existing-badge PATCH and the never-fail guard), ci.yml's rc-handling fragment is executed (exit 0/2/3 with the full fallback), the changed_files fetch failure exits 2, duplicate badges PATCH the last (newest) comment, and the relay's POSTed body must carry the marker that keys both the upsert and the supersede. --- .github/scripts/ci/classify-pr-profile.sh | 1 + .../scripts/ci/classify-pr-profile.test.mjs | 10 ++ .github/scripts/upsert-bot-comment.test.mjs | 12 ++ .github/workflows/qwen-code-pr-review.yml | 44 +++-- scripts/tests/qwen-pr-review-workflow.test.js | 161 +++++++++++++++++- 5 files changed, 211 insertions(+), 17 deletions(-) diff --git a/.github/scripts/ci/classify-pr-profile.sh b/.github/scripts/ci/classify-pr-profile.sh index 4b188476fb5..f9659915c5f 100755 --- a/.github/scripts/ci/classify-pr-profile.sh +++ b/.github/scripts/ci/classify-pr-profile.sh @@ -37,6 +37,7 @@ fi declared="$(gh api "repos/${repo}/pulls/${pr}" --jq '.changed_files')" || exit 2 retrieved="$(wc -l < "${files}")" if [ "${retrieved}" -ne "${declared}" ]; then + echo "classify-pr-profile: retrieved ${retrieved} file entries but PR declares ${declared}; classifying full." >&2 echo "full" exit 0 fi diff --git a/.github/scripts/ci/classify-pr-profile.test.mjs b/.github/scripts/ci/classify-pr-profile.test.mjs index d753304774d..a154972bcff 100644 --- a/.github/scripts/ci/classify-pr-profile.test.mjs +++ b/.github/scripts/ci/classify-pr-profile.test.mjs @@ -61,12 +61,14 @@ function run(scenario, { stubNodeFailure = false } = {}) { ' docs-only) FIXTURE=\'[{"filename":"docs/users/a.md","status":"modified","previous_filename":null,"sha":"x","additions":1},{"filename":"README.md","status":"modified","previous_filename":null,"sha":"y","additions":1}]\' ;;', ' renamed-source) FIXTURE=\'[{"filename":"docs/new.md","status":"renamed","previous_filename":"packages/core/src/runtime.ts","sha":"z","additions":0}]\' ;;', ' truncated) FIXTURE=\'[{"filename":"docs/users/a.md","status":"modified","previous_filename":null,"sha":"x","additions":1}]\' ;;', + ' declared-fails) FIXTURE=\'[{"filename":"docs/users/a.md","status":"modified","previous_filename":null,"sha":"x","additions":1}]\' ;;', ' *) exit 9 ;;', ' esac', ' printf \'%s\' "$FIXTURE" | jq -c "$jqfilter" ;;', ' *"repos/"*)', ' case "$SCENARIO" in', ' truncated) echo 5 ;;', + ' declared-fails) exit 1 ;;', ' docs-only) echo 2 ;;', ' renamed-source) echo 1 ;;', ' *) exit 9 ;;', @@ -116,3 +118,11 @@ test('exit 2 when the file listing fails', () => { test('exit 3 when the classifier fails', () => { assert.equal(run('docs-only', { stubNodeFailure: true }).code, 3); }); + +test('exit 2 when the changed_files fetch fails after a successful listing', () => { + // The truncation guard's precondition: a swallowed failure here leaves + // `declared` empty and the guard silently skipped (probed mutant + // `|| exit 2` → `|| true` classified a docs first page as docs_only). + const r = run('declared-fails'); + assert.equal(r.code, 2); +}); diff --git a/.github/scripts/upsert-bot-comment.test.mjs b/.github/scripts/upsert-bot-comment.test.mjs index 687380220c5..6ecd2fd7649 100644 --- a/.github/scripts/upsert-bot-comment.test.mjs +++ b/.github/scripts/upsert-bot-comment.test.mjs @@ -61,6 +61,7 @@ function run(scenario, { updateOnly = false } = {}) { ' if [ "$n" -le 1 ]; then exit 1; else echo \'[{"id":7,"user":{"login":"bot"},"body":" old"}]\'; fi ;;', ' fresh) echo "[]" ;;', ' existing-bot) echo \'[{"id":7,"user":{"login":"bot"},"body":" old"}]\' ;;', + ' duplicate-pair) echo \'[{"id":5,"user":{"login":"bot"},"body":" older"},{"id":9,"user":{"login":"bot"},"body":" newer"}]\' ;;', ' existing-user) echo \'[{"id":8,"user":{"login":"alice"},"body":" mine"}]\' ;;', ' deleted-mid-retry)', ' if [ "$n" -le 1 ]; then echo \'[{"id":9,"user":{"login":"bot"},"body":" old"}]\'; else echo "[]"; fi ;;', @@ -173,3 +174,14 @@ test('--update-only PATCHes an existing bot-authored badge', () => { assert.match(r.stdout, /updated comment 7/); assert.match(r.calls, /--method PATCH repos\/o\/r\/issues\/comments\/7/); }); + +test('with duplicate badges, the upsert refreshes the LAST (newest) one', () => { + // The header's documented duplicate-resolution semantics: after a + // transient failure once minted a pair, every subsequent upsert must + // target the newest — a flip to `first` would refresh the older comment + // while the newer stale one stays the visible latest. + const r = run('duplicate-pair'); + assert.equal(r.code, 0); + assert.match(r.stdout, /updated comment 9/); + assert.doesNotMatch(r.calls, /comments\/5/); +}); diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 8c5215a3de6..de349ed4a4e 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -977,7 +977,14 @@ jobs: # (parse-args), and medium never posts — so the "Report docs-only # medium outcome" step relays the review's completion line instead. # Explicit requests (dispatch, @qwen-code /review) never downgrade. - DOCS_ONLY_MEDIUM=false + # Three-valued on purpose: 'true' (classified docs-only), 'false' + # (POSITIVELY classified not-docs-only by a successful automatic + # classification), '' (never determined — explicit run, or the + # classification failed). The supersede step keys on 'false', so + # conflating "never determined" with "determined not docs-only" + # would retire a still-accurate badge on a transient classifier + # failure or a dry run. + DOCS_ONLY_MEDIUM="" if [ "${AUTO_REVIEW:-false}" = "true" ]; then # Fetch + classify through the shared wrapper so this gate and # ci.yml's profile gate cannot drift on the classifier's input @@ -998,6 +1005,8 @@ jobs: EFFECTIVE_TIMEOUT_MINUTES=90 fi echo "PR #${PR_NUMBER} is docs-only; automatic review runs at --effort medium (${EFFECTIVE_TIMEOUT_MINUTES}-minute budget)." + else + DOCS_ONLY_MEDIUM=false fi fi echo "docs_only_medium=$DOCS_ONLY_MEDIUM" >> "$GITHUB_OUTPUT" @@ -1299,21 +1308,30 @@ jobs: fi - name: 'Supersede stale docs-only badge' - # Runs whenever this run determined the PR is NOT (or no longer) - # docs-only — including a FAILED full review and an EXPLICIT - # requested review: the stale badge misrepresents the PR whether or - # not this particular run completed (a failed full review posts its - # own failure fallback; an explicit run is the badge's own CTA and - # must retire it). `docs_only_medium == 'false'` is the load-bearing - # gate: a run that failed before classifying emits nothing (empty != - # 'false'), so a badge is never superseded on ignorance — and the - # supersede body is cause-neutral, asserting only what is true on - # every path here: the badge described an earlier revision. + # Two paths owe the badge correction, and only these two: + # (1) an automatic run whose classification POSITIVELY determined the + # PR is not (or no longer) docs-only — docs_only_medium == 'false' + # is three-valued and empty when the classifier failed or never + # ran, so a badge is never retired on ignorance; the review's own + # success is deliberately not required (a failed full review still + # leaves the badge misdescribing the head); + # (2) an explicit comment-mode review that actually completed — the + # badge's own CTA path, whose posted full review makes the badge + # redundant; a dispatch dry-run that posts nothing retires + # nothing. The body is cause-neutral: it asserts only what is + # true on every covered path. if: |- !cancelled() && steps.context.outputs.should_run == 'true' && - steps.review.outputs.docs_only_medium == 'false' && - steps.context.outputs.pr_number != '' + steps.context.outputs.pr_number != '' && + ( + steps.review.outputs.docs_only_medium == 'false' || + ( + steps.context.outputs.auto_review == 'false' && + steps.context.outputs.review_mode == 'comment' && + steps.review.outputs.review_completed == 'true' + ) + ) env: GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' PR_NUMBER: '${{ steps.context.outputs.pr_number }}' diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index db85114a0d8..71b93aba865 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1473,7 +1473,7 @@ describe('docs-only gate and relay, executed', () => { ).run; function gateSource() { - const start = runStep.indexOf('DOCS_ONLY_MEDIUM=false'); + const start = runStep.indexOf('DOCS_ONLY_MEDIUM=""'); const endAnchor = 'echo "docs_only_medium=$DOCS_ONLY_MEDIUM" >> "$GITHUB_OUTPUT"'; const end = runStep.indexOf(endAnchor) + endAnchor.length; @@ -1531,7 +1531,9 @@ describe('docs-only gate and relay, executed', () => { it('falls back to the full review when the wrapper fails', () => { const r = runGate({ autoReview: 'true', wrapper: '#!/bin/bash\nexit 2\n' }); - expect(r.output).toBe('docs_only_medium=false'); + // Empty, not 'false': a failed classification is 'never determined', and + // the supersede step must not read it as a positive determination. + expect(r.output).toBe('docs_only_medium='); expect(r.stdout).toContain('could not classify'); expect(r.stdout).toContain('timeout=360'); }); @@ -1550,7 +1552,7 @@ describe('docs-only gate and relay, executed', () => { autoReview: 'false', wrapper: '#!/bin/bash\necho docs_only\n', }); - expect(r.output).toBe('docs_only_medium=false'); + expect(r.output).toBe('docs_only_medium='); expect(r.stdout).toContain('timeout=360'); }); @@ -1612,6 +1614,10 @@ describe('docs-only gate and relay, executed', () => { expect(r.stdout).toContain('relayed to PR #42'); expect(r.calls).toContain('api repos/o/r/issues/42/comments -f'); expect(r.calls).not.toContain('--method PATCH'); + // The POSTed body must carry the marker — it is the dedup key both the + // upsert lookup and the supersede step match on; a body without it makes + // every push stack a new badge and supersede match nothing. + expect(r.calls).toContain(''); }); it('PATCHes the existing bot-authored relay comment', () => { @@ -1654,10 +1660,22 @@ describe('docs-only gate and relay, executed', () => { const supersede = doc2.jobs['review-pr'].steps.find( (s) => s.name === 'Supersede stale docs-only badge', ); - expect(supersede.if).not.toContain('review_completed'); + // Path (1): a POSITIVE not-docs-only determination (three-valued output; + // empty = never determined) — deliberately without review success. expect(supersede.if).toContain( "steps.review.outputs.docs_only_medium == 'false'", ); + // Path (2): an explicit comment-mode review that completed (the badge's + // CTA); a dispatch dry-run retires nothing. + expect(supersede.if).toContain( + "steps.context.outputs.auto_review == 'false'", + ); + expect(supersede.if).toContain( + "steps.context.outputs.review_mode == 'comment'", + ); + expect(supersede.if).toContain( + "steps.review.outputs.review_completed == 'true'", + ); expect(supersede.if).toContain('!cancelled()'); }); @@ -1694,3 +1712,138 @@ describe('docs-only gate and relay, executed', () => { ); }); }); + +describe('supersede step and ci.yml rc-handling, executed', () => { + const doc = parse(workflow); + const supersedeRun = doc.jobs['review-pr'].steps.find( + (s) => s.name === 'Supersede stale docs-only badge', + ).run; + + function runSupersede({ scenario }) { + const dir = mkdtempSync(join(tmpdir(), 'docs-supersede-')); + try { + const bin = join(dir, 'bin'); + mkdirSync(bin); + const calls = join(dir, 'calls'); + writeFileSync(calls, ''); + const write = (name, body) => { + writeFileSync(join(bin, name), body); + chmodSync(join(bin, name), 0o755); + }; + write('sleep', '#!/bin/bash\nexit 0\n'); + write( + 'gh', + [ + '#!/bin/bash', + 'echo "$*" >> "$CALLS"', + 'if [ "$SCENARIO" = "all-fail" ]; then exit 1; fi', + 'case "$*" in', + ' "api user"*) echo bot ;;', + ' *"--method GET"*)', + ' echo \'[{"id":31,"user":{"login":"bot"},"body":" badge"}]\' ;;', + ' *) : ;;', + 'esac', + 'exit 0', + ].join('\n') + '\n', + ); + const script = [ + 'set -euo pipefail', + 'GITHUB_REPOSITORY=o/r', + 'PR_NUMBER=42', + `RUNNER_TEMP="${dir}"`, + supersedeRun, + 'echo "STEP_EXIT_OK"', + ].join('\n'); + const stdout = execFileSync('bash', ['-c', script], { + encoding: 'utf8', + cwd: process.cwd(), + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + SCENARIO: scenario, + CALLS: calls, + }, + }); + return { stdout, calls: readFileSync(calls, 'utf8') }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + + it('supersedes an existing bot-authored badge (PATCH, update-only)', () => { + const r = runSupersede({ scenario: 'existing' }); + expect(r.calls).toContain( + 'api --method PATCH repos/o/r/issues/comments/31', + ); + expect(r.stdout).toContain('STEP_EXIT_OK'); + }); + + it('warns and exits 0 when every supersede attempt fails', () => { + // The never-fail guard is load-bearing: a failing step here would trip + // the post-failure fallback into announcing a review failure that never + // happened (the same phantom the relay guard prevents). + const r = runSupersede({ scenario: 'all-fail' }); + expect(r.stdout).toContain('::warning::Could not supersede'); + expect(r.stdout).toContain('STEP_EXIT_OK'); + }); + + function ciRcFragment() { + const ci = readFileSync('.github/workflows/ci.yml', 'utf8'); + const ciDoc = parse(ci); + let run; + for (const job of Object.values(ciDoc.jobs)) { + for (const step of job.steps ?? []) { + if ((step.run ?? '').includes('classify-pr-profile.sh')) run = step.run; + } + } + expect(run).toBeTruthy(); + const start = run.indexOf('set +e'); + expect(start).toBeGreaterThan(-1); + const indent = run.slice(run.lastIndexOf('\n', start) + 1, start); + const end = run.indexOf(`\n${indent}fi`, start) + `\n${indent}fi`.length; + expect(end).toBeGreaterThan(start); + return run.slice(start, end); + } + + function runCiFragment(wrapper) { + const dir = mkdtempSync(join(tmpdir(), 'ci-rc-')); + try { + const stub = join(dir, '.github/scripts/ci'); + mkdirSync(stub, { recursive: true }); + writeFileSync(join(stub, 'classify-pr-profile.sh'), wrapper); + chmodSync(join(stub, 'classify-pr-profile.sh'), 0o755); + const script = [ + 'set -euo pipefail', + 'profile=full', + 'GITHUB_REPOSITORY=o/r', + 'PR_NUMBER=42', + ciRcFragment(), + 'printf "profile=%s" "$profile"', + ].join('\n'); + return execFileSync('bash', ['-c', script], { + encoding: 'utf8', + cwd: dir, + }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + + it('ci.yml consumes the wrapper result on success', () => { + expect(runCiFragment('#!/bin/bash\necho docs_only\n')).toContain( + 'profile=docs_only', + ); + }); + + it.each([ + ['#!/bin/bash\nexit 2\n', 'Unable to list PR changed files'], + ['#!/bin/bash\nexit 3\n', 'classifier exited non-zero'], + ])('ci.yml falls back to full on wrapper failure (%#)', (wrapper, note) => { + // The probed mutant (deleting the rc handling) leaves profile EMPTY on + // failure — no downstream matrix bucket matches empty, and a broken PR + // would pass CI with zero tests run. + const out = runCiFragment(wrapper); + expect(out).toContain(note); + expect(out).toContain('profile=full'); + }); +}); From 6196fd976ec99480789936be97202c9afd1ef91e Mon Sep 17 00:00:00 2001 From: verify Date: Fri, 7 Aug 2026 16:12:27 +0000 Subject: [PATCH 7/7] perf(ci): bind the docs badge to the reviewed head and retire it on failure MDX pages are executable (imported components, expressions), so the classifier no longer treats them as inert docs-only changes. The relay and supersede writes re-read the live PR state/head immediately before the mutation and skip unless the PR is still open at the reviewed SHA, the badge body names that SHA, a failed docs-only review now retires the singleton badge instead of leaving the previous revision's outcome visible, and the retired wording is cause-neutral (an explicit review can complete on the very head the badge describes). Co-authored-by: Qwen-Coder --- .github/scripts/ci/classify-profile.mjs | 9 +- .github/scripts/ci/classify-profile.test.mjs | 11 +- .github/workflows/qwen-code-pr-review.yml | 113 ++++++++++--- scripts/tests/qwen-pr-review-workflow.test.js | 149 +++++++++++++++++- 4 files changed, 252 insertions(+), 30 deletions(-) diff --git a/.github/scripts/ci/classify-profile.mjs b/.github/scripts/ci/classify-profile.mjs index bdccc000f1b..12890755d21 100644 --- a/.github/scripts/ci/classify-profile.mjs +++ b/.github/scripts/ci/classify-profile.mjs @@ -16,12 +16,15 @@ export const GITHUB_CI_ONLY_FILES = new Set([ function isDocsOnlyFile(file) { const normalized = file.replace(/\\/g, '/'); return ( - /^docs\/.+\.(?:md|mdx)$/i.test(normalized) || + // .md ONLY: MDX pages are executable (imported components, expressions) + // and keep the full profile with its runtime/build failure surface. + /^docs\/.+\.md$/i.test(normalized) || // Extensionless or known-inert documentation extensions ONLY: the open // `(?:\.[^/]*)?` form classified executable files named after reserved // prose basenames (README.js, SECURITY.ts, LICENSE.sh) as docs, which - // would downgrade an automatic review over runnable code. - /^(?:README|CHANGELOG|CONTRIBUTING|CODE_OF_CONDUCT|SECURITY|SUPPORT|LICENSE|NOTICE)(?:\.(?:md|mdx|txt|rst))?$/i.test( + // would downgrade an automatic review over runnable code. MDX is + // excluded here for the same reason it is above. + /^(?:README|CHANGELOG|CONTRIBUTING|CODE_OF_CONDUCT|SECURITY|SUPPORT|LICENSE|NOTICE)(?:\.(?:md|txt|rst))?$/i.test( normalized, ) ); diff --git a/.github/scripts/ci/classify-profile.test.mjs b/.github/scripts/ci/classify-profile.test.mjs index c2626f99548..2260919ba28 100644 --- a/.github/scripts/ci/classify-profile.test.mjs +++ b/.github/scripts/ci/classify-profile.test.mjs @@ -19,11 +19,20 @@ test('uses docs_only for markdown-only changes', () => { test('uses docs_only for uppercase and extensionless docs', () => { assert.equal( - classifyChangedFiles(['README.MD', 'docs/guide.MDX', 'LICENSE', 'README']), + classifyChangedFiles(['README.MD', 'docs/guide.MD', 'LICENSE', 'README']), 'docs_only', ); }); +test('MDX is executable content, never docs_only', () => { + // MDX pages can import components and carry expressions — a runtime/build + // failure surface the docs-only downgrade must not skip over. + assert.equal(classifyChangedFiles(['docs/guide.mdx']), 'full'); + assert.equal(classifyChangedFiles(['docs/guide.MDX']), 'full'); + assert.equal(classifyChangedFiles(['README.mdx']), 'full'); + assert.equal(classifyChangedFiles(['docs/usage.md', 'docs/guide.mdx']), 'full'); +}); + test('falls back to full for root docs names used as directories', () => { assert.equal(classifyChangedFiles(['README.md/evil.ts']), 'full'); assert.equal(classifyChangedFiles(['LICENSE.txt/src/index.ts']), 'full'); diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index de349ed4a4e..625edb9d047 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -966,12 +966,14 @@ jobs: # Docs-only automatic reviews run at medium effort. The classifier is # the same one the Test workflow's CI-profile gate already trusts # (.github/scripts/ci/classify-profile.mjs) and it is conservative by - # construction: only docs/**.md(x) and root-level prose files classify - # docs_only — markdown under any src/ tree stays `full`, matching the - # review skill's own "markdown inside a source tree counts as source" - # rule — and any fetch or classifier failure falls back to the full - # review. On a diff with zero source lines, the passes medium drops - # (adversarial personas, reverse audit) have no failure mode to hunt; + # construction: only docs/**.md and root-level prose files classify + # docs_only — MDX is executable (imported components, expressions) + # and stays `full`, as does markdown under any src/ tree, matching + # the review skill's own "markdown inside a source tree counts as + # source" rule — and any fetch or classifier failure falls back to + # the full review. On a diff with zero source lines, the passes + # medium drops (adversarial personas, reverse audit) have no + # failure mode to hunt; # medium keeps the verified finder fan-out. `--comment` is dropped # with the downgrade — an effective --comment forces high # (parse-args), and medium never posts — so the "Report docs-only @@ -1262,10 +1264,29 @@ jobs: env: GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' PR_NUMBER: '${{ steps.context.outputs.pr_number }}' + EXPECTED_HEAD_SHA: '${{ steps.review.outputs.expected_head_sha }}' COMPLETION_LINE: "${{ steps.review.outputs.completion_line || '(no relayable \"Review complete:\" line in the run output — see the run log)' }}" RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' run: |- set -euo pipefail + # The write is bound to the reviewed head: re-read the live PR + # state immediately before the mutation. The stale-head guard in + # "Run review" checked at step start; a push that landed since + # must not receive this head's outcome (the fallback-comment step + # uses the same guard shape). + if ! PR_DATA="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')"; then + echo "::warning::Could not verify PR #${PR_NUMBER} before relaying the docs-only outcome; skipping the relay." + exit 0 + fi + IFS=$'\t' read -r PR_STATE CURRENT_HEAD_SHA <<< "$PR_DATA" + if [ "$PR_STATE" != "OPEN" ]; then + echo "Skipping docs-only relay: PR #${PR_NUMBER} is ${PR_STATE}." + exit 0 + fi + if [ "$CURRENT_HEAD_SHA" != "$EXPECTED_HEAD_SHA" ]; then + echo "Skipping docs-only relay: PR #${PR_NUMBER} moved from ${EXPECTED_HEAD_SHA} to ${CURRENT_HEAD_SHA}." + exit 0 + fi # Medium never posts inline comments and --comment would force high # effort back on (parse-args), so a docs-only automatic review # reports through this single relay instead. The quoted line arrives @@ -1281,11 +1302,11 @@ jobs: '' \ "> ${COMPLETION_LINE}" \ '' \ - "Full report in the [workflow run](${RUN_URL}). For a full high-effort review with inline comments, comment \`@qwen-code /review\`." \ + "Reviewed head: \`${EXPECTED_HEAD_SHA}\`. Full report in the [workflow run](${RUN_URL}). For a full high-effort review with inline comments, comment \`@qwen-code /review\`." \ '' \ '
中文说明' \ '' \ - "📄 **纯文档变更** —— 自动评审以 \`--effort medium\` 运行（发现已验证、无反向审计；medium 不发布行内评论），结果见上方引用行。完整报告见 [workflow 运行](${RUN_URL})；如需带行内评论的完整高强度（high-effort）评审，请评论 \`@qwen-code /review\`。" \ + "📄 **纯文档变更** —— 自动评审以 \`--effort medium\` 运行（发现已验证、无反向审计；medium 不发布行内评论），结果见上方引用行。评审的 head：\`${EXPECTED_HEAD_SHA}\`。完整报告见 [workflow 运行](${RUN_URL})；如需带行内评论的完整高强度（high-effort）评审，请评论 \`@qwen-code /review\`。" \ '' \ '
')" # The shared upsert protocol (.github/scripts/upsert-bot-comment.sh) @@ -1308,7 +1329,7 @@ jobs: fi - name: 'Supersede stale docs-only badge' - # Two paths owe the badge correction, and only these two: + # Three paths owe the badge correction, and only these three: # (1) an automatic run whose classification POSITIVELY determined the # PR is not (or no longer) docs-only — docs_only_medium == 'false' # is three-valued and empty when the classifier failed or never @@ -1318,8 +1339,13 @@ jobs: # (2) an explicit comment-mode review that actually completed — the # badge's own CTA path, whose posted full review makes the badge # redundant; a dispatch dry-run that posts nothing retires - # nothing. The body is cause-neutral: it asserts only what is - # true on every covered path. + # nothing; + # (3) a FAILED automatic docs-only review — the relay only runs on + # success, so without this path the badge would keep quoting the + # previous revision's outcome for a head whose own run died. + # The retired body is cause-neutral: it asserts only what is true on + # every covered path (an explicit review can complete on the very + # same SHA the badge describes). if: |- !cancelled() && steps.context.outputs.should_run == 'true' && @@ -1330,29 +1356,74 @@ jobs: steps.context.outputs.auto_review == 'false' && steps.context.outputs.review_mode == 'comment' && steps.review.outputs.review_completed == 'true' + ) || + ( + failure() && + steps.review.outputs.docs_only_medium == 'true' ) ) env: GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' PR_NUMBER: '${{ steps.context.outputs.pr_number }}' + EXPECTED_HEAD_SHA: "${{ steps.review.outputs.expected_head_sha || '' }}" + DOCS_ONLY_MEDIUM: '${{ steps.review.outputs.docs_only_medium }}' + REVIEW_COMPLETED: '${{ steps.review.outputs.review_completed }}' + RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' run: |- set -euo pipefail + # The write is bound to the reviewed head: re-read the live PR + # state immediately before the mutation (the same guard shape as + # the fallback-comment step). A run that failed before "Run + # review" emitted the reviewed SHA has nothing to bind to — a + # badge is never updated on ignorance. + if [ -z "$EXPECTED_HEAD_SHA" ]; then + echo "Skipping badge update: the reviewed head SHA is unknown." + exit 0 + fi + if ! PR_DATA="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')"; then + echo "::warning::Could not verify PR #${PR_NUMBER} before updating the docs-only badge." + exit 0 + fi + IFS=$'\t' read -r PR_STATE CURRENT_HEAD_SHA <<< "$PR_DATA" + if [ "$PR_STATE" != "OPEN" ]; then + echo "Skipping badge update: PR #${PR_NUMBER} is ${PR_STATE}." + exit 0 + fi + if [ "$CURRENT_HEAD_SHA" != "$EXPECTED_HEAD_SHA" ]; then + echo "Skipping badge update: PR #${PR_NUMBER} moved from ${EXPECTED_HEAD_SHA} to ${CURRENT_HEAD_SHA}." + exit 0 + fi # --update-only makes this a strict no-op on PRs that never # carried the badge; a failed lookup exits 1 (never the no-op), so # the warning below fires instead of silently keeping a stale # badge. The marker is defined once and used for both the body and # the lookup — they must be byte-identical. MARKER='' - BODY="$(printf '%s\n' \ - "$MARKER" \ - '' \ - '📄 ~~Docs-only change~~ **(superseded)** — this badge described an earlier docs-only revision of this PR and no longer reflects the current head. See the latest review activity on this PR.' \ - '' \ - '
中文说明' \ - '' \ - '📄 ~~纯文档变更~~ **（已失效）** —— 该徽章描述的是本 PR 更早的纯文档修订，已不再反映当前 head。请以本 PR 上最新的评审动态为准。' \ - '' \ - '
')" + if [ "$DOCS_ONLY_MEDIUM" = "true" ] && [ "$REVIEW_COMPLETED" != "true" ]; then + # A failed docs-only run: the singleton badge must not keep + # quoting the previous revision's success for this head. + BODY="$(printf '%s\n' \ + "$MARKER" \ + '' \ + "📄 **Docs-only change** — the automatic \`--effort medium\` review of head \`${EXPECTED_HEAD_SHA}\` **did not complete**, so no outcome currently applies. See the failure comment on this PR and the [workflow run](${RUN_URL})." \ + '' \ + '
中文说明' \ + '' \ + "📄 **纯文档变更** —— head \`${EXPECTED_HEAD_SHA}\` 的自动 \`--effort medium\` 评审**未能完成**，当前没有有效的评审结果。详见本 PR 上的失败评论与 [workflow 运行](${RUN_URL})。" \ + '' \ + '
')" + else + BODY="$(printf '%s\n' \ + "$MARKER" \ + '' \ + '📄 ~~Docs-only change~~ **(superseded)** — this badge no longer reflects the current review state of this PR. See the latest review activity on this PR.' \ + '' \ + '
中文说明' \ + '' \ + '📄 ~~纯文档变更~~ **（已失效）** —— 该徽章已不再反映本 PR 当前的评审状态。请以本 PR 上最新的评审动态为准。' \ + '' \ + '
')" + fi printf '%s' "$BODY" > "${RUNNER_TEMP}/docs-only-supersede-body.md" .github/scripts/upsert-bot-comment.sh \ "${GITHUB_REPOSITORY}" "${PR_NUMBER}" \ diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 71b93aba865..9d0e990cb79 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1415,6 +1415,13 @@ describe('docs-only medium gate', () => { const context = doc.jobs['review-pr'].steps.find((s) => s.id === 'context'); // One assignment site, guarded on both the event and the action. expect(context.run.match(/AUTO_REVIEW=true/g)).toHaveLength(1); + // The false DEFAULT is load-bearing too: without it, dispatch, + // issue-comment and review-comment triggers inherit whatever the + // environment carries and can enter the automatic downgrade path. + expect(context.run.match(/AUTO_REVIEW=false/g)).toHaveLength(1); + expect(context.run.indexOf('AUTO_REVIEW=false')).toBeLessThan( + context.run.indexOf('AUTO_REVIEW=true'), + ); // Both halves of the guard: the event must be pull_request_target AND the // action must not be review_requested. Pinning only the action half let a // deleted event condition survive — the branch is shared with @@ -1435,15 +1442,20 @@ describe('docs-only medium gate', () => { expect(m).not.toBeNull(); // Filter side: every autofix exclusion of that marker must carry the // author scope ($rb) — a human quoting the marker stays actionable — - // and all six sites (definition + five inline copies) must be present. + // and all six inline copies in qwen-autofix.yml must be present. const autofix = readFileSync('.github/workflows/qwen-autofix.yml', 'utf8'); const scoped = autofix.match( - /\(\.user\.login \/\/ ""\) == \$rb\)\) and \(\(\.body \/\/ ""\) \| test\("'); + // The badge is bound to the reviewed head: a later push must never be + // described by an earlier revision's outcome. + expect(r.calls).toContain('Reviewed head: `abc123`'); }); it('PATCHes the existing bot-authored relay comment', () => { @@ -1634,6 +1660,56 @@ describe('docs-only gate and relay, executed', () => { expect(r.stdout).toContain('the review itself succeeded'); }); + it('skips the relay when the head moved before the write', () => { + const r = runRelay({ scenario: 'moved-head' }); + expect(r.stdout).toContain('moved from abc123 to deadbeef'); + expect(r.calls).not.toContain('api repos/o/r/issues/42/comments'); + }); + + it('skips the relay when the PR closed before the write', () => { + const r = runRelay({ scenario: 'closed-pr' }); + expect(r.stdout).toContain('is MERGED'); + expect(r.calls).not.toContain('api repos/o/r/issues/42/comments'); + }); + + function normalizedIf(step) { + return step.if.replace(/\s+/g, ' ').trim(); + } + + it('pins the relay if: as the exact reviewed conjunction', () => { + // Full-string pin, not substrings: deleting or weakening any conjunct — + // or re-grouping them — edits this string, so every truth-table mutant + // reduces to a red test here without an Actions-expression evaluator. + const doc2 = parse(workflow); + const relay = doc2.jobs['review-pr'].steps.find( + (s) => s.name === 'Report docs-only medium outcome', + ); + expect(normalizedIf(relay)).toBe( + "steps.context.outputs.should_run == 'true' && " + + "steps.review.outcome == 'success' && " + + "steps.review.outputs.review_completed == 'true' && " + + "steps.review.outputs.docs_only_medium == 'true' && " + + "steps.context.outputs.pr_number != ''", + ); + }); + + it('pins the supersede if: including the OR grouping of its three paths', () => { + const doc2 = parse(workflow); + const supersede = doc2.jobs['review-pr'].steps.find( + (s) => s.name === 'Supersede stale docs-only badge', + ); + expect(normalizedIf(supersede)).toBe( + '!cancelled() && ' + + "steps.context.outputs.should_run == 'true' && " + + "steps.context.outputs.pr_number != '' && " + + "( steps.review.outputs.docs_only_medium == 'false' || " + + "( steps.context.outputs.auto_review == 'false' && " + + "steps.context.outputs.review_mode == 'comment' && " + + "steps.review.outputs.review_completed == 'true' ) || " + + "( failure() && steps.review.outputs.docs_only_medium == 'true' ) )", + ); + }); + it('pins the review_completed wiring end to end', () => { // The state/head guards exit 0 without running the review; the relay // must require the dedicated output, and the run step must emit it @@ -1719,7 +1795,12 @@ describe('supersede step and ci.yml rc-handling, executed', () => { (s) => s.name === 'Supersede stale docs-only badge', ).run; - function runSupersede({ scenario }) { + function runSupersede({ + scenario, + docsOnlyMedium = 'false', + reviewCompleted = 'true', + expectedHeadSha = 'abc123', + }) { const dir = mkdtempSync(join(tmpdir(), 'docs-supersede-')); try { const bin = join(dir, 'bin'); @@ -1736,6 +1817,16 @@ describe('supersede step and ci.yml rc-handling, executed', () => { [ '#!/bin/bash', 'echo "$*" >> "$CALLS"', + // The head-binding guard runs BEFORE the upsert attempts: pr view + // succeeds even in all-fail so that scenario still exercises the + // retry loop. + 'case "$*" in', + ' "pr view"*)', + ' if [ "$SCENARIO" = "moved-head" ]; then printf "OPEN\\tdeadbeef\\n";', + ' elif [ "$SCENARIO" = "closed-pr" ]; then printf "MERGED\\tabc123\\n";', + ' else printf "OPEN\\tabc123\\n"; fi', + ' exit 0 ;;', + 'esac', 'if [ "$SCENARIO" = "all-fail" ]; then exit 1; fi', 'case "$*" in', ' "api user"*) echo bot ;;', @@ -1751,6 +1842,10 @@ describe('supersede step and ci.yml rc-handling, executed', () => { 'GITHUB_REPOSITORY=o/r', 'PR_NUMBER=42', `RUNNER_TEMP="${dir}"`, + `EXPECTED_HEAD_SHA=${expectedHeadSha}`, + `DOCS_ONLY_MEDIUM=${docsOnlyMedium}`, + `REVIEW_COMPLETED=${reviewCompleted}`, + 'RUN_URL=https://x', supersedeRun, 'echo "STEP_EXIT_OK"', ].join('\n'); @@ -1776,6 +1871,50 @@ describe('supersede step and ci.yml rc-handling, executed', () => { 'api --method PATCH repos/o/r/issues/comments/31', ); expect(r.stdout).toContain('STEP_EXIT_OK'); + // Cause-neutral retired wording: it must hold even when an explicit full + // review completes on the SAME head the badge describes, so it may not + // claim the badge described an earlier revision. + expect(r.calls).toContain('(superseded)'); + expect(r.calls).toContain( + 'no longer reflects the current review state of this PR', + ); + expect(r.calls).not.toContain('earlier docs-only revision'); + }); + + it('updates the badge to a failure notice when a docs-only review failed', () => { + // The relay only runs on success; without this path the badge would keep + // quoting the previous revision's outcome for a head whose own run died. + const r = runSupersede({ + scenario: 'existing', + docsOnlyMedium: 'true', + reviewCompleted: '', + }); + expect(r.calls).toContain( + 'api --method PATCH repos/o/r/issues/comments/31', + ); + expect(r.calls).toContain('did not complete'); + expect(r.calls).toContain('abc123'); + expect(r.stdout).toContain('STEP_EXIT_OK'); + }); + + it('skips the badge update when the head moved before the write', () => { + const r = runSupersede({ scenario: 'moved-head' }); + expect(r.stdout).toContain('moved from abc123 to deadbeef'); + expect(r.calls).not.toContain('--method PATCH'); + }); + + it('skips the badge update when the PR closed before the write', () => { + const r = runSupersede({ scenario: 'closed-pr' }); + expect(r.stdout).toContain('is MERGED'); + expect(r.calls).not.toContain('--method PATCH'); + }); + + it('skips the badge update when the reviewed head SHA is unknown', () => { + // A run that failed before "Run review" emitted the SHA has nothing to + // bind to — a badge is never updated on ignorance. + const r = runSupersede({ scenario: 'existing', expectedHeadSha: '' }); + expect(r.stdout).toContain('reviewed head SHA is unknown'); + expect(r.calls).not.toContain('--method PATCH'); }); it('warns and exits 0 when every supersede attempt fails', () => {