diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 4d9f0e94f30..0370d0024a8 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1058,6 +1058,17 @@ jobs: # would retire a still-accurate badge on a transient classifier # failure or a dry run. DOCS_ONLY_MEDIUM="" + # One implementation for both downgrades' arithmetic: the docs-only + # branch and the micro tightening below both halve with the same + # 90-minute floor, and two verbatim copies once let a one-sided + # divisor edit diverge them silently while the comments still + # claimed they matched. + halve_budget_floor() { + EFFECTIVE_TIMEOUT_MINUTES=$(( EFFECTIVE_TIMEOUT_MINUTES / 2 )) + if [ "$EFFECTIVE_TIMEOUT_MINUTES" -lt 90 ]; then + EFFECTIVE_TIMEOUT_MINUTES=90 + fi + } 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 @@ -1073,15 +1084,38 @@ jobs: 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 + halve_budget_floor 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 + # A micro automatic review — total churn (additions + deletions) + # strictly below 25 — keeps its full high-effort posting run: a + # medium downgrade would drop the inline comments a source fix + # deserves. The threshold is an INDEPENDENT "this is a small PR" + # bound, deliberately NOT the skill's SWEEP_FLOOR: the two count + # different things (this gate counts churn; the skill weighs raw + # unified-diff lines — file/hunk headers and context included), so + # a scattered micro diff can still run the sweep and the full + # reverse-audit loop. That is fine — the justification is not "the + # pipeline shrank" but "churn < 25 bounds the reviewed territory, + # and 90 minutes is ample for that territory even on the full + # pipeline" (measured: a 23-line PR runs high end to end in ~30 + # min). What a micro run must not keep is the full 180-minute + # small-PR budget when its measured worst case is ~30 minutes: + # the same halve-with-floor the docs downgrade uses, so a hung + # run dies at the scale of its work. + # Skipped when the size lookup failed (an unknown size must not + # tighten anything), on explicit runs (their timeout is the + # caller's), and for docs-only runs, which are already halved. + if [ "${AUTO_REVIEW:-false}" = "true" ] \ + && [ "$DOCS_ONLY_MEDIUM" != "true" ] \ + && [ -n "${PR_SIZE_LINES:-}" ] \ + && [ "$PR_SIZE_LINES" -lt 25 ]; then + halve_budget_floor + echo "PR #${PR_NUMBER} is a micro diff (${PR_SIZE_LINES} changed lines); the automatic review keeps --effort high and inline posting, with a tightened ${EFFECTIVE_TIMEOUT_MINUTES}-minute budget." + 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 @@ -1364,7 +1398,7 @@ jobs: 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)' }}" + 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 diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 6deb3b64303..db4e46297dc 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -1435,15 +1435,15 @@ describe('docs-only medium gate', () => { }); 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; + // The arithmetic lives in ONE function shared by the docs-only branch + // and the micro tightening; extract the definition plus one call, so + // these cases execute the same implementation both branches run. + const start = run.indexOf('halve_budget_floor() {'); + const endAnchor = '\n}'; + const end = run.indexOf(endAnchor, start) + endAnchor.length; expect(start).toBeGreaterThan(-1); expect(end).toBeGreaterThan(start); - return run.slice(start, end); + return `${run.slice(start, end)}\nhalve_budget_floor`; } it.each([ @@ -1645,7 +1645,7 @@ describe('docs-only gate and relay, executed', () => { return runStep.slice(start, end); } - function runGate({ autoReview, wrapper }) { + function runGate({ autoReview, wrapper, prSizeLines, timeoutMinutes }) { const dir = mkdtempSync(join(tmpdir(), 'docs-gate-')); try { const stub = join(dir, '.github/scripts/ci'); @@ -1659,7 +1659,8 @@ describe('docs-only gate and relay, executed', () => { `AUTO_REVIEW=${autoReview}`, 'REPO=o/r', 'PR_NUMBER=42', - 'EFFECTIVE_TIMEOUT_MINUTES=360', + `EFFECTIVE_TIMEOUT_MINUTES=${timeoutMinutes ?? 360}`, + ...(prSizeLines === undefined ? [] : [`PR_SIZE_LINES=${prSizeLines}`]), `GITHUB_OUTPUT="${gho}"`, gateSource(), 'printf "timeout=%s" "$EFFECTIVE_TIMEOUT_MINUTES"', @@ -1683,6 +1684,120 @@ describe('docs-only gate and relay, executed', () => { expect(r.stdout).toContain('timeout=180'); }); + it("tightens a micro diff's budget without touching its effort or posting", () => { + // Below the independent churn bound (25 changed lines — NOT the skill's + // SWEEP_FLOOR, which weighs source/unified-diff lines) the automatic run + // keeps --effort high and its inline comments — only the kill switch + // halves, to the same 90-minute floor the docs downgrade uses. + const r = runGate({ + autoReview: 'true', + wrapper: '#!/bin/bash\necho full\n', + prSizeLines: 24, + timeoutMinutes: 180, + }); + expect(r.output).toBe('docs_only_medium=false'); + expect(r.stdout).toContain('micro diff (24 changed lines)'); + expect(r.stdout).toContain('keeps --effort high'); + expect(r.stdout).toContain('timeout=90'); + }); + + it('micro tightening floors at 90 minutes', () => { + const r = runGate({ + autoReview: 'true', + wrapper: '#!/bin/bash\necho full\n', + prSizeLines: 10, + timeoutMinutes: 100, + }); + expect(r.stdout).toContain('timeout=90'); + }); + + it('twenty-five changed lines is not micro — the boundary of the churn bound', () => { + // The 25 is an independent "small PR" churn bound (NOT the skill's + // SWEEP_FLOOR — the two measures differ, so a scattered micro diff may + // still run the sweep); the tightening is justified by "90 min is ample + // for churn < 25 work", not by the pipeline shrinking. 25 itself must + // not tighten. + const r = runGate({ + autoReview: 'true', + wrapper: '#!/bin/bash\necho full\n', + prSizeLines: 25, + timeoutMinutes: 180, + }); + expect(r.stdout).not.toContain('micro diff'); + expect(r.stdout).toContain('timeout=180'); + }); + + it('both downgrades share one halve-with-floor implementation', () => { + // Two verbatim copies once let a one-sided divisor edit diverge micro + // runs from docs-only runs while the comments claimed they matched — + // probe: a / 2 → / 3 mutant survived every test because both micro + // inputs land on the floor under any divisor ≥ 2. One named function, + // called from both branches, makes the invariant structural. + const gate = gateSource(); + expect(gate.match(/halve_budget_floor\(\)/g)).toHaveLength(1); + expect(gate.match(/halve_budget_floor$/gm)).toHaveLength(2); + expect( + gate.match( + /EFFECTIVE_TIMEOUT_MINUTES=\$\(\( EFFECTIVE_TIMEOUT_MINUTES \/ 2 \)\)/g, + ), + ).toHaveLength(1); + }); + + it('a docs-only micro diff is halved once, by the docs gate, not twice', () => { + const r = runGate({ + autoReview: 'true', + wrapper: '#!/bin/bash\necho docs_only\n', + prSizeLines: 10, + timeoutMinutes: 360, + }); + expect(r.output).toBe('docs_only_medium=true'); + expect(r.stdout).toContain('timeout=180'); + expect(r.stdout).not.toContain('micro diff'); + }); + + it('a manually requested review is never tightened, whatever its size', () => { + // Production-reachable: an @qwen-code /review comment without --timeout + // populates PR_SIZE_LINES but is not an automatic review — its budget + // is the caller's. A mutant dropping the AUTO_REVIEW guard survived the + // suite until this pin. + const r = runGate({ + autoReview: 'false', + wrapper: '#!/bin/bash\necho full\n', + prSizeLines: 10, + timeoutMinutes: 180, + }); + expect(r.stdout).not.toContain('micro diff'); + expect(r.stdout).toContain('timeout=180'); + }); + + it('a failed docs classification still tightens a micro automatic run', () => { + // DOCS_ONLY_MEDIUM stays '' when the classifier fails; the micro guard + // keys on != "true", not = "false" — a mutant conflating the two kept + // 180 on exactly the runs the tightening exists for. + const r = runGate({ + autoReview: 'true', + wrapper: '#!/bin/bash\nexit 2\n', + prSizeLines: 10, + timeoutMinutes: 180, + }); + expect(r.output).toBe('docs_only_medium='); + expect(r.stdout).toContain('micro diff (10 changed lines)'); + expect(r.stdout).toContain('timeout=90'); + }); + + it('an unknown size never tightens — and neither does an explicit run', () => { + // PR_SIZE_LINES is unset when the size lookup failed or when the caller + // passed --timeout (the size block is skipped); both must keep the + // budget they have. + const r = runGate({ + autoReview: 'true', + wrapper: '#!/bin/bash\necho full\n', + timeoutMinutes: 180, + }); + expect(r.stdout).not.toContain('micro diff'); + expect(r.stdout).toContain('timeout=180'); + }); + it('keeps the full review for a full classification', () => { const r = runGate({ autoReview: 'true',