From 7885db23d06b415463a8e01ae0ac4e217f2dd174 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 27 Oct 2026 15:05:55 +0100 Subject: [PATCH 1/3] Fail the runtime boundary PR run until the PR's baseline accepts its increase PR mode only failed for a Deno.* API with a src/deno_ral replacement; every other increase was a warning on a green job, relying on a maintainer to read the job summary and push the baseline bump before merging. Twice a PR merged without it and the absolute push check turned main red (#15134, #15202). An increase the PR's own tools/runtime-boundary-baseline.json does not accept now fails the run. The per-line findings stay warnings, since the code is fine as written, and one ::error plus the job summary tell the contributor there is nothing for them to change: a maintainer updates the branch with main, runs --update-baseline --accept-increase and pushes the baseline to the PR. Once accepted, the findings become notices and the run passes, so a green PR no longer implies a red main. A PR label clearing the check was rejected: the merged tree would still carry the old baseline and main would fail anyway. Replayed #15202 (55a313cce against its parent): exit 1 with the four tests/unit/zip.test.ts calls; with the baseline from 4bebcb499 applied, exit 0 and "Accepted in this PR: 870 -> 874". --- .github/workflows/runtime-boundary.yml | 8 +++-- dev-docs/runtime-migration/README.md | 1 + dev-docs/runtime-migration/_TEMPLATE.md | 2 +- .../unit/tools/check-runtime-boundary.test.ts | 36 ++++++++++++------- tools/check-runtime-boundary.ts | 22 ++++++++---- 5 files changed, 47 insertions(+), 22 deletions(-) diff --git a/.github/workflows/runtime-boundary.yml b/.github/workflows/runtime-boundary.yml index 8f841e2b72..01e21bb12f 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 is a warning that keeps the job red +# until the PR's own baseline accepts it (then 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..d5e25bcfdb 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 their own `tools/runtime-boundary-baseline.json` does not accept, instead of only warning; 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..2593c9cdc3 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 until the PR's own baseline accepts it (`--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..0c0186e973 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).", ]); }); @@ -684,8 +687,8 @@ unitTest("check-runtime-boundary - renderDelta accepts only counts the PR itself assert(rendered.summary.includes("### Maintainer action before merging")); }); -/** 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 +699,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 +716,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..57f9d4640d 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 is a warning that still fails the run until the working tree's + * baseline accepts it, which a maintainer pushes 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: an increase not accepted by the PR's baseline 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. @@ -633,6 +638,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 +654,7 @@ export function renderDelta( accepted, notices, summary: summary.join("\n"), - failed: errors.length > 0, + failed: errors.length > 0 || warnings.length > 0, }; } @@ -886,6 +893,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); } From d99be3192d47d5019ce5ad35fb9d75dac238326b Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 27 Oct 2026 15:17:48 +0100 Subject: [PATCH 2/3] Gate runtime boundary PR runs on the count main will compare The PR-mode failure used "accepted by this PR", which needs the PR to raise the baseline above the base revision's value. When main sits below its baseline (a count fell without --update-baseline), a PR adding a call and running --accept-increase writes a value at or under the old one, so the run stayed red although the push check on main would pass. A finding now fails only when the count exceeds the PR's own baseline, the same comparison the push check makes after merge. A count within it that the PR did not raise is reported as a notice ("within the baseline, which main sits below"); "accepted by this PR" keeps its meaning for counts the PR raised. --- .github/workflows/runtime-boundary.yml | 4 +-- dev-docs/runtime-migration/README.md | 2 +- dev-docs/runtime-migration/_TEMPLATE.md | 2 +- .../unit/tools/check-runtime-boundary.test.ts | 27 +++++++++++++++++-- tools/check-runtime-boundary.ts | 19 ++++++++----- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/.github/workflows/runtime-boundary.yml b/.github/workflows/runtime-boundary.yml index 01e21bb12f..3a71947f68 100644 --- a/.github/workflows/runtime-boundary.yml +++ b/.github/workflows/runtime-boundary.yml @@ -3,8 +3,8 @@ # (src, src-js, resources, package/src, tests). Two modes: # - pull_request: compares the merge commit with its first parent (the base # branch tip). A Deno.* API that src/deno_ral replaces is an error the -# contributor fixes; any other increase is a warning that keeps the job red -# until the PR's own baseline accepts it (then a notice). The contributor +# 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 diff --git a/dev-docs/runtime-migration/README.md b/dev-docs/runtime-migration/README.md index d5e25bcfdb..d1f4cfa1cf 100644 --- a/dev-docs/runtime-migration/README.md +++ b/dev-docs/runtime-migration/README.md @@ -132,7 +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 their own `tools/runtime-boundary-baseline.json` does not accept, instead of only warning; 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 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 2593c9cdc3..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 (a suggestion never makes a finding an error). Any increase that is not an error still fails the PR run until the PR's own baseline accepts it (`--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 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 0c0186e973..187847c27c 100644 --- a/tests/unit/tools/check-runtime-boundary.test.ts +++ b/tests/unit/tools/check-runtime-boundary.test.ts @@ -678,13 +678,36 @@ 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 Deno.* 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 exit status and stdout. */ diff --git a/tools/check-runtime-boundary.ts b/tools/check-runtime-boundary.ts index 57f9d4640d..ce7f4bd04c 100644 --- a/tools/check-runtime-boundary.ts +++ b/tools/check-runtime-boundary.ts @@ -9,8 +9,8 @@ * without flags it fails when any count exceeds the committed baseline (main); * 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 is a warning that still fails the run until the working tree's - * baseline accepts it, which a maintainer pushes to the PR before merging. + * 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 @@ -506,7 +506,7 @@ 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 or warnings: an increase not accepted by the PR's baseline would turn main red + 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"; @@ -544,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 => { @@ -561,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; } From 2a5cefa16b749134d74c0ce8bdab4c2c691f8338 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 27 Oct 2026 15:21:42 +0100 Subject: [PATCH 3/3] Keep the literal runtime API name out of a boundary test comment The check counts Deno. occurrences in comments too, so the comment added with the headroom test raised the tests scope from 874 to 875 and failed the push check against the committed baseline. --- tests/unit/tools/check-runtime-boundary.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/tools/check-runtime-boundary.test.ts b/tests/unit/tools/check-runtime-boundary.test.ts index 187847c27c..95ff2a6ba0 100644 --- a/tests/unit/tools/check-runtime-boundary.test.ts +++ b/tests/unit/tools/check-runtime-boundary.test.ts @@ -678,7 +678,7 @@ 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 Deno.* call fits under the untouched baseline: main stays green, so it is a notice, not "accepted by this PR". + // 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.",