Address review comments

This commit is contained in:
Henry Mercer
2026-05-18 20:08:43 +01:00
parent 9b6438e936
commit 15a712bbc2
6 changed files with 207 additions and 331 deletions
+94 -25
View File
@@ -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: "<!-- repo-size-diff-bot -->"
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
-2
View File
@@ -10408,8 +10408,6 @@
},
"devDependencies": {
"@types/node": "^20.19.39",
"@types/sinon": "^21.0.1",
"sinon": "^22.0.0",
"tsx": "^4.21.0"
}
}
+68 -140
View File
@@ -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, "\\$&");
}
+44 -162
View File
@@ -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 = "<!-- repo-size-diff-bot -->";
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<UpsertResult> {
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/<baseRef>` 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<number> {
const args = readArgs();
// The script lives at `<repoRoot>/pr-checks/check-repo-size.ts`, so the repo
// root is always the parent directory.
// The script lives at `<repoRoot>/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<number> {
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<number> {
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;
}
+1
View File
@@ -12,5 +12,6 @@ is:
- "Agent"
- "Cleanup artifacts"
- "Prepare"
- "Post repo size comment"
- "Upload results"
- "Label PR with size"
-2
View File
@@ -11,8 +11,6 @@
},
"devDependencies": {
"@types/node": "^20.19.39",
"@types/sinon": "^21.0.1",
"sinon": "^22.0.0",
"tsx": "^4.21.0"
}
}