Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/3081-cli-migration-exit-codes.fixed.md
Original file line number Diff line number Diff line change
@@ -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 <version>.`) 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 `<version>` / missing `--yes`) still exits 0. The migrate-side sibling of the #2973/#2987 seeder honesty fix (#3081)
74 changes: 68 additions & 6 deletions cli/lucli/Module.cfc
Original file line number Diff line number Diff line change
Expand Up @@ -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 <version>."
* 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 = [
Expand Down Expand Up @@ -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 <version>.") 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
Expand Down Expand Up @@ -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 <version> / 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 "";
}

Expand Down Expand Up @@ -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)
Expand Down
129 changes: 129 additions & 0 deletions cli/lucli/tests/specs/commands/MigrationExitCodeSpec.cfc
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/**
* 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 <version>." 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 $?.
mod.__arguments = ["reset", "--force"];
expect(() => mod.db()).toThrow();
});

});

});

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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 <version> --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 <version> --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 <version> --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 <version> --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`

Expand All @@ -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: <message>`.
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: <message>` — and the command exits non-zero, so a `wheels migrate latest && ...` CI gate does not proceed as if the schema moved.

<Aside type="note" title="Generating migration files">
`wheels migrate` only applies migrations — it does not create them. To stamp out a new migration file, use `wheels generate migration` (blank) or `wheels generate property` (add-column). See [Code Generation](../code-generation/).
Expand Down Expand Up @@ -142,7 +142,7 @@ Runs pending migrations and reseeds the database in one shot. Destructive in the
wheels db reset --force
```

Prints `Running migrations...`, dispatches `wheels migrate latest`, then prints `Running seeds...` and dispatches `wheels seed`. Ends with `Database reset complete.` in green.
Prints `Running migrations...`, dispatches `wheels migrate latest`, then prints `Running seeds...` and dispatches `wheels seed`. Ends with `Database reset complete.` in green. A migration or seed failure exits non-zero so the error is visible in scripts and CI.

##### `status`

Expand Down
Loading