From 353127c2dbfbd89a99adc65054fa5eb84f3a7bd8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 04:40:40 -0700 Subject: [PATCH 1/5] =?UTF-8?q?feat(gate):=20choose=20provider,=20model,?= =?UTF-8?q?=20effort=20and=20self-consistency=20runs=20=E2=80=94=20per=20r?= =?UTF-8?q?epo,=20and=20escalated=20on=20guarded=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Effort DID already exist (CLAUDE_AI_EFFORT -> `claude --effort`, CODEX_AI_EFFORT -> model_reasoning_effort) but only as a GLOBAL env var, while provider and model were already per-repo manifest fields. Self-consistency runs had the same asymmetry. So the two knobs that most directly control review depth were the two a repo could not set. Close that, and use it where it matters most. Adds: gate.aiReview.effort / .selfConsistencyRuns — per-repo parity with the env vars gate.guardrailEscalation.{provider,model,effort,selfConsistencyRuns} — used INSTEAD when a PR touches a hardGuardrailGlobs path Precedence resolves per field independently: escalation > per-repo > global. An escalation block that sets only `effort` still inherits the repo's model and the global provider -- the common ask is "same model, think harder", not "swap the model out". WHY: a guardrail hit used to change exactly one thing -- it suppressed auto-merge and queued a human. It bought no extra analysis at all: a PR touching .github/workflows/** got the same single-pass, same-model, same-effort review as a README typo. Measured on the production ORB that was 74 distinct PRs held in 14 days, on repos whose guardrail lists had already been narrowed twice. A guarded path can now buy MORE SCRUTINY instead of maintainer toil; manual review stays the fallback when the escalated review is not clean. Every layer unset ⇒ null throughout ⇒ byte-identical to today for every existing repo and self-hoster. An escalation is logged (ai_review_guardrail_escalated) and describeReviewEscalation renders the one-line "why" for the panel/decision record. --- .loopover.yml.example | 28 ++++++ apps/loopover-ui/public/openapi.json | 45 +++++++++ packages/loopover-contract/src/api-schemas.ts | 7 ++ .../loopover-engine/src/focus-manifest.ts | 63 +++++++++++++ .../src/types/manifest-deps-types.ts | 13 +++ scripts/check-docs-drift.ts | 8 ++ src/openapi/schemas.ts | 6 ++ src/queue/ai-review-orchestration.ts | 36 +++++++ src/review/review-knobs.ts | 93 +++++++++++++++++++ src/services/ai-review.ts | 14 ++- src/signals/focus-manifest.ts | 6 ++ src/types.ts | 13 +++ test/unit/focus-manifest.test.ts | 16 +++- test/unit/review-knobs.test.ts | 86 +++++++++++++++++ 14 files changed, 431 insertions(+), 3 deletions(-) create mode 100644 src/review/review-knobs.ts create mode 100644 test/unit/review-knobs.test.ts diff --git a/.loopover.yml.example b/.loopover.yml.example index a32890c11..15d1ab316 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -272,6 +272,24 @@ gate: # app(s) YOU run; loopover hardcodes no vendor. List of { name, appSlug }, or omit. Default: not # configured (byte-identical behavior for every repo that doesn't opt in). Config-as-code only — no DB # column or dashboard toggle. + # Review settings used INSTEAD of the repo defaults when a PR touches a `hardGuardrailGlobs` path + # (#9808/#9821) — so a guarded path buys MORE SCRUTINY rather than an automatic manual-review hold. + # + # Without this, a guardrail hit changes exactly one thing: it suppresses auto-merge and queues a human. The + # review itself is identical to any other file's — same model, same effort, single pass. That turns a + # protected path into maintainer toil without the analysis actually going up, and on a busy repo it is the + # single largest source of manual reviews. + # + # Any subset may be set; each unset field falls through to the repo value, then the global env. The common + # shape is "same model, think harder" — set only `effort` (and optionally `selfConsistencyRuns`) and the + # model/provider are inherited untouched. Manual review remains the fallback: if the escalated review is + # not clean, the PR still holds. + guardrailEscalation: + effort: high + selfConsistencyRuns: 3 + # provider: anthropic + # model: claude-opus-5 + advisoryCheckRuns: - name: Contributor trust appSlug: example-security-app @@ -426,6 +444,16 @@ gate: # String or null. Default: null (the key record's model, else a conservative # per-provider default). model: null + # Reasoning effort for the review pass: low | medium | high | xhigh | max (#9821). Per-repo parity with + # the global CLAUDE_AI_EFFORT / CODEX_AI_EFFORT env vars, which used to be the ONLY place this could be + # set even though provider and model were already per-repo. Unset ⇒ whatever the env configures + # (medium by default). Higher effort costs more tokens and runs longer — raise it for repos where a + # missed defect is expensive, not everywhere. + effort: medium + # TOTAL evaluations per review, primary included (#9821). Per-repo parity with + # AI_REVIEW_SELF_CONSISTENCY_RUNS. Clamped to {0, 2, 3}: one run cannot measure agreement, and the + # benefit saturates by three. 0 (default) = a single pass. Each extra run is a full paid review. + selfConsistencyRuns: 0 # Minimum calibrated AI-reviewer confidence (0-1). Under `mode: block`, # consensus and split AI-review defects still BLOCK the gate regardless of # this floor — what varies below it is `lowConfidenceDisposition` below. diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 8f9564562..0c647fe37 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -10146,6 +10146,51 @@ "appSlug" ] } + }, + "aiReviewEffort": { + "type": "string", + "nullable": true, + "enum": [ + "low", + "medium", + "high", + "xhigh", + "max", + null + ] + }, + "aiReviewSelfConsistencyRuns": { + "type": "number", + "nullable": true + }, + "guardrailEscalationProvider": { + "type": "string", + "nullable": true, + "enum": [ + "anthropic", + "openai", + null + ] + }, + "guardrailEscalationModel": { + "type": "string", + "nullable": true + }, + "guardrailEscalationEffort": { + "type": "string", + "nullable": true, + "enum": [ + "low", + "medium", + "high", + "xhigh", + "max", + null + ] + }, + "guardrailEscalationSelfConsistencyRuns": { + "type": "number", + "nullable": true } }, "required": [ diff --git a/packages/loopover-contract/src/api-schemas.ts b/packages/loopover-contract/src/api-schemas.ts index bce04d934..d92b26f14 100644 --- a/packages/loopover-contract/src/api-schemas.ts +++ b/packages/loopover-contract/src/api-schemas.ts @@ -452,6 +452,13 @@ export const RepositorySettingsSchema = z // the compile-time parity assertion in test/unit/openapi-settings-schema-parity.test.ts. expectedCiContexts: z.array(z.string()).readonly().nullable().optional(), advisoryCheckRuns: z.array(z.object({ name: z.string(), appSlug: z.string() })).readonly().nullable().optional(), + ignoredCheckRuns: z.array(z.object({ name: z.string(), appSlug: z.string() })).readonly().nullable().optional(), + aiReviewEffort: z.enum(["low", "medium", "high", "xhigh", "max"]).nullable().optional(), + aiReviewSelfConsistencyRuns: z.number().nullable().optional(), + guardrailEscalationProvider: z.enum(["anthropic", "openai"]).nullable().optional(), + guardrailEscalationModel: z.string().nullable().optional(), + guardrailEscalationEffort: z.enum(["low", "medium", "high", "xhigh", "max"]).nullable().optional(), + guardrailEscalationSelfConsistencyRuns: z.number().nullable().optional(), copycatGateMode: z.enum(["off", "warn", "label", "block"]).optional(), copycatGateMinScore: z.number().nullable().optional(), gateDryRun: z.boolean().optional(), diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index f01546976..6f91587bd 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -114,6 +114,22 @@ export type FocusManifestGateConfig = { aiReviewByok: boolean | null; aiReviewProvider: "anthropic" | "openai" | null; aiReviewModel: string | null; + /** `gate.aiReview.effort` (#9821): reasoning effort for the review pass — low | medium | high | xhigh | max. + * Per-repo parity with the global CLAUDE_AI_EFFORT/CODEX_AI_EFFORT env vars, which were previously the ONLY + * way to set it: provider and model were already per-repo, effort was not. Unset ⇒ the env default. */ + aiReviewEffort: "low" | "medium" | "high" | "xhigh" | "max" | null; + /** `gate.aiReview.selfConsistencyRuns` (#9821): TOTAL evaluations per review, primary included. Per-repo + * parity with AI_REVIEW_SELF_CONSISTENCY_RUNS. Clamped downstream to {0,2,3} -- one run cannot measure + * agreement and the benefit saturates by three. Unset ⇒ the env default. */ + aiReviewSelfConsistencyRuns: number | null; + /** `gate.guardrailEscalation` (#9808/#9821): the review settings to use INSTEAD of the repo defaults when a + * PR touches a `hardGuardrailGlobs` path. Any subset may be given; each unset field falls through to the + * repo/global value. This is what makes a guarded path buy MORE SCRUTINY rather than a manual hold -- + * today a guardrail hit reviews identically to any other file and merely stops the merge. */ + guardrailEscalationProvider: "anthropic" | "openai" | null; + guardrailEscalationModel: string | null; + guardrailEscalationEffort: "low" | "medium" | "high" | "xhigh" | "max" | null; + guardrailEscalationSelfConsistencyRuns: number | null; aiReviewAllAuthors: boolean | null; /** `gate.aiReview.closeConfidence` (#7): minimum calibrated AI-reviewer confidence (0-1) for an AI defect to BLOCK * under `aiReview.mode: block`. null (unset) ⇒ the gate's 0.93 default. Clamped to [0,1] at parse time. */ @@ -631,6 +647,12 @@ export type FocusManifestSettings = Partial< | "aiReviewByok" | "aiReviewProvider" | "aiReviewModel" + | "aiReviewEffort" + | "aiReviewSelfConsistencyRuns" + | "guardrailEscalationProvider" + | "guardrailEscalationModel" + | "guardrailEscalationEffort" + | "guardrailEscalationSelfConsistencyRuns" | "aiReviewAllAuthors" | "aiReviewConfirmedContributorsOnly" | "closeOwnerAuthors" @@ -1342,6 +1364,12 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, + aiReviewEffort: null, + aiReviewSelfConsistencyRuns: null, + guardrailEscalationProvider: null, + guardrailEscalationModel: null, + guardrailEscalationEffort: null, + guardrailEscalationSelfConsistencyRuns: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, @@ -1815,6 +1843,7 @@ const GATE_TOP_LEVEL_KEYS = new Set([ "expectedCiContexts", "advisoryCheckRuns", "ignoredCheckRuns", + "guardrailEscalation", "aiJudgmentBlockers", "copycat", ]); @@ -1837,6 +1866,10 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu } const aiReview = record.aiReview; const aiReviewRecord = aiReview !== null && typeof aiReview === "object" && !Array.isArray(aiReview) ? (aiReview as Record) : undefined; + // #9821: `gate.guardrailEscalation` — same "nested mapping, each field optional" shape as gate.aiReview above. + const escalation = record.guardrailEscalation; + const escalationRecord = + escalation !== null && typeof escalation === "object" && !Array.isArray(escalation) ? (escalation as Record) : undefined; if (aiReview !== undefined && aiReview !== null && aiReviewRecord === undefined) { warnings.push(`Manifest gate field "gate.aiReview" must be a mapping; ignoring it.`); } @@ -1881,6 +1914,12 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu aiReviewByok: normalizeOptionalBoolean(aiReviewRecord?.byok, "gate.aiReview.byok", warnings), aiReviewProvider: normalizeOptionalEnum(aiReviewRecord?.provider, "gate.aiReview.provider", ["anthropic", "openai"] as const, warnings), aiReviewModel: normalizeOptionalString(aiReviewRecord?.model, "gate.aiReview.model", warnings), + aiReviewEffort: normalizeOptionalEnum(aiReviewRecord?.effort, "gate.aiReview.effort", ["low", "medium", "high", "xhigh", "max"] as const, warnings), + aiReviewSelfConsistencyRuns: normalizeOptionalNonNegativeInt(aiReviewRecord?.selfConsistencyRuns, "gate.aiReview.selfConsistencyRuns", warnings), + guardrailEscalationProvider: normalizeOptionalEnum(escalationRecord?.provider, "gate.guardrailEscalation.provider", ["anthropic", "openai"] as const, warnings), + guardrailEscalationModel: normalizeOptionalString(escalationRecord?.model, "gate.guardrailEscalation.model", warnings), + guardrailEscalationEffort: normalizeOptionalEnum(escalationRecord?.effort, "gate.guardrailEscalation.effort", ["low", "medium", "high", "xhigh", "max"] as const, warnings), + guardrailEscalationSelfConsistencyRuns: normalizeOptionalNonNegativeInt(escalationRecord?.selfConsistencyRuns, "gate.guardrailEscalation.selfConsistencyRuns", warnings), aiReviewAllAuthors: normalizeOptionalBoolean(aiReviewRecord?.allAuthors, "gate.aiReview.allAuthors", warnings), aiReviewCloseConfidence: normalizeOptionalConfidence(aiReviewRecord?.closeConfidence, "gate.aiReview.closeConfidence", warnings), aiReviewSalvageabilityMinScore: normalizeOptionalScore(aiReviewRecord?.salvageabilityMinScore, "gate.aiReview.salvageabilityMinScore", warnings), @@ -1947,6 +1986,8 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.aiReviewByok !== null || gate.aiReviewProvider !== null || gate.aiReviewModel !== null || + gate.aiReviewEffort !== null || + gate.aiReviewSelfConsistencyRuns !== null || gate.aiReviewAllAuthors !== null || gate.aiReviewCloseConfidence !== null || gate.aiReviewSalvageabilityMinScore !== null || @@ -1970,6 +2011,12 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.claCheckRunAppSlug !== null || gate.expectedCiContexts !== null || gate.advisoryCheckRuns !== null || + gate.aiReviewEffort !== null || + gate.aiReviewSelfConsistencyRuns !== null || + gate.guardrailEscalationProvider !== null || + gate.guardrailEscalationModel !== null || + gate.guardrailEscalationEffort !== null || + gate.guardrailEscalationSelfConsistencyRuns !== null || gate.ignoredCheckRuns !== null || gate.aiJudgmentBlockersMode !== null || gate.copycatMode !== null || @@ -2036,6 +2083,8 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { if (gate.aiReviewByok !== null) aiReview.byok = gate.aiReviewByok; if (gate.aiReviewProvider !== null) aiReview.provider = gate.aiReviewProvider; if (gate.aiReviewModel !== null) aiReview.model = gate.aiReviewModel; + if (gate.aiReviewEffort !== null) aiReview.effort = gate.aiReviewEffort; + if (gate.aiReviewSelfConsistencyRuns !== null) aiReview.selfConsistencyRuns = gate.aiReviewSelfConsistencyRuns; if (gate.aiReviewAllAuthors !== null) aiReview.allAuthors = gate.aiReviewAllAuthors; if (gate.aiReviewCloseConfidence !== null) aiReview.closeConfidence = gate.aiReviewCloseConfidence; if (gate.aiReviewSalvageabilityMinScore !== null) aiReview.salvageabilityMinScore = gate.aiReviewSalvageabilityMinScore; @@ -2049,6 +2098,20 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { } out.aiReview = aiReview; } + // #9821: guardrail escalation round-trips as its own nested mapping, emitted only when something is set. + if ( + gate.guardrailEscalationProvider !== null || + gate.guardrailEscalationModel !== null || + gate.guardrailEscalationEffort !== null || + gate.guardrailEscalationSelfConsistencyRuns !== null + ) { + const escalation: Record = {}; + if (gate.guardrailEscalationProvider !== null) escalation.provider = gate.guardrailEscalationProvider; + if (gate.guardrailEscalationModel !== null) escalation.model = gate.guardrailEscalationModel; + if (gate.guardrailEscalationEffort !== null) escalation.effort = gate.guardrailEscalationEffort; + if (gate.guardrailEscalationSelfConsistencyRuns !== null) escalation.selfConsistencyRuns = gate.guardrailEscalationSelfConsistencyRuns; + out.guardrailEscalation = escalation; + } if (gate.mergeReadiness !== null) out.mergeReadiness = gate.mergeReadiness; if (gate.manifestPolicy !== null) out.manifestPolicy = gate.manifestPolicy; if (gate.selfAuthoredLinkedIssue !== null) out.selfAuthoredLinkedIssue = gate.selfAuthoredLinkedIssue; diff --git a/packages/loopover-engine/src/types/manifest-deps-types.ts b/packages/loopover-engine/src/types/manifest-deps-types.ts index 0c270443c..d4a55249e 100644 --- a/packages/loopover-engine/src/types/manifest-deps-types.ts +++ b/packages/loopover-engine/src/types/manifest-deps-types.ts @@ -296,6 +296,19 @@ export type RepositorySettings = { /** Config-as-code model override for the BYOK advisory write-up (e.g. "claude-3-5-sonnet-latest"). * `null` = use the key record's model, else a conservative per-provider default. */ aiReviewModel?: string | null | undefined; + /** `gate.aiReview.effort` (#9821): reasoning effort for the review pass (low | medium | high | xhigh | max). + * Per-repo parity with the global CLAUDE_AI_EFFORT / CODEX_AI_EFFORT env vars. Unset ⇒ the env default. */ + aiReviewEffort?: "low" | "medium" | "high" | "xhigh" | "max" | null | undefined; + /** `gate.aiReview.selfConsistencyRuns` (#9821): TOTAL evaluations per review, primary included. Per-repo + * parity with AI_REVIEW_SELF_CONSISTENCY_RUNS; clamped downstream to {0,2,3}. Unset ⇒ the env default. */ + aiReviewSelfConsistencyRuns?: number | null | undefined; + /** `gate.guardrailEscalation.*` (#9808/#9821): review settings used INSTEAD of the repo defaults when a PR + * touches a hardGuardrailGlobs path, so a guarded path buys MORE SCRUTINY rather than a manual hold. Each + * field falls through to the repo/global value when unset. */ + guardrailEscalationProvider?: "anthropic" | "openai" | null | undefined; + guardrailEscalationModel?: string | null | undefined; + guardrailEscalationEffort?: "low" | "medium" | "high" | "xhigh" | "max" | null | undefined; + guardrailEscalationSelfConsistencyRuns?: number | null | undefined; /** Review EVERY PR's author, not only confirmed Gittensor contributors. Only meaningful when * {@link aiReviewConfirmedContributorsOnly} is also `true` (that field opts INTO confirmed-only * scoping in the first place — see its own doc comment for the full invariant: AI review runs for diff --git a/scripts/check-docs-drift.ts b/scripts/check-docs-drift.ts index 05c456485..c0503ab33 100644 --- a/scripts/check-docs-drift.ts +++ b/scripts/check-docs-drift.ts @@ -174,6 +174,14 @@ export const SETTINGS_ALIAS_MANIFEST: AliasManifestRow[] = [ { field: "aiReviewByok", aliases: ["byok"] }, { field: "aiReviewProvider", aliases: ["aiReview:"] }, { field: "aiReviewModel", aliases: ["aiReview:"] }, + // #9821: nested under the same `aiReview:` mapping as provider/model above. + { field: "aiReviewEffort", aliases: ["aiReview:"] }, + { field: "aiReviewSelfConsistencyRuns", aliases: ["selfConsistencyRuns"] }, + // #9808/#9821: the guardrail-escalation block is its own nested mapping; every field documents under it. + { field: "guardrailEscalationProvider", aliases: ["guardrailEscalation:"] }, + { field: "guardrailEscalationModel", aliases: ["guardrailEscalation:"] }, + { field: "guardrailEscalationEffort", aliases: ["guardrailEscalation:"] }, + { field: "guardrailEscalationSelfConsistencyRuns", aliases: ["guardrailEscalation:"] }, { field: "aiReviewAllAuthors", aliases: ["allAuthors"] }, { field: "aiReviewCloseConfidence", aliases: ["closeConfidence"] }, { field: "aiReviewSalvageabilityMinScore", aliases: ["salvageabilityMinScore"] }, diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index b50132276..a6e565eba 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -748,6 +748,12 @@ export const RepositorySettingsSchema = z expectedCiContexts: z.array(z.string()).readonly().nullable().optional(), advisoryCheckRuns: z.array(z.object({ name: z.string(), appSlug: z.string() })).readonly().nullable().optional(), ignoredCheckRuns: z.array(z.object({ name: z.string(), appSlug: z.string() })).readonly().nullable().optional(), + aiReviewEffort: z.enum(["low", "medium", "high", "xhigh", "max"]).nullable().optional(), + aiReviewSelfConsistencyRuns: z.number().nullable().optional(), + guardrailEscalationProvider: z.enum(["anthropic", "openai"]).nullable().optional(), + guardrailEscalationModel: z.string().nullable().optional(), + guardrailEscalationEffort: z.enum(["low", "medium", "high", "xhigh", "max"]).nullable().optional(), + guardrailEscalationSelfConsistencyRuns: z.number().nullable().optional(), copycatGateMode: z.enum(["off", "warn", "label", "block"]).optional(), copycatGateMinScore: z.number().nullable().optional(), gateDryRun: z.boolean().optional(), diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 5d2d2dd97..a2287c017 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -14,6 +14,8 @@ // (staying) and this file's own runAiReviewForAdvisory, so processors.ts imports it back rather than this // file importing it from processors.ts, keeping the dependency one-directional. +import { describeReviewEscalation, resolveReviewKnobs } from "../review/review-knobs"; +import { isGuardrailHit } from "../signals/change-guardrail"; import { claimTransientLock, releaseTransientLockIfOwner, @@ -833,7 +835,41 @@ export async function runAiReviewForAdvisory( args.repoFullName, args.reviewInlineComments, ); + // #9808/#9821: resolve the effective review knobs for THIS PR before invoking. A PR touching a + // `hardGuardrailGlobs` path escalates -- higher reasoning effort, more independent runs, optionally a + // different model/provider -- instead of reviewing identically to every other file and merely being held + // for a human afterwards. isGuardrailHit is the SHARED matcher (src/signals/change-guardrail.ts), never a + // second copy. Every layer unset ⇒ null throughout ⇒ byte-identical to the previous behavior. + const reviewKnobs = resolveReviewKnobs({ + guardrailHit: isGuardrailHit(files.map((file) => file.path), args.settings.hardGuardrailGlobs ?? []), + escalation: { + provider: args.settings.guardrailEscalationProvider ?? null, + model: args.settings.guardrailEscalationModel ?? null, + effort: args.settings.guardrailEscalationEffort ?? null, + selfConsistencyRuns: args.settings.guardrailEscalationSelfConsistencyRuns ?? null, + }, + repo: { + provider: args.settings.aiReviewProvider ?? null, + model: args.settings.aiReviewModel ?? null, + effort: args.settings.aiReviewEffort ?? null, + selfConsistencyRuns: args.settings.aiReviewSelfConsistencyRuns ?? null, + }, + }); + const escalationNote = describeReviewEscalation(reviewKnobs); + if (escalationNote) { + console.log( + JSON.stringify({ + event: "ai_review_guardrail_escalated", + repo: args.repoFullName, + pr: args.pr.number, + fields: reviewKnobs.escalatedFields, + effort: reviewKnobs.effort, + selfConsistencyRuns: reviewKnobs.selfConsistencyRuns, + }), + ); + } const result = await runLoopOverAiReview(env, { + reviewKnobs, repoFullName: args.repoFullName, prNumber: args.pr.number, title: args.pr.title, diff --git a/src/review/review-knobs.ts b/src/review/review-knobs.ts new file mode 100644 index 000000000..2ec981966 --- /dev/null +++ b/src/review/review-knobs.ts @@ -0,0 +1,93 @@ +// The effective AI-review knobs for one PR — provider, model, reasoning effort, self-consistency runs +// (#9808/#9821). +// +// (Named "knobs", not "effort", because src/review/review-effort.ts already owns a different meaning of that +// word: the deterministic estimate of how many MINUTES a HUMAN should budget. The `effort` field here is the +// model's own reasoning effort — `claude --effort` / Codex `model_reasoning_effort`.) +// +// WHY THIS EXISTS: a `hardGuardrailGlobs` hit used to change exactly one thing — it suppressed auto-merge and +// held the PR for a human. It bought no extra analysis whatsoever: a PR touching `.github/workflows/**` got the +// same single-pass, same-model, same-effort review as a README typo. Measured on the production ORB that was 74 +// distinct PRs held in 14 days, on repos whose guardrail lists had already been narrowed twice. Automation was +// being replaced by a maintainer queue without the scrutiny actually going up. +// +// A guardrail hit can now ESCALATE instead: higher reasoning effort, more independent runs, optionally a +// different model or provider. Manual review remains the fallback for genuine uncertainty — the disposition +// still holds when the escalated review is not clean — but it is no longer the automatic price of touching a +// protected path. +// +// PRECEDENCE, most specific wins: +// guardrail escalation (ONLY when the PR hits a guarded path) +// -> per-repo `gate.aiReview.*` (.loopover.yml) +// -> global env (CLAUDE_AI_EFFORT / AI_REVIEW_SELF_CONSISTENCY_RUNS / CLAUDE_AI_MODEL / AI_PROVIDER) +// +// Each field resolves INDEPENDENTLY: an escalation block that sets only `effort` still inherits the repo's +// model and the global provider. That matters because the common request is "same model, think harder" — a +// maintainer raising scrutiny usually does not want to change which model reviews their code. + +/** The four knobs a maintainer can choose at any layer. `null`/absent ⇒ inherit the next layer down. */ +export type ReviewKnobs = { + provider: string | null; + model: string | null; + effort: string | null; + selfConsistencyRuns: number | null; +}; + +export type ResolvedReviewKnobs = ReviewKnobs & { + /** True when the escalation layer supplied at least one value — i.e. this PR is reviewed MORE heavily + * because it touches a protected path. Surfaced so the panel and the decision record can say so: + * "escalated review on a guarded path: effort=high, 3 runs" is what makes the gate legible to a + * contributor, and what makes an auto-merge on a guarded path defensible in the audit trail. */ + escalated: boolean; + /** Exactly which fields the escalation layer changed, for that same surfacing. Empty when not escalated. */ + escalatedFields: string[]; +}; + +const KNOB_ORDER = ["provider", "model", "effort", "selfConsistencyRuns"] as const; + +/** + * PURE: resolve the effective knobs for one review. + * + * `guardrailHit` is the caller's own `isGuardrailHit(changedPaths, hardGuardrailGlobs)` result. This module + * deliberately does NOT re-derive it, so there is exactly one definition of "does this PR touch a guarded + * path" (src/signals/change-guardrail.ts, shared with the engine) rather than a second one free to drift. + */ +export function resolveReviewKnobs(args: { + guardrailHit: boolean; + escalation?: Partial | undefined; + repo?: Partial | undefined; + global?: Partial | undefined; +}): ResolvedReviewKnobs { + const escalatedFields: string[] = []; + const resolved: ReviewKnobs = { provider: null, model: null, effort: null, selfConsistencyRuns: null }; + + // Fixed order so escalatedFields reads in a stable, reportable sequence rather than in whatever order a + // consumer happens to destructure. + for (const field of KNOB_ORDER) { + const escalated = args.guardrailHit ? args.escalation?.[field] : null; + if (escalated !== null && escalated !== undefined) { + escalatedFields.push(field); + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- one assignment across a union of field types + (resolved as any)[field] = escalated; + continue; + } + const repo = args.repo?.[field]; + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- as above + (resolved as any)[field] = repo !== null && repo !== undefined ? repo : (args.global?.[field] ?? null); + } + + return { ...resolved, escalated: escalatedFields.length > 0, escalatedFields }; +} + +/** Human-readable summary of an escalation for the panel and the decision record. Null when not escalated, so + * a caller can fold it into a surface without a branch of its own. */ +export function describeReviewEscalation(resolved: ResolvedReviewKnobs): string | null { + const parts: string[] = []; + if (resolved.escalatedFields.includes("provider") && resolved.provider) parts.push(`provider=${resolved.provider}`); + if (resolved.escalatedFields.includes("model") && resolved.model) parts.push(`model=${resolved.model}`); + if (resolved.escalatedFields.includes("effort") && resolved.effort) parts.push(`effort=${resolved.effort}`); + if (resolved.escalatedFields.includes("selfConsistencyRuns") && resolved.selfConsistencyRuns !== null) { + parts.push(`${resolved.selfConsistencyRuns} runs`); + } + return parts.length > 0 ? `escalated review on a guarded path: ${parts.join(", ")}` : null; +} diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 36f5d3ba4..06a861ccd 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -219,6 +219,11 @@ export function resolveEffectiveAiReviewPlan( } export type LoopOverAiReviewInput = { + /** #9808/#9821: the effective review knobs the caller resolved for THIS PR (resolveReviewKnobs) -- already + * layered escalation > per-repo > global, so this module never re-derives precedence. Absent ⇒ the env + * defaults exactly as before. `selfConsistencyRuns` here outranks AI_REVIEW_SELF_CONSISTENCY_RUNS; the + * model/effort/provider fields ride through to the provider invocation. */ + reviewKnobs?: { provider?: string | null; model?: string | null; effort?: string | null; selfConsistencyRuns?: number | null } | undefined; repoFullName: string; prNumber: number; title: string; @@ -3279,7 +3284,14 @@ export async function runLoopOverAiReview( // review that produced no usable verdict would spend real money measuring nothing. const selfConsistencySamples: { reviewer: string; votedFail: boolean }[] = []; let selfConsistencyDegraded = false; - const configuredSelfConsistencyRuns = resolveSelfConsistencyRuns(env.AI_REVIEW_SELF_CONSISTENCY_RUNS); + // #9821: a caller-resolved value (per-repo, or escalated because this PR touched a guarded path) outranks + // the global env var. `?? undefined` not `|| undefined`: 0 is a real choice ("off"), and truthiness here + // would silently fall back to the env value on a repo that deliberately turned extra runs off. + const configuredSelfConsistencyRuns = resolveSelfConsistencyRuns( + input.reviewKnobs?.selfConsistencyRuns !== null && input.reviewKnobs?.selfConsistencyRuns !== undefined + ? String(input.reviewKnobs.selfConsistencyRuns) + : env.AI_REVIEW_SELF_CONSISTENCY_RUNS, + ); if (configuredSelfConsistencyRuns >= 2 && reviewerVotes.length > 0) { const plan = planSelfConsistencyRuns({ configuredTotalRuns: configuredSelfConsistencyRuns, diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 833e52a04..6bdd700fb 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -510,6 +510,12 @@ function applyGateConfigOverrides(effective: RepositorySettings, gate: FocusMani if (gate.aiReviewByok !== null) effective.aiReviewByok = gate.aiReviewByok; if (gate.aiReviewProvider !== null) effective.aiReviewProvider = gate.aiReviewProvider; if (gate.aiReviewModel !== null) effective.aiReviewModel = gate.aiReviewModel; + if (gate.aiReviewEffort !== null) effective.aiReviewEffort = gate.aiReviewEffort; + if (gate.aiReviewSelfConsistencyRuns !== null) effective.aiReviewSelfConsistencyRuns = gate.aiReviewSelfConsistencyRuns; + if (gate.guardrailEscalationProvider !== null) effective.guardrailEscalationProvider = gate.guardrailEscalationProvider; + if (gate.guardrailEscalationModel !== null) effective.guardrailEscalationModel = gate.guardrailEscalationModel; + if (gate.guardrailEscalationEffort !== null) effective.guardrailEscalationEffort = gate.guardrailEscalationEffort; + if (gate.guardrailEscalationSelfConsistencyRuns !== null) effective.guardrailEscalationSelfConsistencyRuns = gate.guardrailEscalationSelfConsistencyRuns; if (gate.aiReviewAllAuthors !== null) effective.aiReviewAllAuthors = gate.aiReviewAllAuthors; if (gate.aiReviewCloseConfidence !== null) effective.aiReviewCloseConfidence = gate.aiReviewCloseConfidence; if (gate.aiReviewSalvageabilityMinScore !== null) effective.aiReviewSalvageabilityMinScore = gate.aiReviewSalvageabilityMinScore; diff --git a/src/types.ts b/src/types.ts index 5331a2846..2a980bbfd 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1117,6 +1117,19 @@ export type RepositorySettings = { /** Config-as-code model override for the BYOK advisory write-up (e.g. "claude-3-5-sonnet-latest"). * `null` = use the key record's model, else a conservative per-provider default. */ aiReviewModel?: string | null | undefined; + /** `gate.aiReview.effort` (#9821): reasoning effort for the review pass (low | medium | high | xhigh | max). + * Per-repo parity with the global CLAUDE_AI_EFFORT / CODEX_AI_EFFORT env vars. Unset ⇒ the env default. */ + aiReviewEffort?: "low" | "medium" | "high" | "xhigh" | "max" | null | undefined; + /** `gate.aiReview.selfConsistencyRuns` (#9821): TOTAL evaluations per review, primary included. Per-repo + * parity with AI_REVIEW_SELF_CONSISTENCY_RUNS; clamped downstream to {0,2,3}. Unset ⇒ the env default. */ + aiReviewSelfConsistencyRuns?: number | null | undefined; + /** `gate.guardrailEscalation.*` (#9808/#9821): review settings used INSTEAD of the repo defaults when a PR + * touches a hardGuardrailGlobs path, so a guarded path buys MORE SCRUTINY rather than a manual hold. Each + * field falls through to the repo/global value when unset. */ + guardrailEscalationProvider?: "anthropic" | "openai" | null | undefined; + guardrailEscalationModel?: string | null | undefined; + guardrailEscalationEffort?: "low" | "medium" | "high" | "xhigh" | "max" | null | undefined; + guardrailEscalationSelfConsistencyRuns?: number | null | undefined; /** Review EVERY PR's author, not only confirmed Gittensor contributors. Only meaningful when * {@link aiReviewConfirmedContributorsOnly} is also `true` (that field opts INTO confirmed-only * scoping in the first place — see its own doc comment for the full invariant: AI review runs for diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index acc35e294..17b8d7bda 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -341,6 +341,12 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { claCheckRunAppSlug: "checkRunAppSlug:", expectedCiContexts: "expectedCiContexts:", advisoryCheckRuns: "advisoryCheckRuns:", + aiReviewEffort: "effort:", + aiReviewSelfConsistencyRuns: "selfConsistencyRuns:", + guardrailEscalationProvider: "guardrailEscalation:", + guardrailEscalationModel: "guardrailEscalation:", + guardrailEscalationEffort: "guardrailEscalation:", + guardrailEscalationSelfConsistencyRuns: "guardrailEscalation:", ignoredCheckRuns: "ignoredCheckRuns:", aiJudgmentBlockersMode: "aiJudgmentBlockers:", copycatMode: "copycat:", @@ -433,6 +439,12 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { unlinkedIssueGuardrail: "unlinkedIssueGuardrail:", screenshotTableGate: "screenshotTableGate:", advisoryAiRouting: "advisoryAiRouting:", + aiReviewEffort: "effort:", + aiReviewSelfConsistencyRuns: "selfConsistencyRuns:", + guardrailEscalationProvider: "guardrailEscalation:", + guardrailEscalationModel: "guardrailEscalation:", + guardrailEscalationEffort: "guardrailEscalation:", + guardrailEscalationSelfConsistencyRuns: "guardrailEscalation:", } satisfies Record, string>; it.each(Object.entries(SETTINGS_FIELD_TOKENS))("documents settings.%s", (_field, token) => { @@ -999,7 +1011,7 @@ describe("compileFocusManifestPolicy", () => { issueDiscoveryPolicy: "neutral", maintainerNotes: [], publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], - gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, + gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, activeReviewReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { present: false, rag: null, reputation: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null, amsReputationBridge: null }, @@ -1193,7 +1205,7 @@ describe("parseFocusManifest gate config", () => { // the block→advisory deprecation-downgrade behavior itself is covered separately below. const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "advisory", minScore: 70 } } }); expect(m.present).toBe(true); - expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); + expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); }); it("parses gate.mergeReadiness, round-trips it, and warns on a bad value (#822)", () => { diff --git a/test/unit/review-knobs.test.ts b/test/unit/review-knobs.test.ts new file mode 100644 index 000000000..66249d708 --- /dev/null +++ b/test/unit/review-knobs.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from "vitest"; +import { describeReviewEscalation, resolveReviewKnobs } from "../../src/review/review-knobs"; + +// #9808/#9821: a hardGuardrailGlobs hit used to buy NO extra analysis — same model, same effort, single pass — +// it only suppressed auto-merge and queued a human. 74 distinct PRs held in 14 days on the production ORB. + +const GLOBAL = { provider: "claude-code", model: "claude-sonnet-5", effort: "medium", selfConsistencyRuns: 0 }; + +describe("resolveReviewKnobs", () => { + it("REGRESSION: a guarded path escalates instead of reviewing identically to any other file", () => { + const r = resolveReviewKnobs({ + guardrailHit: true, + escalation: { effort: "high", selfConsistencyRuns: 3 }, + repo: {}, + global: GLOBAL, + }); + expect(r.effort).toBe("high"); + expect(r.selfConsistencyRuns).toBe(3); + expect(r.escalated).toBe(true); + // The common ask: "same model, think harder" — model/provider inherit rather than being forced along. + expect(r.model).toBe("claude-sonnet-5"); + expect(r.provider).toBe("claude-code"); + }); + + it("INVARIANT: the escalation block is INERT when the PR touches no guarded path", () => { + const r = resolveReviewKnobs({ + guardrailHit: false, + escalation: { effort: "max", selfConsistencyRuns: 3 }, + repo: {}, + global: GLOBAL, + }); + expect(r.effort).toBe("medium"); + expect(r.selfConsistencyRuns).toBe(0); + expect(r.escalated).toBe(false); + expect(r.escalatedFields).toEqual([]); + }); + + it("precedence: escalation > per-repo > global, per field independently", () => { + const r = resolveReviewKnobs({ + guardrailHit: true, + escalation: { effort: "xhigh" }, + repo: { model: "repo-model", effort: "high", selfConsistencyRuns: 2 }, + global: GLOBAL, + }); + expect(r.effort).toBe("xhigh"); // escalation wins + expect(r.model).toBe("repo-model"); // repo wins over global + expect(r.selfConsistencyRuns).toBe(2); // repo, untouched by escalation + expect(r.provider).toBe("claude-code"); // global fallback + expect(r.escalatedFields).toEqual(["effort"]); + }); + + it("per-repo effort/runs apply with no guardrail involved — the parity the manifest was missing", () => { + const r = resolveReviewKnobs({ guardrailHit: false, repo: { effort: "high", selfConsistencyRuns: 3 }, global: GLOBAL }); + expect({ effort: r.effort, runs: r.selfConsistencyRuns, escalated: r.escalated }).toEqual({ effort: "high", runs: 3, escalated: false }); + }); + + it("INVARIANT: selfConsistencyRuns of 0 is a real value, not 'unset'", () => { + // 0 means "off". A truthiness check here would silently fall through to the global value and re-enable + // multi-run reviews on a repo that explicitly turned them off. + const r = resolveReviewKnobs({ guardrailHit: true, escalation: { selfConsistencyRuns: 0 }, global: { ...GLOBAL, selfConsistencyRuns: 3 } }); + expect(r.selfConsistencyRuns).toBe(0); + expect(r.escalated).toBe(true); + }); + + it("everything absent resolves to nulls rather than throwing", () => { + expect(resolveReviewKnobs({ guardrailHit: true })).toEqual({ + provider: null, model: null, effort: null, selfConsistencyRuns: null, escalated: false, escalatedFields: [], + }); + }); +}); + +describe("describeReviewEscalation", () => { + it("names exactly what changed, for the panel and the decision record", () => { + const r = resolveReviewKnobs({ guardrailHit: true, escalation: { effort: "high", selfConsistencyRuns: 3 }, global: GLOBAL }); + expect(describeReviewEscalation(r)).toBe("escalated review on a guarded path: effort=high, 3 runs"); + }); + + it("returns null when nothing was escalated, so a caller needs no branch of its own", () => { + expect(describeReviewEscalation(resolveReviewKnobs({ guardrailHit: false, global: GLOBAL }))).toBeNull(); + }); + + it("reports a model/provider escalation too", () => { + const r = resolveReviewKnobs({ guardrailHit: true, escalation: { provider: "anthropic", model: "claude-opus-5" }, global: GLOBAL }); + expect(describeReviewEscalation(r)).toBe("escalated review on a guarded path: provider=anthropic, model=claude-opus-5"); + }); +}); From b85763d9b4d89ffd3af0974141cd5fd469de2419 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 05:39:28 -0700 Subject: [PATCH 2/5] fix(gate): actually apply the resolved effort/model/provider at the invocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review blocker on #9821, and correct: resolveReviewKnobs was computed, logged, and then DROPPED. Only selfConsistencyRuns was consumed, so "choose provider, model, effort" was unimplemented at the one place it takes effect -- and the doc comment claiming the fields "ride through to the provider invocation" was false. Apply them where review.ai_model's overrides already land: claudeModel/codexModel/ollamaModel/openaiModel/openaiCompatibleModel/ anthropicModel <- reviewKnobs.model ?? review.ai_model ?? env claudeEffort/codexEffort <- reviewKnobs.effort ?? ... ?? env reviewKnobs wins over review.ai_model because it is strictly more specific: it is the only layer that can differ PER PR, and an escalation existing at all means it should win for the PR that triggered it. Unset ⇒ `??` falls straight through, so a repo using only review.ai_model is untouched. Provider also now governs the BYOK key. The providerKey gate runs before the knobs are resolved and only knew settings.aiReviewProvider, so an ESCALATED provider would have kept using a stored key belonging to a different one. Same rule, re-applied with the resolved value: a mismatch drops the key. Also re-syncs config/examples/loopover.full.yml with .loopover.yml.example -- caught by running the FULL suite this time, which is the same break class that took main down after #9813. --- config/examples/loopover.full.yml | 28 ++++++++++++++++++ src/queue/ai-review-orchestration.ts | 33 +++++++++++++++------ test/unit/review-knobs.test.ts | 44 ++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 9 deletions(-) diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index 24056fc0c..30bb77ce2 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -286,6 +286,24 @@ gate: # app(s) YOU run; loopover hardcodes no vendor. List of { name, appSlug }, or omit. Default: not # configured (byte-identical behavior for every repo that doesn't opt in). Config-as-code only — no DB # column or dashboard toggle. + # Review settings used INSTEAD of the repo defaults when a PR touches a `hardGuardrailGlobs` path + # (#9808/#9821) — so a guarded path buys MORE SCRUTINY rather than an automatic manual-review hold. + # + # Without this, a guardrail hit changes exactly one thing: it suppresses auto-merge and queues a human. The + # review itself is identical to any other file's — same model, same effort, single pass. That turns a + # protected path into maintainer toil without the analysis actually going up, and on a busy repo it is the + # single largest source of manual reviews. + # + # Any subset may be set; each unset field falls through to the repo value, then the global env. The common + # shape is "same model, think harder" — set only `effort` (and optionally `selfConsistencyRuns`) and the + # model/provider are inherited untouched. Manual review remains the fallback: if the escalated review is + # not clean, the PR still holds. + guardrailEscalation: + effort: high + selfConsistencyRuns: 3 + # provider: anthropic + # model: claude-opus-5 + advisoryCheckRuns: - name: Contributor trust appSlug: example-security-app @@ -440,6 +458,16 @@ gate: # String or null. Default: null (the key record's model, else a conservative # per-provider default). model: null + # Reasoning effort for the review pass: low | medium | high | xhigh | max (#9821). Per-repo parity with + # the global CLAUDE_AI_EFFORT / CODEX_AI_EFFORT env vars, which used to be the ONLY place this could be + # set even though provider and model were already per-repo. Unset ⇒ whatever the env configures + # (medium by default). Higher effort costs more tokens and runs longer — raise it for repos where a + # missed defect is expensive, not everywhere. + effort: medium + # TOTAL evaluations per review, primary included (#9821). Per-repo parity with + # AI_REVIEW_SELF_CONSISTENCY_RUNS. Clamped to {0, 2, 3}: one run cannot measure agreement, and the + # benefit saturates by three. 0 (default) = a single pass. Each extra run is a full paid review. + selfConsistencyRuns: 0 # Minimum calibrated AI-reviewer confidence (0-1). Under `mode: block`, # consensus and split AI-review defects still BLOCK the gate regardless of # this floor — what varies below it is `lowConfidenceDisposition` below. diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index a2287c017..b2319f5bb 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -878,7 +878,12 @@ export async function runAiReviewForAdvisory( actor: args.author, mode: args.settings.aiReviewMode === "block" ? "block" : "advisory", jobId: args.deliveryId, - providerKey, + // #9821: a provider chosen by gate.aiReview.provider or a guardrail escalation must also govern the BYOK + // key, not just the model. The `providerKey` gate above ran before the knobs were resolved and only knew + // `settings.aiReviewProvider`, so an ESCALATED provider would otherwise have kept using a stored key + // belonging to a different one. Same rule the gate above applies, re-applied with the resolved value: + // a mismatch drops the key and the review falls back to the operator's own configured provider. + providerKey: reviewKnobs.provider && providerKey && reviewKnobs.provider !== providerKey.provider ? null : providerKey, grounding, ragContext: ragContextResult?.text, cultureProfileContext, @@ -897,18 +902,28 @@ export async function runAiReviewForAdvisory( // Self-host per-repo model/effort/timeout override (#selfhost-ai-model-override, #8364): absent/null // fields fall through runLoopOverAiReview -> runWorkersOpinion -> the self-host provider's own // global-env/hardcoded default, exactly as if review.ai_model had never been set. - claudeModel: args.reviewSelfHostAiModel?.claudeModel ?? null, - claudeEffort: args.reviewSelfHostAiModel?.claudeEffort ?? null, - codexModel: args.reviewSelfHostAiModel?.codexModel ?? null, - codexEffort: args.reviewSelfHostAiModel?.codexEffort ?? null, + // + // #9808/#9821: `reviewKnobs` (gate.aiReview.effort/model, or the guardrailEscalation override when this + // PR touched a guarded path) takes priority over `review.ai_model` when set. Both are per-repo + // config-as-code; this one is strictly more specific -- it is the only one that can differ PER PR, and + // the whole point of an escalation is that it wins for the PR that triggered it. Unset ⇒ `??` falls + // straight through to review.ai_model, then to the global env, exactly as before. + // + // The provider CLIs read model and effort separately, so the resolved values are applied to BOTH the + // claude and codex pairs: whichever provider actually runs sees them, and the other's fields are inert. + claudeModel: reviewKnobs.model ?? args.reviewSelfHostAiModel?.claudeModel ?? null, + claudeEffort: reviewKnobs.effort ?? args.reviewSelfHostAiModel?.claudeEffort ?? null, + codexModel: reviewKnobs.model ?? args.reviewSelfHostAiModel?.codexModel ?? null, + codexEffort: reviewKnobs.effort ?? args.reviewSelfHostAiModel?.codexEffort ?? null, claudeTimeoutMs: args.reviewSelfHostAiModel?.claudeTimeoutMs ?? null, codexTimeoutMs: args.reviewSelfHostAiModel?.codexTimeoutMs ?? null, claudeFirstOutputTimeoutMs: args.reviewSelfHostAiModel?.claudeFirstOutputTimeoutMs ?? null, codexFirstOutputTimeoutMs: args.reviewSelfHostAiModel?.codexFirstOutputTimeoutMs ?? null, - ollamaModel: args.reviewSelfHostAiModel?.ollamaModel ?? null, - openaiModel: args.reviewSelfHostAiModel?.openaiModel ?? null, - openaiCompatibleModel: args.reviewSelfHostAiModel?.openaiCompatibleModel ?? null, - anthropicModel: args.reviewSelfHostAiModel?.anthropicModel ?? null, + // Same precedence for the HTTP-API providers, which take a model but have no effort concept. + ollamaModel: reviewKnobs.model ?? args.reviewSelfHostAiModel?.ollamaModel ?? null, + openaiModel: reviewKnobs.model ?? args.reviewSelfHostAiModel?.openaiModel ?? null, + openaiCompatibleModel: reviewKnobs.model ?? args.reviewSelfHostAiModel?.openaiCompatibleModel ?? null, + anthropicModel: reviewKnobs.model ?? args.reviewSelfHostAiModel?.anthropicModel ?? null, // Inline comments (#inline-comments): ask the model for line-anchored findings only when the operator flag, // the cutover allowlist, AND the per-repo manifest toggle all pass. Otherwise the prompt is byte-identical. inlineFindings: inlineFindingsRequested, diff --git a/test/unit/review-knobs.test.ts b/test/unit/review-knobs.test.ts index 66249d708..1794961ff 100644 --- a/test/unit/review-knobs.test.ts +++ b/test/unit/review-knobs.test.ts @@ -84,3 +84,47 @@ describe("describeReviewEscalation", () => { expect(describeReviewEscalation(r)).toBe("escalated review on a guarded path: provider=anthropic, model=claude-opus-5"); }); }); + +// The wiring, not just the resolver. A reviewer on #9821 correctly caught that resolveReviewKnobs was +// computed, logged, and then DROPPED: only selfConsistencyRuns was consumed, so "choose provider, model, +// effort" was unimplemented at the one place it takes effect. These pin the consumption side by asserting +// on the exact precedence expression the orchestration now uses, so the resolver can never again be wired +// to nothing without a test failing. +describe("resolved knobs reach the provider invocation (#9821 review blocker)", () => { + // Mirrors src/queue/ai-review-orchestration.ts: `reviewKnobs.X ?? reviewSelfHostAiModel?.X ?? null`. + const apply = (knob: string | null, perRepoAiModel: string | null) => knob ?? perRepoAiModel ?? null; + + it("REGRESSION: an escalated effort overrides review.ai_model's effort, not just selfConsistencyRuns", () => { + const r = resolveReviewKnobs({ guardrailHit: true, escalation: { effort: "high" }, global: GLOBAL }); + expect(apply(r.effort, "medium")).toBe("high"); + }); + + it("an escalated model overrides review.ai_model's model for every provider pair", () => { + const r = resolveReviewKnobs({ guardrailHit: true, escalation: { model: "escalated-model" }, global: GLOBAL }); + // claude/codex/ollama/openai/anthropic all take the same resolved value. + expect(apply(r.model, "repo-ai-model")).toBe("escalated-model"); + }); + + it("INVARIANT: an unset knob falls through to review.ai_model, then to the env — never clobbers with null", () => { + // The precedence that keeps this backwards-compatible: a repo using only review.ai_model is untouched. + const r = resolveReviewKnobs({ guardrailHit: false, global: { ...GLOBAL, effort: null, model: null } }); + expect(apply(r.effort, "medium")).toBe("medium"); + expect(apply(r.model, "repo-ai-model")).toBe("repo-ai-model"); + }); + + it("INVARIANT: a provider escalation that disagrees with the stored BYOK key drops the key", () => { + // Mirrors the orchestration's providerKey guard: an escalated provider must govern the KEY too, or the + // review would run the escalated provider's name against another provider's credential. + const r = resolveReviewKnobs({ guardrailHit: true, escalation: { provider: "anthropic" }, global: GLOBAL }); + const storedKey = { provider: "openai", key: "sk-x", model: "gpt" }; + const effective = r.provider && storedKey && r.provider !== storedKey.provider ? null : storedKey; + expect(effective).toBeNull(); + }); + + it("a provider escalation that MATCHES the stored key keeps it", () => { + const r = resolveReviewKnobs({ guardrailHit: true, escalation: { provider: "openai" }, global: GLOBAL }); + const storedKey = { provider: "openai", key: "sk-x", model: "gpt" }; + const effective = r.provider && storedKey && r.provider !== storedKey.provider ? null : storedKey; + expect(effective).toBe(storedKey); + }); +}); From 937e0279450ebc8b41feb6ca4b93ef8c620e4d07 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 06:21:56 -0700 Subject: [PATCH 3/5] test(gate): cover every new manifest branch, and fix the serialize guard the coverage exposed Codecov flagged 39 uncovered lines on #9821, and closing them found a REAL bug: gateConfigToJson's outer aiReview guard was never taught the two new fields, so a manifest whose aiReview block set ONLY effort/selfConsistencyRuns serialized to NOTHING -- the setting silently vanished on the next snapshot reload. The new round-trip assertion failed on exactly that before touching coverage at all. (An earlier edit had aimed at this guard and hit the presence expression instead; the round-trip test is what caught the difference.) Coverage added: - engine parse: full/partial/invalid/non-mapping guardrailEscalation, invalid effort + runs values warn and stay null, presence flips on EACH of the six fields alone (the #9813 presence-gap class, per field), serialize round-trips full AND partial blocks - resolution: every field lands on effective settings; absent leaves the stored value untouched - END-TO-END escalation: env self-consistency OFF, manifest hardGuardrailGlobs + guardrailEscalation.selfConsistencyRuns=3, a guarded changed file -- the pipeline delivers 1 primary + 2 rotated-exemplar extra calls, proving the resolved knobs reach the invocation (the #9821 review blocker, now pinned by an integration test, not just the precedence mirror) - INERT invariant: same escalation block, no guarded file -- zero selfConsistency usage rows (counted via the usage marker, not raw AI.run calls, matching the flag-off baseline's discipline) --- .../loopover-engine/src/focus-manifest.ts | 4 +- test/unit/focus-manifest.test.ts | 70 +++++++++++++++++++ test/unit/self-consistency.test.ts | 65 +++++++++++++++++ 3 files changed, 138 insertions(+), 1 deletion(-) diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index 6f91587bd..8c9fe7209 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -2076,7 +2076,9 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { gate.aiReviewLowConfidenceDisposition !== null || gate.aiReviewCombine !== null || gate.aiReviewOnMerge !== null || - gate.aiReviewReviewers !== null + gate.aiReviewReviewers !== null || + gate.aiReviewEffort !== null || + gate.aiReviewSelfConsistencyRuns !== null ) { const aiReview: Record = {}; if (gate.aiReviewMode !== null) aiReview.mode = gate.aiReviewMode; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 17b8d7bda..849eeaabe 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1642,6 +1642,76 @@ describe("parseFocusManifest gate config", () => { expect(over.warnings.some((w) => /gate\.aiReview\.reviewers" is capped/.test(w))).toBe(true); }); + it("parses gate.aiReview.effort/selfConsistencyRuns and gate.guardrailEscalation with full branch coverage (#9821)", () => { + // Every field set: parse, presence, round-trip, resolve. + const full = parseFocusManifest({ + gate: { + aiReview: { effort: "high", selfConsistencyRuns: 3 }, + guardrailEscalation: { provider: "anthropic", model: "claude-opus-5", effort: "xhigh", selfConsistencyRuns: 2 }, + }, + }); + expect(full.gate.present).toBe(true); + expect(full.gate.aiReviewEffort).toBe("high"); + expect(full.gate.aiReviewSelfConsistencyRuns).toBe(3); + expect(full.gate.guardrailEscalationProvider).toBe("anthropic"); + expect(full.gate.guardrailEscalationModel).toBe("claude-opus-5"); + expect(full.gate.guardrailEscalationEffort).toBe("xhigh"); + expect(full.gate.guardrailEscalationSelfConsistencyRuns).toBe(2); + expect(parseFocusManifest({ gate: gateConfigToJson(full.gate) }).gate).toEqual(full.gate); // serialize round-trips + + // Each of the six fields ALONE flips presence — the #9813 presence-gap class, per field. + for (const gate of [ + { aiReview: { effort: "low" } }, + { aiReview: { selfConsistencyRuns: 0 } }, + { guardrailEscalation: { provider: "openai" } }, + { guardrailEscalation: { model: "m" } }, + { guardrailEscalation: { effort: "max" } }, + { guardrailEscalation: { selfConsistencyRuns: 3 } }, + ]) { + expect(parseFocusManifest({ gate }).gate.present).toBe(true); + } + + // Partial escalation: unset fields stay null (each falls through to repo/global downstream). + const partial = parseFocusManifest({ gate: { guardrailEscalation: { effort: "high" } } }); + expect(partial.gate.guardrailEscalationEffort).toBe("high"); + expect(partial.gate.guardrailEscalationProvider).toBeNull(); + expect(partial.gate.guardrailEscalationModel).toBeNull(); + expect(partial.gate.guardrailEscalationSelfConsistencyRuns).toBeNull(); + expect(parseFocusManifest({ gate: gateConfigToJson(partial.gate) }).gate).toEqual(partial.gate); + + // Invalid values warn and stay null — never silently coerced. + const bad = parseFocusManifest({ + gate: { aiReview: { effort: "ultra", selfConsistencyRuns: -1 }, guardrailEscalation: { provider: "grok", effort: 7, selfConsistencyRuns: "three", model: 42 } }, + }); + expect(bad.gate.aiReviewEffort).toBeNull(); + expect(bad.gate.aiReviewSelfConsistencyRuns).toBeNull(); + expect(bad.gate.guardrailEscalationProvider).toBeNull(); + expect(bad.gate.guardrailEscalationEffort).toBeNull(); + expect(bad.gate.guardrailEscalationSelfConsistencyRuns).toBeNull(); + expect(bad.gate.guardrailEscalationModel).toBeNull(); + expect(bad.warnings.some((w) => /gate\.aiReview\.effort/.test(w))).toBe(true); + expect(bad.warnings.some((w) => /gate\.guardrailEscalation\.provider/.test(w))).toBe(true); + + // A non-mapping guardrailEscalation is ignored wholesale (same contract as gate.aiReview). + const notMap = parseFocusManifest({ gate: { guardrailEscalation: "high", claMode: "advisory" } }); + expect(notMap.gate.guardrailEscalationEffort).toBeNull(); + + // Resolution: each field lands on effective settings; absent leaves the DB value untouched. + const eff = resolveEffectiveSettings({} as unknown as RepositorySettings, full); + expect(eff.aiReviewEffort).toBe("high"); + expect(eff.aiReviewSelfConsistencyRuns).toBe(3); + expect(eff.guardrailEscalationProvider).toBe("anthropic"); + expect(eff.guardrailEscalationModel).toBe("claude-opus-5"); + expect(eff.guardrailEscalationEffort).toBe("xhigh"); + expect(eff.guardrailEscalationSelfConsistencyRuns).toBe(2); + const untouched = resolveEffectiveSettings( + { aiReviewEffort: "low", guardrailEscalationModel: "existing" } as unknown as RepositorySettings, + parseFocusManifest({ gate: { claMode: "advisory" } }), + ); + expect(untouched.aiReviewEffort).toBe("low"); + expect(untouched.guardrailEscalationModel).toBe("existing"); + }); + it("parses gate.ignoredCheckRuns, makes the gate present, round-trips + resolves it, and drops spoofable entries (#9810)", () => { const m = parseFocusManifest({ gate: { ignoredCheckRuns: [{ name: "Contributor trust", appSlug: "example-security-app" }] } }); expect(m.gate.present).toBe(true); diff --git a/test/unit/self-consistency.test.ts b/test/unit/self-consistency.test.ts index b0a6142e3..54595a2c4 100644 --- a/test/unit/self-consistency.test.ts +++ b/test/unit/self-consistency.test.ts @@ -236,6 +236,71 @@ describe("rotated-exemplar self-consistency (#8834)", () => { expect(usage?.n).toBe(2); // every extra run rides the SAME budget sum the quota gate reads }); + it("REGRESSION (#9821): a guardrail hit ESCALATES runs end-to-end — env off, gate.guardrailEscalation.selfConsistencyRuns=3 delivers 2 extra judged calls", async () => { + // The review blocker on #9821 was that resolveReviewKnobs was computed and dropped. This drives the + // whole pipeline: manifest -> settings -> orchestration (isGuardrailHit on src/a.ts) -> reviewKnobs -> + // runLoopOverAiReview's runs override (the `input.reviewKnobs?.selfConsistencyRuns` branch) -> the + // planner -> two extra rotated-exemplar calls. AI_REVIEW_SELF_CONSISTENCY_RUNS is deliberately UNSET: + // every extra call here exists only because the escalation reached the invocation. + const prompts: string[] = []; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async (_model: string, options: { messages?: Array<{ role: string; content: string }> }) => { + prompts.push(options.messages?.find((m) => m.role === "system")?.content ?? ""); + return { response: JSON.stringify({ assessment: "Fine.", blockers: [], nits: [], suggestions: [] }) }; + } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seed(env); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { + settings: { + commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "block", + hardGuardrailGlobs: ["src/**"], // the PR's one changed file is src/a.ts -- a guardrail hit + }, + gate: { guardrailEscalation: { selfConsistencyRuns: 3 } }, + }); + + await reReviewStoredPullRequest(env, "sc-escalated-900", 123, "JSONbored/gittensory", 900); + + const judgeCalls = prompts.filter((prompt) => prompt.length > 0); + expect(judgeCalls.length).toBeGreaterThanOrEqual(3); // 1 primary + 2 escalated extras + expect(judgeCalls.filter((prompt) => prompt.includes("Calibration examples"))).toHaveLength(2); + }); + + it("INVARIANT (#9821): the same escalation block is INERT when no changed file hits a guardrail glob", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { + aiCalls += 1; + return { response: JSON.stringify({ assessment: "Fine.", blockers: [], nits: [], suggestions: [] }) }; + } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seed(env); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { + settings: { + commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "block", + hardGuardrailGlobs: ["migrations/**"], // src/a.ts does NOT match -- no hit, no escalation + }, + gate: { guardrailEscalation: { selfConsistencyRuns: 3 } }, + }); + + await reReviewStoredPullRequest(env, "sc-inert-900", 123, "JSONbored/gittensory", 900); + // Counted via the selfConsistency usage marker, not raw AI.run calls: other pipeline consumers (the + // slop advisory, summaries) legitimately also call AI.run -- the precise claim is that the ESCALATED + // runs never fired, same discipline as the flag-off baseline above. + const usage = await env.DB.prepare( + "select count(*) as n from ai_usage_events where json_extract(metadata_json, '$.selfConsistency') = 1", + ).first<{ n: number }>(); + expect(usage?.n).toBe(0); + expect(aiCalls).toBeGreaterThan(0); // the pass itself still reviewed + }); + it("INVARIANT: flag off (default) spends nothing extra and adds no self-consistency usage rows", async () => { let aiCalls = 0; const env = createTestEnv({ From a04ea97240e67757aa3dcbb052a83b37c8116e1b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 06:36:06 -0700 Subject: [PATCH 4/5] fix(gate): consume the resolved model/effort in ai-review.ts itself, where the reviewer looked Re-review blocker: the diff still showed only selfConsistencyRuns consumed in src/services/ai-review.ts. The orchestration DID pre-apply model/effort to the claude*/codex*/HTTP-model fields, so behavior was correct -- but this module, which owns the doc comment claiming the fields "ride through to the provider invocation", never read them, and any OTHER caller passing reviewKnobs would have been silently ignored. Fold reviewKnobs.model/effort into the AiRunCorrelation, which IS what becomes the provider's per-call options at this module's dispatch. Same ?? precedence as the orchestration, so applying it in both places is idempotent, and this module is now self-sufficient. Correct the doc comment to state exactly what is consumed WHERE, including that `provider` is deliberately NOT consumed here -- it governs BYOK key selection at the orchestration, before this runs, and rides along for logging/replay only. Test captures the options the provider actually receives (claudeEffort/ claudeModel/codexEffort/anthropicModel) for a guarded-path PR with an escalation block, rather than asserting on the precedence mirror. --- src/services/ai-review.ts | 26 +++++++++++++--------- test/unit/self-consistency.test.ts | 35 ++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 06a861ccd..bb36116cb 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -221,8 +221,11 @@ export function resolveEffectiveAiReviewPlan( export type LoopOverAiReviewInput = { /** #9808/#9821: the effective review knobs the caller resolved for THIS PR (resolveReviewKnobs) -- already * layered escalation > per-repo > global, so this module never re-derives precedence. Absent ⇒ the env - * defaults exactly as before. `selfConsistencyRuns` here outranks AI_REVIEW_SELF_CONSISTENCY_RUNS; the - * model/effort/provider fields ride through to the provider invocation. */ + * defaults exactly as before. Consumed HERE: `selfConsistencyRuns` outranks + * AI_REVIEW_SELF_CONSISTENCY_RUNS, and `model`/`effort` outrank the review.ai_model fields on the + * AiRunCorrelation that rides to every provider invocation. `provider` is the one field NOT consumed in + * this module: it governs the BYOK key selection, which happens at the orchestration BEFORE this runs + * (a mismatched stored key is dropped there) -- it is carried on this object for logging/replay only. */ reviewKnobs?: { provider?: string | null; model?: string | null; effort?: string | null; selfConsistencyRuns?: number | null } | undefined; repoFullName: string; prNumber: number; @@ -3111,18 +3114,21 @@ export async function runLoopOverAiReview( jobId: input.jobId, repoFullName: input.repoFullName, pullNumber: input.prNumber, - claudeModel: input.claudeModel ?? undefined, - claudeEffort: input.claudeEffort ?? undefined, - codexModel: input.codexModel ?? undefined, - codexEffort: input.codexEffort ?? undefined, + // #9821: reviewKnobs (per-repo gate.aiReview.*, or the guardrailEscalation override on a guarded path) + // outrank the review.ai_model fields -- the same precedence the orchestration pre-applies; folding it + // here too makes THIS module honor the knobs for any caller, and is idempotent when both did. + claudeModel: input.reviewKnobs?.model ?? input.claudeModel ?? undefined, + claudeEffort: input.reviewKnobs?.effort ?? input.claudeEffort ?? undefined, + codexModel: input.reviewKnobs?.model ?? input.codexModel ?? undefined, + codexEffort: input.reviewKnobs?.effort ?? input.codexEffort ?? undefined, claudeTimeoutMs: input.claudeTimeoutMs ?? undefined, codexTimeoutMs: input.codexTimeoutMs ?? undefined, claudeFirstOutputTimeoutMs: input.claudeFirstOutputTimeoutMs ?? undefined, codexFirstOutputTimeoutMs: input.codexFirstOutputTimeoutMs ?? undefined, - ollamaModel: input.ollamaModel ?? undefined, - openaiModel: input.openaiModel ?? undefined, - openaiCompatibleModel: input.openaiCompatibleModel ?? undefined, - anthropicModel: input.anthropicModel ?? undefined, + ollamaModel: input.reviewKnobs?.model ?? input.ollamaModel ?? undefined, + openaiModel: input.reviewKnobs?.model ?? input.openaiModel ?? undefined, + openaiCompatibleModel: input.reviewKnobs?.model ?? input.openaiCompatibleModel ?? undefined, + anthropicModel: input.reviewKnobs?.model ?? input.anthropicModel ?? undefined, }; if (input.providerKey) { const outcome = await runProviderReview( diff --git a/test/unit/self-consistency.test.ts b/test/unit/self-consistency.test.ts index 54595a2c4..d1b2f5a7f 100644 --- a/test/unit/self-consistency.test.ts +++ b/test/unit/self-consistency.test.ts @@ -269,6 +269,41 @@ describe("rotated-exemplar self-consistency (#8834)", () => { expect(judgeCalls.filter((prompt) => prompt.includes("Calibration examples"))).toHaveLength(2); }); + it("REGRESSION (#9821 review blocker): an escalated EFFORT/MODEL reaches the provider options, not just runs", async () => { + // The reviewer's exact objection: only selfConsistencyRuns was consumed, so "choose model, effort" was + // unimplemented where it takes effect. runLoopOverAiReview folds reviewKnobs into the AiRunCorrelation, + // which is what becomes the provider's per-call options (claudeModel/claudeEffort/... at ai-review.ts's + // provider dispatch). Capturing those options is therefore the honest end-to-end assertion. + const seenOptions: Array> = []; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async (_model: string, options: Record) => { + seenOptions.push(options); + return { response: JSON.stringify({ assessment: "Fine.", blockers: [], nits: [], suggestions: [] }) }; + } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seed(env); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { + settings: { + commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "block", + hardGuardrailGlobs: ["src/**"], + }, + gate: { guardrailEscalation: { effort: "xhigh", model: "escalated-model-x" } }, + }); + + await reReviewStoredPullRequest(env, "sc-effort-900", 123, "JSONbored/gittensory", 900); + + // At least one provider call carries BOTH escalated values. + const escalated = seenOptions.filter((o) => o.claudeEffort === "xhigh" && o.claudeModel === "escalated-model-x"); + expect(escalated.length).toBeGreaterThan(0); + // And the HTTP-API provider fields carry the same resolved model (whichever provider actually runs). + expect(escalated[0]?.anthropicModel).toBe("escalated-model-x"); + expect(escalated[0]?.codexEffort).toBe("xhigh"); + }); + it("INVARIANT (#9821): the same escalation block is INERT when no changed file hits a guardrail glob", async () => { let aiCalls = 0; const env = createTestEnv({ From e98bbb2c24fbe5b238341f5f3df6e969d739509d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:23:48 -0700 Subject: [PATCH 5/5] test(engine): cover the new manifest branches in the ENGINE's own suite Codecov kept reporting focus-manifest.ts at ~65% despite the root vitest suite covering every added line. The reports disagree because they measure differently: the root v8 run instruments only the 11 lines it actually executes from that source, while the engine's own c8 run uses --all over dist/ and instruments all 66 -- and the engine's node:test suite never exercised the new branches. Codecov merges both, so 21 lines read as uncovered no matter how thorough the vitest tests were. Add the engine-side tests: parse, per-field presence, serialize round-trip (full AND partial -- the case whose guard was genuinely broken), invalid-value rejection, non-mapping escalation, and absent-means-null including that the serializer emits no escalation block at all. Engine c8 now reports 0 of 66 added lines uncovered, down from 21. --- .../test/focus-manifest-review-knobs.test.ts | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 packages/loopover-engine/test/focus-manifest-review-knobs.test.ts diff --git a/packages/loopover-engine/test/focus-manifest-review-knobs.test.ts b/packages/loopover-engine/test/focus-manifest-review-knobs.test.ts new file mode 100644 index 000000000..8a716e3d9 --- /dev/null +++ b/packages/loopover-engine/test/focus-manifest-review-knobs.test.ts @@ -0,0 +1,90 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { gateConfigToJson, parseFocusManifest } from "../dist/focus-manifest.js"; + +// #9821 added per-repo `gate.aiReview.effort`/`selfConsistencyRuns` and the `gate.guardrailEscalation` block. +// These live in the ENGINE, which has its own c8 coverage run over dist/ with `--all` — so every line counts +// whether or not a test loads it, and the root vitest suite's coverage of the same source does NOT satisfy it +// (that report only instruments the 11 lines it actually executes; the engine report instruments all 66). +// Codecov merges both, so the engine's own suite has to exercise these branches or they read as uncovered. + +test("parses the new aiReview knobs and the guardrailEscalation block", () => { + const parsed = parseFocusManifest({ + gate: { + aiReview: { effort: "high", selfConsistencyRuns: 3 }, + guardrailEscalation: { provider: "anthropic", model: "claude-opus-5", effort: "xhigh", selfConsistencyRuns: 2 }, + }, + }); + assert.equal(parsed.gate.present, true); + assert.equal(parsed.gate.aiReviewEffort, "high"); + assert.equal(parsed.gate.aiReviewSelfConsistencyRuns, 3); + assert.equal(parsed.gate.guardrailEscalationProvider, "anthropic"); + assert.equal(parsed.gate.guardrailEscalationModel, "claude-opus-5"); + assert.equal(parsed.gate.guardrailEscalationEffort, "xhigh"); + assert.equal(parsed.gate.guardrailEscalationSelfConsistencyRuns, 2); +}); + +test("each new field ALONE makes the gate present", () => { + // The #9813 presence-gap class, per field: a gate block setting only one of these must not parse as absent, + // or the whole block is silently discarded. + for (const gate of [ + { aiReview: { effort: "low" } }, + { aiReview: { selfConsistencyRuns: 0 } }, + { guardrailEscalation: { provider: "openai" } }, + { guardrailEscalation: { model: "m" } }, + { guardrailEscalation: { effort: "max" } }, + { guardrailEscalation: { selfConsistencyRuns: 3 } }, + ]) { + assert.equal(parseFocusManifest({ gate }).gate.present, true, JSON.stringify(gate)); + } +}); + +test("serialize round-trips the new fields, full and partial", () => { + // The serialize guard was genuinely broken until a round-trip assertion caught it: an aiReview block setting + // ONLY effort/selfConsistencyRuns emitted nothing, so the setting vanished on the next snapshot reload. + for (const gate of [ + { aiReview: { effort: "high", selfConsistencyRuns: 3 }, guardrailEscalation: { effort: "xhigh", selfConsistencyRuns: 2, provider: "anthropic", model: "m" } }, + { aiReview: { effort: "medium" } }, + { aiReview: { selfConsistencyRuns: 2 } }, + { guardrailEscalation: { effort: "high" } }, + { guardrailEscalation: { selfConsistencyRuns: 3 } }, + ]) { + const parsed = parseFocusManifest({ gate }); + const round = parseFocusManifest({ gate: gateConfigToJson(parsed.gate) }); + assert.deepEqual(round.gate, parsed.gate, JSON.stringify(gate)); + } +}); + +test("invalid values warn and stay null rather than being coerced", () => { + const parsed = parseFocusManifest({ + gate: { + aiReview: { effort: "ultra", selfConsistencyRuns: -1 }, + guardrailEscalation: { provider: "grok", effort: 7, selfConsistencyRuns: "three", model: 42 }, + }, + }); + assert.equal(parsed.gate.aiReviewEffort, null); + assert.equal(parsed.gate.aiReviewSelfConsistencyRuns, null); + assert.equal(parsed.gate.guardrailEscalationProvider, null); + assert.equal(parsed.gate.guardrailEscalationEffort, null); + assert.equal(parsed.gate.guardrailEscalationSelfConsistencyRuns, null); + assert.equal(parsed.gate.guardrailEscalationModel, null); + assert.ok(parsed.warnings.some((w) => /gate\.aiReview\.effort/.test(w))); + assert.ok(parsed.warnings.some((w) => /gate\.guardrailEscalation\.provider/.test(w))); +}); + +test("a non-mapping guardrailEscalation is ignored wholesale, and absence leaves every field null", () => { + const notMapping = parseFocusManifest({ gate: { guardrailEscalation: "high", claMode: "advisory" } }); + assert.equal(notMapping.gate.guardrailEscalationEffort, null); + + const absent = parseFocusManifest({ gate: { claMode: "advisory" } }); + assert.equal(absent.gate.aiReviewEffort, null); + assert.equal(absent.gate.aiReviewSelfConsistencyRuns, null); + assert.equal(absent.gate.guardrailEscalationProvider, null); + assert.equal(absent.gate.guardrailEscalationModel, null); + assert.equal(absent.gate.guardrailEscalationEffort, null); + assert.equal(absent.gate.guardrailEscalationSelfConsistencyRuns, null); + // Absent ⇒ the serializer emits no escalation block at all (not an empty one). + const json = gateConfigToJson(absent.gate) as Record; + assert.equal(json.guardrailEscalation, undefined); +});