From 7925df703128f9d040a4fd7a913039945f99bddc Mon Sep 17 00:00:00 2001 From: Hiro-Chiba <203865699+Hiro-Chiba@users.noreply.github.com> Date: Sun, 30 Aug 2026 18:02:11 +0900 Subject: [PATCH 1/4] fix(cli): validate common option values --- .../common-option-validation.test.ts | 52 ++++++++++++++ ix-cli/src/cli/options.ts | 67 ++++++++++++++++++- ix-cli/src/cli/register/oss.ts | 5 ++ 3 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 ix-cli/src/cli/__tests__/common-option-validation.test.ts diff --git a/ix-cli/src/cli/__tests__/common-option-validation.test.ts b/ix-cli/src/cli/__tests__/common-option-validation.test.ts new file mode 100644 index 00000000..1c88fad8 --- /dev/null +++ b/ix-cli/src/cli/__tests__/common-option-validation.test.ts @@ -0,0 +1,52 @@ +import { Command } from "commander"; +import { describe, expect, it } from "vitest"; + +import { registerOssCommands } from "../register/oss.js"; +import { validateCliOptions } from "../options.js"; + +async function parseInvalid(args: string[]): Promise { + const program = new Command(); + program.name("ix").exitOverride(); + registerOssCommands(program); + try { + await program.parseAsync(args, { from: "user" }); + return undefined; + } catch (error) { + return error; + } +} + +describe("common CLI option validation", () => { + it.each([ + [["doctor", "--format", "yaml"], "--format"], + [["inventory", "--kind", "file", "--limit", "1e3"], "--limit"], + [["rank", "--by", "dependents", "--kind", "class", "--top", "10abc"], "--top"], + [["patches", "--limit", "0"], "--limit"], + [["search", "term", "--as-of", "abc"], "--as-of"], + [["search", "term", "--as-of", "1e3"], "--as-of"], + [["map", "--level", "nope"], "--level"], + [["map", "--min-confidence", "1.1"], "--min-confidence"], + [["map", "--sort", "newest"], "--sort"], + [["savings", "--model", "unknown"], "--model"], + ] as const)("rejects %j before running the command", async (args, option) => { + const error = await parseInvalid([...args]); + + expect(error).toMatchObject({ code: "commander.invalidArgument" }); + expect(String((error as Error).message)).toContain(option); + }); + + it.each([ + ["doctor", "--format", "json"], + ["map", "--format", "silent"], + ["map", "--min-confidence", "0.75"], + ["subsystems", "--offset", "0"], + ] as const)("accepts the documented value in %j", (command, option, value) => { + const program = new Command(); + program.name("ix"); + registerOssCommands(program); + const action = program.commands.find((candidate) => candidate.name() === command)!; + action.parseOptions([option, value]); + + expect(() => validateCliOptions(action)).not.toThrow(); + }); +}); diff --git a/ix-cli/src/cli/options.ts b/ix-cli/src/cli/options.ts index 921580e1..a79cdda4 100644 --- a/ix-cli/src/cli/options.ts +++ b/ix-cli/src/cli/options.ts @@ -1,4 +1,4 @@ -import { InvalidArgumentError } from "commander"; +import { InvalidArgumentError, type Command, type Option } from "commander"; /** * Parse a flag that must be a positive integer, rejecting anything else. @@ -23,6 +23,71 @@ function parsePositiveInt(value: string, example: string): number { return parsed; } +function parseNonNegativeInt(value: string, example: string): number { + const normalized = value.trim(); + const reject = () => + new InvalidArgumentError(`must be a non-negative integer (for example, ${example})`); + if (!/^\+?\d+$/.test(normalized)) throw reject(); + + const parsed = Number(normalized); + if (!Number.isSafeInteger(parsed)) throw reject(); + return parsed; +} + +function documentedChoices(option: Option, command: Command): string[] | null { + const group = option.description.match(/\(([^()]*(?:\|)[^()]*)\)/)?.[1]; + if (!group) return null; + const choices = group.split("|").map((choice) => choice.trim()); + if (option.long === "--format" && command.name() === "map") choices.push("silent"); + return choices; +} + +function invalidOption(option: Option, detail: string): InvalidArgumentError { + return new InvalidArgumentError(`option '${option.long}' ${detail}`); +} + +/** + * Validate common option domains before a command can contact the backend. + * + * Most commands read numeric options with `parseInt` and route any unrecognized + * output format through their text branch. That turns typos into a different, + * successful request. Keeping this at the root command covers OSS and optional + * Pro commands consistently, including the long-lived MCP runner. + */ +export function validateCliOptions(command: Command): void { + const values = command.opts(); + + for (const option of command.options) { + const value = values[option.attributeName()]; + if (value === undefined || value === null) continue; + + if (typeof value === "string") { + const choices = documentedChoices(option, command); + if (choices && !choices.includes(value)) { + throw invalidOption(option, `must be one of: ${choices.join(", ")}`); + } + + if (option.long === "--as-of") { + try { parseRevisionOption(value); } + catch { throw invalidOption(option, "must be a positive integer"); } + } + + if (option.long === "--min-confidence") { + const parsed = Number(value); + if (!/^(?:0(?:\.\d+)?|1(?:\.0+)?)$/.test(value.trim()) || parsed < 0 || parsed > 1) { + throw invalidOption(option, "must be a number from 0 to 1"); + } + } else if (option.long === "--offset") { + try { parseNonNegativeInt(value, "0 or 10"); } + catch { throw invalidOption(option, "must be a non-negative integer"); } + } else if (option.flags.includes("")) { + try { parsePositiveInt(value, "1 or 10"); } + catch { throw invalidOption(option, "must be a positive integer"); } + } + } + } +} + export function parsePickOption(value: string): number { return parsePositiveInt(value, "1 or 2"); } diff --git a/ix-cli/src/cli/register/oss.ts b/ix-cli/src/cli/register/oss.ts index c6defe3a..6a7e6c00 100644 --- a/ix-cli/src/cli/register/oss.ts +++ b/ix-cli/src/cli/register/oss.ts @@ -37,6 +37,7 @@ import { registerSavingsCommand } from "../commands/savings.js"; import { registerPatchesCommand } from "../commands/patches.js"; import { registerMcpCommand } from "../commands/mcp.js"; import { registerContextCommand } from "../commands/context.js"; +import { validateCliOptions } from "../options.js"; const PRO_COMMANDS: { name: string; desc: string }[] = [ { name: "briefing", desc: "Session-resume briefing" }, @@ -105,6 +106,10 @@ export function registerOssCommands(program: Command): void { registerMcpCommand(program); registerContextCommand(program); + program.hook("preAction", (_thisCommand, actionCommand) => { + validateCliOptions(actionCommand); + }); + // Hide advanced commands from default help const advancedSet = new Set(ADVANCED_COMMANDS); for (const cmd of program.commands) { From 839f9ee17279736e72f284ea04c078fca702eeab Mon Sep 17 00:00:00 2001 From: KageBinary Date: Mon, 31 Aug 2026 11:20:33 -0700 Subject: [PATCH 2/4] fix(cli): stop the option validator rejecting a command's own defaults `validateCliOptions` read every option through `command.opts()`, which returns defaults for options nobody passed, and then applied the `` rule to them. `ix smells` declares `--orphan-max-connections ` with a default of "0", and the rule was positive-integer -- so `ix smells`, with no arguments, exited 1 with Error: option '--orphan-max-connections' must be a positive integer before its action ever ran. Verified against main, where the same command reaches the backend. Nothing caught it because no test runs `ix smells`, and the suite only exercised options it passed explicitly. Two changes: - Validate only what the caller actually typed, via commander's own `getOptionValueSource`. A default is the command author's choice and is by definition a value the command accepts; checking it can only ever produce a command nobody can run. This also retires the whole class -- any future option whose default falls outside its documented `(a|b|c)` group would have bricked its command the same way. - `` now means a non-negative integer rather than a positive one. 0 is documented for several of these flags -- `--offset 0`, `--weak-max-neighbors 0`, and `--orphan-max-connections 0`, which is its own default -- while every typo this exists to catch still fails: `1e3`, `10abc`, `0x10`, `-5`, `abc`. The flags that really do mean "at least one" declare `parsePositiveInt` as their own commander parser (`--pick`), so commander still rejects 0 there at parse time, ahead of this hook. Tests: replaced the `--limit 0` case with `--limit -1` (0 is now legal, -1 is still not), added the two `smells` zero cases, and added a case per command asserting it survives validation on its defaults alone -- the check that would have caught this. --- .../common-option-validation.test.ts | 22 ++++++++++++++++++- ix-cli/src/cli/options.ts | 22 ++++++++++++++----- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/ix-cli/src/cli/__tests__/common-option-validation.test.ts b/ix-cli/src/cli/__tests__/common-option-validation.test.ts index 1c88fad8..c91c933d 100644 --- a/ix-cli/src/cli/__tests__/common-option-validation.test.ts +++ b/ix-cli/src/cli/__tests__/common-option-validation.test.ts @@ -21,7 +21,7 @@ describe("common CLI option validation", () => { [["doctor", "--format", "yaml"], "--format"], [["inventory", "--kind", "file", "--limit", "1e3"], "--limit"], [["rank", "--by", "dependents", "--kind", "class", "--top", "10abc"], "--top"], - [["patches", "--limit", "0"], "--limit"], + [["patches", "--limit", "-1"], "--limit"], [["search", "term", "--as-of", "abc"], "--as-of"], [["search", "term", "--as-of", "1e3"], "--as-of"], [["map", "--level", "nope"], "--level"], @@ -40,6 +40,9 @@ describe("common CLI option validation", () => { ["map", "--format", "silent"], ["map", "--min-confidence", "0.75"], ["subsystems", "--offset", "0"], + // 0 is this flag's own default, so rejecting it was incoherent. + ["smells", "--orphan-max-connections", "0"], + ["smells", "--weak-max-neighbors", "0"], ] as const)("accepts the documented value in %j", (command, option, value) => { const program = new Command(); program.name("ix"); @@ -49,4 +52,21 @@ describe("common CLI option validation", () => { expect(() => validateCliOptions(action)).not.toThrow(); }); + + // The regression this hook shipped with: it read every option's *default* + // through `command.opts()`, so a command whose own default fell outside the + // rule could not be run at all. `ix smells` defaults + // --orphan-max-connections to "0" and died on `ix smells` with no arguments. + it.each(["smells", "map", "subsystems", "rank", "inventory", "patches", "doctor", "status", "context"])( + "runs %s on its defaults alone", + (command) => { + const program = new Command(); + program.name("ix"); + registerOssCommands(program); + const action = program.commands.find((candidate) => candidate.name() === command)!; + action.parseOptions([]); + + expect(() => validateCliOptions(action)).not.toThrow(); + }, + ); }); diff --git a/ix-cli/src/cli/options.ts b/ix-cli/src/cli/options.ts index a79cdda4..67712c2e 100644 --- a/ix-cli/src/cli/options.ts +++ b/ix-cli/src/cli/options.ts @@ -58,7 +58,15 @@ export function validateCliOptions(command: Command): void { const values = command.opts(); for (const option of command.options) { - const value = values[option.attributeName()]; + const name = option.attributeName(); + // Only what the caller actually typed. A default is the command author's + // own choice, and checking it turns a mismatch into a command nobody can + // run: `ix smells` defaults `--orphan-max-connections` to 0, which the + // `` rule below rejected, so the command failed before its action ever + // started -- on no arguments at all. Anything the author ships as a + // default is by definition a value the command accepts. + if (command.getOptionValueSource(name) !== "cli") continue; + const value = values[name]; if (value === undefined || value === null) continue; if (typeof value === "string") { @@ -77,12 +85,16 @@ export function validateCliOptions(command: Command): void { if (!/^(?:0(?:\.\d+)?|1(?:\.0+)?)$/.test(value.trim()) || parsed < 0 || parsed > 1) { throw invalidOption(option, "must be a number from 0 to 1"); } - } else if (option.long === "--offset") { + } else if (option.flags.includes("")) { + // Non-negative, not positive. 0 is a documented value for several of + // these -- `--offset 0`, `--orphan-max-connections 0` (its own + // default), `--weak-max-neighbors 0` -- and none of the typos this + // exists to catch survive either rule: `1e3`, `10abc`, `0x10`, `-5` + // and `abc` are all rejected. The flags that genuinely mean "at least + // one" declare `parsePositiveInt` as their own commander parser + // (`--pick`), so commander still rejects 0 for them at parse time. try { parseNonNegativeInt(value, "0 or 10"); } catch { throw invalidOption(option, "must be a non-negative integer"); } - } else if (option.flags.includes("")) { - try { parsePositiveInt(value, "1 or 10"); } - catch { throw invalidOption(option, "must be a positive integer"); } } } } From 3ceb184c64c248909650d32afdeeb10b23ed4c5a Mon Sep 17 00:00:00 2001 From: Hiro-Chiba <203865699+Hiro-Chiba@users.noreply.github.com> Date: Tue, 1 Sep 2026 08:14:30 +0900 Subject: [PATCH 3/4] fix(cli): declare option choices explicitly --- .../common-option-validation.test.ts | 34 +++++++++++++++++++ ix-cli/src/cli/commands/map.ts | 2 +- ix-cli/src/cli/options.ts | 13 ------- ix-cli/src/cli/register/oss.ts | 24 +++++++++++++ 4 files changed, 59 insertions(+), 14 deletions(-) diff --git a/ix-cli/src/cli/__tests__/common-option-validation.test.ts b/ix-cli/src/cli/__tests__/common-option-validation.test.ts index c91c933d..5adfd2a0 100644 --- a/ix-cli/src/cli/__tests__/common-option-validation.test.ts +++ b/ix-cli/src/cli/__tests__/common-option-validation.test.ts @@ -17,6 +17,40 @@ async function parseInvalid(args: string[]): Promise { } describe("common CLI option validation", () => { + it("declares every documented enum as Commander choices", () => { + const program = new Command(); + program.name("ix"); + registerOssCommands(program); + const pending = [...program.commands]; + + while (pending.length > 0) { + const command = pending.shift()!; + pending.push(...command.commands); + for (const option of command.options) { + const group = option.description.match(/\(([^()]*(?:\|)[^()]*)\)/)?.[1]; + if (!group) continue; + expect(option.argChoices, `${command.name()} ${option.long}`).toEqual( + group.split("|").map((choice) => choice.trim()), + ); + } + } + }); + + it("does not make a later Pro command's help text an implicit runtime contract", async () => { + const program = new Command(); + program.name("ix"); + registerOssCommands(program); + let received: string | undefined; + program + .command("pro-test") + .option("--mode ", "Presentation mode (short|full)") + .action((options: { mode?: string }) => { received = options.mode; }); + + await program.parseAsync(["pro-test", "--mode", "custom"], { from: "user" }); + + expect(received).toBe("custom"); + }); + it.each([ [["doctor", "--format", "yaml"], "--format"], [["inventory", "--kind", "file", "--limit", "1e3"], "--limit"], diff --git a/ix-cli/src/cli/commands/map.ts b/ix-cli/src/cli/commands/map.ts index 24dc648f..b025e1ac 100644 --- a/ix-cli/src/cli/commands/map.ts +++ b/ix-cli/src/cli/commands/map.ts @@ -259,7 +259,7 @@ export function registerMapCommand(program: Command): void { program .command("map [path]") .description("Map the architectural hierarchy of a codebase") - .option("--format ", "Output format (text|json|llm)", "text") + .option("--format ", "Output format (text|json|llm|silent)", "text") .option("--level ", "Show only regions at this level (1=finest, higher=coarser)") .option("--min-confidence ", "Only show regions above this confidence threshold (0-1)", "0") .option("--max-items ", "Max items to show per section in text output (default: 10)", "10") diff --git a/ix-cli/src/cli/options.ts b/ix-cli/src/cli/options.ts index 67712c2e..06cfa776 100644 --- a/ix-cli/src/cli/options.ts +++ b/ix-cli/src/cli/options.ts @@ -34,14 +34,6 @@ function parseNonNegativeInt(value: string, example: string): number { return parsed; } -function documentedChoices(option: Option, command: Command): string[] | null { - const group = option.description.match(/\(([^()]*(?:\|)[^()]*)\)/)?.[1]; - if (!group) return null; - const choices = group.split("|").map((choice) => choice.trim()); - if (option.long === "--format" && command.name() === "map") choices.push("silent"); - return choices; -} - function invalidOption(option: Option, detail: string): InvalidArgumentError { return new InvalidArgumentError(`option '${option.long}' ${detail}`); } @@ -70,11 +62,6 @@ export function validateCliOptions(command: Command): void { if (value === undefined || value === null) continue; if (typeof value === "string") { - const choices = documentedChoices(option, command); - if (choices && !choices.includes(value)) { - throw invalidOption(option, `must be one of: ${choices.join(", ")}`); - } - if (option.long === "--as-of") { try { parseRevisionOption(value); } catch { throw invalidOption(option, "must be a positive integer"); } diff --git a/ix-cli/src/cli/register/oss.ts b/ix-cli/src/cli/register/oss.ts index 6a7e6c00..b5c1235e 100644 --- a/ix-cli/src/cli/register/oss.ts +++ b/ix-cli/src/cli/register/oss.ts @@ -66,6 +66,28 @@ const ADVANCED_COMMANDS = [ "init", "ingest", ]; +const DEFAULT_FORMAT_CHOICES = ["text", "json", "llm"]; +const OPTION_CHOICES: Record> = { + query: { depth: ["shallow", "standard", "deep"], format: ["text", "json"] }, + map: { format: [...DEFAULT_FORMAT_CHOICES, "silent"], sort: ["importance", "confidence", "size", "alpha"] }, + subsystems: { sort: ["importance", "confidence", "size", "alpha"] }, + savings: { model: ["opus", "sonnet", "haiku", "gpt-4o"] }, + context: { depth: ["compact", "standard", "full", "shallow", "deep"] }, +}; + +function configureOssOptionChoices(root: Command): void { + const visit = (command: Command): void => { + const commandChoices = OPTION_CHOICES[command.name()] ?? {}; + for (const option of command.options) { + const choices = commandChoices[option.attributeName()] + ?? (option.long === "--format" ? DEFAULT_FORMAT_CHOICES : undefined); + if (choices) option.choices(choices); + } + for (const child of command.commands) visit(child); + }; + visit(root); +} + export function registerOssCommands(program: Command): void { registerQueryCommand(program); registerIngestCommand(program); @@ -106,6 +128,8 @@ export function registerOssCommands(program: Command): void { registerMcpCommand(program); registerContextCommand(program); + configureOssOptionChoices(program); + program.hook("preAction", (_thisCommand, actionCommand) => { validateCliOptions(actionCommand); }); From c35e0b159f3653270c613a759361c92178ffcf90 Mon Sep 17 00:00:00 2001 From: Ian Hock Date: Tue, 1 Sep 2026 09:13:00 -0700 Subject: [PATCH 4/4] fix(cli): scope the option-domain hook to OSS commands `validateCliOptions` runs from a `preAction` hook on the root program, so it fires for every command -- including the Pro commands `tryLoadProCommands` registers against that same root, after this file has run. The rules it applies read an option's *shape*: `` in the flags means a non-negative integer, `--min-confidence` means 0..1, `--as-of` means a revision. That is a sound claim about option declarations in this repo, where the test above keeps every documented enum honest. It is not a claim this repo can make about `@ix/pro`: a Pro command spelling `--threshold ` for a float, or `--min-confidence ` for something that is not a probability, would be rejected by a rule its author never opted into -- and no test here could catch it, because the package is not visible from this side of the split. `configureOssOptionChoices` already walks exactly the commands this file registered, so it now records them and the hook skips anything else. OSS validation is unchanged; the only commands that stop being validated are ones this repo never declared. This mirrors what the sibling test already pins for `.choices()`, which Pro escapes only because it is registered after `configureOssOptionChoices` runs. The new test covers the half that ordering does not protect, and fails without the guard (mutation-checked). ix-cli: 1475 passed / 2 skipped. typecheck, eslint and knip clean. --- ix-cli/node_modules | 1 + .../common-option-validation.test.ts | 28 +++++++++++++++++++ ix-cli/src/cli/register/oss.ts | 15 ++++++++++ 3 files changed, 44 insertions(+) create mode 120000 ix-cli/node_modules diff --git a/ix-cli/node_modules b/ix-cli/node_modules new file mode 120000 index 00000000..ce7a785d --- /dev/null +++ b/ix-cli/node_modules @@ -0,0 +1 @@ +/home/ianhock/ix-work/oss/Ix/ix-cli/node_modules \ No newline at end of file diff --git a/ix-cli/src/cli/__tests__/common-option-validation.test.ts b/ix-cli/src/cli/__tests__/common-option-validation.test.ts index 5adfd2a0..9e697327 100644 --- a/ix-cli/src/cli/__tests__/common-option-validation.test.ts +++ b/ix-cli/src/cli/__tests__/common-option-validation.test.ts @@ -51,6 +51,34 @@ describe("common CLI option validation", () => { expect(received).toBe("custom"); }); + // Same reasoning, one step further: the preAction hook lives on the root + // program, so it fires for Pro commands too. The `` rule is a claim about + // option declarations in *this* repo -- a Pro flag that spells a float or a + // negative `` would be rejected by a rule its author never opted into. + it("does not apply the numeric rules to a later Pro command's options", async () => { + const program = new Command(); + program.name("ix").exitOverride(); + registerOssCommands(program); + const received: Record = {}; + program + .command("pro-numeric") + .option("--threshold ", "Similarity threshold") + .option("--min-confidence ", "Confidence floor") + .option("--as-of ", "Revision") + .action((options: { threshold?: string; minConfidence?: string; asOf?: string }) => { + received.threshold = options.threshold; + received.minConfidence = options.minConfidence; + received.asOf = options.asOf; + }); + + await program.parseAsync( + ["pro-numeric", "--threshold", "0.8", "--min-confidence", "7", "--as-of", "HEAD~2"], + { from: "user" }, + ); + + expect(received).toEqual({ threshold: "0.8", minConfidence: "7", asOf: "HEAD~2" }); + }); + it.each([ [["doctor", "--format", "yaml"], "--format"], [["inventory", "--kind", "file", "--limit", "1e3"], "--limit"], diff --git a/ix-cli/src/cli/register/oss.ts b/ix-cli/src/cli/register/oss.ts index b5c1235e..3c6db449 100644 --- a/ix-cli/src/cli/register/oss.ts +++ b/ix-cli/src/cli/register/oss.ts @@ -75,8 +75,17 @@ const OPTION_CHOICES: Record> = { context: { depth: ["compact", "standard", "full", "shallow", "deep"] }, }; +/** + * Every command this file registered, so the preAction hook below can tell an + * OSS option from a Pro one. Pro commands are registered later, against the + * same root program, from a package this repo cannot see -- so their option + * domains are not ours to infer. + */ +const ossCommands = new WeakSet(); + function configureOssOptionChoices(root: Command): void { const visit = (command: Command): void => { + ossCommands.add(command); const commandChoices = OPTION_CHOICES[command.name()] ?? {}; for (const option of command.options) { const choices = commandChoices[option.attributeName()] @@ -131,6 +140,12 @@ export function registerOssCommands(program: Command): void { configureOssOptionChoices(program); program.hook("preAction", (_thisCommand, actionCommand) => { + // OSS commands only. The rules below read an option's *shape* -- `` + // means a non-negative integer, `--min-confidence` means 0..1 -- which is + // a claim about commands whose declarations live in this repo. A Pro + // command declaring `--threshold ` for a float would be rejected by a + // rule its author never opted into, and no test here could catch it. + if (!ossCommands.has(actionCommand)) return; validateCliOptions(actionCommand); });