diff --git a/.github/scripts/review-auto-stop.mjs b/.github/scripts/review-auto-stop.mjs new file mode 100644 index 00000000000..cc2bfbd9331 --- /dev/null +++ b/.github/scripts/review-auto-stop.mjs @@ -0,0 +1,137 @@ +#!/usr/bin/env node +// Should this repository keep auto-triggering reviews on this PR? +// +// The review pipeline measures whether a loop is settling and says so; it +// owns no threshold and stops nothing (see issue #9278's governance rule — +// the tool measures, the caller decides). This is the caller's half: OUR +// number, applied to telemetry the pipeline already publishes, deciding only +// whether the AUTOMATIC trigger keeps firing. +// +// What it never does: +// +// - It never blocks an explicit request. The gate that calls this runs only +// on `opened`/`synchronize`; `@qwen-code /review` and a review_requested +// go around it entirely. Stopping the treadmill is not refusing to review. +// - It never fails closed. Every doubt — telemetry that will not parse, a +// round we cannot see, a posture that changed underneath the numbers — +// continues reviewing. A caller that silences reviews when it cannot read +// its own evidence is worse than one with no rule at all. +// +// The evidence is the ledger marker each posted review carries. Only two +// fields are read (`round`, `fresh`) plus `floor` to reject a comparison +// across a posture change; the reader is deliberately narrow because it is a +// SECOND reader of a format `parseLedger` owns, and a narrow one that fails +// open cannot drift into a wrong decision — only into "keep reviewing". + +/** The default window, and the whole of this module's policy. */ +export const DEFAULT_WINDOW = 3; + +const OPEN = ''; + +/** + * The fields the caller's rule needs, or null when the body carries none. + * + * LAST marker, like `parseLedger`: an edited or quote-carrying body can hold + * more than one, and the newest round describes the current state. + */ +export function readMarker(body) { + if (typeof body !== 'string') return null; + const start = body.lastIndexOf(OPEN); + if (start < 0) return null; + const end = body.indexOf(CLOSE, start); + if (end < 0) return null; + let raw; + try { + raw = JSON.parse(body.slice(start + OPEN.length, end)); + } catch { + return null; + } + if (!raw || raw.v !== 1) return null; + const round = Number.isInteger(raw.round) && raw.round > 0 ? raw.round : null; + if (round === null) return null; + // `fresh` is the count the trend is about — the round's FIRST-TIME + // findings, not its whole output, which only ever rises while an unfixed + // blocker keeps being re-posted. + const fresh = Number.isInteger(raw.fresh) && raw.fresh >= 0 ? raw.fresh : null; + const floor = raw.floor === 'c' || raw.floor === 'o' ? raw.floor : null; + return { round, fresh, floor }; +} + +/** + * Keep auto-triggering, or stop? + * + * `bodies` is every review body this account posted on the PR, newest first. + * Returns `{ stop, reason, evidence }` — `reason` is rendered to the operator + * and to the PR, so it states the measurement, never a verdict about the work. + * + * `evidence.rounds` is present on BOTH answers, oldest first, and is the + * readable rounds this call actually saw. The caller reads its length to + * decide whether a stale pause notice could exist at all: a PR with no + * readable round has never been paused, because a pause needs `window + 1` + * of them. + */ +export function decideAutoStop(bodies, options = {}) { + const window = Number.isInteger(options.window) && options.window > 0 + ? options.window + : DEFAULT_WINDOW; + const rounds = []; + const evidence = () => ({ window, rounds: rounds.slice().reverse() }); + const cont = (reason) => ({ stop: false, reason, evidence: evidence() }); + + for (const body of Array.isArray(bodies) ? bodies : []) { + const m = readMarker(body); + if (m) rounds.push(m); + if (rounds.length === window + 1) break; + } + + // Not enough published rounds to see a trend of `window` steps. Absence is + // unevaluable, never "diverging". + if (rounds.length < window + 1) { + return cont( + `only ${rounds.length} round(s) carry a readable marker; ${window + 1} are needed to see ${window} step(s)`, + ); + } + // A round whose marker predates the fresh count cannot be compared. + if (rounds.some((r) => r.fresh === null)) { + return cont('a round in the window recorded no first-time count'); + } + // Rounds must be CONSECUTIVE. A gap means a round we cannot see, and a + // trend measured across it is a trend over unknown work. + for (let i = 0; i < rounds.length - 1; i++) { + if (rounds[i].round !== rounds[i + 1].round + 1) { + return cont( + `rounds ${rounds[rounds.length - 1].round}..${rounds[0].round} are not consecutive`, + ); + } + } + // A settled round is the observation the trend exists to find, not a + // symptom — the same reading the pipeline itself refuses to call divergence. + if (rounds[0].fresh === 0) { + return cont('the latest round produced no first-time findings'); + } + // A posture change is not loop behaviour. Unrecorded floors are not a + // change; two DIFFERENT recorded ones are. + const floors = new Set(rounds.map((r) => r.floor).filter((f) => f !== null)); + if (floors.size > 1) { + return cont('the posting floor changed inside the window'); + } + // Every step non-shrinking: the loop has not converged in `window` rounds. + for (let i = 0; i < rounds.length - 1; i++) { + if (rounds[i].fresh < rounds[i + 1].fresh) { + return cont( + `first-time findings fell from ${rounds[i + 1].fresh} to ${rounds[i].fresh} at round ${rounds[i].round}`, + ); + } + } + const counts = rounds + .slice() + .reverse() + .map((r) => `r${r.round}=${r.fresh}`) + .join(' → '); + return { + stop: true, + reason: `first-time findings did not fall across ${window} consecutive round(s): ${counts}`, + evidence: evidence(), + }; +} diff --git a/.github/scripts/review-auto-stop.test.mjs b/.github/scripts/review-auto-stop.test.mjs new file mode 100644 index 00000000000..4ab0578ce1c --- /dev/null +++ b/.github/scripts/review-auto-stop.test.mjs @@ -0,0 +1,467 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { DEFAULT_WINDOW, decideAutoStop, readMarker } from './review-auto-stop.mjs'; + +/** A posted review body carrying a ledger marker. */ +const body = (round, fresh, floor = 'o', extra = {}) => + `prose\n\n`; + +/** + * The shape that DOES stop, newest first: four consecutive rounds whose + * first-time counts never fall. Every test below starts here and changes one + * thing, so each assertion measures the condition it names — an arm that + * would have continued for a second reason proves nothing about the first. + */ +const STOPPING = [body(6, 3), body(5, 3), body(4, 2), body(3, 2)]; + +test('the baseline shape stops the automatic trigger', () => { + const d = decideAutoStop(STOPPING); + assert.equal(d.stop, true); + assert.match(d.reason, /did not fall across 3 consecutive/); + // The reason states the measurement, so an operator can check it. + assert.match(d.reason, /r3=2 → r4=2 → r5=3 → r6=3/); + assert.equal(d.evidence.window, DEFAULT_WINDOW); + // `evidence.rounds` is read in production — the gate takes its length to + // decide whether a stale pause notice could exist — so its contents are + // pinned here, oldest first, rather than left to rot unobserved. + assert.deepEqual(d.evidence.rounds, [ + { round: 3, fresh: 2, floor: 'o' }, + { round: 4, fresh: 2, floor: 'o' }, + { round: 5, fresh: 3, floor: 'o' }, + { round: 6, fresh: 3, floor: 'o' }, + ]); +}); + +test('the readable rounds are reported on a continue answer too', () => { + // The gate reads this length on the CONTINUE path, where it decides + // whether to spend a listing looking for a notice to supersede. + assert.equal(decideAutoStop(STOPPING.slice(0, 2)).evidence.rounds.length, 2); + assert.equal(decideAutoStop(['no marker at all']).evidence.rounds.length, 0); +}); + +test('one falling step is convergence, and keeps the trigger', () => { + const d = decideAutoStop([body(6, 1), ...STOPPING.slice(1)]); + assert.equal(d.stop, false); + assert.match(d.reason, /fell from 3 to 1 at round 6/); +}); + +test('a settled round is the observation, not the symptom', () => { + const d = decideAutoStop([body(6, 0), ...STOPPING.slice(1)]); + assert.equal(d.stop, false); + assert.match(d.reason, /no first-time findings/); +}); + +test('too few published rounds is unevaluable, never diverging', () => { + const d = decideAutoStop(STOPPING.slice(0, 3)); + assert.equal(d.stop, false); + assert.match(d.reason, /only 3 round\(s\)/); +}); + +test('a gap in the rounds is a trend over work nobody can see', () => { + const d = decideAutoStop([body(7, 3), body(5, 3), body(4, 2), body(3, 2)]); + assert.equal(d.stop, false); + assert.match(d.reason, /not consecutive/); +}); + +test('a posture change is not loop behaviour', () => { + const d = decideAutoStop([body(6, 3, 'c'), body(5, 3, 'o'), body(4, 2, 'o'), body(3, 2, 'o')]); + assert.equal(d.stop, false); + assert.match(d.reason, /posting floor changed/); + // Two recorded floors that AGREE are not a change. + assert.equal(decideAutoStop(STOPPING.map((b) => b)).stop, true); + // An unrecorded floor is not a change either — a pre-field marker must not + // silence the rule, and must not be read as a different posture. + const mixedAbsent = [body(6, 3, null), body(5, 3), body(4, 2), body(3, 2)]; + assert.equal(decideAutoStop(mixedAbsent).stop, true); +}); + +test('a round with no first-time count cannot be compared', () => { + const d = decideAutoStop([body(6, null), ...STOPPING.slice(1)]); + assert.equal(d.stop, false); + assert.match(d.reason, /recorded no first-time count/); +}); + +test('the window is the callers number, and only the callers', () => { + // Two steps of flatness stop a caller that tolerates two; the same + // evidence keeps a caller that tolerates four. + assert.equal(decideAutoStop(STOPPING, { window: 2 }).stop, true); + assert.equal(decideAutoStop(STOPPING, { window: 4 }).stop, false); + // A nonsense window falls back to the default rather than being taken + // literally. `stop` alone cannot tell the two apart on this evidence — a + // literal window of 0 stops too, on zero trend steps — so the assertion is + // on the measurement the reason states. + assert.match(decideAutoStop(STOPPING, { window: 0 }).reason, /across 3 consecutive/); + assert.equal(decideAutoStop(STOPPING, { window: 0 }).evidence.window, DEFAULT_WINDOW); + assert.match(decideAutoStop(STOPPING, { window: -1 }).reason, /across 3 consecutive/); +}); + +test('unreadable telemetry keeps reviewing', () => { + // Every doubt continues: a body with no marker, a malformed one, a + // truncated one, and a wrong-version one all read as "cannot evaluate". + for (const bad of [ + 'no marker at all', + '', + '', + '', + ]) { + assert.equal(readMarker(bad), null, bad); + assert.equal(decideAutoStop([bad, ...STOPPING.slice(1)]).stop, false, bad); + } + assert.equal(decideAutoStop(null).stop, false); + assert.equal(decideAutoStop([]).stop, false); +}); + +test('reads the LAST marker in a body, like the pipeline does', () => { + // A quoted older marker must not decide the round: an edited or + // quote-carrying body can hold more than one. + const quoted = `${body(2, 9)}\n\nreply quoting the above\n\n${body(6, 3).split('\n\n')[1]}`; + assert.equal(readMarker(quoted).round, 6); +}); + +test('ignores a marker field it does not need', () => { + const withExtras = body(6, 3, 'o', { posted: 40, dropped: 5, sha: 'deadbeef' }); + assert.deepEqual(readMarker(withExtras), { round: 6, fresh: 3, floor: 'o' }); +}); + + +// --------------------------------------------------------------------------- +// The workflow step itself, replayed. +// +// The unit tests above cover the decision; these cover the wiring around it, +// which is where this feature has been broken every time: `gh api --paginate +// --jq` returns one document PER PAGE, so the listing was unreadable — and +// therefore empty, and therefore inert — on every PR past 100 reviews; the +// notice was posted with a credential that cannot post; and the notice body +// omitted the marker its own upsert looks up by, so each stop minted a +// duplicate. All three are fail-open, all three are invisible to a test that +// stubs the shell out. So this section runs the shipped `run:` block verbatim +// against a stubbed `gh` that paginates the way the real one does and keeps a +// real comment store across runs. +// --------------------------------------------------------------------------- + +import { execFileSync } from 'node:child_process'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const here = dirname(fileURLToPath(import.meta.url)); +const repoRoot = join(here, '..', '..'); +const workflow = readFileSync(join(here, '..', 'workflows', 'qwen-code-pr-review.yml'), 'utf8'); + +/** + * The `run:` body of a named step, dedented — no YAML parser, because the + * profile that runs these tests installs no dependencies. + */ +function stepScript(stepName) { + const lines = workflow.split('\n'); + const start = lines.findIndex((l) => l.trim() === `- name: '${stepName}'`); + assert.notEqual(start, -1, `step not found: ${stepName}`); + const stepIndent = lines[start].indexOf('- '); + let end = lines.length; + for (let i = start + 1; i < lines.length; i++) { + const l = lines[i]; + if (l.trim() === '') continue; + const indent = l.length - l.trimStart().length; + if (indent <= stepIndent && l.trimStart().startsWith('- ')) { end = i; break; } + if (indent < stepIndent) { end = i; break; } + } + const runAt = lines.slice(start, end).findIndex((l) => /^\s*run: \|-\s*$/.test(l)); + assert.notEqual(runAt, -1, `step has no block run:: ${stepName}`); + const runLine = lines[start + runAt]; + const bodyIndent = runLine.length - runLine.trimStart().length + 2; + const body = []; + for (let i = start + runAt + 1; i < end; i++) { + const l = lines[i]; + if (l.trim() === '') { body.push(''); continue; } + assert.ok(l.length - l.trimStart().length >= bodyIndent, `dedent failed at line ${i + 1}`); + body.push(l.slice(bodyIndent)); + } + return body.join('\n'); +} + +const RECHECK = stepScript('Re-check PR state'); +// A silent extraction failure would make every arm below pass on an empty +// script, so the anchors are asserted, not assumed. +for (const anchor of ['set -euo pipefail', 'should_review', 'upsert-bot-comment.sh', 'decideAutoStop']) { + assert.ok(RECHECK.includes(anchor), `extracted step is missing ${anchor}`); +} + +/** + * The account whose reviews are evidence, taken from where the workflow + * publishes it rather than retyped. The gate cannot reach review-config with + * `needs` (that job runs only on review_requested), so the literal is copied + * — and a copy nothing checks is a rename away from selecting zero reviews on + * every PR, forever, fail-open and silent. + */ +const BOT_LOGIN = (() => { + const m = /bot_login=([A-Za-z0-9-]+)/.exec(stepScript('Set review constants')); + assert.ok(m, 'review-config no longer publishes bot_login the same way'); + return m[1]; +})(); + +test('the gate filters on the login review-config publishes', () => { + assert.ok( + RECHECK.includes(`select(.user.login == "${BOT_LOGIN}")`), + `the auto-stop listing must filter on ${BOT_LOGIN}, the review-config constant`, + ); +}); + +/** Chronological, oldest first — the order the API returns reviews in. */ +const CHRONOLOGICAL = [...STOPPING].reverse(); +const review = (b, login = BOT_LOGIN) => ({ user: { login }, body: b }); +/** The two pages a diverging PR's listing actually arrives in. */ +const twoPages = (bodies) => [ + bodies.slice(0, 2).map((b) => review(b)), + bodies.slice(2).map((b) => review(b)), +]; + +/** + * A stub environment that OUTLIVES one run: the comment store is what makes + * the difference between an upsert and a duplicate visible at all. + */ +function makeReplay() { + const dir = mkdtempSync(join(tmpdir(), 'review-auto-stop-step-')); + const bin = join(dir, 'bin'); + mkdirSync(bin); + const calls = join(dir, 'calls'); + const comments = join(dir, 'comments.json'); + const pagesFile = join(dir, 'pages.json'); + writeFileSync(calls, ''); + writeFileSync(comments, '[]'); + const write = (name, body) => { + writeFileSync(join(bin, name), body); + chmodSync(join(bin, name), 0o755); + }; + write('sleep', '#!/usr/bin/env bash\nexit 0\n'); + write( + 'gh', + [ + '#!/usr/bin/env bash', + // Record the credential each call was made with: which token reaches + // which endpoint is one of the defects this file exists to pin. + 'echo "TOKEN=${GH_TOKEN:-} ARGS=$1 $2 $3" >> "$CALLS"', + 'case "$*" in', + ' "pr view"*) printf \'OPEN\\tfalse\\n\' ;;', + ' "api user"*) echo "$BOT" ;;', + // Faithful to gh on the one point that matters: `--jq` is applied PER + // PAGE, and the outputs are concatenated. A stub that ignored the flag + // would fail the multi-page and single-page arms alike, and prove + // neither. + ' *"/pulls/"*"/reviews"*)', + ' if [ -n "${FAIL_LISTING:-}" ]; then echo "HTTP 502" >&2; exit 1; fi', + ' jqexpr=""; prev=""', + ' for a in "$@"; do if [ "$prev" = "--jq" ]; then jqexpr="$a"; fi; prev="$a"; done', + ' if [ -n "$jqexpr" ]; then', + ' while IFS= read -r page; do printf \'%s\' "$page" | jq -c "$jqexpr"; done < "$PAGES"', + ' else', + ' cat "$PAGES"', + ' fi ;;', + // A real comment store, so a second stop run can find the first one. + ' *"--method PATCH"*"/issues/comments/"*)', + ' cid="${4##*/}"; b="${6#body=}"', + ' jq --arg id "$cid" --arg b "$b" \'map(if (.id|tostring) == $id then .body = $b else . end)\' "$COMMENTS" > "$COMMENTS.tmp"', + ' mv "$COMMENTS.tmp" "$COMMENTS" ;;', + ' *"/issues/"*"/comments"*"--method GET"*) cat "$COMMENTS" ;;', + ' *"/issues/"*"/comments"*)', + ' b="${4#body=}"', + ' jq --arg login "$BOT" --arg b "$b" \'. + [{id: (length + 1), user: {login: $login}, body: $b}]\' "$COMMENTS" > "$COMMENTS.tmp"', + ' mv "$COMMENTS.tmp" "$COMMENTS" ;;', + ' *) : ;;', + 'esac', + 'exit 0', + ].join('\n') + '\n', + ); + + const run = (pages, { window = '', disabled = '', noticeToken = 'pat-token', failListing = '' } = {}) => { + // One JSON document per page, concatenated — `gh api --paginate` without + // `--jq` emits precisely this, and it is the shape the first version + // could not read. + writeFileSync(pagesFile, pages.map((p) => JSON.stringify(p)).join('\n') + '\n'); + const outputs = join(dir, 'outputs'); + const summary = join(dir, 'summary.md'); + writeFileSync(outputs, ''); + writeFileSync(summary, ''); + const runnerTemp = join(dir, 'runner-temp'); + rmSync(runnerTemp, { recursive: true, force: true }); + mkdirSync(runnerTemp); + const stdout = execFileSync('bash', ['-e', '-c', RECHECK], { + cwd: repoRoot, + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + CALLS: calls, + PAGES: pagesFile, + COMMENTS: comments, + BOT: BOT_LOGIN, + FAIL_LISTING: failListing, + GH_TOKEN: 'job-token', + NOTICE_TOKEN: noticeToken, + PR_NUMBER: '42', + GITHUB_REPOSITORY: 'QwenLM/qwen-code', + RUNNER_TEMP: runnerTemp, + GITHUB_OUTPUT: outputs, + GITHUB_STEP_SUMMARY: summary, + AUTO_STOP_WINDOW: window, + AUTO_STOP_DISABLED: disabled, + }, + }); + return { + stdout, + output: readFileSync(outputs, 'utf8'), + summary: readFileSync(summary, 'utf8'), + calls: readFileSync(calls, 'utf8'), + }; + }; + + return { + run, + comments: () => JSON.parse(readFileSync(comments, 'utf8')), + cleanup: () => rmSync(dir, { recursive: true, force: true }), + }; +} + +/** One run, one throwaway environment — cleaned up like the sibling suite. */ +function replayRecheck(pages, opts) { + const r = makeReplay(); + try { + return r.run(pages, opts); + } finally { + r.cleanup(); + } +} + +test('a multi-page review listing still reaches the decision', () => { + // The regression: 100 reviews per page, so a diverging PR is ALWAYS + // multi-page by the time this rule could matter. + const r = replayRecheck(twoPages(CHRONOLOGICAL)); + assert.match(r.output, /should_review=false/); + assert.match(r.summary, /Automatic review skipped/); +}); + +test('a single-page listing decides the same way', () => { + const r = replayRecheck([CHRONOLOGICAL.map((b) => review(b))]); + assert.match(r.output, /should_review=false/); +}); + +test('a converging history keeps the trigger, through the same wiring', () => { + const r = replayRecheck(twoPages([...CHRONOLOGICAL.slice(0, 3), body(6, 1)])); + assert.match(r.output, /should_review=true/); + assert.match(r.summary, /fell from 3 to 1 at round 6/); +}); + +test("a stranger's forged markers cannot silence the automatic trigger", () => { + // The named threat, in its own direction: this PR is settling, and an + // outsider posts a flat, consecutive, newer series that WOULD stop the + // trigger if the listing read markers from any author. Only the + // author-scoped filter stands between the two answers. + const settling = [...CHRONOLOGICAL.slice(0, 3), body(6, 1)]; + const forged = [body(7, 3), body(8, 3), body(9, 3), body(10, 3)]; + const r = replayRecheck([ + settling.map((b) => review(b)), + [review('drive-by', 'someone-else'), ...forged.map((b) => review(b, 'someone-else'))], + ]); + assert.match(r.output, /should_review=true/); +}); + +test('the notice posts with the credential that can post', () => { + const r = replayRecheck(twoPages(CHRONOLOGICAL)); + assert.match(r.stdout, /Auto-stop notice relayed/); + const lines = r.calls.trim().split('\n'); + // The reads run on the job token; the POST — and the `gh api user` that + // scopes it — must not, because neither works under one. + const post = lines.find((l) => /ARGS=api repos\/\S+\/issues\/\S+\/comments -f$/.test(l)); + assert.ok(post, `no issue-comment POST recorded:\n${r.calls}`); + assert.match(post, /^TOKEN=pat-token /); + assert.match(lines.find((l) => /ARGS=api user/.test(l)) ?? '', /^TOKEN=pat-token /); + assert.match(lines.find((l) => /ARGS=pr view/.test(l)) ?? '', /^TOKEN=job-token /); +}); + +test('repeated stops update one notice instead of minting one per push', () => { + const r = makeReplay(); + try { + r.run(twoPages(CHRONOLOGICAL)); + const first = r.comments(); + assert.equal(first.length, 1); + // The body must carry the marker the upsert looks up by, byte-identical + // — that lookup is the only thing standing between an upsert and + // unbounded duplicates on a paused, long-diverging PR. + assert.match(first[0].body, //); + assert.match(first[0].body, /Automatic review paused/); + r.run(twoPages(CHRONOLOGICAL)); + assert.equal(r.comments().length, 1, 'a second stop minted a duplicate notice'); + } finally { + r.cleanup(); + } +}); + +test('a pause that lifts stops advertising itself', () => { + const r = makeReplay(); + try { + r.run(twoPages(CHRONOLOGICAL)); + assert.match(r.comments()[0].body, /Automatic review paused/); + // The author asked for a review, that round settled, and the next push + // resumes automation — the banner must not keep saying otherwise. + r.run(twoPages([...CHRONOLOGICAL.slice(0, 3), body(6, 1)])); + const after = r.comments(); + assert.equal(after.length, 1); + assert.match(after[0].body, /Automatic review resumed/); + assert.match(after[0].body, //); + } finally { + r.cleanup(); + } +}); + +test('a PR with no readable round is not charged for a supersede it cannot need', () => { + // A pause needs `window + 1` readable rounds, so nothing to supersede here + // — and the listing that would look for it is the cost being avoided. + const r = replayRecheck([[review('no marker at all')]]); + assert.match(r.output, /should_review=true/); + assert.doesNotMatch(r.calls, /ARGS=api user/); +}); + +test('a listing that cannot be read keeps reviewing, and leaves a mark', () => { + // The fail-open path that matters most and was previously unreachable: a + // 5xx or a rate limit on a long PR must never become a failed step, and + // must never read as "this PR has no reviews". + const r = replayRecheck(twoPages(CHRONOLOGICAL), { failListing: '1' }); + assert.match(r.output, /should_review=true/); + assert.match(r.summary, /Convergence auto-stop not engaged/); + assert.match(r.stdout, /::warning::Review listing failed/); + assert.match(r.stdout, /HTTP 502/); +}); + +test('no PAT posts nothing and still stops, rather than retrying a dead credential', () => { + const r = replayRecheck(twoPages(CHRONOLOGICAL), { noticeToken: '' }); + assert.match(r.output, /should_review=false/); + assert.match(r.stdout, /::warning::Auto-stop notice could not be posted/); + assert.doesNotMatch(r.calls, /ARGS=api user/); +}); + +test("the caller's two repository variables reach the replayed step", () => { + // window=2 makes a history too short for the default window decide. + const short = [[body(5, 3), body(4, 2), body(3, 2)].reverse().map((b) => review(b))]; + assert.match(replayRecheck(short).output, /should_review=true/); + assert.match(replayRecheck(short, { window: '2' }).output, /should_review=false/); + assert.match( + replayRecheck(twoPages(CHRONOLOGICAL), { disabled: 'true' }).output, + /should_review=true/, + ); +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0c3e0f329bf..1c9e189fb3d 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/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/dsw-swe-verified/make-terminal-bench-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/serve-ab-drive.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/assign-issue-owner.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/dsw-swe-verified/make-terminal-bench-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/serve-ab-drive.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/assign-issue-owner.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs .github/scripts/review-auto-stop.test.mjs' jobs: classify_pr: diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 1a4acdc500a..a687383bdfe 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -222,11 +222,41 @@ jobs: outputs: should_review: '${{ steps.pr_state.outputs.should_review }}' steps: + # Base branch only, and credentials dropped: this job runs on + # `pull_request_target`, so the PR head is untrusted code with a token + # in scope. Nothing here reads the PR's tree — the two scripts below + # are this repository's own, and the evidence comes from the API. + - name: 'Checkout base branch' + uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 + with: + ref: '${{ github.event.repository.default_branch }}' + fetch-depth: 1 + persist-credentials: false + - name: 'Re-check PR state' id: 'pr_state' env: + # The two reads (PR state, review listing) are all the job token + # needs, and `pull-requests: read` covers them. GH_TOKEN: '${{ secrets.GITHUB_TOKEN }}' + # The notice is the one WRITE here, and the job token cannot make + # it: the notice is an ISSUE comment (this job holds no + # `issues: write`), and upsert-bot-comment.sh opens by resolving its + # author scope through `gh api user`, which a GITHUB_TOKEN cannot + # call at all. Under the job token every stop was therefore silent + # on the PR — the one failure mode this notice exists to prevent. + # Same posture as `authorize`'s PAT, and narrower: the PAT is + # scoped to this one call, the job checks out the base branch alone + # with credentials dropped, and PR-controlled bytes reach nothing + # but JSON.parse and an integer check — never a shell, never the + # posted text. Posting as qwen-code-ci-bot is also what makes the + # upsert's author-scoped lookup find its own earlier notice. + NOTICE_TOKEN: '${{ secrets.CI_BOT_PAT }}' PR_NUMBER: '${{ github.event.pull_request.number }}' + # The caller's number, and the caller's off switch. Defaults live in + # the decision module; an unset variable takes them. + AUTO_STOP_WINDOW: '${{ vars.REVIEW_AUTO_STOP_WINDOW }}' + AUTO_STOP_DISABLED: '${{ vars.REVIEW_AUTO_STOP_DISABLED }}' run: |- set -euo pipefail pr_data="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,isDraft --jq '[.state, .isDraft] | @tsv')" @@ -242,7 +272,140 @@ jobs: echo "should_review=false" >> "$GITHUB_OUTPUT" exit 0 fi - echo "should_review=true" >> "$GITHUB_OUTPUT" + + # The caller's half of issue #9278's rule: the pipeline measures + # whether a loop is settling and owns no threshold; THIS repository + # owns the number and decides only whether the AUTOMATIC trigger + # keeps firing. It runs here and nowhere else on purpose — this job + # is reached only by `opened`/`synchronize`, so `@qwen-code /review` + # and a review_requested go around it. Stopping the treadmill is + # never refusing to review. + # + # Author-scoped on purpose: the evidence is the ledger marker a + # posted review carries, and reading markers from ANY author would + # let a stranger's forged marker silence this repository's automatic + # reviews. KEEP IN SYNC with the bot_login constant in + # review-config (that job runs only on review_requested, so it + # cannot be a `needs` here). + if [ "${AUTO_STOP_DISABLED}" = "true" ]; then + echo "Convergence auto-stop disabled by repository variable." >> "$GITHUB_STEP_SUMMARY" + echo "should_review=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + bodies="${RUNNER_TEMP}/bot-review-bodies.json" + # The marker is BOTH the lookup key and the first line of every body + # written below. upsert-bot-comment.sh finds a prior notice only by + # `contains($marker)`, so a body without it makes every stop POST a + # brand-new comment — unbounded duplicates on exactly the paused, + # long-diverging PRs this rule targets. One variable, so the two can + # never drift apart. + notice_marker='' + # Never fail the gate over the read: a listing that errors must + # leave the trigger alone, which is what the empty array does. But + # it must leave a MARK — under `2>/dev/null` a listing that COULD + # NOT BE READ was byte-identical to a PR with no reviews yet, and a + # paginated listing is likeliest to fail on precisely the long-lived + # PRs this rule exists for. + # + # `--paginate` WITHOUT `--jq`, then `jq -s` — this repository's + # convention everywhere else, and here the convention IS the + # feature. `--paginate --jq` applies the filter per page and + # concatenates the outputs, so a PR past 100 reviews emits + # `[...][...]`: two documents, which JSON.parse rejects, which the + # fallback below turns into "no rounds carry a marker", which keeps + # reviewing. Fail-open and silent — and inert on exactly the long + # diverging loops this rule exists for, while working on every PR + # short enough that the treadmill is still bearable. `jq -s` slurps + # every page into one document before the filter runs. + listing_err="${RUNNER_TEMP}/bot-review-listing.err" + if ! gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate 2>"$listing_err" \ + | jq -s '[add // [] | .[] | select(.user.login == "qwen-code-ci-bot") | .body]' \ + > "$bodies"; then + echo "::warning::Review listing failed; the convergence auto-stop skipped this run: $(head -c 500 "$listing_err" 2>/dev/null | tr '\n' ' ')" + echo '[]' > "$bodies" + fi + decision="$(node -e ' + import("./.github/scripts/review-auto-stop.mjs").then(async (m) => { + const { readFileSync } = await import("node:fs"); + let bodies = []; + try { + bodies = JSON.parse(readFileSync(process.argv[1], "utf8")); + if (!Array.isArray(bodies)) bodies = []; + } catch { bodies = []; } + const w = Number.parseInt(process.env.AUTO_STOP_WINDOW ?? "", 10); + const d = m.decideAutoStop(bodies.reverse(), Number.isInteger(w) ? { window: w } : {}); + process.stdout.write(JSON.stringify({ stop: d.stop, reason: d.reason, readable: d.evidence?.rounds?.length ?? 0 })); + }).catch(() => process.stdout.write("{\"stop\":false,\"reason\":\"decision script failed\",\"readable\":0}")); + ' "$bodies")" || decision='{"stop":false,"reason":"decision script failed","readable":0}' + stop="$(printf '%s' "$decision" | jq -r '.stop')" + reason="$(printf '%s' "$decision" | jq -r '.reason')" + readable="$(printf '%s' "$decision" | jq -r '.readable // 0')" + if [ "$stop" != "true" ]; then + echo "Convergence auto-stop not engaged: ${reason}" >> "$GITHUB_STEP_SUMMARY" + echo "should_review=true" >> "$GITHUB_OUTPUT" + # A pause that lifted must stop advertising itself, or a recovered + # PR keeps a "paused" banner that contradicts what the pipeline is + # doing. `--update-only` PATCHes an existing notice and no-ops + # when there is none, so this mints nothing. Skipped when no round + # is readable: a pause needs `window + 1` of them, so such a PR has + # never been paused and the listing would be pure cost. The body + # is static, so a PR that stays resumed is re-PATCHed with the + # same bytes rather than churning an edit per push. + if [ "${readable}" != "0" ] && [ -n "${NOTICE_TOKEN:-}" ]; then + lifted_file="${RUNNER_TEMP}/auto-stop-lifted.md" + { + printf '%s\n\n' "${notice_marker}" + printf '%s\n\n' '### Automatic review resumed' + printf '%s\n' 'The pause on this pull request has lifted: its review history is settling again, so reviews fire on every push as usual.' + printf '\n%s\n' '
中文说明' + printf '\n%s\n\n' '### 自动评审已恢复' + printf '%s\n' '本 PR 的暂停已解除:评审历史重新呈现收敛,每次推送会照常自动触发评审。' + printf '\n%s\n' '
' + } > "$lifted_file" + GH_TOKEN="${NOTICE_TOKEN}" .github/scripts/upsert-bot-comment.sh \ + "${GITHUB_REPOSITORY}" "${PR_NUMBER}" \ + "${notice_marker}" \ + "$lifted_file" --update-only \ + || echo "::warning::Could not supersede the auto-stop notice; it may still read as paused." + fi + exit 0 + fi + + echo "Automatic review skipped — ${reason}" >> "$GITHUB_STEP_SUMMARY" + echo "should_review=false" >> "$GITHUB_OUTPUT" + # A silent stop is indistinguishable from a pipeline that broke, so + # the PR says what happened, on what evidence, and how to resume. + # Upserted, so a run per push does not mint a comment per push. + body_file="${RUNNER_TEMP}/auto-stop-body.md" + { + printf '%s\n\n' "${notice_marker}" + printf '%s\n\n' '### Automatic review paused' + printf '%s\n\n' "This repository stops auto-triggering reviews when a pull request's own review history shows the loop is not settling — ${reason}." + printf '%s\n\n' 'Nothing is blocked and nothing was withheld: the reviews already posted stand, and a review on request still runs. What stops is the review that fires on every push.' + printf '%s\n\n' 'While the pause holds no round is posted, so a push alone cannot change the evidence it is measured on. An explicit review is what moves it:' + printf '%s\n' '- Comment `@qwen-code /review` for a review now.' + printf '%s\n' '- Or batch the remaining fixes, verify them, then comment `@qwen-code /review` — once that round is settling, this pause lifts by itself for the pushes after it.' + printf '\n%s\n' '
中文说明' + printf '\n%s\n\n' "### 自动评审已暂停" + printf '%s\n\n' "当一个 PR 自身的评审历史显示回路没有收敛时,本仓库会停止在每次推送上自动触发评审——${reason}。" + printf '%s\n\n' '没有任何东西被阻断或扣留:已发布的评审依然有效,按请求的评审也照常运行。停下的只是"每次推送都跑一轮"。' + printf '%s\n\n' '暂停期间不会发布新的轮次,所以只推送无法改变它所依据的证据。推动它的是一次显式评审:' + printf '%s\n' '- 评论 `@qwen-code /review` 可立即获得一轮评审。' + printf '%s\n' '- 或把剩余修复攒成一批、验证后评论 `@qwen-code /review`——当那一轮呈现收敛,此后的推送该暂停会自行解除。' + printf '\n%s\n' '
' + } > "$body_file" + # An absent PAT (a fork of this repository) posts nothing rather + # than spending three authenticated-retry rounds on a credential + # that cannot work; the skip still stands, and the warning says so. + if [ -n "${NOTICE_TOKEN:-}" ] && GH_TOKEN="${NOTICE_TOKEN}" \ + .github/scripts/upsert-bot-comment.sh \ + "${GITHUB_REPOSITORY}" "${PR_NUMBER}" \ + "${notice_marker}" \ + "$body_file"; then + echo "Auto-stop notice relayed to PR #${PR_NUMBER}." + else + echo "::warning::Auto-stop notice could not be posted; the skip itself stands. Reason: ${reason}" + fi authorize: needs: ['precheck-pr'] diff --git a/docs/design/2026-08-22-review-auto-stop.md b/docs/design/2026-08-22-review-auto-stop.md new file mode 100644 index 00000000000..cc4d7003d74 --- /dev/null +++ b/docs/design/2026-08-22-review-auto-stop.md @@ -0,0 +1,130 @@ +# Caller-side convergence enforcement for automatic reviews + +## What this is + +The `/review` pipeline measures whether a pull request's review loop is +settling and says so in the posted body. It owns no threshold and stops +nothing — that is the governance rule this whole line of work is built on +(issue #9278): **the tool measures, the caller decides.** + +This is the caller's half. This repository reads the telemetry the pipeline +already publishes and applies **its own** number to one question: should the +automatic review keep firing on every push? + +## What it does not do + +- **It never blocks a review anyone asked for.** The check lives in + `delay-automatic-review`, a job reached only by `opened` and `synchronize`. + `@qwen-code /review` and a requested review go around it. Stopping the + treadmill is not refusing to review. +- **It never fails closed.** Telemetry that will not parse, a round the + listing could not fetch, a gap in the round numbers, a posture that changed + underneath the numbers, a missing Node — every one of them keeps reviewing. + A caller that silences reviews when it cannot read its own evidence is + worse than one with no rule at all. +- **It withholds nothing already found.** Reviews already posted stand. + +## The rule + +From the last posted reviews by this account, read each ledger marker's +`round`, `fresh` (findings reported for the FIRST time — not the round's +whole output, which only rises while an unfixed blocker keeps being +re-posted) and `floor`. Then, over a window of `W` consecutive rounds: + +| Condition | Decision | +| ------------------------------------------------ | ----------------------------------------- | +| fewer than `W + 1` readable rounds | keep reviewing (unevaluable) | +| any round in the window recorded no `fresh` | keep reviewing | +| the rounds are not consecutive | keep reviewing (a trend over unseen work) | +| the newest round produced no first-time findings | keep reviewing (settled) | +| two different posting floors inside the window | keep reviewing (posture change) | +| every step non-shrinking across `W` rounds | **stop the automatic trigger** | + +Every clause mirrors a reading the pipeline itself refuses to call +divergence. The only thing this repository adds is `W`. + +When it stops, the PR gets one upserted comment naming the measurement, the +evidence, and how to resume. + +## How a pause lifts + +Not by pushing. While the pause holds, `review-pr` never runs for +`opened`/`synchronize`, so no round is posted, so no new marker joins the +window — the evidence the rule measures is frozen, and every later push +re-decides the identical stop. The only thing that moves it is a review on a +path the gate cannot reach: `@qwen-code /review`, a requested review, +`ready_for_review`, `reopened`. Once such a round posts a marker whose +first-time count falls, the window is no longer flat and the pushes after it +are automatic again. + +The notice says exactly this, because an author who reads "push once and it +lifts by itself" waits forever — which is the same silent-stop confusion the +notice exists to prevent. + +When the pause does lift, the same comment is superseded in place (the +`--update-only` upsert, which mints nothing where no notice exists): a +recovered pull request must not keep advertising a pause that is over. The +supersede is skipped when the listing produced no readable round, because a +pause needs `W + 1` of them and such a pull request has never been paused. + +## Configuration + +| Repository variable | Default | Meaning | +| --------------------------- | ------- | ------------------------------------------------------------------------------ | +| `REVIEW_AUTO_STOP_WINDOW` | `3` | Consecutive non-shrinking rounds tolerated before the automatic trigger stops. | +| `REVIEW_AUTO_STOP_DISABLED` | unset | `true` turns the rule off entirely. | + +Both are read in `delay-automatic-review`. Neither exists in the pipeline — +they are the caller's, and changing them changes no verdict, no finding, and +no posted review. + +## Why the number is not a round count + +The obvious rule — "stop after round N" — is the one the measured data +rejects. Two pull requests that ran this feature's own review loop to +completion (#9461 and #9623) each took nine rounds, and #9461's rounds 6 and +7 still produced 5 and 2 Critical findings. A round-count bar would have cut +those off. The trend is the signal; the count is not. + +## Three runtime traps, all fail-open, all silent + +Every round of this feature has shipped green and broken, and always for the +same reason: fail-open failures leave no mark. They are pinned by tests now, +and they are the things to check before editing the step. + +1. **The listing must not use `--paginate --jq`.** `gh` applies the filter + per page and concatenates the outputs, so a pull request past 100 reviews + emits two JSON documents rather than one array. `JSON.parse` rejects it, + the fallback reads "no rounds carry a marker", and the rule keeps + reviewing — permanently inert on exactly the long diverging loops it + exists for, while working on every pull request short enough that the + treadmill is still bearable. Use `--paginate` alone and slurp with + `jq -s`, this repository's convention everywhere else. +2. **The notice body must contain the marker it is looked up by.** + `upsert-bot-comment.sh` finds a prior comment only through + `contains($marker)`. A body without the marker never matches, so every + stop POSTs a new comment — and a paused pull request re-decides the same + stop on every push, so the duplicates are unbounded, on exactly the + long-diverging loops this rule targets. The marker is one shell variable + used for both the body and the lookup key, so the two cannot drift. +3. **The notice must be posted with `CI_BOT_PAT`.** It is an issue comment, + the job holds no `issues: write`, and `upsert-bot-comment.sh` opens by + resolving its author scope through `gh api user` — an endpoint a + `GITHUB_TOKEN` cannot call at all. Under the job token every stop was + silent on the pull request, which is the one failure mode the notice + exists to prevent. The two reads stay on the job token. + +## Where it lives + +- `.github/scripts/review-auto-stop.mjs` — the decision, as a pure function. +- `.github/scripts/review-auto-stop.test.mjs` — its tests, registered in + `HELPER_TESTS`. The later half of the file replays the shipped `run:` block + against a stubbed `gh` that paginates the way the real one does and keeps a + comment store across runs; a unit test over the decision alone cannot see + any of the traps above. +- `packages/cli/src/commands/review/lib/ledger-auto-stop-contract.test.ts` — + the only coupling between the marker this pipeline writes and the second, + hand-copied reader that consumes it. It feeds real `serializeLedger` output + through the real gate, so a producer-side format change cannot leave the + gate silently reading zero rounds. +- `.github/workflows/qwen-code-pr-review.yml` — the gate that calls it. diff --git a/packages/cli/src/commands/review/lib/ledger-auto-stop-contract.test.ts b/packages/cli/src/commands/review/lib/ledger-auto-stop-contract.test.ts new file mode 100644 index 00000000000..4cc460fddc1 --- /dev/null +++ b/packages/cli/src/commands/review/lib/ledger-auto-stop-contract.test.ts @@ -0,0 +1,159 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * The marker format has a SECOND reader outside this package: + * `.github/scripts/review-auto-stop.mjs`, the caller-side gate that decides + * whether this repository keeps auto-triggering reviews (issue #9278). It + * hand-copies the `OPEN`/`CLOSE` tokens, the `v === 1` pin and three field + * names from this module, which are module-private here — so nothing but this + * file couples the two. + * + * Without it, a producer-side `v: 1` → `v: 2` bump makes that gate read every + * genuine review as unmarked: it reports "only 0 round(s) carry a readable + * marker" on every PR and keeps reviewing forever — permanently inert, + * fail-open, silent, and green in the gate's own suite, because every fixture + * there is built from the gate's own constants. + * + * So these assertions feed REAL `serializeLedger` output through the real + * gate. It runs in a child `node`, not as an import: the gate is plain ESM + * outside every package's tsconfig, and spawning it exercises the module the + * workflow actually loads. + */ + +import { execFileSync } from 'node:child_process'; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +import { + LEDGER_MAX_BYTES, + LEDGER_MAX_FINDINGS, + serializeLedger, + type Ledger, + type LedgerFinding, +} from './ledger.js'; + +/** + * Found by walking up rather than by a relative hop: this file's own URL is + * not a file URL under Vitest's transform, and a wrong path here would be a + * spawn failure, never a silent pass. + */ +const gate = (() => { + const rel = join('.github', 'scripts', 'review-auto-stop.mjs'); + for (let dir = process.cwd(); ; dir = dirname(dir)) { + if (existsSync(join(dir, rel))) return join(dir, rel); + if (dirname(dir) === dir) + throw new Error(`could not locate ${rel} above ${process.cwd()}`); + } +})(); + +interface GateAnswer { + markers: Array<{ + round: number; + fresh: number | null; + floor: string | null; + } | null>; + decision: { stop: boolean; reason: string; evidence: { window: number } }; +} + +/** Run the shipped gate over these bodies, newest first. */ +function askGate(bodies: string[]): GateAnswer { + const dir = mkdtempSync(join(tmpdir(), 'auto-stop-contract-')); + try { + const input = join(dir, 'input.json'); + writeFileSync(input, JSON.stringify({ bodies })); + const out = execFileSync( + process.execPath, + [ + '-e', + `import(${JSON.stringify(pathToFileURL(gate).href)}).then(async (m) => { + const { readFileSync } = await import('node:fs'); + const { bodies } = JSON.parse(readFileSync(process.argv[1], 'utf8')); + process.stdout.write(JSON.stringify({ + markers: bodies.map((b) => m.readMarker(b)), + decision: m.decideAutoStop(bodies), + })); + });`, + input, + ], + { encoding: 'utf8' }, + ); + return JSON.parse(out) as GateAnswer; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +const finding = (round: number, n: number): LedgerFinding => ({ + id: `R${round}-${n}`, + sev: 'S', + file: 'packages/cli/src/commands/review/compose-review.ts', + line: 10 * n, + title: `finding ${n} of round ${round}`, +}); + +const round = (r: number, fresh: number, findings = 2): Ledger => ({ + v: 1, + round: r, + findings: Array.from({ length: findings }, (_, i) => finding(r, i + 1)), + posted: findings, + prevPosted: findings, + fresh, + floor: 'o', + sha: 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef', + model: 'qwen3.8-max', +}); + +describe('the caller-side auto-stop gate reads what this module writes', () => { + it('round-trips a real marker into the three fields the gate reads', () => { + const body = `## Code review\n\nprose\n\n${serializeLedger(round(6, 3))}`; + expect(askGate([body]).markers[0]).toEqual({ + round: 6, + fresh: 3, + floor: 'o', + }); + }); + + it('stops on a real non-shrinking series, with the measurement it states', () => { + // Newest first, exactly as the workflow hands them over. + const bodies = [round(6, 3), round(5, 3), round(4, 2), round(3, 2)].map( + (l) => `prose\n\n${serializeLedger(l)}`, + ); + const { decision } = askGate(bodies); + expect(decision.stop).toBe(true); + expect(decision.reason).toMatch(/r3=2 → r4=2 → r5=3 → r6=3/); + }); + + it('keeps reviewing when the byte cap sheds the count the trend is about', () => { + // `fresh` and `floor` are the FIRST things the size cascade sheds, so a + // heavy round genuinely publishes a marker without them. The gate must + // read that as unevaluable — never as a flat trend. + const heavy: Ledger = { + ...round(6, 3, LEDGER_MAX_FINDINGS), + findings: Array.from({ length: LEDGER_MAX_FINDINGS }, (_, i) => ({ + ...finding(6, i + 1), + file: `packages/cli/src/commands/review/lib/${'d'.repeat(150)}/f${i}.ts`, + title: `${'t'.repeat(70)}${i}`, + })), + }; + const shed = serializeLedger(heavy); + expect(shed.length).toBeLessThanOrEqual(LEDGER_MAX_BYTES); + expect(shed).not.toContain('"fresh"'); + + const answer = askGate([ + `prose\n\n${shed}`, + ...[round(5, 3), round(4, 2), round(3, 2)].map( + (l) => `prose\n\n${serializeLedger(l)}`, + ), + ]); + expect(answer.markers[0]).toEqual({ round: 6, fresh: null, floor: null }); + expect(answer.decision.stop).toBe(false); + expect(answer.decision.reason).toMatch(/no first-time count/); + }); +});