diff --git a/.loopover.yml.example b/.loopover.yml.example index 3f8ec53bf..2b5a25c9b 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -289,6 +289,14 @@ gate: selfConsistencyRuns: 3 # provider: anthropic # model: claude-opus-5 + # What a CLEAN escalated review buys (#9808). `hold` (default): the PR still waits for a human even when + # the escalated review found nothing — human-in-the-loop mode. `proceed`: a clean escalated review + # (gate success + green CI) RELEASES the guardrail hold and the normal approve/merge path continues — + # full-autonomy mode, where the guarded path is protected by the escalated review instead of a queue. + # Fail-closed: `proceed` is inert unless at least one escalation knob above is actually set, so the hold + # can never be released without the extra scrutiny that justifies releasing it. Layered like every other + # manifest field (global -> per-repo), so repos can be flipped to full-auto one at a time. + # onCleanReview: proceed advisoryCheckRuns: - name: Contributor trust diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 1aab6d0a7..020e537ea 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -10197,6 +10197,15 @@ "nullable": true, "minimum": 0, "maximum": 1440 + }, + "guardrailEscalationOnCleanReview": { + "type": "string", + "nullable": true, + "enum": [ + "hold", + "proceed", + null + ] } }, "required": [ diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index afd2fc1a8..f938254b5 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -303,6 +303,14 @@ gate: selfConsistencyRuns: 3 # provider: anthropic # model: claude-opus-5 + # What a CLEAN escalated review buys (#9808). `hold` (default): the PR still waits for a human even when + # the escalated review found nothing — human-in-the-loop mode. `proceed`: a clean escalated review + # (gate success + green CI) RELEASES the guardrail hold and the normal approve/merge path continues — + # full-autonomy mode, where the guarded path is protected by the escalated review instead of a queue. + # Fail-closed: `proceed` is inert unless at least one escalation knob above is actually set, so the hold + # can never be released without the extra scrutiny that justifies releasing it. Layered like every other + # manifest field (global -> per-repo), so repos can be flipped to full-auto one at a time. + # onCleanReview: proceed advisoryCheckRuns: - name: Contributor trust diff --git a/packages/loopover-contract/src/api-schemas.ts b/packages/loopover-contract/src/api-schemas.ts index d92b26f14..db2abf766 100644 --- a/packages/loopover-contract/src/api-schemas.ts +++ b/packages/loopover-contract/src/api-schemas.ts @@ -459,6 +459,7 @@ export const RepositorySettingsSchema = z guardrailEscalationModel: z.string().nullable().optional(), guardrailEscalationEffort: z.enum(["low", "medium", "high", "xhigh", "max"]).nullable().optional(), guardrailEscalationSelfConsistencyRuns: z.number().nullable().optional(), + guardrailEscalationOnCleanReview: z.enum(["hold", "proceed"]).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 4ac9b43f3..73f8a3ec2 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -130,6 +130,13 @@ export type FocusManifestGateConfig = { guardrailEscalationModel: string | null; guardrailEscalationEffort: "low" | "medium" | "high" | "xhigh" | "max" | null; guardrailEscalationSelfConsistencyRuns: number | null; + /** `gate.guardrailEscalation.onCleanReview` (#9808 second half): what a CLEAN escalated review buys. + * `hold` (default) keeps today's behavior -- the PR still waits for a human even when the escalated + * review found nothing. `proceed` releases the guardrail hold when the gate passed and CI is green, so a + * guarded path is protected by the ESCALATED REVIEW rather than by a human queue -- the full-autonomy + * mode. Fail-closed: `proceed` does nothing unless at least one escalation knob is actually set, so the + * hold can never be released without the extra scrutiny that justifies releasing it. */ + guardrailEscalationOnCleanReview: "hold" | "proceed" | 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. */ @@ -659,6 +666,7 @@ export type FocusManifestSettings = Partial< | "guardrailEscalationModel" | "guardrailEscalationEffort" | "guardrailEscalationSelfConsistencyRuns" + | "guardrailEscalationOnCleanReview" | "aiReviewAllAuthors" | "aiReviewConfirmedContributorsOnly" | "closeOwnerAuthors" @@ -1376,6 +1384,7 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, + guardrailEscalationOnCleanReview: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, @@ -1928,6 +1937,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu 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), + guardrailEscalationOnCleanReview: normalizeOptionalEnum(escalationRecord?.onCleanReview, "gate.guardrailEscalation.onCleanReview", ["hold", "proceed"] as const, 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), @@ -2028,6 +2038,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.guardrailEscalationModel !== null || gate.guardrailEscalationEffort !== null || gate.guardrailEscalationSelfConsistencyRuns !== null || + gate.guardrailEscalationOnCleanReview !== null || gate.ignoredCheckRuns !== null || gate.aiJudgmentBlockersMode !== null || gate.copycatMode !== null || @@ -2116,13 +2127,15 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { gate.guardrailEscalationProvider !== null || gate.guardrailEscalationModel !== null || gate.guardrailEscalationEffort !== null || - gate.guardrailEscalationSelfConsistencyRuns !== null + gate.guardrailEscalationSelfConsistencyRuns !== null || + gate.guardrailEscalationOnCleanReview !== 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; + if (gate.guardrailEscalationOnCleanReview !== null) escalation.onCleanReview = gate.guardrailEscalationOnCleanReview; out.guardrailEscalation = escalation; } if (gate.mergeReadiness !== null) out.mergeReadiness = gate.mergeReadiness; diff --git a/packages/loopover-engine/src/types/manifest-deps-types.ts b/packages/loopover-engine/src/types/manifest-deps-types.ts index d4a55249e..eb8d02cff 100644 --- a/packages/loopover-engine/src/types/manifest-deps-types.ts +++ b/packages/loopover-engine/src/types/manifest-deps-types.ts @@ -309,6 +309,10 @@ export type RepositorySettings = { guardrailEscalationModel?: string | null | undefined; guardrailEscalationEffort?: "low" | "medium" | "high" | "xhigh" | "max" | null | undefined; guardrailEscalationSelfConsistencyRuns?: number | null | undefined; + /** `gate.guardrailEscalation.onCleanReview` (#9808 second half): `proceed` releases the guardrail hold + * when the escalated review came back clean (gate success + CI green); `hold` (default) keeps a human in + * the loop even then. Fail-closed: `proceed` is inert unless an escalation knob is actually set. */ + guardrailEscalationOnCleanReview?: "hold" | "proceed" | 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/packages/loopover-engine/test/focus-manifest-review-knobs.test.ts b/packages/loopover-engine/test/focus-manifest-review-knobs.test.ts index 8a716e3d9..410fdfde3 100644 --- a/packages/loopover-engine/test/focus-manifest-review-knobs.test.ts +++ b/packages/loopover-engine/test/focus-manifest-review-knobs.test.ts @@ -88,3 +88,18 @@ test("a non-mapping guardrailEscalation is ignored wholesale, and absence leaves const json = gateConfigToJson(absent.gate) as Record; assert.equal(json.guardrailEscalation, undefined); }); + +test("onCleanReview parses, round-trips, makes the gate present alone, and rejects junk (#9808)", () => { + const parsed = parseFocusManifest({ gate: { guardrailEscalation: { onCleanReview: "proceed" } } }); + assert.equal(parsed.gate.present, true); + assert.equal(parsed.gate.guardrailEscalationOnCleanReview, "proceed"); + assert.deepEqual(parseFocusManifest({ gate: gateConfigToJson(parsed.gate) }).gate, parsed.gate); + + const hold = parseFocusManifest({ gate: { guardrailEscalation: { onCleanReview: "hold", effort: "high" } } }); + assert.equal(hold.gate.guardrailEscalationOnCleanReview, "hold"); + assert.deepEqual(parseFocusManifest({ gate: gateConfigToJson(hold.gate) }).gate, hold.gate); + + const junk = parseFocusManifest({ gate: { guardrailEscalation: { onCleanReview: "yolo" } } }); + assert.equal(junk.gate.guardrailEscalationOnCleanReview, null); + assert.ok(junk.warnings.some((w) => /guardrailEscalation\.onCleanReview/.test(w))); +}); diff --git a/scripts/check-docs-drift.ts b/scripts/check-docs-drift.ts index 174d21aed..0ee8c9685 100644 --- a/scripts/check-docs-drift.ts +++ b/scripts/check-docs-drift.ts @@ -182,6 +182,7 @@ export const SETTINGS_ALIAS_MANIFEST: AliasManifestRow[] = [ { field: "guardrailEscalationModel", aliases: ["guardrailEscalation:"] }, { field: "guardrailEscalationEffort", aliases: ["guardrailEscalation:"] }, { field: "guardrailEscalationSelfConsistencyRuns", aliases: ["guardrailEscalation:"] }, + { field: "guardrailEscalationOnCleanReview", 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 3b1c71cfb..1206401a6 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -755,6 +755,7 @@ export const RepositorySettingsSchema = z guardrailEscalationModel: z.string().nullable().optional(), guardrailEscalationEffort: z.enum(["low", "medium", "high", "xhigh", "max"]).nullable().optional(), guardrailEscalationSelfConsistencyRuns: z.number().nullable().optional(), + guardrailEscalationOnCleanReview: z.enum(["hold", "proceed"]).nullable().optional(), copycatGateMode: z.enum(["off", "warn", "label", "block"]).optional(), copycatGateMinScore: z.number().nullable().optional(), gateDryRun: z.boolean().optional(), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c6524e5f1..dc8d374be 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2868,6 +2868,13 @@ function buildAgentMaintenancePlanInput(args: { slopGateMinScore: settings.slopGateMinScore, changedPaths, hardGuardrailGlobs, + // #9808 second half: the escalation settings ride with the globs they modify, so the planner can release + // the guardrail hold when a clean escalated review has vouched for the guarded path. + guardrailEscalationOnCleanReview: settings.guardrailEscalationOnCleanReview ?? null, + guardrailEscalationEffort: settings.guardrailEscalationEffort ?? null, + guardrailEscalationSelfConsistencyRuns: settings.guardrailEscalationSelfConsistencyRuns ?? null, + guardrailEscalationModel: settings.guardrailEscalationModel ?? null, + guardrailEscalationProvider: settings.guardrailEscalationProvider ?? null, manualReviewLabel: settings.manualReviewLabel, readyToMergeLabel: settings.readyToMergeLabel, changesRequestedLabel: settings.changesRequestedLabel, diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index b4643cad9..40f5dacd0 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -275,6 +275,14 @@ export type AgentActionPlanInput = { // review-good PRs; blockers, red CI, and base conflicts still close for close-eligible contributors. changedPaths: string[]; hardGuardrailGlobs: string[]; + /** #9808 second half: the guardrail-escalation settings, threaded as discrete fields like every other + * settings-derived input here. `onCleanReview: "proceed"` + at least one knob set + reviewGood releases + * the guardrail hold; anything less keeps today's behavior exactly. */ + guardrailEscalationOnCleanReview?: "hold" | "proceed" | null | undefined; + guardrailEscalationEffort?: string | null | undefined; + guardrailEscalationSelfConsistencyRuns?: number | null | undefined; + guardrailEscalationModel?: string | null | undefined; + guardrailEscalationProvider?: string | null | undefined; // Configured manual-review hold label. Undefined uses the default "manual-review"; null disables only the // label, not the guardrail hold. Separate from review_state_label so operators can avoid ready/changes labels. manualReviewLabel?: string | null | undefined; @@ -1115,10 +1123,28 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // are not disposition. reviewGood is computed here (moved up from beside canMerge — same formula, // gate passes AND CI green) because the disposition needs it. const reviewGood = gatePassing && ciPassed; + // #9808 second half: a clean ESCALATED review releases the guardrail hold -- full-autonomy mode. All four + // conjuncts are load-bearing: + // - the mode is an explicit opt-in (`guardrailEscalation.onCleanReview: proceed`), default hold, layered + // global -> per-repo like every other manifest field, so an operator can flip one repo at a time; + // - at least one escalation knob must actually be SET -- the release is justified by extra scrutiny, so + // absent scrutiny there is nothing to vouch for the guarded path (fail-closed against a config that + // sets the mode but forgets the escalation); + // - reviewGood: the gate passed (which already folds in the AI verdict's blockers -- a consensus blocker + // fails the gate) AND CI is green; + // - guardrailHit, or there is nothing to clear. + const escalationConfigured = + input.guardrailEscalationEffort != null || + input.guardrailEscalationSelfConsistencyRuns != null || + input.guardrailEscalationModel != null || + input.guardrailEscalationProvider != null; + const guardrailEscalationCleared = + guardrailHit && reviewGood && escalationConfigured && input.guardrailEscalationOnCleanReview === "proceed"; const disposition = derivePrDisposition({ mergeableState: input.pr.mergeableState, reviewGood, guardrailHit, + guardrailEscalationCleared, migrationCollisionHold: input.migrationCollisionHold !== undefined, unlinkedIssueMatchHold: input.unlinkedIssueMatchHold !== undefined, priorityEligibilityHold: input.priorityEligibilityHold !== undefined, @@ -1214,7 +1240,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // still separately wants manualReview) — without this, the cleanup would remove a label the fallback is // about to re-add later in this same pass. See its own doc comment at the (former) point of use below. const manualHoldReason = - guardrailHit + guardrailHit && !guardrailEscalationCleared ? `verdict=${conclusion}; ${guardrailReason}` : ciUnverified ? "CI could not be verified" @@ -1228,7 +1254,13 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // separate from review_state_label so a one-shot repo can opt into `manual-review` without also enabling the // older ready/changes disposition labels. It is authorized by merge autonomy because it only fires when a // would-merge PR is held for a human by a guardrail. - if (reviewGood && guardrailHit && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) { + // + // `!guardrailEscalationCleared` is load-bearing (#9808/#9869): the label announces a HOLD, and a cleared + // escalation means there is no hold — the escalated review vouched for the guarded path and the merge below + // proceeds. Without this term the planner emitted both in the same pass, merging the PR while also tagging it + // for a human to look at, which is self-contradictory and leaves a manual-review label sitting on merged PRs + // in exactly the full-autonomy mode this feature exists to enable. + if (reviewGood && guardrailHit && !guardrailEscalationCleared && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) { actions.push({ actionClass: "label", autonomyClass: "merge", diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index c8e99da2c..0ce087443 100644 --- a/src/settings/pr-disposition.ts +++ b/src/settings/pr-disposition.ts @@ -90,6 +90,13 @@ export type PrDispositionInput = Record & { * held anyway (observed on JSONbored/loopover#9816, reason "mergeable_state is unstable — non-required * check(s) not passing: Contributor trust"). Set ONLY when nothing else adverse was seen. */ unstableExplainedByIgnoredChecks?: boolean | undefined; + /** #9808 second half: the `guardrailHit` hold was CLEARED by a clean escalated review — see `releasedHolds` + * in derivePrDisposition. Resolved by the caller (agent-actions.ts), which requires ALL of: + * `guardrailEscalation.onCleanReview: proceed` configured, at least one escalation knob actually set (the + * extra scrutiny must exist before it can vouch for anything), and reviewGood (gate success — which folds in + * the AI verdict's blockers — plus green CI). Releases ONLY the guardrail term; every other hold in + * MERGE_HOLD_INPUTS is untouched. Default false ⇒ byte-identical to the pre-#9808 behaviour. */ + guardrailEscalationCleared?: boolean | undefined; }; export type PrDisposition = { @@ -120,7 +127,15 @@ export function derivePrDisposition(input: PrDispositionInput): PrDisposition { const unstableHolds = mergeable === "unstable" && input.unstableExplainedByIgnoredChecks !== true; // Derived from MERGE_HOLD_INPUTS, so a hold declared in that table is folded in by construction and a // new one can never be added-but-not-honoured. - const heldForManualReview = MERGE_HOLD_INPUT_KEYS.some((key) => input[key] === true) || unstableHolds; + // + // #9808: a hold may also be explicitly RELEASED. A guardrail hit whose escalated review came back clean no + // longer summons a human -- the guarded path is protected by the escalated review instead of by a queue. + // Expressed as a release map rather than by dropping the term, so the table stays the single declaration of + // what holds and this stays the single declaration of what can lift one. + const releasedHolds: Partial> = { + guardrailHit: input.guardrailEscalationCleared === true, + }; + const heldForManualReview = MERGE_HOLD_INPUT_KEYS.some((key) => input[key] === true && releasedHolds[key] !== true) || unstableHolds; const heldForUnstableMergeState = unstableHolds; const wouldApprove = input.reviewGood && !heldForManualReview && mergeable !== "conflict"; const wouldMerge = input.reviewGood && !heldForManualReview && mergeable === "clean"; diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index fd084525f..c11e436ec 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -516,6 +516,7 @@ function applyGateConfigOverrides(effective: RepositorySettings, gate: FocusMani 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.guardrailEscalationOnCleanReview !== null) effective.guardrailEscalationOnCleanReview = gate.guardrailEscalationOnCleanReview; 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 74bf6bb57..b04b6f655 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1135,6 +1135,10 @@ export type RepositorySettings = { guardrailEscalationModel?: string | null | undefined; guardrailEscalationEffort?: "low" | "medium" | "high" | "xhigh" | "max" | null | undefined; guardrailEscalationSelfConsistencyRuns?: number | null | undefined; + /** `gate.guardrailEscalation.onCleanReview` (#9808 second half): `proceed` releases the guardrail hold + * when the escalated review came back clean (gate success + CI green); `hold` (default) keeps a human in + * the loop even then. Fail-closed: `proceed` is inert unless an escalation knob is actually set. */ + guardrailEscalationOnCleanReview?: "hold" | "proceed" | 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/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 690bc45bc..7f720df04 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -66,6 +66,43 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(classes(collision)).not.toContain("merge"); }); + // #9808/#9869: a guardrail hit used to mean ONE thing -- suppress auto-merge and hold for a human -- while + // buying no extra scrutiny at all. With an escalation configured, a CLEAN escalated review can now release + // that hold instead. These two cases are the whole point of the feature and the only place all three + // preconditions (guardrail hit, clean review, escalation configured) hold at once, which is what decides + // whether onCleanReview is even consulted. + it("guardrailEscalation.onCleanReview: proceed RELEASES the guardrail hold when the escalated review is clean (#9808)", () => { + const escalated = { + conclusion: "success" as const, + autonomy: { merge: "auto" as const, review_state_label: "auto" as const }, + manualReviewLabel: "human-review", + changedPaths: ["src/settings/agent-actions.ts"], + hardGuardrailGlobs: ["src/settings/**"], + guardrailEscalationEffort: "high", + pr: { labels: [], mergeableState: "clean" as const, reviewDecision: "APPROVED" as const }, + }; + + // proceed: the escalated review vouched for the guarded path, so the PR merges and is NOT parked for a human. + const proceed = planAgentMaintenanceActions(input({ ...escalated, guardrailEscalationOnCleanReview: "proceed" })); + expect(proceed.some((a) => a.actionClass === "label" && a.label === "human-review")).toBe(false); + expect(classes(proceed)).toContain("merge"); + + // hold (the default): identical input, opposite outcome -- the hold stands and the label goes on. Asserting + // both against the SAME input is what proves onCleanReview is the deciding term rather than something else + // in the escalated shape. + const hold = planAgentMaintenanceActions(input({ ...escalated, guardrailEscalationOnCleanReview: "hold" })); + expect(hold.some((a) => a.actionClass === "label" && a.label === "human-review")).toBe(true); + expect(classes(hold)).not.toContain("merge"); + + // proceed with NO escalation configured must not release: "proceed" is a promise about an ESCALATED review, + // so honouring it without one would hand guarded paths a weaker gate than they had before the feature. + const unconfigured = planAgentMaintenanceActions( + input({ ...escalated, guardrailEscalationEffort: null, guardrailEscalationOnCleanReview: "proceed" }), + ); + expect(unconfigured.some((a) => a.actionClass === "label" && a.label === "human-review")).toBe(true); + expect(classes(unconfigured)).not.toContain("merge"); + }); + it("uses manualReviewLabel for manual holds without enabling ready/changes review-state labels", () => { const guarded = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, manualReviewLabel: "human-review", readyToMergeLabel: null, changesRequestedLabel: null, changedPaths: ["src/settings/agent-actions.ts"], hardGuardrailGlobs: ["src/settings/**"], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); expect(guarded.some((a) => a.actionClass === "label" && a.label === "human-review" && a.autonomyClass === "merge")).toBe(true); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 98abeea6c..4880c21c2 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -347,6 +347,7 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { guardrailEscalationModel: "guardrailEscalation:", guardrailEscalationEffort: "guardrailEscalation:", guardrailEscalationSelfConsistencyRuns: "guardrailEscalation:", + guardrailEscalationOnCleanReview: "guardrailEscalation:", ignoredCheckRuns: "ignoredCheckRuns:", aiJudgmentBlockersMode: "aiJudgmentBlockers:", copycatMode: "copycat:", @@ -446,6 +447,7 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { guardrailEscalationModel: "guardrailEscalation:", guardrailEscalationEffort: "guardrailEscalation:", guardrailEscalationSelfConsistencyRuns: "guardrailEscalation:", + guardrailEscalationOnCleanReview: "guardrailEscalation:", } satisfies Record, string>; it.each(Object.entries(SETTINGS_FIELD_TOKENS))("documents settings.%s", (_field, token) => { @@ -1012,7 +1014,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, 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, priorityEligibilityWindowMinutes: 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, guardrailEscalationOnCleanReview: 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, priorityEligibilityWindowMinutes: 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 }, @@ -1206,7 +1208,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, 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, priorityEligibilityWindowMinutes: 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, guardrailEscalationOnCleanReview: 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, priorityEligibilityWindowMinutes: 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)", () => { @@ -1672,6 +1674,22 @@ describe("parseFocusManifest gate config", () => { expect(parseFocusManifest({ gate }).gate.present).toBe(true); } + // #9808/#9869: onCleanReview is the field that decides whether a CLEAN escalated review RELEASES the + // guardrail hold or merely records it, so it must survive the whole path -- parse, presence, round-trip, + // and (the gap this closes) the RESOLVE step that folds it into effective settings. Parsing it correctly + // and then dropping it during resolution would silently restore the old always-hold behaviour, with the + // manifest still reading as if full autonomy were configured. + const clean = parseFocusManifest({ gate: { guardrailEscalation: { effort: "high", onCleanReview: "proceed" } } }); + expect(clean.gate.guardrailEscalationOnCleanReview).toBe("proceed"); + expect(clean.gate.present).toBe(true); + expect(resolveEffectiveSettings({} as RepositorySettings, clean).guardrailEscalationOnCleanReview).toBe("proceed"); + expect(parseFocusManifest({ gate: gateConfigToJson(clean.gate) }).gate).toEqual(clean.gate); + // onCleanReview ALONE flips presence, like each of its six siblings above. + expect(parseFocusManifest({ gate: { guardrailEscalation: { onCleanReview: "hold" } } }).gate.present).toBe(true); + // Left unset it stays null, so the resolver leaves effective settings untouched and the default (hold) + // stands -- the nullish arm of the same line, which is what a repo that never configured this gets. + expect(resolveEffectiveSettings({} as RepositorySettings, parseFocusManifest({ gate: { guardrailEscalation: { effort: "high" } } })).guardrailEscalationOnCleanReview).toBeUndefined(); + // 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"); diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts index 70efdbcc0..031f57ca5 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/test/unit/pr-disposition-invariants.test.ts @@ -218,3 +218,46 @@ describe("unstable explained only by an IGNORED check (#9810 follow-up)", () => expect(d.wouldApprove).toBe(true); }); }); + +describe("guardrail hold released by a clean escalated review (#9808 second half)", () => { + const base = { + reviewGood: true, guardrailHit: true, migrationCollisionHold: false, unlinkedIssueMatchHold: false, + advisoryCheckHold: false, priorityEligibilityHold: false, unlinkedIssueMatchCloseWithoutCloseActing: false, + mergeableState: "clean", + }; + + it("REGRESSION: cleared ⇒ the PR proceeds instead of summoning a human", () => { + // The gap this closes: #9821 shipped the escalated review, but the disposition still held every guarded + // PR unconditionally — the 74 held PRs / 14 days kept landing on the maintainer, just with better + // reviews attached. Full-autonomy mode releases the hold when the escalation came back clean. + const d = derivePrDisposition({ ...base, guardrailEscalationCleared: true }); + expect(d.heldForManualReview).toBe(false); + expect(d.wouldApprove).toBe(true); + expect(d.wouldMerge).toBe(true); + }); + + it("INVARIANT: default (flag absent/false) is byte-identical to today — hold", () => { + expect(derivePrDisposition(base).heldForManualReview).toBe(true); + expect(derivePrDisposition({ ...base, guardrailEscalationCleared: false }).heldForManualReview).toBe(true); + }); + + it("INVARIANT: cleared releases ONLY the guardrail term — every other hold still holds", () => { + for (const extra of [ + { migrationCollisionHold: true }, + { unlinkedIssueMatchHold: true }, + { advisoryCheckHold: true }, + { mergeableState: "unstable" }, + ]) { + const d = derivePrDisposition({ ...base, guardrailEscalationCleared: true, ...extra }); + expect(d.heldForManualReview, JSON.stringify(extra)).toBe(true); + } + }); + + it("INVARIANT: cleared without reviewGood cannot merge — the release rides ON the clean verdict", () => { + // The caller only sets cleared when reviewGood, but the disposition must not trust that: a red gate or + // CI still blocks even if the flag were mis-set upstream. + const d = derivePrDisposition({ ...base, reviewGood: false, guardrailEscalationCleared: true }); + expect(d.wouldApprove).toBe(false); + expect(d.wouldMerge).toBe(false); + }); +});