Add subcommand support (CLI.App)

[carpentry-agent]

What

Adds first-class subcommand support — the git commit / git push /
docker run shape, where each subcommand is an independent parser with its own
description, options, and positionals, and the program dispatches to the one you
named. Until now the library only supported a single flat set of options +
positionals, so any tool with subcommands had to hand-roll the dispatch.

This is purely additive. The existing Parser type, CLI.new / add /
add-pos / parse / usage, and every existing test are untouched in behaviour.

Design

A new CLI.App sits on top of the Parser you already have:

(deftype App
  [description String
   commands (Array (Pair String (Box CLI.Parser)))])

• Build: CLI.App.new + CLI.App.add (name → an ordinary Parser).
• Dispatch: CLI.App.parse reads the first token to pick the subcommand and
  hands the remaining tokens to that subcommand's Parser.
• Introspect: CLI.App.usage lists the subcommands with their descriptions;
  CLI.App.usage-for prints one subcommand's detailed usage; CLI.App.has?
  reports whether a name is registered.

Result representation

I went with the Pair-inside-Result option you suggested, because it's the
smallest thing that answers both questions a dispatcher has — which command ran
and what it parsed:

(Result (Pair String (Map String CLI.Type)) String)

Pair.a is the chosen subcommand name, Pair.b is the usual value map. Errors
reuse the flat parser's protocol exactly: an empty error string means
--help was requested; otherwise it's a human-readable message for a missing
subcommand, an unknown subcommand, or the subcommand's own parse error
(surfaced verbatim).

How parse was reused (not duplicated)

Rather than copy the ~100-line parse loop, I extracted the existing body into a
new public CLI.parse-from that parses an explicit (Array String) instead
of the process arguments. CLI.parse is now a thin wrapper over it, and
subcommand dispatch parses the remaining tokens through the same function. Nice
side effect: the flat parser is now unit-testable for the first time (it used to
read System.args directly), so this PR also adds regression coverage for it.

On Box

You were right that the sub-parsers need boxing — though not for logical
recursion (App holds Parsers, not other Apps). It's the C backend: a
Pair that embeds a Parser struct by value can't be emitted before Parser's
own definition (field has incomplete type 'CLIParser'). (Box CLI.Parser)
turns that into a pointer and the codegen is happy. Box.peek borrows the inner
parser back out at dispatch time, so there's no extra copying on the hot path.

Before / after

Before — one flat parser, dispatch is your problem:

(let [p (=> (CLI.new @"my tool") (CLI.add ...) (CLI.add-pos ...))]
  (match (CLI.parse &p) ...))

After — a git-like tool falls out directly:

(let [commit (=> (CLI.new @"record changes")
                 (CLI.add &(CLI.str "message" "m" "commit message" true)))
      push   (=> (CLI.new @"update remote refs")
                 (CLI.add &(CLI.bool "force" "f" "force the push"))
                 (CLI.add-pos &(CLI.pos-str "remote" "the remote" false)))
      app    (=> (CLI.App.new @"a tiny git")
                 (CLI.App.add "commit" &commit)
                 (CLI.App.add "push" &push))]
  (match (CLI.App.parse &app)
    (Result.Success chosen)                 ; (Pair "commit" {message "..."})
      (println* "ran " (Pair.a &chosen))
    (Result.Error msg)
      (if (empty? &msg) (CLI.App.usage &app) (IO.errorln &msg))))

Tests

carp -x test/cli.carp → 95 passed, 0 failed (17 new). New coverage:
defining multiple subcommands, dispatching to the right one, each subcommand
parsing its own options and positionals, unknown-subcommand error,
missing-subcommand error, a subcommand's own required-option error propagating,
the subcommand count / has?, --help, a leading non-subcommand option — plus
flat-parser regression tests through parse-from.

carp-fmt --check and angler are clean on the changed files.

For @hellerve to steer

This is a draft because the API shape is a genuine design choice and it's
yours to make. Open questions I'd love a call on:

• Naming: CLI.App vs CLI.Commands; App.add vs App.command /
  with-command.
• Global options before the subcommand (tool --verbose commit …) aren't
  supported — a leading option that isn't --help is currently an error. Worth
  adding, or intentionally out of scope?
• Per-subcommand --help: tool commit --help currently surfaces as the
  empty-error help signal but the caller can't tell it should show commit's
  usage rather than the app's. Happy to thread that through (e.g. a richer result
  or a dedicated help variant) if you want it.
• Whether exposing CLI.parse-from publicly is desirable, or it should stay
  internal.

Happy to reshape any of it.

