Repository navigation
perf(ci): run docs-only automatic reviews at medium effort #8648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
3537b25
perf(ci): run docs-only automatic reviews at medium effort
f178af5
perf(ci): address review feedback on the docs-only medium gate
119845d
perf(ci): harden the docs-only gate against round-2 review findings
f87dec5
perf(ci): fix the medium Request-changes swallow and the stale docs b…
2c648b1
perf(ci): never let a failed lookup mint or keep a stale docs badge
e3a20ec
perf(ci): make docs_only_medium three-valued and pin the untested guards
6196fd9
perf(ci): bind the docs badge to the reviewed head and retire it on f…
77a1cd0
chore(ci): merge origin/main into docs-only medium-effort review branch
qwen-code-dev-bot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| #!/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 <owner/repo> <pr-number> | ||
| # 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 <owner/repo> <pr-number>}" | ||
| pr="${2:?usage: classify-pr-profile.sh <owner/repo> <pr-number>}" | ||
|
|
||
| # 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="$(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}")" | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
| if [ "${retrieved}" -ne "${declared}" ]; then | ||
| echo "full" | ||
| exit 0 | ||
| fi | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
|
|
||
| node "$(dirname "$0")/classify-profile.mjs" "${files}" || exit 3 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| /** | ||
| * @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 | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
| // 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 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) 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', | ||
| ' printf \'%s\' "$FIXTURE" | jq -c "$jqfilter" ;;', | ||
| ' *"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); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <owner/repo> <issue-number> <marker> <body-file> [--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 <owner/repo> <issue-number> <marker> <body-file> [--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 | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
| 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 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 = '<!-- test-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":"<!-- test-marker --> old"}]\' ;;', | ||
| ' existing-user) echo \'[{"id":8,"user":{"login":"alice"},"body":"<!-- test-marker --> mine"}]\' ;;', | ||
| ' deleted-mid-retry)', | ||
| ' if [ "$n" -le 1 ]; then echo \'[{"id":9,"user":{"login":"bot"},"body":"<!-- test-marker --> 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/); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.