diff --git a/.gitignore b/.gitignore index 7cf61abcae..0685cfe229 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ -node_modules/ +node_modules dist/ dist-ssr/ .output diff --git a/src/env.d.ts b/src/env.d.ts index 2399ec1a9d..2b32af5b6e 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -15,6 +15,11 @@ declare global { /** Convergence (infra): Browser Rendering binding for visual (before/after screenshot) capture. Optional — * absent ⇒ no visual capture. Unused until the per-module wiring chunk; an unbound deploy is inert. */ BROWSER?: Fetcher; + /** Convergence (infra): the shared REVIEW_CONFIG KV (reviewbot's per-repo config, keyed by repo slug). + * The converged auto-maintain path resolves each repo's `hardGuardrailGlobs` from it so guarded paths + * force MANUAL review (no auto-merge / auto-close). Optional — absent ⇒ the conservative + * DEFAULT_CRUCIAL_GUARDRAIL_GLOBS fallback applies (CI workflows + scripts still guarded). */ + REVIEW_CONFIG?: KVNamespace; /** TODO (convergence follow-up): a per-PR LOCK Durable Object (`SubmissionLock` mutex) is a separate, * more-involved sub-task — it needs the ported DO class + its own migration tag, not just a binding here. * Deliberately NOT declared in this chunk; the review path keeps its current concurrency behavior. */ diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 807d9d799d..26071810b9 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -160,6 +160,7 @@ import { buildReviewGroundingText, isGroundingEnabled } from "../review/groundin import { buildReviewRagContext, isRagEnabled } from "../review/rag-wire"; import { isReputationEnabled, recordReputationOutcome, shouldSkipAiForReputation } from "../review/reputation-wire"; import { isConvergenceRepoAllowed } from "../review/cutover-gate"; +import { loadHardGuardrailGlobs } from "../review/guardrail-config"; import { isOpsEnabled, runOpsAlerts } from "../review/ops-wire"; import { isSelfTuneEnabled, runSelfTune } from "../review/selftune-wire"; import { recordNativeGateDecision } from "../review/parity-wire"; @@ -480,12 +481,26 @@ async function maybeRunAgentMaintenance( if (pr.state !== "open") return; if (!gate) return; + // Convergence safety: feed the planner the PR's changed paths + the repo's hard-guardrail globs so guarded + // paths force manual review, and flag owner-authored PRs so they are never auto-closed (standing rule). + const [changedFiles, hardGuardrailGlobs] = await Promise.all([ + listPullRequestFiles(env, repoFullName, pr.number), + loadHardGuardrailGlobs(env, repoFullName), + ]); + const changedPaths = changedFiles.map((file) => file.path).filter((path) => path.length > 0); + const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")) : ""; + const authorLogin = pr.authorLogin ?? ""; + const authorIsOwner = authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase(); + const planned = planAgentMaintenanceActions({ conclusion: gate.conclusion, blockerTitles: gate.blockers.map((blocker) => blocker.title), autonomy: settings.autonomy, autoMaintain: settings.autoMaintain, slopGateMinScore: settings.slopGateMinScore, + changedPaths, + hardGuardrailGlobs, + authorIsOwner, pr: { mergeableState: pr.mergeableState, reviewDecision: pr.reviewDecision, diff --git a/src/review/guardrail-config.ts b/src/review/guardrail-config.ts new file mode 100644 index 0000000000..d89794183f --- /dev/null +++ b/src/review/guardrail-config.ts @@ -0,0 +1,36 @@ +import type { JsonValue } from "../types"; + +// Per-repo hard-guardrail path globs (paths that force MANUAL review — no auto-merge / no auto-close). +// +// Convergence note: gittensory does not have its own per-repo guardrail config surface, but reviewbot already +// stores carefully-tuned globs per repo in the shared REVIEW_CONFIG KV (keyed by repo slug, e.g. "gittensory" +// / "awesome-claude" / "metagraphed"). That KV is the established home for private, runtime-editable operator +// tuning, so the converged auto-maintain path reads its guardrail globs from there too — no redeploy needed +// to retune, and the same KV survives reviewbot's decommission. + +// Conservative cross-repo fallback when a repo has no KV-configured globs: CI workflows + build/policy scripts +// are universally sensitive (the awesome-claude #4196 incident class). Fail-SAFE — a config miss still guards +// these, it never opens the gate wide. +export const DEFAULT_CRUCIAL_GUARDRAIL_GLOBS = [".github/workflows/**", "scripts/**"]; + +function asNonEmptyStringArray(value: unknown): string[] | null { + if (!Array.isArray(value)) return null; + const out = value.filter((entry): entry is string => typeof entry === "string" && entry.length > 0); + return out.length > 0 ? out : null; +} + +/** + * Resolve a repo's hard-guardrail path globs from the shared REVIEW_CONFIG KV (key = repo slug). Falls back to + * DEFAULT_CRUCIAL_GUARDRAIL_GLOBS when the binding / key / field is absent or malformed — fail-SAFE and never + * throws (the auto-maintain trigger is best-effort and must not be sunk by a config read). + */ +export async function loadHardGuardrailGlobs(env: Env, repoFullName: string): Promise { + const slug = repoFullName.includes("/") ? repoFullName.slice(repoFullName.indexOf("/") + 1) : repoFullName; + if (!env.REVIEW_CONFIG) return DEFAULT_CRUCIAL_GUARDRAIL_GLOBS; + try { + const config = (await env.REVIEW_CONFIG.get(slug, "json")) as { hardGuardrailGlobs?: JsonValue } | null; + return asNonEmptyStringArray(config?.hardGuardrailGlobs) ?? DEFAULT_CRUCIAL_GUARDRAIL_GLOBS; + } catch { + return DEFAULT_CRUCIAL_GUARDRAIL_GLOBS; + } +} diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index e73575cd90..232e309c78 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1,6 +1,7 @@ import type { AgentActionClass, AutoMaintainPolicy, AutoMergeMethod, AutonomyPolicy } from "../types"; import type { GateCheckConclusion } from "../rules/advisory"; import { DEFAULT_AUTO_MAINTAIN_POLICY, autonomyRequiresApproval, isActingAutonomyLevel, resolveAutonomy } from "./autonomy"; +import { changedPathsHittingGuardrail } from "../signals/change-guardrail"; // High-slop threshold default when a repo hasn't set slopGateMinScore (mirrors the gate's `high` band). const DEFAULT_SLOP_GATE_MIN_SCORE = 60; @@ -35,6 +36,14 @@ export type AgentActionPlanInput = { // Optional so the trigger can pass raw repo settings; both fall back to conservative defaults here. autoMaintain?: AutoMaintainPolicy | undefined; slopGateMinScore?: number | null | undefined; + // Convergence safety (hard-guardrail port, #4196 incident class): the PR's changed paths + the repo's + // hard-guardrail globs. Any changed path matching a guardrail glob forces MANUAL review — gittensory will + // neither auto-merge, auto-approve, nor auto-close such a PR; it falls through to a human. + changedPaths: string[]; + hardGuardrailGlobs: string[]; + // True when the PR author is the repo owner (e.g. JSONbored). Standing rule: owner PRs are NEVER + // auto-closed. They may still auto-merge when clean + passing. + authorIsOwner: boolean; pr: { mergeableState?: string | null | undefined; reviewDecision?: string | null | undefined; @@ -75,6 +84,9 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne const blocking = isBlocking(input.conclusion); 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; // 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. @@ -94,7 +106,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne reason: `${input.blockerTitles.length || 1} blocker(s)`, reviewBody: `Gittensory requests changes — the gate is not yet satisfied:\n\n${summary}`, }); - } else if (passing && acting("approve") && input.pr.reviewDecision !== "APPROVED") { + } else if (passing && acting("approve") && !guardrailHit && input.pr.reviewDecision !== "APPROVED") { actions.push({ actionClass: "approve", requiresApproval: approval("approve"), @@ -105,7 +117,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // 3) disposition — merge a clean, approved, passing PR; otherwise close clear noise. Mutually exclusive. const mergeableClean = input.pr.mergeableState === "clean"; - const canMerge = passing && acting("merge") && mergeableClean && approvalsSatisfied; + const canMerge = passing && acting("merge") && mergeableClean && approvalsSatisfied && !guardrailHit; if (canMerge) { actions.push({ actionClass: "merge", @@ -113,7 +125,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne reason: `gate passed, mergeable, ${autoMaintain.requireApprovals} approval(s) satisfied`, mergeMethod: autoMaintain.mergeMethod, }); - } else if (acting("close") && !passing) { + } else if (acting("close") && !passing && !guardrailHit && !input.authorIsOwner) { const noiseReasons: string[] = []; if (input.pr.slopRisk != null && input.pr.slopRisk >= slopGateMinScore) noiseReasons.push(`slop score ${input.pr.slopRisk} ≥ ${slopGateMinScore}`); if ((input.pr.linkedDuplicateCount ?? 0) > 0) noiseReasons.push("duplicate of another open PR"); diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts new file mode 100644 index 0000000000..5ce3f0880b --- /dev/null +++ b/src/signals/change-guardrail.ts @@ -0,0 +1,41 @@ +// Convergence safety: the hard-guardrail path check for the auto-maintain layer (#778). Changed paths that +// match a repo's hardGuardrailGlobs force MANUAL review — gittensory must never auto-merge OR auto-close a PR +// that touches a guarded path (scoring / auth / CI workflows / policy scripts, etc.). Ported verbatim from +// reviewbot core/change-classifier.ts — the mechanism that prevents the awesome-claude #4196 incident class +// (a weakened policy script auto-merging because its path wasn't guarded). Pure + dependency-free. + +/** Convert a path glob (`*` matches within a segment, `**` matches across `/`) to an anchored RegExp. */ +function globToRegExp(glob: string): RegExp { + let re = ""; + for (let i = 0; i < glob.length; i += 1) { + const c = glob.charAt(i); + if (c === "*") { + if (glob.charAt(i + 1) === "*") { + re += ".*"; + i += 1; + if (glob.charAt(i + 1) === "/") i += 1; // `**/` also matches zero segments + } else { + re += "[^/]*"; + } + } else if (/[.+?^${}()|[\]\\]/.test(c)) { + re += `\\${c}`; + } else { + re += c; + } + } + return new RegExp(`^${re}$`); +} + +/** True if `path` matches any of the globs (`*` within a segment, `**` across `/`). */ +export function matchesAny(path: string, globs: string[]): boolean { + return globs.some((g) => globToRegExp(g).test(path)); +} + +/** + * The changed paths (if any) that trip a hard guardrail. A non-empty result means the PR touches a guarded + * path and MUST fall through to a human — gittensory may neither auto-merge nor auto-close it. Pure. + */ +export function changedPathsHittingGuardrail(changedPaths: string[], hardGuardrailGlobs: string[]): string[] { + if (hardGuardrailGlobs.length === 0) return []; + return changedPaths.filter((path) => path.length > 0 && matchesAny(path, hardGuardrailGlobs)); +} diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index d505e4dc2e..4391c37adf 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -8,6 +8,9 @@ function input(overrides: Partial & { conclusion: GateChec autonomy: {}, autoMaintain: { requireApprovals: 1, mergeMethod: "squash" }, slopGateMinScore: 60, + changedPaths: [], + hardGuardrailGlobs: [], + authorIsOwner: false, pr: { labels: [] }, ...overrides, }; @@ -73,11 +76,11 @@ describe("planAgentMaintenanceActions (#778)", () => { it("applies conservative defaults when autoMaintain / slopGateMinScore are omitted", () => { // no autoMaintain → requireApprovals defaults to 1 → a clean passing PR without APPROVED does NOT merge - expect(classes(planAgentMaintenanceActions({ conclusion: "success", blockerTitles: [], autonomy: { merge: "auto" }, pr: { labels: [], mergeableState: "clean" } }))).not.toContain("merge"); + expect(classes(planAgentMaintenanceActions({ conclusion: "success", blockerTitles: [], autonomy: { merge: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, pr: { labels: [], mergeableState: "clean" } }))).not.toContain("merge"); // no slopGateMinScore → defaults to 60 → slopRisk 70 counts as noise and closes - expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, pr: { labels: [], slopRisk: 70 } }))).toContain("close"); + expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, pr: { labels: [], slopRisk: 70 } }))).toContain("close"); // ...and slopRisk 50 is below the default → no close - expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, pr: { labels: [], slopRisk: 50 } }))).not.toContain("close"); + expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, pr: { labels: [], slopRisk: 50 } }))).not.toContain("close"); }); it("closes clear noise (high slop or duplicate) on a non-passing verdict, and never closes a passing PR", () => { @@ -112,4 +115,51 @@ describe("planAgentMaintenanceActions (#778)", () => { ); expect(classes(plan)).toEqual(["label", "approve", "merge"]); }); + + describe("hard-guardrail: a changed path matching a guardrail glob forces manual review", () => { + const guarded = { changedPaths: ["src/scoring/model.ts"], hardGuardrailGlobs: ["src/scoring/**", "scripts/**"] }; + + it("does NOT auto-merge a clean+approved+passing PR that touches a guarded path", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, ...guarded, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).not.toContain("merge"); + }); + + it("does NOT auto-close a noisy failing PR that touches a guarded path", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], ...guarded, pr: { labels: [], slopRisk: 95 } }))); + expect(plan).not.toContain("close"); + }); + + it("does NOT auto-approve a passing PR that touches a guarded path (so it can't later satisfy a merge)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto" }, ...guarded, pr: { labels: [] } }))); + expect(plan).not.toContain("approve"); + }); + + it("still labels a guarded PR (the reversible action is unaffected — it just falls to a human)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto", merge: "auto" }, ...guarded, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).toContain("label"); + expect(plan).not.toContain("merge"); + }); + + it("still auto-merges when the changed paths do NOT match any guardrail glob", () => { + 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"); + }); + }); + + describe("owner-PR guard: never auto-close the repo owner's own PRs", () => { + it("does NOT auto-close a noisy failing PR authored by the repo owner", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], authorIsOwner: true, pr: { labels: [], slopRisk: 95 } }))); + expect(plan).not.toContain("close"); + }); + + it("DOES auto-close the same noisy PR when the author is not the owner", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], authorIsOwner: false, pr: { labels: [], slopRisk: 95 } }))); + expect(plan).toContain("close"); + }); + + it("still auto-merges a clean+approved owner PR (the guard blocks only close, never merge)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, authorIsOwner: true, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).toContain("merge"); + }); + }); }); diff --git a/test/unit/change-guardrail.test.ts b/test/unit/change-guardrail.test.ts new file mode 100644 index 0000000000..b18205d2c8 --- /dev/null +++ b/test/unit/change-guardrail.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from "vitest"; +import { changedPathsHittingGuardrail, matchesAny } from "../../src/signals/change-guardrail"; + +describe("change-guardrail glob matching", () => { + it("`**` matches across path separators (a guarded dir guards its whole subtree)", () => { + expect(matchesAny("scripts/foo/bar.sh", ["scripts/**"])).toBe(true); + expect(matchesAny("scripts/build.mjs", ["scripts/**"])).toBe(true); + expect(matchesAny(".github/workflows/ci.yml", [".github/workflows/**"])).toBe(true); + expect(matchesAny("src/scoring/deep/nested/model.ts", ["src/scoring/**"])).toBe(true); + }); + + it("`**/` also matches zero segments (the dir root itself)", () => { + expect(matchesAny("packages/index.ts", ["packages/**"])).toBe(true); + }); + + it("`*` matches only within a single segment", () => { + expect(matchesAny("src/auth.ts", ["src/*.ts"])).toBe(true); + expect(matchesAny("src/auth/session.ts", ["src/*.ts"])).toBe(false); + }); + + it("does not match unrelated paths", () => { + expect(matchesAny("docs/readme.md", ["scripts/**", "src/scoring/**"])).toBe(false); + expect(matchesAny("src/ui/button.tsx", ["src/scoring/**", "src/auth/**"])).toBe(false); + }); + + it("changedPathsHittingGuardrail returns the offending paths (empty globs ⇒ no hits)", () => { + const globs = ["src/scoring/**", "scripts/**"]; + expect(changedPathsHittingGuardrail(["docs/a.md", "src/scoring/x.ts", "scripts/y.mjs"], globs)).toEqual(["src/scoring/x.ts", "scripts/y.mjs"]); + expect(changedPathsHittingGuardrail(["docs/a.md", "src/ui/b.tsx"], globs)).toEqual([]); + expect(changedPathsHittingGuardrail(["src/scoring/x.ts"], [])).toEqual([]); + }); +}); diff --git a/test/unit/guardrail-config.test.ts b/test/unit/guardrail-config.test.ts new file mode 100644 index 0000000000..fdb2a91a6d --- /dev/null +++ b/test/unit/guardrail-config.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it, vi } from "vitest"; +import { DEFAULT_CRUCIAL_GUARDRAIL_GLOBS, loadHardGuardrailGlobs } from "../../src/review/guardrail-config"; + +function envWith(get: (key: string, type: string) => Promise): Env { + return { REVIEW_CONFIG: { get } } as unknown as Env; +} + +describe("loadHardGuardrailGlobs", () => { + it("returns the conservative default when REVIEW_CONFIG is unbound", async () => { + expect(await loadHardGuardrailGlobs({} as Env, "JSONbored/gittensory")).toEqual(DEFAULT_CRUCIAL_GUARDRAIL_GLOBS); + }); + + it("reads globs from KV keyed by the repo slug (owner stripped)", async () => { + const get = vi.fn().mockResolvedValue({ hardGuardrailGlobs: ["src/scoring/**", "scripts/**"] }); + const globs = await loadHardGuardrailGlobs(envWith(get), "JSONbored/gittensory"); + expect(globs).toEqual(["src/scoring/**", "scripts/**"]); + expect(get).toHaveBeenCalledWith("gittensory", "json"); + }); + + it("falls back to the default when the field is absent, null, or empty", async () => { + expect(await loadHardGuardrailGlobs(envWith(async () => ({})), "o/r")).toEqual(DEFAULT_CRUCIAL_GUARDRAIL_GLOBS); + expect(await loadHardGuardrailGlobs(envWith(async () => null), "o/r")).toEqual(DEFAULT_CRUCIAL_GUARDRAIL_GLOBS); + expect(await loadHardGuardrailGlobs(envWith(async () => ({ hardGuardrailGlobs: [] })), "o/r")).toEqual(DEFAULT_CRUCIAL_GUARDRAIL_GLOBS); + }); + + it("drops non-string entries and keeps the valid globs", async () => { + const globs = await loadHardGuardrailGlobs(envWith(async () => ({ hardGuardrailGlobs: [123, "scripts/**", ""] })), "o/r"); + expect(globs).toEqual(["scripts/**"]); + }); + + it("fails safe to the default when the KV read throws", async () => { + const globs = await loadHardGuardrailGlobs( + envWith(async () => { + throw new Error("kv down"); + }), + "o/r", + ); + expect(globs).toEqual(DEFAULT_CRUCIAL_GUARDRAIL_GLOBS); + }); + + it("uses the whole name as the slug when there is no owner prefix", async () => { + const get = vi.fn().mockResolvedValue({ hardGuardrailGlobs: ["a/**"] }); + await loadHardGuardrailGlobs(envWith(get), "soloname"); + expect(get).toHaveBeenCalledWith("soloname", "json"); + }); +}); diff --git a/wrangler.jsonc b/wrangler.jsonc index e77008dd3b..c7148ef5dc 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -143,6 +143,16 @@ "browser": { "binding": "BROWSER", }, + // Convergence: the shared REVIEW_CONFIG KV (reviewbot's per-repo config, keyed by repo slug). The converged + // auto-maintain path reads each repo's hardGuardrailGlobs from it so guarded paths (scoring / auth / CI / + // policy scripts) force MANUAL review — never auto-merge/auto-close. Absent ⇒ DEFAULT_CRUCIAL_GUARDRAIL_GLOBS + // fallback (CI workflows + scripts still guarded). This KV survives reviewbot's decommission. + "kv_namespaces": [ + { + "binding": "REVIEW_CONFIG", + "id": "aed9890dbd3f4f73bf46b43d7d0478d7", + }, + ], // TODO (convergence follow-up): a per-PR LOCK Durable Object (`SubmissionLock` mutex) so concurrent // webhook deliveries for the same PR serialize. That is a separate, more-involved sub-task — it needs a DO // class (ported) + its own `migrations` tag (`new_sqlite_classes: ["SubmissionLock"]`) + a `durable_objects`