fix(cli): add --release alias for deploy --version to dodge picocli (#2674)#2691
Conversation
…2674) picocli absorbs the bare --version flag at the LuCLI root before Module.cfc runs, so the documented Kamal-compatible form `wheels deploy --version=v1.2.3` fails with `Invalid value for option '--version': 'v1.2.3' is not a boolean`. Adds --release as a picocli-safe alias for the deploy version flag, extracts the deploy arg parser into a standalone DeployArgsParser.cfc so it can be unit-tested without the BaseModule mapping, and teaches the brew/scoop wrappers to rewrite --version[=val] -> --release[=val] when "deploy" is the first positional. End result: the documented `--version` form keeps working on a current-channel wrapper, and a hand-typed `--release` works on older wrappers too.
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly fixes a real picocli conflict (--version absorbed as a boolean root flag before Module.cfc runs) by adding --release as a parser-level alias, extracting DeployArgsParser.cfc for testability, and teaching the brew/scoop wrappers to rewrite transparently. The core approach is sound. Three nits worth addressing before merge: a confusing error message, a missing CHANGELOG entry, and a missing edge-case test that preserves a pre-existing silent-drop bug.
Correctness
Error message exposes internal flag name — DeployAppCli.cfc, line 112
message="This verb requires --release (or --version on a wrapper that rewrites it)");The user-facing deployment docs (first-deploy.mdx:158,181 and migrating-from-kamal.mdx:81,93) consistently document --version=<tag> as the flag to use. A user who forgets the version flag entirely would see this error message and be confused: the docs say --version, but the error leads with --release.
The parenthetical is also backwards. The mental model should be: --version is the public flag; --release is the internal alias users can fall back to on older wrappers. Consider reversing it:
message="This verb requires --version (pass --release directly on older wrappers that pre-date the rewrite)");Or, if brevity is preferred, just keep the documented flag name:
message="This verb requires --version (e.g. --version=v1.2.3)");Tests
Missing edge-case test: --release with no following value — DeployArgsParser.cfc, lines 42–44
} else if (a == "--release" && i < n) {
opts.version = arguments.args[i+1];
i++;
}When --release appears as the last token in the args array (i == n), the condition i < n is false, so the flag is silently dropped and opts.version is never set. This mirrors the original --version behavior (same bug, same line-pattern), so the refactor faithfully preserved it — but the new spec file is an opportunity to document (and optionally fix) the behavior. Even a test that asserts the silent-drop happens explicitly would prevent a future maintainer from accidentally changing it in a breaking way:
it("silently drops --release with no following value", () => {
var opts = parser.parse(["--release"]);
expect(structKeyExists(opts, "version")).toBeFalse();
});Docs
No CHANGELOG.md [Unreleased] entry
This is a user-observable bug fix — the documented wheels deploy --version=v1.2.3 form crashing with a boolean-flag error is a significant UX problem. The [Unreleased] section in CHANGELOG.md has entries for other recent fixes; this one should be there too.
Conventions
$deployArgsToOptions changed from private to public — Module.cfc, line 1959
public struct function $deployArgsToOptions(required array args) {The stated reason (enabling unit testing without a modules.BaseModule mapping) is valid, and in a CLI module context the private-in-mixin cross-engine concern from CLAUDE.md does not apply. This is a minor style note only: the $ prefix already signals "internal intent," but a caller outside Module.cfc could now invoke it directly. If testability is the goal and there are no callers outside the deploy path, keeping the method private and testing through DeployArgsParser.cfc directly (which this PR already does) would be the cleaner long-term shape. Not blocking, but worth a decision comment if left public.
Commits
Single commit fix(cli): add --release alias for deploy --version to dodge picocli — type, subject, and length all conform to commitlint.config.js. Body explains the why. No issues.
Security
No concerns. The arg parser processes a hardcoded list of flag names; no user-controlled input reaches SQL or HTML.
Wheels Bot — Reviewer B (round 1)A's review is well-grounded: three concrete findings backed by specific line references, no blanket approval, and no invented issues. One false positive to correct and one missed issue worth surfacing. SycophancyNone detected. A did not approve and did not file empty praise. The "The core approach is sound" opener is accurate given the fix is a clean extract-and-alias pattern. False positives
Missed issues
Verdict alignmentA's three substantive findings (error message wording, missing silent-drop test, missing CHANGELOG entry) are valid and actionable. The false-positive caveat above does not change the verdict — the error-message rewrite A recommends is still an improvement regardless of whether the framing is precisely right. ConvergencePartially aligned with A. A's findings are accurate and should be addressed. The missed issues above (unquoted batch accumulation and docs omission for |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's critique is largely fair. I'm conceding on three points and defending one. The overall verdict does not change — request-changes — because the surviving and newly-surfaced items are all concrete and actionable.
Conceded points
-
"Backwards" framing was imprecise. B is right that the error message in
DeployAppCli.cfc:112is not strictly in the wrong order for the error-path audience. A user on an old wrapper (without the--version→--releaserewrite) really should type--releasedirectly. My characterization of the parenthetical as "backwards" overstated it. The underlying recommendation — align the error text with the documented flag--versionso current-wrapper users get consistent guidance — still holds, but the "backwards" framing was wrong. -
Missing
--versionsilent-drop test. B is correct thatDeployArgsParser.cfclines 33–35 (--versionarm) have the identicali < nguard as lines 42–44 (--releasearm). I flagged only the--releasecase. For symmetry and future-safety both silent-drop behaviors should be documented in the spec together, not just one. -
Docs gap for
--release. B correctly notes thatmigrating-from-kamal.mdxandfirst-deploy.mdxcontinue to document only--version. A user on an older wrapper who needs to invoke--releasedirectly has no documentation reference. I flagged the CHANGELOG omission but missed this gap; B's addition is valid.
Defended points
-
Error message should still lead with
--version. Even granting that the current text is not "backwards," the majority audience hitting the MissingVersion exception is on a current-channel wrapper that performs the transparent rewrite — they forgot to supply the flag at all. For that user, the docs say--version=v1.2.3and the error message says--release, creating a mismatch. The old-wrapper minority can be served by a parenthetical, not the lead. Recommended wording remains:"This verb requires --version (or --release directly on older wrappers without the rewrite)"This is not a framing quibble — it is a UX question about which audience the primary error text should address. Current-wrapper users are the larger population.
-
CHANGELOG finding stands. B did not dispute this; noting it here for completeness. A user-observable bug fix of this severity belongs in
[Unreleased]. -
Batch wrapper unquoted accumulation is a real pre-existing bug. B surfaced this correctly and I missed it. The
%~1expansion in the scoop JSON templates strips outer quotes but does not re-quote the value before appending toWHEELS_DEPLOY_ARGS. Arguments with embedded spaces word-split on!WHEELS_DEPLOY_ARGS!expansion. Low-severity for this PR since version tags never contain spaces, but a real footgun worth a follow-up issue.
Updated verdict
Still request-changes: the error message UX point, both silent-drop tests (for --release and --version), the CHANGELOG entry, and the --release docs gap are all outstanding and concrete. The private→public style note remains non-blocking.
- Error message now leads with --version (the documented form) and mentions --release as the fallback on older wrappers - Add silent-drop tests for both --release and --version when no value follows — documents the pre-existing last-token guard - CHANGELOG.md [Unreleased] entry under Fixed - migrating-from-kamal.mdx: document the --version <-> --release wrapper rewrite and the older-wrapper fallback - Revert $deployArgsToOptions to private — the parser extraction means the public visibility is no longer load-bearing
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR fully resolves issue #2674 (picocli absorbing --version before Module.cfc runs) and addresses every finding from the previous Reviewer A/B cycle: the error message now leads with the documented --version flag, both silent-drop edge cases are under test, the CHANGELOG entry is present, migrating-from-kamal.mdx documents the older-wrapper fallback, and $deployArgsToOptions is back to private. The implementation is correct, cross-engine clean, and tests follow established conventions. Two minor nits remain — neither is blocking.
Correctness
All four previous findings confirmed resolved:
DeployAppCli.cfc:112— error message now leads with--version (e.g. --version=v1.2.3)and mentions--releaseas the fallback for older wrappers.DeployArgsParserSpec.cfc:61-69— silent-drop tests added for both--releaseand--versionlast-token cases.CHANGELOG.md—[Unreleased]Fixedentry present.migrating-from-kamal.mdx:104— wrapper-rewrite semantics documented.
No new correctness issues found.
Conventions
first-deploy.mdx still uses --version=<tag> without a --release callout (lines 158, 181)
The rollout and rollback docs (first-deploy.mdx:158,181) continue to document --version=<tag> as the flag to pass. For current-channel wrapper users this is correct — the wrapper rewrites transparently. The older-wrapper fallback is now documented in migrating-from-kamal.mdx. This is acceptable as-is because first-deploy.mdx targets new users on current wrappers, but a single parenthetical "(or --release=<tag> on older wrappers)" at line 158 would make the docs fully consistent with the migrating-from-kamal.mdx note. Not blocking.
Commits
Both commits lack Signed-off-by: trailers
c79281f4c fix(cli): address Reviewer A/B feedback on #2691
30df2d7f9 fix(cli): add --release alias for deploy --version to dodge picocli (#2674)
Neither commit carries a Signed-off-by: trailer. The DCO GitHub App is listed as a required status check in CONTRIBUTING.md § Developer Certificate of Origin (DCO) and in the PR template checklist, and the DCO App blocks merge when any commit in the PR is unsigned. Per CONTRIBUTING.md § Forgot to sign off, the fix is:
git rebase --signoff HEAD~2
git push --force-with-leaseOr for the two commits individually via interactive rebase adding -s to each. The Validate Commit Messages CI check passes (commit type and subject conform to commitlint), but the DCO check is a separate required gate.
Security
No concerns. The arg parser processes a hardcoded set of flag names; the only user-controlled token (--release=<value>) is stored as a plain struct value and never interpolated into SQL or HTML.
Wheels Bot — Reviewer B (round 1)A's review is solid on the core correctness pass: all four round-1 findings are confirmed resolved with precise line references, no empty praise, no invented issues. One verdict-alignment issue and one lingering missed issue remain, but neither changes the joint recommendation. SycophancyNone detected. A did not approve, did not file blank LGTM language. The positive TL;DR is accurate given the author addressed every finding from round 1. False positivesNone detected. Every claim A makes — confirmed resolutions at specific files and lines, the DCO trailer check, the Specifically: the DCO finding is confirmed. Both commits were examined via Missed issues
Verdict alignmentMostly consistent but weakened by the ConvergenceAligned on the joint recommendation: changes needed — DCO sign-off on both commits. The implementation is correct, the previous round's four findings are all addressed, the |
Addresses Reviewer A's round-2 nit on PR #2691 — add a parenthetical at first-deploy.mdx line 158 so users on older wrappers (without the --version → --release rewrite) have a docs reference for the fallback, keeping it consistent with the existing migrating-from-kamal.mdx note. Only the snapshot doc set (v4-0-1-snapshot) is updated, matching the prior round's pattern. 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) on
Skipped — requires human author action, not bot action:
The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new head commit (a7ba014b0) makes a single targeted documentation addition — a parenthetical in first-deploy.mdx step 3 noting the --release fallback for older wrappers and linking to the migration guide. This fully addresses the sole minor nit from the previous Reviewer A round. All substantive findings from the A/B cycle are confirmed resolved: error message, silent-drop tests, CHANGELOG, migrating-from-kamal.mdx docs, and $deployArgsToOptions back to private. One finding carries over unchanged from the previous review and still blocks merge: the DCO sign-off is missing on two of the three PR commits.
Commits
DCO Signed-off-by: trailer missing on commits 30df2d7f9 and c79281f4c
Both of Peter Amiri's commits are unsigned:
30df2d7f9 fix(cli): add --release alias for deploy --version to dodge picocli (#2674)
c79281f4c fix(cli): address Reviewer A/B feedback on #2691
The third commit (a7ba014b0, authored by claude[bot]) carries the required trailer, but it does not sign off the earlier two. Per CONTRIBUTING.md section "Developer Certificate of Origin (DCO)":
The DCO GitHub App runs as a required status check on every PR. It verifies that every commit in the PR carries a valid
Signed-off-by:trailer and blocks merge if any are missing.
Fix (from CONTRIBUTING.md section "Forgot to sign off?"):
git rebase --signoff HEAD~3
git push --force-with-leaseThis finding was already present in the previous Reviewer A round (SHA c79281f4c). The new head commit added a signed commit on top but did not rebase the two earlier commits.
Docs
Minor nit (non-blocking): rollback example at first-deploy.mdx:181 still uses --version= without a fallback callout
wheels deploy rollback --version=abc1234Line 158 now explains the --version/--release situation and links to the migration guide, so a reader following the tutorial in order has the context before hitting this code block. For current-channel wrapper users the example is correct as written. A parenthetical mirroring line 158 (or --release=abc1234 on older wrappers) would make the page fully self-contained without requiring a cross-reference jump, but this is not blocking.
Correctness, Conventions, Cross-engine, Tests, Security
No new issues. All prior round findings confirmed resolved:
DeployAppCli.cfc:112error message leads with--version(documented form) and mentions--releaseas the older-wrapper fallback.DeployArgsParserSpec.cfccovers both silent-drop cases (--releaseand--versionas last token).CHANGELOG.md[Unreleased]entry present underFixed.migrating-from-kamal.mdxdocuments the wrapper rewrite semantics and the older-wrapper fallback.$deployArgsToOptionsis back toprivateinModule.cfc.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new head commit is a merge from develop (489f99631) that pulls in two unrelated features (#2686 Dockerfile scaffolding, #2688 SSH config propagation). The merge is clean — the PR's own DeployArgsParser extraction and --release alias are untouched and still correct. All substantive findings from the A/B cycle are confirmed resolved: error message, silent-drop tests, CHANGELOG, migrating-from-kamal.mdx, and $deployArgsToOptions back to private. The one blocking issue from the previous round remains open: two of Peter's commits still lack Signed-off-by: trailers. The merge commit makes the fix slightly more involved.
Commits
DCO Signed-off-by: trailer missing on two PR commits — still blocking
Commits authored by Peter Amiri remain unsigned:
30df2d7f9 fix(cli): add --release alias for deploy --version to dodge picocli (#2674)
c79281f4c fix(cli): address Reviewer A/B feedback on #2691
The third PR commit (a7ba014b0, authored by claude[bot]) is signed. The new merge commit (489f99631) is a structural merge and is typically exempt by the DCO App.
The DCO App is a required status check; it blocks merge while any non-merge commit in the PR lacks the trailer. Since a merge commit (489f99631) now sits on top of the two unsigned commits, a plain git rebase --signoff HEAD~N won't target the right set cleanly. Interactive rebase is the right path:
git rebase -i origin/develop
# In the editor: mark 30df2d7f9 and c79281f4c as 'edit'
# For each, after the shell drops to the edit prompt:
git commit --amend --signoff --no-edit
git rebase --continue
git push --force-with-leaseOr, if the DCO App's merge-commit exemption applies to 489f99631:
git rebase --signoff origin/develop
git push --force-with-leaseEither approach signs the two unsigned commits and leaves the signed claude[bot] commit intact.
Correctness
All prior correctness findings confirmed resolved:
DeployAppCli.cfc:112error message leads with--version (e.g. --version=v1.2.3)and mentions--releaseas the older-wrapper fallback.DeployArgsParserSpec.cfc:61–69covers both silent-drop cases (--releaseand--versionas last token).Module.cfc::$deployArgsToOptionsisprivateagain; delegation toDeployArgsParseris clean.- The merge did not introduce conflicts in
Module.cfc—$deployBuildSshPool(from #2688) and$deployArgsToOptions(from this PR) occupy distinct lines and compose correctly.
Docs
Minor nit (non-blocking, carried from prior round): rollback example at first-deploy.mdx:183 lacks --release callout
wheels deploy rollback --version=abc1234Line 160 now explains the --version/--release situation and links to the migration guide, so a reader following the tutorial in order has context before reaching this block. The example is correct for current-channel wrapper users. A parenthetical (or --release=abc1234 on older wrappers) mirroring line 160 would make the rollback block self-contained without a cross-reference jump — but this was already flagged as non-blocking in the previous round and does not block merge.
Security
No concerns. The arg parser processes a hardcoded set of flag names; the only user-controlled token (--release=<value>) is stored as a plain struct value and never interpolated into SQL or HTML.
Wheels Bot — Reviewer B (round 1)A's review on SHA SycophancyNone detected. A maintained CHANGES_REQUESTED throughout; every positive statement is backed by specific line references. False positivesNone detected. Verified against the diff:
Missed issues
Verdict alignmentConsistent. A correctly issued CHANGES_REQUESTED for a merge-blocking DCO requirement. The implementation findings are all clean. ConvergenceAligned with A on the joint recommendation: changes needed — DCO sign-off on the two Peter Amiri commits. All code and documentation findings from the prior cycles are addressed. The batch-wrapper word-split footgun remains pre-existing and low-severity (B round 2 closed that item). The only remaining action is DCO, which requires the author to run |
… bat-jar pre-parse (#2766) * fix(cli): scoop wheels.cmd invokes java.exe directly to dodge cmd.exe bat-jar pre-parse The Scoop-installed wheels.cmd wrapper failed on every invocation on at least one Windows 11 build (10.0.26200.8457). cmd.exe pre-parses the entire body of a `call <bat>` target looking for labels and control flow before running it; the lucli-<ver>.bat artifact is a bat-jar concatenation (bat preamble + :JAR_BOUNDARY + raw JAR ZIP bytes, ~915 KB), and the pre-parser trips on bytes inside the ZIP tail and aborts with `The filename, directory name, or volume label syntax is incorrect.` before lucli ever executes. Dispatch lucli via `"%JAVA_HOME%\bin\java.exe" -client -jar "%~dp0lucli-<ver>.bat" %*` instead. java reads the JAR via stream and skips the bat preamble in front of the ZIP central directory, sidestepping cmd.exe's parser entirely. The same fix applies to the `:deploy_dispatch` branch added by PR #2691, which used the same `call` pattern with rewritten args. Add a JAVA_HOME resolver near the top of the dispatch path that locates the openjdk21 dependency declared via `depends: java/openjdk21` -- preferring `%SCOOP%\apps\openjdk21\current` (the canonical Scoop layout) and falling back to the sibling-app form `%~dp0..\..\openjdk21\current`. Fail fast with a `scoop install java/openjdk21` hint when neither is found. Also restores the :deploy_rewrite block to build-manifests.py so the Python source-of-truth matches the published JSON (the block was added direct-to-JSON in #2691 and never made it back to the generator); without this, a future `python3 build-manifests.py` would wipe both the deploy rewrite AND this fix. Closes #2765. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * docs(web/guides): add scoop bucket add java step; fix JAVA_HOME prose for Windows install The Scoop packages declare `depends: java/openjdk21` — Scoop's `depends:` does not auto-add the dependency bucket, so the `java` bucket must be added explicitly before `scoop install wheels`. The `wheels.cmd` wrapper now resolves `JAVA_HOME` from that dependency automatically (#2765/#2766). Corrects the "no separate java bucket, no JAVA_HOME setup" claim that was wrong since the openjdk21 dependency was introduced. Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> --------- Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
1 participant
Summary
Resolves #2674.
wheels deploy --version=v1.2.3(the form documented in the Kamal migration guide) fails withInvalid value for option '--version': 'v1.2.3' is not a booleanbecause picocli treats--versionas aversionHelp = trueroot flag and intercepts it during arg parsing — beforeModule.cfcever sees the subcommand args.--releaseas a picocli-safe alias for the deploy version flag. The parser routes both forms toopts.version, so internal call sites and integration tests are untouched.cli/lucli/services/deploy/cli/DeployArgsParser.cfc. The previous implementation lived as a private method onModule.cfc, which extendsmodules.BaseModule— a mapping that only exists inside the LuCLI runtime, so the harness couldn't unit-test it.Module.cfc::$deployArgsToOptionsnow delegates in one line.wheels-be.rb) and scoop (wheels.json,wheels-be.json) wrappers to rewrite--version[=val]→--release[=val]whendeployis the first positional. The documented--versionform keeps working transparently on a current-channel wrapper; users on an older wrapper can type--releasedirectly.The stable homebrew formula lives in
wheels-dev/homebrew-wheelsand gets the same wrapper change in a follow-up PR.Closes #2674.
Test plan
bash tools/test-cli-local.sh—DeployArgsParserSpecpasses 6/0/0; alldeploy/**specs green.DeployAppCliSpecstill green (the parser refactor is behavior-preserving — Module.cfc delegates).deploy-ci.yml) green on PR.cli/lucli/Module.cfc).