fix: a CLI flag rejects unknown spec keys and accepts extra long spellings

[davydog187]

Closes #65

Two related gaps in JustBash.CLI flag specs: an unrecognised spec key was silently accepted and never read, and a flag could only ever have one long form.

What was broken

normalize_flags!/2 filled in :long and handed the spec to validate_flag_spec!/3, which checked only :long/:short, :required + :default, and :default ∈ :values. Every other key passed straight through, unread. :aliases is an especially easy key to reach for because it already exists one level up on CLI.new/2 — and the library agreed with you right up until runtime.

Against 82ee9e5, the issue's AliasProbe repro:

$ probe go --target-on 1
   exit=0
$ probe go --target-date 1
   exit=2 stderr="probe go: unknown option: --target-date"

aliases: did nothing, totally_bogus_key: 123 did nothing, and neither produced a peep at build time.

What the fix does

(a) Unknown flag-spec keys raise at build time. validate_flag_spec!/3 now checks the spec's keys against the full set — :type, :short, :long, :aliases, :default, :required, :values, :transform, :doc — and raises naming the offender and listing the valid options. This joins the guards already there (--help/-h reserved, :required + :default, :default outside :values), whose comment already says these are guards that "can't drift into runtime surprises".

raised: command "go" flag :other: unknown flag option :totally_bogus_key; valid options are
[:type, :short, :long, :aliases, :default, :required, :values, :transform, :doc]

(b) :aliases on a flag now works. A list of extra long spellings accepted by the parser, for symmetry with CLI.new/2's tool-level :aliases. JustBash.Commands.ArgParser.build_flag_maps/1 registers each alias in the same long-form map as :long, so an alias parses identically — including the --flag=value form, booleans, and satisfying required: true. :long stays canonical: it is the only spelling shown in usage lines, --help, error messages, and describe/1. Aliases are accepted, not advertised.

Alias validation mirrors :long: the list must be a list of strings, each must be a long flag form starting with -- (so "-t", "target-date", and a bare "--" all raise), --help is reserved, and no alias may collide with another flag's long form or alias (nor with its own flag's :long) — a collision would otherwise resolve to whichever flag the parser indexed last and silently bind the wrong one.

After the fix, the issue's repro:

$ probe go --target-on 1
   exit=0 stdout="target_on=1\n"
$ probe go --target-date 1
   exit=0 stdout="target_on=1\n"

The moduledoc's flag-key list, build-time-guard list, and a new "Flag aliases" section document the behaviour.

New tests

Written first, watched fail (11 failures against the unfixed tree), then fixed.

test/cli/builder_test.exs — build-time guards:

• raises on an unrecognised flag-spec key (totally_bogus_key) and on a misspelling (requird)
• accepts an :aliases list and keeps :long canonical
• raises when :aliases is not a list of strings (bare string; atom entry)
• raises when an alias is not a long flag form ("target-date", "-t", "--")
• raises when an alias claims the reserved --help
• raises when an alias collides with another flag's long form, another flag's alias, or its own flag's long form

test/cli/routing_test.exs — end-to-end through the shell, the issue's probe go CLI:

• the canonical long form still parses; an alias parses into the same flag; --target-date=1 works; an alias works on a boolean flag
• an unrelated unknown flag (--target-dat) still errors at exit 2
• --help and the error usage line show only --target-on, never --target-date or --loud

test/commands/arg_parser_test.exs — parser level: canonical form, alias, --flag=value alias, alias satisfying required: true, alias setting a boolean, and the missing-required error still naming the canonical --target-on rather than an alias.

Gates

All five run on the final tree.

$ mix compile --warnings-as-errors
Compiling 2 files (.ex)
Generated just_bash app

$ mix format --check-formatted
format ok

$ mix credo --strict
Checking 229 source files (this might take a while) ...

  Refactoring opportunities
┃
┃ [F] ↘ Avoid `apply/2` and `apply/3` when the number of arguments is known.
┃       test/support/banned_fixture_apply.ex:4:16 #(BannedCallTracer.Fixture.Apply.run)

Analysis took 1.5 seconds (0.08s to load, 1.4s running 68 checks on 229 files)
5117 mods/funs, found 1 refactoring opportunity.

(the sole credo finding is the pre-existing intentional test fixture, untouched by this branch)

