diff --git a/changelog.d/3081-cli-migration-exit-codes.fixed.md b/changelog.d/3081-cli-migration-exit-codes.fixed.md new file mode 100644 index 0000000000..116e8ecf3a --- /dev/null +++ b/changelog.d/3081-cli-migration-exit-codes.fixed.md @@ -0,0 +1 @@ +- CLI: migration failures now reach the exit code. A failed `up()`/`down()` step that `migrateTo()` folds into its output (e.g. `Error migrating to .`) made `wheels migrate latest|up|down` print the error inside the green success block and still exit 0; `wheels db reset --force` exited 0 when it refused on a `ServerNotRunning` check; and `wheels migrate forget|pretend` refusals (not in tracking table, matching local file exists, already applied, no matching file) printed red but exited 0. All three now exit non-zero, so a `wheels migrate latest && …` CI gate no longer proceeds as if the schema moved. Informational dry-run output (missing `` / missing `--yes`) still exits 0. The migrate-side sibling of the #2973/#2987 seeder honesty fix (#3081) diff --git a/cli/lucli/Module.cfc b/cli/lucli/Module.cfc index 3d9a863ee2..578c1adf62 100644 --- a/cli/lucli/Module.cfc +++ b/cli/lucli/Module.cfc @@ -3793,6 +3793,39 @@ component extends="modules.BaseModule" { // ── Migration Execution ────────────────────────── + /** + * True when migrator output carries the failed-step signature that + * Migrator.$runMigrationStep() emits — "Error migrating to ." + * migrateTo() concatenates that line into its returned string instead of + * throwing, so the /wheels/cli bridge reports success:true and the CLI's + * parseCliResponse() success->exit-code mapping never trips. Detecting the + * signature lets a failed up()/down() reach the exit code — the + * migrate-side sibling of the #2973/#2987 seeder honesty fix (#3081). + * + * Anchored on the literal label plus a numeric version so normal progress + * lines ("Migrating from 0 up to N.") never match. Public ONLY so the CLI + * specs can reach it (cli/CLAUDE.md "public for specs" carve-out); the + * mcpHiddenTools() structural $-prefix sweep keeps it off the MCP surface. + */ + public boolean function $migrationOutputIndicatesFailure(required string output) { + return reFindNoCase("Error migrating(\s+to)?\s+[0-9]+\.", arguments.output) > 0; + } + + /** + * True when a /wheels/cli migration-style response should map to a non-zero + * CLI exit: either an explicit success:false (forget/pretend refusals such + * as "not found in the tracking table" or "matching local file exists", or + * a bridge-surfaced error) OR the subtler honesty gap where the bridge + * reports success:true while migrateTo() folded a failed step into the + * message. Public for specs; hidden from MCP via the structural sweep (#3081). + */ + public boolean function $cliMigrationResponseFailed(required struct response) { + if (!(arguments.response.success ?: true)) { + return true; + } + return $migrationOutputIndicatesFailure(arguments.response.message ?: ""); + } + private string function runMigration(required string action) { var serverPort = $requireRunningServer( hints = [ @@ -3835,6 +3868,20 @@ component extends="modules.BaseModule" { // the previous code silently treated it as success. See issue #2315. var result = parseCliResponse(httpResult, "Migration #action#"); + // Honesty gap (#3081): migrateTo() folds a failed up()/down() step into + // its returned message ("Error migrating to .") instead of + // throwing, so the bridge reports success:true and parseCliResponse() + // above doesn't trip. Detect that signature for the schema-mutating + // actions so a failed migration reaches the exit code — the migrate-side + // sibling of the #2973/#2987 seeder honesty fix. + if (mutatingAction && $migrationOutputIndicatesFailure(result.message ?: "")) { + out(result.message ?: "", "red"); + throw( + type = "MigrationError", + message = "Migration #arguments.action# failed — a migration step reported an error (see output above)." + ); + } + // For `doctor`, switch the output color to yellow when the report // signals unhealthy state (orphans or pending migrations). Green // on an unhealthy result reads as "everything's fine" when it @@ -3913,14 +3960,24 @@ component extends="modules.BaseModule" { } var parsed = isJSON(httpResult) ? deserializeJSON(httpResult) : {success: false, message: "Invalid response"}; - var success = parsed.success ?: false; var msg = parsed.message ?: ""; - if (success) { - out(msg, "green"); - } else { - out(msg, "red"); + // Honesty gap (#3081): forget/pretend refusals ("not found in the + // tracking table", "matching local file exists", "already applied", + // "no matching file") come back as success:false but previously printed + // red and returned "" — exit 0, indistinguishable from a real mutation + // in a script. Throw so the refusal reaches the exit code. The + // informational dry-run branches above (missing / missing + // --yes) still return "" (exit 0) because they precede the server call. + if ($cliMigrationResponseFailed(parsed)) { + out(Len(msg) ? msg : "#verb# refused.", "red"); + throw( + type = "MigrationError", + message = Len(msg) ? msg : "#verb# refused — no change made." + ); } + + out(msg, "green"); return ""; } @@ -4061,7 +4118,12 @@ component extends="modules.BaseModule" { runMigration("latest"); } catch (any e) { out("Migration failed: #e.message#", "red"); - return ""; + // #3081: a refusal (e.g. ServerNotRunning) or a failed migration + // must reach the exit code — swallowing it (return "") made + // `db reset --force` report success while the schema never moved. + // `wheels migrate latest` and `wheels seed` already exit non-zero + // on the same refusal; this aligns `db reset` with them. + rethrow; } // Step 2: Seed (unless skipped) diff --git a/cli/lucli/tests/specs/commands/MigrationExitCodeSpec.cfc b/cli/lucli/tests/specs/commands/MigrationExitCodeSpec.cfc new file mode 100644 index 0000000000..d78324c8b8 --- /dev/null +++ b/cli/lucli/tests/specs/commands/MigrationExitCodeSpec.cfc @@ -0,0 +1,138 @@ +/** + * Migration failures must reach the CLI exit code (issue #3081). + * + * The seeder got an honesty fix in #2973/#2987 (partial failure → + * success=false + non-zero exit). The migrate surface had the same class of + * bug: a failed up()/down() printed "Error migrating to ." yet + * exited 0, `db reset --force` exited 0 on a ServerNotRunning refusal, and + * forget/pretend refusals exited 0 — all indistinguishable from success in + * a `wheels migrate latest && ...` CI gate. + * + * These specs cover the CLI-side reporting-honesty seams: the failure + * detection helpers and the `db reset --force` rethrow. Transaction/rollback + * behaviour is already correct and is not exercised here. + * + * Module instantiation mirrors DbCommandSpec/MigrateCommandSpec — the CLI + * test runner loads the module against a scaffolded temp project; server- + * dependent paths throw "No running Wheels server detected" because the + * temp project has no bound server, which is exactly what gap 2 relies on. + */ +component extends="wheels.wheelstest.system.BaseSpec" { + + function beforeAll() { + variables.testHelper = new cli.lucli.tests.TestHelper(); + variables.tempRoot = testHelper.scaffoldTempProject(expandPath("/")); + + // Create vendor/wheels stub + directoryCreate(tempRoot & "/vendor/wheels", true, true); + + // scaffoldTempProject() copies the repo's lucee.json (which carries a + // `port`) into the temp project. Strip the port so detectServerPort() + // can never resolve a live server for this project — the gap-2 case + // must deterministically take the ServerNotRunning refusal path + // (requireProjectConfig=true refuses the common-port fallback), and we + // must never accidentally POST a real `reset` at a server that happens + // to be bound to the configured port. + fileWrite(tempRoot & "/lucee.json", "{}"); + + variables.mod = new cli.lucli.Module(cwd = variables.tempRoot); + } + + function afterAll() { + testHelper.cleanupTempProject(variables.tempRoot); + } + + function run() { + + describe("migration failure → CLI exit code (##3081)", () => { + + describe("$migrationOutputIndicatesFailure — swallowed-step detection (gap 1)", () => { + + it("flags a failed up() step that migrateTo() folded into its return string", () => { + var output = "Migrating from 0 up to 20260101000000." & chr(10) + & "-------- 20260101000000_create_widgets --------" & chr(10) + & "Error migrating to 20260101000000." & chr(10) + & "[SQLITE_ERROR] SQL error or missing database (no such function: NOW)"; + expect(mod.$migrationOutputIndicatesFailure(output)).toBeTrue(); + }); + + it("flags an IrreversibleMigration down() failure", () => { + var output = "Migrating from 20260101000000 down to 0." & chr(10) + & "Error migrating to 20260101000000." & chr(10) + & "Cannot reverse this migration (IrreversibleMigration)."; + expect(mod.$migrationOutputIndicatesFailure(output)).toBeTrue(); + }); + + it("does not flag normal successful migration output", () => { + var output = "Migrating from 0 up to 20260101000000." & chr(10) + & "-------- 20260101000000_create_widgets --------" & chr(10) + & "CREATE TABLE widgets (id INTEGER PRIMARY KEY)"; + expect(mod.$migrationOutputIndicatesFailure(output)).toBeFalse(); + }); + + it("does not flag the no-op 'No pending migrations' message", () => { + expect( + mod.$migrationOutputIndicatesFailure("No pending migrations. Database is at version 20260101000000.") + ).toBeFalse(); + }); + + }); + + describe("$cliMigrationResponseFailed — bridge response honesty", () => { + + it("treats success:true carrying a failed-step message as a failure (gap 1)", () => { + expect( + mod.$cliMigrationResponseFailed({ + success: true, + message: "Error migrating to 20260101000000." & chr(10) & "[SQLITE_ERROR] no such function: NOW" + }) + ).toBeTrue(); + }); + + it("treats an explicit success:false refusal as a failure (gap 3)", () => { + expect( + mod.$cliMigrationResponseFailed({ + success: false, + message: "Version 20260101000000 was not found in the tracking table." + }) + ).toBeTrue(); + }); + + it("treats a clean success response as not-failed", () => { + expect( + mod.$cliMigrationResponseFailed({ + success: true, + message: "Migrating from 0 up to 20260101000000." & chr(10) & "CREATE TABLE widgets (...)" + }) + ).toBeFalse(); + }); + + }); + + describe("db reset --force refusal honesty (gap 2)", () => { + + it("rethrows the ServerNotRunning refusal instead of swallowing it (exit non-zero)", () => { + // No server is bound to the scaffolded temp project, so + // runMigration("latest") -> $requireRunningServer throws. + // The pre-fix dbReset catch printed red and returned "" + // (exit 0); the fix rethrows so the refusal reaches $?. + // + // arg1=/arg2= exercises the callerArgs path through + // structuredArgs() — the same mechanism DbCommandSpec's + // throwing spec uses. The instance-level `__arguments` + // stash is NOT reliable here: set externally it lands in + // the component's `this` scope, but structuredArgs()'s + // unscoped read resolves the variables scope in the + // in-server suite, so db() would see zero args and print + // usage help instead of dispatching reset. + expect(() => mod.db(arg1 = "reset", arg2 = "--force")) + .toThrow(type = "Wheels.ServerNotRunning"); + }); + + }); + + }); + + } + +} diff --git a/web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/database.mdx b/web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/database.mdx index 6c3146410f..88329eb0cb 100644 --- a/web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/database.mdx +++ b/web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/database.mdx @@ -57,11 +57,11 @@ Single-command health report for the migration tracking table. Lists orphan trac ##### `forget` -Deletes a stale tracking row: `wheels migrate forget --yes`. Dry-run by default; `--yes` is required to mutate. Refuses if a matching local migration file exists, or if the version isn't in the tracking table. +Deletes a stale tracking row: `wheels migrate forget --yes`. Dry-run by default; `--yes` is required to mutate. Refuses if a matching local migration file exists, or if the version isn't in the tracking table. Refusals exit non-zero. ##### `pretend` -Records a version as applied without running its `up()`: `wheels migrate pretend --yes`. Dry-run by default; `--yes` is required to mutate. Refuses if the version is already applied or has no matching file. +Records a version as applied without running its `up()`: `wheels migrate pretend --yes`. Dry-run by default; `--yes` is required to mutate. Refuses if the version is already applied or has no matching file. Refusals exit non-zero. ##### `rename-system-tables` @@ -73,7 +73,7 @@ Migrates legacy framework bookkeeping tables to their current `wheels_`-prefixed wheels migrate latest ``` -Prints `Running migration: latest...` in cyan, dispatches the run to the server, and reports completion with a message like `Migration latest completed.` in green. Migration errors are caught and reported as `Migration failed: `. +Prints `Running migration: latest...` in cyan, dispatches the run to the server, and reports completion with a message like `Migration latest completed.` in green. Migration errors are caught and reported as `Migration failed: ` — and the command exits non-zero, so a `wheels migrate latest && ...` CI gate does not proceed as if the schema moved.