diff --git a/packages/loopover-miner/lib/deny-hooks-cli.ts b/packages/loopover-miner/lib/deny-hooks-cli.ts index eab9584727..32c954fb63 100644 --- a/packages/loopover-miner/lib/deny-hooks-cli.ts +++ b/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 ` + * 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 \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 \n" + USAGE); - return 2; + return reportCliFailure(json, "refresh requires --history \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: " — 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(); } diff --git a/test/unit/miner-deny-hooks-cli.test.ts b/test/unit/miner-deny-hooks-cli.test.ts index 0beb1a532f..e450eb8257 100644 --- a/test/unit/miner-deny-hooks-cli.test.ts +++ b/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 ") }); + }); + + 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 ") }); + }); + + 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(); + }); });