$ mix test
Running ExUnit with seed: 373057, max_cases: 20
Excluding tags: [:live]
Finished in 23.8 seconds (23.6s async, 0.2s sync)
2 doctests, 62 properties, 4774 tests, 0 failures (5 excluded)

$ mix dialyzer
Checking PLT...
PLT is up to date!
ignore_warnings: .dialyzer_ignore.exs
Starting Dialyzer
Total errors: 13, Skipped: 13, Unnecessary Skips: 0
done in 0m3.02s
done (passed successfully)

Review round 2

Five confirmed review findings addressed on top of the original branch (see the reply comment for the repro-by-repro walkthrough). Two of them change what this PR claims to do:

The collision guard now covers :long and :short, not just aliases. Its comment said "two flags claiming the same long form would resolve to whichever the parser indexed last, silently binding the wrong flag", but the fold seeded taken with the long forms and walked only the aliases, so the case the comment named went unchecked. long: "--dup" on two flags built clean and bound the wrong one. Both long forms and short forms are now checked.

Scope extension: command/2, new/2, and :args entries also reject unknown and repeated keys. The flag-spec allowlist closed the silent drop inside a flag and left the identical drop one level up and one level down. Both are pre-existing rather than introduced here, but they are the same class the issue was filed about — visible: for :visible? discarded an authorization predicate and left an intended-hidden node routable by every caller, and requird: on a positional silently made a required argument optional. All four allowlists share one validate_spec_keys!/4.

Also in this round: a :long form or alias containing = is rejected (the parser splits on = before matching, so such a spelling was registered and unreachable); a duplicated valid key is now diagnosed as a duplicate instead of being reported as unknown in a message that listed it as valid; and each of the four allowlists gained enumerated positive tests pinned to the builder's own "valid options are" output — deleting :transform from the flag list previously left the whole suite green.

The gate output below is from the original branch tip; the round-2 gates (4796 tests, 0 failures) are in the reply comment.

Scope

Both halves the issue describes are delivered. Nothing left out. The issue floated deprecated_long: (accepted, hidden, warns on stderr) as an alternative shape; :aliases was chosen per the issue's own preference for symmetry with CLI.new/2, and no stderr deprecation warning is emitted.

[davydog187]

