Repository navigation
fix(cli): CLI audit follow-ups (test exit codes, deploy flags, release channel, guide fixes) - #2883
Conversation
Further CLI audit follow-ups (all reproduced + verified in an isolated harness): - wheels test now EXITS NON-ZERO when specs fail/error (was always 0, silently green-lighting broken builds — testing.mdx documents a non-zero exit). The throw fires after output is flushed and only when totalFail+totalError > 0; a fully-passing run still exits 0. - --reporter=tap no longer crashes on a failing spec: failOrigin is an array of stack-frame structs, not a string; $tapOriginString() coerces it (extracting Raw_Trace) before the YAML emitter, so TAP output is well-formed. - deploy: accept --config as an alias for --configPath (guides document --config); parse --role / --container / --follow so 'deploy app <verb>' filters by role (DeployAppCli already read these opts but the parser never populated them — a bare 'app boot --role=web' previously booted ALL roles). - ReleaseChannel: assemble the @build.version@ dev-sentinel at runtime so the release build's token replacer can't clobber the string literal — a packaged stable build was matching its own version and misreporting as (development). Signed-off-by: Peter Amiri <peter@alurium.com>
Sweep of the v4-0-0 CLI guides (audit follow-up), correcting documented commands/flags/verbs that error or behave differently: - deploy: --version=<v> → --release=<v> (--version is absorbed by picocli's root flag); note --configPath as an accepted alias of --config; rollback takes a positional <version>; nested 'deploy server/secrets <verb>' are picocli-shadowed → document the flat aliases (bootstrap, exec, *-secrets). - testing/CI: 'wheels browser install' → 'wheels browser setup' (install hits LuCLI's extension installer); 'wheels test run' → 'wheels test' / positional filter; note non-zero exit on failure; chromium-only engine; document 'wheels browser test'. - app-inspection: wheels info writes to STDOUT, not stderr. - dev-server: 'wheels start' runs in the BACKGROUND (not foreground); document -f/--force. - database: list all 8 migrate subcommands + document doctor/forget/pretend/rename-system-tables. - creating-a-project: reload password is random; no app/helpers/ directory. - code-generation: belongsTo generator emits the positional form. - console: /env returns a struct. - upgrade: same-major still runs the advisory scan. - packages: a newly-installed package needs a cold restart (wheels stop && start), not reload; fix broken registry/ links; 'add' not 'install' wording. - installation: 'wheels help' falls through to LuCLI's generic help, not the Wheels banner. Generated by a fan-out doc-sweep workflow; every edit reviewed against verified CLI behavior and for MDX/JSX safety. Signed-off-by: Peter Amiri <peter@alurium.com>
![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 — PR #2883 correctly fixes five CLI bugs (test exit codes, TAP reporter crash, --config/--role/--container/--follow deploy flags, and the build-token sentinel clobbering ReleaseChannel) and sweeps 23 doc files to match verified CLI behavior. All production code changes are sound. Two test gaps are worth addressing before merge: the DeployArgsParserSpec has no coverage for the four newly-parsed flags, and ReleaseChannelSpec line 41 carries the same build-token replacement vulnerability that was just fixed in the production code. Overall verdict: comment — no blocking correctness, cross-engine, or security issues, but the test gaps should be resolved.
Correctness
The five code fixes are structurally correct:
-
testsFailedflag (Module.cfc:4367): setting the flag outside the switch but inside theisJSONblock is the right placement. Thethrow()at 4428 is after the try/catch, so it propagates to the LuCLI runtime without being swallowed. Thereportervariable (notformat) drives the display-format switch, so JSON is always returned from the server andtestsFailedfires correctly for all three reporter modes (simple,json,tap). -
$tapOriginString(Module.cfc:4535): the fallback chain (simple value → array[1] as simple → array[1].Raw_Trace → array[1].template:line →"") covers the TestBox stack-frame struct shape correctly. The?: ""on the call-site coerces missing keys but leaves arrays intact (truthy in CFML), so array origins reach the helper as intended. -
devTokenassembly (ReleaseChannel.cfc:102): correct approach — assembling"@" & "build.version" & "@"at runtime prevents the build's token replacer from clobbering the comparison. Production misclassification is fixed. -
DeployArgsParserflag offsets:left(a, 9) == "--config="→mid(a, 10, ...)(9 chars prefix),left(a, 7) == "--role="→mid(a, 8, ...)(7 chars),left(a, 12) == "--container="→mid(a, 13, ...)(12 chars). All correct. Space-separated forms incrementiinside the handler + once more at loop bottom = total skip of 2. Correct.--followis a boolean flag with no extrai++. Correct. -
opts.role/opts.follow/opts.containerare consumed inDeployAppCli.cfcat lines 70–71 and 116–120 ($forEachHostrole filter). Verified the wiring is complete.
Tests
ReleaseChannelSpec.cfc line 41 — same build-token vulnerability as the production bug just fixed:
// ReleaseChannelSpec.cfc:41
expect(rc.classify("@build.version@")).toBe("development");In a stable build the token replacer turns "@build.version@" → "4.0.2", so this assertion becomes expect(rc.classify("4.0.2")).toBe("development") which fails with "stable". The test passes in dev/CI (source-checkout, no replacement) but would produce a misleading failure in any packaged stable release. Apply the same runtime-assembly fix:
// fix:
var sentinel = "@" & "build.version" & "@";
expect(rc.classify(sentinel)).toBe("development");DeployArgsParserSpec.cfc — four new code paths in DeployArgsParser.cfc (lines 120–146) have no test coverage:
// DeployArgsParser.cfc — new branches, none covered
} else if (left(a, 9) == "--config=") { opts.configPath = mid(a, 10, 99999); }
} else if (a == "--config" && i < n) { opts.configPath = arguments.args[i+1]; i++; }
} else if (left(a, 7) == "--role=") { opts.role = mid(a, 8, 99999); }
} else if (a == "--role" && i < n) { opts.role = arguments.args[i+1]; i++; }
} else if (left(a, 12) == "--container=") { opts.container = mid(a, 13, 99999); }
} else if (a == "--container" && i < n) { opts.container = arguments.args[i+1]; i++; }
} else if (a == "--follow") { opts.follow = true; }The existing spec (DeployArgsParserSpec.cfc) already tests both the --release=value and "--release value" forms with the same parser. Adding four it(...) blocks for --config, --role, --container, and --follow would follow the same pattern directly and would close this coverage gap:
it("parses --config=<path> into opts.configPath", () => {
var opts = parser.parse(["--config=/my/deploy.yml"]);
expect(opts.configPath).toBe("/my/deploy.yml");
});
it("parses '--config <path>' (space-separated) into opts.configPath", () => {
var opts = parser.parse(["--config", "/my/deploy.yml"]);
expect(opts.configPath).toBe("/my/deploy.yml");
});
it("--config and --configPath are equivalent", () => {
var a = parser.parse(["--config=/x.yml"]);
var b = parser.parse(["--configPath=/x.yml"]);
expect(a.configPath).toBe(b.configPath);
});
it("parses --role=web into opts.role", () => {
var opts = parser.parse(["--role=web"]);
expect(opts.role).toBe("web");
});
it("parses --follow into opts.follow = true", () => {
var opts = parser.parse(["--follow"]);
expect(opts.follow).toBeTrue();
});$tapOriginString unit coverage — the helper is private (correctly, to stay off the MCP surface), so it can't be reached from an external spec. The bug scenario (array origin crashes tapEscapeYaml) is exercised only through the xdescribe'd integration path that requires a live server. Consider promoting $tapOriginString to public access so the TAP branch behaviour can be captured as a unit test alongside $normalizeTestFilter and $resolveAppTestDataSource in TestCommandSpec.cfc. This isn't blocking (the integration path verified it), but the branchy nature of the helper makes it a good candidate.
Docs
The 23-file guide sweep is well-targeted and the individual changes are accurate:
wheels browser install→wheels browser setupis consistent acrosstesting.mdx,browser-tests.mdx,ci-integration.mdx,running-tests-locally.mdx,07-testing-deploying.mdx. No stragglers visible in the changed files.wheels startbackground-process framing is consistent acrossquick-start.mdxanddev-server.mdx. The<Aside>flip from "caution / foreground" to "note / background" is correct.--version=<v>→--release=<v>indeploy.mdx,redeploy.mdx,rollback.mdx,setup.mdx,index.mdx. Consistent.wheels deploy rollback <version>(positional, not--version=) inrollback.mdxmatches the parser.registry/refreshandregistry/infolink paths inpackages/index.mdx(line change from./registry-refreshto./registry/refresh): correct fix — the old relative paths would 404.belongsTo("author")positional form incode-generation.mdxis consistent with CLAUDE.md anti-pattern #1 (positional-only when no options).wheels stopreplaceslucli server stopinrunning-tests-locally.mdx. Correct rebrand.
One minor observation: the new LinkCard descriptions in index.mdx for deploy server and deploy secrets are now sentence-length strings inside a JSX attribute:
description="Host-level operations. Nested verbs are picocli-shadowed — use the flat aliases: wheels deploy bootstrap and wheels deploy exec."The em-dash and ASCII-safe characters are fine in JSX. Length is not a JSX error but may truncate or overflow in the rendered card grid depending on the Starlight theme. Worth a visual check on the preview deploy.
Commits
fix(cli): test exit codes, TAP reporter, deploy flags, release channel (70 chars ✓)
docs(web): correct CLI guides to match verified command behavior (63 chars ✓)
Both headers are within 100 chars, types are from the allowlist, scopes are valid. Subjects are sentence-case. No DCO sign-off concern visible (bot-authored branch follows the standard trailer path).
PR title fix(cli): CLI audit follow-ups (test exit codes, deploy flags, release channel, guide fixes) = 90 chars, within limit. Note that the squash-merge will land only the fix(cli) title — the docs(web) commit message will be lost unless the squash body is edited to note the guide sweep.
Security
No SQL interpolation, no user input reflected into HTML, no secrets in fixtures. No concerns.
Two more CLI audit follow-ups (verified in the isolated harness): - generate test now refuses to overwrite an existing spec unless --force, and threads --force through the command (mirrors generateHelper). Previously it silently clobbered the spec while still printing 'create'. - wheels notes scans only the COMMENT portion of each line and anchors the token with \b, so annotations inside string literals (x = "TODO: ...") and identifier suffixes (methodTODO) are no longer reported as real notes. Known limitation: multi-line /* ... */ blocks whose opener is on an earlier line aren't tracked. Signed-off-by: Peter Amiri <peter@alurium.com>
|
Added two more verified follow-ups to this PR:
Both reproduced + verified in the isolated harness. The prior commits were fully green (CLI suite, Deploy guides docs build, Reviewer A, visual regression). |
There was a problem hiding this comment.
Wheels Bot --- Reviewer A
TL;DR - A new commit (20d8355) has been pushed on top of the work already reviewed at af8dba3. It adds a --force guard to wheels generate test and a comment-context pre-filter to the Notes/Stats scanner. Both code changes are structurally correct. Three test gaps carried forward from the prior review remain open, and the new commit adds two more. No blocking correctness, cross-engine, or security issues; verdict: comment.
What Is New Since the Previous Review
The prior review (at af8dba3) already covered the test-exit-code fix, TAP failOrigin crash, ReleaseChannel build-token sentinel, and DeployArgsParser flag offsets. This review picks up the single new commit: fix(cli): generate test --force guard + notes comment-context scanner.
Correctness
generateTest --force guard (CodeGen.cfc:244-246): correct. Module.cfc handles success:false at line 2981, so the guard wires end-to-end without issues.
Stats.cfc comment-context pre-filter (Stats.cfc:242-249): sound. (1) Walks three markers and keeps the leftmost; (2) falls back to doc-comment continuation lines; (3) the continue statement exits the outer lineNum loop correctly; (4) the word-boundary assertion prepended to the pattern prevents matching annotation tokens that are suffixes of identifiers (e.g. methodTODO). Acknowledged limitation: multi-line block comments whose opener is on an earlier line are not tracked. Acceptable for a stat-gathering tool.
Tests
Four gaps - two carried forward, two new.
Carried forward:
ReleaseChannelSpec.cfc:41 - still has the literal @build.version@ which stable-build token replacement rewrites to the version string, causing the classification assertion to fail on every packaged release. The production fix (runtime sentinel assembly) was applied to ReleaseChannel.cfc:163 but not the spec. Fix: assemble the sentinel at runtime the same way - var sentinel = '@' & 'build.version' & '@'; expect(rc.classify(sentinel)).toBe('development');
DeployArgsParserSpec.cfc - the spec exists at cli/lucli/tests/specs/deploy/DeployArgsParserSpec.cfc but still has no cases for --config, --role, --container, or --follow (added in DeployArgsParser.cfc:220-245).
New from this commit:
CodeGen.cfc::generateTest - CodeGenSpec.cfc has no describe(generateTest) block. The --force guard at CodeGen.cfc:245 is the first meaningful branch in this path, with no coverage. Minimum needed: (a) existing file without --force returns {success:false}; (b) --force overwrites without error.
StatsSpec.cfc - the existing spec (lines 92-113) only asserts annotations ARE found in comments. It does not verify the new filter suppresses false positives. Cases worth adding: a TODO inside a string literal should NOT be reported; a TODO as an identifier suffix (trackTODO) should NOT be reported.
Commits
fix(cli): generate test --force guard + notes comment-context scanner - 66 chars, type fix, scope cli, sentence-case. Conformant. Minor observation: bundles two independent features in one commit, making atomic revert harder. Splitting is optional.
Security
No SQL interpolation, no user input reflected to HTML, no secrets. The testDir and fileName values in the error string at CodeGen.cfc:246 are internally generated. No concerns.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-scoped. All four test-gap findings check out against the actual files; no false positives; no sycophancy. Verdict of "comment" is consistent with the findings. Aligned on changes needed. SycophancyNone detected. A gave a "comment" verdict and left four explicit test gaps open rather than approving. False positivesNone detected. Every factual claim I spot-checked holds:
Missed issuesNone of substance. A explicitly scoped this pass to the single new commit (the Verdict alignmentA's "comment" verdict is consistent with the findings: four concrete, actionable test gaps documented, no correctness or security problems, no approval issued with outstanding issues open. ConvergenceAligned on changes needed. The four test gaps A identified should be addressed before merge: (1) assemble the |
Addresses the wheels-bot review's test-gap findings on this PR: - DeployArgsParserSpec: cover the new --config alias and --role/--container/--follow app-filter flags (#2883 H9). - CodeGenSpec: add generateTest() block — creates a spec, refuses to overwrite without --force, overwrites with force=true (H6/M4). - StatsSpec: assert getNotes() ignores annotations in string literals and identifier suffixes — only comments count (M11). - ReleaseChannelSpec: assemble the @build.version@ dev-sentinel at runtime so the build's token replacer can't clobber the literal (mirrors the production fix; H10). Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Tests: the local CLI runner ( 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 — Commit 5998e4fcd directly addresses all four test gaps called out in the two prior reviews: ReleaseChannelSpec build-token fix, DeployArgsParserSpec new-flag coverage, CodeGenSpec generateTest block, and StatsSpec false-positive suppression. No blocking correctness, cross-engine, or security issues. Two minor nits remain — space-separated forms for --role/--container are still untested, and the ReleaseChannelSpec comment text contains a literal @build.version@ that a naive build replacer would clobber (harmless in practice, but cosmetically inconsistent with the production comment). Verdict: comment.
Correctness
All four addressed gaps are resolved correctly:
ReleaseChannelSpec.cfc:44 — the runtime-assembly fix ("@" & "build.version" & "@") mirrors the production ReleaseChannel.cfc:50 fix exactly. The expect(...) assertion is no longer at risk of replacement. One cosmetic note: the comment text on line 42 (the build's @build.version@) still contains a literal token that a naive whole-file replacer would clobber into a real version string. Since it is a // comment, this does not affect test behaviour, but the production file's equivalent comment has the same property. No action required.
DeployArgsParserSpec.cfc:73–98 — five new cases cover --config=, --config (space), --role=, --container=, and --follow. The =-form mid-offset math matches the production parser:
--config=prefix is 9 chars →mid(a, 10, 99999)✓--role=prefix is 7 chars →mid(a, 8, 99999)✓--container=prefix is 12 chars →mid(a, 13, 99999)✓--follow→opts.follow = true✓
One remaining gap: the space-separated forms of --role and --container are untested. The production arms at DeployArgsParser.cfc:113–114 and 118–119 handle ["--role", "web"] / ["--container", "app-web-v1"] respectively — the same arms that the analogous --config case already has a space-form test for. Not blocking, but worth adding for parity:
it("parses '--role value' (space-separated) into opts.role", () => {
var opts = parser.parse(["--role", "web"]);
expect(opts.role).toBe("web");
});
it("parses '--container value' (space-separated) into opts.container", () => {
var opts = parser.parse(["--container", "app-web-v1"]);
expect(opts.container).toBe("app-web-v1");
});CodeGenSpec.cfc:28–53 — three cases cover create, no-overwrite-without-force, and overwrite-with-force. The fixture pattern (create with force first → write SENTINEL → re-test) is correct and matches the generateHelper precedent. The expect(find("SENTINEL", fileRead(path))).toBe(0) assertion correctly uses find() returning 0 for no-match. The generateTest fallback path (inline BDD template, CodeGen.cfc:288) sets result = {success: true, ...}, so expect(result.success).toBeTrue() holds even when no template file is present in the harness. ✓
controller and default type paths in generateTest() remain uncovered, but the force-guard logic is type-agnostic, so the model path exercises the relevant branch.
StatsSpec.cfc:140–157 — the false-positive suppression test is sound. getNotes(annotations = "QUIRK") uses an uncommon sentinel not present in any other fixture in the temp project, so the strict toBe(1) count is safe. The text.toInclude("real") assertion confirms the one detected entry is from the genuine // QUIRK: comment, not from the string-literal or identifier-suffix lines. The temp project's scan dirs (app/, config/, tests/) do not include the spec files themselves, so the QUIRK occurrences inside the spec source cannot self-report.
Commits
test(cli): regression coverage for CLI audit follow-up fixes (60 chars ✓)
Type test is in the allowlist, scope cli is valid, subject is sentence-case, header is under 100 chars. DCO Signed-off-by: Peter Amiri <peter@alurium.com> is present and matches the commit author. Conformant. ✓
Security
No SQL interpolation, no user input reflected to HTML, no secrets. The fileWrite / fileRead calls in the test fixtures operate on paths derived from tempRoot (a scaffolded temp directory, not a user-supplied value). No concerns.
Wheels Bot — Reviewer B (round 1)A's review is well-reasoned and structurally sound. All four findings from prior rounds are correctly verified as resolved. The two remaining nits (missing space-form tests for SycophancyNone detected. A used False positivesNone detected. I re-checked A's claims against the diff:
One edge case A did not mention: Missed issuesNone that are blocking. One structural note: A's review is scoped to commit Verdict alignmentA's ConvergenceAligned. A's findings are accurate, I have no unresolved false-positive disputes, and no missed blocking issues. A's two nits are genuinely optional suggestions, not consensus-required changes. The PR is review-clean for this SHA. |
Release-cut reconciliation ahead of the 4.0.3 GA: - Rename [Unreleased] -> [4.0.3] with the 2026-06-09 release date and a summary blurb, matching the 4.0.2 promotion format (#2824). - Consolidate the duplicated ### Changed / ### Fixed sections that accumulated from incremental entry additions into one Added/Changed/Fixed trio (same order as the 4.0.2 block). - Drop the byte-identical duplicate #2844 (protectedControllerMethods) entry. - Add the missing consolidated entry for the 24-command CLI audit sweep (#2882, #2883, #2884, #2885). - Fix the separator below the promoted block to exactly '---' (3 dashes) so release.yml's release-notes awk extraction terminates correctly (the #2606/#2768 footgun its Validate Release Checklist step guards against). Cutting the release branch from develop AFTER this lands avoids the 4.0.2 back-port dance (#2824) where the rename lived only on the release branch. Signed-off-by: Peter Amiri <peter@alurium.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What & why
Follow-up to #2882 — the next tier of CLI-audit fixes (the deferred Phase 3 items you asked me to tackle), plus a user-facing guide sweep. Every code fix was reproduced and re-verified against the CLI in an isolated harness; every doc edit was reviewed against verified behavior and for MDX/JSX safety.
Code fixes (
cli/lucli/**)wheels testexit code0, even with failing specs (CI green-but-failing)--reporter=tapfailOriginis an array → "Cannot cast Array to string")$tapOriginString()coerces it; emits well-formed TAP (verified)deploy --config--configPathparsed; guides document--config--configaccepted as an alias (verified)deploy app --role/--container/--followapp boot --role=webbooted all roles--role=web→ web only)@build.version@token-replacer clobbered the dev-sentinel string literal → stable builds reported(development)Doc sweep (
web/sites/guides/v4-0-0/**, 23 files)Corrected guides that documented commands/flags/verbs that error or behave differently:
--version=<v>→--release=<v>(picocli root absorbs--version);--configPathalias noted;rollback <version>positional; nesteddeploy server/secrets <verb>→ flat aliases (bootstrap,exec,*-secrets).wheels browser install→wheels browser setup;wheels test run→wheels test; non-zero-exit-on-failure noted; chromium-only engine; documentedwheels browser test.wheels info→ STDOUT (not stderr);wheels startruns in the background +-f/--force; all 8migratesubcommands documented (incl. doctor/forget/pretend/rename-system-tables); reload-password is random; noapp/helpers/dir;belongsTogenerator emits positional form;/envreturns a struct; upgrade same-major still runs the advisory scan.wheels stop && wheels start), notreload; fixed brokenregistry/links;add(notinstall) wording.wheels helpfalls through to LuCLI's generic help, not the Wheels banner.Verification
$?, TAP output,--roledry-run filtering,--configload).LinkCarddescription that had a JSX-invalid\"escape; all other angle-brackets are inside code fences / inline-code / valid<Aside>components.tools/test-cli-local.shsuite can't run from a.claude/worktreesworktree (framework demo app won't stay up there); CI is the backstop (CLI suite + deploy verb-smoke + docs build).Still deferred (upstream / out of this repo)
The remaining systemic items live in the LuCLI runtime (
cybersonic/LuCLI) + the per-launcher wrapper, not incli/lucli/, so they need upstream + Homebrew-formula PRs:--help/-h/barehelpall print the global/LuCLI banner (preprocessModuleHelp interception).--verbose/-vswallowed by the runtime (breaksdoctor/stats/testverbose).--versionpicocli root shadowing (the launcher should rewrite--version→--releasefor deploy);wheels help/mcp/error paths leak theluclibrand.(Lower-priority in-repo nits —
generate enumnot emitted, silent view-gen failures,generate testoverwrite guard,notescomment-context scanner — remain as smaller follow-ups.)