From 15a712bbc2336f64eee2e053e438db3c90ee6e29 Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Mon, 18 May 2026 20:08:43 +0100 Subject: [PATCH] Address review comments --- .github/workflows/pr-checks.yml | 119 +++++++++++++---- package-lock.json | 2 - pr-checks/check-repo-size.test.ts | 208 ++++++++++-------------------- pr-checks/check-repo-size.ts | 206 +++++++---------------------- pr-checks/excluded.yml | 1 + pr-checks/package.json | 2 - 6 files changed, 207 insertions(+), 331 deletions(-) diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 55ad7d56a..f2f781e43 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -78,8 +78,7 @@ jobs: if: github.triggering_actor != 'dependabot[bot]' permissions: contents: read - pull-requests: write - runs-on: ubuntu-slim + runs-on: ubuntu-latest timeout-minutes: 10 concurrency: @@ -89,10 +88,6 @@ jobs: steps: - name: Checkout repository uses: actions/checkout@v6 - with: - # Need full history so we have both the PR merge commit (HEAD) and the base SHA locally - # for `git archive` to work against either. - fetch-depth: 0 - name: Set up Node.js uses: actions/setup-node@v6 @@ -110,27 +105,10 @@ jobs: working-directory: pr-checks run: npx tsx --test - - name: Check repo size - # Forks and Dependabot PRs don't have permission to write comments, so skip the 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]' - working-directory: pr-checks - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - BASE_REF: ${{ github.event.pull_request.base.ref }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} - PR_NUMBER: ${{ github.event.pull_request.number }} - GITHUB_REPOSITORY: ${{ github.repository }} - RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} - run: npx tsx check-repo-size.ts - - name: Verify all Actions use the same Node version id: head-version run: | - NODE_VERSION=$(find . -name "action.yml" -exec yq -e '.runs.using' {} \; | grep node | sort | uniq) + NODE_VERSION=$(find . -path "*/node_modules" -prune -o -name "action.yml" -exec yq -o=json '.runs.using' {} \; | jq -rs '[.[] | select(. != null and startswith("node"))] | unique | .[]') echo "NODE_VERSION: ${NODE_VERSION}" if [[ $(echo "$NODE_VERSION" | wc -l) -gt 1 ]]; then echo "::error::More than one node version used in 'action.yml' files." @@ -138,6 +116,44 @@ jobs: fi echo "node_version=${NODE_VERSION}" >> $GITHUB_OUTPUT + - name: Fetch base commit + # 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]' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: git fetch --no-tags --depth=1 origin "$BASE_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]' + working-directory: pr-checks + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + BASE_SHA: ${{ github.event.pull_request.base.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]' + uses: actions/upload-artifact@v7 + with: + name: repo-size-comment + path: ${{ runner.temp }}/repo-size/ + if-no-files-found: error + - name: 'Backport: Check out base ref' id: checkout-base if: ${{ startsWith(github.head_ref, 'backport-') }} @@ -150,10 +166,63 @@ jobs: env: HEAD_VERSION: ${{ steps.head-version.outputs.node_version }} run: | - BASE_VERSION=$(find . -name "action.yml" -exec yq -e '.runs.using' {} \; | grep node | sort | uniq) + BASE_VERSION=$(find . -path "*/node_modules" -prune -o -name "action.yml" -exec yq -o=json '.runs.using' {} \; | jq -rs '[.[] | select(. != null and startswith("node"))] | unique | .[]') echo "HEAD_VERSION: ${HEAD_VERSION}" echo "BASE_VERSION: ${BASE_VERSION}" if [[ "$BASE_VERSION" != "$HEAD_VERSION" ]]; then echo "::error::Cannot change the Node version of an Action in a backport PR." exit 1 fi + + post-repo-size-comment: + name: Post repo size comment + needs: pr-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' + permissions: + contents: read + pull-requests: write + runs-on: ubuntu-slim + timeout-minutes: 10 + + concurrency: + cancel-in-progress: true + group: check-repo-size-${{ github.event.pull_request.number }} + + steps: + - name: Download repo size comment + uses: actions/download-artifact@v8 + with: + name: repo-size-comment + path: repo-size-comment + + - name: Post repo size comment + 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) + body=$(cat repo-size-comment/body.md) + comment_id=$( + gh api "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments" \ + --paginate \ + --jq ".[] | select(.body | contains(\"$COMMENT_MARKER\")) | .id" \ + | head -n 1 + ) + + if [[ -n "$comment_id" ]]; then + echo "Updating existing comment $comment_id." + gh api --method PATCH "repos/$GITHUB_REPOSITORY/issues/comments/$comment_id" --field body="$body" + elif [[ "$significant" == "true" ]]; then + echo "Creating new repo size comment." + gh api --method POST "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments" --field body="$body" + else + echo "Skipping repo size comment because the delta is below the threshold and no sticky comment exists." + fi diff --git a/package-lock.json b/package-lock.json index c4c542a01..6608c0560 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10408,8 +10408,6 @@ }, "devDependencies": { "@types/node": "^20.19.39", - "@types/sinon": "^21.0.1", - "sinon": "^22.0.0", "tsx": "^4.21.0" } } diff --git a/pr-checks/check-repo-size.test.ts b/pr-checks/check-repo-size.test.ts index d91316547..1cd8d3c8d 100644 --- a/pr-checks/check-repo-size.test.ts +++ b/pr-checks/check-repo-size.test.ts @@ -1,3 +1,9 @@ +#!/usr/bin/env npx tsx + +/* +Tests for check-repo-size.ts. +*/ + import * as assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; import { randomBytes } from "node:crypto"; @@ -6,17 +12,15 @@ import * as os from "node:os"; import * as path from "node:path"; import { afterEach, beforeEach, describe, it } from "node:test"; -import * as sinon from "sinon"; - -import { type ApiClient, getApiClient } from "./api-client"; import { COMMENT_MARKER, + DEFAULT_BASE_REF, buildCommentBody, formatBytes, formatPercent, isDeltaSignificant, measureArchiveSize, - upsertSizeComment, + readArgs, } from "./check-repo-size"; describe("formatBytes", async () => { @@ -113,6 +117,66 @@ describe("buildCommentBody", async () => { }); }); +describe("readArgs", async () => { + await it("defaults the base ref for local runs", () => { + const originalEnv = process.env; + const originalArgv = process.argv; + + try { + process.env = {}; + process.argv = ["node", "check-repo-size.ts", "--output-dir", "/tmp/out"]; + + const args = readArgs(); + + assert.equal(args.baseRef, DEFAULT_BASE_REF); + assert.equal(args.baseCommitish, `origin/${DEFAULT_BASE_REF}`); + assert.equal(args.outputDir, "/tmp/out"); + assert.equal(args.runUrl, undefined); + } finally { + process.env = originalEnv; + process.argv = originalArgv; + } + }); + + await it("uses the base SHA when provided by the workflow", () => { + const originalEnv = process.env; + const originalArgv = process.argv; + + try { + process.env = { + BASE_REF: "main", + BASE_SHA: "abc123", + RUN_URL: "https://example.test/run", + }; + process.argv = ["node", "check-repo-size.ts", "--output-dir", "/tmp/out"]; + + const args = readArgs(); + + assert.equal(args.baseRef, "main"); + assert.equal(args.baseCommitish, "abc123"); + assert.equal(args.outputDir, "/tmp/out"); + assert.equal(args.runUrl, "https://example.test/run"); + } finally { + process.env = originalEnv; + process.argv = originalArgv; + } + }); + + await it("throws when --output-dir is missing", () => { + const originalEnv = process.env; + const originalArgv = process.argv; + + try { + process.env = {}; + process.argv = ["node", "check-repo-size.ts"]; + assert.throws(() => readArgs(), /--output-dir is required/); + } finally { + process.env = originalEnv; + process.argv = originalArgv; + } + }); +}); + let repoDir: string; beforeEach(() => { @@ -193,142 +257,6 @@ describe("measureArchiveSize", async () => { }); }); -describe("upsertSizeComment", async () => { - const owner = "test-owner"; - const repo = "test-repo"; - const prNumber = 42; - - let client: ApiClient; - - beforeEach(() => { - client = getApiClient("test-token"); - }); - - afterEach(() => { - sinon.restore(); - }); - - function stubExistingComments(comments: Array<{ id: number; body: string }>) { - // upsertSizeComment calls `client.paginate(...)`, so stubbing `paginate` - // directly mocks the listing without depending on how paginate walks - // Octokit's response (link headers etc.). - return sinon.stub(client, "paginate").resolves(comments); - } - - await it("creates a new comment when none exists and the delta is significant", async () => { - stubExistingComments([]); - const createStub = sinon - .stub(client.rest.issues, "createComment") - .resolves({ data: { id: 999 } } as never); - - const result = await upsertSizeComment({ - client, - owner, - repo, - prNumber, - body: `${COMMENT_MARKER}\nhello`, - delta: 200, - baseSize: 1000, - }); - - assert.deepEqual(result, { action: "created", commentId: 999 }); - sinon.assert.calledOnce(createStub); - const createArgs = createStub.firstCall.args[0]!; - assert.equal(createArgs.owner, owner); - assert.equal(createArgs.repo, repo); - assert.equal(createArgs.issue_number, prNumber); - assert.ok(createArgs.body.includes(COMMENT_MARKER)); - }); - - await it("creates a new comment for a significant size decrease", async () => { - // Shrinkage matters too: it might indicate accidentally deleted tracked - // files. The full pipeline (not just isDeltaSignificant) needs to post on - // negative deltas. - stubExistingComments([]); - const createStub = sinon - .stub(client.rest.issues, "createComment") - .resolves({ data: { id: 999 } } as never); - - const result = await upsertSizeComment({ - client, - owner, - repo, - prNumber, - body: `${COMMENT_MARKER}\nhello`, - delta: -200, - baseSize: 1000, - }); - - assert.deepEqual(result, { action: "created", commentId: 999 }); - sinon.assert.calledOnce(createStub); - }); - - await it("skips when no existing comment and delta is below threshold", async () => { - stubExistingComments([]); - const createStub = sinon.stub(client.rest.issues, "createComment"); - const updateStub = sinon.stub(client.rest.issues, "updateComment"); - - const result = await upsertSizeComment({ - client, - owner, - repo, - prNumber, - body: `${COMMENT_MARKER}\nhello`, - delta: 50, - baseSize: 1000, - }); - - assert.equal(result.action, "skipped"); - sinon.assert.notCalled(createStub); - sinon.assert.notCalled(updateStub); - }); - - await it("updates the existing comment when the delta is significant", async () => { - stubExistingComments([{ id: 7, body: `${COMMENT_MARKER}\nold body` }]); - const updateStub = sinon - .stub(client.rest.issues, "updateComment") - .resolves({ data: { id: 7 } } as never); - - const result = await upsertSizeComment({ - client, - owner, - repo, - prNumber, - body: `${COMMENT_MARKER}\nnew body`, - delta: 200, - baseSize: 1000, - }); - - assert.deepEqual(result, { action: "updated", commentId: 7 }); - sinon.assert.calledOnce(updateStub); - const updateArgs = updateStub.firstCall.args[0]!; - assert.equal(updateArgs.comment_id, 7); - assert.ok(updateArgs.body.includes("new body")); - }); - - await it("updates an existing comment even when the delta is below threshold", async () => { - // This keeps the comment in sync after a PR that initially had a big diff - // gets reduced below the threshold by a follow-up commit. - stubExistingComments([{ id: 7, body: `${COMMENT_MARKER}\nold body` }]); - const updateStub = sinon - .stub(client.rest.issues, "updateComment") - .resolves({ data: { id: 7 } } as never); - - const result = await upsertSizeComment({ - client, - owner, - repo, - prNumber, - body: `${COMMENT_MARKER}\nnew body`, - delta: 1, - baseSize: 1000, - }); - - assert.deepEqual(result, { action: "updated", commentId: 7 }); - sinon.assert.calledOnce(updateStub); - }); -}); - function escapeRegExp(s: string): string { return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } diff --git a/pr-checks/check-repo-size.ts b/pr-checks/check-repo-size.ts index 0b7d8cdcf..11e6abf1f 100644 --- a/pr-checks/check-repo-size.ts +++ b/pr-checks/check-repo-size.ts @@ -1,32 +1,28 @@ #!/usr/bin/env npx tsx /* -Computes the difference in the `.tar.gz`'d checkout size of the repo between the PR head and the PR -base, and posts/updates a sticky comment on the PR when the change is significant in either -direction. This size is relevant because it corresponds to the duration of the "Download action +Measures the difference in the `.tar.gz`'d checkout size of the repo between the PR head and the PR +base. This size is relevant because it corresponds to the duration of the "Download action repository" step that happens at the start of every job that uses this Action. -Designed to be invoked from the `Check repo size` workflow on PR events, but also runnable locally -(with --dry-run) for testing. +Writes the candidate sticky-comment body and a small metadata file to `--output-dir`. A separate +workflow job consumes those artifacts and decides whether to create or update a PR comment. */ import { spawn } from "node:child_process"; +import * as fs from "node:fs"; import * as path from "node:path"; import { parseArgs } from "node:util"; -import { type ApiClient, getApiClient } from "./api-client"; - /** Hidden marker used to find the existing sticky comment on a PR. */ export const COMMENT_MARKER = ""; export const DEFAULT_BASE_REF = "main"; -export const DEFAULT_REPOSITORY = "github/codeql-action"; /** - * Fraction of the base archive size at which a delta is considered - * significant enough to warrant a new sticky comment. We always update an - * existing comment regardless, so the comment stays in sync as the diff - * evolves. + * Fraction of the base archive size at which a delta is considered significant enough to warrant + * a new sticky comment. We always update an existing comment regardless, so the comment stays in + * sync as the diff evolves. */ export const SIGNIFICANT_DELTA_FRACTION = 0.1; @@ -66,9 +62,8 @@ 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 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. */ export function formatBytes(bytes: number, signed = false): string { const sign = bytes < 0 ? "-" : signed ? "+" : ""; @@ -131,130 +126,46 @@ export function isDeltaSignificant( return Math.abs(delta) >= baseSize * fraction; } -export interface UpsertOptions { - client: ApiClient; - owner: string; - repo: string; - prNumber: number; - body: string; - delta: number; - baseSize: number; -} - -export type UpsertResult = - | { action: "updated"; commentId: number } - | { action: "created"; commentId: number } - | { action: "skipped"; reason: string }; - -/** - * Find an existing sticky comment on the PR by HTML marker. If one exists, - * always update it (so it stays in sync). Otherwise, only create a new - * comment when the delta is currently significant. - */ -export async function upsertSizeComment( - opts: UpsertOptions, -): Promise { - const { client, owner, repo, prNumber, body, delta, baseSize } = opts; - - const comments = await client.paginate( - "GET /repos/{owner}/{repo}/issues/{issue_number}/comments", - { - owner, - repo, - issue_number: prNumber, - per_page: 100, - }, - ); - const existing = comments.find((c) => - (c.body ?? "").includes(COMMENT_MARKER), - ); - - if (existing) { - await client.rest.issues.updateComment({ - owner, - repo, - comment_id: existing.id, - body, - }); - return { action: "updated", commentId: existing.id }; - } - - if (isDeltaSignificant(delta, baseSize, SIGNIFICANT_DELTA_FRACTION)) { - const { data } = await client.rest.issues.createComment({ - owner, - repo, - issue_number: prNumber, - body, - }); - return { action: "created", commentId: data.id }; - } - - return { - action: "skipped", - reason: - `delta ${delta} bytes is below ` + - `${(SIGNIFICANT_DELTA_FRACTION * 100).toFixed(2)}% of base size ` + - `${baseSize} bytes`, - }; -} - interface MainArgs { - /** Base ref of the PR. Defaults to `main`, and is used as the label in the PR comment. */ + /** Base ref of the PR. Defaults to `main`. Used as the label in the PR comment. */ baseRef: string; - /** Base commit to archive. Defaults to `origin/main` for local dry runs. */ + /** Base commit-ish to archive. Defaults to `origin/` for local runs. */ baseCommitish: string; - /** Numeric PR number used to find / create / update the sticky comment. Required outside dry-run. */ - prNumber?: number; - /** `owner/repo` slug, defaulting to `github/codeql-action`, split before being passed to Octokit. */ - ownerRepo: string; /** Optional URL of the workflow run, surfaced in the comment footer. */ runUrl?: string; - /** When true, log the would-be comment instead of calling GitHub. */ - dryRun: boolean; - /** GitHub token used to authenticate Octokit. Required unless `dryRun` is true. */ - token?: string; + /** Directory where `body.md` and `metadata.json` are written. */ + outputDir: string; } export function readArgs(): MainArgs { const { values } = parseArgs({ options: { - "dry-run": { type: "boolean", default: false }, + "output-dir": { type: "string" }, }, strict: true, }); - const dryRun = values["dry-run"] ?? false; + const outputDir = values["output-dir"]; + if (!outputDir) { + throw new Error("--output-dir is required"); + } + const baseRef = process.env.BASE_REF ?? DEFAULT_BASE_REF; const baseCommitish = process.env.BASE_SHA ?? `origin/${baseRef}`; - const prNumberStr = process.env.PR_NUMBER; - const repo = process.env.GITHUB_REPOSITORY ?? DEFAULT_REPOSITORY; - - let prNumber: number | undefined; - if (prNumberStr) { - prNumber = Number.parseInt(prNumberStr, 10); - if (!Number.isFinite(prNumber)) { - throw new Error(`Invalid PR_NUMBER value: ${prNumberStr}`); - } - } else if (!dryRun) { - throw new Error("Missing PR_NUMBER env var"); - } return { baseRef, baseCommitish, - prNumber, - ownerRepo: repo, runUrl: process.env.RUN_URL, - dryRun, - token: process.env.GITHUB_TOKEN, + outputDir, }; } async function main(): Promise { const args = readArgs(); - // The script lives at `/pr-checks/check-repo-size.ts`, so the repo - // root is always the parent directory. + // 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}...`); @@ -266,7 +177,16 @@ async function main(): Promise { console.log(` ${prSize} bytes`); const delta = prSize - baseSize; - console.log(`Delta: ${delta} bytes`); + const significant = isDeltaSignificant( + delta, + baseSize, + SIGNIFICANT_DELTA_FRACTION, + ); + console.log( + `Delta: ${delta} bytes (significant=${significant}, threshold=${( + SIGNIFICANT_DELTA_FRACTION * 100 + ).toFixed(2)}%)`, + ); const body = buildCommentBody({ baseRef: args.baseRef, @@ -275,55 +195,17 @@ async function main(): Promise { runUrl: args.runUrl, }); - if (args.dryRun) { - const significant = isDeltaSignificant( - delta, - baseSize, - SIGNIFICANT_DELTA_FRACTION, - ); - console.log( - `--dry-run: significant=${significant} (threshold ${( - SIGNIFICANT_DELTA_FRACTION * 100 - ).toFixed(2)}%); would post:\n${body}`, - ); - return 0; - } - - if (!args.token) { - throw new Error( - "GITHUB_TOKEN env var is required when not running with --dry-run", - ); - } - if (args.prNumber === undefined) { - throw new Error("Missing PR_NUMBER env var"); - } - - const [owner, repo] = args.ownerRepo.split("/"); - if (!owner || !repo) { - throw new Error(`Invalid GITHUB_REPOSITORY value: ${args.ownerRepo}`); - } - - const result = await upsertSizeComment({ - client: getApiClient(args.token), - owner, - repo, - prNumber: args.prNumber, - body, - delta, - baseSize, - }); - - switch (result.action) { - case "updated": - console.log(`Updated existing comment ${result.commentId}.`); - break; - case "created": - console.log(`Created new comment ${result.commentId}.`); - break; - case "skipped": - console.log(`Skipped commenting: ${result.reason}.`); - break; - } + fs.mkdirSync(args.outputDir, { recursive: true }); + fs.writeFileSync(path.join(args.outputDir, "body.md"), body); + fs.writeFileSync( + path.join(args.outputDir, "metadata.json"), + `${JSON.stringify( + { significant, baseRef: args.baseRef, baseSize, prSize, delta }, + null, + 2, + )}\n`, + ); + console.log(`Wrote body.md and metadata.json to ${args.outputDir}.`); return 0; } diff --git a/pr-checks/excluded.yml b/pr-checks/excluded.yml index 104c1009e..e2814240a 100644 --- a/pr-checks/excluded.yml +++ b/pr-checks/excluded.yml @@ -12,5 +12,6 @@ is: - "Agent" - "Cleanup artifacts" - "Prepare" + - "Post repo size comment" - "Upload results" - "Label PR with size" diff --git a/pr-checks/package.json b/pr-checks/package.json index 0032b5fc5..2741560f6 100644 --- a/pr-checks/package.json +++ b/pr-checks/package.json @@ -11,8 +11,6 @@ }, "devDependencies": { "@types/node": "^20.19.39", - "@types/sinon": "^21.0.1", - "sinon": "^22.0.0", "tsx": "^4.21.0" } }