fix(cli): wheels reload exits non-zero and reports the real verdict instead of false success - #3139
Conversation
…nstead of false success reload() treated any completed HTTP exchange as success — it never read the status code, so a 500 from the reload endpoint (the #3053 Adobe regression) or a wrong-password page render both printed 'Application reloaded successfully.' while nothing reloaded. The framework's reload gate restarts the app then location()-redirects, so a successful reload is always a 302. The CLI now requests with redirects OFF and judges the raw status via $evaluateReloadResponse(): 3xx = success (output unchanged), 2xx = not triggered (wrong password hint), 4xx/5xx = endpoint error — failures print red then throw Wheels.ReloadFailed per the #2941 exit-code convention. The console's /reload applies the same verdict, printing red instead of throwing. makeHttpRequest() now delegates to a status-carrying variant (makeHttpRequestWithStatus) so the other call sites keep their exact body-only behavior. Specs: unit coverage of the verdict helper plus integration tests that drive the real reload() against a raw-socket fixed-status HTTP stub (StubHttpServer — com.sun.net.httpserver is unreachable from Lucee's OSGi classloader) on an ephemeral port: 500 -> throws, 200 -> throws, 302 -> succeeds quietly. Fixes #3059 Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This PR makes wheels reload read the HTTP status of the ?reload=true request and report honestly — 3xx (the framework's restart redirect) is success, 2xx/404 means the reload gate fell through (wrong password), 4xx/5xx is an endpoint error, and failures print red then throw Wheels.ReloadFailed for a non-zero exit. The core change is correct and well-evidenced; I verified the 302 contract, the MCP-surface claim, and the error-stream handling against the actual code. Verdict: comment — no blocking findings, one user-facing docs page now describes superseded behavior and should be updated (fine as a fast follow-up).
Verified (claims checked against the code, not just the PR body)
- The 302 contract is real.
public/Application.cfc::$handleRestartAppRequest(lines 473–474) doesapplicationStop()thenlocation(url=..., addToken=false)— a successful reload always answers with a redirect. The wrong-password fall-through to normal page serving is visible in the gate atpublic/Application.cfc:273-296. Keeping redirects off in the CLI (makeHttpRequestWithStatus(reloadUrl, false),cli/lucli/Module.cfc:847) is the right call — following the redirect would collapse a real reload into the same 200 a wrong password produces. $evaluateReloadResponsewon't leak as an MCP tool. The PR's claim about the "structural $-prefix sweep" checks out:mcpHiddenTools()(cli/lucli/Module.cfc:199-217) reflects overgetMetaData(this).functionsand hides every public$-prefixed function. Thepublic-for-specs access is the documentedcli/CLAUDE.mdcarve-out ("must bepubliconly when they need to be reachable as mixins/specs").- 4xx/5xx responses don't throw out of the HTTP helper.
makeHttpRequestWithStatusalready routes error statuses throughconn.getErrorStream()with a null guard (cli/lucli/Module.cfc:7003-7010), so a 500 reaches the verdict helper as a status code rather than detouring through the connection-error catch. The other ~12makeHttpRequest()call sites keep their exact prior behavior (body-only, redirects followed —setInstanceFollowRedirects(true)matches theHttpURLConnectiondefault). - The new throw doesn't break internal flows. No internal caller of
reload()exists (only CLI/MCP dispatch and the spec), and print-then-throw for non-zero exit has prior art inWheels.TestsFailed(cli/lucli/Module.cfc:5341) andWheels.ServerNotRunning(cli/lucli/Module.cfc:6731). - Tests are the right shape. Unit coverage of all four verdict classes plus integration tests that drive the real
reload()against the raw-socket stub per the issue's acceptance criterion, with red-first evidence in the PR body.ReloadCommandSpecextendswheels.wheelstest.system.BaseSpec(the base used by all 23 specs incli/lucli/tests/specs/commands/),##3059in describe strings is correctly escaped, and the stub's lifecycle is guarded (stop()infinally,threadJoinwith timeout, per-connectionsetSoTimeoutso a silent port probe can't wedge the accept loop). - Commit & changelog. Header is
fix(cli): …at 92 chars with a why-focused body and aSigned-off-by:matching the author. Changelog fragmentchangelog.d/3059-cli-reload-false-success.fixed.mdfollows the fragment system (no directCHANGELOG.mdedit).
Docs
web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/dev-server.mdx:115now describes superseded behavior. It reads: "On failure — most often a password mismatch — it printsFailed to reload: <message>and hints at settingWHEELS_RELOAD_PASSWORD…". After this PR, a password mismatch prints the new "Reload was not triggered: the server served the page normally (HTTP 200)…" message and the command exits non-zero;Failed to reload: <message>is now only the connection-error path. Suggest updating that paragraph to describe the three outcomes (302 success / 2xx-404 not-triggered with password hint / 4xx-5xx endpoint error) and the non-zero exit, sincewheels reload && …gating is exactly what users will want to know about. Fine as a follow-up PR if you prefer to keep this one CLI-only.- Minor: the PR body doesn't use the repo PR template's feature-completeness checklist (
.github/pull_request_template.md). The body covers the same ground honestly (tests, changelog, verification evidence), so this is purely a consistency nit.
No findings under Correctness, Conventions, Cross-engine (the CLI runs on the bundled Lucee only, per cli/CLAUDE.md), Tests, Commits, or Security.
Summary
wheels reloadprintedApplication reloaded successfully.whenever the HTTP exchange completed — it never read the status code. Against an app whose?reload=true500s (the #3053 Adobe regression) or with a wrong reload password (the framework serves the page normally, no restart), the CLI claimed success while nothing reloaded.Verify-first: still reproduced on develop @
bb98ffecd—reload()(Module.cfc) calledmakeHttpRequest()(which readsconn.getResponseCode()then discards it, and follows redirects) and unconditionally printed the green success line. The new failure-path specs were run red-first against unmodified develop:reload()against a stub returning 500 completed and printed success (toThrowfailed), proving the defect live.The 302-vs-200 contract
The framework's reload gate restarts the app and then
location()-redirects (public/Application.cfc :: $handleRestartAppRequest), so a successful reload is always a 302. A wrong password falls through to normal page serving (200, or 404 without a root route). Verified live against the lucee7 docker harness (develop framework,reloadPassword="smokepw"):-L)?reload=true&password=wrongpw?reload=true&password=smokepwChanges (
cli/lucli/Module.cfc)reload()requests with redirects off and judges the raw status via a new$evaluateReloadResponse(statusCode)helper:Wheels.ReloadFailedper the fix(cli): upgrade-check exit code, breaker-scan coverage, ArgSpec positional gaps #2941 exit-code convention, sowheels reload && …CI gates workmakeHttpRequest()now delegates to a status-carrying variantmakeHttpRequestWithStatus(url, followRedirects=true)— the other ~12 call sites keep their exact body-only, redirect-following behavior (the bot-triage "shared helper" trade-off resolved without disturbing callers)/reloadapplies the same verdict, printing red instead of throwing (same false-success line, interactive context)$evaluateReloadResponseis public only for specs; the structural$-prefix sweep inmcpHiddenTools()keeps it off the MCP surfaceCoordination with #3062 (empty-password semantics, branch a2): based on develop, no dependency. If a2 lands "empty = disabled", an empty-password
?reload=truewill render 200 and this CLI correctly reports "not triggered" + exits non-zero, with the existingWHEELS_RELOAD_PASSWORDhint.Tests (CLI suite, lucee7 docker harness)
New in
ReloadCommandSpec(+cli/lucli/tests/StubHttpServer.cfc, a raw-socket fixed-status HTTP stub —com.sun.net.httpserveris unreachable from Lucee's OSGi classloader):$evaluateReloadResponsefor 302 (success), 200 (failure, password hint), 404, 500reload()against the stub on an ephemeral port via the temp project'slucee.json— 500 → throwsWheels.ReloadFailed, 200 → throws, 302 → succeeds quietlyEvidence:
throws … returns 500 -> Failed: The incoming function did not throw…✔ defect reproSshClientSpec/SshPoolSpec:docker: command not foundinside the container;ServerCommandsSpec"reload endpoint responds": the harness stagesreloadPassword=""while the spec hardcodespassword=wheels)No
vendor/wheelschanges; core suite untouched.Fixes #3059
🤖 Generated with Claude Code