Files
qwen-code/scripts/verify-pr-guard.mjs
T
Shaojin Wen 21c8b594db feat(verify-pr): attribute failures, close the publish guard, enumerate coverage (#15764)
* feat(verify-pr): attribute failures, close the publish guard, enumerate coverage

Three gaps measured on a maintainer round, each a deterministic rule that
lived only in prose and failed silently.

The counting rule was taught in two halves that never met: one paragraph
showed how to prove a red environmental with an A/A control, another said
expected reds are passes. Nothing joined them, so a host-environment red
had no category. The round booked the same repository test twice — head
and its base control — and published fail: 2 with findings while its own
verdict line said neither reason was a defect in the PR. The base cell
asserts that base fails; it failed as predicted, so it passed. assertions
gains an optional failures ledger attributing each red to pr, harness or
environment, and the checker refuses findings or blocked when nothing is
attributed to the PR. The verdict vocabulary stays at four words.

The pre-publish guard asked "anything new?", and a failed fetch returns
zero items, so the answer was "no". Measured 324 review comments down to
0 on a retry that never reached the API. It was the only local-path
safety rule with no script behind it. The guard now owns both halves: a
failed call is fatal and writes no file, and the compare side independently
treats a short list, a vanished id, a moved head or a changed state as
dirty. No retry flag — a silent retry budget hides a machine that cannot
reach the API.

"Counts are sacred" bound only numbers. A universally-quantified coverage
claim over an enumerable set was asserted, not generated, and two
production commits had no mutation row. mutations.json now carries the
claim and the enumeration it is about, and the checker names any listed
commit no row covers.

Verified: 230 pin tests across the three suites, an 11-row vacuity matrix
with 11 killed including a control on a pre-existing guard, and a real
round re-encoded from findings to merge-ready at 548/548. Three of the
matrix rows exist because self-audit found defects in this change itself.

* fix(verify-pr): summarize the ids a short list lost

Driving the new guard against a live PR to capture evidence for this change
showed it printing the short-list reason and then 321 "disappeared" lines.
The one actionable line was buried under its own echoes, which is the same
failure the guard exists to prevent: a maintainer who cannot read the output
cannot act on it.

The absent ids are a consequence of the truncation, not independent reasons,
so a short list now reports its reason plus a count. At equal length the
missing id is the whole signal and is still named — a second pin keeps the
suppression from swallowing a genuine deletion, and a vacuity pair proves
both halves (2 rows, 2 killed).

* fix(verify-pr): enumerate the branch's own commits, and ask for the files the checks read

A second audit pass over this change found five defects in it, two of which
defeated the rules the change exists to add.

The coverage rule told the round to enumerate with `git log --no-merges`, which
drops a merge commit but keeps every commit that merge brought in. On the very
branch whose wrong claim motivated the rule, that recipe lists 85 production
commits where `--first-parent` lists 24; the 61 extra come from one merge of
main. A pin was defending the wrong command. The recipe now carries
`--first-parent`, explains why the other flag does not substitute for it, and
quotes the measurement.

The checker read `mutations.json` and the failures ledger, but the artifact
contract never asked for either, so a round following the contract would
produce neither and both new checks would sit silent. The contract now names
both, with a pin per file.

A per-commit claim over an empty `productionCommits` list passed: it is
vacuously true, and the empty list is the one incomplete enumeration that
satisfies every other check. It is now refused, while making no claim stays
silent so a docs-only round is unaffected.

Also corrected a cross-reference to a report item that does not exist, and
dropped a `warnings` return value no caller could ever observe.

The vacuity matrix was rebuilt to cover every guard this change adds: 25 rows,
25 killed, including a control row on a pre-existing guard. Each row carries
the commit that introduced its clause, verified against that commit's blob and
its parent's, so this change's own per-commit coverage claim is checked rather
than asserted.

* fix(verify-pr): ask for the mutation matrix only when a round ran one

The artifact contract listed it unconditionally while the checker treats it as
optional, so a documentation-only round with no matrix was told to produce a
file it has nothing to put in. The ask is now conditional on having run
mutations, which is what closes the gap: a round that ran them must write them
down.

* docs(design): count the third-pass finding in the self-audit enumeration

The validation section enumerates what self-audit found in this change, so
leaving one out is the same incomplete enumeration the change is about.

* docs(design): report the matrix as per-guard, which is what it is

Applying the enumeration this change teaches to the change itself names
docs-only commits that carry no mutation row, and the checker refuses a
per-commit claim over them. Narrowing the claim is the honest response; the
refusal is recorded as the rule catching its own author.

* fix(verify-pr): the per-commit claim is earned by the enumeration

The paragraph told every round to record coverageClaim "per-commit", which is
the same overstatement the artifact contract had: a round whose enumeration has
an uncovered commit would be instructed to assert coverage it does not have.
The claim is now conditional on every enumerated commit having a row, and a
round with a gap reports per-guard and names what it did not reach. Pinned, and
added as a vacuity row.

* fix(verify-pr): a review edited without its stamp moving is still an edit

The guard compared ids and timestamps, and a review carries submitted_at
with no updated_at: editing its body moves neither, so a CHANGES_REQUESTED
review rewritten to LGTM compared clean. The snapshot now carries each
item's body next to its stamp and compares both, and refuses a fetched
list whose items have no body rather than reading it as unchanged.

* fix(verify-pr): a dismissed review moves neither stamp nor body

On PR 15388 review 5673309455 was submitted CHANGES_REQUESTED and dismissed
five and a half hours later with its 4838-character body and submitted_at
unchanged; only state moved, and that dismissal unblocked the merge gate.
The snapshot now carries each review's state beside its body and stamp, and
refuses a review list without one. The compared fields are the ones that
change what a reviewer said or decided; reaction counts stay out, so a
thumbs-up is not a dirty round.

* docs(design): cite what the dismissal record actually proves

The dismissed-review note claimed its 4838-character body was unchanged.
The API records only state and dismissal_message for a dismissal, so the
body could have been edited too and nothing would show it. What the record
does prove is that submitted_at still reads the submission time 5.5 hours
before the dismissal, which is why state is the field that has to be
compared.

* docs(verify-pr): say which refusal applies to which list

A comment list has no state, so 'refuses a fetched list missing either'
overstated what the guard refuses. It refuses an item with no body, and a
review with no state, and the sentence now says that; 'a changed state' in
the same paragraph becomes 'a changed PR state' so it cannot be read as the
review state the snapshot now compares.

* docs(design): finish the self-audit enumeration and scope the refusal

The design doc repeated the scope error the skill just dropped: a comment
item has no state to be missing. And the enumeration of findings fixed
without a row stopped before the last two corrections, so the count of
self-audit work was short of what the branch carries.

* docs(design): state the interval the record shows, not a rounded one

19:47:16Z to 01:22:57Z is five hours and thirty-six minutes. 'Five and a
half hours' understates it by six minutes, which is the sort of figure a
reader checks.

* refactor(verify-pr): LISTS has no consumer outside the guard

It was exported and nothing imported it, which is the same shape as the
unobserved return value an earlier pass removed.

* docs(design): enumerate the non-guard findings instead of counting them

'Five further findings' is a count over a set that has grown twice since
it was written, which is the failure this change exists to name. The list is
now the list, and it carries the three findings that landed after it.

* docs(verify-pr): name the matrix the survivors belong to

Inserting the coverage rule between 'pinned / not-pinned' and 'Survivors are
not noise' left the second sentence opening a paragraph with no antecedent.
It now says which matrix.

* docs(verify-pr): keep the no-projected-entries rule with the rule it belongs to

The coverage sentence was inserted between 'maps to a check that ran' and the
prohibition that continues it, so the prohibition read as part of the aside.

* fix(verify-pr): subtract the target instead of cutting at the first parent

--first-parent removes the over-count from a main merge by discarding every
second parent, which also discards the PR's own commits when it merged one of
its own feature branches: on a four-commit graph with one feature merge and
one main merge, --no-merges lists 5 (2 of them main's), --first-parent lists
1 (dropping the 2 the PR wrote), and --not <target> lists the right 3. The
undercount is worse than the overcount, because a complete-looking enumeration
lets the checker accept a claim over commits that have no row.

--not is a live-round recipe: it subtracts what the target already contains,
so a branch that has landed enumerates empty. Measured on this change's own
merged range, 85 / 24 / 0 for the three recipes, and 15 / 15 / 15 on its live
one. The rule now carries that precondition, and the empty-enumeration refusal
is what catches a round that runs it too late.

* fix(verify-pr): the refusal recommends the recipe this change now teaches

The empty-enumeration refusal still told the round to run
`git log --first-parent --no-merges`, which the skill now documents as
dropping the PR's own feature-merge commits, and a test pinned that advice.
A round that hits the refusal, follows the hint to the under-counting recipe,
can then file a per-commit claim the checker accepts, because the omitted
commits are simply not in the list. Message, doc comment and pin now name
`git log --no-merges <previous>..<head> --not <target>`, and the pin forbids
the old form.

* docs(design): catch the matrix counts up with the rows that exist

The two numbers in the design docs had drifted three revisions behind the
body and the comment (32/16 against 35/19). The claim checker now parses all
four surfaces, so the next drift fails a check instead of surviving.
2026-10-28 11:57:28 +00:00

354 lines
12 KiB
JavaScript

#!/usr/bin/env node
/**
* @license
* Copyright 2026 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/
/**
* The pre-publish guard for a local `/verify-pr` round.
*
* The skill asks the round to snapshot every review, review comment and issue
* comment at the start, re-fetch immediately before posting, and abort on
* anything new or edited. Hand-run, that rule fails open: a re-fetch that errors
* returns no items, "is anything new?" answers "no", and the guard reads clean.
* A round measured exactly that — 324 review comments at the start, 0 on the
* re-fetch, because the call failed rather than the PR going quiet.
*
* So this script owns both halves. A failed fetch is fatal and writes no
* snapshot, and a comparison treats a missing or short list as dirty. Neither
* path can produce a clean verdict from an unanswered question.
*
* Usage:
* node verify-pr-guard.mjs --pr <n> --repo <owner>/<repo> --snapshot <file>
* node verify-pr-guard.mjs --compare --start <file> --now <file>
*
* Exit codes: 0 clean / snapshot written, 1 dirty or fetch failed, 2 usage.
*/
import { spawnSync } from 'node:child_process';
import { readFileSync, writeFileSync } from 'node:fs';
import { resolve } from 'node:path';
import { isEntryPoint } from './lib/entry-point.js';
/**
* The three lists the guard covers; `--paginate` or only the first 30 arrive.
* The third column says whether the list's items carry a review state, which
* only reviews do and which a dismissal changes on its own.
*/
const LISTS = [
['reviews', (pr) => `repos/{repo}/pulls/${pr}/reviews`, true],
['reviewComments', (pr) => `repos/{repo}/pulls/${pr}/comments`, false],
['issueComments', (pr) => `repos/{repo}/issues/${pr}/comments`, false],
];
const HELP = `Usage:
node scripts/verify-pr-guard.mjs --pr <n> --repo <owner>/<repo> --snapshot <file>
node scripts/verify-pr-guard.mjs --compare --start <file> --now <file>
--snapshot fetches the head, the PR state and all three comment lists with
--paginate, and writes them only when every call succeeded. A failed call exits
1 and leaves no file, so a retry cannot be mistaken for a snapshot.
--compare is fail-closed: a malformed snapshot, a moved head, a changed state, a
new or edited item, a vanished item, or a list shorter than the start snapshot
all report dirty. A short list is a failed fetch, not a quiet PR. Edits are read
from the body and the review state as well as the timestamp, because neither
editing nor dismissing a review moves a timestamp. Passing the same file as both
sides is a usage error, not a clean round.
Exit codes: 0 clean / written, 1 dirty or fetch failed, 2 usage.
`;
/**
* Reviews carry `submitted_at`, not `updated_at`, so reading one field name
* across all three lists leaves one of them comparing `undefined` with
* `undefined`. Neither stamp moves when a review body is edited, or when a
* review is dismissed — measured against this repo's API — so the body and the
* review state travel with the stamp and are compared too. These are the
* fields that change what a reviewer said or decided; reaction counts and
* other metadata are deliberately left out, so a thumbs-up is not a dirty
* round.
*/
function stampOf(item) {
return item?.updated_at ?? item?.submitted_at ?? null;
}
function normalize(items, carriesState) {
return items.map((item) => ({
id: item.id,
stamp: stampOf(item),
body: item.body,
...(carriesState ? { state: item.state } : {}),
}));
}
/**
* Fetches every list. `run` is injected so tests can drive the failure paths
* without a network; it returns a spawnSync-shaped result.
*/
export function fetchSnapshot({ pr, repo, run }) {
const errors = [];
const call = (args, label) => {
const result = run(args);
if (result.status !== 0) {
errors.push(
`${label}: gh exited ${result.status ?? 'null'}${result.stderr ? ` — ${result.stderr.trim().split('\n')[0]}` : ''}`,
);
return null;
}
try {
return JSON.parse(result.stdout);
} catch {
errors.push(`${label}: response was not JSON`);
return null;
}
};
const meta = call(
['pr', 'view', String(pr), '--repo', repo, '--json', 'headRefOid,state'],
'pr view',
);
const snapshot = {
pr,
repo,
headRefOid: meta?.headRefOid ?? null,
state: meta?.state ?? null,
};
for (const [key, path, carriesState] of LISTS) {
// `--paginate` merges array pages into one JSON array (verified on gh
// 2.103.0: 17 comments over 4 pages parsed as a single array). `--slurp`
// would instead nest one array per page and break the normalize below.
const items = call(
['api', '--paginate', path(pr).replace('{repo}', repo)],
key,
);
if (items === null) continue;
if (!Array.isArray(items)) {
errors.push(`${key}: expected an array, got ${typeof items}`);
continue;
}
snapshot[key] = normalize(items, carriesState);
}
if (errors.length > 0) return { ok: false, errors, snapshot: null };
if (!isSnapshot(snapshot)) {
return {
ok: false,
errors: [
'fetched snapshot failed validation: a head OID, a state, or an id, body or review state in one of the lists is missing or malformed',
],
snapshot: null,
};
}
return { ok: true, errors: [], snapshot };
}
/** A snapshot is only usable when every list is present and well-formed. */
export function isSnapshot(value) {
if (value === null || typeof value !== 'object' || Array.isArray(value)) {
return false;
}
if (!Number.isInteger(value.pr) || value.pr <= 0) return false;
if (typeof value.repo !== 'string' || !value.repo.includes('/')) return false;
if (!/^[0-9a-f]{40}$/.test(value.headRefOid ?? '')) return false;
if (typeof value.state !== 'string' || value.state === '') return false;
return LISTS.every(([key, , carriesState]) => {
const list = value[key];
return (
Array.isArray(list) &&
list.every(
(item) =>
item &&
Number.isInteger(item.id) &&
(item.stamp === null || typeof item.stamp === 'string') &&
// A list that cannot answer "was anything edited?" or "was a review
// dismissed?" is refused, not compared as if the answer were no.
typeof item.body === 'string' &&
(carriesState
? typeof item.state === 'string'
: item.state === undefined),
)
);
});
}
/**
* Compares two snapshots. Every reason is reported, not just the first: the
* round has to read what arrived and fold it in, so a truncated list of
* reasons costs another fetch.
*/
export function compareSnapshots(start, now) {
const reasons = [];
for (const [label, snapshot] of [
['start', start],
['now', now],
]) {
if (!isSnapshot(snapshot)) {
reasons.push(`${label} snapshot is missing or malformed`);
}
}
if (reasons.length > 0) return { clean: false, reasons };
if (start.pr !== now.pr || start.repo !== now.repo) {
reasons.push(
`snapshots describe different PRs: ${start.repo}#${start.pr} vs ${now.repo}#${now.pr}`,
);
}
if (start.headRefOid !== now.headRefOid) {
reasons.push(`head moved ${start.headRefOid} -> ${now.headRefOid}`);
}
if (start.state !== now.state) {
reasons.push(`PR state changed ${start.state} -> ${now.state}`);
}
for (const [key] of LISTS) {
const before = start[key];
const after = now[key];
const short = after.length < before.length;
if (short) {
reasons.push(
`${key}: re-fetch returned ${after.length} of ${before.length} items — a short list is a failed fetch, not a quiet PR`,
);
}
const byId = new Map(after.map((item) => [item.id, item]));
const startIds = new Set(before.map((item) => item.id));
let missing = 0;
for (const item of before) {
const seen = byId.get(item.id);
if (!seen) {
// A short list already says the fetch was truncated. Naming every
// absent id buries that one actionable line under hundreds of echoes.
if (short) missing++;
else reasons.push(`${key}: ${item.id} disappeared`);
} else if (seen.stamp !== item.stamp) {
reasons.push(
`${key}: ${item.id} was edited (${item.stamp} -> ${seen.stamp})`,
);
} else if (seen.body !== item.body) {
reasons.push(`${key}: ${item.id} was edited (body changed)`);
} else if (seen.state !== item.state) {
reasons.push(
`${key}: ${item.id} changed state (${item.state} -> ${seen.state})`,
);
}
}
if (missing > 0) {
reasons.push(
`${key}: ${missing} ids absent from the re-fetch (implied by the short list)`,
);
}
for (const item of after) {
if (!startIds.has(item.id)) reasons.push(`${key}: ${item.id} is new`);
}
}
return { clean: reasons.length === 0, reasons };
}
function parseArgs(argv) {
const out = {
pr: null,
repo: null,
snapshot: null,
compare: false,
start: null,
now: null,
};
for (let i = 0; i < argv.length; i++) {
const arg = argv[i];
if (arg === '--compare') out.compare = true;
else if (arg === '--pr') out.pr = Number(argv[++i]);
else if (arg === '--repo') out.repo = argv[++i];
else if (arg === '--snapshot') out.snapshot = argv[++i];
else if (arg === '--start') out.start = argv[++i];
else if (arg === '--now') out.now = argv[++i];
}
return out;
}
function readSnapshot(path) {
try {
return JSON.parse(readFileSync(path, 'utf8'));
} catch {
return null;
}
}
function main(argv) {
if (argv.includes('--help') || argv.includes('-h')) {
process.stdout.write(HELP);
return 0;
}
const args = parseArgs(argv);
if (args.compare) {
if (!args.start || !args.now) {
process.stderr.write(
`verify-pr guard: --compare needs --start and --now\n\n${HELP}`,
);
return 2;
}
// Comparing a snapshot with itself answers nothing about what landed, and
// a copy-pasted path would otherwise report the cleanest possible round.
if (resolve(args.start) === resolve(args.now)) {
process.stderr.write(
'verify-pr guard: --start and --now are the same file; re-fetch into a second path before comparing\n',
);
return 2;
}
const { clean, reasons } = compareSnapshots(
readSnapshot(args.start),
readSnapshot(args.now),
);
const lines = [`verify-pr guard: ${clean ? 'clean' : 'dirty'}`];
for (const reason of reasons) lines.push(` ${reason}`);
if (!clean) {
lines.push(
' read what arrived, fold it into the report, refresh the snapshot, then re-run the guard',
);
}
process.stdout.write(`${lines.join('\n')}\n`);
return clean ? 0 : 1;
}
if (
!Number.isInteger(args.pr) ||
args.pr <= 0 ||
!args.repo ||
!args.snapshot
) {
process.stderr.write(
`verify-pr guard: --pr, --repo and --snapshot are required\n\n${HELP}`,
);
return 2;
}
const run = (ghArgs) =>
spawnSync('gh', ghArgs, { encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 });
const { ok, errors, snapshot } = fetchSnapshot({
pr: args.pr,
repo: args.repo,
run,
});
if (!ok) {
process.stderr.write(
`verify-pr guard: snapshot refused, nothing written\n${errors.map((e) => ` ${e}\n`).join('')}`,
);
return 1;
}
writeFileSync(args.snapshot, `${JSON.stringify(snapshot, null, 2)}\n`);
const counts = LISTS.map(([key]) => `${key} ${snapshot[key].length}`).join(
', ',
);
process.stdout.write(
`verify-pr guard: wrote ${args.snapshot} (${counts}) at ${snapshot.headRefOid}\n`,
);
return 0;
}
if (isEntryPoint(import.meta.url, process.argv[1])) {
process.exitCode = main(process.argv.slice(2));
}