Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 39 additions & 5 deletions .github/workflows/qwen-code-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:-}" ] \
Comment thread
wenshao marked this conversation as resolved.
&& [ "$PR_SIZE_LINES" -lt 25 ]; then
Comment thread
wenshao marked this conversation as resolved.
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
Expand Down Expand Up @@ -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
Expand Down
133 changes: 124 additions & 9 deletions scripts/tests/qwen-pr-review-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand Down Expand Up @@ -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');
Expand All @@ -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"',
Expand All @@ -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',
Expand Down
Loading