From fe187311075b0cbdac1a74f6f6b42cbd76550b65 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 04:08:05 -0700 Subject: [PATCH 1/2] test(commands): cover the intent-router ask/chat question-threading branch #5036's codecov/patch flagged one partial branch in processors.ts: matchedCommand === "ask" || matchedCommand === "chat" ? command.unrecognizedText : undefined. The 3 existing #4596 integration tests only ever reroute to "blockers", so the true side (rerouting to ask/chat specifically) was never exercised -- meaning the one behavior that actually matters here (does the contributor's original free text survive the reroute into the command's own question field, instead of silently dropping it) was untested. --- test/unit/queue-5.test.ts | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 3c6b13bc3e..2bbdd09be0 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -1343,6 +1343,42 @@ describe("queue processors", () => { expect(seen.comments[0]).not.toContain("Did you mean"); }); + it("#4596: re-routing to ask/chat threads the original free text through as the command's own question (not dropped)", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI_ADVISORY: { run: async () => ({ response: '{"command": "ask"}' }) } as unknown as Ai, + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 315, title: "Rate limit target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[] }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".gittensory.yml")) { + return new Response("settings:\n advisoryAiRouting:\n intentRouting: true\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/315/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/315/comments") && method === "POST") { + seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: seen.comments.length }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + await processJob(env, { + type: "github-webhook", + deliveryId: "intent-routing-ask-question-threaded", + eventName: "issue_comment", + payload: mentionPayload(315, "@gittensory what should I fix first?"), + }); + expect(seen.comments).toHaveLength(1); + expect(seen.comments[0]).toContain('Interpreted "what should I fix first?" as `@gittensory ask`'); + // ask's own card only prints this fallback when its question is empty/undefined -- its absence proves + // command.unrecognizedText actually reached `ask` as its question rather than being dropped by the + // reroute (unlike blockers/next-action/etc., ask and chat are the only two commands that take one). + expect(seen.comments[0]).not.toContain("No specific question was provided"); + }); + it("#4596: falls through to the existing did-you-mean hint end-to-end when intentRouting is off, the default", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 310, title: "Rate limit target", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); From 2d842950befc57ba86e1868402e90680a9fc8f6b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 04:46:10 -0700 Subject: [PATCH 2/2] fix(selfhost): widen GitHub-native id columns to bigint on Postgres Every migrations/*.sql column storing a raw GitHub-native numeric id (installation, account/user, check-run, comment) is declared bare INTEGER -- fine on SQLite/D1 (a type-affinity hint that already stores any 64-bit value), but a real 4-byte column on the self-host Postgres backend. GitHub's comment ids are already past 2^31, confirmed live on edge-nl-01 via a "value out of range for type integer" insert failure on github_agent_command_answers. widenGithubIdColumnsToBigint mirrors the existing tuneGithubRateLimitObservationsAutovacuum pattern: a Postgres-only, idempotent ALTER batch run unconditionally after migrations on every boot, never touching the original (SQLite/D1-correct) migration files. Closes #5059 --- src/selfhost/pg-adapter.ts | 55 +++++++++++++ src/server.ts | 5 +- ...host-pg-adapter-github-id-widening.test.ts | 77 +++++++++++++++++++ 3 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 test/unit/selfhost-pg-adapter-github-id-widening.test.ts diff --git a/src/selfhost/pg-adapter.ts b/src/selfhost/pg-adapter.ts index 446e0a92c5..e2770c8cef 100644 --- a/src/selfhost/pg-adapter.ts +++ b/src/selfhost/pg-adapter.ts @@ -120,3 +120,58 @@ export async function tuneGithubRateLimitObservationsAutovacuum(db: D1Database): ); }); } + +// #selfhost-github-id-overflow: every migrations/*.sql column below stores a raw GitHub-native numeric ID +// (installation id, account/user id, check-run id, or comment id) as bare `INTEGER` -- correct on SQLite/D1, +// where INTEGER is only a type-affinity hint and already stores any 64-bit value without truncation, but a +// real, enforced 4-byte column on Postgres. GitHub's own ids are a single global counter shared across all of +// GitHub (not scoped per-repo the way issue/PR *numbers* are), and comment ids in particular are already well +// past 2^31 (~2.1B) as of 2026 -- confirmed live via a `value "…" is out of range for type integer` failure on +// github_agent_command_answers.request_comment_id/response_comment_id. installation/account/user ids are +// nowhere near that threshold yet, but are widened here too rather than waiting for their own future incident +// -- bigint costs nothing extra at this table size. Same idempotent-ALTER, no-migration-ledger shape as +// GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL above: re-applying to an already-bigint column is a no-op, so +// this runs unconditionally on every Postgres boot. The original migrations are left untouched (already +// applied/ledger-tracked everywhere, and correct as written for SQLite/D1) -- this is a purely additive, +// Postgres-only follow-up, not a rewrite of history. New migrations introducing a GitHub-native id column +// going forward should declare it BIGINT directly instead of adding another line here. +export const GITHUB_ID_BIGINT_WIDENING_SQL = [ + "ALTER TABLE installations ALTER COLUMN id TYPE bigint", + "ALTER TABLE installations ALTER COLUMN account_id TYPE bigint", + "ALTER TABLE repositories ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE advisories ALTER COLUMN check_run_id TYPE bigint", + "ALTER TABLE webhook_events ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE installation_health ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE auth_sessions ALTER COLUMN github_user_id TYPE bigint", + "ALTER TABLE github_agent_command_answers ALTER COLUMN request_comment_id TYPE bigint", + "ALTER TABLE github_agent_command_answers ALTER COLUMN response_comment_id TYPE bigint", + "ALTER TABLE agent_pending_actions ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE review_targets ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE orb_webhook_events ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE orb_github_installations ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE orb_pr_outcomes ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE orb_enrollments ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE orb_enrollments ALTER COLUMN maintainer_github_id TYPE bigint", + "ALTER TABLE orb_relay_failures ALTER COLUMN installation_id TYPE bigint", + "ALTER TABLE orb_relay_pending ALTER COLUMN installation_id TYPE bigint", +].join(";\n"); + +/** Apply the bigint widening above via the same D1Database.exec() surface runSelfHostMigrations already uses, + * mirroring tuneGithubRateLimitObservationsAutovacuum's shape exactly. Must run AFTER migrations (every table + * above has to exist by then, so a mid-batch "relation does not exist" is not a realistic failure mode here); + * best-effort by design -- a failure here must not stop the self-host from booting. Postgres's simple-query + * protocol runs this whole multi-statement string as one implicit transaction, so either all 19 ALTERs commit + * together or (on any single failure) none do -- fine given every ALTER is independently idempotent and this + * reruns unconditionally on every boot: a failed attempt just retries whole next boot instead of leaving a + * partially-widened, inconsistent state. */ +export async function widenGithubIdColumnsToBigint(db: D1Database): Promise { + await db.exec(GITHUB_ID_BIGINT_WIDENING_SQL).catch((error: unknown) => { + console.error( + JSON.stringify({ + level: "warn", + event: "selfhost_github_id_bigint_widen_failed", + error: error instanceof Error ? error.message : String(error), + }), + ); + }); +} diff --git a/src/server.ts b/src/server.ts index 00a7a7deda..ea8d417324 100644 --- a/src/server.ts +++ b/src/server.ts @@ -58,7 +58,7 @@ import { clockSkewSecondsSample } from "./selfhost/clock-skew"; import { d1DatabaseSizeBytesSample, d1SignalSnapshotsRowsPerKeySample, d1TableRowCountSamples, isD1SizeProbeEnabled, runD1SizeProbe } from "./selfhost/d1-size-probe"; import { gauge, gaugeVector, incr, observe, renderMetrics, setSelfHostedMetricsMode } from "./selfhost/metrics"; import { runSelfHostMigrations } from "./selfhost/migrate"; -import { createPgAdapter, tuneGithubRateLimitObservationsAutovacuum } from "./selfhost/pg-adapter"; +import { createPgAdapter, tuneGithubRateLimitObservationsAutovacuum, widenGithubIdColumnsToBigint } from "./selfhost/pg-adapter"; import { createPgQueue } from "./selfhost/pg-queue"; import { createPgVectorize, initPgVectorize } from "./selfhost/pg-vectorize"; import { resolvePostgresPoolMax } from "./selfhost/queue-common"; @@ -426,6 +426,9 @@ async function main(): Promise { // #2543: Postgres-only, applied AFTER migrations (the table must already exist). No-op on SQLite, which has // no autovacuum concept at all -- gated on the same usePostgres check the backend was built from. if (usePostgres) await tuneGithubRateLimitObservationsAutovacuum(backend.db); + // #selfhost-github-id-overflow: Postgres-only, same reasoning -- SQLite's INTEGER already stores a raw + // GitHub id at full width, so this would be a meaningless no-op there even if run. + if (usePostgres) await widenGithubIdColumnsToBigint(backend.db); const ai = createSelfHostAi(process.env); if (ai) diff --git a/test/unit/selfhost-pg-adapter-github-id-widening.test.ts b/test/unit/selfhost-pg-adapter-github-id-widening.test.ts new file mode 100644 index 0000000000..9d400450b3 --- /dev/null +++ b/test/unit/selfhost-pg-adapter-github-id-widening.test.ts @@ -0,0 +1,77 @@ +// Unit tests for the GitHub-native-id bigint widening step (#selfhost-github-id-overflow). Uses a mock +// D1Database (just the .exec() surface runSelfHostMigrations already relies on) so no real Postgres is +// required -- the SQL itself is plain, already-Postgres-native syntax with no SQLite constructs for +// pg-dialect.ts to translate, so a mocked interaction test is a faithful, fast substitute for a live ALTER +// TABLE. Mirrors selfhost-pg-adapter-autovacuum.test.ts's shape exactly (#2543's sibling fix-up step). +import { describe, expect, it, vi } from "vitest"; +import { GITHUB_ID_BIGINT_WIDENING_SQL, widenGithubIdColumnsToBigint } from "../../src/selfhost/pg-adapter"; + +function mockDb(execImpl: (sql: string) => Promise): D1Database { + return { exec: vi.fn(execImpl) } as unknown as D1Database; +} + +describe("GITHUB_ID_BIGINT_WIDENING_SQL (#selfhost-github-id-overflow)", () => { + it("widens every known GitHub-native-id column to bigint, one ALTER per statement", () => { + const statements = GITHUB_ID_BIGINT_WIDENING_SQL.split(";").map((s) => s.trim()).filter(Boolean); + expect(statements.length).toBeGreaterThanOrEqual(18); + for (const statement of statements) { + expect(statement.toUpperCase()).toMatch(/^ALTER TABLE \w+ ALTER COLUMN \w+ TYPE bigint$/i); + } + }); + + it("covers the column that was seen actively overflowing in production (request/response comment ids)", () => { + expect(GITHUB_ID_BIGINT_WIDENING_SQL).toContain("ALTER TABLE github_agent_command_answers ALTER COLUMN request_comment_id TYPE bigint"); + expect(GITHUB_ID_BIGINT_WIDENING_SQL).toContain("ALTER TABLE github_agent_command_answers ALTER COLUMN response_comment_id TYPE bigint"); + }); + + it("is additive-only DDL, never destructive", () => { + expect(GITHUB_ID_BIGINT_WIDENING_SQL).not.toMatch(/DROP|DELETE|TRUNCATE/i); + }); +}); + +describe("widenGithubIdColumnsToBigint (#selfhost-github-id-overflow)", () => { + it("applies the widening SQL via db.exec() as a single batch", async () => { + const db = mockDb(async () => ({ count: 1, duration: 0 })); + + await widenGithubIdColumnsToBigint(db); + + expect(db.exec).toHaveBeenCalledWith(GITHUB_ID_BIGINT_WIDENING_SQL); + expect(db.exec).toHaveBeenCalledTimes(1); + }); + + it("fails open (does not throw) when db.exec rejects -- an idempotent follow-up, never a boot-blocking dependency", async () => { + const db = mockDb(async () => { + throw new Error("connection reset"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await expect(widenGithubIdColumnsToBigint(db)).resolves.toBeUndefined(); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("selfhost_github_id_bigint_widen_failed")); + errorSpy.mockRestore(); + }); + + it("logs the underlying error message on failure", async () => { + const db = mockDb(async () => { + throw new Error("relation does not exist"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await widenGithubIdColumnsToBigint(db); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("relation does not exist")); + errorSpy.mockRestore(); + }); + + it("stringifies a non-Error rejection instead of throwing on error.message access", async () => { + const db = mockDb(async () => { + throw "a plain string rejection"; + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await expect(widenGithubIdColumnsToBigint(db)).resolves.toBeUndefined(); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("a plain string rejection")); + errorSpy.mockRestore(); + }); +});