From 99803a0f9dd146955ff6d52d53075ae99d5e4c94 Mon Sep 17 00:00:00 2001 From: 3alpha <15694175+3alpha@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:57:33 +0200 Subject: [PATCH] feat(github-actions): add versioned build-result marker to PR build comment Append a stable marker and a single-line structured marker to the build bot comment, with bare-CID normalization for /ipfs/ and ipfs:// prefixes. The legacy (by dappnodebot/build-action) signature is preserved so isTargetComment recognizes existing comments and updates them in place. GITHUB_RUN_ID and GITHUB_RUN_ATTEMPT are forwarded into the structured payload. commentToPr now awaits the create path so the SDK process does not exit before the comment is posted. --- .../githubActions/build/botComment.ts | 176 ++++++++++- src/commands/githubActions/build/index.ts | 7 +- src/providers/github/Github.ts | 2 +- .../commands/githubActions/botComment.test.ts | 287 ++++++++++++++++++ test/providers/github/Github.test.ts | 71 +++++ 5 files changed, 526 insertions(+), 17 deletions(-) create mode 100644 test/commands/githubActions/botComment.test.ts create mode 100644 test/providers/github/Github.test.ts diff --git a/src/commands/githubActions/build/botComment.ts b/src/commands/githubActions/build/botComment.ts index 876290fe..b8adf2a8 100644 --- a/src/commands/githubActions/build/botComment.ts +++ b/src/commands/githubActions/build/botComment.ts @@ -1,23 +1,74 @@ import { ListrContextBuild } from "../../../types.js"; import { getInstallDnpLink } from "../../../utils/getLinks.js"; -const botCommentTag = "(by dappnodebot/build-action)"; +/** + * Persistent tag used to identify the build bot comment in a PR. + * Commented out from the readable body so it does not affect the user experience. + */ +export const stableCommentMarker = ""; + +/** + * Prefix of the structured result marker. The remainder of the line is a single + * JSON payload terminated by `-->`. + */ +export const resultMarkerPrefix = ""; + +/** + * Legacy human-readable signature kept during the migration period so that + * existing comments are still recognized as the build bot comment and updated + * in place rather than duplicated. + */ +const legacyBotCommentTag = "(by dappnodebot/build-action)"; + +/** + * Schema of the JSON payload embedded in the result marker. + * Keep stable: any breaking change requires bumping `:v1` to `:vN`. + */ +export interface BuildResultMarkerV1 { + commit_sha: string; + run_id?: string; + run_attempt?: number; + packages: Array<{ + dnp_name: string; + cid: string; + }>; +} + +export interface GetBuildBotCommentParams { + commitSha: string; + buildResults: ListrContextBuild; + /** + * GitHub Actions run id. When undefined or empty the field is omitted. + */ + runId?: string; + /** + * GitHub Actions run attempt. When not a positive integer the field is omitted. + */ + runAttempt?: string | number; +} /** * Constructs a comment summarizing build results for a specific commit. - * This comment includes a tag to identify it, allowing easy retrieval or replacement. + * The comment is human-readable and is also tagged with: + * - a stable `` marker, and + * - a machine-readable `` marker + * carrying a bare-CID payload for downstream consumers. * - * @param {string} commitSha - The Git commit SHA associated with the build. - * @param {ListrContextBuild} buildResults - The results of the build process. - * @return {string} A formatted comment with links to install the built packages and their hashes. + * The legacy `(by dappnodebot/build-action)` signature is preserved during the + * migration period so that comments authored before this change are still + * recognized as the build bot comment and updated in place. */ export function getBuildBotComment({ commitSha, - buildResults -}: { - commitSha: string; - buildResults: ListrContextBuild; -}): string { + buildResults, + runId, + runAttempt +}: GetBuildBotCommentParams): string { const buildEntries = Object.entries(buildResults) .map(([dnpName, { releaseMultiHash }], index) => { if (releaseMultiHash) @@ -25,13 +76,24 @@ export function getBuildBotComment({ }) .join("\n\n"); + const resultMarker = renderResultMarker({ + commitSha, + buildResults, + runId, + runAttempt + }); + + // Trailing newline is added so the result marker (when emitted) sits on its + // own line, with the stable comment marker as the very last line. return `Dappnode bot has built and pinned the built packages to an IPFS node, for commit: ${commitSha} This is a development version and should **only** be installed for testing purposes. ${buildEntries} -${botCommentTag} +${legacyBotCommentTag} +${resultMarker} +${stableCommentMarker} `; } @@ -46,15 +108,99 @@ function formatBuildEntry({ }) { const installLink = getInstallDnpLink(releaseMultiHash); return `${index + 1}. Package **${dnpName}** - + [Install link](${installLink}) - + Hash: \`${releaseMultiHash}\``; } /** - * Locates any existing comment by a persistent tag used in all build bot comments + * Build the JSON payload embedded in the result marker, normalized to bare CIDs. + * Returns an empty string when there is no package with a non-empty hash, so + * the caller can decide whether to emit the marker at all. + */ +export function buildResultMarkerJson({ + commitSha, + buildResults, + runId, + runAttempt +}: GetBuildBotCommentParams): string { + const packages: BuildResultMarkerV1["packages"] = []; + for (const [dnpName, { releaseMultiHash }] of Object.entries(buildResults)) { + if (!releaseMultiHash) continue; + const cid = normalizeToBareCid(releaseMultiHash); + if (!cid) continue; + packages.push({ dnp_name: dnpName, cid }); + } + + if (packages.length === 0) return ""; + + const payload: BuildResultMarkerV1 = { commit_sha: commitSha, packages }; + + if (typeof runId === "string" && runId.trim() !== "") + payload.run_id = runId; + + const attempt = parseRunAttempt(runAttempt); + if (attempt !== undefined) payload.run_attempt = attempt; + + return JSON.stringify(payload); +} + +/** + * Render the full result marker line, or an empty string when no package has a + * usable hash. The stable comment marker is always emitted separately so that + * the comment is recognized as the build bot comment even when the result + * payload is empty. + */ +export function renderResultMarker( + params: GetBuildBotCommentParams +): string { + const json = buildResultMarkerJson(params); + if (!json) return ""; + // Defensive: collapse the JSON into a single line. + const oneline = json.replace(/\s+/g, " "); + return `${resultMarkerPrefix}${oneline}${markerSuffix}`; +} + +/** + * Strip the `ipfs://` and `/ipfs/` URL prefixes from a release hash so the + * structured payload always carries a bare CID. Unknown shapes are returned + * as-is when the value is a non-empty string. + */ +export function normalizeToBareCid(value: string | undefined | null): string { + if (typeof value !== "string") return ""; + const trimmed = value.trim(); + if (!trimmed) return ""; + if (trimmed.startsWith("ipfs://")) return trimmed.slice("ipfs://".length); + if (trimmed.startsWith("/ipfs/")) return trimmed.slice("/ipfs/".length); + return trimmed; +} + +/** + * Parse `GITHUB_RUN_ATTEMPT` (or any string) into a positive integer run + * attempt. Returns undefined when the value is not a positive integer. + */ +export function parseRunAttempt( + value: string | number | undefined | null +): number | undefined { + if (value === undefined || value === null) return undefined; + const n = typeof value === "number" ? value : Number(value); + if (!Number.isFinite(n)) return undefined; + if (!Number.isInteger(n)) return undefined; + if (n <= 0) return undefined; + return n; +} + +/** + * Locates any existing comment by a persistent tag used in all build bot + * comments. Returns true for both the new stable marker and the legacy + * signature so that existing comments are updated in place instead of being + * duplicated. */ export function isTargetComment(commentBody: string): boolean { - return commentBody.includes(botCommentTag); + if (!commentBody) return false; + return ( + commentBody.includes(stableCommentMarker) || + commentBody.includes(legacyBotCommentTag) + ); } diff --git a/src/commands/githubActions/build/index.ts b/src/commands/githubActions/build/index.ts index 78def92d..0b34fae4 100644 --- a/src/commands/githubActions/build/index.ts +++ b/src/commands/githubActions/build/index.ts @@ -128,7 +128,12 @@ export async function buildAndComment({ variants }); - const body = getBuildBotComment({ commitSha, buildResults }); + const body = getBuildBotComment({ + commitSha, + buildResults, + runId: process.env.GITHUB_RUN_ID, + runAttempt: process.env.GITHUB_RUN_ATTEMPT + }); console.log(`Build bot comment: \n\n${body}`); diff --git a/src/providers/github/Github.ts b/src/providers/github/Github.ts index f09d850f..a6cbd1ee 100644 --- a/src/providers/github/Github.ts +++ b/src/providers/github/Github.ts @@ -363,7 +363,7 @@ export class Github { } }); } else { - this.createCommentInPr({ number, body }); + await this.createCommentInPr({ number, body }); } } diff --git a/test/commands/githubActions/botComment.test.ts b/test/commands/githubActions/botComment.test.ts new file mode 100644 index 00000000..c93ed8c2 --- /dev/null +++ b/test/commands/githubActions/botComment.test.ts @@ -0,0 +1,287 @@ +import { expect } from "chai"; +import { ListrContextBuild } from "../../../src/types.js"; +import { + buildResultMarkerJson, + getBuildBotComment, + isTargetComment, + normalizeToBareCid, + parseRunAttempt, + renderResultMarker, + resultMarkerPrefix, + stableCommentMarker +} from "../../../src/commands/githubActions/build/botComment.js"; + +const commitSha = "a7cfeb18800dd33f25bafc4b397e88d09356b034"; +const cid = "QmP7HGvTh2jEccAymRiSvsdZDg7K8a5HGf1rB7ciMEWfL2"; + +/** + * Extract and parse the JSON payload from the result marker. + * Returns the parsed object and a flag indicating whether a marker was found. + */ +function parseResultMarker(body: string): { + found: boolean; + json: string; + data: { + commit_sha: string; + run_id?: string; + run_attempt?: number; + packages: Array<{ dnp_name: string; cid: string }>; + } | null; +} { + const start = body.indexOf(resultMarkerPrefix); + if (start < 0) return { found: false, json: "", data: null }; + const jsonStart = start + resultMarkerPrefix.length; + const end = body.indexOf("-->", jsonStart); + if (end < 0) return { found: false, json: "", data: null }; + const json = body.slice(jsonStart, end).trim(); + return { found: true, json, data: JSON.parse(json) }; +} + +describe("botComment", () => { + describe("normalizeToBareCid", () => { + it("returns the input unchanged for a bare Qm... CID", () => { + expect(normalizeToBareCid(cid)).to.equal(cid); + }); + + it("strips the /ipfs/ prefix", () => { + expect(normalizeToBareCid(`/ipfs/${cid}`)).to.equal(cid); + }); + + it("strips the ipfs:// prefix", () => { + expect(normalizeToBareCid(`ipfs://${cid}`)).to.equal(cid); + }); + + it("returns an empty string for empty, null, or undefined", () => { + expect(normalizeToBareCid("")).to.equal(""); + expect(normalizeToBareCid(" ")).to.equal(""); + expect(normalizeToBareCid(undefined)).to.equal(""); + expect(normalizeToBareCid(null)).to.equal(""); + }); + }); + + describe("parseRunAttempt", () => { + it("parses a valid positive integer string", () => { + expect(parseRunAttempt("1")).to.equal(1); + expect(parseRunAttempt("7")).to.equal(7); + }); + + it("parses a positive integer number", () => { + expect(parseRunAttempt(3)).to.equal(3); + }); + + it("rejects zero, negative, and non-integer values", () => { + expect(parseRunAttempt("0")).to.equal(undefined); + expect(parseRunAttempt("-1")).to.equal(undefined); + expect(parseRunAttempt("1.5")).to.equal(undefined); + }); + + it("rejects empty or non-numeric strings", () => { + expect(parseRunAttempt("")).to.equal(undefined); + expect(parseRunAttempt("abc")).to.equal(undefined); + }); + + it("rejects null and undefined", () => { + expect(parseRunAttempt(undefined)).to.equal(undefined); + expect(parseRunAttempt(null)).to.equal(undefined); + }); + }); + + describe("getBuildBotComment - single package", () => { + const buildResults: ListrContextBuild = { + "dms.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp", releaseMultiHash: cid } + }; + + it("emits a single-package result marker with bare CID and stable marker", () => { + const body = getBuildBotComment({ + commitSha, + buildResults + }); + + expect(body).to.include(stableCommentMarker); + const { found, data } = parseResultMarker(body); + expect(found).to.equal(true); + expect(data).to.deep.equal({ + commit_sha: commitSha, + packages: [{ dnp_name: "dms.dnp.dappnode.eth", cid }] + }); + }); + + it("includes the install link and the human-readable hash", () => { + const body = getBuildBotComment({ commitSha, buildResults }); + expect(body).to.include(`[Install link](http://my.dappnode/installer/public/${encodeURIComponent(cid)})`); + expect(body).to.include(`Hash: \`${cid}\``); + }); + + it("keeps the legacy (by dappnodebot/build-action) tag", () => { + const body = getBuildBotComment({ commitSha, buildResults }); + expect(body).to.include("(by dappnodebot/build-action)"); + }); + }); + + describe("getBuildBotComment - multiple packages", () => { + const buildResults: ListrContextBuild = { + "a.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp/a", releaseMultiHash: "QmAAAA" }, + "b.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp/b", releaseMultiHash: "/ipfs/QmBBBB" }, + "c.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp/c", releaseMultiHash: "ipfs://QmCCCC" } + }; + + it("renders all packages with bare CIDs in the structured marker", () => { + const body = getBuildBotComment({ commitSha, buildResults }); + const { data } = parseResultMarker(body); + expect(data?.packages).to.deep.equal([ + { dnp_name: "a.dnp.dappnode.eth", cid: "QmAAAA" }, + { dnp_name: "b.dnp.dappnode.eth", cid: "QmBBBB" }, + { dnp_name: "c.dnp.dappnode.eth", cid: "QmCCCC" } + ]); + }); + + it("preserves the original /ipfs/ and ipfs:// hashes in the human-readable block", () => { + const body = getBuildBotComment({ commitSha, buildResults }); + expect(body).to.include("Hash: `/ipfs/QmBBBB`"); + expect(body).to.include("Hash: `ipfs://QmCCCC`"); + }); + }); + + describe("getBuildBotComment - GITHUB_RUN_ID", () => { + const buildResults: ListrContextBuild = { + "a.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp", releaseMultiHash: cid } + }; + + it("omits run_id when not provided", () => { + const body = getBuildBotComment({ commitSha, buildResults }); + const { data } = parseResultMarker(body); + expect(data).to.not.have.property("run_id"); + }); + + it("includes run_id when provided", () => { + const body = getBuildBotComment({ + commitSha, + buildResults, + runId: "123456" + }); + const { data } = parseResultMarker(body); + expect(data?.run_id).to.equal("123456"); + }); + + it("omits run_id when it is an empty string", () => { + const body = getBuildBotComment({ + commitSha, + buildResults, + runId: "" + }); + const { data } = parseResultMarker(body); + expect(data).to.not.have.property("run_id"); + }); + }); + + describe("getBuildBotComment - GITHUB_RUN_ATTEMPT", () => { + const buildResults: ListrContextBuild = { + "a.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp", releaseMultiHash: cid } + }; + + it("includes a valid positive integer run_attempt", () => { + const body = getBuildBotComment({ + commitSha, + buildResults, + runAttempt: "1" + }); + const { data } = parseResultMarker(body); + expect(data?.run_attempt).to.equal(1); + }); + + it("omits an invalid run_attempt", () => { + const body = getBuildBotComment({ + commitSha, + buildResults, + runAttempt: "abc" + }); + const { data } = parseResultMarker(body); + expect(data).to.not.have.property("run_attempt"); + }); + + it("omits a zero or negative run_attempt", () => { + const a = getBuildBotComment({ commitSha, buildResults, runAttempt: "0" }); + const b = getBuildBotComment({ commitSha, buildResults, runAttempt: "-1" }); + expect(parseResultMarker(a).data).to.not.have.property("run_attempt"); + expect(parseResultMarker(b).data).to.not.have.property("run_attempt"); + }); + }); + + describe("getBuildBotComment - empty result marker", () => { + it("emits the stable marker but no result marker when no package has a hash", () => { + const buildResults: ListrContextBuild = { + "a.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp/a" }, + "b.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp/b", releaseMultiHash: "" } + }; + const body = getBuildBotComment({ commitSha, buildResults }); + expect(body).to.include(stableCommentMarker); + const { found } = parseResultMarker(body); + expect(found).to.equal(false); + }); + }); + + describe("isTargetComment", () => { + it("accepts the new stable marker", () => { + expect(isTargetComment(`hello\n${stableCommentMarker}`)).to.equal(true); + }); + + it("accepts the legacy (by dappnodebot/build-action) marker", () => { + expect(isTargetComment("hello\n(by dappnodebot/build-action)")).to.equal(true); + }); + + it("rejects unrelated comments", () => { + expect(isTargetComment("just a regular comment")).to.equal(false); + expect(isTargetComment("")).to.equal(false); + }); + }); + + describe("renderResultMarker / buildResultMarkerJson", () => { + const buildResults: ListrContextBuild = { + "a.dnp.dappnode.eth": { variant: null, releaseDir: "/tmp", releaseMultiHash: cid } + }; + + it("emits a single-line JSON payload", () => { + const json = buildResultMarkerJson({ commitSha, buildResults }); + expect(json).to.not.include("\n"); + const parsed = JSON.parse(json); + expect(parsed.commit_sha).to.equal(commitSha); + expect(parsed.packages).to.deep.equal([{ dnp_name: "a.dnp.dappnode.eth", cid }]); + }); + + it("renderResultMarker wraps the JSON in the result marker prefix and suffix", () => { + const marker = renderResultMarker({ commitSha, buildResults }); + expect(marker.startsWith(resultMarkerPrefix)).to.equal(true); + expect(marker.endsWith("-->")).to.equal(true); + }); + + it("renderResultMarker returns empty string when no package has a hash", () => { + const empty: ListrContextBuild = { + a: { variant: null, releaseDir: "/tmp" } + }; + expect(renderResultMarker({ commitSha, buildResults: empty })).to.equal(""); + }); + }); +}); + +describe("botComment - GITHUB_RUN_ID env", () => { + const original = process.env.GITHUB_RUN_ID; + + afterEach(() => { + if (original === undefined) delete process.env.GITHUB_RUN_ID; + else process.env.GITHUB_RUN_ID = original; + }); + + it("reads runId from GITHUB_RUN_ID when runId is not explicitly provided", () => { + process.env.GITHUB_RUN_ID = "987654"; + const buildResults: ListrContextBuild = { + a: { variant: null, releaseDir: "/tmp", releaseMultiHash: cid } + }; + // We pass runAttempt as undefined to make sure the call works without + // explicit params. runId is intentionally not provided. + const json = buildResultMarkerJson({ commitSha, buildResults }); + // Without explicit runId, the build helper omits run_id; this documents + // that the helper itself does not read process.env. The wiring lives in + // the gaBuild handler which forwards the env var explicitly. + expect(JSON.parse(json)).to.not.have.property("run_id"); + }); +}); diff --git a/test/providers/github/Github.test.ts b/test/providers/github/Github.test.ts new file mode 100644 index 00000000..bb0b5e18 --- /dev/null +++ b/test/providers/github/Github.test.ts @@ -0,0 +1,71 @@ +import { expect } from "chai"; +import { Github } from "../../../src/providers/github/Github.js"; + +/** + * These tests exercise the `commentToPr` create-path of `Github` without a + * broad refactor. We construct a real `Github` instance (which needs + * `GITHUB_TOKEN`) and then replace its private `octokit` with a minimal stub + * that records the calls and resolves immediately. The goal is to assert that + * the create path is awaited, not to verify the entire Octokit surface. + */ +describe("Github.commentToPr - create path is awaited", () => { + const originalToken = process.env.GITHUB_TOKEN; + let github: Github; + let createCommentCalls: number; + let createCommentDelayMs: number; + let listCommentsCalls: number; + + beforeEach(() => { + process.env.GITHUB_TOKEN = "test-token"; + createCommentCalls = 0; + createCommentDelayMs = 25; + listCommentsCalls = 0; + + github = new Github({ owner: "acme", repo: "widget" }); + + const stub = { + rest: { + issues: { + listComments: async () => { + listCommentsCalls += 1; + return { data: [] }; + }, + createComment: async (args: unknown) => { + createCommentCalls += 1; + // Simulate a slow request. If `commentToPr` did NOT await the + // call, the assertion below would observe a dangling, un-awaited + // promise and the test would flake. + await new Promise(resolve => setTimeout(resolve, createCommentDelayMs)); + return { data: { id: 1, body: (args as { body: string }).body } }; + }, + updateComment: async () => ({ data: { id: 1 } }) + } + } + }; + + // Inject the stub into the private field. + (github as unknown as { octokit: unknown }).octokit = stub; + }); + + afterEach(() => { + if (originalToken === undefined) delete process.env.GITHUB_TOKEN; + else process.env.GITHUB_TOKEN = originalToken; + }); + + it("awaits createCommentInPr on the create path", async () => { + const body = "hello "; + const start = Date.now(); + await github.commentToPr({ + number: 7, + body, + isTargetComment: () => false + }); + const elapsed = Date.now() - start; + + expect(listCommentsCalls).to.equal(1); + expect(createCommentCalls).to.equal(1); + // If the call is not awaited, the elapsed time will be far below the + // simulated request delay. + expect(elapsed).to.be.gte(createCommentDelayMs); + }); +});