From 58ce73619c28f39dd0b960a102b6c21f807a1948 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 12:56:05 +0000 Subject: [PATCH] =?UTF-8?q?fix(ci):=20check-adr-0087-registration=20?= =?UTF-8?q?=E7=9A=84=20CLI=20=E6=B4=BE=E5=8F=91=E5=8A=A0=E5=85=A5=E5=85=A5?= =?UTF-8?q?=E5=8F=A3=E5=AE=88=E5=8D=AB,import=20=E4=B8=8D=E5=86=8D?= =?UTF-8?q?=E8=B7=91=E9=97=A8=E7=A6=81=20(#6566)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scripts/check-adr-0087-registration.mjs` 在模块顶层读 `process.argv` 并直接 派发(`--self-test` / `--list` / `--audit-stock` / 门禁本体),没有比较 `import.meta.url` 与 `process.argv[1]` 的入口守卫。后果是:任何一次 `import` 都会真的把门禁跑在**调用方的仓库**上,判红时六个 `process.exit(1)` 里的一个还会直接掐死调用方进程 —— 调用方自己的代码一行都执行不到。 改为 `objectui-changeset-digest.mjs` 已有的写法:仅当本文件是进程入口时才 派发。`argv` / `readFlag` 只被派发用到,一并收进守卫块内;模块顶层此后只剩 纯常量计算(`REPO_ROOT` 由 `__dirname` 推出,不外调 git),import 无副作用。 判定逻辑、词表、分支行为、`--self-test` 的位置一概未动。CLI 输出与退出码 逐字节比对 origin/main 基线:`--list`、`--audit-stock`、门禁本体、 `--base <不存在的 ref>` 四条路径 stdout/stderr 全等,退出码全等。 自测新增 I1/I2 两组共 7 条断言(100 → 107): - I1:在一个**判红**的临时仓库里 `import` 本脚本的纯导出,必须不跑门禁、 不打印判词、不改退出码; - I2:同一个 fixture 下把同一份文件当入口跑,门禁仍判红并 exit 1,`--list` 仍打表并 exit 0。 反向验证(两个方向都实测):去掉守卫恢复顶层派发 → I1 三条全红、I2 不动; 把 CLI 块从被拷贝的脚本里删掉 → I2 四条里红三条、I1 不动。第四条 (`--list` 退出码为 0)在后一种消融下仍为绿 —— 没有 CLI 的脚本同样退出 0, 退出码分不清「分支跑了且成功」和「什么都没跑」,故每条退出码断言都配了一 条输出断言。 顺带修正 `objectui-changeset-digest.mjs` 里一处**注释**:它原文断言本脚本 「CLI dispatch runs at TOP LEVEL (it has no `import.meta.url === argv[1]` guard)」,本 PR 之后该陈述为假。只改注释,不动代码。 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZgRyPVwi1jLb1mNNuUQ9o --- scripts/check-adr-0087-registration.mjs | 207 +++++++++++++++++------- scripts/objectui-changeset-digest.mjs | 12 +- 2 files changed, 153 insertions(+), 66 deletions(-) diff --git a/scripts/check-adr-0087-registration.mjs b/scripts/check-adr-0087-registration.mjs index 58c1e7ca36..bf3fd6887f 100644 --- a/scripts/check-adr-0087-registration.mjs +++ b/scripts/check-adr-0087-registration.mjs @@ -153,8 +153,8 @@ // `pnpm install` -- the same constraint its neighbours in the Check Changeset job // carry. -import { execFileSync } from 'node:child_process'; -import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -2032,6 +2032,79 @@ function selfTest() { assert(!breakingDeclaration(parseChangeset(CS({ bumps: [['a', 'patch']], body: 'plain\n' }))).breaking, 'P7: a plain patch is not'); assert(extractIds(" id: 'object-titleFormat-to-nameField',\n").length === 1, 'P8: an id with a capital letter must be extracted'); + // ---- I1 (#6566): a bare `import` of this module must NOT run the gate ----- + // The CLI dispatch at the bottom of this file is entry-guarded; importing the + // module for its pure exports (readDisposition, extractIds, ...) must be + // side-effect-free. The fixture is a RED-verdict repo (a declared-breaking + // changeset, no marker) holding a copy of this very script, and the importer + // is a child process whose only code borrows one pure function. Delete the + // entry guard and all three assertions go red: the imported copy judges the + // fixture repo, prints its verdict, `process.exit(1)`s, and the importer's + // own line never runs -- the behaviour measured in #6566, which forced + // PR #6556 into a subprocess fixture instead of an import. + { + const { dir, base } = mk({ + files: { '.changeset/unanswered-breaking.md': CS({ body: '**BREAKING** x\n\nno marker here\n' }) }, + }); + // Park `main` back at BASE with the breaking changeset ahead of it on `work`: + // an unguarded import default-resolves base to `main` and must judge the + // changeset RED -- the caller-killing shape #6566 measured. With both commits + // on `main` the ablated gate would see an empty diff and merely print a green + // verdict, pinning the output leak but not the exit(1). + git(['checkout', '-q', '-b', 'work'], dir); + git(['branch', '-f', 'main', base], dir); + const w = (rel, text) => { + mkdirSync(dirname(join(dir, rel)), { recursive: true }); + writeFileSync(join(dir, rel), text); + }; + const copy = 'scripts/check-adr-0087-registration.mjs'; + w(copy, readFileSync(fileURLToPath(import.meta.url), 'utf8')); + w( + 'importer.mjs', + "import { readDisposition } from './scripts/check-adr-0087-registration.mjs';\n" + + "console.log('IMPORTER_OWN_LINE', JSON.stringify(readDisposition('nothing here')));\n", + ); + const r = spawnSync(process.execPath, [join(dir, 'importer.mjs')], { cwd: dir, encoding: 'utf8' }); + const all = `${r.stdout}\n${r.stderr}`; + assert(r.status === 0, `I1: a bare import must not adopt the gate's exit code (got ${r.status})\n${all}`); + assert( + r.stdout.includes('IMPORTER_OWN_LINE {"ok":false'), + `I1: the importer's own code must run and receive the export\n${all}`, + ); + assert(!all.includes('check-adr-0087-registration:'), `I1: importing must not print a gate verdict\n${all}`); + + // ---- I2 (#6566): the SAME file, run as the entry point, still dispatches -- + // I1 alone is satisfied by deleting the CLI block outright -- it only ever + // asserts that nothing happens. I2 is its other half over the very same + // fixture: the guard must let the entry point through, on both a branch that + // exits non-zero and one that does not. Measured both ways: restore the + // top-level dispatch and I1's three go red with I2 untouched; strip the CLI + // block from the copied script and three of I2's four go red with I1 + // untouched. + // + // Why each exit-code assertion is PAIRED with an output one: the fourth, + // `--list` exiting 0, stayed green under that second ablation -- a script + // with no CLI at all also exits 0. An exit code cannot distinguish "the + // branch ran and succeeded" from "nothing ran"; only the printed table can. + const cli = (...args) => spawnSync(process.execPath, [join(dir, copy), ...args], { cwd: dir, encoding: 'utf8' }); + + const gate = cli(); + assert(gate.status === 1, `I2: the entry point must still run the gate and exit 1 on red (got ${gate.status})\n${gate.stdout}\n${gate.stderr}`); + assert( + gate.stderr.includes('unanswered-breaking.md') && gate.stderr.includes('no `adr-0087:` disposition marker'), + // The EXACT verdict, not merely "it is red": an entry point that died for + // an unrelated reason also exits 1, and would pin nothing about dispatch. + `I2: the entry point must report the real verdict\n${gate.stdout}\n${gate.stderr}`, + ); + + const listed = cli('--list'); + assert(listed.status === 0, `I2: --list must still dispatch and exit 0 (got ${listed.status})\n${listed.stdout}\n${listed.stderr}`); + assert( + listed.stdout.includes('declared-breaking changeset(s) in stock'), + `I2: --list must still print its table\n${listed.stdout}\n${listed.stderr}`, + ); + } + for (const d of cleanup) rmSync(d, { recursive: true, force: true }); if (failures.length) { @@ -2045,72 +2118,84 @@ function selfTest() { // --------------------------------------------------------------------------- // CLI // --------------------------------------------------------------------------- +// +// Entry-guarded (#6566): the dispatch below runs ONLY when this file is the +// process entry point (the `objectui-changeset-digest.mjs` pattern). No branch +// is a no-op and four of them `process.exit(1)` on red, so an unguarded top +// level made `import` mean "run the gate against the IMPORTER's repo and adopt +// its verdict as my exit code" -- PR #6556 paid a ~40-line subprocess fixture +// to avoid exactly that. The exported pure functions (readDisposition, +// extractIds, ...) are the module's import surface; the CLI is this block, and +// the I1/I2 self-test assertions pin BOTH halves of the separation -- silent as +// an import, unchanged as an entry point. + +if (resolve(process.argv[1] ?? '') === resolve(fileURLToPath(import.meta.url))) { + const argv = process.argv.slice(2); + const readFlag = (name) => { + const i = argv.indexOf(name); + return i >= 0 ? argv[i + 1] : undefined; + }; -const argv = process.argv.slice(2); -const readFlag = (name) => { - const i = argv.indexOf(name); - return i >= 0 ? argv[i + 1] : undefined; -}; - -if (argv.includes('--self-test')) { - selfTest(); -} else if (argv.includes('--list')) { - list(REPO_ROOT, 'HEAD'); -} else if (argv.includes('--audit-stock')) { - auditStock(REPO_ROOT, readFlag('--head') ?? 'HEAD'); -} else { - const head = readFlag('--head') ?? 'HEAD'; - const requested = readFlag('--base'); - let base = requested; - if (base) { - if (!resolveCommit(base, REPO_ROOT)) { - console.error(`⛔ check-adr-0087-registration: --base '${base}' does not resolve to a commit.`); - console.error(' A base that cannot be resolved is a failure, never a pass (#4690).'); - process.exit(1); - } + if (argv.includes('--self-test')) { + selfTest(); + } else if (argv.includes('--list')) { + list(REPO_ROOT, 'HEAD'); + } else if (argv.includes('--audit-stock')) { + auditStock(REPO_ROOT, readFlag('--head') ?? 'HEAD'); } else { - base = ['origin/main', 'main'].find((r) => resolveCommit(r, REPO_ROOT)); - if (!base) { - console.error('⛔ check-adr-0087-registration: neither origin/main nor main resolves in this checkout.'); - console.error(' Pass one explicitly: --base . Missing input is a failure, never a pass (#4690).'); - process.exit(1); + const head = readFlag('--head') ?? 'HEAD'; + const requested = readFlag('--base'); + let base = requested; + if (base) { + if (!resolveCommit(base, REPO_ROOT)) { + console.error(`⛔ check-adr-0087-registration: --base '${base}' does not resolve to a commit.`); + console.error(' A base that cannot be resolved is a failure, never a pass (#4690).'); + process.exit(1); + } + } else { + base = ['origin/main', 'main'].find((r) => resolveCommit(r, REPO_ROOT)); + if (!base) { + console.error('⛔ check-adr-0087-registration: neither origin/main nor main resolves in this checkout.'); + console.error(' Pass one explicitly: --base . Missing input is a failure, never a pass (#4690).'); + process.exit(1); + } } - } - const inputProblems = assertInputs({ cwd: REPO_ROOT, head }); - if (inputProblems.length > 0) { - console.error(`\n✗ check-adr-0087-registration: ${inputProblems.length} input problem(s) -- refusing to report a verdict.\n`); - for (const p of inputProblems) console.error(` • ${p}\n`); - console.error(' A gate that cannot find its input and exits 0 is worse than no gate (#4690).'); - process.exit(1); - } + const inputProblems = assertInputs({ cwd: REPO_ROOT, head }); + if (inputProblems.length > 0) { + console.error(`\n✗ check-adr-0087-registration: ${inputProblems.length} input problem(s) -- refusing to report a verdict.\n`); + for (const p of inputProblems) console.error(` • ${p}\n`); + console.error(' A gate that cannot find its input and exits 0 is worse than no gate (#4690).'); + process.exit(1); + } - let result; - try { - result = scan({ cwd: REPO_ROOT, base, head }); - } catch (e) { - console.error(`⛔ check-adr-0087-registration: ${e.message}`); - process.exit(1); - } + let result; + try { + result = scan({ cwd: REPO_ROOT, base, head }); + } catch (e) { + console.error(`⛔ check-adr-0087-registration: ${e.message}`); + process.exit(1); + } - if (result.problems.length > 0) { - report(result.problems); - process.exit(1); - } + if (result.problems.length > 0) { + report(result.problems); + process.exit(1); + } - const n = result.judged.length; - if (n === 0) { - console.log(`✓ check-adr-0087-registration: this PR adds no declared-breaking changeset (${result.skipped.length} non-breaking changeset(s) seen).`); - } else { - console.log(`✓ check-adr-0087-registration: ${n} declared-breaking changeset(s), each carrying an ADR-0087 disposition.`); - for (const j of result.judged) { - const what = j.verdict === 'registered' ? `registered ${j.ids.join(', ')} (new here: ${j.fresh.join(', ')})` : `not-required (${j.category})`; - console.log(` ${j.file} [${j.signals.join('+')}] ${what}`); - if (j.why) { - // Every exemption is printed AND annotated, on every run: an exemption - // nobody re-reads is the allow-list failure mode this gate exists to avoid. - console.log(` reason: ${j.why}`); - console.log(`::notice file=${j.file}::ADR-0087 exemption (${j.category}): ${j.why}`); + const n = result.judged.length; + if (n === 0) { + console.log(`✓ check-adr-0087-registration: this PR adds no declared-breaking changeset (${result.skipped.length} non-breaking changeset(s) seen).`); + } else { + console.log(`✓ check-adr-0087-registration: ${n} declared-breaking changeset(s), each carrying an ADR-0087 disposition.`); + for (const j of result.judged) { + const what = j.verdict === 'registered' ? `registered ${j.ids.join(', ')} (new here: ${j.fresh.join(', ')})` : `not-required (${j.category})`; + console.log(` ${j.file} [${j.signals.join('+')}] ${what}`); + if (j.why) { + // Every exemption is printed AND annotated, on every run: an exemption + // nobody re-reads is the allow-list failure mode this gate exists to avoid. + console.log(` reason: ${j.why}`); + console.log(`::notice file=${j.file}::ADR-0087 exemption (${j.category}): ${j.why}`); + } } } } diff --git a/scripts/objectui-changeset-digest.mjs b/scripts/objectui-changeset-digest.mjs index 2ebdc81519..99d079ffd1 100644 --- a/scripts/objectui-changeset-digest.mjs +++ b/scripts/objectui-changeset-digest.mjs @@ -419,11 +419,13 @@ export const ADR_0087_SCAFFOLD = * `/\*\*BREAKING/` matches it. Reading the artifact closes that by construction * instead of by agreement between two predicates. * - * Why the gate's own function is not imported: that module's CLI dispatch runs - * at TOP LEVEL (it has no `import.meta.url === argv[1]` guard), so importing it - * would execute the gate; and it would put the release-critical bump path one - * rename away from failing. The agreement is pinned in the self-test instead, - * which runs the real gate as a child process over a real temp repository. + * Why the gate's own function is not imported: it would put the release-critical + * bump path one rename away from failing, and the claim being pinned is about + * ANOTHER script's verdict -- only that script's own run settles it. (The gate's + * former top-level CLI dispatch, which made any import execute the gate, was + * entry-guarded in #6566; the coupling reason stands on its own.) The agreement + * is pinned in the self-test instead, which runs the real gate as a child + * process over a real temp repository. * * @param {{ bump: string, body: string }} artifact */