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.

Author: JSONbored

src/settings/agent-actions.ts

@@ -1248,7 +1248,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",

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);

test/unit/focus-manifest.test.ts

@@ -1673,6 +1673,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");