From 9f9be2eb0c042bf8d2110618d79459c3142a78f3 Mon Sep 17 00:00:00 2001 From: galuis116 Date: Mon, 22 Jun 2026 12:55:35 -0700 Subject: [PATCH] fix(review): fail auto-maintain guardrail safe on unknown changed paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auto-maintain planner (#1050) suppressed auto-merge/close/approve only when a changed path hit a hard-guardrail glob. But maybeRunAgentMaintenance reads the changed paths from the pull_request_files cache without refreshing, so an empty (fresh PR pre-backfill) or stale (post-synchronize) cache yields changedPaths=[], making guardrailHit=false — letting the automation auto-merge/close a PR that actually touches a guarded path (.github/workflows, scripts, scoring/auth). When guardrails are configured but the changed-file set is unknown (empty), treat it as a hit: we cannot prove the PR is safe, so it must fall through to a human. Repos with no guardrails configured stay permissive. Closes #1061 --- src/settings/agent-actions.ts | 7 ++++++- test/unit/agent-actions.test.ts | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index e3530ac21b..605a78200a 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -99,7 +99,12 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne const passing = input.conclusion === "success"; // A changed path matching a hard guardrail forces manual review: suppress the irreversible dispositions // (merge / close) AND the auto-approve that could later satisfy a merge. label + request_changes still run. - const guardrailHit = changedPathsHittingGuardrail(input.changedPaths, input.hardGuardrailGlobs).length > 0; + // Fail SAFE on UNKNOWN paths: when guardrails are configured but the changed-file set is empty (cache not + // yet/no-longer populated), we cannot prove the PR doesn't touch a guarded path, so treat it as a hit — + // never auto-merge/close a PR whose diff we don't know. Repos with no guardrails stay permissive. + const guardrailHit = + input.hardGuardrailGlobs.length > 0 && + (input.changedPaths.length === 0 || changedPathsHittingGuardrail(input.changedPaths, input.hardGuardrailGlobs).length > 0); // 1) label — reflect the verdict bucket. After the neutral/skipped return above, a non-blocking verdict is // necessarily `success`. Idempotent: skip if the PR already carries the label. diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 22bb626323..7dc3ea380c 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -145,6 +145,23 @@ describe("planAgentMaintenanceActions (#778)", () => { const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, changedPaths: ["docs/readme.md", "src/ui/button.tsx"], hardGuardrailGlobs: ["src/scoring/**", "scripts/**"], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); expect(plan).toContain("merge"); }); + + it("fails SAFE on UNKNOWN paths: with guardrails configured but no changed-file set, suppress merge/close/approve", () => { + // The changed-file cache can be empty (fresh PR before backfill) or stale; we cannot prove the PR + // doesn't touch a guarded path, so it must fall through to a human (label/request_changes still run). + const merge = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto", label: "auto" }, changedPaths: [], hardGuardrailGlobs: ["src/scoring/**", "scripts/**"], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(merge).not.toContain("merge"); + expect(merge).toContain("label"); + const close = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], changedPaths: [], hardGuardrailGlobs: ["src/scoring/**"], pr: { labels: [], slopRisk: 95 } }))); + expect(close).not.toContain("close"); + const approve = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto" }, changedPaths: [], hardGuardrailGlobs: ["src/scoring/**"], pr: { labels: [] } }))); + expect(approve).not.toContain("approve"); + }); + + it("stays permissive on empty paths when the repo has NO guardrails configured (opted out)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, changedPaths: [], hardGuardrailGlobs: [], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).toContain("merge"); + }); }); describe("owner-PR guard: never auto-close the repo owner's own PRs", () => {