Reviewed with two lenses — a correctness/completeness lens on the new validators, and a test-adequacy lens (does the suite pin the new behaviour, per the #70 "enumerate, don't grow by example" lesson). Every claim below was reproduced against beca739 in a detached worktree; three candidate findings were dismissed and are listed at the bottom.

The core of the PR is good: :aliases works end to end, :long stays canonical in help/usage/errors, and the unknown-key guard closes the headline half of #65. Five things worth a look, none of them blockers.

---

1. The collision guard does not enforce the invariant its own comment states (:long vs :long, :short vs :short)

major — lib/just_bash/cli.ex:998-1017

The comment introducing validate_alias_collisions!/2 justifies the guard with:

Two flags claiming the same long form would resolve to whichever the parser indexed last, silently binding the wrong flag.

But the fold seeds taken with every flag's :long and then walks only the aliases. The exact case the comment names — two flags whose :long collide — is still unchecked, and build_flag_maps/1 (arg_parser.ex:113) is a plain Map.put reduce, so last-write-wins is live. The result is now inconsistently policed: writing the duplicate as aliases: raises at build time, writing the identical duplicate as long: builds clean.

Reproduced on the PR branch:

CLI.command("go",
  flags: [a: [type: :string, long: "--dup"], b: [type: :string, long: "--dup"]],
  run: ...)

$ probe go --dup x
exit=0  stdout="a=nil b=\"x\"\n"      # :a silently never receives its value

Same for shorts — a: [type: :boolean, short: "-x"], b: [type: :boolean, short: "-x"] then probe go -x gives a=false b=true.

Fix: the PR already builds the fold that closes this. Seed taken with [], run the :long values through do_validate_alias_collisions!/3 first (with a message that says "duplicate long form" rather than "alias … collides"), then the aliases. A parallel pass over :short is the same shape.

---

2. The unknown-key guard stops at flag specs — command/2/new/2 options and :args maps still silently drop typos

major — lib/just_bash/cli.ex:334-345 and lib/just_bash/cli.ex:1069-1076

Issue #65's framing is "a typo'd or imagined spec key is indistinguishable from a working one". The PR fixes that inside a flag spec, but the same silent drop is untouched one level up and one level down, and the @flag_spec_keys idiom applies verbatim to both. Both instances are pre-existing, not regressions — flagging them because the PR is the natural place to finish the thought.

(a) command/2 / new/2 option lists. Options are read with a fixed set of Keyword.get/3 calls and leftovers are never checked. The sharpest instance is :visible?, which the moduledoc documents as the authorization mechanism ("the node is absent for that caller"). Drop the question mark and the predicate is discarded with no diagnostic:

CLI.command("admin",
  visible: fn _bash -> false end,        # note: no `?`
  run: fn inv -> {Command.ok("SECRET\n"), inv.bash} end)

$ probe admin
exit=0  stdout="SECRET\n"    # intended-hidden node is routable for every caller

flgs:/exmaples:/comand: are the same shape — flgs: [target_on: [type: :string]] builds a flagless command, and probe go --target-on 1 then fails with unknown option: --target-on.

(b) :args positional specs. normalize_arg!/2 rebuilds each map from Map.get(spec, :doc) / :required / :variadic and discards everything else, so a misspelled :required defaults the positional to optional with no build-time signal:

args: [%{name: :path, requird: true, doc: "path"}]

$ probe go            # no positional supplied
exit=0  stdout="path=nil\n"   # expected: exit 2, "missing required argument: path"

Fix: the same two lines in each place — an @command_opt_keys / @arg_spec_keys allowlist checked against Keyword.keys(opts) / Map.keys(spec).

---

3. @flag_spec_keys has no enumerated positive test — :transform is unprotected (mutation-verified)

major — lib/just_bash/cli.ex:145-155

The allowlist is now load-bearing in the strict direction: any key missing from it turns a spec that works today into a build-time ArgumentError. Nothing in the suite pins the list to the set of keys ArgParser actually reads — the positive side is covered only incidentally, by whichever keys other tests happen to use.

Verified by mutation on the PR branch: deleting :transform, from line 153 leaves the whole suite green —

2 doctests, 62 properties, 4774 tests, 0 failures

— yet :transform is a real, working key through this path (CLI.command("go", flags: [n: [type: :string, transform: &String.upcase/1]], ...) then probe go --n hi → exit=0, "n=HI\n"). With the key omitted, that same downstream CLI raises command "go" flag :n: unknown flag option :transform at build time, breaking every caller that uses transform, with green CI. (The only existing transform coverage is test/commands/arg_parser_test.exs:183, which goes through ArgParser.parse/3 directly and never touches the CLI builder.)

This is the #70 lesson applied to something that is literally a list to enumerate.

Fix: for key <- @flag_spec_keys, do: test that CLI.command/2 accepts a spec carrying it — about nine lines, and it makes the allowlist self-checking. A companion test that one spec carrying all nine keys at once builds would close it too.

---

4. An alias containing = passes the new validator but can never be matched

minor — lib/just_bash/cli.ex:975-990

validate_flag_alias!/3 rejects "-t", "target-date", "--" and "--help", but accepts any other ---prefixed string. The long-flag clause in ArgParser.parse_loop/5 (arg_parser.ex:161-166) splits the token on = before consulting long_map, so a registered alias whose text contains = is unreachable — the lookup key is only the part before the first =.

flags: [target_on: [type: :string, aliases: ["--target=date"]]]

$ probe go --target=date 1     exit=2  "unknown option: --target=date"
$ probe go --target=date=1     exit=2  "unknown option: --target=date=1"

Builds clean, does nothing — the failure mode this PR exists to eliminate, reintroduced inside the new validator. Low likelihood, but it costs one clause.

Fix: add String.contains?(form, "=") to the rejection cond. ("--=" falls out of the same check.)

---

5. A duplicated valid key raises a self-contradictory message

minor — lib/just_bash/cli.ex:951-960

Keyword.keys(spec) -- @flag_spec_keys uses list subtraction, which removes only one occurrence of each right-hand element. A keyword list may legally repeat a key, so a duplicated valid key survives the subtraction and is reported as unknown:

CLI.command("x", flags: [n: [type: :integer, type: :string]], run: ...)

raised: command "x" flag :n: unknown flag option :type; valid options are
        [:type, :short, :long, :aliases, :default, :required, :values, :transform, :doc]

The message calls :type unknown and lists :type as valid in the same sentence. Raising is the right outcome here (Keyword access silently returns the first value and drops the second — exactly the silent-drop class this PR targets), but the diagnosis sends the author hunting a typo that isn't there.

Fix: Enum.uniq(Keyword.keys(spec)) -- @flag_spec_keys for the unknown-key check, plus a separate clause that names duplicates as duplicates.

---

Considered and dismissed (1 of 6 candidate findings)

ArgParser now documents :aliases but still ignores unknown flag-spec keys when used directly. The claim: ArgParser.parse(["--loud"], verbose: [type: :boolean, long: "--verbose", aliasses: ["--loud"]]) returns {:error, "unknown option: --loud\n"} with no build-time signal, so the newly-advertised key can be misspelled through the other public entry point. Reproduced — but dismissed as a layering preference rather than a defect. ArgParser is the low-level per-invocation parser used by builtins (curl.ex:31 and friends); spec validation deliberately lives in the CLI builder, which runs once at build time. Moving it into parse/3 would put an allowlist walk on every command invocation to catch a typo in a spec that is a compile-time literal in the same repo, already covered by that builtin's own tests. The finding itself only claims the guard "arguably belongs" there. No traced cost.

Also merged rather than dropped: the two lenses independently reported the :long vs :long collision gap (kept as finding 1, with the better-argued phrasing and the :short case folded in), and independently reported the command/2 option-list and :args unknown-key drops (merged into finding 2 — same root cause, same fix idiom).

[davydog187]

All five findings addressed; none disputed. Every repro was re-run before touching code (all reproduced exactly as reported, including the :long variant of finding 4 that the review asked me to check), each fix landed test-first, and each repro was re-run afterwards against the stated oracle.

#	Finding	Resolution	Commit
1	Collision guard does not enforce its own invariant (:long vs :long, :short vs :short)	fixed	571bcfc
2	Unknown-key guard stops at flag specs — command/2/new/2 options and :args maps still drop typos	fixed (scope extension, see below)	1bb6770
3	@flag_spec_keys has no enumerated positive test; :transform unprotected	fixed	85fafcb
4	An alias containing = passes the validator but can never be matched	fixed	a26c4d8
5	A duplicated valid key raises a self-contradictory message	fixed	884231e

---

1 — collision guard (571bcfc)

Reproduced first. flags: [a: [long: "--dup"], b: [long: "--dup"]] built clean and probe go --dup x exited 0 with a=nil b="x"; short: "-x" twice gave a=false b=true.

Took the suggested fix. The fold now seeds taken with [] and runs the long forms through do_validate_flag_collisions!/3 first, then the aliases — that ordering keeps a long-vs-alias collision reported against the alias, the form that moved, so the three existing alias-collision tests keep their wording. A second pass covers short forms. Messages name the kind of spelling: long form "--dup" collides with an existing flag long form or alias / short form "-x" collides with an existing flag short form.

dup long:  raised: command "go" flag :b: long form "--dup" collides with an existing flag long form or alias
dup short: raised: command "go" flag :b: short form "-x" collides with an existing flag short form

Three new tests, including one for the derivation case the explicit-:long test misses: flags: [dry_run: [...], "dry-run": [...]] both derive --dry-run.

2 — unknown keys one level up and one level down (1bb6770)

This is a deliberate scope extension and the PR body now says so. Both instances are pre-existing rather than introduced here, but they are the same class #65 was filed about and the @flag_spec_keys idiom applies verbatim.

Reproduced: visible: (no ?) built clean and probe admin exited 0 printing SECRET for every caller; flgs: built a flagless command; %{name: :path, requird: true} built clean and probe go exited 0 with path=nil.

command/2, new/2, and normalize_arg!/2 now go through the same guard, which was extracted to a shared validate_spec_keys!/4 (so they also get finding 5's duplicate diagnosis for free):

raised: command "admin": unknown option :visible; valid options are [:doc, :commands, :run, :flags, :args, :examples, :validate, :allow_unknown_flags, :visible?, :on_missing_subcommand]
raised: CLI "probe": unknown option :comands; valid options are [:doc, :commands, :aliases, :on_missing_subcommand]
raised: command "go": unknown positional argument option :requird; valid options are [:name, :doc, :required, :variadic]

3 — enumerated positive tests (85fafcb)

Reproduced the mutation before writing anything: deleting :transform, from @flag_spec_keys left the suite green at 2 doctests, 62 properties, 4787 tests, 0 failures, while probe go --n hi through CLI.command/2 returns exit=0, "n=HI\n" — so the key is real and unprotected.

Each of the four allowlists is now enumerated twice over: every key exercised positively, and the builder's own valid options are list asserted equal to the enumeration, so a key added or dropped on either side goes red. Flag-spec keys are built one at a time (for key <- @flag_spec_keys) and all-at-once in two variants, since :required/:default are mutually exclusive. Command options are split across a leaf and a group (some are leaf-only, some group-only) with an assertion that the union is exactly the allowlist. :transform is held in a defp flag_spec_value/1 rather than a module attribute — a function capture cannot be escaped into one.

Mutations re-run against the finished tests:

mutation	result
drop :transform from @flag_spec_keys	3 failures
drop :visible? from @command_opt_keys	2 failures
drop :variadic from @arg_spec_keys	3 failures

4 — = in a long form or alias (a26c4d8)

Reproduced, and the :long hole the review asked about is real too — long: "--target=date" builds clean, is unreachable, and the dead spelling is printed in the usage line. Both now raise:

raised: command "go" flag :target_on: alias "--target=date" cannot contain "=" — the parser splits a long flag on "=" before matching it
raised: command "go" flag :target_on: long form "--target=date" cannot contain "=" — the parser splits a long flag on "=" before matching it

"--=" falls out of the same check. Kept scope tight: this adds no ---prefix requirement to :long, only the = rejection.

5 — duplicate-key diagnosis (884231e)

Reproduced verbatim. Enum.uniq/1 before the subtraction, plus a dedicated duplicate clause:

raised: command "x" flag :n: duplicate flag option :type

Gates

$ mix compile --warnings-as-errors --force
Compiling 168 files (.ex)
Generated just_bash app                                  # exit 0

$ mix format --check-formatted                           # exit 0

$ mix credo --strict
5133 mods/funs, found 1 refactoring opportunity.
# the pre-existing intentional test fixture finding: test/support/banned_fixture_apply.ex:4

$ mix test
2 doctests, 62 properties, 4796 tests, 0 failures (5 excluded)
# baseline was 4774; +22 new tests, no regressions

$ mix dialyzer
Total errors: 13, Skipped: 13, Unnecessary Skips: 0
done (passed successfully)

[davydog187]

Verification of review fixes

Independently re-ran every repro at 85fafcb, and re-ran each one at beca739 (the head the
review was written against) to confirm the "before" state myself. All five findings are fixed.

Repro harness: a probe CLI built with CLI.new/2 + CLI.command/2, registered as
JustBash.new(commands: %{"probe" => cli}) and driven through JustBash.exec/2.

#	Verdict	Evidence (beca739 → 85fafcb)
1 — :long/:short collisions unchecked	fixed	flags: [a: [long: "--dup"], b: [long: "--dup"]] — before: built clean, probe go --dup x → exit=0 stdout="a=nil b=\"x\"". Now: ArgumentError: command "go" flag :b: long form "--dup" collides with an existing flag long form or alias. Duplicate :short — before: probe go -x → exit=0 stdout="a=false b=true"; now ... flag :b: short form "-x" collides with an existing flag short form. Derived case [dry_run: …, "dry-run": …] — before: probe go --dry-run → exit=0 stdout="%{dry_run: false, \"dry-run\": true}"; now raises on --dry-run. Mutation-checked: reverting the fold to do_validate_flag_collisions!(name, aliased, longs) (dropping the shorts pass) turns the three new tests red (139 tests, 3 failures).
2 — unknown keys in command/2 / new/2 / :args	fixed	visible: (no ?) — before: built clean, probe admin → exit=0 stdout="SECRET\n"; now ArgumentError: command "admin": unknown option :visible; valid options are [:doc, :commands, :run, :flags, :args, :examples, :validate, :allow_unknown_flags, :visible?, :on_missing_subcommand]. flgs: — before: built clean, probe go --n hi → exit=2 "unknown option: --n"; now raises unknown option :flgs. %{name: :path, requird: true} — before: built clean, probe go → exit=0 stdout="path=nil\n"; now command "go": unknown positional argument option :requird; valid options are [:name, :doc, :required, :variadic]. CLI.new("probe", command: []) — before: built clean; now CLI "probe": unknown option :command; …. @arg_spec_keys matches t:JustBash.CLI.Command.arg_spec/0 exactly; @command_opt_keys matches every key command/2 reads. Flagged in the PR body as a scope extension, as asked.
3 — allowlists unpinned, :transform unprotected	fixed	Mutation-verified in both directions against mix test test/cli/, which is green (139 tests, 0 failures) at HEAD. Deleting :transform from @flag_spec_keys → 139 tests, 3 failures (a flag spec carrying every key at once is accepted, every flag-spec key is accepted on its own, the builder reports exactly the enumerated flag-spec keys as valid). Deleting :visible? from @command_opt_keys → 9 failures. Deleting :variadic from @arg_spec_keys → 3 failures. Additive direction also pinned: adding :bogus to @arg_spec_keys → 1 failure (left: "… valid options are [:name, :doc, :required, :variadic, :bogus]").
4 — = in an alias / long form	fixed	aliases: ["--target=date"] — before: built clean; probe go --target=date 1 → exit=2 "unexpected argument(s): 1". (One correction to the original write-up: probe go --target=date=1 exited 0 with t="date=1", not 2 — it split on the first = and bound the canonical --target. The alias itself was still unreachable, so the finding stands.) Now: command "go" flag :target: alias "--target=date" cannot contain "=" — the parser splits a long flag on "=" before matching it. long: "--target=date" — before: built clean and advertised the dead spelling (Usage: probe go [--target=date <string>]); now raises the same message with long form. Mutation: no-op'ing reject_equals! turns both new tests red.
5 — duplicated valid key reported as unknown	fixed	flags: [n: [type: :integer, type: :string]] — before: command "x" flag :n: unknown flag option :type; valid options are [:type, …]. Now: command "x" flag :n: duplicate flag option :type. Mutation: replacing unique = Enum.uniq(keys) with unique = keys turns the new test red (left: "… unknown flag option :type …"). Duplicates are diagnosed at the other three call sites too: CLI.command("go", doc: "a", doc: "b") → command "go": duplicate option :doc; CLI.new("probe", doc: "a", doc: "b") → CLI "probe": duplicate option :doc.

Gates — re-run here, at 85fafcb, clean tree

$ mix compile --warnings-as-errors
(no output, exit 0)

$ mix format --check-formatted
(no output, exit 0)

$ mix credo --strict
5133 mods/funs, found 1 refactoring opportunity.
  [F] Avoid `apply/2` and `apply/3` … test/support/banned_fixture_apply.ex:4:16

exit 8 — the single finding is the intentional banned-call test fixture, introduced in
5b2393d and untouched by this branch.

$ mix test
Finished in 28.5 seconds (28.2s async, 0.3s sync)
2 doctests, 62 properties, 4796 tests, 0 failures (5 excluded)

$ mix dialyzer
Total errors: 13, Skipped: 13, Unnecessary Skips: 0
done (passed successfully)

Collateral damage

None found. A full leaf exercising every allowlisted key (doc, flags with
default/values/aliases/short, args, examples, validate, run) still builds,
routes, parses --format json, --fmt=json, -v, and renders help unchanged. No CLI is
defined in lib/ outside the moduledoc, so the new guards have no in-repo callers to break.

Residual gaps (pre-existing, not introduced here — noting, not blocking)

1. :examples entries are not key-guarded. examples: [%{cmd: "probe go", dco: "typo"}]
   builds clean and yields %{cmd: "probe go", doc: nil} — the same silent drop as
   requird:, one map over. Outside the three sites finding 2 named, so this PR is complete
   as specified.
2. Short-vs-long is a cross-map collision the guard doesn't cover, because
   parse_short_flag/6 falls back to long_map:
   flags: [a: [type: :boolean, short: "-v"], b: [type: :boolean, long: "-v"]] builds clean
   and probe go -v → exit=0 stdout="a=true b=false\n". Contrived (a single-dash :long),
   and true on main too.