fix(ai-review): BYOK round 2 — key-lifecycle audit, budget separation, per-record salt

migrations/0030_ai_key_salt.sql

@@ -0,0 +1,4 @@
+-- Per-record PBKDF2 salt for the v2 BYOK key-encryption envelope (defense-in-depth; decouples each
+-- record's derived AES key). Nullable: existing v1 rows keep salt = NULL and decrypt with the legacy
+-- constant salt. New writes store a random salt and set key_version = 2. See src/utils/crypto.ts.
+ALTER TABLE repository_ai_keys ADD COLUMN salt TEXT;

node_modules

@@ -0,0 +1 @@
+/Users/shadowbook/Documents/gittensory/node_modules

src/api/routes.ts

@@ -1839,7 +1839,8 @@ export function createApp() {
     const fullName = `${c.req.param("owner")}/${c.req.param("repo")}`;
     const gate = await requireRepoMaintainer(c, fullName);
     if (gate instanceof Response) return gate;
-    await deleteRepositoryAiKey(c.env, fullName);
+    const actor = gate.identity?.kind === "session" ? gate.identity.actor : null;
+    await deleteRepositoryAiKey(c.env, fullName, actor);
     return c.json({ configured: false });
   });
 

src/db/repositories.ts

@@ -531,7 +531,7 @@ export type AiKeyProvider = "anthropic" | "openai";
 
 /** Public, secret-free status of a repo's BYOK key. NEVER includes the key or ciphertext. */
 export type RepositoryAiKeyStatus =
-  | { configured: true; provider: AiKeyProvider; last4: string; model: string | null }
+  | { configured: true; provider: AiKeyProvider; last4: string; model: string | null; createdBy: string | null; updatedAt: string | null }
   | { configured: false };
 
 /** A decrypted provider key for use at AI-call time only. Never returned from the API, never logged. */
@@ -546,7 +546,7 @@ export async function getRepositoryAiKeyStatus(env: Env, fullName: string): Prom
   const db = getDb(env.DB);
   const [row] = await db.select().from(repositoryAiKeys).where(eq(repositoryAiKeys.repoFullName, fullName)).limit(1);
   if (!row) return { configured: false };
