Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions src/selfhost/pg-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
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),
}),
);
});
}
5 changes: 4 additions & 1 deletion src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -426,6 +426,9 @@ async function main(): Promise<void> {
// #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)
Expand Down
36 changes: 36 additions & 0 deletions test/unit/queue-5.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: "" });
Expand Down
77 changes: 77 additions & 0 deletions test/unit/selfhost-pg-adapter-github-id-widening.test.ts
Original file line number Diff line number Diff line change
@@ -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<unknown>): 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();
});
});
Loading