feat(review): add a performance-regression instruction to the AI review system prompt

src/services/ai-review.ts

@@ -54,13 +54,14 @@ const REVIEW_SYSTEM_PROMPT = [
   '{"assessment": string, "blockers": string[], "nits": string[], "suggestions": string[], "confidence": number}',
   "- assessment: a substantive but CONCISE summary (2-4 sentences) — what the change does, whether it is correct, and the most notable detail. Specific to THIS diff; never a generic one-liner and never hedging ('appears to', 'seems to').",
   "The assessment field is REQUIRED and must never be empty; if blockers is [] then the assessment still summarizes why the visible diff is safe enough to proceed.",
-  "- blockers: each ONE sentence naming a defect that WILL break the code as written — a missing import/symbol (ReferenceError), a logic error that produces wrong output, a security hole, data loss, a build/test breakage, or an API/contract break. Reference the file (and function/line). Empty [] if there are genuinely none.",
+  "- blockers: each ONE sentence naming a defect that WILL break the code as written — a missing import/symbol (ReferenceError), a logic error that produces wrong output, a security hole, data loss, a build/test breakage, an API/contract break, or a genuine algorithmic-complexity/performance regression introduced by the diff (e.g. a DB query or network call moved inside a loop creating an N+1 pattern, an unbounded loop/fanout over input whose size is not capped). Reference the file (and function/line). Empty [] if there are genuinely none.",
   "- confidence: a single number in [0,1] — your CALIBRATED probability that the blockers above are REAL, must-fix defects (not false positives). Use 1.0 only when you are certain the diff itself breaks; use 0.5 for a genuine coin-flip; lower it when you cannot fully see the breaking code or the defect is speculative. When blockers is empty, set confidence to 1.0.",
   "- nits: each ONE sentence — a NON-blocking point: style, naming, a missing doc, or DEFENSIVE hardening ('should handle the empty case', 'consider catching errors', 'add validation'). File-reference where you can.",
   "- suggestions: a few concrete, file-referenced improvements (may overlap nits).",
   "BE SELECTIVE — report only the findings that genuinely matter. List at MOST ~3 blockers and ~5 nits, keeping only the most important; prefer signal over volume and do NOT pad the lists.",
   "DEDUPLICATE — if the same kind of issue recurs across several functions or lines, report it ONCE and note it applies broadly; never repeat a near-identical finding per occurrence.",
   "SEVERITY DISCIPLINE — defensive or speculative hardening ('should handle X', 'consider validating', 'add error handling') is a NIT, not a blocker, UNLESS a real input WILL actually trigger the failure. CI or check status itself (failing, pending, unverified) is NOT a code defect — never list it (the gate evaluates CI separately).",
+  "PERFORMANCE SEVERITY — a performance concern is a blocker ONLY when the diff introduces a genuine, visible regression with a concrete trigger (a DB query or network call moved inside a loop, a loop/fanout over input whose size the diff removed a bound on). A stylistic or micro-optimization preference ('could use a Map instead of an array', 'this could be slightly faster') is a NIT, not a blocker, even if real.",
   "DIFF SCOPE — the diff shows only CHANGED lines, NOT whole files. A function, variable, import, type, or symbol you do not SEE may already be defined or imported elsewhere in the same file/module. NEVER report a 'missing import', 'undefined/not-imported symbol', or 'X is not defined -> ReferenceError' as a blocker unless the diff ITSELF removes the definition or introduces the symbol without defining it anywhere shown. When you cannot confirm a symbol is missing from the visible diff, it is NOT a blocker — at most a nit ('verify X is imported/defined').",
   "TRACE BEFORE ASSERTING ABSENCE — this rule extends to ANY 'X is missing' blocker (a missing schema/annotation/field, a missing null/array/type guard, a missing await/error-handler, an unregistered route/tool/handler): a backfill loop, a default, an early guard, or a registration ELSEWHERE may already supply it. Before calling absence a blocker, find the line in the visible context that WOULD break and reference it; if you cannot SEE the breaking code, downgrade to a nit phrased as a verification ('confirm X is registered/guarded'), never a blocker.",
   `FAIL CLOSED ON AN INCOHERENT DIFF — if the diff does not cohere with the PR title/description (it appears to describe a DIFFERENT change, the changed-file set looks stale or wrong, or you cannot map it to one coherent change), DO NOT emit a confident assessment or approval: set assessment to exactly '${INCOHERENT_DIFF_ASSESSMENT}' and return empty blockers, nits, and suggestions. Never rubber-stamp a change you cannot actually see.`,

test/unit/ai-review.test.ts

@@ -1785,3 +1785,33 @@ describe("pure helpers", () => {
     expect(system).toContain("untrusted advisory context");
   });
 });
+
+describe("REVIEW_SYSTEM_PROMPT performance-regression instruction (#2559)", () => {
+  it("instructs the model to treat a genuine algorithmic/performance regression as a blocker category", async () => {
+    const run = vi.fn(
+      async (
+        _model: string,
+        _options: { messages: Array<{ content: string }> },
+      ) => ({ 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",
+    });
+    const result = await runGittensoryAiReview(env, baseInput);
+    expect(result.status).toBe("ok");
+    const opts = run.mock.calls[0]?.[1] as {
+      messages: Array<{ role?: string; content: string }>;
+    };
+    const system =
+      opts.messages.find((m) => m.role === "system")?.content ??
+      String(opts.messages[0]?.content);
+    expect(system).toContain("N+1");
+    expect(system).toContain("unbounded loop/fanout");
+    expect(system).toContain("PERFORMANCE SEVERITY");
+    // Severity discipline: a micro-optimization/style preference must still be steered toward a nit, not a blocker.
+    expect(system).toContain("micro-optimization preference");
+  });
+});