diff --git a/.github/workflows/runtime-boundary.yml b/.github/workflows/runtime-boundary.yml index 8f841e2b72..3a71947f68 100644 --- a/.github/workflows/runtime-boundary.yml +++ b/.github/workflows/runtime-boundary.yml @@ -2,9 +2,11 @@ # Counts `Deno.` occurrences and direct std-library imports per scope # (src, src-js, resources, package/src, tests). Two modes: # - pull_request: compares the merge commit with its first parent (the base -# branch tip) and fails only for a Deno.* API that src/deno_ral replaces; -# other increases are warnings plus a maintainer block in the job summary, -# or notices once the PR's own baseline accepts them. +# branch tip). A Deno.* API that src/deno_ral replaces is an error the +# contributor fixes; any other increase above the PR's own baseline is a +# warning that keeps the job red (within that baseline: a notice). The contributor +# has nothing to change: a maintainer pushes the baseline update to the PR +# before merging (job summary), so a green PR never turns main red. # - push (main) and workflow_dispatch: fails when a count rises above # tools/runtime-boundary-baseline.json. # See dev-docs/runtime-migration/02-runtime-boundary-check.md. diff --git a/dev-docs/runtime-migration/README.md b/dev-docs/runtime-migration/README.md index deee01cc33..d1f4cfa1cf 100644 --- a/dev-docs/runtime-migration/README.md +++ b/dev-docs/runtime-migration/README.md @@ -132,6 +132,7 @@ The rows below are the ones that still need a named person or an external answer | PR 18, all tasks (from #15180) | the Deno backend keeps the `v8-` store directory; reconcile it with the store names `index`/`sass-index` and the `-.css` naming. Task 2 Step 2 shrinks to the API change: `this.index.list({ prefix: [] })` becomes `this.index.list([])`, and the `clear()` body #15180 shipped stays as is (`storeDir` paths, `safeRemoveSync`, immediate entries skipped; test `projectCache - clear() removes disk and immediate entries of a populated cache`). Drop the Context sentence saying `join` throws in `clear()`, and the Risks lines about that throwing path; one `cache.ts` `Deno.removeSync` is now `safeRemoveSync`. Refresh every Verified usage line number | coordinator | PR 18 step 0 | | PR 18 Tasks 1–3 (from the #15119 fix, once `src/deno_ral/kv.ts` is on `main`) | `kv.ts` already exists (`openKv`, `getKv`, `kvOp`, `KvDamagedError`) and both caches go through it, so Task 1 extends the file instead of creating it and its "Module not found" RED step no longer holds. `openKeyValueStore` keeps the damage mapping: a damaged value reads as a miss with one warning, a damaged file fails with `KvDamagedError` naming the folder to delete, any other error is wrapped with the path. The `src` baseline drops by 5, not 7 (the two `Deno.openKv` calls are already in the layer). The `Deno.openKv` grep acceptance check covers `src` and `package` only, since three cache tests build damaged fixtures with `Deno.openKv` on purpose. The `kv.ts` row in `.claude/rules/typescript/deno-ral.md` exists: update it | coordinator | PR 18 step 0 | | PR 19 Task 1, PR 27b | both replace the whole body of `platform.ts`: keep the `v8Branch` export (`_TEMPLATE.md`, "`platform.ts`"); PR 27b computes it from `process.versions.v8` | coordinator | step 0 of each | +| PR 02c (from #15202, which turned `main` red) | Decided 2026-10-27: PR runs fail on any increase that leaves a count above their own `tools/runtime-boundary-baseline.json` (exactly what the push check on `main` will compare), instead of only warning; an increase that still fits because `main` sits below its baseline is a notice; 02c's "everything else is a warning for the maintainer" and its "exit 1 if any error, else 0" no longer hold. Per-line findings stay warnings saying the contributor has nothing to change, plus one `::error` saying a maintainer pushes the baseline update before merging; the remedy is unchanged (update the branch with `main`, `--update-baseline --accept-increase`, push to the PR). Rejected: an acceptance label (the merged tree still carries the old baseline, so `main` fails anyway) and warn-only with a merge checklist (the human step is what failed). A plan whose expected delta raises a count runs `--accept-increase` in its PR (PRs 01 and 05 already do) | maintainer | step 0 of any plan that raises a count | | PR 30 | tagged `v8-/` directories are left behind like the untagged `.kv` files; remove `v8Branch` and its contract-test line together with its last consumer. What "damaged" means for the JSON index (unparsable or truncated file): keep the miss-with-warning versus fail-naming-the-folder split or not. The Deno KV damage tests (`tests/unit/kv-damage.test.ts` and the damaged-store cases in `sass-cache.test.ts` and `project-cache.test.ts`, from the #15119 fix) build real SQLite damage: they are replaced or deleted here, because PR 32 runs the full unit suite on Node, where neither `Deno.openKv` nor its damage exists, and "`sass-cache.test.ts` stays green" is restated accordingly | maintainer (damage semantics), coordinator (the rest) | PR 30 step 0 | ## Shared rules and tables diff --git a/dev-docs/runtime-migration/_TEMPLATE.md b/dev-docs/runtime-migration/_TEMPLATE.md index d3d06f1b31..70647a5308 100644 --- a/dev-docs/runtime-migration/_TEMPLATE.md +++ b/dev-docs/runtime-migration/_TEMPLATE.md @@ -84,7 +84,7 @@ Repository conventions that plans must respect (`.claude/CLAUDE.md` and `.claude - Unit tests live in `tests/unit/` (runtime-layer tests under the existing `tests/unit/ral/`), use `unitTest` from `tests/test.ts` and assertions from `tests/asserts.ts` (added by PR 02), and run with `cd tests && QUARTO_TESTS_NO_CONFIG=true ./run-tests.sh --agent unit/.test.ts`. The ratchet (PR 02) has a `tests` scope: tests written by PRs 09 and later use `deno_ral` `fs`/`temp`/`process`, never raw `Deno.*`. - Landing order inside Phase 1: 02 → 02b → 08 → 22 → 10–13 → 14, with 09 before 10; 22 depends on PR 04 (Phase 0). 02c lands before any other count-changing PR. -- A PR that adds an equivalent `deno_ral` replacement for a `Deno.*` API adds its exact keys (e.g. `readTextFileSync`) to `kRalDenoReplacements` in `tools/check-runtime-boundary.ts` in the same change, which turns later increases of that API into errors; a PR that adds a std seam may add a `kStdSuggestions` entry (suggestions never fail a PR). Under `kBrowserSharedPaths` (`src/core/lib/`, which the browser-side YAML intelligence imports and which therefore cannot import `deno_ral`) a covered API is only a warning. +- A PR that adds an equivalent `deno_ral` replacement for a `Deno.*` API adds its exact keys (e.g. `readTextFileSync`) to `kRalDenoReplacements` in `tools/check-runtime-boundary.ts` in the same change, which turns later increases of that API into errors; a PR that adds a std seam may add a `kStdSuggestions` entry (a suggestion never makes a finding an error). Any increase that is not an error still fails the PR run while the count exceeds the PR's own baseline (`--update-baseline --accept-increase`), so a plan that raises a count runs that step in the PR. Under `kBrowserSharedPaths` (`src/core/lib/`, which the browser-side YAML intelligence imports and which therefore cannot import `deno_ral`) a covered API is only a warning. A conflict in a codemod PR is resolved by resetting the file to `main` and re-running the codemod, never by hand (the codemod is idempotent, PR 09); a conflict on a shared file (`tools/runtime-boundary-baseline.json`, `deno.lock`, the shim, the `deno-ral.md` table, a changelog) follows README ground rule 8. - Commands in the plans write the Linux path `package/dist/bin/tools/x86_64/deno`; on Apple Silicon it is `tools/aarch64/deno`, on Windows `tools\x86_64\deno.exe`. - Size is by file count (S < 10, M 10–40, L > 40 files); a plan that is large by volume but small by files says so next to the letter. diff --git a/tests/unit/tools/check-runtime-boundary.test.ts b/tests/unit/tools/check-runtime-boundary.test.ts index 1cb1bba7e4..95ff2a6ba0 100644 --- a/tests/unit/tools/check-runtime-boundary.test.ts +++ b/tests/unit/tools/check-runtime-boundary.test.ts @@ -446,10 +446,10 @@ unitTest("check-runtime-boundary - diffChanges an import split across more files }); const kUncoveredWarning = (scope: string, n: number, api: string, file: string) => - `${scope}: +${n} ${DENO}${api} in ${file}; src/deno_ral has no replacement yet, so nothing to change in this PR. A maintainer accepts the new count before merging (see the job summary).`; + `${scope}: +${n} ${DENO}${api} in ${file}; src/deno_ral has no replacement yet, so nothing to change in this PR. A maintainer accepts the new count before merging; this job stays red until then (see the job summary).`; // deno-lint-ignore require-await -unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and maintainer block, not failed", async () => { +unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and maintainer block, failed until accepted", async () => { const current = { "tests": { denoCalls: 832, stdImportFiles: 177 } }; const rendered = renderDelta( { @@ -462,7 +462,7 @@ unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and current, { "tests": { denoCalls: 826, stdImportFiles: 177 } }, ); - assertEquals(rendered.failed, false); + assertEquals(rendered.failed, true); assertEquals(rendered.errors, []); assertEquals(rendered.notices, []); assertEquals(rendered.warnings.map((w) => w.message), [ @@ -471,6 +471,9 @@ unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and ]); assert(rendered.summary.includes("| tests | +6 | 0 |")); assert(rendered.summary.includes("### Maintainer action before merging")); + assert(rendered.summary.includes( + "Nothing for the contributor to change: this job stays red until a maintainer accepts the new counts.", + )); assert(rendered.summary.includes("Expected counts after merge: tests 832 / 177.")); assert(!rendered.summary.includes("### What to change")); }); @@ -514,9 +517,9 @@ unitTest("check-runtime-boundary - renderDelta a std import with a suggestion is { "tests": { denoCalls: 832, stdImportFiles: 178 } }, { "tests": { denoCalls: 832, stdImportFiles: 177 } }, ); - assertEquals(rendered.failed, false); + assertEquals(rendered.failed, true); assertEquals(rendered.warnings.map((w) => w.message), [ - "tests: tests/unit/c.test.ts now imports the standard library directly (testing/asserts); if tests/asserts.ts exports what this file uses with the same behaviour, import it from there. Otherwise nothing to change in this PR: a maintainer accepts the new count before merging (see the job summary).", + "tests: tests/unit/c.test.ts now imports the standard library directly (testing/asserts); if tests/asserts.ts exports what this file uses with the same behaviour, import it from there. Otherwise nothing to change in this PR: a maintainer accepts the new count before merging; this job stays red until then (see the job summary).", ]); }); @@ -675,17 +678,40 @@ unitTest("check-runtime-boundary - renderDelta accepts only counts the PR itself // main sits below its baseline for calls; the PR raises only std-import files. const base = { "tests": { denoCalls: 832, stdImportFiles: 177 } }; const rendered = renderDelta(report, current, base, { "tests": { denoCalls: 832, stdImportFiles: 178 } }); + // The runtime call fits under the untouched baseline: main stays green, so it is a notice, not "accepted by this PR". assertEquals(rendered.accepted.map((a) => a.message), [ + `tests: +1 ${DENO}cwd in tests/unit/a.test.ts; within tools/runtime-boundary-baseline.json, which main sits below.`, "tests: tests/unit/c.test.ts now imports the standard library directly (testing/asserts); accepted by this PR's tools/runtime-boundary-baseline.json.", ]); - assertEquals(rendered.warnings.map((w) => w.file), ["tests/unit/a.test.ts"]); + assertEquals(rendered.warnings, []); + assertEquals(rendered.failed, false); assert(rendered.summary.includes("- tests std-import files: baseline 177 → 178")); assert(!rendered.summary.includes(`- tests ${DENO}* calls`)); - assert(rendered.summary.includes("### Maintainer action before merging")); + assert(!rendered.summary.includes("### Maintainer action before merging")); +}); + +// deno-lint-ignore require-await +unitTest("check-runtime-boundary - renderDelta passes once the PR's baseline covers an increase made while main sits below its baseline", async () => { + const report = { + deltas: { "tests": { denoCalls: 1, stdImportFiles: 0 } }, + findings: [{ scope: "tests", file: "tests/unit/a.test.ts", kind: "deno" as const, name: "cwd", delta: 1 }], + }; + // main counts 829 against a committed 832; the PR adds one, and --accept-increase writes 830. + const rendered = renderDelta( + report, + { "tests": { denoCalls: 830, stdImportFiles: 177 } }, + { "tests": { denoCalls: 832, stdImportFiles: 177 } }, + { "tests": { denoCalls: 830, stdImportFiles: 177 } }, + ); + assertEquals(rendered.failed, false); + assertEquals(rendered.warnings, []); + assertEquals(rendered.accepted.map((a) => a.message), [ + `tests: +1 ${DENO}cwd in tests/unit/a.test.ts; within tools/runtime-boundary-baseline.json, which main sits below.`, + ]); }); -/** Runs the script in PR mode on `root`, as CI does, and returns its stdout. */ -async function runAgainst(root: string, rev: string): Promise { +/** Runs the script in PR mode on `root`, as CI does, and returns its exit status and stdout. */ +async function runAgainst(root: string, rev: string): Promise<{ success: boolean; out: string }> { const result = await execProcess({ cmd: resolve(quartoDevBinCmd()), args: [ @@ -696,8 +722,7 @@ async function runAgainst(root: string, rev: string): Promise { stdout: "piped", stderr: "piped", }); - assert(result.success, `check-runtime-boundary failed: ${result.stdout}\n${result.stderr}`); - return result.stdout ?? ""; + return { success: result.success, out: (result.stdout ?? "") + (result.stderr ?? "") }; } unitTest("check-runtime-boundary - PR mode: an increase the PR's baseline already accepts is a notice, not a warning", async () => { @@ -714,16 +739,26 @@ unitTest("check-runtime-boundary - PR mode: an increase the PR's baseline alread const raised = { ...kExpected, "tests": { denoCalls: 2, stdImportFiles: 2 } }; writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify(raised)); - const out = await runAgainst(root, "HEAD"); + const { success, out } = await runAgainst(root, "HEAD"); + assert(success, out); assert(!out.includes("::warning"), out); + assert(!out.includes("::error"), out); assert(out.includes("::notice file=tests/j.test.ts,line=1::tests: +1 " + DENO + "cwd in tests/j.test.ts; accepted by this PR's tools/runtime-boundary-baseline.json"), out); assert(out.includes("### Accepted in this PR"), out); assert(out.includes(`- tests ${DENO}* calls: baseline 1 → 2`), out); assert(!out.includes("### Maintainer action before merging"), out); - // A baseline raised for a smaller increase than the PR makes leaves the warnings. + // A baseline raised for a smaller increase than the PR makes leaves the warnings and fails the job, + // telling the contributor that accepting the count is the maintainer's step. writeFixture(root, "tests/k.test.ts", `${DENO}cwd();\n`); - const stale = await runAgainst(root, "HEAD"); + const { success: staleSuccess, out: stale } = await runAgainst(root, "HEAD"); + assert(!staleSuccess, stale); + assert( + stale.includes( + "::error::This PR raises runtime boundary counts that tools/runtime-boundary-baseline.json does not accept yet. Nothing for the contributor to change: a maintainer pushes the baseline update to this PR before merging (see the job summary).", + ), + stale, + ); assert(stale.includes("::warning file=tests/j.test.ts"), stale); assert(stale.includes("### Maintainer action before merging"), stale); assert(!stale.includes("### Accepted in this PR"), stale); diff --git a/tools/check-runtime-boundary.ts b/tools/check-runtime-boundary.ts index e3d116c0c4..ce7f4bd04c 100644 --- a/tools/check-runtime-boundary.ts +++ b/tools/check-runtime-boundary.ts @@ -7,9 +7,10 @@ * Counts, per scope, the `Deno.` occurrences and the files that import the * standard library directly instead of through src/deno_ral/. Two modes: * without flags it fails when any count exceeds the committed baseline (main); - * with --against it compares the working tree with and fails only - * for a Deno.* API that src/deno_ral already replaces (pull requests); other - * increases are warnings until the working tree's baseline accepts them. + * with --against it compares the working tree with (pull requests): + * a Deno.* API that src/deno_ral already replaces is an error, any other + * increase above the working tree's baseline is a warning that still fails the + * run until a maintainer pushes the accepted baseline to the PR before merging. * * Usage (from the repository root): * quarto run --dev tools/check-runtime-boundary.ts @@ -505,15 +506,19 @@ export interface Rendered { accepted: Annotation[]; // increases the PR's own baseline already covers notices: string[]; summary: string; // markdown for GITHUB_STEP_SUMMARY - failed: boolean; // errors.length > 0 + failed: boolean; // errors or warnings: a warning is a count above the PR's baseline, which would turn main red } const kReadmeRelPath = "dev-docs/runtime-migration/README.md"; const kAcceptClause = - "a maintainer accepts the new count before merging (see the job summary)."; + "a maintainer accepts the new count before merging; this job stays red until then (see the job summary)."; const kAcceptSentence = kAcceptClause[0].toUpperCase() + kAcceptClause.slice(1); +/** Printed once in PR mode when a warning fails the job, so the contributor knows the remedy is not theirs. */ +const kPendingAcceptanceError = + `This PR raises runtime boundary counts that ${kBaselineRelPath} does not accept yet. Nothing for the contributor to change: a maintainer pushes the baseline update to this PR before merging (see the job summary).`; + const signed = (n: number) => n > 0 ? `+${n}` : `${n}`; /** `current` is measure() of the working tree (the merge result on CI), so `current - deltas` is the base. @@ -539,6 +544,11 @@ export function renderDelta( (current[scope]?.[field] ?? 0) <= raised; }; const acceptedSuffix = `; accepted by this PR's ${kBaselineRelPath}.`; + // The push check on main compares with `merged`: a count within it cannot turn main red, + // even when the PR did not raise it (main sits below its baseline). + const isCovered = (scope: string, field: keyof BoundaryCounts) => + (current[scope]?.[field] ?? 0) <= (merged[scope]?.[field] ?? 0); + const coveredSuffix = `; within ${kBaselineRelPath}, which main sits below.`; for (const finding of report.findings) { const annotation = (message: string): Annotation => { @@ -556,11 +566,13 @@ export function renderDelta( ); continue; } - if (isAccepted(finding.scope, finding.kind === "deno" ? "denoCalls" : "stdImportFiles")) { + const field = finding.kind === "deno" ? "denoCalls" : "stdImportFiles"; + if (isAccepted(finding.scope, field) || isCovered(finding.scope, field)) { + const suffix = isAccepted(finding.scope, field) ? acceptedSuffix : coveredSuffix; accepted.push(annotation( finding.kind === "deno" - ? `${finding.scope}: +${finding.delta} Deno.${finding.name} in ${finding.file}${acceptedSuffix}` - : `${finding.scope}: ${finding.file} now imports the standard library directly (${finding.name})${acceptedSuffix}`, + ? `${finding.scope}: +${finding.delta} Deno.${finding.name} in ${finding.file}${suffix}` + : `${finding.scope}: ${finding.file} now imports the standard library directly (${finding.name})${suffix}`, )); continue; } @@ -633,6 +645,8 @@ export function renderDelta( summary.push( "### Maintainer action before merging", "", + "Nothing for the contributor to change: this job stays red until a maintainer accepts the new counts.", + "", "This PR raises counts (warnings above). Where a warning suggests a module, check whether the PR should use it; accept the rest. Before merging, update the branch with `main`, then run", "", " quarto run --dev tools/check-runtime-boundary.ts --update-baseline --accept-increase", @@ -647,7 +661,7 @@ export function renderDelta( accepted, notices, summary: summary.join("\n"), - failed: errors.length > 0, + failed: errors.length > 0 || warnings.length > 0, }; } @@ -886,6 +900,9 @@ if (import.meta.main) { for (const { message, file, line } of rendered.errors) { error(message, { file, line }); } + if (rendered.warnings.length > 0) { + error(kPendingAcceptanceError); + } if (!stepSummary(rendered.summary)) { console.log(rendered.summary); }