From 1b3b63382800a2644e19a60d195f2097177d72a2 Mon Sep 17 00:00:00 2001 From: Sergey Ivochkin Date: Mon, 5 Oct 2026 14:00:58 +1100 Subject: [PATCH] fix(ci): stop labelling detached scans as main; upload the PR head sha (#98) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - detectRepoUrl no longer falls back to "main": a detached HEAD (how actions/checkout leaves a pull_request run) or a missing git yields branch "", and a missing remote no longer discards the real branch. - fileLink takes a ref; the HTML report links to meta.git.sha when the report carries it (permalinks), else the branch, else plain text — never /blob//. - --upload attributes the report to pull_request.head.sha on PR events, else GITHUB_SHA, else the local HEAD, so the quality-gate status lands on the PR head instead of GitHub's merge commit. - GitHub event payload fixtures (push, new-branch push, pull_request, pull_request_target, merge_group, workflow_dispatch) for these tests and the CI-context work that follows. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SmtSK7qDTyXhimJ9grap88 --- fixtures/github-events/merge_group.json | 26 ++++++++++ fixtures/github-events/pull_request.json | 33 ++++++++++++ .../github-events/pull_request_target.json | 33 ++++++++++++ fixtures/github-events/push-new-branch.json | 25 +++++++++ fixtures/github-events/push.json | 28 ++++++++++ fixtures/github-events/workflow_dispatch.json | 17 +++++++ src/detect.test.ts | 51 ++++++++++++++++++- src/detect.ts | 26 ++++++++-- src/report/components.test.ts | 26 +++++++++- src/report/components.ts | 17 +++++-- src/report/html.ts | 6 +-- src/report/pages.test.ts | 33 ++++++++++++ src/upload.test.ts | 51 ++++++++++++++++++- src/upload.ts | 25 ++++++++- 14 files changed, 378 insertions(+), 19 deletions(-) create mode 100644 fixtures/github-events/merge_group.json create mode 100644 fixtures/github-events/pull_request.json create mode 100644 fixtures/github-events/pull_request_target.json create mode 100644 fixtures/github-events/push-new-branch.json create mode 100644 fixtures/github-events/push.json create mode 100644 fixtures/github-events/workflow_dispatch.json diff --git a/fixtures/github-events/merge_group.json b/fixtures/github-events/merge_group.json new file mode 100644 index 0000000..ed2d2fd --- /dev/null +++ b/fixtures/github-events/merge_group.json @@ -0,0 +1,26 @@ +{ + "action": "checks_requested", + "merge_group": { + "head_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "head_ref": "refs/heads/gh-readonly-queue/main/pr-42-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "base_sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "base_ref": "refs/heads/main", + "head_commit": { + "id": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "timestamp": "2026-10-01T10:00:00Z", + "message": "Merge pull request #42" + } + }, + "repository": { + "full_name": "octo-org/widgets", + "name": "widgets", + "owner": { + "login": "octo-org" + }, + "default_branch": "main", + "html_url": "https://github.com/octo-org/widgets" + }, + "sender": { + "login": "octocat" + } +} diff --git a/fixtures/github-events/pull_request.json b/fixtures/github-events/pull_request.json new file mode 100644 index 0000000..d4d6589 --- /dev/null +++ b/fixtures/github-events/pull_request.json @@ -0,0 +1,33 @@ +{ + "action": "synchronize", + "number": 42, + "pull_request": { + "number": 42, + "head": { + "ref": "feature/login", + "sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "repo": { + "full_name": "octo-org/widgets" + } + }, + "base": { + "ref": "main", + "sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "repo": { + "full_name": "octo-org/widgets" + } + } + }, + "repository": { + "full_name": "octo-org/widgets", + "name": "widgets", + "owner": { + "login": "octo-org" + }, + "default_branch": "main", + "html_url": "https://github.com/octo-org/widgets" + }, + "sender": { + "login": "octocat" + } +} diff --git a/fixtures/github-events/pull_request_target.json b/fixtures/github-events/pull_request_target.json new file mode 100644 index 0000000..7a379ba --- /dev/null +++ b/fixtures/github-events/pull_request_target.json @@ -0,0 +1,33 @@ +{ + "action": "opened", + "number": 43, + "pull_request": { + "number": 43, + "head": { + "ref": "patch-1", + "sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "repo": { + "full_name": "someone/widgets" + } + }, + "base": { + "ref": "main", + "sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "repo": { + "full_name": "octo-org/widgets" + } + } + }, + "repository": { + "full_name": "octo-org/widgets", + "name": "widgets", + "owner": { + "login": "octo-org" + }, + "default_branch": "main", + "html_url": "https://github.com/octo-org/widgets" + }, + "sender": { + "login": "someone" + } +} diff --git a/fixtures/github-events/push-new-branch.json b/fixtures/github-events/push-new-branch.json new file mode 100644 index 0000000..61daf38 --- /dev/null +++ b/fixtures/github-events/push-new-branch.json @@ -0,0 +1,25 @@ +{ + "ref": "refs/heads/feature/new", + "before": "0000000000000000000000000000000000000000", + "after": "cccccccccccccccccccccccccccccccccccccccc", + "created": true, + "deleted": false, + "forced": false, + "head_commit": { + "id": "cccccccccccccccccccccccccccccccccccccccc", + "timestamp": "2026-10-01T09:30:00+10:00", + "message": "start" + }, + "repository": { + "full_name": "octo-org/widgets", + "name": "widgets", + "owner": { + "login": "octo-org" + }, + "default_branch": "main", + "html_url": "https://github.com/octo-org/widgets" + }, + "sender": { + "login": "octocat" + } +} diff --git a/fixtures/github-events/push.json b/fixtures/github-events/push.json new file mode 100644 index 0000000..6c9b23d --- /dev/null +++ b/fixtures/github-events/push.json @@ -0,0 +1,28 @@ +{ + "ref": "refs/heads/main", + "before": "dddddddddddddddddddddddddddddddddddddddd", + "after": "cccccccccccccccccccccccccccccccccccccccc", + "created": false, + "deleted": false, + "forced": false, + "head_commit": { + "id": "cccccccccccccccccccccccccccccccccccccccc", + "timestamp": "2026-10-01T09:30:00+10:00", + "message": "fix: things" + }, + "repository": { + "full_name": "octo-org/widgets", + "name": "widgets", + "owner": { + "login": "octo-org" + }, + "default_branch": "main", + "html_url": "https://github.com/octo-org/widgets" + }, + "pusher": { + "name": "octocat" + }, + "sender": { + "login": "octocat" + } +} diff --git a/fixtures/github-events/workflow_dispatch.json b/fixtures/github-events/workflow_dispatch.json new file mode 100644 index 0000000..8ba9b1c --- /dev/null +++ b/fixtures/github-events/workflow_dispatch.json @@ -0,0 +1,17 @@ +{ + "inputs": {}, + "ref": "refs/heads/main", + "repository": { + "full_name": "octo-org/widgets", + "name": "widgets", + "owner": { + "login": "octo-org" + }, + "default_branch": "main", + "html_url": "https://github.com/octo-org/widgets" + }, + "sender": { + "login": "octocat" + }, + "workflow": ".github/workflows/vcqa.yml" +} diff --git a/src/detect.test.ts b/src/detect.test.ts index fa553e6..2bb95bd 100644 --- a/src/detect.test.ts +++ b/src/detect.test.ts @@ -1,7 +1,9 @@ -import { mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { detectStack, detectWorkspace, parseYamlList } from "./detect.js"; +import { detectRepoUrl, detectStack, detectWorkspace, parseYamlList } from "./detect.js"; const TMP = join(import.meta.dirname!, "__test_fixture__"); @@ -576,3 +578,48 @@ describe("parseYamlList", () => { expect(parseYamlList(yaml, "packages")).toEqual(["packages/*", "apps/*"]); }); }); + +describe("detectRepoUrl", () => { + const git = (cwd: string, ...args: string[]) => + execFileSync("git", ["-c", "user.name=t", "-c", "user.email=t@example.com", "-c", "commit.gpgsign=false", ...args], { + cwd, + encoding: "utf-8", + stdio: ["pipe", "pipe", "pipe"], + }).trim(); + const repos: string[] = []; + const makeRepo = () => { + const dir = mkdtempSync(join(tmpdir(), "vcqa-detect-git-")); + repos.push(dir); + git(dir, "init", "-q", "-b", "trunk"); + writeFileSync(join(dir, "a.txt"), "a\n"); + git(dir, "add", "a.txt"); + git(dir, "commit", "-q", "-m", "init"); + git(dir, "remote", "add", "origin", "git@github.com:octo-org/widgets.git"); + return dir; + }; + afterEach(() => { + for (const dir of repos.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + it("reports the checked-out branch and an https repo url", () => { + expect(detectRepoUrl(makeRepo())).toEqual({ repoUrl: "https://github.com/octo-org/widgets", branch: "trunk" }); + }); + + it("never guesses main on a detached HEAD", () => { + const dir = makeRepo(); + git(dir, "checkout", "-q", "--detach"); + expect(detectRepoUrl(dir)).toEqual({ repoUrl: "https://github.com/octo-org/widgets", branch: "" }); + }); + + it("keeps the branch when there is no remote", () => { + const dir = makeRepo(); + git(dir, "remote", "remove", "origin"); + expect(detectRepoUrl(dir)).toEqual({ repoUrl: null, branch: "trunk" }); + }); + + it("returns an empty branch outside git", () => { + const dir = mkdtempSync(join(tmpdir(), "vcqa-detect-nogit-")); + repos.push(dir); + expect(detectRepoUrl(dir)).toEqual({ repoUrl: null, branch: "" }); + }); +}); diff --git a/src/detect.ts b/src/detect.ts index b11a4e0..8e28d00 100644 --- a/src/detect.ts +++ b/src/detect.ts @@ -1139,21 +1139,37 @@ function shouldSkipConventionEntry(entry: string): boolean { return discoveryConventions.ignoredConventionEntries.includes(entry) || entry.startsWith("."); } -/** Detect GitHub/GitLab repo URL from git remote. */ +/** Detect GitHub/GitLab repo URL from git remote, and the checked-out branch. + * + * `branch` is "" when it is not known — a detached HEAD (which is how + * `actions/checkout` leaves a pull_request run), or no git at all. It is never + * guessed: a guessed "main" mislabels every PR scan and points file links at + * the wrong tree. */ export function detectRepoUrl(cwd: string): { repoUrl: string | null; branch: string } { + return { repoUrl: detectRemoteUrl(cwd), branch: detectLocalBranch(cwd) ?? "" }; +} + +/** The current local branch, or null on a detached HEAD / outside git. */ +function detectLocalBranch(cwd: string): string | null { + try { + return execSync("git branch --show-current", { cwd, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim() || null; + } catch { + return null; + } +} + +function detectRemoteUrl(cwd: string): string | null { try { const remote = execSync("git remote get-url origin", { cwd, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim(); - const branch = execSync("git branch --show-current", { cwd, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim() || "main"; // Convert SSH to HTTPS - const url = remote + return remote .replace(/^git@github\.com:/, "https://github.com/") .replace(/^git@gitlab\.com:/, "https://gitlab.com/") .replace(/^git@bitbucket\.org:/, "https://bitbucket.org/") .replace(/^ssh:\/\/git@github\.com\//, "https://github.com/") .replace(/^ssh:\/\/git@gitlab\.com\//, "https://gitlab.com/") .replace(/\.git$/, ""); - return { repoUrl: url, branch }; } catch { - return { repoUrl: null, branch: "main" }; + return null; } } diff --git a/src/report/components.test.ts b/src/report/components.test.ts index 817f37d..4f29ffa 100644 --- a/src/report/components.test.ts +++ b/src/report/components.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { e, fileLink, gc, pc } from "./components.js"; +import { e, fileLink, gc, linkRef, pc } from "./components.js"; describe("e (HTML escape)", () => { it("escapes all dangerous characters", () => { @@ -37,12 +37,36 @@ describe("fileLink", () => { expect(result).toContain(">src/foo.ts:10"); // display text preserves original }); + it("links to a commit sha as a permalink", () => { + const sha = "a".repeat(40); + expect(fileLink("src/foo.ts", 3, "https://github.com/org/repo", sha)).toContain( + `https://github.com/org/repo/blob/${sha}/src/foo.ts#L3`, + ); + }); + + it("renders plain text instead of a /blob// link when there is no ref", () => { + const result = fileLink("src/foo.ts", 3, "https://github.com/org/repo", ""); + expect(result).toBe("src/foo.ts"); + expect(result).not.toContain("/blob//"); + }); + it("escapes special characters in path", () => { const result = fileLink("src/a&b.ts", undefined, "https://github.com/org/repo", "main"); expect(result).toContain("&"); }); }); +describe("linkRef", () => { + it("prefers the scanned commit sha over the branch", () => { + expect(linkRef({ branch: "main", git: { sha: "abc123" } })).toBe("abc123"); + }); + + it("falls back to the branch, then to no ref", () => { + expect(linkRef({ branch: "main", git: { sha: null } })).toBe("main"); + expect(linkRef({ branch: "" })).toBe(""); + }); +}); + describe("gc (grade color)", () => { it("returns green for A", () => { expect(gc("A")).toBe("#22c55e"); diff --git a/src/report/components.ts b/src/report/components.ts index 5aa9e1d..bb94405 100644 --- a/src/report/components.ts +++ b/src/report/components.ts @@ -13,14 +13,23 @@ export function e(s: string): string { return s.replace(/&/g, "&").replace(//g, ">").replace(/"/g, """).replace(/'/g, "'"); } -/** Make a file path a clickable GitHub link if repoUrl is available. */ -export function fileLink(path: string, line: number | undefined, repoUrl: string | null, branch: string): string { +/** The tree file links should point at: the scanned commit when the report + * knows it (a permalink that stays correct after the branch moves), else the + * branch, else "" — meaning no link at all. */ +export function linkRef(meta: { branch?: string | null; git?: { sha?: string | null } | null }): string { + return meta.git?.sha || meta.branch || ""; +} + +/** Make a file path a clickable link into the repo at `ref` (a commit sha or a + * branch). Plain text when there is no http(s) repoUrl or no ref — a link with + * an empty ref would be `/blob//…`, which resolves nowhere. */ +export function fileLink(path: string, line: number | undefined, repoUrl: string | null, ref: string): string { const clean = path.split(":")[0]!; - if (!repoUrl || !/^https?:\/\//.test(repoUrl)) return e(path); + if (!repoUrl || !/^https?:\/\//.test(repoUrl) || !ref) return e(path); // Encode path segments for URL safety (spaces, #, ?, etc.) const encodedPath = clean.split("/").map(encodeURIComponent).join("/"); const safeRepoUrl = repoUrl.replace(/ /g, "%20"); - const href = `${safeRepoUrl}/blob/${encodeURIComponent(branch)}/${encodedPath}${line ? `#L${line}` : ""}`; + const href = `${safeRepoUrl}/blob/${encodeURIComponent(ref)}/${encodedPath}${line ? `#L${line}` : ""}`; return `${e(path)}`; } diff --git a/src/report/html.ts b/src/report/html.ts index e93e035..756c9bc 100644 --- a/src/report/html.ts +++ b/src/report/html.ts @@ -14,7 +14,7 @@ import { getCheckMeta } from "../check-meta.js"; import { computeDelta } from "../delta.js"; import type { CheckResult, VibeReport } from "../types.js"; -import { det, e, fileLink, gc } from "./components.js"; +import { det, e, fileLink, gc, linkRef } from "./components.js"; import { FAVICON_SVG } from "./favicon.js"; import { actionsPage, @@ -55,8 +55,8 @@ export function generatePages(report: VibeReport, historyDir?: string, prevRepor const checkMap = new Map(allChecks.map((c) => [c.name, c])); const active = allChecks.filter((c) => !det(c).skipped && !det(c).comingSoon); const ru = report.meta.repoUrl; - const br = report.meta.branch; - const fl = (path: string, line?: number) => fileLink(path, line, ru, br); + const ref = linkRef(report.meta); + const fl = (path: string, line?: number) => fileLink(path, line, ru, ref); const totalIssues = allChecks.reduce((s, c) => s + c.issues.length, 0); const proj = report.meta.cwd.split("/").pop() || "project"; diff --git a/src/report/pages.test.ts b/src/report/pages.test.ts index 2d3b32f..c9fa177 100644 --- a/src/report/pages.test.ts +++ b/src/report/pages.test.ts @@ -225,3 +225,36 @@ describe("report generation", () => { expect(index).toContain("Feature Map"); }); }); + +describe("report file links", () => { + const issueCheck = (): CheckResult => ({ + name: "lint", + score: 90, + grade: "A", + details: {}, + issues: [{ severity: "warning", message: "console.log found", file: "src/auth.ts", line: 2, rule: "no-console" }], + duration: 10, + }); + const allHtml = (pages: Map) => [...pages.values()].join("\n"); + + it("detached HEAD with no sha: no /blob// links, file names stay plain text", () => { + const report = makeReport(mkdtempSync(join(tmpdir(), "vcqa-report-")), [issueCheck()]); + report.meta.repoUrl = "https://github.com/octo-org/widgets"; + report.meta.branch = ""; + const html = allHtml(generatePages(report)); + expect(html).not.toContain("/blob//"); + expect(html).not.toContain("https://github.com/octo-org/widgets/blob/"); + expect(html).toContain("src/auth.ts"); + }); + + it("links to the scanned commit sha when the report carries one", () => { + const sha = "c".repeat(40); + const report = makeReport(mkdtempSync(join(tmpdir(), "vcqa-report-")), [issueCheck()]); + report.meta.repoUrl = "https://github.com/octo-org/widgets"; + report.meta.branch = ""; + Object.assign(report.meta, { git: { sha } }); + const html = allHtml(generatePages(report)); + expect(html).toContain(`https://github.com/octo-org/widgets/blob/${sha}/src/auth.ts#L2`); + expect(html).not.toContain("/blob//"); + }); +}); diff --git a/src/upload.test.ts b/src/upload.test.ts index fd97e3c..d8bb621 100644 --- a/src/upload.test.ts +++ b/src/upload.test.ts @@ -1,6 +1,12 @@ -import { describe, expect, it } from "vitest"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; import type { VibeReport } from "./types.js"; -import { buildReportUploadPayload, repoSlugFromReport } from "./upload.js"; +import { buildReportUploadPayload, currentGitSha, repoSlugFromReport } from "./upload.js"; + +const EVENTS = join(import.meta.dirname!, "..", "fixtures", "github-events"); function report(): VibeReport { return { @@ -84,3 +90,44 @@ describe("upload payload contract", () => { expect(buildReportUploadPayload(missingRepo)).toBeNull(); }); }); + +describe("upload sha", () => { + // Every case passes an explicit env, so the GITHUB_* of a CI run executing + // this suite cannot leak in. + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + const repoWithCommit = () => { + const dir = mkdtempSync(join(tmpdir(), "vcqa-upload-git-")); + dirs.push(dir); + const git = (...args: string[]) => + execFileSync("git", ["-c", "user.name=t", "-c", "user.email=t@example.com", "-c", "commit.gpgsign=false", ...args], { + cwd: dir, + encoding: "utf-8", + stdio: ["pipe", "pipe", "pipe"], + }).trim(); + git("init", "-q"); + writeFileSync(join(dir, "a.txt"), "a\n"); + git("add", "a.txt"); + git("commit", "-q", "-m", "init"); + return { dir, head: git("rev-parse", "HEAD") }; + }; + + it("uses the PR head sha on pull_request, not the merge sha", () => { + const { dir } = repoWithCommit(); + const env = { GITHUB_ACTIONS: "true", GITHUB_EVENT_PATH: join(EVENTS, "pull_request.json"), GITHUB_SHA: "1".repeat(40) }; + expect(currentGitSha(dir, env)).toBe("a".repeat(40)); + }); + + it("uses GITHUB_SHA on push", () => { + const { dir } = repoWithCommit(); + const env = { GITHUB_ACTIONS: "true", GITHUB_EVENT_PATH: join(EVENTS, "push.json"), GITHUB_SHA: "c".repeat(40) }; + expect(currentGitSha(dir, env)).toBe("c".repeat(40)); + }); + + it("falls back to the local HEAD outside CI", () => { + const { dir, head } = repoWithCommit(); + expect(currentGitSha(dir, {})).toBe(head); + }); +}); diff --git a/src/upload.ts b/src/upload.ts index e408364..f82218e 100644 --- a/src/upload.ts +++ b/src/upload.ts @@ -1,4 +1,5 @@ import { execSync } from "node:child_process"; +import { existsSync, readFileSync } from "node:fs"; import { withFreshAnalyzerSnapshots } from "./report-contract.js"; import type { VibeReport } from "./types.js"; @@ -18,9 +19,29 @@ export function buildReportUploadPayload(report: VibeReport, sha?: string): Repo return { repo, report: withFreshAnalyzerSnapshots(report), ...(sha ? { sha } : {}) }; } -export function currentGitSha(cwd: string): string | undefined { +/** The commit an upload is attributed to — where github-app posts the + * quality-gate status. On a pull_request run the checkout is GitHub's + * synthetic merge commit, which no PR displays, so the PR head sha from the + * event payload wins; then `GITHUB_SHA`; then the local `HEAD`. */ +export function currentGitSha(cwd: string, env: NodeJS.ProcessEnv = process.env): string | undefined { + return prHeadShaFromEvent(env) || env.GITHUB_SHA || localHeadSha(cwd); +} + +function prHeadShaFromEvent(env: NodeJS.ProcessEnv): string | undefined { + const eventPath = env.GITHUB_EVENT_PATH; + if (!eventPath || !existsSync(eventPath)) return undefined; + try { + const event = JSON.parse(readFileSync(eventPath, "utf-8")) as { pull_request?: { head?: { sha?: unknown } } }; + const sha = event.pull_request?.head?.sha; + return typeof sha === "string" && sha ? sha : undefined; + } catch { + return undefined; + } +} + +function localHeadSha(cwd: string): string | undefined { try { - return execSync("git rev-parse HEAD", { cwd, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim(); + return execSync("git rev-parse HEAD", { cwd, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim() || undefined; } catch { return undefined; }