github/codeql-action
1
0
Fork 0
mirror of https://github.com/github/codeql-action.git synced 2026-10-03 09:14:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

Address review comments

Browse Source
This commit is contained in:
Henry Mercer
2026-05-20 15:39:14 +01:00
parent 2c8faa5e9f
commit a14f75e3ac
4 changed files with 68 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
.github/workflows/pr-checks.yml vendored
View File

@@ -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: "<!-- 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)

35
pr-checks/check-repo-size.test.ts
View File

@@ -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 {

48
pr-checks/check-repo-size.ts
View File

@@ -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 = "<!-- repo-size-diff-bot -->";
@@ -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/<baseRef>` 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<number> {
const args = readArgs();
// 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}...`);
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<number> {
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<void> {
try {
process.exit(await main());
} catch (err) {
console.error(getErrorMessage(err));
process.exit(1);
}
}
if (require.main === module) {
void run();
}

7
pr-checks/config.ts
View File

@@ -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(