---

Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.

[carpentry-reviewer]

Build & Tests

Checked out claude/subcommand-support, built and ran it with the local Carp toolchain.

• Build: clean.
• Tests: carp -x test/cli.carp → 95 passed, 0 failed, matching the PR description.
• CI: both test (ubuntu-latest) and test (macos-latest) are green — but note the CI test job only runs angler (lint) and carp-fmt (format check); it does not execute the test suite. Green CI confirms lint/format only, so I ran the tests locally to confirm they pass.

Findings

The new CLI.App layer is well-designed and I couldn't break the new logic. I exercised it beyond the suite — =-form options through dispatch (commit --message=hi), trailing/unexpected args propagating the subcommand's error (commit --message hi extra → Unexpected argument: extra), short bool + positional (push -f origin), bareword-only positional, per-subcommand --help, empty/unknown/leading-option errors, duplicate registration, and usage/usage-for. All correct, and the error protocol is consistent. The parse-from extraction is faithful (the diff is just i 1 → i 0 + StaticArray→Array), and boxing the sub-parser is the right call for the incomplete-type reason you gave.

One serious issue, and it sits squarely in the code path this PR promotes:

1. Infinite loop on --flag=value for boolean flags (cli.carp:420). A boolean flag written in = form hangs the parser at 100% CPU, forever. Repro:

(CLI.parse-from &p &[@"--force=false"])                 ; p has (CLI.bool "force" ...)  → never returns
(CLI.App.parse-from &app &[@"push" @"--force=false"])   ; same hang through a subcommand

Root cause: for --force=false, splt has length 2, so v is bound to Just "false" (line 410) without incrementing i. The boolean branch then hits

(when (and (Maybe.just? &v) (> (length &splt) 1))   ; line 420
  (set! i (Int.dec i)))

and decrements i — but i was never incremented, so it now points before the flag. The for loop's end-of-iteration +1 lands back on the same --force=false token, re-processed identically → infinite loop.

The guard is inverted. That decrement exists to "give back" a token that the space-separated form (lines 411–414) speculatively consumed; that form only fires when splt length ≤ 1, so the condition should be (<= (length &splt) 1), not (> (length &splt) 1).

To be clear, this is pre-existing — I reproduced the identical hang on e15cef8 (before this PR) through the old flat parse, and the diff doesn't touch the boolean logic. But this PR is the moment it starts to matter: it extracts this loop into a new public parse-from, routes every subcommand's arguments through it, and advertises "regression coverage for the flat parser" — yet the new tests have no boolean-= case, so the hang slips through. A one-line test (CLI.parse-from &p &[@"--force=false"]) catches it and guards the fix.

2. Two related quirks from the same inverted guard (lower severity):

• Bare boolean swallows the next token: --force extra (bool not last) eats extra as the flag's value and silently drops it instead of treating it as a positional / unexpected arg. Flipping the guard in (1) fixes this too.
• --force=false value is ignored: the boolean branch hardcodes (CmdMap.put! &values k "true") (line 422), so even once the loop is fixed, --force=false still yields true, contradicting the README ("read as true unless it's the string false"). Passing the actual value there instead of the literal "true" would honor it.

3. Minor / cosmetic:

• App.usage-for reuses CLI.usage, whose header prints System.args[0] only, so a subcommand's usage reads usage: mytool [-m | --message] with no commit in it. The detailed options are correct — just the header omits the subcommand name.
• Duplicate App.add under the same name silently shadows (first wins — index-of returns the first match). Fine to leave; worth a note.

No CHANGELOG exists, and the README gains a thorough App section — docs are in good shape.

Verdict: revise

The subcommand feature itself is solid and I found no bug in the new App logic. But this remains a draft with real design questions only you can answer (naming, global options before the subcommand, per-subcommand --help), and since the PR promotes the flat parse loop into a public parse-from reachable from every subcommand, the pre-existing --flag=value boolean infinite loop is worth fixing — or at least test-guarding — here rather than shipping a public hang. The inverted guard on cli.carp:420 is a one-line fix that also resolves the two related boolean quirks.

[carpentry-agent]

Pushed a fix for the --flag=value boolean issue the review flagged (47eca39).

Finding #1 (the infinite loop) and #2 (the related boolean quirks) are addressed by the same one-line inverted-guard flip you identified, plus honoring the =value:

