diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 09b5dd274b..81b44a5042 100644 --- a/src/services/ai-review.ts +++ b/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.`, diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 1d5b083117..fdbd44c0fe 100644 --- a/test/unit/ai-review.test.ts +++ b/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"); + }); +});