diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index bae248df..d19cd647 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -223,20 +223,21 @@ jobs: fi env: FLY_API_TOKEN: ${{ secrets.FLY_API_TOKEN }} - - name: Activate release job kinds after fleet replacement + - name: Activate release capabilities after fleet replacement id: activate # Migration 0020 stages new job kinds with an infinite # run_after. This runs only after flyctl deploy has replaced the managed # fleet, so no pre-capability worker can claim them. It retires the # quiesced escalation-email state, backfills billing contacts, then - # atomically releases every staged supported job. A + # activates private-review author enforcement, then atomically releases + # every staged supported job. A # failure leaves jobs safely staged and fails the workflow for a retry. timeout-minutes: 5 run: | set -euo pipefail machines=$(flyctl machine list --app postil-web --json) if ! fleet_summary=$(jq -ce -f scripts/verify-managed-fleet.jq <<<"${machines}"); then - echo "::error::Refusing to activate release job kinds: fleet image, process, or hosted-inference configuration is inconsistent." + echo "::error::Refusing to activate release capabilities: fleet image, process, or hosted-inference configuration is inconsistent." exit 1 fi echo "managed fleet verified: ${fleet_summary}" diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 20b3dc46..ba3a364d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -149,8 +149,11 @@ complete post-trial entitlement state idempotently through `scripts/set-org-entitlement.ts`; the billing page reports the stored state and lets organization administrators set the hosted overage hard cap. BYOK billing copy directs administrators to provider-side budgets because Postil cannot -enforce external charges. The page does not represent a payment checkout. Review rows snapshot the pull request -author GitHub ID and login supplied by the reviewable pull-request webhook. +enforce external charges. The page does not represent a payment checkout. Before +a private review row can run, the worker resolves the pull request's author ID, +login, head, and base from GitHub. A rollout-activated database trigger rejects +anonymous active reviews and makes the recorded author identity immutable. +Historical rows remain unknown rather than receiving a guessed identity. Billing counts distinct GitHub author IDs on private pull requests within the entitlement period; bot and service identities count by the same ID rule. diff --git a/drizzle/0030_private_review_author_invariant.sql b/drizzle/0030_private_review_author_invariant.sql new file mode 100644 index 00000000..1fb0d308 --- /dev/null +++ b/drizzle/0030_private_review_author_invariant.sql @@ -0,0 +1,48 @@ +CREATE OR REPLACE FUNCTION enforce_private_review_author_identity() RETURNS trigger +LANGUAGE plpgsql +AS $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 + FROM repositories + WHERE repositories.id = NEW.repository_id + AND repositories.private = true + ) THEN + RETURN NEW; + END IF; + + PERFORM pg_advisory_xact_lock(hashtextextended('postil:private-review-author-v1', 0)); + IF NOT EXISTS ( + SELECT 1 + FROM deployment_capabilities + WHERE deployment_capabilities.name = 'private-review-author-v1' + ) THEN + RETURN NEW; + END IF; + + IF TG_OP = 'UPDATE' + AND ( + OLD.author_github_id IS DISTINCT FROM NEW.author_github_id + OR OLD.author_login IS DISTINCT FROM NEW.author_login + ) THEN + RAISE EXCEPTION 'private review author identity is immutable'; + END IF; + + IF NEW.status IN ('queued', 'running', 'completed') + AND ( + NEW.author_github_id IS NULL + OR NEW.author_github_id <= 0 + OR NEW.author_github_id > 9007199254740991 + OR NEW.author_login IS NULL + OR length(btrim(NEW.author_login)) = 0 + OR length(NEW.author_login) > 100 + ) THEN + RAISE EXCEPTION 'private review author identity is required'; + END IF; + RETURN NEW; +END; +$$; +--> statement-breakpoint +CREATE TRIGGER reviews_private_author_identity_required +BEFORE INSERT OR UPDATE OF status, repository_id, author_github_id, author_login ON reviews +FOR EACH ROW EXECUTE FUNCTION enforce_private_review_author_identity(); diff --git a/drizzle/meta/_journal.json b/drizzle/meta/_journal.json index f1772485..b58922cf 100644 --- a/drizzle/meta/_journal.json +++ b/drizzle/meta/_journal.json @@ -211,6 +211,13 @@ "when": 1784297400000, "tag": "0029_review_trigger_provenance", "breakpoints": true + }, + { + "idx": 30, + "version": "7", + "when": 1784394144533, + "tag": "0030_private_review_author_invariant", + "breakpoints": true } ] } diff --git a/scripts/activate-release-jobs.ts b/scripts/activate-release-jobs.ts index 7dc4f910..5d86f400 100644 --- a/scripts/activate-release-jobs.ts +++ b/scripts/activate-release-jobs.ts @@ -1,6 +1,9 @@ import { closeDb, getDb, getPool } from "@/lib/db"; import { finalizeEscalationEmailRetirement } from "@/lib/escalation-email-retirement"; -import { activateReleaseJobs } from "@/lib/release-job-rollout"; +import { + activatePrivateReviewAuthorIdentity, + activateReleaseJobs, +} from "@/lib/release-job-rollout"; import { backfillBillingContactVerification } from "./backfill-billing-contact-verification"; async function main(): Promise { @@ -12,9 +15,12 @@ async function main(): Promise { const billing = await backfillBillingContactVerification(getDb(), { confirm: true, }); + const privateReviewAuthorActivated = + await activatePrivateReviewAuthorIdentity(getPool()); const released = await activateReleaseJobs(getPool()); console.log( `release job kinds activated: released=${released} ` + + `private_review_author=${privateReviewAuthorActivated ? "activated" : "already_active"} ` + `billing_pending=${billing.pending} billing_queued=${billing.queued} ` + `escalation_terminalized=${retirement.terminalized} ` + `escalation_redacted=${retirement.redacted} ` + diff --git a/src/lib/github/checks.ts b/src/lib/github/checks.ts index d90af50e..73e7ea5f 100644 --- a/src/lib/github/checks.ts +++ b/src/lib/github/checks.ts @@ -295,11 +295,15 @@ export async function getPullRequestReviewContext( if (!headSha || !baseSha) { throw new Error(`GitHub pull request ${repoFullName}#${number} has incomplete refs`); } + const authorGithubId = data.user?.id; + const authorLogin = typeof data.user?.login === "string" ? data.user.login.trim() : undefined; return { headSha, baseSha, draft: data.draft === true, - ...(typeof data.user?.id === "number" ? { authorGithubId: data.user.id } : {}), - ...(data.user?.login ? { authorLogin: data.user.login } : {}), + ...(typeof authorGithubId === "number" && Number.isSafeInteger(authorGithubId) && authorGithubId > 0 + ? { authorGithubId } + : {}), + ...(authorLogin && authorLogin.length <= 100 ? { authorLogin } : {}), }; } diff --git a/src/lib/release-job-rollout.ts b/src/lib/release-job-rollout.ts index 13f127a5..6a4c6eaa 100644 --- a/src/lib/release-job-rollout.ts +++ b/src/lib/release-job-rollout.ts @@ -8,6 +8,57 @@ export const RELEASE_V1_JOB_KINDS = [ const CAPABILITY = "release-v1-jobs"; const ADVISORY_LOCK_NAME = "postil:release-v1-jobs"; +export const PRIVATE_REVIEW_AUTHOR_CAPABILITY = "private-review-author-v1"; +const PRIVATE_REVIEW_AUTHOR_LOCK = "postil:private-review-author-v1"; + +/** Activate private-review author enforcement after every managed process runs compatible code. */ +export async function activatePrivateReviewAuthorIdentity( + pool: Pool, +): Promise { + const client = await pool.connect(); + try { + await client.query("BEGIN"); + await client.query( + "SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", + [PRIVATE_REVIEW_AUTHOR_LOCK], + ); + const anonymousActive = await client.query<{ blocked: boolean }>( + `SELECT EXISTS ( + SELECT 1 + FROM reviews + JOIN repositories ON repositories.id = reviews.repository_id + WHERE repositories.private = true + AND reviews.status IN ('queued', 'running') + AND ( + reviews.author_github_id IS NULL + OR reviews.author_github_id <= 0 + OR reviews.author_github_id > 9007199254740991 + OR reviews.author_login IS NULL + OR length(btrim(reviews.author_login)) = 0 + OR length(reviews.author_login) > 100 + ) + ) AS blocked`, + ); + if (anonymousActive.rows[0]?.blocked) { + throw new Error( + "private review author enforcement has anonymous active reviews", + ); + } + const activated = await client.query( + `INSERT INTO deployment_capabilities (name) + VALUES ($1) + ON CONFLICT (name) DO NOTHING`, + [PRIVATE_REVIEW_AUTHOR_CAPABILITY], + ); + await client.query("COMMIT"); + return (activated.rowCount ?? 0) > 0; + } catch (error) { + await client.query("ROLLBACK").catch(() => undefined); + throw error; + } finally { + client.release(); + } +} /** * Activate new job consumers after the deploy has replaced every old diff --git a/src/worker/review.ts b/src/worker/review.ts index 33f712a8..e6ed57dc 100644 --- a/src/worker/review.ts +++ b/src/worker/review.ts @@ -33,6 +33,7 @@ import { completeCheckRun, createCheckRun, findCheckRunByExternalId, + getPullRequestReviewContext, } from "@/lib/github/checks"; import { materializeOrgConfig, @@ -605,6 +606,31 @@ export async function runReviewJob( return; } + let authorGithubId = payload.authorGithubId; + let authorLogin = payload.authorLogin; + if (currentRepository.private) { + const context = await translateWorkerAbort( + getPullRequestReviewContext(token, currentRepository.full_name, payload.prNumber, signal), + signal, + ); + if (context.headSha !== payload.headSha || context.baseSha !== payload.baseSha) { + console.warn( + `review job skipped: ${currentRepository.full_name}#${payload.prNumber} refs changed before author verification`, + ); + return; + } + if ( + typeof context.authorGithubId !== "number" || + !Number.isSafeInteger(context.authorGithubId) || + context.authorGithubId <= 0 || + !context.authorLogin + ) { + throw new OperationalError("private review author identity is unavailable"); + } + authorGithubId = context.authorGithubId; + authorLogin = context.authorLogin; + } + const hostedReviewUnavailable = !llm.byok && !hostedInferenceEnabled(); // Incremental re-review: baseline = last completed review of this PR. @@ -636,8 +662,8 @@ export async function runReviewJob( const reviewValues = { repositoryId: repository.id, prNumber: payload.prNumber, - authorGithubId: payload.authorGithubId ?? null, - authorLogin: payload.authorLogin ?? null, + authorGithubId: authorGithubId ?? null, + authorLogin: authorLogin ?? null, headSha: payload.headSha, baseSha: payload.baseSha, sinceSha: baseline?.headSha ?? null, diff --git a/tests/github-checks.test.ts b/tests/github-checks.test.ts index dd336b08..30e84fa3 100644 --- a/tests/github-checks.test.ts +++ b/tests/github-checks.test.ts @@ -101,6 +101,37 @@ describe("pull-request review context", () => { }); }); + test("normalizes only a complete bounded author identity", async () => { + globalThis.fetch = (async (_input) => + Response.json({ + draft: false, + head: { sha: "head-sha" }, + base: { sha: "base-sha" }, + user: { id: 0, login: " " }, + })) as typeof fetch; + + await expect(getPullRequestReviewContext("token", "octo/repo", 7)).resolves.toEqual({ + headSha: "head-sha", + baseSha: "base-sha", + draft: false, + }); + + globalThis.fetch = (async (_input) => + Response.json({ + draft: false, + head: { sha: "head-sha" }, + base: { sha: "base-sha" }, + user: { id: 42, login: ` ${"a".repeat(101)} ` }, + })) as typeof fetch; + + await expect(getPullRequestReviewContext("token", "octo/repo", 7)).resolves.toEqual({ + headSha: "head-sha", + baseSha: "base-sha", + draft: false, + authorGithubId: 42, + }); + }); + test("fails closed when either immutable ref is absent", async () => { globalThis.fetch = (async (_input) => Response.json({ head: { sha: "head-sha" }, base: {} })) as typeof fetch; diff --git a/tests/private-review-author-migration.test.ts b/tests/private-review-author-migration.test.ts new file mode 100644 index 00000000..d68b1354 --- /dev/null +++ b/tests/private-review-author-migration.test.ts @@ -0,0 +1,187 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { readdir, readFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { Pool } from "pg"; + +import { activatePrivateReviewAuthorIdentity } from "@/lib/release-job-rollout"; + +const TEST_URL = process.env.POSTIL_TEST_DATABASE_URL; +const describeDb = TEST_URL ? describe : describe.skip; + +describeDb("private review author migration", () => { + const pool = new Pool({ connectionString: TEST_URL, max: 4 }); + let privateRepositoryId: number; + let publicRepositoryId: number; + + beforeAll(async () => { + await pool.query( + "DROP SCHEMA IF EXISTS public CASCADE; CREATE SCHEMA public", + ); + const migrationDirectory = join(import.meta.dir, "..", "drizzle"); + const migrations = (await readdir(migrationDirectory)) + .filter((file) => file.endsWith(".sql") && file < "0030_") + .sort(); + for (const migration of migrations) { + const sql = await readFile(join(migrationDirectory, migration), "utf8"); + for (const statement of sql.split("--> statement-breakpoint")) { + if (statement.trim()) await pool.query(statement); + } + } + + const organization = await pool.query<{ id: string }>( + "INSERT INTO organizations (slug, name) VALUES ('author-audit', 'Author audit') RETURNING id", + ); + const installation = await pool.query<{ id: string }>( + `INSERT INTO installations + (github_installation_id, org_id, account_login, account_type) + VALUES (404, $1, 'author-audit', 'Organization') RETURNING id`, + [organization.rows[0]!.id], + ); + const repositories = await pool.query<{ id: string; private: boolean }>( + `INSERT INTO repositories + (installation_id, github_repo_id, full_name, private, enabled) + VALUES + ($1, 4001, 'author-audit/private', true, true), + ($1, 4002, 'author-audit/public', false, true) + RETURNING id, private`, + [installation.rows[0]!.id], + ); + privateRepositoryId = Number( + repositories.rows.find((row) => row.private)!.id, + ); + publicRepositoryId = Number( + repositories.rows.find((row) => !row.private)!.id, + ); + + await pool.query( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status, finished_at) + VALUES ($1, 1, 'historical-head', 'base', 'completed', now())`, + [privateRepositoryId], + ); + + const migration = await readFile( + join(migrationDirectory, "0030_private_review_author_invariant.sql"), + "utf8", + ); + for (const statement of migration.split("--> statement-breakpoint")) { + if (statement.trim()) await pool.query(statement); + } + }); + + afterAll(async () => { + await pool.end(); + }); + + test("closes the activation race and enforces durable private author identity", async () => { + const legacy = await pool.query<{ + author_github_id: string | null; + author_login: string | null; + }>( + "SELECT author_github_id, author_login FROM reviews WHERE head_sha = 'historical-head'", + ); + expect(legacy.rows).toEqual([ + { author_github_id: null, author_login: null }, + ]); + + const rollingClient = await pool.connect(); + try { + await rollingClient.query("BEGIN"); + await rollingClient.query( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status) + VALUES ($1, 2, 'rolling-head', 'base', 'running')`, + [privateRepositoryId], + ); + + const activation = activatePrivateReviewAuthorIdentity(pool); + const earlyResult = await Promise.race([ + activation.then( + () => "activated" as const, + () => "rejected" as const, + ), + Bun.sleep(75).then(() => "blocked" as const), + ]); + expect(earlyResult).toBe("blocked"); + await rollingClient.query("COMMIT"); + await expect(activation).rejects.toThrow("anonymous active reviews"); + } finally { + await rollingClient.query("ROLLBACK").catch(() => undefined); + rollingClient.release(); + } + await pool.query( + "UPDATE reviews SET status = 'stale' WHERE head_sha = 'rolling-head'", + ); + await pool.query( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status) + VALUES ($1, 8, 'queued-before-activation', 'base', 'queued')`, + [privateRepositoryId], + ); + await expect(activatePrivateReviewAuthorIdentity(pool)).rejects.toThrow( + "anonymous active reviews", + ); + await pool.query( + "UPDATE reviews SET status = 'stale' WHERE head_sha = 'queued-before-activation'", + ); + expect(await activatePrivateReviewAuthorIdentity(pool)).toBe(true); + expect(await activatePrivateReviewAuthorIdentity(pool)).toBe(false); + + const rolling = await pool.query<{ + status: string; + author_github_id: string | null; + }>( + "SELECT status, author_github_id FROM reviews WHERE head_sha = 'rolling-head'", + ); + expect(rolling.rows).toEqual([{ status: "stale", author_github_id: null }]); + + await expect( + pool.query( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status) + VALUES ($1, 3, 'anonymous-head', 'base', 'running')`, + [privateRepositoryId], + ), + ).rejects.toMatchObject({ code: "P0001" }); + await expect( + pool.query( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status, author_github_id, author_login) + VALUES ($1, 4, 'invalid-head', 'base', 'running', 0, ' ')`, + [privateRepositoryId], + ), + ).rejects.toMatchObject({ code: "P0001" }); + + await pool.query( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status) + VALUES ($1, 5, 'public-head', 'base', 'running')`, + [publicRepositoryId], + ); + await expect( + pool.query( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status) + VALUES ($1, 6, 'queued-head', 'base', 'queued')`, + [privateRepositoryId], + ), + ).rejects.toMatchObject({ code: "P0001" }); + + const verified = await pool.query<{ id: string }>( + `INSERT INTO reviews + (repository_id, pr_number, head_sha, base_sha, status, author_github_id, author_login) + VALUES ($1, 7, 'verified-head', 'base', 'running', 42, 'octocat') + RETURNING id`, + [privateRepositoryId], + ); + await pool.query("UPDATE reviews SET status = 'completed' WHERE id = $1", [ + verified.rows[0]!.id, + ]); + await expect( + pool.query("UPDATE reviews SET author_login = 'another' WHERE id = $1", [ + verified.rows[0]!.id, + ]), + ).rejects.toMatchObject({ code: "P0001" }); + }); +}); diff --git a/tests/private-worker-gates.test.ts b/tests/private-worker-gates.test.ts index 02bc3c27..3a45a495 100644 --- a/tests/private-worker-gates.test.ts +++ b/tests/private-worker-gates.test.ts @@ -7,8 +7,10 @@ describe("private repository worker defense in depth", () => { const installationSync = readFileSync("src/lib/github/installation-sync.ts", "utf8"); const gate = source.indexOf("await canProcessPrivateRepository", source.indexOf("runReviewJob")); expect(gate).toBeGreaterThan(0); - expect(source).toContain("authorGithubId: payload.authorGithubId"); - expect(source).toContain("authorLogin: payload.authorLogin"); + const authorLookup = source.indexOf("getPullRequestReviewContext", gate); + expect(authorLookup).toBeGreaterThan(gate); + expect(source).toContain("authorGithubId: authorGithubId"); + expect(source).toContain("authorLogin: authorLogin"); for (const sideEffect of [ "insert(schema.reviews)", "await getInstallationToken", @@ -24,11 +26,26 @@ describe("private repository worker defense in depth", () => { expect(source.indexOf("fetchRepositorySummary", gate)).toBeLessThan( source.indexOf("insert(schema.reviews)", gate), ); + expect(authorLookup).toBeLessThan(source.indexOf("insert(schema.reviews)", gate)); + expect(authorLookup).toBeLessThan(source.indexOf("await reserveHostedReviewSpend", gate)); + expect(authorLookup).toBeLessThan(source.indexOf("await runCli", gate)); + expect( + source.slice(authorLookup, source.indexOf("const hostedReviewUnavailable", gate)), + ).toContain("private review author identity is unavailable"); expect(installationSync).toContain( "AbortSignal.any([signal, AbortSignal.timeout(10_000)])", ); }); + test("private author enforcement activates only after the managed fleet replacement", () => { + const deploy = readFileSync(".github/workflows/deploy.yml", "utf8"); + const activation = readFileSync("scripts/activate-release-jobs.ts", "utf8"); + expect(deploy.indexOf("Deploy managed fleet")).toBeLessThan( + deploy.indexOf("Activate release capabilities after fleet replacement"), + ); + expect(activation).toContain("activatePrivateReviewAuthorIdentity"); + }); + test("disabled hosted inference stops before reservation, config fetch, or CLI spawn", () => { const source = readFileSync("src/worker/review.ts", "utf8"); const claimSource = readFileSync("src/lib/hosted-review-pause.ts", "utf8"); diff --git a/tests/queue.test.ts b/tests/queue.test.ts index 7881d99d..ce19d930 100644 --- a/tests/queue.test.ts +++ b/tests/queue.test.ts @@ -20,7 +20,11 @@ import { finalizeEscalationEmailRetirement, quiesceEscalationEmailJobs, } from "@/lib/escalation-email-retirement"; -import { activateReleaseJobs } from "@/lib/release-job-rollout"; +import { + activatePrivateReviewAuthorIdentity, + activateReleaseJobs, + PRIVATE_REVIEW_AUTHOR_CAPABILITY, +} from "@/lib/release-job-rollout"; /** * Queue claim semantics against a real Postgres (FOR UPDATE SKIP LOCKED @@ -149,6 +153,16 @@ describeDb("postgres job queue", () => { expect(later.rows[0]?.runnable).toBe(true); }); + test("activates private author enforcement idempotently", async () => { + expect(await activatePrivateReviewAuthorIdentity(pool)).toBe(true); + expect(await activatePrivateReviewAuthorIdentity(pool)).toBe(false); + const capability = await pool.query<{ name: string }>( + "SELECT name FROM deployment_capabilities WHERE name = $1", + [PRIVATE_REVIEW_AUTHOR_CAPABILITY], + ); + expect(capability.rows).toEqual([{ name: PRIVATE_REVIEW_AUTHOR_CAPABILITY }]); + }); + test("retires staged escalation email work and clears recipient material", async () => { const slug = `retirement-${randomUUID()}`; const organization = await pool.query<{ id: string }>(