From a14f75e3ac73e9f581a27093221e895f0d08b3b6 Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Wed, 20 May 2026 15:39:14 +0100 Subject: [PATCH] Address review comments --- .github/workflows/pr-checks.yml | 40 ++++++++++++++------------ pr-checks/check-repo-size.test.ts | 35 +++++++++++----------- pr-checks/check-repo-size.ts | 48 +++++++++++++++++-------------- pr-checks/config.ts | 7 +++-- 4 files changed, 68 insertions(+), 62 deletions(-) diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 6dee8fc30..7f45c28ae 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -73,8 +73,8 @@ jobs: # These checks do not need to be run as part of the same matrix that we use for the `unit-tests` # job. - pr-checks: - name: PR Checks + other-checks: + name: Other checks if: github.triggering_actor != 'dependabot[bot]' permissions: contents: read @@ -96,12 +96,15 @@ jobs: cache: 'npm' - name: Install dependencies + id: install-deps run: npm ci - name: Verify PR checks up to date + if: ${{ !cancelled() && steps.install-deps.outcome == 'success' }} run: .github/workflows/script/verify-pr-checks.sh - name: Run pr-checks tests + if: ${{ !cancelled() && steps.install-deps.outcome == 'success' }} working-directory: pr-checks run: npx tsx --test @@ -117,6 +120,7 @@ jobs: echo "node_version=${NODE_VERSION}" >> $GITHUB_OUTPUT - name: Fetch base commit + id: fetch-base # Forks and Dependabot PRs don't have permission to write comments, so skip the repo size # check in those cases. if: >- @@ -125,29 +129,28 @@ jobs: github.event.pull_request.user.login != 'dependabot[bot]' env: BASE_SHA: ${{ github.event.pull_request.base.sha }} - run: git fetch --no-tags --depth=1 origin "$BASE_SHA" + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + # Compare against the merge base so the size delta reflects only the commits actually + # added by this PR, ignoring any changes that have landed on the base branch since the + # PR branched off. + merge_base=$(gh api "repos/$GITHUB_REPOSITORY/compare/$BASE_SHA...$HEAD_SHA" --jq '.merge_base_commit.sha') + echo "merge_base=$merge_base" >> "$GITHUB_OUTPUT" + git fetch --no-tags --depth=1 origin "$merge_base" "$HEAD_SHA" - name: Check repo size - # Forks and Dependabot PRs don't have permission to write comments, so skip the repo size - # check in those cases. - if: >- - github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository && - github.event.pull_request.user.login != 'dependabot[bot]' + if: steps.fetch-base.outcome == 'success' working-directory: pr-checks env: BASE_REF: ${{ github.event.pull_request.base.ref }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} + BASE_SHA: ${{ steps.fetch-base.outputs.merge_base }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: npx tsx check-repo-size.ts --output-dir "$RUNNER_TEMP/repo-size" - name: Upload repo size comment - # Forks and Dependabot PRs don't have permission to write comments, so skip the repo size - # check in those cases. - if: >- - github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository && - github.event.pull_request.user.login != 'dependabot[bot]' + if: steps.fetch-base.outcome == 'success' uses: actions/upload-artifact@v7 with: name: repo-size-comment @@ -176,14 +179,14 @@ jobs: post-repo-size-comment: name: Post repo size comment - needs: pr-checks + needs: other-checks # Keep write permissions isolated from the job that checks out and tests PR code. This job only # posts the candidate comment body produced by the read-only `pr-checks` job. if: >- github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && github.event.pull_request.user.login != 'dependabot[bot]' && - needs.pr-checks.result == 'success' + needs.other-checks.result == 'success' permissions: contents: read pull-requests: write @@ -205,7 +208,6 @@ jobs: env: COMMENT_MARKER: "" GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | significant=$(jq -r '.significant' repo-size-comment/metadata.json) diff --git a/pr-checks/check-repo-size.test.ts b/pr-checks/check-repo-size.test.ts index 1cd8d3c8d..3299fa03c 100644 --- a/pr-checks/check-repo-size.test.ts +++ b/pr-checks/check-repo-size.test.ts @@ -25,24 +25,18 @@ import { describe("formatBytes", async () => { const cases: Array<[number, boolean, string]> = [ - // Unsigned: bytes / KiB / MiB boundaries. - [0, false, "0 B"], - [1, false, "1 B"], - [1023, false, "1023 B"], + // Unsigned values, including sub-KiB amounts which round to 0.00. + [0, false, "0.00 KiB"], + [512, false, "0.50 KiB"], [1024, false, "1.00 KiB"], - [2048, false, "2.00 KiB"], - [1024 * 1024 - 1, false, "1024.00 KiB"], - [1024 * 1024, false, "1.00 MiB"], - [2.5 * 1024 * 1024, false, "2.50 MiB"], + [1024 * 1024, false, "1024.00 KiB"], + [2 * 1024 * 1024, false, "2048.00 KiB"], // Negative values always use a leading minus. - [-512, false, "-512 B"], - [-2048, false, "-2.00 KiB"], - [-2 * 1024 * 1024, false, "-2.00 MiB"], + [-2 * 1024 * 1024, false, "-2048.00 KiB"], // signed=true prepends a + to non-negative values. - [0, true, "+0 B"], - [512, true, "+512 B"], - [2048, true, "+2.00 KiB"], - [-512, true, "-512 B"], + [0, true, "+0.00 KiB"], + [2 * 1024 * 1024, true, "+2048.00 KiB"], + [-2 * 1024 * 1024, true, "-2048.00 KiB"], ]; for (const [bytes, signed, expected] of cases) { await it(`formats ${bytes} (signed=${signed}) as ${expected}`, () => { @@ -94,8 +88,8 @@ describe("buildCommentBody", async () => { }); assert.match(body, new RegExp(`^${escapeRegExp(COMMENT_MARKER)}`)); - assert.match(body, /Base \(`main`\) \| 1\.91 MiB \(2000000 bytes\)/); - assert.match(body, /This PR \| 2\.19 MiB \(2300000 bytes\)/); + assert.match(body, /Base \(`main`\) \| 1953\.13 KiB \(2000000 bytes\)/); + assert.match(body, /This PR \| 2246\.09 KiB \(2300000 bytes\)/); assert.match( body, /\*\*Delta\*\* \| \*\*\+292\.97 KiB \(\+300000 bytes, \+15\.00%\)\*\*/, @@ -118,7 +112,7 @@ describe("buildCommentBody", async () => { }); describe("readArgs", async () => { - await it("defaults the base ref for local runs", () => { + await it("defaults the base ref and head commit for local runs", () => { const originalEnv = process.env; const originalArgv = process.argv; @@ -130,6 +124,7 @@ describe("readArgs", async () => { assert.equal(args.baseRef, DEFAULT_BASE_REF); assert.equal(args.baseCommitish, `origin/${DEFAULT_BASE_REF}`); + assert.equal(args.headCommitish, "HEAD"); assert.equal(args.outputDir, "/tmp/out"); assert.equal(args.runUrl, undefined); } finally { @@ -138,7 +133,7 @@ describe("readArgs", async () => { } }); - await it("uses the base SHA when provided by the workflow", () => { + await it("uses the base and head SHAs when provided by the workflow", () => { const originalEnv = process.env; const originalArgv = process.argv; @@ -146,6 +141,7 @@ describe("readArgs", async () => { process.env = { BASE_REF: "main", BASE_SHA: "abc123", + HEAD_SHA: "def456", RUN_URL: "https://example.test/run", }; process.argv = ["node", "check-repo-size.ts", "--output-dir", "/tmp/out"]; @@ -154,6 +150,7 @@ describe("readArgs", async () => { assert.equal(args.baseRef, "main"); assert.equal(args.baseCommitish, "abc123"); + assert.equal(args.headCommitish, "def456"); assert.equal(args.outputDir, "/tmp/out"); assert.equal(args.runUrl, "https://example.test/run"); } finally { diff --git a/pr-checks/check-repo-size.ts b/pr-checks/check-repo-size.ts index 11e6abf1f..a102494f2 100644 --- a/pr-checks/check-repo-size.ts +++ b/pr-checks/check-repo-size.ts @@ -14,6 +14,10 @@ import * as fs from "node:fs"; import * as path from "node:path"; import { parseArgs } from "node:util"; +import { getErrorMessage } from "../src/util"; + +import { REPO_ROOT } from "./config"; + /** Hidden marker used to find the existing sticky comment on a PR. */ export const COMMENT_MARKER = ""; @@ -62,15 +66,13 @@ export async function measureArchiveSize( } /** - * Format a byte count into a human-readable string with binary units. If `signed` is true, a - * leading `+` is prepended for non-negative values so gains and losses are visually distinct. + * Format a byte count as KiB. If `signed` is true, a leading `+` is prepended for non-negative + * values so gains and losses are visually distinct. */ export function formatBytes(bytes: number, signed = false): string { const sign = bytes < 0 ? "-" : signed ? "+" : ""; - const abs = Math.abs(bytes); - if (abs < 1024) return `${sign}${abs} B`; - if (abs < 1024 * 1024) return `${sign}${(abs / 1024).toFixed(2)} KiB`; - return `${sign}${(abs / 1024 / 1024).toFixed(2)} MiB`; + const kib = Math.abs(bytes) / 1024; + return `${sign}${kib.toFixed(2)} KiB`; } /** Format a fraction as a signed percentage with 2 decimal places. */ @@ -131,6 +133,8 @@ interface MainArgs { baseRef: string; /** Base commit-ish to archive. Defaults to `origin/` for local runs. */ baseCommitish: string; + /** Head commit-ish to archive. Defaults to `HEAD` for local runs. */ + headCommitish: string; /** Optional URL of the workflow run, surfaced in the comment footer. */ runUrl?: string; /** Directory where `body.md` and `metadata.json` are written. */ @@ -152,10 +156,12 @@ export function readArgs(): MainArgs { const baseRef = process.env.BASE_REF ?? DEFAULT_BASE_REF; const baseCommitish = process.env.BASE_SHA ?? `origin/${baseRef}`; + const headCommitish = process.env.HEAD_SHA ?? "HEAD"; return { baseRef, baseCommitish, + headCommitish, runUrl: process.env.RUN_URL, outputDir, }; @@ -164,16 +170,12 @@ export function readArgs(): MainArgs { async function main(): Promise { const args = readArgs(); - // The script lives at `/pr-checks/check-repo-size.ts`, so the repo root is the parent - // directory. - const repoRoot = path.resolve(__dirname, ".."); - console.log(`Measuring base archive size for ${args.baseCommitish}...`); - const baseSize = await measureArchiveSize(args.baseCommitish, repoRoot); + const baseSize = await measureArchiveSize(args.baseCommitish, REPO_ROOT); console.log(` ${baseSize} bytes`); - console.log("Measuring PR archive size for HEAD..."); - const prSize = await measureArchiveSize("HEAD", repoRoot); + console.log(`Measuring PR archive size for ${args.headCommitish}...`); + const prSize = await measureArchiveSize(args.headCommitish, REPO_ROOT); console.log(` ${prSize} bytes`); const delta = prSize - baseSize; @@ -209,13 +211,15 @@ async function main(): Promise { return 0; } -if (require.main === module) { - void (async () => { - try { - process.exit(await main()); - } catch (err) { - console.error(err instanceof Error ? err.message : String(err)); - process.exit(1); - } - })(); +async function run(): Promise { + try { + process.exit(await main()); + } catch (err) { + console.error(getErrorMessage(err)); + process.exit(1); + } +} + +if (require.main === module) { + void run(); } diff --git a/pr-checks/config.ts b/pr-checks/config.ts index 92c8beef0..4c7cc03f3 100644 --- a/pr-checks/config.ts +++ b/pr-checks/config.ts @@ -6,14 +6,17 @@ export const OLDEST_SUPPORTED_MAJOR_VERSION = 3; /** The `pr-checks` directory. */ export const PR_CHECKS_DIR = __dirname; +/** The repository root. */ +export const REPO_ROOT = path.join(PR_CHECKS_DIR, ".."); + /** The path of the file configuring which checks shouldn't be required. */ export const PR_CHECK_EXCLUDED_FILE = path.join(PR_CHECKS_DIR, "excluded.yml"); /** The path to the esbuild metadata file. */ -export const BUNDLE_METADATA_FILE = path.join(PR_CHECKS_DIR, "..", "meta.json"); +export const BUNDLE_METADATA_FILE = path.join(REPO_ROOT, "meta.json"); /** The `src` directory. */ -const SOURCE_ROOT = path.join(PR_CHECKS_DIR, "..", "src"); +const SOURCE_ROOT = path.join(REPO_ROOT, "src"); /** The path to the built-in languages file. */ export const BUILTIN_LANGUAGES_FILE = path.join(