diff --git a/changelog.d/3059-cli-reload-false-success.fixed.md b/changelog.d/3059-cli-reload-false-success.fixed.md new file mode 100644 index 0000000000..5b64e95630 --- /dev/null +++ b/changelog.d/3059-cli-reload-false-success.fixed.md @@ -0,0 +1 @@ +- `wheels reload` no longer prints `Application reloaded successfully.` when nothing was reloaded: the CLI now inspects the HTTP status of the `?reload=true` request instead of treating any completed exchange as success. A successful reload always answers with the framework's restart redirect (302), so a normal page render (200/404 — wrong reload password, the warm-path gate fell through) and endpoint errors (4xx/5xx, e.g. the #3053 Adobe regression) are now reported in red with the status and a hint, and the command exits non-zero (`Wheels.ReloadFailed`) so `wheels reload && ...` CI gates work. The interactive console's `/reload` applies the same 302-vs-200 verdict (printing red instead of throwing). Success-path output is unchanged (#3059) diff --git a/cli/lucli/Module.cfc b/cli/lucli/Module.cfc index c2979530c3..38d9c03f8a 100644 --- a/cli/lucli/Module.cfc +++ b/cli/lucli/Module.cfc @@ -834,26 +834,79 @@ component extends="modules.BaseModule" { // See onboarding finding F5. $purgeServerCfclasses(); + // Response honesty (#3059): a SUCCESSFUL reload is ALWAYS a 302 — the + // framework's reload gate restarts the app and then location()-redirects + // (public/Application.cfc :: $handleRestartAppRequest). Redirects stay + // OFF so the raw status is the verdict: following the success-redirect + // would collapse a real reload (302 -> 200 at `/`) into the same 200 a + // wrong-password page render produces. Failures print-then-throw per + // the #2941 exit-code convention so `wheels reload && ...` gates work. + var reloadUrl = "http://localhost:#serverPort#/?reload=true&password=#password#"; + var reloadState = { statusCode = 0 }; try { - var reloadUrl = "http://localhost:#serverPort#/?reload=true&password=#password#"; - var httpResult = makeHttpRequest(reloadUrl); - out("Application reloaded successfully.", "green"); - // Surface the hot-vs-cold reload contract — Wheels does NOT - // re-fire onApplicationStart on `?reload=true`. Users editing - // app/events/onapplicationstart.cfm or config/services.cfm need - // a full restart. See finding #8 in the 2026-04-29 fresh-VM - // triage. - out("Note: onApplicationStart does NOT re-fire. For init-code edits, run `wheels stop && wheels start`.", "cyan"); - verbose("URL: http://localhost:#serverPort#/?reload=true&password=***"); + reloadState.statusCode = makeHttpRequestWithStatus(reloadUrl, false).statusCode; } catch (any e) { out("Failed to reload: #e.message#", "red"); if (!len(password)) { out("Hint: Set WHEELS_RELOAD_PASSWORD in .env or config/settings.cfm", "yellow"); } + throw( + type = "Wheels.ReloadFailed", + message = "Reload request to localhost:#serverPort# failed: #e.message#" + ); } + + var verdict = $evaluateReloadResponse(reloadState.statusCode); + if (!verdict.success) { + out(verdict.message, "red"); + if (!len(password)) { + out("Hint: Set WHEELS_RELOAD_PASSWORD in .env or config/settings.cfm", "yellow"); + } + verbose("URL: http://localhost:#serverPort#/?reload=true&password=***"); + throw(type = "Wheels.ReloadFailed", message = verdict.message); + } + + out("Application reloaded successfully.", "green"); + // Surface the hot-vs-cold reload contract — Wheels does NOT + // re-fire onApplicationStart on `?reload=true`. Users editing + // app/events/onapplicationstart.cfm or config/services.cfm need + // a full restart. See finding #8 in the 2026-04-29 fresh-VM + // triage. + out("Note: onApplicationStart does NOT re-fire. For init-code edits, run `wheels stop && wheels start`.", "cyan"); + verbose("URL: http://localhost:#serverPort#/?reload=true&password=***"); return ""; } + /** + * Verdict for a `?reload=true` response status (#3059). + * + * The framework's reload gate restarts the app and then redirects via + * location(), so a successful reload is always a 3xx (302 in practice). + * A 2xx means the warm-path gate fell through and the page was served + * normally — the reload password didn't match, nothing restarted (404 + * is the same fall-through on an app without a root route). 4xx/5xx + * means the endpoint itself errored (e.g. the #3053 Adobe regression). + * + * Public ONLY so ReloadCommandSpec can unit-test it (the cli/CLAUDE.md + * "public for specs" carve-out) — hidden from MCP via the structural + * $-prefix sweep in mcpHiddenTools(). + */ + public struct function $evaluateReloadResponse(required numeric statusCode) { + if (arguments.statusCode >= 300 && arguments.statusCode < 400) { + return { success = true, message = "" }; + } + if (arguments.statusCode >= 400) { + return { + success = false, + message = "Reload failed: the server returned HTTP #arguments.statusCode#. The application was NOT reloaded — check the server's error output." + }; + } + return { + success = false, + message = "Reload was not triggered: the server served the page normally (HTTP #arguments.statusCode#) instead of answering with the reload redirect (302). The application was NOT reloaded — check the reload password." + }; + } + // ───────────────────────────────────────────────── // start / stop — Dev server management // ───────────────────────────────────────────────── @@ -1506,9 +1559,18 @@ component extends="modules.BaseModule" { case "/reload": out("Reloading application...", "cyan"); try { + // Same 302-vs-200 honesty contract as the reload + // command (#3059) — but interactive, so failures + // print red instead of throwing. var reloadUrl = "http://localhost:#serverPort#/?reload=true&password=#password#"; - makeHttpRequest(reloadUrl); - out("Application reloaded.", "green"); + var reloadVerdict = $evaluateReloadResponse( + makeHttpRequestWithStatus(reloadUrl, false).statusCode + ); + if (reloadVerdict.success) { + out("Application reloaded.", "green"); + } else { + out(reloadVerdict.message, "red"); + } } catch (any e) { out("Reload failed: #e.message#", "red"); } @@ -6913,9 +6975,28 @@ component extends="modules.BaseModule" { } private string function makeHttpRequest(required string requestUrl) { + return makeHttpRequestWithStatus(arguments.requestUrl).body; + } + + /** + * GET `requestUrl` and return BOTH the final status code and the body: + * `{statusCode: numeric, body: string}`. Callers that need to act on the + * HTTP status (reload's 302-vs-200 contract, #3059) use this directly; + * everything else keeps the body-only makeHttpRequest() wrapper above. + * + * `followRedirects=false` surfaces the raw 3xx instead of the post- + * redirect response — required by reload(), where following the + * success-redirect would make a real reload (302 -> 200 at `/`) + * indistinguishable from the wrong-password page render (200). + */ + private struct function makeHttpRequestWithStatus( + required string requestUrl, + boolean followRedirects = true + ) { var javaUrl = createObject("java", "java.net.URL").init(arguments.requestUrl); var conn = javaUrl.openConnection(); conn.setRequestMethod("GET"); + conn.setInstanceFollowRedirects(javacast("boolean", arguments.followRedirects)); conn.setConnectTimeout(5000); conn.setReadTimeout(120000); @@ -6925,7 +7006,7 @@ component extends="modules.BaseModule" { // Scanner.init(null) NPEs on Lucee and surfaces as a useless "null" // error message (#2947 review, #2977). No body — return empty. if (isNull(inputStream)) { - return ""; + return { statusCode = responseCode, body = "" }; } var scanner = createObject("java", "java.util.Scanner").init(inputStream, "UTF-8"); var response = ""; @@ -6933,7 +7014,7 @@ component extends="modules.BaseModule" { response &= scanner.nextLine() & chr(10); } scanner.close(); - return trim(response); + return { statusCode = responseCode, body = trim(response) }; } /** diff --git a/cli/lucli/tests/StubHttpServer.cfc b/cli/lucli/tests/StubHttpServer.cfc new file mode 100644 index 0000000000..ea87c9d1a7 --- /dev/null +++ b/cli/lucli/tests/StubHttpServer.cfc @@ -0,0 +1,92 @@ +/** + * Minimal fixed-status HTTP stub server for CLI specs (#3059). + * + * Binds a wildcard java.net.ServerSocket on an ephemeral port and answers + * every connection with a fixed status line and an empty body, from a + * background thread. Used by ReloadCommandSpec to stand in for a Wheels dev + * server whose `?reload=true` endpoint 500s (the #3053 Adobe regression), + * serves the page normally (wrong reload password -> 200), or + * restarts-then-redirects (successful reload -> 302). + * + * Built on raw sockets (java.base) instead of com.sun.net.httpserver — the + * jdk.httpserver module is not reachable from Lucee's OSGi classloader, so + * createDynamicProxy over HttpHandler dies with NoClassDefFoundError. + * + * Callers MUST stop() the stub (in `finally`) — the accept loop runs until + * the ServerSocket is closed, and the engine waits on spawned threads at + * request end, so a leaked stub would hang the whole test request. + */ +component { + + public any function init(required numeric statusCode) { + variables.statusCode = arguments.statusCode; + // Port 0 + no bind address = ephemeral port on the wildcard address, + // covering both stacks so the CLI's `http://localhost:/...` + // connect succeeds whether localhost resolves to 127.0.0.1 or ::1 + // (same dual-stack concern as PortProbeSpec). + variables.serverSocket = createObject("java", "java.net.ServerSocket").init(javacast("int", 0)); + variables.threadName = "stub-http-" & createUUID(); + + // Thread attributes are passed unquoted so the ServerSocket arrives as + // the live object, not a string render. Unscoped assignments inside a + // thread body are thread-local (`var` is reserved for functions). + thread name="#variables.threadName#" srv=variables.serverSocket code=variables.statusCode { + crlf = chr(13) & chr(10); + response = "HTTP/1.1 " & attributes.code & " Stub" & crlf + & "Content-Length: 0" & crlf + & "Connection: close" & crlf & crlf; + responseBytes = response.getBytes("ISO-8859-1"); + try { + while (true) { + sock = attributes.srv.accept(); + try { + // Never let a silent client (e.g. the isPortOpen() + // connect-probe, which sends nothing) wedge the loop. + sock.setSoTimeout(javacast("int", 2000)); + // Drain the request headers (until CRLFCRLF or EOF) + // before responding, so the client never sees a reset + // while its request is still in flight. + tail = ""; + inStream = sock.getInputStream(); + while (true) { + byteRead = inStream.read(); + if (byteRead == -1) break; + tail = right(tail & chr(byteRead), 4); + if (tail == crlf & crlf) break; + } + outStream = sock.getOutputStream(); + outStream.write(responseBytes); + outStream.flush(); + } catch (any inner) { + // Per-connection failure (probe disconnects, read + // timeout) — keep serving until the socket closes. + } + try { + sock.close(); + } catch (any closeErr) { + } + } + } catch (any e) { + // ServerSocket closed by stop() — accept() throws, loop exits. + } + } + + return this; + } + + public numeric function getPort() { + return variables.serverSocket.getLocalPort(); + } + + public void function stop() { + try { + variables.serverSocket.close(); + } catch (any e) { + } + try { + threadJoin(variables.threadName, 5000); + } catch (any e) { + } + } + +} diff --git a/cli/lucli/tests/specs/commands/ReloadCommandSpec.cfc b/cli/lucli/tests/specs/commands/ReloadCommandSpec.cfc index 51b57e483a..10a7df329a 100644 --- a/cli/lucli/tests/specs/commands/ReloadCommandSpec.cfc +++ b/cli/lucli/tests/specs/commands/ReloadCommandSpec.cfc @@ -1,16 +1,61 @@ /** - * Source-level regression for the `wheels reload` hot-vs-cold contract. + * `wheels reload` — output hints, response honesty, and exit-code contract. * - * The reload command makes a real HTTP request, which is hard to unit-test - * without spinning up a server. Test the *output formatting* by asserting - * the relevant strings appear in Module.cfc::reload(). A heavier integration - * test for reload behavior is out of scope here. + * Source-scan blocks assert output wiring that is impractical to capture from + * a spec (see finding #8 in + * docs/superpowers/plans/2026-04-29-fresh-vm-onboarding-findings.md). * - * See finding #8 in - * docs/superpowers/plans/2026-04-29-fresh-vm-onboarding-findings.md + * The #3059 blocks cover reporting honesty: reload() used to print + * "Application reloaded successfully." whenever the HTTP exchange completed, + * never inspecting the status code. 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/404), and the #3053 + * Adobe regression 500s — both were reported as success. + * + * Failure-path integration tests drive the real reload() against a raw-socket + * HTTP stub (cli.lucli.tests.StubHttpServer) on an ephemeral port (lucee.json + * points the temp project at it), per the issue's "stub server returning 500" + * acceptance criterion. + * Module instantiation mirrors MigrationExitCodeSpec/ServerDetectionSpec. */ component extends="wheels.wheelstest.system.BaseSpec" { + function beforeAll() { + variables.testHelper = new cli.lucli.tests.TestHelper(); + variables.tempRoot = testHelper.scaffoldTempProject(expandPath("/")); + + // Create vendor/wheels stub so the module treats this as a Wheels app. + directoryCreate(tempRoot & "/vendor/wheels", true, true); + + // No inherited port config: each integration test writes its own + // lucee.json pointing at the stub server's ephemeral port. + if (fileExists(tempRoot & "/lucee.json")) fileDelete(tempRoot & "/lucee.json"); + if (fileExists(tempRoot & "/.env")) fileDelete(tempRoot & "/.env"); + + variables.mod = new cli.lucli.Module(cwd = variables.tempRoot); + } + + function afterAll() { + testHelper.cleanupTempProject(variables.tempRoot); + } + + /** + * Boot a fixed-status HTTP stub on an ephemeral port and point the temp + * project's lucee.json at it so detectServerPort() resolves it as the + * project's dev server. Callers stop it via stopStubServer() in `finally`. + */ + private any function startStubServer(required numeric statusCode) { + var stubServer = new cli.lucli.tests.StubHttpServer(arguments.statusCode); + fileWrite(tempRoot & "/lucee.json", serializeJSON({port: stubServer.getPort()})); + return stubServer; + } + + private void function stopStubServer(required any stubServer) { + arguments.stubServer.stop(); + if (fileExists(tempRoot & "/lucee.json")) fileDelete(tempRoot & "/lucee.json"); + } + function run() { describe("wheels reload — output hints", () => { @@ -31,7 +76,7 @@ component extends="wheels.wheelstest.system.BaseSpec" { var moduleSource = fileRead(expandPath("/cli/lucli/Module.cfc")); var startIdx = reFindNoCase("(?m)^[ \t]*public\s+string\s+function\s+reload\s*\(", moduleSource); expect(startIdx).toBeGT(0); - var body = mid(moduleSource, startIdx, 1200); + var body = mid(moduleSource, startIdx, 1600); expect(body).toInclude("parseConsoleArgs(structuredArgs(arguments))"); expect(body).toInclude("detectReloadPassword()"); expect(reFindNoCase("len\(\s*reloadOpts\.password\s*\)\s*\?", body)).toBeGT(0); @@ -39,6 +84,74 @@ component extends="wheels.wheelstest.system.BaseSpec" { }); + describe("$evaluateReloadResponse — 302-vs-200 reload contract (##3059)", () => { + + it("treats the reload redirect (302) as success", () => { + var verdict = mod.$evaluateReloadResponse(302); + expect(verdict.success).toBeTrue(); + }); + + it("treats a normal page render (200) as NOT reloaded and points at the password", () => { + // Wrong reload password: the warm-path gate falls through and + // the framework serves the page normally — no restart happened. + var verdict = mod.$evaluateReloadResponse(200); + expect(verdict.success).toBeFalse(); + expect(verdict.message).toInclude("200"); + expect(verdict.message).toInclude("NOT reloaded"); + expect(lCase(verdict.message)).toInclude("password"); + }); + + it("treats a 404 page render as NOT reloaded", () => { + // Same fall-through as 200 when the app has no root route. + var verdict = mod.$evaluateReloadResponse(404); + expect(verdict.success).toBeFalse(); + expect(verdict.message).toInclude("404"); + }); + + it("treats a 500 as a failed reload and surfaces the status", () => { + // The #3053 Adobe `local.url` shadowing regression 500s on + // every ?reload=true — the CLI used to report it as success. + var verdict = mod.$evaluateReloadResponse(500); + expect(verdict.success).toBeFalse(); + expect(verdict.message).toInclude("500"); + expect(verdict.message).toInclude("NOT reloaded"); + }); + + }); + + describe("reload() against a stub dev server — exit-code honesty (##3059)", () => { + + it("throws Wheels.ReloadFailed when the reload endpoint returns 500", () => { + var stubServer = startStubServer(500); + try { + expect(() => mod.reload(password = "testpw")) + .toThrow(type = "Wheels.ReloadFailed"); + } finally { + stopStubServer(stubServer); + } + }); + + it("throws Wheels.ReloadFailed when the server serves the page normally (200, wrong password)", () => { + var stubServer = startStubServer(200); + try { + expect(() => mod.reload(password = "wrongpw")) + .toThrow(type = "Wheels.ReloadFailed"); + } finally { + stopStubServer(stubServer); + } + }); + + it("succeeds quietly on the reload redirect (302)", () => { + var stubServer = startStubServer(302); + try { + expect(mod.reload(password = "testpw")).toBe(""); + } finally { + stopStubServer(stubServer); + } + }); + + }); + } }