fix(cli): audit-tail polish — info version line, MCP helper leak, help/console text - #2888
Conversation
…p/console text Follow-up to the #2882-#2886 CLI-audit sweep; closes the verified low-risk tail. - wheels info: the framework-version line read a long-gone path (vendor/wheels/events/onapplicationstart/settings.cfm) and silently rendered nothing. Read the authoritative vendor/wheels/wheels.json by absolute path (no wheels mapping needed) with the same structural placeholder guard as wheels.BuildInfo, so a dev checkout reports 0.0.0-dev instead of leaking the raw @build.version@ token. - MCP tools/list leaked two internal $-helpers ($normalizeTestFilter, $resolveAppTestDataSource). Added both to mcpHiddenTools() (kept public so TestCommandSpec unit-tests them directly; LuCLI matches hidden names case-insensitively). - wheels --help: list the working 'create app' command (was absent from the banner) and drop the HACK default from the notes line (parser default stays TODO,FIXME,OPTIMIZE; --annotations customizes). - wheels reload: honor an explicit --password=<value> override (parity with console); auto-detect stays the default. - console /help: list the /datasource and /q aliases it already accepts. - wheels mcp instructions + the deprecated /wheels/mcp endpoint notice now point to the live MCP integration guide instead of a doc path (mcp-configuration-guide.md) that never existed. - Docs: wheels test flag table documents the real --directory alias; CLAUDE.md packages block lists the real 'registry info' verb. 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: This is a focused polish pass on the CLI audit tail — version-line fix, MCP helper leak, help text updates, and doc-path cleanup. The logic in every change is sound, but one correction introduces a new broken URL by routing users to a /v4-0-0-snapshot/ deep-link that Astro explicitly cannot redirect via catch-all, meaning wheels mcp and the /wheels/mcp deprecation notice now point to a 404 rather than a 404-under-a-different-name. Requesting changes for that; everything else is minor.
Correctness
Broken URL introduced in two places — v4-0-0-snapshot deep-link is a 404.
cli/lucli/Module.cfc line 1253:
out(" https://guides.wheels.dev/v4-0-0-snapshot/command-line-tools/mcp-integration");vendor/wheels/public/mcp/McpServer.cfc line 11:
"...see https://guides.wheels.dev/v4-0-0-snapshot/command-line-tools/mcp-integration for details."
web/sites/guides/astro.config.mjs lines 68–75 documents exactly why this 404s:
"Astro's static
redirectsmap doesn't support[...spread]sources … so a full catch-all for/v4-0-0-snapshot/*→/v4-0-0/*isn't possible at this layer. The high-traffic landing pages get explicit redirects here; deep links fall through to the site's 404 page."
mcp-integration is not among the explicitly listed redirects (lines 76–111 of that config — only installation, start-here/*, and the retired cli-reference paths are covered). The guide file exists at v4-0-0/command-line-tools/mcp-integration.mdx, not under a v4-0-0-snapshot tree.
Fix in both files: replace v4-0-0-snapshot with v4-0-0:
https://guides.wheels.dev/v4-0-0/command-line-tools/mcp-integration
This replaces one broken link (the path that never existed) with another that will 404 in production. The slug needs to be correct to be an improvement.
Tests
wheels reload --password has no behavioral test.
cli/lucli/tests/specs/commands/ReloadCommandSpec.cfc only source-scans for the onApplicationStart does NOT re-fire / wheels stop && wheels start strings. The new code path — parseConsoleArgs(structuredArgs(arguments)) plumbing the --password override — has no coverage. A spec asserting that when password is in the collection, detectReloadPassword() is skipped, would guard this. Low-risk nit since the console() function uses the identical pattern and its tests pass, but worth noting.
mcpHiddenTools() window-scan won't reach the new entries.
cli/lucli/tests/specs/commands/MainCommandSpec.cfc lines 74–77:
var body = mid(variables.source, startIdx, 800);
expect(body).toInclude("""main""");The four-line comment block added before "$normalizeTestFilter" (lines 170–174 in Module.cfc) pushes those entries past the 800-character cutoff — the existing entries plus comments accumulate to roughly 900 chars before line 175 is reached. The "main" assertion still passes (it's near the top), but there's no regression guard to keep $normalizeTestFilter and $resolveAppTestDataSource in mcpHiddenTools(). Extending the window (e.g. to 1200) or adding a second toInclude assertion would close the gap. Not a blocker, but the stated fix is unguarded.
Conventions
@build.version@ placeholder check in info() at line 1152:
if (left(fwVersion, 7) == "@build." && right(fwVersion, 1) == "@") {This is the right pattern — it matches BuildInfo.cfc line 57 verbatim, not the older exact-equality forms in PackageLoader.cfc/Plugins.cfc. No issue here; noting it for completeness.
Commits
Single commit fix(cli): audit-tail polish — info version line, MCP helper leak, help/console text — valid type, scope, subject ≤ 100 chars, sentence-case, Signed-off-by present. Clean.
Docs
web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/testing.mdx — --directory alias documented correctly; the alias is implemented at Module.cfc lines 551/561.
CLAUDE.md — wheels packages registry info is a real verb (Module.cfc line 2415). Addition is correct.
CHANGELOG.md — entry under [Unreleased] ### Fixed is present and accurate.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-founded. The blocking finding (broken URL introduced in two places) is verified against the actual source; the two test-coverage nits are real and correctly scoped as non-blockers. No false positives, no sycophancy, no missed issues of significance. Aligning with A's SycophancyNone detected. A requested changes on a concrete finding rather than approving with platitudes. False positivesNone detected.
Missed issuesNone of material weight. A did not explicitly verify that the console Verdict alignmentA's ConvergenceAligned. The joint recommendation is to fix both
The two test-coverage gaps (no behavioral test for |
- Replace broken `v4-0-0-snapshot/command-line-tools/mcp-integration` deep-link with the correct `v4-0-0/` slug in both the `wheels mcp` guidance output (cli/lucli/Module.cfc) and the deprecation notice on the legacy `/wheels/mcp` endpoint (vendor/wheels/public/mcp/McpServer.cfc). Astro's static redirect map cannot catch-all `v4-0-0-snapshot/*` → `v4-0-0/*` (see web/sites/guides/astro.config.mjs:68-75), so the previous URL would fall through to the site 404. - Extend the `mcpHiddenTools()` window-scan in MainCommandSpec.cfc from 800 to 1500 chars and assert on the new `$normalizeTestFilter` / `$resolveAppTestDataSource` entries so the regression guard actually covers them past the comment block. 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):
Skipped (intentionally):
Note: I could not re-run 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: The round-1 fixup commit (e61b11e) correctly addresses both consensus findings from the prior review: the broken v4-0-0-snapshot deep-link slug is replaced with v4-0-0 in both Module.cfc and McpServer.cfc (file verified at web/sites/guides/src/content/docs/v4-0-0/command-line-tools/mcp-integration.mdx), and the mcpHiddenTools() source-scan window is extended from 800 to 1500 characters with explicit assertions for both $normalizeTestFilter and $resolveAppTestDataSource. The 1500-char window is confirmed sufficient: both entries sit at Module.cfc lines 175-176, only 17 lines into the function that starts at line 159. The full PR is clean on correctness, conventions, cross-engine, docs, commits, and security. One minor test nit remains open.
Tests
ReloadCommandSpec still has no assertion for the --password override branch.
cli/lucli/tests/specs/commands/ReloadCommandSpec.cfc (unchanged by this PR) does not cover the new code path added at cli/lucli/Module.cfc lines 694-695:
var reloadOpts = parseConsoleArgs(structuredArgs(arguments));
var password = len(reloadOpts.password) ? reloadOpts.password : detectReloadPassword();
A spec asserting that when password is present in the argument collection detectReloadPassword() is bypassed would guard this branch. This was flagged in the round-0 review and is still open. Low-risk (the pattern is identical to console() which IS covered), but worth a follow-up if a ReloadCommandSpec suite is touched in a subsequent PR.
Commits
Both commits conform to commitlint.config.js: valid fix(cli) type, scope, subject under 100 chars, sentence-case, Signed-off-by trailers present on both. Clean.
Correctness
The notes help-text correction (removing HACK from the listed defaults) is accurate -- Module.cfc line 2604 confirms the ArgSpec default is "TODO,FIXME,OPTIMIZE". The create app banner addition is correct; public string function create() is at line 1016. The info() framework-version path correctly reads vendor/wheels/wheels.json with the structural placeholder guard (left(fwVersion, 7) == "@build.") matching wheels.BuildInfo line 57 exactly. The /ds, /datasource and /q console aliases documented in the /help output are real. All clean.
Wheels Bot — Reviewer B (round 1)A's review of the round-1 fixup commit is accurate, evidence-backed, and complete. Both consensus findings are correctly verified: the v4-0-0-snapshot slug fix is real (first commit introduced SycophancyNone detected. Every claim A made is accompanied by a specific file, line number, or diff reference. The approval is evidence-based. False positivesNone detected. Spot-checked each claim:
Missed issuesNone detected for the fixup commit scope (3 files: Module.cfc, MainCommandSpec.cfc, McpServer.cfc). The round-0 findings from the broader PR were addressed in prior rounds and are not re-litigated here. Verdict alignmentA's COMMENTED state (no blocking findings, one acknowledged follow-up nit) is consistent with the findings. The PR is clean after the fixup. ConvergenceAligned on approve. A correctly identified that the fixup resolved both blocking issues and that the ReloadCommandSpec gap is a low-risk follow-up item, not a merge blocker. Joint recommendation: the PR is review-clean for this SHA. |
Adds a source-scan assertion to ReloadCommandSpec (matching the existing hot-reload-contract test) verifying reload() parses --password via parseConsoleArgs and only auto-detects when no override is supplied. A behavioral test would need a running server (reload() calls $requireRunningServer first), so — like the sibling assertion — this guards the wiring at the source level. Closes the one open nit from the Reviewer A/B round-1 review. Builds on the bot's round-1 fixup (e61b11e). Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This third commit adds the ReloadCommandSpec source-scan assertion that was the sole open nit from round-1. The implementation is correct, the window calculation is sound, and all issues from prior rounds are resolved. Approving.
Correctness
Round-1 issues (broken URL, mcpHiddenTools window): both confirmed resolved in e61b11e. Module.cfc line 1250 and McpServer.cfc line 11 now carry https://guides.wheels.dev/v4-0-0/command-line-tools/mcp-integration. The mcpHiddenTools() source-scan in MainCommandSpec.cfc uses a 1500-char window with explicit assertions for "$normalizeTestFilter" and "$resolveAppTestDataSource".
New commit (f246899ab) — --password override wiring guard: The spec in ReloadCommandSpec.cfc (lines 27–42 after the patch) is correct on all fronts:
- The anchoring regex
(?m)^[ \t]*public\s+string\s+function\s+reload\s*\(correctly matches the live signaturepublic string function reload()atModule.cfc:681. - The span from that signature to the
detectReloadPassword()ternary at line 698 is approximately 901 characters, comfortably within the 1200-charmid()window (~299-char margin). - All three
expectassertions are exact string matches against the live source:"parseConsoleArgs(structuredArgs(arguments))"— line 697 ✓"detectReloadPassword()"— line 698 ✓reFindNoCase("len\(\s*reloadOpts\.password\s*\)\s*\?", body)matcheslen(reloadOpts.password) ?at line 698 ✓ (CFML double-quoted string passes backslashes literally to the regex engine — correct behavior).
Tests
The source-scan approach is the right tradeoff: reload() gates on $requireRunningServer() before it reaches the password logic, so a behavioral test would require a live server. The sibling assertion (onApplicationStart does NOT re-fire) uses the same pattern and precedent. All round-1 test gaps are now closed.
Commits
All three commits conform to commitlint.config.js:
| Commit | Type | Header length | Sign-off |
|---|---|---|---|
fix(cli): audit-tail polish … |
fix |
77 chars | ✓ |
fix(cli): address Reviewer A/B consensus findings (round 1) |
fix |
58 chars | ✓ |
test(cli): guard wheels reload --password override wiring |
test |
56 chars | ✓ |
Wheels Bot — Reviewer B (round 1)A's review is accurate and thorough. All claims check out against the diff and the live source. One minor style nit A missed — doesn't change the verdict. SycophancyNone detected. A cites specific line numbers and diff excerpts for every claim rather than stating general approval. False positivesNone detected. Verified each of A's assertions against the diff:
Independently confirmed: Missed issuesOne style issue: CLAUDE.md says "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." Three new multi-line comment blocks appear in this PR:
The WHY in each is non-obvious (which is when a comment is warranted), so the comments themselves are defensible — the violation is length only. Non-blocking. Verdict alignmentA's APPROVED verdict is consistent with the findings: the implementation is correct, the wiring is verified, cross-engine concerns are not applicable to CLI-only code, and all prior-round nits are closed. ConvergenceAligned. Both A and B reach approve. The only gap is the multi-line comment style nit, which does not change the recommendation. The PR is ready for a human to mark ready-for-review and merge. |
…il-polish Signed-off-by: Peter Amiri <peter@alurium.com> # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This head SHA (d34b27c72) is a housekeeping merge commit that brings origin/develop (commit 8f3283fdb, PR #2890 — the non-zero exit code fix) into the PR branch and resolves a single CHANGELOG.md conflict. The PR's substantive code changes were approved in the round-3 review against f246899ab; GitHub dismissed that approval automatically when the merge commit was pushed. This pass confirms the merge commit is clean and the prior approval holds.
Correctness
The merge commit changes exactly one file: CHANGELOG.md. The conflict resolution is correct:
- The
### Changedsection (from #2890) is preserved intact. - The PR's CLI-audit tail polish entry is placed at the top of
### Fixed, above the existing develop entries — correct chronological ordering. - No develop-side
### Fixedentries were dropped.
All the PR's key changes are confirmed present in the merged Module.cfc and McpServer.cfc:
| Change | Location | Status |
|---|---|---|
$normalizeTestFilter / $resolveAppTestDataSource in mcpHiddenTools() |
Module.cfc:175–176 | ✓ |
create app in --help banner |
Module.cfc:287 | ✓ |
0.0.0-dev placeholder guard for wheels info |
Module.cfc:1159 | ✓ |
--password override via parseConsoleArgs in reload() |
Module.cfc:699–700 | ✓ |
v4-0-0 MCP integration guide URL |
Module.cfc:1259 | ✓ |
v4-0-0 MCP integration guide URL |
McpServer.cfc:11 | ✓ |
1500-char mcpHiddenTools() window + $normalizeTestFilter/$resolveAppTestDataSource assertions |
MainCommandSpec.cfc:78–81 | ✓ |
--password wiring source-scan |
ReloadCommandSpec.cfc:24–37 | ✓ |
Commits
The merge commit carries a Signed-off-by: Peter Amiri trailer. The underlying PR commits (9a2cf88, e61b11e71, f246899ab) all conform to commitlint.config.js as verified in the round-3 review.
No new correctness, cross-engine, convention, test, docs, or security issues introduced by this update.
Wheels Bot — Reviewer B (round 1)A's review of the merge commit is accurate, correctly scoped, and consistent with the prior-round history. No issues to raise. SycophancyNone detected. A identifies this as a merge-commit pass, correctly limits its scope to the CHANGELOG.md conflict resolution, and backs every claim with a specific file/line reference. The approval is evidence-based. False positivesNone detected. A's characterisation of the HEAD commit is accurate: the commit message names CHANGELOG.md as the sole conflict, and the three prior commits carrying the substantive code changes all have prior-round Missed issuesNone detected. The one open non-blocking nit from the Verdict alignmentA's APPROVED verdict is consistent with the findings. Approving a merge commit that only resolves a documentation conflict, after the substantive changes were already approved in prior rounds, is appropriate. ConvergenceAligned on approve. A and B both reach the same recommendation: the PR is review-clean for this SHA. No changes needed. |
…ntegration guide URL (#3019) * fix(mcp): replace phantom mcp-configuration-guide.md path with live integration guide URL PR ##2888 standardized the deprecated /wheels/mcp HTTP transport's doc pointer on https://guides.wheels.dev/v4-0-0/command-line-tools/mcp-integration, but three runtime-visible strings still cited the phantom docs/command-line-tools/commands/mcp/mcp-configuration-guide.md path that has never existed in the repo: the one-time-per-JVM deprecation log message in vendor/wheels/public/views/mcp.cfm, the CLI-disabled-tool error returned to clients in vendor/wheels/public/mcp/McpServer.cfc, and the legacy CommandBox wheels mcp setup output in cli/src/commands/wheels/mcp/setup.cfc. Replace all three (plus the file-header comment in mcp.cfm) with the URL #2888 already standardized on, matching the serverInfo.deprecationNotice field that PR did fix. Refs #3016 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * docs(web/guides): replace deprecated "dependencies" key with "requires" in package.json example Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> --------- Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
1 participant
Summary
Follow-up to the #2882–#2886 CLI-audit sweep. A reconciliation pass over every audit finding against current
develop(43 confirmed shipped, ~22 open) surfaced a small low-risk tail; this PR clears the verified-safe code + doc items. (--hasOnethreading and typed-exit-code changes follow as separate PRs.)Fixes
wheels infoframework-version line was blank. It read the long-gonevendor/wheels/events/onapplicationstart/settings.cfmpath. Now reads the authoritativevendor/wheels/wheels.jsonmanifest by absolute path (nowheelsmapping needed), with the same structural placeholder guard aswheels.BuildInfo— a dev checkout reports0.0.0-devrather than leaking the raw@build.version@token. (The audit logged this as fixed in fix(cli): repair broken commands and correct command docs (CLI audit) #2882, but fix(cli): repair broken commands and correct command docs (CLI audit) #2882 only fixed the route-count half of the finding and left the version line broken.)tools/listleaked two internal$-helpers ($normalizeTestFilter,$resolveAppTestDataSource). Added both tomcpHiddenTools(). KeptpublicsoTestCommandSpec's direct unit tests still run; LuCLI matches hidden names case-insensitively (McpCommandlowercases both sides).wheels --helpnow lists the workingcreate appcommand (it was a live command + MCP tool but missing from the banner), and itsnotesline no longer advertises aHACKdefault the parser doesn't use (default staysTODO,FIXME,OPTIMIZE;--annotationscustomizes).wheels reloadhonors an explicit--password=<value>override (parity withwheels console); auto-detect from.env/config stays the default./helplists the/datasourceand/qaliases it already accepts.mcp-configuration-guide.md, which never existed) in bothwheels mcpinstructions and the deprecated/wheels/mcpendpoint's deprecation notice → now points to the live MCP integration guide.wheels testflag table documents the real--directoryalias for--filter; agentCLAUDE.mdpackages block lists the realwheels packages registry infoverb.Verification
Run against the worktree source via an isolated
LUCLI_HOME(bypassing the brew wrapper):wheels info→Wheels: v4.0.2on a stamped manifest;Wheels: v0.0.0-devon an@build.version@manifest (no token leak). Previously printed nothing.wheels mcp wheels --once tools/list→ 18 real commands, no$-helper entries (was leaking 2).wheels --help→ showscreate appand the correctednotesline.wheels version/notes/validatesmoke clean (Module.cfc compiles + loads).Spec-safety checked: the
HACKspecs test--customparsing (unchanged defaults);MainCommandSpec'smid(...,800/900)source-scans target content before these insertions; the$-helpers stayedpublicsoTestCommandSpec's 31 direct calls are unaffected.Notes
vendor/wheels/public/mcp/McpServer.cfcchange is a one-line deprecation-notice string (no logic, no spec guard) — it pulls the core matrix into CI, which will confirm it.wheels mcp(bare verb) is still intercepted by the LuCLI builtin on the shipped 4.0.2 runtime; themcp()string fix is forward-correct (reachable once a newer LuCLI ships), and the bare-verb branding is handled upstream in fix: resolve active binary name in completion and module usage strings cybersonic/LuCLI#70. TheMcpServer.cfcHTTP-endpoint notice is reachable today.GenerateCommandSpec"generate migration" describe — it's test-hygiene and its harness-readiness can't be cheaply proven from a worktree, so it's left rather than risk a speculative red.