Skip to content

Commit 8cde324

Browse files
committed
fix(ci): the staleness premise is false for CodeRabbit, so ship it off (#3312)
Fifth-round review of #3317. The `claimed-verdict` rule false-positives on 2 of its 4 claimed fires, INCLUDING THE FLAGSHIP #3276, and the cause is a premise this repo's primary reviewer does not honour: CODERABBIT SUBMITS NO REVIEW EVENT AT ALL WHEN A RUN FINDS NOTHING ACTIONABLE. So "no review object naming the head" is not "the head was not reviewed". Measured live 2026-08-26 on all four: #3276 head 1305f77 -- Review queued 14:09:52 -> in progress 14:09:55 -> success 14:12:27. A real 155 s cycle ON THE HEAD, and the walkthrough comment updated 14:12:25Z reads "No actionable comments were generated in the recent review" over "changes between c26e453 and 1305f77": the head, including the commit the rule called unreviewed. #3288 is identical (181 s, head named). BOTH FALSE. #3227 (14 s) and #2952 (9 s) are genuine -- their walkthroughs read "Reviews paused ... under active development", and CodeRabbit published `success / Review completed` regardless. NOTHING IN THE STRUCTURED DATA SEPARATES THE TWO PAIRS. The status is byte-identical across all four; CodeRabbit publishes no check RUN on any of these heads, so there is no `conclusion` or `output.title` to read; and the suggested narrowing -- "a completed review cycle on this head counts as review" -- is not a narrowing but a deletion, because clause (b) already requires `success` on the head and a `success` on the head IS a completed cycle, so it silences #3227 and #2952 too. What is left is cycle DURATION, an unversioned timing heuristic on a third party, and the reviewer's PROSE, which the config rules out on purpose. It also contradicted this file's own stated hole: #3316 has success on its head, zero reviews, and is deliberately silent. A rule that is wrong half the time cannot gate a PR and cannot be repaired with a discriminator that does not exist, so the machinery, the three scopings and the four worked examples all ship and `staleReviewPolicy` DEFAULTS TO `off`. `off` is inert rather than merely silent -- it adjudicates nothing, so it refuses nothing and does not pay for the paginated reviews walk -- and it NEVER prints a pass: it prints `STALE_REVIEW not adjudicated` naming the knob. #3227/#2952 stay catchable for whoever opts in. Verified live over #3276, #3288, #3227, #2952, #3315, #3309, #2931 and #3316: `off` is silent on all eight, and `claimed-verdict` still reproduces its 4-fire/4-silent table exactly. A SUPPRESSED FINDING NO LONGER RENDERS AS A CLEAN PASS. With `staleReviewSeverity: "fail"` and the shipped `reviewVerdictSeverity: "warn"`, a rate-limited CodeRabbit with a stale review printed `✅ No reviewer claims a verdict ... from a review of an older commit` and exited 0, while the same input under `configured-authors` printed `❌ STALE_REVIEW` and exited 1: the `alreadyFlagged` dedup dropped the finding, so `stale.length === 0` conflated "clean" with "suppressed" and the severity knob was inoperative. `staleReviews` now returns the finding with `suppressedBy` set, and the caller suppresses the SENTENCE, not the VERDICT -- one line naming what already reported it, and the exit code still tracks the knob. ORDERING IS `id` ALONE, and the old `(submitted_at, id)` was strictly worse: the primary key was the one field that can be absent, so a review AT THE HEAD with no timestamp sorted to `''`, lost to every dated review, and would have reported a CURRENT PR as stale -- the finding the JSDoc promises is impossible. `id` is always present (`UNREADABLE_REVIEW_ID` refuses otherwise) and removes the class outright. `submitted_at` is still printed, no longer compared. `fetchCheckRunDescriptions` now walks `--paginate --slurp` through `flattenCheckRunPages`, which refuses a partial walk. It was not live (31 check runs on the largest head measured, against a 100 page size) but the failure mode was the bad one: under `claimed-verdict` a missing context is adjudicated by SILENCE, so truncation was a false negative, not a failure. And the gate's own unit tests now RUN. Neither test file was reached by any workflow -- test.yml names its script tests one by one and this pair was never added, and check-test-glob-coverage audits package globs, not `scripts/`. 10 mutations run against the guards; 10 of 10 caught, and the tenth only after adding the WIRING test the sweep proved was missing: replacing `flattenCheckRunPages(...)` with an inline `pages.flatMap(p => p.check_runs ?? [])` survived the entire suite, because the helper's refusal was tested and its USE was not. Every mutation restored by inverse edit, byte-identity asserted. Refs #3312 Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
1 parent 4d62077 commit 8cde324

6 files changed

Lines changed: 493 additions & 75 deletions

File tree

.github/workflows/pr-review-signal.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,18 @@ jobs:
113113
# because the fan-out boundary produces a ~1 s window in which every
114114
# published lane is terminal and more are still coming -- so the
115115
# genuine-absence path costs 60 s, not the whole budget.
116+
# THE GATE'S OWN UNIT TESTS, RUN BEFORE THE GATE. Neither
117+
# `scripts/check-pr-review-signal.test.mjs` nor
118+
# `scripts/lib/pr-review-signal.test.mjs` was reached by any workflow: the
119+
# test.yml lanes name their script files one by one and this pair was
120+
# never added, and `check-test-glob-coverage` audits package globs, not
121+
# `scripts/`. A gate whose fail-closed refusals, severity knobs and
122+
# policy scopings are asserted only on a developer's machine is a gate
123+
# nobody is checking, which is the shape this whole workflow exists to
124+
# reject. Same job, no `pnpm install` -- node builtins only, like the gate.
125+
- name: Unit-test the gate itself
126+
run: node --test scripts/check-pr-review-signal.test.mjs scripts/lib/pr-review-signal.test.mjs
127+
116128
- name: Check required lanes ran and reviews produced a verdict
117129
env:
118130
GH_TOKEN: ${{ github.token }}

scripts/check-pr-review-signal.mjs

Lines changed: 79 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -107,12 +107,23 @@
107107
* WHICH REVIEWS COUNT IS A POLICY CALL AND IS DELIBERATELY NOT SETTLED HERE.
108108
* It is `staleReviewPolicy` in the config, validated like
109109
* `reviewVerdictSeverity` — an unrecognised value is `BAD_CONFIG`, never a
110-
* silent downgrade. The shipped default `claimed-verdict` is the narrowest
111-
* rule that still catches #3276. The lib documents the measurements that
112-
* rule out the two obvious alternatives, both of which are wrong against
113-
* this repository's data: dropping `COMMENTED` makes the check a no-op, and
114-
* treating "no review at the head" as staleness fires on PRs where the
115-
* reviewer never submitted a review object at all.
110+
* silent downgrade. `claimed-verdict` is the narrowest rule that still
111+
* catches #3276. The lib documents the measurements that rule out the two
112+
* obvious alternatives, both of which are wrong against this repository's
113+
* data: dropping `COMMENTED` makes the check a no-op, and treating "no
114+
* review at the head" as staleness fires on PRs where the reviewer never
115+
* submitted a review object at all.
116+
*
117+
* AND THE SHIPPED DEFAULT IS `off`, BECAUSE THE PREMISE IS FALSE FOR THIS
118+
* REPO'S PRIMARY REVIEWER. CodeRabbit submits NO REVIEW EVENT when a run
119+
* finds nothing actionable, so "no review object naming the head" is not
120+
* "the head was not reviewed": #3276 and #3288 each show a real 155 s /
121+
* 181 s review cycle ON THE HEAD and a walkthrough naming the head, and
122+
* they are false fires; #3227 and #2952 are genuine ("Reviews paused"). No
123+
* structured field separates the two pairs — see the lib and the config for
124+
* the measurements and for why the obvious narrowing deletes the rule
125+
* instead. So it ships OFF, it NEVER prints a pass while off, and the knob
126+
* opts back in. #3227 and #2952 stay catchable for whoever sets it.
116127
*
117128
* SEVERITY `warn`, same `@unwired-by-design` ruling as part 2: whether a bot
118129
* has re-reviewed the newest push is transient GitHub state, not a fact
@@ -147,6 +158,7 @@ import {
147158
expandJobNames,
148159
flattenReviewPages,
149160
missingLanes,
161+
flattenCheckRunPages,
150162
noVerdictReviews,
151163
pollForLanes,
152164
staleReviews,
@@ -436,22 +448,21 @@ function fetchStatusDescriptions(opts) {
436448
* @param {{ repo: string, sha: string }} opts
437449
*/
438450
function fetchCheckRunDescriptions(opts) {
439-
const data = gh(
451+
// `--paginate --slurp`, for the same reason as the reviews walk and one that
452+
// is worse here: a truncated check-run read drops a reviewer CONTEXT, and a
453+
// dropped context is adjudicated by silence, not by a failure. `--jq` is not
454+
// available alongside `--slurp`, so the `.check_runs` projection moves into
455+
// `flattenCheckRunPages`, where the partial-walk refusal is testable.
456+
const pages = gh(
440457
[
441458
'api',
459+
'--paginate',
460+
'--slurp',
442461
`repos/${opts.repo}/commits/${opts.sha}/check-runs?per_page=100`,
443-
'--jq',
444-
'.check_runs',
445462
],
446463
`check runs for ${opts.sha}`,
447464
);
448-
if (!Array.isArray(data)) {
449-
throw new ReviewSignalError(
450-
'NO_CHECK_RUNS',
451-
`The check-runs API returned a non-array for ${opts.sha}. Refusing to read that as ` +
452-
'"no reviewer said anything".',
453-
);
454-
}
465+
const data = flattenCheckRunPages(pages, `check runs for ${opts.sha}`);
455466
return data.map((c) => ({
456467
name: c.name ?? '',
457468
state: String(c.conclusion ?? '').toLowerCase(),
@@ -598,7 +609,28 @@ export function evaluate({ required, lanes, reviewChecks, reviews, headSha, isFo
598609
// specific of the two. Saying it twice is noise, not diligence.
599610
alreadyFlagged: findings.map((f) => f.name),
600611
});
601-
if (stale.length === 0) {
612+
// THREE STATES, NOT TWO. `clean`, `found`, and `not adjudicated` -- and only
613+
// the first of them may print a tick. An empty finding list used to mean both
614+
// "nobody is stale" and "the finding was deduped away", so a suppressed
615+
// finding rendered as a pass the gate had not earned. That is the exact
616+
// defect class this file exists to remove, so a suppressed finding now prints
617+
// its own line and still moves the exit code.
618+
const reported = stale.filter((f) => f.suppressedBy === null);
619+
const suppressed = stale.filter((f) => f.suppressedBy !== null);
620+
if (cfg.staleReviewPolicy === 'off') {
621+
// NOT a tick. `off` means this question was not asked; saying "no reviewer
622+
// claims a verdict from an older review" would be an answer nobody
623+
// computed. See the config for why `off` is the shipped default.
624+
lines.push(
625+
'➖ STALE_REVIEW not adjudicated: `staleReviewPolicy` is "off", so this gate does NOT ' +
626+
'tell you whether a',
627+
' review of an older commit is standing in for a review of the head. That is a hole, ' +
628+
'stated rather than',
629+
' papered over with a tick — see the config for the measured premise defect that turned ' +
630+
'it off, and the',
631+
' knob that opts back in.',
632+
);
633+
} else if (stale.length === 0) {
602634
lines.push(
603635
`✅ No reviewer claims a verdict on ${headSha.slice(0, 8)} from a review of an older commit ` +
604636
`(policy: ${cfg.staleReviewPolicy}).`,
@@ -607,14 +639,33 @@ export function evaluate({ required, lanes, reviewChecks, reviews, headSha, isFo
607639
const fatal = cfg.staleReviewSeverity === 'fail';
608640
if (fatal) ok = false;
609641
const mark = fatal ? '❌' : '⚠️ ';
610-
for (const f of stale) {
642+
for (const f of reported) {
611643
lines.push(
612644
`${mark} STALE_REVIEW: \`${f.context ?? f.login}\` reads as having reviewed this PR, but ` +
613645
`${f.login}'s newest review is of ${f.reviewedSha.slice(0, 8)}` +
614646
`${f.submittedAt ? ` (${f.submittedAt})` : ''}, not of the head ` +
615647
`${headSha.slice(0, 8)}.`,
616648
);
617649
}
650+
// The SENTENCE is the duplicate, not the finding. Part 2 has already
651+
// quoted this reviewer verbatim and the remedy is identical, so this says
652+
// so in one line rather than repeating the whole paragraph — and the
653+
// severity knob above has already been applied to it.
654+
for (const f of suppressed) {
655+
lines.push(
656+
`${mark} STALE_REVIEW: \`${f.context ?? f.login}\` is ALSO stale — its newest review is ` +
657+
`of ${f.reviewedSha.slice(0, 8)}, not ${headSha.slice(0, 8)} — reported above as ` +
658+
`\`${f.suppressedBy}\`; same remedy, so it is not restated.`,
659+
);
660+
}
661+
if (reported.length === 0) {
662+
lines.push(
663+
' Re-run the reviewer on the head. Everything pushed since that commit is unreviewed ' +
664+
'code under a passing signal.',
665+
);
666+
}
667+
}
668+
if (reported.length > 0) {
618669
lines.push(
619670
' A review whose `commit_id` is not the PR head has not reviewed the PR (#3312). ' +
620671
'Everything pushed since',
@@ -719,12 +770,20 @@ function main() {
719770
...fetchStatusDescriptions({ repo, sha: state.sha }),
720771
...fetchCheckRunDescriptions({ repo, sha: state.sha }),
721772
];
722-
const reviews = fetchReviews({ repo, pr: args.pr });
773+
// `off` READS NOTHING. The policy adjudicates nothing, so paying for a
774+
// paginated walk — and, worse, letting its REVIEWS_TRUNCATED refusal take the
775+
// gate down — over a question this run does not ask would be noise. Parts 1
776+
// and 2 are untouched.
777+
const reviews = cfg.staleReviewPolicy === 'off' ? [] : fetchReviews({ repo, pr: args.pr });
723778

724779
console.log(`PR #${args.pr} @ ${state.sha}${state.isFork ? ' (fork)' : ''}`);
725780
console.log(`Required lanes derived from ${args.workflow}: ${required.length}`);
726781
console.log(`Rollup lanes seen: ${state.lanes.length}`);
727-
console.log(`Review events read: ${reviews.length}`);
782+
console.log(
783+
cfg.staleReviewPolicy === 'off'
784+
? 'Review events read: none (staleReviewPolicy is "off")'
785+
: `Review events read: ${reviews.length}`,
786+
);
728787
console.log('');
729788

730789
const { ok, lines } = evaluate({

scripts/check-pr-review-signal.test.mjs

Lines changed: 121 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -616,11 +616,25 @@ const STATE_3276 = (commitId) => ({
616616
headSha: HEAD_3276,
617617
});
618618

619+
/**
620+
* PART 3 OPTED IN.
621+
*
622+
* The SHIPPED default is `off` — see the config's premise note: CodeRabbit
623+
* submits no review event when a run finds nothing actionable, so 2 of
624+
* `claimed-verdict`'s 4 live fires were false. The rule still ships, and every
625+
* test below that exercises it therefore says so explicitly rather than
626+
* inheriting a default. The `off` behaviour is asserted separately.
627+
*/
628+
const ON = (patch = {}, tag = 'part3-on') => [
629+
'--config',
630+
cfgWith({ staleReviewPolicy: 'claimed-verdict', ...patch }, tag),
631+
];
632+
619633
/** The staleness knob forced to `fail`, as `FATAL` does for part 2. */
620-
const FATAL_STALE = () => ['--config', cfgWith({ staleReviewSeverity: 'fail' }, 'fatal-stale')];
634+
const FATAL_STALE = () => ON({ staleReviewSeverity: 'fail' }, 'fatal-stale');
621635

622636
test('the #3276 shape: lanes clean, no-verdict clean, and the review still names an older commit', () => {
623-
const r = run(STATE_3276(OLD_3276));
637+
const r = run(STATE_3276(OLD_3276), ON());
624638
assert.match(r.output, /All 3 required lane\(s\)/);
625639
assert.match(r.output, /none reports a passing state over a review it did not perform/);
626640
assert.match(r.output, /STALE_REVIEW: `CodeRabbit`/);
@@ -633,7 +647,7 @@ test('the #3276 shape: lanes clean, no-verdict clean, and the review still names
633647
test('ANTI-VACUITY: the identical PR with the review AT the head reports no staleness', () => {
634648
// #3315, #3309 and #2931 live. Without this, an implementation that always
635649
// reports stale passes the test above.
636-
const r = run(STATE_3276(HEAD_3276));
650+
const r = run(STATE_3276(HEAD_3276), ON());
637651
assert.doesNotMatch(r.output, /STALE_REVIEW/);
638652
assert.match(r.output, /No reviewer claims a verdict on 1305f778 from a review of an older commit/);
639653
assert.match(r.output, /policy: claimed-verdict/);
@@ -650,35 +664,77 @@ test('ESCALATION: `staleReviewSeverity: fail` turns the same finding red', () =>
650664
assert.equal(green.code, 0, green.output);
651665
});
652666

653-
test('the SHIPPED default for part 3 is `warn`, asserted once', () => {
667+
test('the SHIPPED default for part 3 is `off`, and `off` NEVER prints a tick', () => {
654668
const cfg = JSON.parse(readFileSync(CONFIG, 'utf8'));
655669
assert.equal(cfg.staleReviewSeverity, 'warn');
656-
assert.equal(cfg.staleReviewPolicy, 'claimed-verdict');
670+
assert.equal(cfg.staleReviewPolicy, 'off');
671+
672+
// #3276's own shape, the one `claimed-verdict` fires on, under the shipped
673+
// config. It must say the question was not asked — not answer it.
674+
const r = run(STATE_3276(OLD_3276));
675+
assert.doesNotMatch(r.output, /No reviewer claims a verdict/, 'a tick nobody earned');
676+
assert.doesNotMatch(r.output, /STALE_REVIEW: /);
677+
assert.match(r.output, /STALE_REVIEW not adjudicated/);
678+
assert.match(r.output, /`staleReviewPolicy` is "off"/);
679+
assert.equal(r.code, 0, r.output);
680+
681+
// MUTATION GUARD: `off` is inert, not merely silent. Under `claimed-verdict`
682+
// each of these is a refusal (asserted below); under `off` the gate does not
683+
// fall over on a question it never asks.
684+
const inert = runRaw({ required: HEALTHY, lanes: HEALTHY.map((n) => LANE(n)) });
685+
assert.equal(inert.code, 0, inert.output);
686+
assert.match(inert.output, /STALE_REVIEW not adjudicated/);
687+
assert.doesNotMatch(inert.output, /NO_REVIEWS|NO_HEAD_SHA/);
657688
});
658689

659690
test('NO NAG: a reviewer with no review event and a `Review completed` status is silent', () => {
660691
// #3316 and #3205 verbatim: `CodeRabbit :: success / Review completed` and
661692
// zero review events. A stated hole, not an oversight.
662-
const r = run({ ...STATE_3276(OLD_3276), reviews: [] });
693+
const r = run({ ...STATE_3276(OLD_3276), reviews: [] }, ON());
663694
assert.doesNotMatch(r.output, /STALE_REVIEW/);
664695
assert.equal(r.code, 0, r.output);
665696
});
666697

667-
test('DEDUPE: a rate-limited CodeRabbit is reported by part 2 only, not twice', () => {
668-
const r = run(
669-
{
670-
...STATE_3276(OLD_3276),
671-
reviewChecks: [
672-
{ name: 'CodeRabbit', state: 'success', description: 'Review rate limited' },
673-
],
674-
},
698+
test('DEDUPE: a suppressed finding is said ONCE and still moves the exit code', () => {
699+
// RED, verbatim, before the fix: this exact input with `staleReviewSeverity`
700+
// forced to `fail` printed
701+
// ✅ No reviewer claims a verdict on 1305f778 from a review of an older commit
702+
// and exited 0, while the same input under `configured-authors` printed
703+
// ❌ STALE_REVIEW: `CodeRabbit` …
704+
// and exited 1. The dedupe made the severity knob inoperative and rendered a
705+
// suppressed finding as a clean pass — the defect class this gate exists to
706+
// remove.
707+
const rateLimited = {
708+
...STATE_3276(OLD_3276),
709+
reviewChecks: [{ name: 'CodeRabbit', state: 'success', description: 'Review rate limited' }],
710+
};
711+
const r = run(rateLimited, FATAL_STALE());
712+
assert.match(r.output, /NO_VERDICT: `CodeRabbit`/);
713+
// A tick is the one thing it must never print here.
714+
assert.doesNotMatch(r.output, /No reviewer claims a verdict/);
715+
// Said once: the full three-line staleness paragraph is NOT repeated…
716+
assert.doesNotMatch(r.output, /reads as having reviewed this PR/);
717+
// …but the finding is stated, and it names what already reported it.
718+
assert.match(r.output, /STALE_REVIEW: `CodeRabbit` is ALSO stale/);
719+
assert.match(r.output, /reported above as `CodeRabbit`/);
720+
assert.match(r.output, /Re-run the reviewer on the head/);
721+
// And the knob is operative: `fail` is red.
722+
assert.equal(r.code, 1, r.output);
723+
724+
// ANTI-VACUITY, both ways. Same rate-limited input with the review AT the
725+
// head is genuinely clean and prints the tick it earned…
726+
const clean = run(
727+
{ ...rateLimited, reviews: [REVIEW_3276(HEAD_3276)] },
675728
FATAL_STALE(),
676729
);
677-
assert.match(r.output, /NO_VERDICT: `CodeRabbit`/);
678-
assert.doesNotMatch(r.output, /STALE_REVIEW/);
679-
// Part 2 is `warn` by default, part 3 was forced to `fail` — so a leaked
680-
// duplicate would have shown up as a non-zero exit as well as a second line.
681-
assert.equal(r.code, 0, r.output);
730+
assert.match(clean.output, /No reviewer claims a verdict/);
731+
assert.doesNotMatch(clean.output, /STALE_REVIEW/);
732+
assert.equal(clean.code, 0, clean.output);
733+
// …and at the shipped `warn` the suppressed finding is a warning, not red,
734+
// so the exit code tracks the knob rather than the branch.
735+
const warned = run(rateLimited, ON());
736+
assert.match(warned.output, /STALE_REVIEW: `CodeRabbit` is ALSO stale/);
737+
assert.equal(warned.code, 0, warned.output);
682738
});
683739

684740
// ------------------------------------------------- fail-closed, as a process
@@ -688,21 +744,27 @@ test('FAIL CLOSED: a state file that omits `reviews` gets NO_REVIEWS, not a succ
688744
// path, and the last defect in this file was exactly that path quietly
689745
// supplying a value (`timedOut: false`) the real path computes. Defaulting
690746
// `reviews` to `[]` inside the gate would repeat it.
691-
const r = runRaw({ required: HEALTHY, lanes: HEALTHY.map((n) => LANE(n)), headSha: HEAD_3276 });
747+
const r = runRaw(
748+
{ required: HEALTHY, lanes: HEALTHY.map((n) => LANE(n)), headSha: HEAD_3276 },
749+
ON(),
750+
);
692751
assert.equal(r.code, 1, r.output);
693752
assert.match(r.output, /NO_REVIEWS/);
694753
assert.doesNotMatch(r.output, /No reviewer claims a verdict/);
695754
});
696755

697756
test('FAIL CLOSED: a state file that omits `headSha` gets NO_HEAD_SHA', () => {
698-
const r = runRaw({ required: HEALTHY, lanes: HEALTHY.map((n) => LANE(n)), reviews: [] });
757+
const r = runRaw(
758+
{ required: HEALTHY, lanes: HEALTHY.map((n) => LANE(n)), reviews: [] },
759+
ON(),
760+
);
699761
assert.equal(r.code, 1, r.output);
700762
assert.match(r.output, /NO_HEAD_SHA/);
701763
assert.doesNotMatch(r.output, /No reviewer claims a verdict/);
702764
});
703765

704766
test('FAIL CLOSED: an unreadable `commit_id` refuses instead of comparing an older review', () => {
705-
const r = run({ ...STATE_3276(OLD_3276), reviews: [REVIEW_3276('c26e453d')] });
767+
const r = run({ ...STATE_3276(OLD_3276), reviews: [REVIEW_3276('c26e453d')] }, ON());
706768
assert.equal(r.code, 1, r.output);
707769
assert.match(r.output, /UNREADABLE_COMMIT_ID/);
708770
});
@@ -748,6 +810,39 @@ test('FAIL CLOSED: an empty or malformed `reviewAuthors` refuses', () => {
748810
assert.match(l.output, /needs a non-empty `login`/);
749811
});
750812

813+
test('WIRING: both API walks are paginated AND flattened through the refusing helper', () => {
814+
// NOT reachable through `--state-file`, which is exactly why it is asserted
815+
// over the source. A mutation that replaced `flattenCheckRunPages(pages, …)`
816+
// with an inline `pages.flatMap((p) => p.check_runs ?? [])` survived the
817+
// whole suite: the helper's refusal is tested, its USE was not. Both walks
818+
// are covered, because the same hole existed on the reviews side.
819+
const src = readFileSync(GATE, 'utf8');
820+
const fn = (name) => {
821+
const at = src.indexOf(`function ${name}(`);
822+
assert.ok(at > 0, `${name} exists`);
823+
return src.slice(at, src.indexOf('\n}\n', at));
824+
};
825+
for (const [name, helper] of [
826+
['fetchCheckRunDescriptions', 'flattenCheckRunPages'],
827+
['fetchReviews', 'flattenReviewPages'],
828+
]) {
829+
// Comments in these bodies legitimately NAME the fields, so compare code.
830+
const body = fn(name)
831+
.split('\n')
832+
.filter((l) => !l.trim().startsWith('//'))
833+
.join('\n');
834+
// A truncated walk drops rows silently — a false NEGATIVE for part 2 and a
835+
// false POSITIVE for part 3 — so neither may rest on a `per_page` guess.
836+
assert.match(body, /'--paginate'/, `${name} must paginate`);
837+
// `--slurp` is what keeps the page boundaries visible, and it is the only
838+
// reason the helper can tell a short page from a stopped walk.
839+
assert.match(body, /'--slurp'/, `${name} must slurp`);
840+
assert.match(body, new RegExp(`${helper}\\(`), `${name} must flatten via ${helper}`);
841+
// …and must not hand-roll the flatten around it.
842+
assert.doesNotMatch(body, /flatMap|\.check_runs\b/, `${name} must not inline the flatten`);
843+
}
844+
});
845+
751846
test('POLICY: `all-authors` is reachable through the config and changes the answer', () => {
752847
// Proves the knob is wired end to end rather than merely validated: the same
753848
// state file, clean under the default, is a finding under `all-authors`.
@@ -766,7 +861,10 @@ test('POLICY: `all-authors` is reachable through the config and changes the answ
766861
],
767862
headSha: HEAD_3276,
768863
};
769-
const dflt = run(state);
864+
// Clean under `claimed-verdict` (no configured context claims a verdict) —
865+
// compared against `claimed-verdict` explicitly, not against the shipped
866+
// `off`, which would make the contrast trivially true for the wrong reason.
867+
const dflt = run(state, ON());
770868
assert.doesNotMatch(dflt.output, /STALE_REVIEW/);
771869

772870
const all = run(state, ['--config', cfgWith({ staleReviewPolicy: 'all-authors' }, 'all')]);

0 commit comments

Comments
 (0)