Skip to content

Commit 5b7009c

Browse files
authored
Fail the runtime boundary PR check on counts main would reject (#15208)
In pull request mode, only a Deno.* call that src/deno_ral replaces failed the Runtime Boundary check. Any other count increase was a warning on a green job, so a PR could merge without its baseline update and turn main red. #15202 did this, and main was fixed by 4bebcb4. #15134 hit the same failure earlier. The check now fails when a count is above the PR's own tools/runtime-boundary-baseline.json. That is the same comparison the push check makes on main after merge. The contributor has nothing to change. A maintainer updates the branch with main, runs --update-baseline --accept-increase, and pushes the baseline to the PR. A count that fits under the PR's baseline and was not raised by the PR is reported as a notice and does not fail the run. Covered Deno.* API errors are unchanged. Related to #15153, #15183, #15202, and #15134.
1 parent 4bebcb4 commit 5b7009c

5 files changed

Lines changed: 82 additions & 27 deletions

File tree

‎.github/workflows/runtime-boundary.yml‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
# Counts `Deno.` occurrences and direct std-library imports per scope
33
# (src, src-js, resources, package/src, tests). Two modes:
44
# - pull_request: compares the merge commit with its first parent (the base
5-
# branch tip) and fails only for a Deno.* API that src/deno_ral replaces;
6-
# other increases are warnings plus a maintainer block in the job summary,
7-
# or notices once the PR's own baseline accepts them.
5+
# branch tip). A Deno.* API that src/deno_ral replaces is an error the
6+
# contributor fixes; any other increase above the PR's own baseline is a
7+
# warning that keeps the job red (within that baseline: a notice). The contributor
8+
# has nothing to change: a maintainer pushes the baseline update to the PR
9+
# before merging (job summary), so a green PR never turns main red.
810
# - push (main) and workflow_dispatch: fails when a count rises above
911
# tools/runtime-boundary-baseline.json.
1012
# See dev-docs/runtime-migration/02-runtime-boundary-check.md.

‎dev-docs/runtime-migration/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ The rows below are the ones that still need a named person or an external answer
132132
| PR 18, all tasks (from #15180) | the Deno backend keeps the `v8-<v8Branch>` store directory; reconcile it with the store names `index`/`sass-index` and the `<identifierHash>-<inputHash>.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 |
133133
| 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 |
134134
| 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 |
135+
| 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 |
135136
| PR 30 | tagged `v8-<branch>/` 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 |
136137

137138
## Shared rules and tables

‎dev-docs/runtime-migration/_TEMPLATE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ Repository conventions that plans must respect (`.claude/CLAUDE.md` and `.claude
8484
- 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/<file>.test.ts`.
8585
The ratchet (PR 02) has a `tests` scope: tests written by PRs 09 and later use `deno_ral` `fs`/`temp`/`process`, never raw `Deno.*`.
8686
- 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.
87-
- 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.
87+
- 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.
8888
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.
8989
- 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`.
9090
- 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.

‎tests/unit/tools/check-runtime-boundary.test.ts‎

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -446,10 +446,10 @@ unitTest("check-runtime-boundary - diffChanges an import split across more files
446446
});
447447

448448
const kUncoveredWarning = (scope: string, n: number, api: string, file: string) =>
449-
`${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).`;
449+
`${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).`;
450450

451451
// deno-lint-ignore require-await
452-
unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and maintainer block, not failed", async () => {
452+
unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and maintainer block, failed until accepted", async () => {
453453
const current = { "tests": { denoCalls: 832, stdImportFiles: 177 } };
454454
const rendered = renderDelta(
455455
{
@@ -462,7 +462,7 @@ unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and
462462
current,
463463
{ "tests": { denoCalls: 826, stdImportFiles: 177 } },
464464
);
465-
assertEquals(rendered.failed, false);
465+
assertEquals(rendered.failed, true);
466466
assertEquals(rendered.errors, []);
467467
assertEquals(rendered.notices, []);
468468
assertEquals(rendered.warnings.map((w) => w.message), [
@@ -471,6 +471,9 @@ unitTest("check-runtime-boundary - renderDelta uncovered increases: warnings and
471471
]);
472472
assert(rendered.summary.includes("| tests | +6 | 0 |"));
473473
assert(rendered.summary.includes("### Maintainer action before merging"));
474+
assert(rendered.summary.includes(
475+
"Nothing for the contributor to change: this job stays red until a maintainer accepts the new counts.",
476+
));
474477
assert(rendered.summary.includes("Expected counts after merge: tests 832 / 177."));
475478
assert(!rendered.summary.includes("### What to change"));
476479
});
@@ -514,9 +517,9 @@ unitTest("check-runtime-boundary - renderDelta a std import with a suggestion is
514517
{ "tests": { denoCalls: 832, stdImportFiles: 178 } },
515518
{ "tests": { denoCalls: 832, stdImportFiles: 177 } },
516519
);
517-
assertEquals(rendered.failed, false);
520+
assertEquals(rendered.failed, true);
518521
assertEquals(rendered.warnings.map((w) => w.message), [
519-
"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).",
522+
"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).",
520523
]);
521524
});
522525

@@ -675,17 +678,40 @@ unitTest("check-runtime-boundary - renderDelta accepts only counts the PR itself
675678
// main sits below its baseline for calls; the PR raises only std-import files.
676679
const base = { "tests": { denoCalls: 832, stdImportFiles: 177 } };
677680
const rendered = renderDelta(report, current, base, { "tests": { denoCalls: 832, stdImportFiles: 178 } });
681+
// The runtime call fits under the untouched baseline: main stays green, so it is a notice, not "accepted by this PR".
678682
assertEquals(rendered.accepted.map((a) => a.message), [
683+
`tests: +1 ${DENO}cwd in tests/unit/a.test.ts; within tools/runtime-boundary-baseline.json, which main sits below.`,
679684
"tests: tests/unit/c.test.ts now imports the standard library directly (testing/asserts); accepted by this PR's tools/runtime-boundary-baseline.json.",
680685
]);
681-
assertEquals(rendered.warnings.map((w) => w.file), ["tests/unit/a.test.ts"]);
686+
assertEquals(rendered.warnings, []);
687+
assertEquals(rendered.failed, false);
682688
assert(rendered.summary.includes("- tests std-import files: baseline 177 → 178"));
683689
assert(!rendered.summary.includes(`- tests ${DENO}* calls`));
684-
assert(rendered.summary.includes("### Maintainer action before merging"));
690+
assert(!rendered.summary.includes("### Maintainer action before merging"));
691+
});
692+
693+
// deno-lint-ignore require-await
694+
unitTest("check-runtime-boundary - renderDelta passes once the PR's baseline covers an increase made while main sits below its baseline", async () => {
695+
const report = {
696+
deltas: { "tests": { denoCalls: 1, stdImportFiles: 0 } },
697+
findings: [{ scope: "tests", file: "tests/unit/a.test.ts", kind: "deno" as const, name: "cwd", delta: 1 }],
698+
};
699+
// main counts 829 against a committed 832; the PR adds one, and --accept-increase writes 830.
700+
const rendered = renderDelta(
701+
report,
702+
{ "tests": { denoCalls: 830, stdImportFiles: 177 } },
703+
{ "tests": { denoCalls: 832, stdImportFiles: 177 } },
704+
{ "tests": { denoCalls: 830, stdImportFiles: 177 } },
705+
);
706+
assertEquals(rendered.failed, false);
707+
assertEquals(rendered.warnings, []);
708+
assertEquals(rendered.accepted.map((a) => a.message), [
709+
`tests: +1 ${DENO}cwd in tests/unit/a.test.ts; within tools/runtime-boundary-baseline.json, which main sits below.`,
710+
]);
685711
});
686712

687-
/** Runs the script in PR mode on `root`, as CI does, and returns its stdout. */
688-
async function runAgainst(root: string, rev: string): Promise<string> {
713+
/** Runs the script in PR mode on `root`, as CI does, and returns its exit status and stdout. */
714+
async function runAgainst(root: string, rev: string): Promise<{ success: boolean; out: string }> {
689715
const result = await execProcess({
690716
cmd: resolve(quartoDevBinCmd()),
691717
args: [
@@ -696,8 +722,7 @@ async function runAgainst(root: string, rev: string): Promise<string> {
696722
stdout: "piped",
697723
stderr: "piped",
698724
});
699-
assert(result.success, `check-runtime-boundary failed: ${result.stdout}\n${result.stderr}`);
700-
return result.stdout ?? "";
725+
return { success: result.success, out: (result.stdout ?? "") + (result.stderr ?? "") };
701726
}
702727

703728
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
714739
const raised = { ...kExpected, "tests": { denoCalls: 2, stdImportFiles: 2 } };
715740
writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify(raised));
716741

717-
const out = await runAgainst(root, "HEAD");
742+
const { success, out } = await runAgainst(root, "HEAD");
743+
assert(success, out);
718744
assert(!out.includes("::warning"), out);
745+
assert(!out.includes("::error"), out);
719746
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);
720747
assert(out.includes("### Accepted in this PR"), out);
721748
assert(out.includes(`- tests ${DENO}* calls: baseline 1 → 2`), out);
722749
assert(!out.includes("### Maintainer action before merging"), out);
723750

724-
// A baseline raised for a smaller increase than the PR makes leaves the warnings.
751+
// A baseline raised for a smaller increase than the PR makes leaves the warnings and fails the job,
752+
// telling the contributor that accepting the count is the maintainer's step.
725753
writeFixture(root, "tests/k.test.ts", `${DENO}cwd();\n`);
726-
const stale = await runAgainst(root, "HEAD");
754+
const { success: staleSuccess, out: stale } = await runAgainst(root, "HEAD");
755+
assert(!staleSuccess, stale);
756+
assert(
757+
stale.includes(
758+
"::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).",
759+
),
760+
stale,
761+
);
727762
assert(stale.includes("::warning file=tests/j.test.ts"), stale);
728763
assert(stale.includes("### Maintainer action before merging"), stale);
729764
assert(!stale.includes("### Accepted in this PR"), stale);

‎tools/check-runtime-boundary.ts‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,10 @@
77
* Counts, per scope, the `Deno.` occurrences and the files that import the
88
* standard library directly instead of through src/deno_ral/. Two modes:
99
* without flags it fails when any count exceeds the committed baseline (main);
10-
* with --against <rev> it compares the working tree with <rev> and fails only
11-
* for a Deno.* API that src/deno_ral already replaces (pull requests); other
12-
* increases are warnings until the working tree's baseline accepts them.
10+
* with --against <rev> it compares the working tree with <rev> (pull requests):
11+
* a Deno.* API that src/deno_ral already replaces is an error, any other
12+
* increase above the working tree's baseline is a warning that still fails the
13+
* run until a maintainer pushes the accepted baseline to the PR before merging.
1314
*
1415
* Usage (from the repository root):
1516
* quarto run --dev tools/check-runtime-boundary.ts
@@ -505,15 +506,19 @@ export interface Rendered {
505506
accepted: Annotation[]; // increases the PR's own baseline already covers
506507
notices: string[];
507508
summary: string; // markdown for GITHUB_STEP_SUMMARY
508-
failed: boolean; // errors.length > 0
509+
failed: boolean; // errors or warnings: a warning is a count above the PR's baseline, which would turn main red
509510
}
510511

511512
const kReadmeRelPath = "dev-docs/runtime-migration/README.md";
512513

513514
const kAcceptClause =
514-
"a maintainer accepts the new count before merging (see the job summary).";
515+
"a maintainer accepts the new count before merging; this job stays red until then (see the job summary).";
515516
const kAcceptSentence = kAcceptClause[0].toUpperCase() + kAcceptClause.slice(1);
516517

518+
/** Printed once in PR mode when a warning fails the job, so the contributor knows the remedy is not theirs. */
519+
const kPendingAcceptanceError =
520+
`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).`;
521+
517522
const signed = (n: number) => n > 0 ? `+${n}` : `${n}`;
518523

519524
/** `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(
539544
(current[scope]?.[field] ?? 0) <= raised;
540545
};
541546
const acceptedSuffix = `; accepted by this PR's ${kBaselineRelPath}.`;
547+
// The push check on main compares with `merged`: a count within it cannot turn main red,
548+
// even when the PR did not raise it (main sits below its baseline).
549+
const isCovered = (scope: string, field: keyof BoundaryCounts) =>
550+
(current[scope]?.[field] ?? 0) <= (merged[scope]?.[field] ?? 0);
551+
const coveredSuffix = `; within ${kBaselineRelPath}, which main sits below.`;
542552

543553
for (const finding of report.findings) {
544554
const annotation = (message: string): Annotation => {
@@ -556,11 +566,13 @@ export function renderDelta(
556566
);
557567
continue;
558568
}
559-
if (isAccepted(finding.scope, finding.kind === "deno" ? "denoCalls" : "stdImportFiles")) {
569+
const field = finding.kind === "deno" ? "denoCalls" : "stdImportFiles";
570+
if (isAccepted(finding.scope, field) || isCovered(finding.scope, field)) {
571+
const suffix = isAccepted(finding.scope, field) ? acceptedSuffix : coveredSuffix;
560572
accepted.push(annotation(
561573
finding.kind === "deno"
562-
? `${finding.scope}: +${finding.delta} Deno.${finding.name} in ${finding.file}${acceptedSuffix}`
563-
: `${finding.scope}: ${finding.file} now imports the standard library directly (${finding.name})${acceptedSuffix}`,
574+
? `${finding.scope}: +${finding.delta} Deno.${finding.name} in ${finding.file}${suffix}`
575+
: `${finding.scope}: ${finding.file} now imports the standard library directly (${finding.name})${suffix}`,
564576
));
565577
continue;
566578
}
@@ -633,6 +645,8 @@ export function renderDelta(
633645
summary.push(
634646
"### Maintainer action before merging",
635647
"",
648+
"Nothing for the contributor to change: this job stays red until a maintainer accepts the new counts.",
649+
"",
636650
"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",
637651
"",
638652
" quarto run --dev tools/check-runtime-boundary.ts --update-baseline --accept-increase",
@@ -647,7 +661,7 @@ export function renderDelta(
647661
accepted,
648662
notices,
649663
summary: summary.join("\n"),
650-
failed: errors.length > 0,
664+
failed: errors.length > 0 || warnings.length > 0,
651665
};
652666
}
653667

@@ -886,6 +900,9 @@ if (import.meta.main) {
886900
for (const { message, file, line } of rendered.errors) {
887901
error(message, { file, line });
888902
}
903+
if (rendered.warnings.length > 0) {
904+
error(kPendingAcceptanceError);
905+
}
889906
if (!stepSummary(rendered.summary)) {
890907
console.log(rendered.summary);
891908
}

0 commit comments

Comments
 (0)