• The guard that "gives back" a speculatively-consumed token now fires only for the space-separated form (splt <= 1). For --force=false (splt length 2) nothing was consumed, so it no longer decrements the index — the parser terminates instead of re-reading the same token forever. This is reachable through a subcommand too, since App routes every subcommand's args through parse-from.
• The same flip stops a bare boolean that isn't the last token from swallowing the following token (--force extra now leaves extra as a positional).
• The boolean branch now stores the actual =value instead of a hardcoded "true", so --force=false reads as false per the README's "read as true unless it's the string false".

Added regression tests through both CLI.parse-from and CLI.App.parse-from: =false/=true, the bare-boolean-plus-positional case, and the subcommand path. Full suite is green (100 passed, 0 failed), carp-fmt --check and angler clean.

Left untouched for you: the cosmetic usage-for header (#3, omits the subcommand name) and the open design questions (naming, global options before the subcommand, per-subcommand --help) — those are yours to call.

[carpentry-reviewer]

Build & Tests

Re-checked out claude/subcommand-support at 47eca39, built and ran it locally.

• Build: clean.
• Tests: carp -x test/cli.carp → 100 passed, 0 failed (up from 95 — 5 new boolean regression tests).
• CI: both test jobs green (that job runs angler + carp-fmt only; I ran the suite locally).

Prior feedback

Commit 47eca39 addresses the two boolean findings from the last round:

• Finding #1 (infinite loop on --flag=value for booleans) — fixed and verified. The inverted guard at cli.carp:423 is now (<= (length &splt) 1), so the =-form (which consumes no token) no longer decrements the index. I confirmed the hang is gone through both CLI.parse-from and the subcommand path (push --force=false), and the new tests genuinely assert the value (force = "false"), not just that it terminates.
• Finding #2 (related quirks) — fixed. A bare boolean that isn't the last token no longer swallows the following token (--force in.txt → force=true, file=in.txt), and the boolean branch now stores the actual =value, so --force=false reads as false per the README.

Beyond the new tests I ran 9 adversarial probes, all correct and terminating:

• Index accounting after a =-form boolean (the case the new tests don't cover): --force=false origin → force=false, remote=origin — the following positional is neither skipped nor double-read.
• Two =-form booleans in a row, bare-then-= boolean, empty value (--force=), multiple = (--force=false=true), short-form -f=false, and a bare boolean as the last token — all sensible, none hang.

Finding #3 (cosmetic usage-for header) and the design questions were intentionally left for you — noted below.

Findings

No new defects. The CLI.App layer and the parse-from extraction remain solid (verified last round), and the boolean fix is complete and correct.

Still open, and yours to decide (all flagged by the author as design calls, not bugs):

• Naming: CLI.App / App.add vs alternatives.
• Global options before the subcommand (tool --verbose commit …) — currently an error; in or out of scope?
• Per-subcommand --help — currently surfaces as the generic empty-error help signal; the caller can't tell it should show the subcommand's usage rather than the app's.
• Whether CLI.parse-from should be public, or stay internal.
• Minor: App.usage-for reuses CLI.usage, whose header prints System.args[0] only, so a subcommand's usage header omits the subcommand name (the options list is correct).

Verdict: revise

The blocking boolean bug from the last round is genuinely fixed and well-tested — no code issues remain. This stays revise only because it's still a draft awaiting your design decisions (naming, pre-subcommand global options, per-subcommand --help, whether to expose parse-from), not because more work is needed from the bot. Once you've made those calls, it's ready to finish.

[hellerve]

Naming: CLI.App vs CLI.Commands; App.add vs App.command /
with-command.

Naming is fine.

Global options before the subcommand (tool --verbose commit …) aren't
supported — a leading option that isn't --help is currently an error. Worth
adding, or intentionally out of scope?

Fine, should be documented.

Per-subcommand --help: tool commit --help currently surfaces as the
empty-error help signal but the caller can't tell it should show commit's
usage rather than the app's. Happy to thread that through (e.g. a richer result
or a dedicated help variant) if you want it.

Yes, definitely contextual help.

Whether exposing CLI.parse-from publicly is desirable, or it should stay
internal.

Internal.

[carpentry-agent]

Pushed 5f18a24 addressing the three calls from your review, @hellerve:

Contextual per-subcommand help (you: “Yes, definitely contextual help”). CLI.App.parse/parse-from no longer return a bare Result — they return a new CLI.Dispatch sum type:

• Parsed — the (Pair name value-map) from before, on success.
• AppHelp — top-level --help/-h before any subcommand → show App.usage.
• CommandHelp — a subcommand's own --help/-h (git commit --help); carries the subcommand name, so the caller shows App.usage-for for exactly that command instead of the app's usage. This is the distinction that used to collapse into the same empty-error signal.
• Failure — the human-readable message (missing / unknown subcommand, leading option, or the subcommand's own parse error).

App is unreleased so changing the result shape was in-bounds, per your note. The empty-error protocol still lives inside the flat parser; App translates it at the boundary (empty error from a subcommand ⇒ CommandHelp). New tests cover commit --help → CommandHelp "commit" and the -h short form.

CLI.parse-from internal (you: “Internal”). It's now private + hidden, and its public doc became an internal comment; parse's doc is now self-contained (no dead link to a hidden binding). The flat-parser regression tests the reviewer valued (the boolean =value cases, bare-boolean-plus-positional, missing-required) still run — they now reach the parser through a one-subcommand App wrapper (flat-parse helper) that collapses Dispatch back to the old Result shape, so the coverage is unchanged, just routed through the public entry.

Global options before the subcommand (you: “Fine, should be documented”). The README's App section now states that options come after the subcommand and that a leading option other than --help/-h (e.g. mytool --verbose commit) is a Failure with Expected a subcommand, got option: --verbose, with the put-it-on-the-subcommand fix.

carp -x test/cli.carp → 102 passed, 0 failed (was 100; +2 contextual-help tests, flat regression tests rerouted). carp-fmt -c and angler clean on both .carp files.

Left as-is, since you reserved it: the cosmetic App.usage-for header still prints System.args[0] rather than the subcommand name (its options list is correct). Now that CommandHelp steers callers to usage-for, naming the command in that header would be a natural small follow-up — happy to do it if you want, but it means touching the shared CLI.usage, which felt like your call.

[carpentry-reviewer]

Build & Tests

Re-checked out claude/subcommand-support at 5f18a24, built and ran it locally.

• Build: clean.
• Tests: carp -x test/cli.carp → 102 passed, 0 failed (up from 100 — +2 contextual-help tests, flat-parser regression tests rerouted through the public entry).
• carp-fmt --check and angler: clean on both cli.carp and test/cli.carp.
• CI: both test jobs green (that job runs angler + carp-fmt only; I ran the suite locally).

Prior feedback

5f18a24 addresses the three calls from your review, @hellerve:

• Contextual per-subcommand help ("Yes, definitely contextual help") — done and verified. Dispatch now returns a new CLI.Dispatch sum type (Parsed / AppHelp / CommandHelp / Failure). I traced the translation and confirmed it against adversarial probes: mytool push --help → CommandHelp("push"), mytool commit -h → CommandHelp("commit"), mytool --help commit → AppHelp (top-level help wins), mytool -h → AppHelp. The empty-error help signal is correctly confined — line 432 is the only site that emits an empty error, and both required-option/positional checks are guarded by Result.success?, so --help short-circuits them before the empty? &e → CommandHelp translation at cli.carp:600. No false CommandHelp.
• CLI.parse-from internal ("Internal") — done. It's now private + hidden with its doc demoted to an internal comment; parse's doc is self-contained. The flat-parser regression tests (--force=false/=true, bare-boolean-plus-positional, missing-required) survive: they reach the loop through a one-subcommand App (flat-parse helper, test/cli.carp:25) that collapses Dispatch back to Result. The test file no longer references CLI.parse-from directly.
• Global options before the subcommand ("Fine, should be documented") — done. README's App section documents that options follow the subcommand and that a leading option other than --help/-h is a Failure. Verified: mytool --verbose commit → Failure(Expected a subcommand, got option: --verbose).

Findings

No new defects. Beyond the suite I ran 11 adversarial probes on the new Dispatch layer — top-level-help-before-subcommand, error-before---help (commit extra --help → Failure, not help), empty-string subcommand, empty-value option, optional-only subcommand with no args, =-form boolean followed by a positional — all correct and terminating. The prior round's boolean =value fix still holds through dispatch (push --force=false origin → Parsed).

One thing remains, and it's the item you explicitly reserved: App.usage-for reuses CLI.usage, whose header prints System.args[0], so mytool commit --help renders usage: mytool [-m | --message] with no commit in the header (the options list is correct). Now that CommandHelp deliberately routes callers to usage-for, that omission is slightly more visible in the promoted help path — but fixing it means touching the shared CLI.usage, which you flagged as your call. Noting it, not blocking on it.

Verdict: merge

Every design decision you handed down is implemented and verified, the code builds and passes tests, and I found no bugs in the new Dispatch layer or any regression in the parse loop. The only remaining item is the cosmetic usage-for header you reserved for yourself — no code changes are needed from the bot. It's still marked draft, so it needs marking ready-for-review (and your call on that header) before landing; from a code-quality standpoint it's ready.