Repository navigation
feat(ci): stop auto-triggering reviews on a loop that is not settling #9711
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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,130 @@ | ||
| #!/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 = '<!-- qwen-review-ledger '; | ||
| const CLOSE = ' -->'; | ||
|
|
||
| /** | ||
| * 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. | ||
| */ | ||
| export function decideAutoStop(bodies, options = {}) { | ||
| const window = Number.isInteger(options.window) && options.window > 0 | ||
| ? options.window | ||
| : DEFAULT_WINDOW; | ||
| const cont = (reason) => ({ stop: false, reason, evidence: null }); | ||
|
|
||
| const rounds = []; | ||
| 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: { window, rounds: rounds.slice().reverse() }, | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
| }; | ||
| } | ||
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,113 @@ | ||
| 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<!-- qwen-review-ledger ${JSON.stringify({ | ||
| v: 1, | ||
| round, | ||
| findings: [], | ||
| ...(fresh === null ? {} : { fresh }), | ||
| ...(floor === null ? {} : { floor }), | ||
| ...extra, | ||
| })} -->`; | ||
|
|
||
| /** | ||
| * 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); | ||
| }); | ||
|
|
||
| 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 to "stop". | ||
| assert.equal(decideAutoStop(STOPPING, { window: 0 }).stop, true); | ||
| assert.equal(decideAutoStop(STOPPING, { window: -1 }).stop, true); | ||
|
wenshao marked this conversation as resolved.
Resolved
|
||
| }); | ||
|
|
||
| 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', | ||
| '<!-- qwen-review-ledger {not json} -->', | ||
| '<!-- qwen-review-ledger {"v":1,"round":6,"fresh":3}', | ||
| '<!-- qwen-review-ledger {"v":2,"round":6,"fresh":3} -->', | ||
| '<!-- qwen-review-ledger {"v":1,"round":0,"fresh":3} -->', | ||
| ]) { | ||
| 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' }); | ||
| }); | ||
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
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.