-  return { configured: true, provider: normalizeAiKeyProvider(row.provider), last4: row.last4, model: row.model ?? null };
+  return { configured: true, provider: normalizeAiKeyProvider(row.provider), last4: row.last4, model: row.model ?? null, createdBy: row.createdBy, updatedAt: row.updatedAt };
 }
 
 /**
@@ -561,24 +561,53 @@ export async function upsertRepositoryAiKey(
   const secret = env.TOKEN_ENCRYPTION_SECRET;
   if (!secret) throw new Error("missing_encryption_secret");
   const trimmedKey = input.key.trim();
-  const { ciphertext, iv, version } = await encryptSecret(trimmedKey, secret);
+  const existing = await getRepositoryAiKeyStatus(env, input.repoFullName);
+  const { ciphertext, iv, salt, version } = await encryptSecret(trimmedKey, secret);
   const last4 = trimmedKey.slice(-4);
   const model = input.model?.trim() ? input.model.trim() : null;
+  const createdBy = input.createdBy ?? null;
+  const updatedAt = nowIso();
   const db = getDb(env.DB);
   await db
     .insert(repositoryAiKeys)
-    .values({ repoFullName: input.repoFullName, provider: input.provider, ciphertext, iv, keyVersion: version, model, last4, createdBy: input.createdBy ?? null, updatedAt: nowIso() })
+    .values({ repoFullName: input.repoFullName, provider: input.provider, ciphertext, iv, salt, keyVersion: version, model, last4, createdBy, updatedAt })
     .onConflictDoUpdate({
       target: repositoryAiKeys.repoFullName,
-      set: { provider: input.provider, ciphertext, iv, keyVersion: version, model, last4, createdBy: input.createdBy ?? null, updatedAt: nowIso() },
+      set: { provider: input.provider, ciphertext, iv, salt, keyVersion: version, model, last4, createdBy, updatedAt },
     });
-  return { configured: true, provider: input.provider, last4, model };
+  await recordAiKeyChange(env, { repoFullName: input.repoFullName, action: existing.configured ? "replace" : "set", provider: input.provider, last4, actor: createdBy });
+  return { configured: true, provider: input.provider, last4, model, createdBy, updatedAt };
 }
 
-/** Remove a repo's BYOK key. */
-export async function deleteRepositoryAiKey(env: Env, fullName: string): Promise<void> {
+/** Remove a repo's BYOK key. Records a lifecycle audit event when a key was actually present. */
+export async function deleteRepositoryAiKey(env: Env, fullName: string, actor?: string | null): Promise<void> {
+  const existing = await getRepositoryAiKeyStatus(env, fullName);
   const db = getDb(env.DB);
   await db.delete(repositoryAiKeys).where(eq(repositoryAiKeys.repoFullName, fullName));
+  if (existing.configured) {
+    await recordAiKeyChange(env, { repoFullName: fullName, action: "delete", provider: existing.provider, last4: existing.last4, actor: actor ?? null });
+  }
+}
+
+/**
+ * Audit a BYOK key lifecycle change (set/replace/delete). Stored in ai_usage_events as a non-"ok"
+ * status so it never counts toward the daily neuron budget. NEVER includes any key material — only the
+ * display-only last4 and the actor who made the change.
+ */
+async function recordAiKeyChange(
+  env: Env,
+  input: { repoFullName: string; action: "set" | "replace" | "delete"; provider: AiKeyProvider; last4: string; actor: string | null },
+): Promise<void> {
+  await recordAiUsageEvent(env, {
+    feature: "ai_key_change",
+    actor: input.actor,
+    route: "maintainer.ai_key",
+    model: `byok:${input.provider}`,
+    status: input.action,
+    estimatedNeurons: 0,
+    detail: `provider key ${input.action}`,
+    metadata: { repoFullName: input.repoFullName, action: input.action, provider: input.provider, last4: input.last4 },
+  });
 }
 
 /**
@@ -593,7 +622,7 @@ export async function getDecryptedRepositoryAiKey(env: Env, fullName: string): P
   const [row] = await db.select().from(repositoryAiKeys).where(eq(repositoryAiKeys.repoFullName, fullName)).limit(1);
   if (!row) return null;
   try {
-    const key = await decryptSecret(row.ciphertext, row.iv, secret);
+    const key = await decryptSecret(row.ciphertext, row.iv, secret, row.salt);
     return { provider: normalizeAiKeyProvider(row.provider), key, model: row.model ?? null };
   } catch {
     return null;

src/db/schema.ts

@@ -75,6 +75,10 @@ export const repositoryAiKeys = sqliteTable("repository_ai_keys", {
   provider: text("provider").notNull(),
   ciphertext: text("ciphertext").notNull(),
   iv: text("iv").notNull(),
+  // Per-record PBKDF2 salt (base64) for the v2 crypto envelope; null for legacy v1 rows (constant salt).
+  salt: text("salt"),
+  // Crypto-envelope version (NOT a key-rotation counter): 1 = legacy constant-salt, 2 = per-record salt.
+  // upsert overwrites in place; there is no rotation history. See src/utils/crypto.ts.
   keyVersion: integer("key_version").notNull().default(1),
   model: text("model"),
   last4: text("last4").notNull(),

src/services/ai-review.ts

@@ -304,9 +304,12 @@ export async function runGittensoryAiReview(env: Env, input: GittensoryAiReviewI
 
   const maxTokens = clampNumber(Number(env.AI_MAX_OUTPUT_TOKENS || 256), 256, 1024);
   const user = buildUserPrompt(input);
-  // block mode = advisory pass + consensus pass (2 models each, minus 1 when BYOK writes the advisory).
-  const calls = input.mode === "block" ? 3 : input.providerKey ? 1 : 2;
-  const estimatedNeurons = estimateNeurons(REVIEW_SYSTEM_PROMPT.length + user.length, maxTokens, calls);
+  // The daily neuron budget governs FREE Workers-AI spend only. BYOK advisory calls bill the maintainer's
+  // own provider account, so they are not counted here (and a BYOK advisory still runs when the free
+  // budget is exhausted). Free calls = the consensus pair in block mode (always Workers AI), plus the
+  // advisory leg only when it is NOT BYOK.
+  const freeAiCalls = (input.mode === "block" ? 2 : 0) + (input.providerKey ? 0 : 1);
+  const estimatedNeurons = freeAiCalls === 0 ? 0 : estimateNeurons(REVIEW_SYSTEM_PROMPT.length + user.length, maxTokens, freeAiCalls);
   const budget = clampNumber(Number(env.AI_DAILY_NEURON_BUDGET || 10000), 0, 1_000_000);
   const used = await sumAiEstimatedNeuronsSince(env, utcDayStartIso());
   const remainingBudget = Math.max(0, budget - used);

src/utils/crypto.ts

@@ -46,33 +46,54 @@ function hexToBytes(hex: string): Uint8Array {
 // AI-call time. The AES key is derived from the worker secret TOKEN_ENCRYPTION_SECRET via PBKDF2; a
 // fresh random 12-byte IV is used per encryption so ciphertexts are unique and the GCM tag authenticates
 // them. The plaintext key is never persisted, never logged, and never returned from the API.
-const SECRET_KDF_SALT = new TextEncoder().encode("gittensory-secret-encryption-v1");
-const SECRET_KEY_VERSION = 1;
-
-async function deriveSecretAesKey(keyMaterial: string): Promise<CryptoKey> {
+//
+// Envelope versions (stored as key_version alongside the row):
+//   1 = legacy: a single constant KDF salt for every record (SECRET_KDF_SALT_V1).
+//   2 = current: a fresh random per-record salt, stored beside the IV, so each record's AES key is
+//       independently derived (defense-in-depth; decouples derived keys, eases future KDF rotation).
+// Decryption keys off whether a per-record salt is present, so existing v1 rows (salt = null) keep
+// decrypting with the constant salt.
+const SECRET_KDF_SALT_V1 = new TextEncoder().encode("gittensory-secret-encryption-v1");
+const SECRET_KEY_VERSION_CURRENT = 2;
+
+async function deriveSecretAesKey(keyMaterial: string, salt: Uint8Array): Promise<CryptoKey> {
   const baseKey = await crypto.subtle.importKey("raw", new TextEncoder().encode(keyMaterial), "PBKDF2", false, ["deriveKey"]);
   return crypto.subtle.deriveKey(
-    { name: "PBKDF2", salt: SECRET_KDF_SALT, iterations: 100_000, hash: "SHA-256" },
+    { name: "PBKDF2", salt, iterations: 100_000, hash: "SHA-256" },
     baseKey,
     { name: "AES-GCM", length: 256 },
     false,
     ["encrypt", "decrypt"],
   );
 }
 
-/** Encrypt a secret with AES-256-GCM. Returns base64 ciphertext (incl. auth tag) + base64 IV + version. */
-export async function encryptSecret(plaintext: string, keyMaterial: string): Promise<{ ciphertext: string; iv: string; version: number }> {
+/**
+ * Encrypt a secret with AES-256-GCM. Returns base64 ciphertext (incl. auth tag) + base64 IV + the
+ * per-record salt (base64, null for the legacy v1 envelope) + envelope version. Production always uses
+ * the current envelope; `version` is parameterized only so tests can produce legacy v1 ciphertexts.
+ */
+export async function encryptSecret(
+  plaintext: string,
+  keyMaterial: string,
+  version: number = SECRET_KEY_VERSION_CURRENT,
+): Promise<{ ciphertext: string; iv: string; salt: string | null; version: number }> {
   if (!keyMaterial) throw new Error("missing_encryption_secret");
-  const key = await deriveSecretAesKey(keyMaterial);
+  const saltBytes = version >= 2 ? crypto.getRandomValues(new Uint8Array(16)) : SECRET_KDF_SALT_V1;
+  const key = await deriveSecretAesKey(keyMaterial, saltBytes);
   const iv = crypto.getRandomValues(new Uint8Array(12));
   const encrypted = await crypto.subtle.encrypt({ name: "AES-GCM", iv }, key, new TextEncoder().encode(plaintext));
-  return { ciphertext: base64Encode(new Uint8Array(encrypted)), iv: base64Encode(iv), version: SECRET_KEY_VERSION };
+  return { ciphertext: base64Encode(new Uint8Array(encrypted)), iv: base64Encode(iv), salt: version >= 2 ? base64Encode(saltBytes) : null, version };
 }
 
-/** Decrypt a secret produced by {@link encryptSecret}. Throws if the secret/IV/ciphertext do not match. */
-export async function decryptSecret(ciphertext: string, iv: string, keyMaterial: string): Promise<string> {
+/**
+ * Decrypt a secret produced by {@link encryptSecret}. Pass the stored per-record `salt` for v2 rows;
+ * omit it (or pass null) for legacy v1 rows, which fall back to the constant salt. Throws if the
+ * secret/IV/salt/ciphertext do not match.
+ */
+export async function decryptSecret(ciphertext: string, iv: string, keyMaterial: string, salt?: string | null): Promise<string> {
   if (!keyMaterial) throw new Error("missing_encryption_secret");
-  const key = await deriveSecretAesKey(keyMaterial);
+  const saltBytes = salt ? base64ToBytes(salt) : SECRET_KDF_SALT_V1;
+  const key = await deriveSecretAesKey(keyMaterial, saltBytes);
   const decrypted = await crypto.subtle.decrypt({ name: "AES-GCM", iv: base64ToBytes(iv) }, key, base64ToBytes(ciphertext));
   return new TextDecoder().decode(decrypted);
 }

test/unit/ai-key-byok.test.ts

@@ -10,20 +10,29 @@ import { createTestEnv } from "../helpers/d1";
 const SECRET = "unit-test-encryption-secret-at-least-32-bytes-long";
 
 describe("encryptSecret / decryptSecret (AES-256-GCM)", () => {
-  it("round-trips a secret and produces a fresh IV each time", async () => {
+  it("round-trips a secret with a fresh IV and a fresh per-record salt each time (v2 envelope)", async () => {
     const a = await encryptSecret("sk-ant-supersecret", SECRET);
     const b = await encryptSecret("sk-ant-supersecret", SECRET);
     expect(a.iv).not.toBe(b.iv); // random IV per encryption
+    expect(a.salt).not.toBe(b.salt); // random per-record salt per encryption
     expect(a.ciphertext).not.toBe(b.ciphertext);
-    expect(a.version).toBe(1);
-    await expect(decryptSecret(a.ciphertext, a.iv, SECRET)).resolves.toBe("sk-ant-supersecret");
+    expect(a.version).toBe(2);
+    await expect(decryptSecret(a.ciphertext, a.iv, SECRET, a.salt)).resolves.toBe("sk-ant-supersecret");
+  });
+
+  it("decrypts legacy v1 rows (no per-record salt) with the constant salt", async () => {
+    const legacy = await encryptSecret("sk-ant-legacy-key", SECRET, 1);
+    expect(legacy.version).toBe(1);
+    expect(legacy.salt).toBeNull();
+    // A v1 row stores salt = NULL; decrypt without a salt falls back to the constant salt.
+    await expect(decryptSecret(legacy.ciphertext, legacy.iv, SECRET)).resolves.toBe("sk-ant-legacy-key");
   });
 
   it("fails to decrypt with the wrong secret and throws without a key", async () => {
-    const { ciphertext, iv } = await encryptSecret("sk-secret", SECRET);
-    await expect(decryptSecret(ciphertext, iv, "a-different-secret-of-sufficient-length-here")).rejects.toThrow();
+    const { ciphertext, iv, salt } = await encryptSecret("sk-secret", SECRET);
+    await expect(decryptSecret(ciphertext, iv, "a-different-secret-of-sufficient-length-here", salt)).rejects.toThrow();
     await expect(encryptSecret("x", "")).rejects.toThrow("missing_encryption_secret");
-    await expect(decryptSecret(ciphertext, iv, "")).rejects.toThrow("missing_encryption_secret");
+    await expect(decryptSecret(ciphertext, iv, "", salt)).rejects.toThrow("missing_encryption_secret");
   });
 });
 
@@ -33,12 +42,13 @@ describe("repository BYOK key storage", () => {
     await expect(getRepositoryAiKeyStatus(env, "acme/widgets")).resolves.toEqual({ configured: false });
 
     const status = await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "anthropic", key: "sk-ant-abc123XYZ7890", model: "claude-3-5-sonnet-latest", createdBy: "maintainer" });
-    expect(status).toEqual({ configured: true, provider: "anthropic", last4: "7890", model: "claude-3-5-sonnet-latest" });
+    expect(status).toMatchObject({ configured: true, provider: "anthropic", last4: "7890", model: "claude-3-5-sonnet-latest", createdBy: "maintainer" });
+    expect(status.configured && status.updatedAt).toMatch(/^\d{4}-\d{2}-\d{2}T/);
 
-    // Status surface never includes the key or ciphertext.
+    // Status surface never includes the key or ciphertext, but does surface who set it + when.
     const fetched = await getRepositoryAiKeyStatus(env, "acme/widgets");
     expect(JSON.stringify(fetched)).not.toContain("sk-ant");
-    expect(fetched).toMatchObject({ configured: true, last4: "7890" });
+    expect(fetched).toMatchObject({ configured: true, last4: "7890", createdBy: "maintainer" });
 
     // Decrypt only happens at call time.
     await expect(getDecryptedRepositoryAiKey(env, "acme/widgets")).resolves.toEqual({ provider: "anthropic", key: "sk-ant-abc123XYZ7890", model: "claude-3-5-sonnet-latest" });
@@ -59,6 +69,26 @@ describe("repository BYOK key storage", () => {
     await expect(getDecryptedRepositoryAiKey(env, "acme/widgets")).resolves.toBeNull();
   });
 
+  it("audits the key lifecycle (set → replace → delete) without ever recording key material", async () => {
+    const env = createTestEnv({ TOKEN_ENCRYPTION_SECRET: SECRET });
+    await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "anthropic", key: "sk-ant-first-key-0000", createdBy: "alice" });
+    await upsertRepositoryAiKey(env, { repoFullName: "acme/widgets", provider: "openai", key: "sk-openai-second-1111", createdBy: "bob" });
+    await deleteRepositoryAiKey(env, "acme/widgets", "carol");
+
+    const events = await env.DB.prepare("select actor, status, model, metadata_json from ai_usage_events where feature = ? order by rowid asc").bind("ai_key_change").all<{ actor: string; status: string; model: string; metadata_json: string }>();
+    expect(events.results.map((e) => e.status)).toEqual(["set", "replace", "delete"]);
+    expect(events.results.map((e) => e.actor)).toEqual(["alice", "bob", "carol"]);
+    // Audit rows never contain key material — only the display-only last4.
+    const blob = JSON.stringify(events.results);
+    expect(blob).not.toContain("sk-ant");
+    expect(blob).not.toContain("sk-openai");
+
+    // A delete with no key present records nothing.
+    await deleteRepositoryAiKey(env, "acme/widgets", "carol");
+    const after = await env.DB.prepare("select count(*) as n from ai_usage_events where feature = ?").bind("ai_key_change").first<{ n: number }>();
+    expect(after?.n).toBe(3);
+  });
+
   it("stores real ISO timestamps when created_at/updated_at are omitted (no literal default)", async () => {
     const env = createTestEnv({ TOKEN_ENCRYPTION_SECRET: SECRET });
     const db = getDb(env.DB);

test/unit/ai-review.test.ts

@@ -52,6 +52,27 @@ describe("runGittensoryAiReview gating", () => {
     await expect(runGittensoryAiReview(env, baseInput)).resolves.toMatchObject({ status: "quota_exceeded" });
     expect(run).not.toHaveBeenCalled();
   });
+
+  it("clamps a non-numeric AI_MAX_OUTPUT_TOKENS back to the default", async () => {
+    const run = vi.fn(async () => ({ response: reviewJson() }));
+    const env = createTestEnv({ AI: { run } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000", AI_MAX_OUTPUT_TOKENS: "not-a-number" });
+    const result = await runGittensoryAiReview(env, baseInput);
+    expect(result.status).toBe("ok"); // NaN → clamped to the 256 floor, review still runs
+  });
+
+  it("does NOT count a BYOK advisory against the free neuron budget (it bills the maintainer)", async () => {
+    const fetchMock = vi.fn(async () => new Response(JSON.stringify({ content: [{ type: "text", text: reviewJson({ assessment: "BYOK advisory." }) }] }), { status: 200 }));
+    vi.stubGlobal("fetch", fetchMock);
+    const run = vi.fn();
+    // Free budget is exhausted (1 neuron), but a BYOK advisory bills the maintainer's account, so it still runs.
+    const env = createTestEnv({ AI: { run } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "1" });
+    const result = await runGittensoryAiReview(env, { ...baseInput, providerKey: { provider: "anthropic", key: "sk-ant-secret" } });
+    expect(result.status).toBe("ok");
+    expect(result.status === "ok" && result.advisoryNotes).toContain("BYOK advisory.");
+    expect(result.status === "ok" && result.estimatedNeurons).toBe(0); // advisory-only BYOK consumes no free budget
+    expect(fetchMock).toHaveBeenCalled();
+    expect(run).not.toHaveBeenCalled();
+  });
 });
 
 describe("runGittensoryAiReview advisory mode", () => {