mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
* 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.
354 lines
12 KiB
JavaScript
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));
|
|
}
|