fix(cli): validate common option values

[Hiro-Chiba]

Fixes #550

Summary

Validate common CLI option domains before command actions can contact the backend.

Type

• Bug fix
• Feature
• Refactor
• Docs
• Test
• CI

Changes

• Reject undocumented output formats and explicit enum violations through Commander choices.
• Declare all 38 documented OSS enum options independently of their help prose. ix map --format silent is now part of the visible option domain.
• Keep later-loaded Pro option descriptions out of the OSS runtime contract; Pro commands own their enum declarations.
• Validate only values supplied by the caller, so a command's own default cannot make that command impossible to run.
• Accept non-negative values for generic <n> options where 0 is documented, while dedicated positive-integer parsers still reject 0 where at least one is required.
• Reject partial or malformed integers instead of truncating them with parseInt.
• Validate --as-of and --min-confidence explicitly.
• Add regression coverage for every documented enum, command defaults, zero-valued smell options, and Pro help-text isolation.

Reviewer fix retained

Commit 839f9ee fixes the blocking ix smells no-argument regression by validating only CLI-sourced values and separating non-negative generic options from positive-only parsers. The follow-up commit leaves that implementation intact and only replaces help-text inference with explicit Commander choices.

Validation

• npm test (85 files, 1473 passed, 3 skipped; parser smoke passed)
• npm run typecheck
• npm run build
• npm run knip
• Built CLI: ix smells --format json reaches the backend instead of failing option validation
• Built CLI: ix doctor --format yaml exits 1 before backend access with Commander's allowed-choice error

Checklist

• Tests pass
• Smoke tests pass
• No raw errors introduced
• CLI output follows Ix format

Release checklist (if merging to main)

• ix-cli/package.json version bumped
• After merge: tag pushed (git tag vX.Y.Z && git push origin vX.Y.Z)
• If backend changes: ix-memory-layer tagged and released first
• If docker-compose.standalone.yml changed: verified curl | sh install works

[KageBinary]

Review — one blocking defect, fixed on the branch

ix smells could not run at all with this PR. Not a corner case: no arguments.

$ ix smells
Error: option '--orphan-max-connections' must be a positive integer
$ echo $?
1

Against main, the same command reaches the backend normally. Verified both ways with a built CLI.

Cause. validateCliOptions reads values through command.opts(), which returns defaults for options nobody passed. smells declares:

.option("--orphan-max-connections <n>", "Max connections for orphan files", "0")

<n> routed to parsePositiveInt, whose regex is /^\+?[1-9]\d*$/ — so the command's own shipped default failed its own validator, in a preAction hook, before the action ever started.

Nothing caught it because the suite only exercised options it passed explicitly, and no test runs ix smells.

I audited every option the hook polices by instantiating the program and running documentedChoices over it — 38 enum-policed, 55 <n>-policed. --orphan-max-connections is the only default that trips today, but the shape generalises: any future option whose default falls outside its documented (a|b|c) group would brick its command the same way, silently, at registration time.

Fixed in 839f9ee

1. Validate only what the caller typed, via commander's own getOptionValueSource(name) !== "cli". A default is the author's choice and by definition a value the command accepts; checking it can only ever produce a command nobody can run. This retires the whole class, defaults and env-sourced values.

2. <n> now means non-negative, not positive. 0 is documented for several of these — --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 for them at parse time, ahead of this hook. That also folded the now-redundant --offset branch into the general one.

Tests: --limit 0 → --limit -1 (0 is legal now, -1 is 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.

ix smells                              -> Running smell detection...
ix smells --orphan-max-connections 0   -> Running smell detection...
ix smells --orphan-max-connections 1e3 -> Error: ... must be a non-negative integer
ix patches --limit -1                  -> Error: ... must be a non-negative integer
ix doctor --format yaml                -> Error: option '--format' must be one of: text, json, llm
ix search x --as-of abc                -> Error: option '--as-of' must be a positive integer
ix map --min-confidence 1.1            -> Error: ... must be a number from 0 to 1

npm run test:unit 1472 passed / 2 skipped, typecheck clean, knip exit 0.

Left for you — the mechanism itself

The blocker is fixed, but I did not touch the design, and it is the part worth your eye: allowed values are inferred by regex-matching the option's help text for a parenthesised group containing |.

const group = option.description.match(/\(([^()]*(?:\|)[^()]*)\)/)?.[1];

Three consequences:

• The help string becomes a load-bearing contract. Rewording "Output format (text|json|llm)" changes what the CLI accepts. Nothing says so at the definition site.
• It is already wrong once, and patched by hand: if (option.long === "--format" && command.name() === "map") choices.push("silent"). The next such case gets found by a user.
• It reaches Pro. The hook is registered on the root program, so tryLoadProCommands options are policed by whatever happens to be in their descriptions — text this repo cannot see.

One real behaviour change it makes that I think is right, for the record: ix query --format llm is now rejected, because query's description says (text|json) and formatContext genuinely has no llm branch — it silently rendered text before.

An explicit .choices([...]) on each option would give the same enforcement with the domain declared where the option is, and commander generates the error message itself. Bigger diff, no inference. Your call.

Everything else in the PR — parsePositiveInt's regex-not-parseInt rationale, --as-of, --min-confidence — is sound and well argued.

[Hiro-Chiba]

Thanks for the fix! I kept your commit unchanged and addressed the remaining validation logic with explicit choices. CI is passing.