Skip to content

Commit a16205a

Browse files
committed
Compare main against the baseline it had at the PR base, not the PR's own
The "main is above the baseline" notice reconstructs the base counts as current minus the PR delta. Comparing them with the PR's working-tree baseline gave a false notice for every PR that lowers a count and commits the lowered baseline (src 994 > 993). Delta mode now reads the baseline committed at the --against revision; a revision without one (before the ratchet) falls back to the working-tree baseline.
1 parent 8c38f94 commit a16205a

3 files changed

Lines changed: 55 additions & 1 deletion

File tree

‎dev-docs/runtime-migration/02c-ratchet-pr-delta-and-guidance.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,3 +432,10 @@ Run from the branch worktree; `Q` is `package/dist/bin/quarto` (`quarto.cmd` on
432432
- Minor, deferred: `git diff -U0 -- <path>` treats the path as a pathspec (`src/[id].ts` picks up another file's hunks: wrong line numbers only, never counts); `--literal-pathspecs` would fix it. No tracked in-scope path contains `[ ] * ? :` today.
433433
- Minor, deferred: `git diff` is run without `--no-color --no-ext-diff`, so a user git config (`color.diff=always`, `diff.external`) can empty `addedLines` in local runs; CI is unaffected.
434434
- Minor, deferred: the CLI exit-code contract and the stdout summary fallback are covered only by manual Verification 5; no test covers a report with both errors and warnings, or that "Expected counts after merge" lists only scopes with warnings.
435+
436+
### 2026-10-22: review fix after the whole-branch review
437+
438+
- Defect: the "main is above the baseline" notice compared the base counts (current minus delta) with the PR's own baseline. A PR that lowers `src` 994 → 993 and commits the lowered baseline got `main is above tools/runtime-boundary-baseline.json (src Deno.* calls 994 > 993); not caused by this PR, a maintainer fixes main`, which every count-lowering migration PR would see. Reproduced at the CLI before the fix (probe edit `Deno.stdout` → `Deno .stdout` in an unrelated command file plus `--update-baseline`, then `--against HEAD`; both files restored with `git checkout --` after checking the diffs).
439+
- Fix: delta mode passes `renderDelta` the baseline committed at the `--against` revision, read by the new exported `baselineAt` (`git cat-file -e` to detect an absent file, fallback to the working-tree baseline for a revision from before the ratchet; a malformed baseline exits 2). New export, not in the plan: needed to unit-test the logic with a real temp git repository. After: probe PR prints no notice, V3 still prints `main is above … (tests Deno.* calls 832 > 826)`, `unit/tools/check-runtime-boundary.test.ts` → 36 passed.
440+
- Correction to the earlier note on the `execProcess` decoder: counts are not always unaffected. A corrupted path from `git diff --name-only` or the untracked list fails both the `ls-tree` membership check and the disk read, so the file contributes nothing and a covered increase in it would be missed silently; only a corrupted `ls-tree` entry over-reports. Still 0 tracked non-ASCII paths, and the fix stays a separate bug on main (backported to v1.10).
441+
- Open, needs a decision: a covered `Deno.build.os` added under `src/core/lib/` (browser-shared: the YAML-intelligence web worker imports it, so it cannot import `src/deno_ral/platform.ts`) gets a blocking error with a hint that cannot compile there. PR 19 plans a runtime-neutral probe for the two existing uses (`errors.ts`, `colors.ts`).

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { execProcess } from "../../../src/core/process.ts";
1616
import { createTempContext } from "../../../src/core/temp.ts";
1717
import {
1818
acceptIncrease,
19+
baselineAt,
1920
compareToBaseline,
2021
countScope,
2122
countText,
@@ -636,3 +637,26 @@ unitTest("check-runtime-boundary - gitChanges handles more changed paths than fi
636637
temp.cleanup();
637638
}
638639
});
640+
641+
unitTest("check-runtime-boundary - baselineAt reads the baseline committed at the revision, not the working tree's", async () => {
642+
const temp = createTempContext();
643+
try {
644+
const root = temp.createDir();
645+
await makeGitRepo(root);
646+
assertEquals(await baselineAt(root, "HEAD"), undefined);
647+
648+
const committed = { "src": { denoCalls: 994, stdImportFiles: 49 } };
649+
writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify(committed));
650+
await git(root, "add", ".");
651+
await git(root, "-c", "user.name=t", "-c", "user.email=t@example.com", "-c", "commit.gpgsign=false", "commit", "-m", "baseline");
652+
writeFixture(root, "tools/runtime-boundary-baseline.json", JSON.stringify({ "src": { denoCalls: 993, stdImportFiles: 49 } }));
653+
assertEquals(await baselineAt(root, "HEAD"), committed);
654+
655+
writeFixture(root, "tools/runtime-boundary-baseline.json", `{"src":{"denoCalls":1}}`);
656+
await git(root, "add", ".");
657+
await git(root, "-c", "user.name=t", "-c", "user.email=t@example.com", "-c", "commit.gpgsign=false", "commit", "-m", "malformed");
658+
await assertRejects(() => baselineAt(root, "HEAD"), Error, "baseline");
659+
} finally {
660+
temp.cleanup();
661+
}
662+
});

‎tools/check-runtime-boundary.ts‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -683,6 +683,21 @@ export async function gitChanges(
683683
return changes;
684684
}
685685

686+
/** The baseline committed at `rev`, or undefined when that revision has none (it predates the ratchet).
687+
* Throws for a malformed baseline, like parseBaseline. */
688+
export async function baselineAt(
689+
root: string,
690+
rev: string,
691+
): Promise<Baseline | undefined> {
692+
const spec = `${rev}:${kBaselineRelPath}`;
693+
try {
694+
await runGit(root, ["cat-file", "-e", spec]);
695+
} catch {
696+
return undefined;
697+
}
698+
return parseBaseline(await runGit(root, ["show", spec]));
699+
}
700+
686701
const isCount = (n: unknown) => Number.isInteger(n) && (n as number) >= 0;
687702

688703
/** Parses a baseline file, rejecting any scope without a non-negative integer for each field:
@@ -798,7 +813,15 @@ if (import.meta.main) {
798813
console.error((e as Error).message);
799814
Deno.exit(2);
800815
}
801-
const rendered = renderDelta(diffChanges(changes), current, baseline);
816+
// "main is above its baseline" is about the base revision's baseline: the PR's own
817+
// baseline may already be lowered or raised.
818+
let baseBaseline = baseline;
819+
try {
820+
baseBaseline = (await baselineAt(root, against)) ?? baseline;
821+
} catch (e) {
822+
usage(`cannot use ${kBaselineRelPath} at ${against}: ${(e as Error).message}`);
823+
}
824+
const rendered = renderDelta(diffChanges(changes), current, baseBaseline);
802825
for (const message of rendered.notices) {
803826
notice(message);
804827
}

0 commit comments

Comments
 (0)