From 228070281b3288227bc0b2fa7dd01e2176f91368 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:57:11 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat(gate):=20guardrailEscalation.onCleanRe?= =?UTF-8?q?view=20=E2=80=94=20release=20the=20guardrail=20hold=20when=20th?= =?UTF-8?q?e=20escalated=20review=20is=20clean?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second half of #9808, as an explicit MODE rather than a policy flip. review -- but the disposition still held it unconditionally: guardrailHit sat in heldForManualReview regardless of what the escalated review found. The 74 held PRs / 14 days kept landing on the maintainer, just with better reviews attached. gate.guardrailEscalation.onCleanReview: hold (default) today's behavior exactly -- human-in-the-loop proceed a clean escalated review releases the guardrail hold and the normal approve/merge path continues -- full-autonomy mode Because it is a manifest field it layers global -> per-repo like everything else, so repos can be flipped one at a time and the mode is reversible without a deploy. Fail-closed on every axis: - `proceed` is inert unless at least one escalation knob is actually SET: the release is justified by extra scrutiny, so absent scrutiny nothing vouches. - the release requires reviewGood -- gate success (which folds in the AI verdict's blockers) AND green CI -- and the disposition re-checks that independently of the caller's flag. - only the guardrail term is released: migration collisions, unlinked-issue holds, advisory-check holds, and unstable merge states all still hold. - the manual-hold reason no longer claims "guarded path -> manual review" for a cleared PR. --- .loopover.yml.example | 8 ++++ apps/loopover-ui/public/openapi.json | 9 ++++ config/examples/loopover.full.yml | 8 ++++ packages/loopover-contract/src/api-schemas.ts | 1 + .../loopover-engine/src/focus-manifest.ts | 15 ++++++- .../src/types/manifest-deps-types.ts | 4 ++ .../test/focus-manifest-review-knobs.test.ts | 15 +++++++ scripts/check-docs-drift.ts | 1 + src/openapi/schemas.ts | 1 + src/queue/processors.ts | 7 ++++ src/settings/agent-actions.ts | 28 ++++++++++++- src/settings/pr-disposition.ts | 17 +++++++- src/signals/focus-manifest.ts | 1 + src/types.ts | 4 ++ test/unit/focus-manifest.test.ts | 6 ++- test/unit/pr-disposition-invariants.test.ts | 42 +++++++++++++++++++ 16 files changed, 162 insertions(+), 5 deletions(-) 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 88e184881..2a6f8222f 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..984c1bdc5 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" 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/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 98abeea6c..a95a53348 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)", () => { diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts index 70efdbcc0..e51e9b624 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/test/unit/pr-disposition-invariants.test.ts @@ -218,3 +218,45 @@ 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, 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); + }); +}); From 59b7643ebefe02478d0ab7848e735c988cb39d2b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:19:18 -0700 Subject: [PATCH 2/2] fix(gate): stop labelling a PR for manual review in the same pass that merges it The manual-review label announces a HOLD. Its emit condition checked `guardrailHit` alone, so once a clean escalated review released that hold (guardrailEscalationCleared), the planner emitted both in one pass: the merge AND the label telling a human to come look at it. Self-contradictory, and it leaves a manual-review label sitting on merged PRs in precisely the full-autonomy mode the feature exists to enable. Also closes the two patch-coverage gaps this PR still carried, both of them the "test both sides" class: - agent-actions: no test made guardrailHit, reviewGood and escalationConfigured true at once, so the fourth conjunct -- `onCleanReview === "proceed"`, the term the whole feature turns on -- was never evaluated at all. Now asserted in both directions against identical input, plus the case where "proceed" is set with NO escalation configured, which must NOT release (that promise is about an escalated review; honouring it without one would leave guarded paths weaker than before the feature). - focus-manifest: onCleanReview was covered through parse but not through resolveEffectiveSettings. Parsing it and dropping it during resolution would silently restore always-hold while the manifest still read as configured for full autonomy. --- src/settings/agent-actions.ts | 8 ++++- test/unit/agent-actions.test.ts | 37 +++++++++++++++++++++ test/unit/focus-manifest.test.ts | 16 +++++++++ test/unit/pr-disposition-invariants.test.ts | 3 +- 4 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 984c1bdc5..40f5dacd0 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1254,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/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 a95a53348..4880c21c2 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1674,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 e51e9b624..031f57ca5 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/test/unit/pr-disposition-invariants.test.ts @@ -222,7 +222,8 @@ describe("unstable explained only by an IGNORED check (#9810 follow-up)", () => describe("guardrail hold released by a clean escalated review (#9808 second half)", () => { const base = { reviewGood: true, guardrailHit: true, migrationCollisionHold: false, unlinkedIssueMatchHold: false, - advisoryCheckHold: false, unlinkedIssueMatchCloseWithoutCloseActing: false, mergeableState: "clean", + advisoryCheckHold: false, priorityEligibilityHold: false, unlinkedIssueMatchCloseWithoutCloseActing: false, + mergeableState: "clean", }; it("REGRESSION: cleared ⇒ the PR proceeds instead of summoning a human", () => {