fix(miner): index-parse deny-hooks --history and route failures through the shared CLI error contract

packages/loopover-miner/lib/deny-hooks-cli.ts

@@ -12,6 +12,7 @@
 // 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";
+import { argsWantJson, describeCliError, reportCliFailure } from "./cli-error.js";
 
 const USAGE = [
   "Usage:",
@@ -29,13 +30,50 @@ function parseHistoryFile(path: string): HistoryRecord[] {
   return parsed as HistoryRecord[];
 }
 
-export function runDenyHooks(args: string[]): number {
-  const json = args.includes("--json");
-  const positional = args.filter((arg) => !arg.startsWith("--"));
+type ParsedDenyHooksArgs =
+  | { subcommand: string | undefined; repoFullName: string | undefined; proposalId: string | undefined; historyPath: string | undefined; json: boolean }
+  | { error: string };
+
+/** Index-based parse (mirroring `parseRepoIdentifierArgs` in `portfolio-queue-cli.ts`): consume `--history <value>`
+ *  as a flag+value pair so the value never leaks into the positional list (the `args.filter` bug this replaces —
+ *  same class as closed #5833's `hooks check --tool/--input`). A missing or flag-like `--history` value, and any
+ *  unrecognized `-`-prefixed token, are parse errors here rather than downstream nonsense-repo operations. */
+export function parseDenyHooksArgs(args: string[]): ParsedDenyHooksArgs {
+  let json = false;
+  let historyPath: string | undefined;
+  const positional: string[] = [];
+  for (let index = 0; index < args.length; index += 1) {
+    const token = args[index]!;
+    if (token === "--json") {
+      json = true;
+      continue;
+    }
+    if (token === "--history") {
+      const value = args[index + 1];
+      if (!value || value.startsWith("-")) {
+        return { error: "refresh requires --history <file.json>\n" + USAGE };
+      }
+      historyPath = value;
+      index += 1;
+      continue;
+    }
+    if (token.startsWith("-")) {
+      return { error: `Unknown option: ${token}` };
+    }
+    positional.push(token);
+  }
   const [subcommand, repoFullName, proposalId] = positional;
+  return { subcommand, repoFullName, proposalId, historyPath, json };
+}
+
+export function runDenyHooks(args: string[]): number {
+  const parsed = parseDenyHooksArgs(args);
+  if ("error" in parsed) {
+    return reportCliFailure(argsWantJson(args), parsed.error);
+  }
+  const { subcommand, repoFullName, proposalId, historyPath, json } = parsed;
   if (!subcommand || !repoFullName) {
-    console.error(USAGE);
-    return 2;
+    return reportCliFailure(json, USAGE);
   }
   const store = initDenyHookSynthesisStore();
   try {
@@ -56,11 +94,8 @@ export function runDenyHooks(args: string[]): number {
         return 0;
       }
       case "refresh": {
-        const historyFlag = args.indexOf("--history");
-        const historyPath = historyFlag !== -1 ? args[historyFlag + 1] : undefined;
         if (!historyPath) {
-          console.error("refresh requires --history <file.json>\n" + USAGE);
-          return 2;
+          return reportCliFailure(json, "refresh requires --history <file.json>\n" + USAGE);
         }
         const proposals = store.refreshProposals(repoFullName, parseHistoryFile(historyPath));
         if (json) {
@@ -73,23 +108,17 @@ export function runDenyHooks(args: string[]): number {
       case "approve":
       case "reject": {
         if (!proposalId) {
-          console.error(USAGE);
-          return 2;
+          return reportCliFailure(json, USAGE);
         }
         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;
+        return reportCliFailure(json, USAGE);
     }
   } catch (error) {
-    // String(error) renders an Error as "Error: <message>" — 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;
+    return reportCliFailure(json, describeCliError(error));
   } finally {
     store.close();
   }

test/unit/miner-deny-hooks-cli.test.ts

@@ -2,7 +2,7 @@ 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";
+import { parseDenyHooksArgs, runDenyHooks } from "../../packages/loopover-miner/lib/deny-hooks-cli";
 import { initDenyHookSynthesisStore } from "../../packages/loopover-miner/lib/deny-hook-synthesis";
 import { resolveAttemptHouseRulesConfig } from "../../packages/loopover-miner/lib/attempt-cli";
 
@@ -78,7 +78,7 @@ describe("loopover-miner deny-hooks (#8806)", () => {
     }
   });
 
-  it("usage/argument errors exit 2; a bad history file exits 1 with the parse error", () => {
+  it("every failure path exits 2 (the shared default), including a bad history file", () => {
     const error = vi.spyOn(console, "error").mockImplementation(() => undefined);
     vi.spyOn(console, "log").mockImplementation(() => undefined);
     expect(runDenyHooks([])).toBe(2); // no subcommand
@@ -88,7 +88,80 @@ describe("loopover-miner deny-hooks (#8806)", () => {
     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);
+    // The catch-all now returns the shared default (2), not the old bespoke 1.
+    expect(runDenyHooks(["refresh", "acme/widgets", "--history", badPath])).toBe(2);
     expect(error).toHaveBeenCalledWith(expect.stringContaining("JSON array"));
   });
+
+  // #9687: the parser hand-rolled `args.filter((a) => !a.startsWith("--"))`, so `--history`'s value (a path,
+  // which does not start with "--") leaked into the positional list. Placed before the repo, it stole the
+  // `repoFullName` slot and the command operated on a nonsense repo. These pin the index-based parse.
+  describe("parseDenyHooksArgs (#9687)", () => {
+    it("REGRESSION: --history before the repo resolves the repo correctly, not to the history path", () => {
+      // Against the old `args.filter` code this yielded repoFullName === "h.json".
+      const parsed = parseDenyHooksArgs(["refresh", "--history", "h.json", "acme/widgets"]);
+      expect(parsed).toEqual({ subcommand: "refresh", repoFullName: "acme/widgets", proposalId: undefined, historyPath: "h.json", json: false });
+    });
+
+    it("consumes --history after the repo too, and threads --json", () => {
+      expect(parseDenyHooksArgs(["refresh", "acme/widgets", "--history", "h.json", "--json"])).toEqual({
+        subcommand: "refresh",
+        repoFullName: "acme/widgets",
+        proposalId: undefined,
+        historyPath: "h.json",
+        json: true,
+      });
+    });
+
+    it("keeps the third positional as the proposal id (approve/reject)", () => {
+      expect(parseDenyHooksArgs(["approve", "acme/widgets", "prop-1"])).toEqual({
+        subcommand: "approve",
+        repoFullName: "acme/widgets",
+        proposalId: "prop-1",
+        historyPath: undefined,
+        json: false,
+      });
+    });
+
+    it("rejects a missing --history value with the usage error", () => {
+      const parsed = parseDenyHooksArgs(["refresh", "acme/widgets", "--history"]);
+      expect(parsed).toEqual({ error: expect.stringContaining("--history <file.json>") });
+    });
+
+    it("rejects a flag-like --history value with the usage error", () => {
+      const parsed = parseDenyHooksArgs(["refresh", "acme/widgets", "--history", "--json"]);
+      expect(parsed).toEqual({ error: expect.stringContaining("--history <file.json>") });
+    });
+
+    it("rejects an unknown -x option with 'Unknown option: -x'", () => {
+      expect(parseDenyHooksArgs(["list", "acme/widgets", "-x"])).toEqual({ error: "Unknown option: -x" });
+    });
+  });
+
+  it("routes parse errors through reportCliFailure — --json emits { ok: false, error } on stdout, exit 2", () => {
+    const log = vi.spyOn(console, "log").mockImplementation(() => undefined);
+    const error = vi.spyOn(console, "error").mockImplementation(() => undefined);
+    // Unknown option under --json: machine-readable on stdout, nothing on stderr, exit 2.
+    expect(runDenyHooks(["list", "acme/widgets", "-x", "--json"])).toBe(2);
+    expect(JSON.parse(String(log.mock.calls.at(-1)?.[0]))).toEqual({ ok: false, error: "Unknown option: -x" });
+    expect(error).not.toHaveBeenCalled();
+    // Same option without --json: plain text on stderr instead, still exit 2.
+    log.mockClear();
+    expect(runDenyHooks(["list", "acme/widgets", "-x"])).toBe(2);
+    expect(error).toHaveBeenCalledWith("Unknown option: -x");
+    expect(log).not.toHaveBeenCalled();
+  });
+
+  it("a failing store call under --json prints { ok: false, error } on stdout with exit 2", () => {
+    const log = vi.spyOn(console, "log").mockImplementation(() => undefined);
+    const error = vi.spyOn(console, "error").mockImplementation(() => undefined);
+    const badPath = join(configDir, "bad.json");
+    writeFileSync(badPath, JSON.stringify({ not: "an array" }));
+    // The catch-all: parseHistoryFile throws inside the store try-block; --json routes it to stdout, exit 2.
+    expect(runDenyHooks(["refresh", "acme/widgets", "--history", badPath, "--json"])).toBe(2);
+    const payload = JSON.parse(String(log.mock.calls.at(-1)?.[0])) as { ok: boolean; error: string };
+    expect(payload.ok).toBe(false);
+    expect(payload.error).toContain("JSON array");
+    expect(error).not.toHaveBeenCalled();
+  });
 });