diff --git a/packages/loopover-miner/bin/loopover-miner.ts b/packages/loopover-miner/bin/loopover-miner.ts index fcaee91318..b7b7dd21be 100755 --- a/packages/loopover-miner/bin/loopover-miner.ts +++ b/packages/loopover-miner/bin/loopover-miner.ts @@ -16,6 +16,7 @@ import { runMetrics } from "../lib/metrics-cli.js"; import { runPlanCli } from "../lib/plan-store-cli.js"; import { runClaimCli } from "../lib/claim-ledger-cli.js"; import { runPurge } from "../lib/purge-cli.js"; +import { runDenyHooks } from "../lib/deny-hooks-cli.js"; import { runQueueCli } from "../lib/portfolio-queue-cli.js"; import { runOrbExportCli } from "../lib/orb-export.js"; import { runTenantCli } from "../lib/tenant-cli.js"; @@ -168,6 +169,12 @@ if (cliArgs[0] === "purge") { process.exit(runPurge(cliArgs.slice(1))); } +// `deny-hooks` (#8806) is strictly local + offline like `purge` above — it only opens the local synthesis +// store to list/refresh/approve the synthesized guardrail proposals buildAttemptDeps now enforces. +if (cliArgs[0] === "deny-hooks") { + process.exit(runDenyHooks(cliArgs.slice(1))); +} + const packageName = "@loopover/miner"; const packageVersion = resolveMinerVersion(process.env); const upgradeCommand = resolveUpgradeCommand(packageName); diff --git a/packages/loopover-miner/lib/attempt-cli.ts b/packages/loopover-miner/lib/attempt-cli.ts index f880cb99d6..6569a6be64 100644 --- a/packages/loopover-miner/lib/attempt-cli.ts +++ b/packages/loopover-miner/lib/attempt-cli.ts @@ -44,6 +44,8 @@ import { openWorktreeAllocator } from "./worktree-allocator.js"; import type { WorktreeAllocation, WorktreeAllocator } from "./worktree-allocator.js"; import { isValidRepoSegment } from "./repo-clone.js"; import { REJECTION_REASON_AI_USAGE_POLICY_BAN, REJECTION_REASON_OWN_SUBMISSION_REJECTED, resolveOwnOpenPrForIssue, resolveRejectionSignaled } from "./rejection-signal.js"; +import { initDenyHookSynthesisStore } from "./deny-hook-synthesis.js"; +import type { DenyRule } from "@loopover/engine"; import type { resolveRejectionSignaled as ResolveRejectionSignaledFn } from "./rejection-signal.js"; import { cleanupAttemptWorktree, prepareAttemptWorktree } from "./attempt-worktree.js"; import type { @@ -262,14 +264,41 @@ export function parseAttemptArgs(args: string[]): ParsedAttemptArgs { * constructProductionCodingAgentDriver's own contract) -- callers should report that clearly rather than * silently falling back to a driver that could never run. */ +/** + * #8806: maintainer-approved synthesized deny rules finally reach a live consumer. Pre-#8806 the synthesis + * store was written and reviewed but NEVER read at driver construction — every attempt fell back to + * DEFAULT_DENY_RULES, so an operator who approved a synthesized guardrail reasonably (and wrongly) believed + * future attempts respected it. Resolves the repo's effective rules (approved proposals merged over the + * defaults) for the driver's PreToolUse hooks. FAIL-OPEN to undefined (→ the pre-#8806 defaults) on any + * store failure — a guardrail read hiccup must never block an attempt, and the defaults are the historical + * floor, never nothing. `initStore` is an injection seam for tests. + */ +export function resolveAttemptHouseRulesConfig( + repoFullName: string | undefined, + initStore: typeof initDenyHookSynthesisStore = initDenyHookSynthesisStore, +): { rules: readonly DenyRule[]; repoFullName: string } | undefined { + if (!repoFullName) return undefined; + try { + const store = initStore(); + try { + return { rules: store.resolveEffectiveRules(repoFullName), repoFullName }; + } finally { + store.close(); + } + } catch { + return undefined; + } +} + export function buildAttemptDeps( env: Record, - ledgers: { claimLedger: ClaimLedger; eventLedger: EventLedger; attemptLog: AttemptLog; governorLedger: GovernorLedger; nowMs: number }, + ledgers: { claimLedger: ClaimLedger; eventLedger: EventLedger; attemptLog: AttemptLog; governorLedger: GovernorLedger; nowMs: number; repoFullName?: string }, ): AttemptDeps { + const houseRulesConfig = resolveAttemptHouseRulesConfig(ledgers.repoFullName); // AttemptDeps' claimLedger/callback parameter types are looser structural stubs than the real ledgers // (pre-existing .d.ts drift on attempt-runner); cast preserves the same runtime wiring the .js had. return { - driver: constructProductionCodingAgentDriver(env), + driver: constructProductionCodingAgentDriver(env, houseRulesConfig !== undefined ? { houseRulesConfig } : {}), runSlopAssessment: (input) => runSlopAssessment(input as Parameters[0]), appendAttemptLogEvent: (event) => { ledgers.attemptLog.appendAttemptLogEvent(event as Parameters[0]); @@ -520,7 +549,9 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {} let deps; try { const buildDeps = options.buildAttemptDeps ?? buildAttemptDeps; - deps = buildDeps(env, { claimLedger, eventLedger, attemptLog, governorLedger, nowMs }); + // #8806: the target repo threads through so the driver's PreToolUse deny hooks carry the repo's + // maintainer-approved synthesized rules, not only DEFAULT_DENY_RULES. + deps = buildDeps(env, { claimLedger, eventLedger, attemptLog, governorLedger, nowMs, repoFullName: parsed.repoFullName }); } catch (error) { const reason = describeCliError(error); return reportCliFailure( diff --git a/packages/loopover-miner/lib/deny-hooks-cli.ts b/packages/loopover-miner/lib/deny-hooks-cli.ts new file mode 100644 index 0000000000..eab9584727 --- /dev/null +++ b/packages/loopover-miner/lib/deny-hooks-cli.ts @@ -0,0 +1,96 @@ +// `loopover-miner deny-hooks` (#8806): the operator surface the deny-hook synthesis store (#5667) never +// had — which is WHY its guardrails never enforced: refreshProposals had no caller anywhere (no CLI, no +// wiring), so nothing ever populated or consumed the store outside tests. This module closes the operate +// half (list / approve / reject / refresh); buildAttemptDeps (#8806's other half) closes the enforce half +// by resolving the repo's effective rules into every coding-agent driver's PreToolUse hooks. +// +// `refresh` takes its blocker/path history from an explicit `--history ` (an array of +// `{ blockerCodes: string[], changedPaths: string[] }` records) — deliberately NOT auto-sourced: no local +// ledger carries both fields today (prediction-ledger has blockerCodes but no changedPaths), and inventing +// an implicit source here would hide that gap instead of documenting it. Auto-sourcing from the miner's own +// PR-outcome history is the tracked follow-up once a ledger records changed paths alongside blockers. +// Strictly local + offline (like `purge`/`queue`): only the local synthesis SQLite is touched. +import { readFileSync } from "node:fs"; +import { initDenyHookSynthesisStore } from "./deny-hook-synthesis.js"; + +const USAGE = [ + "Usage:", + " loopover-miner deny-hooks list [--json]", + " loopover-miner deny-hooks refresh --history [--json]", + " loopover-miner deny-hooks approve ", + " loopover-miner deny-hooks reject ", +].join("\n"); + +type HistoryRecord = { blockerCodes: string[]; changedPaths: string[] }; + +function parseHistoryFile(path: string): HistoryRecord[] { + const parsed: unknown = JSON.parse(readFileSync(path, "utf8")); + if (!Array.isArray(parsed)) throw new Error("history file must be a JSON array of { blockerCodes, changedPaths } records"); + return parsed as HistoryRecord[]; +} + +export function runDenyHooks(args: string[]): number { + const json = args.includes("--json"); + const positional = args.filter((arg) => !arg.startsWith("--")); + const [subcommand, repoFullName, proposalId] = positional; + if (!subcommand || !repoFullName) { + console.error(USAGE); + return 2; + } + const store = initDenyHookSynthesisStore(); + try { + switch (subcommand) { + case "list": { + const proposals = store.listProposals(repoFullName); + const effective = store.resolveEffectiveRules(repoFullName); + if (json) { + console.log(JSON.stringify({ repoFullName, proposals, effectiveRuleCount: effective.length }, null, 2)); + } else if (proposals.length === 0) { + console.log(`No synthesized proposals for ${repoFullName} (${effective.length} effective rule(s), all defaults).`); + } else { + for (const proposal of proposals) { + console.log(`${proposal.id} [${proposal.status}] ${JSON.stringify(proposal.rule)}`); + } + console.log(`${effective.length} effective rule(s) including defaults — approved proposals enforce on the next attempt.`); + } + return 0; + } + case "refresh": { + const historyFlag = args.indexOf("--history"); + const historyPath = historyFlag !== -1 ? args[historyFlag + 1] : undefined; + if (!historyPath) { + console.error("refresh requires --history \n" + USAGE); + return 2; + } + const proposals = store.refreshProposals(repoFullName, parseHistoryFile(historyPath)); + if (json) { + console.log(JSON.stringify({ repoFullName, proposals }, null, 2)); + } else { + console.log(`${proposals.length} proposal(s) for ${repoFullName} — approve with: loopover-miner deny-hooks approve ${repoFullName} `); + } + return 0; + } + case "approve": + case "reject": { + if (!proposalId) { + console.error(USAGE); + return 2; + } + store.setProposalStatus(repoFullName, proposalId, subcommand === "approve" ? "approved" : "rejected"); + console.log(`${subcommand === "approve" ? "Approved" : "Rejected"} ${proposalId} for ${repoFullName}${subcommand === "approve" ? " — it enforces on the next attempt." : "."}`); + return 0; + } + default: + console.error(USAGE); + return 2; + } + } catch (error) { + // String(error) renders an Error as "Error: " — every throw site here (store methods, + // readFileSync, JSON.parse, parseHistoryFile) throws real Errors, so a two-arm instanceof ternary + // would carry a permanently-unreachable branch. + console.error(String(error)); + return 1; + } finally { + store.close(); + } +} diff --git a/test/unit/miner-attempt-cli.test.ts b/test/unit/miner-attempt-cli.test.ts index 526de20e71..98cf9a25c4 100644 --- a/test/unit/miner-attempt-cli.test.ts +++ b/test/unit/miner-attempt-cli.test.ts @@ -15,7 +15,7 @@ import { closeDefaultGovernorLedger, initGovernorLedger } from "../../packages/l import { closeDefaultWorktreeAllocator, openWorktreeAllocator } from "../../packages/loopover-miner/lib/worktree-allocator.js"; import { closeDefaultPortfolioQueueStore } from "../../packages/loopover-miner/lib/portfolio-queue.js"; import { closeDefaultGovernorState } from "../../packages/loopover-miner/lib/governor-state.js"; -import { buildAttemptDeps, parseAttemptArgs, runAttempt } from "../../packages/loopover-miner/lib/attempt-cli.js"; +import { buildAttemptDeps, parseAttemptArgs, runAttempt, resolveAttemptHouseRulesConfig } from "../../packages/loopover-miner/lib/attempt-cli.js"; import type { RunAttemptOptions } from "../../packages/loopover-miner/lib/attempt-cli.js"; import type { RuleFiredEvent, SignalStore } from "../../packages/loopover-engine/src/calibration/signal-tracking.js"; import * as minerSentryModule from "../../packages/loopover-miner/lib/sentry.js"; @@ -2638,3 +2638,35 @@ describe("runAttempt: Neon branch-per-attempt DB fork (#7858)", () => { expect(exitCode).toBe(7); }); }); + +describe("resolveAttemptHouseRulesConfig (#8806)", () => { + it("resolves the repo's effective rules (approved proposals merged over defaults) and closes the store", () => { + const close = vi.fn(); + const resolveEffectiveRules = vi.fn(() => [{ toolNamePattern: /Bash/, inputTokenPattern: /CHANGELOG\.md/ }]); + const config = resolveAttemptHouseRulesConfig("acme/widgets", (() => ({ resolveEffectiveRules, close })) as never); + expect(config?.repoFullName).toBe("acme/widgets"); + expect(config?.rules).toHaveLength(1); + expect(resolveEffectiveRules).toHaveBeenCalledWith("acme/widgets"); + expect(close).toHaveBeenCalled(); // no leaked store handle + }); + + it("FAIL-OPEN: a store failure (or no repo) resolves undefined — the pre-#8806 DEFAULT_DENY_RULES floor, never a blocked attempt", () => { + expect(resolveAttemptHouseRulesConfig(undefined)).toBeUndefined(); + expect( + resolveAttemptHouseRulesConfig("acme/widgets", (() => { + throw new Error("store down"); + }) as never), + ).toBeUndefined(); + // A resolve failure AFTER open still closes fail-open to undefined. + const close = vi.fn(); + expect( + resolveAttemptHouseRulesConfig("acme/widgets", (() => ({ + resolveEffectiveRules: () => { + throw new Error("read failed"); + }, + close, + })) as never), + ).toBeUndefined(); + expect(close).toHaveBeenCalled(); + }); +}); diff --git a/test/unit/miner-deny-hooks-cli.test.ts b/test/unit/miner-deny-hooks-cli.test.ts new file mode 100644 index 0000000000..2a8a95bb94 --- /dev/null +++ b/test/unit/miner-deny-hooks-cli.test.ts @@ -0,0 +1,94 @@ +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { runDenyHooks } from "../../packages/loopover-miner/lib/deny-hooks-cli.js"; +import { initDenyHookSynthesisStore } from "../../packages/loopover-miner/lib/deny-hook-synthesis.js"; +import { resolveAttemptHouseRulesConfig } from "../../packages/loopover-miner/lib/attempt-cli.js"; + +// #8806: the operate half of the deny-hook loop — refresh (explicit --history file) → approve → the +// attempt-side resolver picks the approved rule up. The end-to-end test below is the loop the audit found +// severed: pre-#8806 nothing invoked refreshProposals and nothing read resolveEffectiveRules. +describe("loopover-miner deny-hooks (#8806)", () => { + let configDir: string; + const savedEnv = { configDir: process.env.LOOPOVER_MINER_CONFIG_DIR, dbOverride: process.env.LOOPOVER_MINER_DENY_HOOK_SYNTHESIS_DB }; + + beforeEach(() => { + configDir = mkdtempSync(join(tmpdir(), "miner-deny-hooks-cli-")); + process.env.LOOPOVER_MINER_CONFIG_DIR = configDir; + delete process.env.LOOPOVER_MINER_DENY_HOOK_SYNTHESIS_DB; + }); + afterEach(() => { + if (savedEnv.configDir === undefined) delete process.env.LOOPOVER_MINER_CONFIG_DIR; + else process.env.LOOPOVER_MINER_CONFIG_DIR = savedEnv.configDir; + if (savedEnv.dbOverride === undefined) delete process.env.LOOPOVER_MINER_DENY_HOOK_SYNTHESIS_DB; + else process.env.LOOPOVER_MINER_DENY_HOOK_SYNTHESIS_DB = savedEnv.dbOverride; + vi.restoreAllMocks(); + }); + + function writeHistory(): string { + const path = join(configDir, "history.json"); + writeFileSync( + path, + JSON.stringify([ + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + { blockerCodes: ["guardrail_hold"], changedPaths: ["CHANGELOG.md"] }, + ]), + ); + return path; + } + + it("END-TO-END: refresh --history → approve → the attempt-side resolver enforces the approved rule", () => { + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + expect(runDenyHooks(["refresh", "acme/widgets", "--history", writeHistory(), "--json"])).toBe(0); + const { proposals } = JSON.parse(String(log.mock.calls.at(-1)?.[0])) as { proposals: Array<{ id: string }> }; + expect(proposals.length).toBeGreaterThan(0); + + expect(runDenyHooks(["approve", "acme/widgets", proposals[0]!.id])).toBe(0); + + // The enforce half: buildAttemptDeps' resolver (same default store path) now includes the approved rule. + const baseline = resolveAttemptHouseRulesConfig("other/repo"); + const withApproved = resolveAttemptHouseRulesConfig("acme/widgets"); + expect(withApproved?.rules.length).toBe((baseline?.rules.length ?? 0) + 1); + }); + + it("list renders proposals with status and the effective-rule count", () => { + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + runDenyHooks(["refresh", "acme/widgets", "--history", writeHistory()]); + expect(runDenyHooks(["list", "acme/widgets", "--json"])).toBe(0); + const payload = JSON.parse(String(log.mock.calls.at(-1)?.[0])) as { proposals: unknown[]; effectiveRuleCount: number }; + expect(payload.proposals.length).toBeGreaterThan(0); + expect(payload.effectiveRuleCount).toBeGreaterThan(0); + // Human output too (both list arms + the empty-repo arm). + expect(runDenyHooks(["list", "acme/widgets"])).toBe(0); + expect(runDenyHooks(["list", "empty/repo"])).toBe(0); + }); + + it("reject marks a proposal rejected — it never reaches the effective rules", () => { + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + runDenyHooks(["refresh", "acme/widgets", "--history", writeHistory(), "--json"]); + const { proposals } = JSON.parse(String(log.mock.calls.at(-1)?.[0])) as { proposals: Array<{ id: string }> }; + expect(runDenyHooks(["reject", "acme/widgets", proposals[0]!.id])).toBe(0); + const store = initDenyHookSynthesisStore(); + try { + const baselineCount = store.resolveEffectiveRules("other/repo").length; + expect(store.resolveEffectiveRules("acme/widgets").length).toBe(baselineCount); // defaults only + } finally { + store.close(); + } + }); + + it("usage/argument errors exit 2; a bad history file exits 1 with the parse error", () => { + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + vi.spyOn(console, "log").mockImplementation(() => undefined); + expect(runDenyHooks([])).toBe(2); // no subcommand + expect(runDenyHooks(["list"])).toBe(2); // no repo + expect(runDenyHooks(["refresh", "acme/widgets"])).toBe(2); // no --history + expect(runDenyHooks(["approve", "acme/widgets"])).toBe(2); // no proposal id + expect(runDenyHooks(["bogus", "acme/widgets"])).toBe(2); // unknown subcommand + const badPath = join(configDir, "bad.json"); + writeFileSync(badPath, JSON.stringify({ not: "an array" })); + expect(runDenyHooks(["refresh", "acme/widgets", "--history", badPath])).toBe(1); + expect(error).toHaveBeenCalledWith(expect.stringContaining("JSON array")); + }); +});