feat(cli): add ArgSpec typed argument-spec builder for CLI subcommands - #2862
Conversation
LuCLI already hands every module function a structured argument map — positionals as `arg1, arg2, ...`, `--key=value` as `key=value`, and `--no-key` normalized to `key=false`. `Module.cfc::argsFromCollection()` has historically flattened that map back to argv so each subcommand could re-parse it with a hand-rolled token loop, a round trip that silently dropped every `false` value (the root cause of #2855) and could not distinguish `--no-X` from an explicit `--X=false`. `ArgSpec` consumes the structured handoff directly. Each command declares its positionals, flags, and options up front and calls `.parse(arguments)` to receive a typed result struct — no flatten, no re-parse, no lossy `false` round trip. Designed for incremental adoption: `getArgs()` / `argsFromCollection()` remain as a deprecated shim until every call site is converted. Cross-engine clean (no closures, no struct-member collisions, no `application`-scope function storage, no `attributeCollection = arguments`); boolean coercion handles both the string `"false"` LuCLI normally emits and a literal `false` value, so Lucee/Adobe/BoxLang agree. Required-positional violations throw `Wheels.CLI.MissingArgument`. The cross-framework research that informed the API surface (Rails/Thor, Laravel/Artisan, Django/argparse, Phoenix/Mix, Spring/picocli, Symfony Console) is recorded on the issue. Refs #2861. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — No doc updatesReviewed this PR's diff and found no docs that need updating (internal CLI infrastructure — |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The ArgSpec service is a well-scoped solution to the #2855 false-value round-trip bug. The implementation is clean, the BDD spec covers the regression surface precisely, and the cross-engine commentary in the source is thorough. However, the PR has a correctness defect in CHANGELOG.md that must be fixed before merge: the ArgSpec entry was written as a replacement of the existing #2833 "Reserved scope names" entry rather than a new entry before it, deleting that prior entry from the changelog and leaving a dangling sentence fragment at the tail of the ArgSpec bullet. There are also two minor concerns about dead code and dot-notation access of a keyword-named key. Would request changes, but cannot self-review — flagging for a human maintainer.
Correctness
CHANGELOG.md:25 — existing entry for #2833 deleted; ArgSpec entry corrupted
The diff replaces the existing [Unreleased] → ### Added bullet for #2833 ("A 'Reserved scope names' section...") with the new ArgSpec bullet, rather than prepending a new bullet before it. As landed, the CHANGELOG:
- Loses the #2833 entry entirely — that documented change is now unrecorded.
- Ends the ArgSpec bullet with a dangling fragment absorbed verbatim from the deleted entry:
...is recorded on the issue (#2861) documenting identifiers (`client`, `url`, `form`,
`session`, `cgi`, `request`, `application`, `cookie`, `server`, `arguments`, `variables`,
`local`, `this`) that must not be used as local variable names in Wheels controllers…
(#2833)
The correct edit is to insert a new ArgSpec bullet above the unchanged #2833 bullet (not replace it), and truncate the ArgSpec text at "...is recorded on the issue (#2861)."
Cross-engine
ArgSpec.cfc:86, 102 — dot-notation access on a struct key named default
default is a cfscript keyword (the default: label in switch blocks). The component uses it freely in dot-notation member access:
// line 86
result[optName] = variables.named[optName].default;
// line 102
result[pSpec.name] = pSpec.default;In well-implemented CFML parsers, member-access context is unambiguous from switch-statement context, so this is likely fine on Lucee and BoxLang. Adobe CF's cfscript parser has a history of keyword-collision quirks (anti-patterns #4 and #5; the attributeCollection = arguments fix across 13 sites in #2750). Since the bot sandbox could not run the suite against Adobe CF 2023/2025, this path has not been exercised on the at-risk engines.
The safe fix is bracket notation throughout, which is unambiguous on every engine:
result[optName] = variables.named[optName]["default"];
result[pSpec.name] = pSpec["default"];Alternatively, rename the parameter and key to defaultValue — that also resolves the {default = arguments.default} struct-literal usages at lines 48, 60, and 73.
Conventions
ArgSpec.cfc:57, 71 — aliases declared but never resolved
flag() and option() both accept and store an aliases string:
// flag(), line 57
string aliases = ""
...
variables.named[arguments.name] = {
...
aliases = arguments.aliases
};parse() (lines 109–115) never consults aliases when matching keys from the input struct. The stored value is inert dead code. CLAUDE.md: "Don't add features, refactor, or introduce abstractions beyond what the task requires." Either drop aliases from both signatures and the stored spec now, or open a follow-up issue and add a skip comment — but don't ship a parameter that has no runtime effect.
Tests
The spec covers the target regression surface well: default-absent, string "true", string "false" (the #2855 surface), and literal false. The bot-sandbox caveat (manual trace only, CI is the gate) is documented and understandable given the constraint. No objection here beyond the engine note above.
Commits
Single commit feat(cli): add ArgSpec typed argument-spec builder for CLI subcommands — type, scope, and subject are correct; header is within the 100-char limit; DCO sign-off is present. No issues.
Docs
Framework Docs, AI Reference Docs, and CLAUDE.md deferred to bot-update-docs.yml — acceptable for a service component with no public API surface change. CHANGELOG entry must be repaired as noted above.
PR labels are bug + docs, but this is a feature addition. Minor metadata nit; correct label would be enhancement or no label.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-grounded. All three findings check out against the diff. Converging on request-changes. SycophancyNone detected. A did not LGTM without evidence. A correctly declined to self-approve and surfaced real issues instead. False positivesNone detected.
Missed issuesNone substantive. One minor observation for completeness: Verdict alignmentA's "would request changes, cannot self-review" framing is honest and consistent with the findings. The CHANGELOG bug and ConvergenceAligned. A's three findings are all confirmed by the diff. The changes needed are concrete:
|
- CHANGELOG.md: restore the #2833 "Reserved scope names" bullet that was collapsed into the ArgSpec entry; truncate the ArgSpec bullet at the research footnote so the two changes are recorded as separate Unreleased -> Added entries again. - cli/lucli/services/ArgSpec.cfc: switch every read of the `default` key to bracket notation (`pSpec["default"]`, `variables.named[opt]["default"]`) and quote the struct-literal keys in positional()/flag()/option() so Adobe CF's cfscript parser never sees a bare `default` identifier in member-access or struct-key position. - cli/lucli/services/ArgSpec.cfc: drop the `aliases` parameter and stored key from flag() and option() — parse() never reads it, so the field was inert dead code (CLAUDE.md: "Don't add features beyond what the task requires"). Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Test runner caveat: The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on `approve` or the outer-loop cap (5 rounds) is reached. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is the second look at PR #2862 (head cd53c714), which adds the round-1 fixup commit on top of the original ArgSpec feature. The three consensus findings from the first pass — CHANGELOG collision, default keyword in dot-notation, and the dead aliases code — are all cleanly resolved. Two minor style inconsistencies remain, but neither is a correctness or cross-engine defect. Verdict: comment (no blocking findings; marking for human merge decision given the bot self-review context).
Correctness
All three correctness/cross-engine findings from round 1 are closed:
CHANGELOG.md — two bullets now distinct. The #2833 "Reserved scope names" entry is restored as a separate bullet (line 26). The ArgSpec bullet on line 25 now ends cleanly at (#2861) without absorbing the prior entry. No dangling fragments.
default bracket notation — applied. Every read of the default key in struct literals, stored specs, and the result-seeding loop now uses bracket notation (pSpec["default"], variables.named[optName]["default"]). The keyword collision risk on Adobe CF's cfscript parser is eliminated.
aliases dead code removed. Both flag() and option() no longer accept or store an aliases parameter. The stored struct for each named spec now contains only "default" and "type".
Cross-engine
Minor nit — dot-notation reads for type and required are inconsistent with the defensive pattern established by the fix, but neither is a reserved keyword.
The round-1 fix quoted all struct-literal keys ("name", "required", "default", "type") and switched the two default-key reads to bracket notation. However, the member-access reads for type and required were left as dot-notation:
// ArgSpec.cfc:91
result[pSpec.name] = $coerce(arguments.coll[collKey], pSpec.type);
// ArgSpec.cfc:92
} else if (pSpec.required) {
// ArgSpec.cfc:110
result[key] = $coerce(arguments.coll[key], variables.named[key].type);Neither type nor required is a switch-statement label keyword the way default is, so all supported engines (Lucee 5/6/7, Adobe CF 2018-2025, BoxLang) will resolve these correctly. This is a style nit rather than a bug — but for full consistency with the defensive bracket-notation pattern already applied, these could be written as pSpec["type"], pSpec["required"], and variables.named[key]["type"]. No change required before merge.
Tests
Test coverage is correct and the base class wheels.wheelstest.system.BaseSpec matches every other spec under cli/lucli/tests/specs/services/ (confirmed against AnalysisSpec, HelpersSpec, ReleaseChannelSpec, etc.). The four flag-coercion cases (absent to default, string "true", string "false" — the regression surface — literal boolean false) are all exercised. Required-positional throw and unknown-key isolation are covered. No gaps.
Commits
Both commits have DCO sign-off and use types from the commitlint allowlist. Headers are within the 100-char limit. The fixup commit body names each individual change, which is clear and appropriate. No commitlint violations.
Docs
CHANGELOG.md entry is correct post-fix. Framework docs and CLAUDE.md deferred to bot-update-docs.yml — acceptable for a CLI service with no public framework API surface change.
Wheels Bot -- Reviewer B (round 1)A's second-pass review is clean. All three round-1 findings (CHANGELOG collision, SycophancyNone detected. A verified each finding individually against the diff rather than rubber-stamping the address-review commit. A correctly declined to self-approve per bot self-review policy and is flagging for human merge decision. False positivesNone detected.
Missed issuesNone detected.
Verdict alignmentA's "comment" verdict (no blocking findings; awaiting human merge decision) is consistent with the absence of any actionable findings. Bot self-review restraint is procedurally correct, not evasion. ConvergenceAligned. A has no unresolved findings; B has no disputes or missed issues. The joint recommendation is that this PR is review-clean and a human reviewer can merge. Emitting |
The comment described the pre-#2856 state, claiming the flatten step drops "false" values and re-emits nothing. #2856 added a "--no-key" re-emit workaround, so reword to reflect the actual history (original drop -> #2856 re-emit -> ArgSpec removes the round-trip). Comment-only; no behavior change. Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round 3 (head aad161047). The only change since round 2 is a comment-only correction in ArgSpecSpec.cfc — no behavior change, no CFML code change. All three blocking findings from round 1 remain closed, and the round-2 style nit was already marked non-blocking. This pass is clean. Verdict: comment (no blocking findings; retaining comment verdict given the self-review context — human maintainer approval still required).
Correctness
New commit aad161047 — comment correction verified accurate.
The stale comment at ArgSpecSpec.cfc:69-71 (pre-fix) read:
// This is the #2855 regression surface: the current
// argsFromCollection() flatten step DROPS "false" values
// and re-emits nothing. ArgSpec must preserve the negation.This was factually wrong after PR #2856 landed: that PR patched the flatten step to re-emit --no-key instead of dropping the value silently. The corrected comment now reads:
// This is the #2855 regression surface. argsFromCollection()'s
// flatten step originally DROPPED "false" values outright; #2856
// patched that by re-emitting "--no-key". ArgSpec removes the
// round-trip entirely, so the negation survives structurally.That accurately chronicles the three-step history (original drop, the #2856 re-emit workaround, and ArgSpec's structural fix) and is consistent with the PR body. No functional code was touched.
Round 1 findings — all closed (carried for audit trail)
- CHANGELOG.md collision — resolved; two separate bullets, no fragment absorption.
defaultdot-notation — resolved; bracket notation (pSpec["default"],variables.named[optName]["default"]) applied throughout.aliasesdead code — resolved; parameter removed from bothflag()andoption().
The round-2 style note (type and required still use dot-notation — neither is a switch keyword, so no cross-engine risk) remains a non-blocking nit. No change required.
Commits
Three commits, all valid:
| SHA | Type | Subject | DCO |
|---|---|---|---|
e9f6d51f |
feat(cli) |
add ArgSpec typed argument-spec builder for CLI subcommands | present |
cd53c714 |
fix(cli) |
address Reviewer A/B consensus findings (round 1) | present |
aad16104 |
docs(test) |
correct stale argsFromCollection comment in ArgSpecSpec | present |
docs(test) is the correct type for a comment-only change in a spec file. All headers are within the 100-char limit.
Tests
No behavioral change in this commit. The four flag-coercion cases (absent to default, string "true", string "false" — the regression surface — literal boolean false) remain covered. Required-positional throw, optional-positional default, numeric coercion, and unknown-key isolation are all exercised. No gaps identified.
Wheels Bot — Reviewer B (round 1)A's round-3 review is accurate and complete. The only change on this SHA was the comment correction in SycophancyNone detected. A verified each claim individually against the diff and explicitly deferred to a human for the final merge decision per bot self-review policy. False positivesNone detected.
Missed issuesNone detected.
Verdict alignmentA's "comment" verdict (no blocking findings; human approval required) is consistent with the absence of any actionable findings and the bot self-review policy. Procedurally correct, not evasion. ConvergenceAligned. A has no unresolved findings; B has no disputes or missed issues. The PR is review-clean on this SHA. A human reviewer can merge. |
…round-trip (#2861) Migrate new, seed, notes, analyze, doctor, stats, upgrade, and destroy off the getArgs()/argsFromCollection() argv round-trip onto ArgSpec, consuming LuCLI's structured argCollection directly. Fixes the latent arg1-gate that silently dropped named-only flags (e.g. `wheels doctor --verbose`, `wheels seed --environment=x`). Adds structuredArgs()/argvToCollection() helpers and 38 server-free specs (CommandArgParsingSpec). getArgs() stays as the shim for the not-yet-migrated dispatchers / deploy / packages / migrate / test / console; shim removal follows. Refs #2861, #2862, #2855.
Summary
Introduces
cli.lucli.services.ArgSpec, a typed argument-spec builder that lets each Wheels CLI subcommand consume LuCLI's structured argument map directly instead of round-tripping it throughModule.cfc::argsFromCollection()and re-parsing the flattened argv with a hand-rolled token loop. The flatten step was the root cause of #2855 (it silently dropped everyfalsevalue, so--no-sqlite/--no-routes/--no-test-db/--no-open-browsernever survived the round trip) and was structurally lossy — it could not distinguish--no-Xfrom an explicit--X=false. The new builder declares positionals, flags, and options up front (.positional(name, required, default, type),.flag(name, default),.option(name, default, type)) and returns a typed result struct from.parse(arguments)— no flatten, no re-parse, no lossyfalseround trip.getArgs()/argsFromCollection()stay in place as a deprecated shim so the ~18 commands can be migrated one at a time in follow-ups.Fixes #2861.
Recommended path from research: #2861 (comment)
Related Issue
Closes #2861
Type of Change
Feature Completeness Checklist
Signed-off-by:trailer on the single commitcli/lucli/tests/specs/services/ArgSpecSpec.cfccovers builder chaining, positional binding (ordered + required-throw + optional-default), flag handling (default-when-absent, string"true", string"false"— the Wheels CLI ignores --no-sqlite argument #2855 regression surface — and literal booleanfalsefor cross-engine safety), option value pass-through with numeric coercion, and unknown-key isolationbot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-upbot-update-docs.ymlfollow-up[Unreleased]-> AddedTest Plan
cli/lucli/tests/specs/services/ArgSpecSpec.cfc— written first, failing in the absence ofArgSpec.cfc; passes against the implementation in this PR.bash tools/test-cli-local.sh(CLAUDE.md mapscli/lucli/**changes to this script, nottools/test-local.sh).bash tools/test-cli-local.shcould not be executed locally. Every spec assertion was traced manually against the implementation before committing, and the spec demonstrably fails in the absence ofcli/lucli/services/ArgSpec.cfc(thenew cli.lucli.services.ArgSpec()instantiation throws "Component not found" for everyit()block). CI's CLI test job is the gating runner for this PR.bot-tdd-gate.ymlwill confirm the diff contains both spec changes and implementation changes.Screenshots / Output
n/a — service component, no UI surface.