From a47e3ca1f6b49a085ec9c3be97184fb9cdf78d37 Mon Sep 17 00:00:00 2001 From: zk Date: Tue, 19 May 2026 23:06:22 +0100 Subject: [PATCH] feat(github-bot): use bliss logging and upsert PR comments (#1508) Switch ghbot from console.* to structured bliss/pino logs and edit existing bot comments on PR sync instead of posting a new one each push. --- bun.lock | 1 + typescript/github-bot/package.json | 1 + typescript/github-bot/src/main.ts | 151 +++++++++++++++++++++++-- typescript/github-bot/src/utils/log.ts | 2 + 4 files changed, 144 insertions(+), 11 deletions(-) create mode 100644 typescript/github-bot/src/utils/log.ts diff --git a/bun.lock b/bun.lock index db878516e..d5a9fdf3d 100644 --- a/bun.lock +++ b/bun.lock @@ -258,6 +258,7 @@ "dependencies": { "@octokit/app": "catalog:", "@octokit/webhooks-types": "catalog:", + "bliss": "workspace:*", "dotenv": "catalog:", "express": "catalog:", "prudence": "catalog:", diff --git a/typescript/github-bot/package.json b/typescript/github-bot/package.json index 6ff83afdc..fa0e467e3 100644 --- a/typescript/github-bot/package.json +++ b/typescript/github-bot/package.json @@ -14,6 +14,7 @@ "dependencies": { "@octokit/app": "catalog:", "@octokit/webhooks-types": "catalog:", + "bliss": "workspace:*", "dotenv": "catalog:", "express": "catalog:", "prudence": "catalog:" diff --git a/typescript/github-bot/src/main.ts b/typescript/github-bot/src/main.ts index fdf860a2c..e03708b2c 100644 --- a/typescript/github-bot/src/main.ts +++ b/typescript/github-bot/src/main.ts @@ -5,6 +5,7 @@ import express from "express"; import { URLSearchParams } from "url"; import { ProcessEnv } from "./config"; +import { log } from "#utils/log"; const app = new App({ appId: ProcessEnv.appId, @@ -18,6 +19,26 @@ const app = new App({ }, }); +const COMMENT_MARKERS = { + questProposal: "", + seedDiff: "", +} as const; + +type CommentMarker = (typeof COMMENT_MARKERS)[keyof typeof COMMENT_MARKERS]; + +type OctokitRequester = { + request: (route: string, params: object) => Promise<{ data: unknown }>; +}; + +type IssueComment = { + body: string; + id: number; +}; + +function withCommentMarker(message: string, marker: CommentMarker): string { + return `${message.trimEnd()}\n${marker}`; +} + /** * Create a response that contains a link to the seeds diff viewer on seeds.tachi.ac. */ @@ -98,7 +119,70 @@ async function listAllPullFiles( return out; } -async function sendMsg(message: string, octokit: any, repo: Repository, issue: number) { +async function findBotComment( + octokit: OctokitRequester, + owner: string, + repoName: string, + issueNumber: number, + marker: CommentMarker, +): Promise { + let page = 1; + while (true) { + // eslint-disable-next-line no-await-in-loop -- pagination must be sequential + const { data } = await octokit.request( + "GET /repos/{owner}/{repo}/issues/{issue_number}/comments", + { + owner, + repo: repoName, + issue_number: issueNumber, + per_page: 100, + page, + }, + ); + const batch = data as IssueComment[]; + const existing = batch.find((comment) => comment.body.includes(marker)); + if (existing) { + return existing; + } + if (batch.length < 100) { + break; + } + page += 1; + } + return undefined; +} + +async function upsertBotComment( + message: string, + marker: CommentMarker, + octokit: OctokitRequester, + repo: Repository, + issueNumber: number, +): Promise<"created" | "updated"> { + const owner = repo.owner.login; + const body = withCommentMarker(message, marker); + const existing = await findBotComment(octokit, owner, repo.name, issueNumber, marker); + + if (existing) { + await octokit.request("PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}", { + owner, + repo: repo.name, + comment_id: existing.id, + body, + }); + return "updated"; + } + + await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", { + owner, + repo: repo.name, + issue_number: issueNumber, + body, + }); + return "created"; +} + +async function sendMsg(message: string, octokit: OctokitRequester, repo: Repository, issue: number) { await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", { owner: repo.owner.login, repo: repo.name, @@ -114,15 +198,27 @@ app.webhooks.on( const pr = payload.pull_request; const isQuestProposal = pr.head.ref.startsWith("quest-proposal/"); + log.info( + { + action: payload.action, + prNumber: pr.number, + branch: pr.head.ref, + isQuestProposal, + }, + "Handling pull_request webhook.", + ); + // Quest-proposal PRs get a dedicated preview comment; all other PRs that // touch db/seeds/ get the standard seed-diff link. if (isQuestProposal) { - await sendMsg( + const commentAction = await upsertBotComment( mkQuestProposalMsg(pr.base.sha, pr.head.sha, pr.number), + COMMENT_MARKERS.questProposal, octokit, repo, pr.number, ); + log.info({ prNumber: pr.number, commentAction }, "Upserted quest-proposal comment."); return; } @@ -135,15 +231,25 @@ app.webhooks.on( ); if (prTouchesSeeds(filesChanged)) { - await sendMsg( + const commentAction = await upsertBotComment( mkSeedDiffViewMsg(pr.base.sha, pr.head.sha, pr.number), + COMMENT_MARKERS.seedDiff, octokit, repo, pr.number, ); + log.info({ prNumber: pr.number, commentAction }, "Upserted seed-diff comment."); + return; } + + log.debug({ prNumber: pr.number }, "PR does not touch db/seeds; no comment posted."); } catch (err) { - await sendMsg( + log.error( + { err, prNumber: pr.number }, + "Failed to determine whether PR touches db/seeds.", + ); + + const commentAction = await upsertBotComment( `I failed horribly figuring out whether this was a seeds diff or not. I'm sorry! ***** @@ -156,10 +262,12 @@ ${err} ***** ${mkSeedDiffViewMsg(pr.base.sha, pr.head.sha, pr.number)}`, + COMMENT_MARKERS.seedDiff, octokit, repo, pr.number, ); + log.info({ prNumber: pr.number, commentAction }, "Upserted seed-diff error comment."); } }, ); @@ -167,14 +275,29 @@ ${mkSeedDiffViewMsg(pr.base.sha, pr.head.sha, pr.number)}`, app.webhooks.on(["pull_request.closed"], async ({ payload }) => { const pr = payload.pull_request; + log.info( + { + action: payload.action, + prNumber: pr.number, + branch: pr.head.ref, + merged: pr.merged, + }, + "Handling pull_request.closed webhook.", + ); + // Only fire for merged PRs whose branch starts with quest-proposal/ if (!pr.merged || !pr.head.ref.startsWith("quest-proposal/")) { + log.debug( + { prNumber: pr.number, merged: pr.merged, branch: pr.head.ref }, + "Skipping quest-proposal merge notification.", + ); return; } if (!ProcessEnv.tachiApiOrigin || !ProcessEnv.tachiWebhookSecret) { - console.warn( - `Merged quest-proposal PR #${pr.number} but TACHI_API_ORIGIN or TACHI_WEBHOOK_SECRET are not set. Skipping server notification.`, + log.warn( + { prNumber: pr.number }, + "Merged quest-proposal PR but TACHI_API_ORIGIN or TACHI_WEBHOOK_SECRET are not set; skipping server notification.", ); return; } @@ -190,14 +313,15 @@ app.webhooks.on(["pull_request.closed"], async ({ payload }) => { }); if (!res.ok) { - console.error( - `Failed to notify Tachi server of merged PR #${pr.number}: ${res.status} ${res.statusText}`, + log.error( + { prNumber: pr.number, status: res.status, statusText: res.statusText }, + "Failed to notify Tachi server of merged quest-proposal PR.", ); } else { - console.log(`Notified Tachi server: quest-proposal PR #${pr.number} merged.`); + log.info({ prNumber: pr.number }, "Notified Tachi server: quest-proposal PR merged."); } } catch (err) { - console.error(`Error notifying Tachi server of merged PR #${pr.number}:`, err); + log.error({ err, prNumber: pr.number }, "Error notifying Tachi server of merged quest-proposal PR."); } }); @@ -210,6 +334,11 @@ app.webhooks.on(["issue_comment.created"], async ({ octokit, payload }) => { .split(/\s+/u)[1] ?.replace(/[^a-z]/u, ""); + log.info( + { prNumber: payload.issue.number, cmd: cmd ?? "(none)" }, + "Handling issue_comment +bot command.", + ); + switch (cmd) { case "ping": { await sendMsg("pong!", octokit, payload.repository as any, payload.issue.number); @@ -253,5 +382,5 @@ expressApp.use(serverMiddleware); expressApp.get("/.deploy/up", (_req, res) => res.sendStatus(200)); -console.log(`Listening on port ${ProcessEnv.port}.`); +log.info({ port: ProcessEnv.port }, "Listening for GitHub webhooks."); expressApp.listen(ProcessEnv.port); diff --git a/typescript/github-bot/src/utils/log.ts b/typescript/github-bot/src/utils/log.ts new file mode 100644 index 000000000..42314777c --- /dev/null +++ b/typescript/github-bot/src/utils/log.ts @@ -0,0 +1,2 @@ +export { AppendLogCtx, ChangeRootLogLevel, GetLogLevel, log } from "bliss/log"; +export type { Logger } from "bliss/log";