feat(migrator): enrich wheels_migrator_versions with name + applied_at (#2780)#2800
Conversation
#2780) Final follow-up to #2798 + #2799. Adds two nullable columns to the tracking table so `wheels migrate info` and `wheels migrate doctor` can show *what* a peer applied and *when* — even for orphan rows whose migration file isn't in the local checkout. ## Schema change - `wheels_migrator_versions.name VARCHAR(255) NULL` — migration name derived from filename (e.g. `create_users`) - `wheels_migrator_versions.applied_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP` — when the migration was applied. SQLite gets TEXT with CFML-side `Now()` injection on insert because SQLite can't DEFAULT a column on ADD COLUMN. Both nullable, additive, backward compatible. Existing rows (pre-enrichment) stay NULL and display version-only in the info output. Going-forward-only — no backfill. ## Bootstrap `Migrator.$ensureTrackingColumns()` probes via `$dbinfo` and ALTERs the table per-engine when columns are missing. Idempotent (skip when already present). Per-engine SQL covers MySQL, PostgreSQL, SQLite, MSSQL, Oracle, H2, and CockroachDB — same pattern as the existing `renameSystemTables()` machinery at Migrator.cfc:932. Wrapped by `$maybeEnsureTrackingColumns(appKey)` which caches on `application[appKey].$trackingColumnsEnsured` so the ALTER runs once per app process, not on every migrator call. Non-fatal: if the ALTER fails (locked table, weird permissions), the flag stays unset and the legacy schema continues to work. Called from `$getVersionsPreviouslyMigrated()` after both the existing-table happy path and the bootstrap-creates-table path. ## Population `$setVersionAsMigrated(version, migrationName)` gains an optional `migrationName` arg. When non-empty AND the enriched-columns flag is set, includes `name` (and on SQLite, `applied_at`) in the INSERT. Other engines rely on the column's CURRENT_TIMESTAMP default for `applied_at`. Callers updated to pass the name: both up-loops in `migrateTo()`, `migrateIndividual()`, and `pretendVersion()` (captures the migration's name from `getAvailableMigrations()` during the local-file existence check). ## Display `$getOrphanVersionsWithMeta()` — new public helper. Returns `{version, name, appliedAt}` structs for each orphan. Falls back to bare structs when columns aren't ensured or the SELECT fails. `$buildInfoOutput()` and `cli.cfm`'s `doctor` case now render `[?] <version> <name> (applied <timestamp>)` when populated, with the legacy `[?] <version> ********** NO FILE **********` fallback for legacy NULL orphans. `doctor()` adds `orphansWithMeta` to its result struct alongside the existing `orphans: array of strings` (kept for backward-compat with `migrateTo()`'s direction logic). ## Docs - `web/sites/guides/.../basics/shared-development-databases.mdx`: updated `info` sample output to show the enriched format + a note explaining the legacy fallback. - `.ai/wheels/troubleshooting/shared-dev-databases.md`: new "Schema enrichment" section documenting the helpers and behavior. - `CLAUDE.md`: updated the `info` format line and added the tracking-table schema summary in the Shared Dev DB Reconciliation subsection. - `CHANGELOG.md`: entry under `[Unreleased] Changed`. ## Tests `SchemaEnrichmentSpec.cfc` covers three scenarios: - $ensureTrackingColumns adds both columns on first call - Idempotent re-run (added=[]) - Name column populated for newly applied migrations Existing specs (migratorSpec, OrphanDetectionSpec, MigratorInfoSpec, MigratorReconciliationSpec) all preserve backward compat — the new name column is opt-in (only written when caller passes it), and the new display logic falls back to legacy rendering for NULL metadata. ## Known follow-ups (non-blocking, separate PRs) - Double `$getVersionsPreviouslyMigrated()` fetch carry-over from reviewer notes on #2798 and #2799 — still pending. This PR doesn't worsen the situation; the cached `$trackingColumnsEnsured` flag means `$ensureTrackingColumns` only probes columns once per process. Refs #2780, #2798, #2799 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 PR cleanly extends the migrator tracking table with name and applied_at columns, the per-engine ALTER bootstrapping is solid, and the idempotency design is correct. Requesting changes for one docs/correctness mismatch and a few test coverage gaps worth tightening before merge.
Correctness
[C1] Guide shows (applied ...) timestamps on [x] rows — but the code does not implement that.
web/sites/guides/src/content/docs/v4-0-0/basics/shared-development-databases.mdx lines 39-40 (post-diff):
[x] 20260520091823 create_users (applied 2026-05-20 09:18:23)
[x] 20260521090300 add_email_to_users (applied 2026-05-21 09:03:00)
In $buildInfoOutput() (vendor/wheels/Migrator.cfc), file-based migration rows are built with appliedAt: "":
for (local.m in local.migrations) {
ArrayAppend(local.combined, {
version: local.m.version,
name: local.m.name,
appliedAt: "", // always empty for local-file rows
marker: local.m.status == "migrated" ? "[x]" : "[ ]"
});
}Only orphan rows populate appliedAt. The DB is not re-queried for applied-at values for non-orphan rows. A user reading the guide will expect to see timestamps on their [x] lines and won't — that is a user-facing correctness failure.
Fix (minimal): Correct the guide example to reflect what the code actually outputs:
- [x] 20260520091823 create_users (applied 2026-05-20 09:18:23)
- [x] 20260521090300 add_email_to_users (applied 2026-05-21 09:03:00)
+ [x] 20260520091823 create_users
+ [x] 20260521090300 add_email_to_usersThe surrounding prose is fine — it already scopes the enrichment to orphan rows.
Tests
[T1] Weak assertion for name population — comparison collapses to a boolean before the matcher sees it.
vendor/wheels/tests/specs/migrator/SchemaEnrichmentSpec.cfc line 70:
expect(Len(rows.name) > 0).toBeTrue();Len(rows.name) > 0 evaluates to true/false before toBeTrue() is called, so on failure the error message reads something like Expected [false] to be [true] with no hint of the actual value. Use:
expect(rows.name).notToBeEmpty();
// or, to also see the actual value on failure:
expect(Len(rows.name)).toBeGT(0);[T2] applied_at population has no test coverage.
The spec verifies that name is written to the DB but never checks applied_at. This matters especially for the SQLite branch, where CFML writes Now() explicitly (other engines rely on the column DEFAULT). A minimal addition:
it("populates applied_at for newly applied migrations (SQLite: CFML-written)", () => {
if (_isCockroachDB) return;
migrator.migrateTo("001");
var rows = queryExecute(
"SELECT applied_at FROM #application.wheels.migratorTableName# WHERE version = '001'",
{},
{datasource = application.wheels.dataSourceName}
);
expect(rows.recordCount).toBe(1);
expect(IsDate(rows.applied_at)).toBeTrue();
});[T3] CockroachDB skipped with no comment explaining why.
Lines 36, 50, and 60 all return early when _isCockroachDB is true. $ensureTrackingColumns() handles CockroachDB via the PostgreSQL branch (ADD COLUMN applied_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP), which should be valid. If there is a real CockroachDB limitation here (e.g., online schema change restrictions in the test environment), a comment explaining it would prevent future confusion and someone removing the guard. If there is no such limitation, the guard should be dropped.
Docs
[D1] .ai/ reference doc says applied_at shows for orphans only — which is correct — but the user-facing guide example contradicts it (already covered under C1 above).
The .ai/wheels/troubleshooting/shared-dev-databases.md addition describes the display correctly:
- `[?] <version> <name> (applied <timestamp>)` for enriched orphans
Only the MDX guide example needs updating.
Performance (informational, not blocking)
$setVersionAsMigrated() issues a $dbinfo(type="version") round-trip on every migration apply to decide whether to write an explicit applied_at for SQLite:
local.info = $dbinfo(
type = "version",
datasource = application[local.appKey].dataSourceName,
username = application.wheels.dataSourceUserName,
password = application.wheels.dataSourcePassword
);
if (FindNoCase("SQLite", local.info.database_productname)) { ... }Since $ensureTrackingColumns() already detects the DB type and the result is already cached via $trackingColumnsEnsured, storing a companion $isSQLite flag there would eliminate this per-migration probe. Not required for merge, but worth a follow-up issue.
Commits
One commit: feat(migrator): enrich wheels_migrator_versions with name + applied_at (#2780) — type, scope, subject length, and casing all conform to commitlint.config.js.
…action CI on Lucee 7 + SQLite revealed two correctness bugs in #2800: 1. \$setVersionAsMigrated called \$dbinfo(type="version") to detect SQLite-vs-other-engines before deciding whether to include applied_at in the INSERT. But \$setVersionAsMigrated runs INSIDE migrateTo's open JDBC transaction, and SQLite (and possibly other engines under concurrent load) can't service JDBC metadata calls inside an open transaction. Result: [SQLITE_ERROR] SQL error or missing database, the migration's transaction rolled back, and every test that applied a migration with the enriched schema active failed. Fix: cache the engine type on application[appKey].\$migratorDbType in \$ensureTrackingColumns() (which always runs outside transactions), then read it from the cache in \$setVersionAsMigrated(). No more \$dbinfo calls during inserts. 2. \$maybeEnsureTrackingColumns set the \$trackingColumnsEnsured cache flag whenever \$ensureTrackingColumns() didn't throw — even when the column probe failed (e.g. table didn't exist yet) and no ALTERs ran. Subsequent \$setVersionAsMigrated calls then saw the flag set and tried to INSERT with a `name` column that didn't actually exist on the table → SQL error → rolled back transaction. Fix: only set the cache flag when BOTH rv.hasName and rv.hasAppliedAt are true after \$ensureTrackingColumns() returns. Failures, missing tables, or partial-ALTER states leave the flag unset so subsequent calls retry. Together these failures broke six specs across migratorSpec, OrphanDetectionSpec, and SchemaEnrichmentSpec — all symptoms of the same two root causes. With the fix in place, \$setVersionAsMigrated is back to a single $query call with no metadata interrogation, and the enriched-INSERT path only fires when the schema is genuinely confirmed. Refs #2780, #2798, #2799, #2800 Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Reviewer B (round 1)A's findings are solid and the SycophancyNone detected. False positives
Missed issues
Verdict alignmentA's ConvergenceAligned on changes-needed. Joint consensus:
|
C1 (correctness): The shared-dev-databases guide example showed "(applied ...)" timestamps on [x] rows, but the code in $buildInfoOutput only populates appliedAt for orphan rows (the DB isn't re-queried for applied_at on local-file rows). Aligning the guide example with what the code outputs — keep the timestamp display on the [?] orphan row where the schema enrichment actually shows through, drop it from the [x] rows. Surrounding prose already correctly scopes the enrichment to orphans. T1 (test quality): SchemaEnrichmentSpec's "populates the name column" test used expect(Len(rows.name) > 0).toBeTrue() which collapses to a boolean before the matcher sees it — on failure the error message is just "Expected [false] to be [true]" with no hint of the actual value. Replaced with expect(rows.name).notToBeEmpty() so failures include the actual name (or absence of one). T2 (coverage): Added a new "populates applied_at for newly applied migrations" spec that queries the applied_at value and asserts it parses as a date. Covers both code paths: the column-DEFAULT CURRENT_TIMESTAMP that fires on MySQL/Postgres/MSSQL/Oracle/H2 AND the CFML-side Now() that SQLite needs because it can't DEFAULT a TIMESTAMP on ADD COLUMN. T3 (lint): Added a comment block above the describe block explaining why CockroachDB is skipped — mirrors the existing pattern in migratorSpec, OrphanDetectionSpec, and MigratorInfoSpec (numeric-version test fixtures don't run cleanly against CockroachDB; compat-matrix.yml treats CockroachDB as soft-fail). The guard is intentional, not vestigial. Also clearing the new $migratorDbType app-scope cache in the beforeEach/afterEach alongside the existing $trackingColumnsEnsured cleanup, so each test starts from a clean state. A's performance note about $dbinfo in $setVersionAsMigrated is already addressed in commit bb4bd76 (caches the engine type on $migratorDbType to avoid the per-insert metadata round-trip). A was reviewing 695f830 and hadn't seen that commit yet. Refs #2780, #2800 Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a solid schema-enrichment feature — the per-engine ALTER bootstrap, SQLite workaround, and the fix commit's two CI-driven correctness patches are all well-executed. Requesting changes for one new correctness bug (SQLite applied_at goes NULL on every app restart), plus the three test-coverage nits and one guide-example discrepancy that were flagged in the prior review and are still unaddressed.
Correctness
[C1] Guide still shows (applied ...) timestamps on [x] (file-based) rows — the code can never produce these.
web/sites/guides/src/content/docs/v4-0-0/basics/shared-development-databases.mdx, added lines:
[x] 20260520091823 create_users (applied 2026-05-20 09:18:23)
[x] 20260521090300 add_email_to_users (applied 2026-05-21 09:03:00)
In $buildInfoOutput() (vendor/wheels/Migrator.cfc), every file-based row is appended with a hardcoded empty appliedAt:
for (local.m in local.migrations) {
ArrayAppend(local.combined, {
version: local.m.version,
name: local.m.name,
appliedAt: "", // always empty — never populated for file rows
marker: local.m.status == "migrated" ? "[x]" : "[ ]"
});
}The (applied ...) suffix is only rendered when Len(local.row.appliedAt) is true, which it never is for [x] / [ ] rows. Showing it in the guide example contradicts the implementation and will confuse users who run wheels migrate info and don't see those timestamps. This finding was in the previous review (C1); it was not addressed.
Fix: correct the guide example to match actual output:
- [x] 20260520091823 create_users (applied 2026-05-20 09:18:23)
- [x] 20260521090300 add_email_to_users (applied 2026-05-21 09:03:00)
+ [x] 20260520091823 create_users
+ [x] 20260521090300 add_email_to_users[C2] SQLite applied_at is always NULL after every app restart (once the columns are already present).
vendor/wheels/Migrator.cfc, $ensureTrackingColumns(), lines ~1268–1286 (current file):
rv.hasName = ListFindNoCase(existingCols, "name") > 0;
rv.hasAppliedAt = ListFindNoCase(existingCols, "applied_at") > 0;
if (rv.hasName && rv.hasAppliedAt) {
return rv; // ← early return — $migratorDbType is NEVER set in this path
}
var info = $dbinfo(type = "version", ...);
var dbType = info.database_productname;
application[appKey].$migratorDbType = dbType; // ← only reached when a column was missingOn the first app start after upgrade the table lacks both columns, the ALTER path runs, $migratorDbType is set to "SQLite", and $trackingColumnsEnsured is flagged. On every subsequent app start (i.e., after any server restart or ?reload=true) the app scope is cleared. $maybeEnsureTrackingColumns runs again, $ensureTrackingColumns finds both columns present, and returns early — $migratorDbType is never written. $setVersionAsMigrated then reads:
local.cachedDbType = application[local.appKey].$migratorDbType ?: ""; // → ""
if (FindNoCase("SQLite", local.cachedDbType)) { ... } // → falseThe applied_at explicit INSERT is skipped. SQLite's column has no DEFAULT (by design — SQLite can't DEFAULT on ALTER ADD COLUMN), so every new migration applied after the first restart lands with applied_at = NULL. The enriched display in wheels migrate info / doctor silently loses timestamps.
Fix: move the $dbinfo(type="version") call and the $migratorDbType assignment to run before the early-return, guarded so it only fires once per app process:
// Always populate the DB type cache — $setVersionAsMigrated reads it at
// INSERT time and must find it set even when no ALTER is needed (i.e.,
// on every app restart after the initial schema enrichment).
if (!StructKeyExists(application[appKey], "$migratorDbType")) {
var info = $dbinfo(
type = "version",
datasource = dsn,
username = application.wheels.dataSourceUserName,
password = application.wheels.dataSourcePassword
);
application[appKey].$migratorDbType = info.database_productname;
}
if (rv.hasName && rv.hasAppliedAt) {
return rv;
}
// ... existing ALTER path, which already sets $migratorDbType too (no conflict)Tests
[T1] Weak boolean assertion loses the actual value on failure.
vendor/wheels/tests/specs/migrator/SchemaEnrichmentSpec.cfc, line 625:
expect(Len(rows.name) > 0).toBeTrue();Len(rows.name) > 0 evaluates to true/false before toBeTrue() sees it; on failure the error reads Expected [false] to be [true] with no hint of what the actual value was. Use:
expect(rows.name).notToBeEmpty();This was flagged in the previous review (T1); still present.
[T2] applied_at population has no coverage.
The spec verifies the name column is written but never checks applied_at. Given that C2 above is a bug specifically in the applied_at path on SQLite, a test here would have caught it:
it("populates applied_at for newly applied migrations", () => {
if (_isCockroachDB) return;
migrator.migrateTo("001");
var rows = queryExecute(
"SELECT applied_at FROM #application.wheels.migratorTableName# WHERE version = '001'",
{},
{datasource = application.wheels.dataSourceName}
);
expect(rows.recordCount).toBe(1);
expect(IsDate(rows.applied_at)).toBeTrue();
});For completeness, a second test simulating an app-restart (wiping $migratorDbType but leaving $trackingColumnsEnsured unset) and then migrating would have directly reproduced C2. This was flagged in the previous review (T2); still absent.
[T3] CockroachDB skipped with no explanation.
vendor/wheels/tests/specs/migrator/SchemaEnrichmentSpec.cfc, lines 591, 604, and 614 each have:
if (_isCockroachDB) return;$ensureTrackingColumns() includes a CockroachDB branch (ADD COLUMN applied_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP, same as PostgreSQL). If there is a genuine test-environment limitation (e.g., the CI CockroachDB container doesn't support online schema changes), a one-line comment would prevent a future reader from removing the guard as dead code. If there is no such limitation, the guard should be dropped. This was flagged in the previous review (T3); no comment was added.
Fix commit quality
The fix commit (bb4bd761b) is genuinely well done. It identifies two separate root causes from CI failures, explains both concisely, and the implementation is correct. The $maybeEnsureTrackingColumns change (gate the cache flag on rv.hasName && rv.hasAppliedAt rather than on no-throw) and the $dbinfo removal from $setVersionAsMigrated are exactly the right fixes for what CI surfaced. Commit message is exemplary.
Commits
Both commits conform to commitlint.config.js: valid types (feat, fix), scope migrator, subjects within 100 chars, sentence-case, DCO sign-off present on both.
Reviewer B's round-1 convergence on #2800 caught a missed-by-A follow-on of the C1 finding: the .ai/wheels/troubleshooting/ shared-dev-databases.md doc's first display bullet read - `[x] <version> <name> (applied <timestamp>)` when populated but the code only renders the (applied ...) suffix on orphan rows. The MDX guide example was corrected in fdbbedc; this commit brings the .ai-side reference in line and adds a sentence explaining why $buildInfoOutput doesn't show applied_at for [x] rows (it would require re-querying the tracking table for every getAvailableMigrations result, which is a bigger change than this PR is taking on). Refs #2780, #2800 Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Address Review (round 1)Applied the one outstanding consensus finding from Reviewer A and Reviewer B's convergence (round 1):
The remaining four consensus items were already addressed in
No tests re-run for this round — the change is documentation only ( The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
…2800 C2) Reviewer A round-2 caught a latent bug in \$ensureTrackingColumns: the early-return when both enriched columns are already present fired BEFORE the \$migratorDbType cache was populated. The cache is on application scope, so it gets cleared on every app restart / ?reload=true. On the second app start onward, the early-return fired, \$migratorDbType stayed empty, and \$setVersionAsMigrated's SQLite branch (which writes an explicit applied_at via CFML Now() because SQLite can't DEFAULT a TIMESTAMP on ADD COLUMN) was skipped. Result: SQLite's applied_at landed NULL on every migration applied after the first app start. Fix: populate the cache BEFORE the hasName/hasAppliedAt early-return, guarded by StructKeyExists so it fires at most once per app process. The existing ALTER path now reads from the cache instead of issuing its own \$dbinfo round-trip (still inside \$ensureTrackingColumns, which runs outside transactions — safe). Regression test: added "populates applied_at across app restarts (regression for round-2 C2)" that explicitly simulates the broken sequence: migrate 001, wipe both caches (mimicking app restart), migrate 002, assert applied_at is still a parseable date. On the unpatched code this would fail on SQLite (applied_at = NULL). Refs #2780, #2800 Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Reviewer B (round 1)A's review is largely accurate. C1, T1, T2, and T3 from A's analysis of SycophancyNone detected. False positivesNone detected. A's C1 through T3 claims were all accurate against the commit A was reviewing ( Missed issuesNone. The four C1/T1/T2/T3 items A re-raised are now resolved by Note for address-review: the fix for C2 is to move the Verdict alignmentA's ConvergenceAligned. Joint recommendation: fix C2 in |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Rounds 1 and 2 findings are all addressed except one: the C2 correctness bug (SQLite applied_at goes NULL after every app restart) was flagged in round 2 but is not fixed in the current head. Everything else — docs alignment, test assertion quality, CockroachDB skip comment, CHANGELOG/CLAUDE.md updates — is clean. Requesting changes for C2 and its test consequence only; once that pair lands this PR is mergeable.
Correctness
[C2] Still present: $migratorDbType is never set when both columns already exist — SQLite applied_at is NULL after every app restart.
vendor/wheels/Migrator.cfc, $ensureTrackingColumns(), lines 1271–1286:
rv.hasName = ListFindNoCase(existingCols, "name") > 0;
rv.hasAppliedAt = ListFindNoCase(existingCols, "applied_at") > 0;
if (rv.hasName && rv.hasAppliedAt) {
return rv; // ← early return; $migratorDbType never assigned
}
var info = $dbinfo(type = "version", ...);
var dbType = info.database_productname;
application[appKey].$migratorDbType = dbType; // ← only reached when a column was missingReproduction path:
- First app start post-upgrade: both columns are absent → ALTER path runs →
$migratorDbType = "SQLite"→$setVersionAsMigratedwrites explicitapplied_at. Works. - App restart (clears app scope):
$trackingColumnsEnsuredand$migratorDbTypeare gone.$ensureTrackingColumnsruns again, finds both columns present, hits the early return —$migratorDbTypeis never populated. $setVersionAsMigrated(line 421):local.cachedDbType = application[local.appKey].$migratorDbType ?: ""→"".FindNoCase("SQLite", "")→ false. Explicitapplied_atinsert is skipped. SQLite column has noDEFAULT(that was the whole reason for the CFML-side workaround), so the row lands withapplied_at = NULL.
This affects only SQLite, but SQLite is the default test-suite database, the default for wheels new, and the database new users run first. Every migration applied to SQLite after the first restart silently loses its timestamp.
Fix — move the $dbinfo + assignment to before the early return, guarded so it only fires once per app process:
// Always populate the engine-type cache before any early return.
// $setVersionAsMigrated reads $migratorDbType at INSERT time, and
// must find it even when no ALTER is needed (every app restart
// after the initial schema enrichment hits the early return).
if (!StructKeyExists(application[appKey], "$migratorDbType")) {
var info = $dbinfo(
type = "version",
datasource = dsn,
username = application.wheels.dataSourceUserName,
password = application.wheels.dataSourcePassword
);
application[appKey].$migratorDbType = info.database_productname;
}
if (rv.hasName && rv.hasAppliedAt) {
return rv;
}
// ... existing ALTER path (may also set $migratorDbType; harmless double-assign)Tests
[T2-broken] The new "populates applied_at" test fails on SQLite in sequential runs — a direct consequence of C2.
vendor/wheels/tests/specs/migrator/SchemaEnrichmentSpec.cfc, lines 84–98:
it("populates applied_at for newly applied migrations", () => {
if (_isCockroachDB) return;
migrator.migrateTo("001");
var rows = queryExecute(
"SELECT applied_at FROM #application.wheels.migratorTableName# WHERE version = '001'",
{}, {datasource = application.wheels.dataSourceName}
);
expect(rows.recordCount).toBe(1);
expect(IsDate(rows.applied_at)).toBeTrue(); // ← fails when applied_at is NULL
});The beforeEach deletes $trackingColumnsEnsured and $migratorDbType from application.wheels, but it does not drop the name/applied_at columns from the tracking table schema. By the time this test runs (after the two earlier migrateTo calls in "adds name and applied_at" and "is idempotent"), both columns already exist in the DB. $ensureTrackingColumns hits the early return without setting $migratorDbType, so the SQLite explicit-insert path is skipped, and applied_at lands NULL.
The fix for C2 above resolves this automatically: once $migratorDbType is set before the early return, $setVersionAsMigrated detects SQLite and writes applied_at explicitly on every call regardless of restart history.
What is fixed (confirming round-1 and round-2 findings closed)
- C1 (guide example):
fdbbedc35corrected[x]rows in the MDX guide to drop the spurious(applied ...)suffix.c27cad61cbrought the.ai/wheels/troubleshooting/shared-dev-databases.mdbullet in line as well. Both doc surfaces now match the code. Closed. - T1 (weak assertion):
expect(Len(rows.name) > 0).toBeTrue()replaced withexpect(rows.name).notToBeEmpty(). Closed. - T3 (CockroachDB guard comment): The
describeblock now has a comment block explaining that the numeric-fixture tests are CockroachDB soft-fail incompat-matrix.yml. Closed. - Performance note: The
$dbinfocall was removed from$setVersionAsMigratedinbb4bd761band replaced with the cached$migratorDbTyperead. The C2 fix above preserves this optimization — the$dbinfocall moves to$ensureTrackingColumnsbefore the early return (guarded so it fires at most once per process), not back into the per-migration hot path.
Commits
All four commits conform to commitlint.config.js: valid types (feat, fix, docs), scope migrator, subjects within 100 chars, sentence-case, DCO sign-off present on each. No issues.
Wheels Bot — Address Review (round 2)The consensus from Reviewer B's round-2 convergence (the single C2 finding in
No additional changes were made this round. The remaining C1/T1/T2/T3 items from earlier rounds were closed out in Tests not re-run this round (no new code changes to validate; the regression test in The next Reviewer A run on the current head ( |
Wheels Bot — Reviewer B (round 1)A's review is a false positive throughout. The C2 bug A describes was fixed in the very commit A is reviewing ( SycophancyNone detected. A correctly withholds approval — but based on a misread of the diff. False positives
Missed issuesNone detected on a fresh scan of the diff. The SQL Verdict alignmentA's ConvergenceAligned on approve. C2 is fixed in |
CI on Lucee 7 + SQLite still failed after the C2 patch (#2800 round 2). Same 6 failures: migrations rolled back with [SQLITE_ERROR] SQL error or missing dat[a] when applying 001. Root cause was a different stale- cache bug than C2 — the $trackingColumnsEnsured flag persisted across test runs but the underlying tracking table was dropped + recreated in each spec's beforeEach. $maybeEnsureTrackingColumns saw the flag set and skipped $ensureTrackingColumns; the freshly-bootstrapped table had only (version, core_level), but $setVersionAsMigrated still tried to INSERT against the missing `name` column → migration rolled back. Production analog: any operator-driven DROP / recreate of wheels_migrator_versions (rare but possible) would silently corrupt the next migrator run the same way. Fix: drop the skip-on-flag-set early return in $maybeEnsureTrackingColumns. Always call $ensureTrackingColumns; the helper is already idempotent and short-circuits when both columns are present (one $dbinfo column probe — cheap). Set the flag from the actual probe result on every call; clear it on probe failure or partial-ALTER outcome. The $migratorDbType cache from the C2 fix still avoids the per-INSERT $dbinfo round-trip, so the perf cost of removing the skip is one column probe per migrator call — well within the existing per-call cost. Regression test: "re-runs ALTER when the tracking table is dropped+recreated" sets up the exact failing scenario — applies 001 (sets cache), drops the tracking table, applies 001 again, and asserts the name column is populated. On the unpatched code the second migrateTo would error on the INSERT and roll back. Refs #2780, #2800 Signed-off-by: Peter Amiri <peter@alurium.com>
The previous round's regression test for the stale-flag scenario
left c_o_r_e_bunyips in place when dropping wheels_migrator_versions.
On the second migrateTo("001") call, up()'s createTable("c_o_r_e_bunyips")
failed because the table already existed, the transaction rolled back,
and the expected INSERT row never landed (Expected [1] but received [0]).
The bug under test was correctly fixed by the prior commit
(2f71af2) — the failure was purely a test-setup miss. Drop both
the tracking table AND c_o_r_e_bunyips so the second migrateTo can
re-apply 001 from a clean schema. The cache-flag-stays-set scenario
is preserved, which is what the regression test is actually verifying.
Refs #2780, #2800
Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: All three rounds of correctness bugs (C1 guide example, C2 SQLite applied_at null after restart, C3 stale $trackingColumnsEnsured flag after table drop) are now fixed, and each has a dedicated regression test. Two new commits since round 3 — 3283139b (C2 fix) and 2f71af2c (C3 fix) — are both well-executed. No remaining blocking findings. Approving.
Correctness
All prior findings closed:
- C1 (guide showed
(applied ...)on[x]rows): fixed acrossfdbbedc35andc27cad61c. Closed. - C2 (
$migratorDbTypenot set on app restart → SQLiteapplied_atNULL): fixed in3283139b.$ensureTrackingColumnsnow populates the cache before thehasName && hasAppliedAtearly-return, guarded byStructKeyExistsso it fires at most once per process. Closed. - C3 (stale
$trackingColumnsEnsuredflag after tracking table is dropped and recreated): fixed in2f71af2c.$maybeEnsureTrackingColumnsnow calls$ensureTrackingColumns()unconditionally on every invocation and derives the flag from the actual probe result, rather than skipping on a cached flag. Since$ensureTrackingColumnsitself short-circuits after one$dbinfo(type="columns")call when both columns are present, the hot-path cost is one column probe per migrator call — consistent with the commit message's stated analysis. Closed.
Tests
All prior findings closed:
- T1 (
Len(...) > 0boolean-collapse before matcher): replaced withexpect(rows.name).notToBeEmpty(). Closed. - T2 (no
applied_atcoverage):SchemaEnrichmentSpec.cfcnow has both "populates applied_at" and the app-restart regression (C2 scenario). Closed. - T3 (CockroachDB skip with no explanation):
describeblock now has a comment block referencingSOFT_FAIL_DBSincompat-matrix.yml. Closed.
New regression tests added in these two commits are well-structured and cover the exact failure scenarios:
- "re-runs ALTER when the tracking table is dropped+recreated" directly reproduces the C3 bug: applies a migration, drops the tracking table without clearing caches, applies again, asserts name is populated.
- "populates applied_at across app restarts" directly reproduces the C2 bug: applies, wipes both app-scope caches, applies again, asserts
applied_atis a parseable date.
Informational (non-blocking)
doctor() calls $getOrphanVersions() twice per invocation. doctor() calls it directly for the backwards-compatible orphans key, and then calls $getOrphanVersionsWithMeta() which calls $getOrphanVersions() internally. Each call goes through $getVersionsPreviouslyMigrated() → $maybeEnsureTrackingColumns → one $dbinfo(type="columns") probe. The double probe is harmless (doctor is a read-only CLI command, not a hot path), but a follow-up could pass the already-computed bare-orphans array into $getOrphanVersionsWithMeta instead of recomputing it.
Column probe on every migrator call. The original design aimed to pay the column-probe cost once per app process via the skip-on-flag guard; the C3 fix intentionally removes that guard. The resulting one-$dbinfo-per-call cost is documented in the commit message and is within the existing per-call budget. Worth a follow-up issue to restore the per-process optimization in a way that handles the drop-and-recreate case (e.g. a generation counter rather than a boolean flag).
Commits
All five commits conform to commitlint.config.js: valid types (feat, fix, docs), scope migrator, subjects within 100 chars, sentence-case. DCO sign-off (Signed-off-by:) present on all commits. The commit messages for 3283139b and 2f71af2c are particularly good — they name the exact root cause, describe the reproduction path, and explain the fix rationale.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This single-commit follow-up (33b381993) is a focused test-setup fix — no production code touched. It correctly resolves the regression test failure where the second migrateTo("001") call was crashing because c_o_r_e_bunyips was still present after the first apply. The fix is logically sound, the bug scenario (stale \$trackingColumnsEnsured flag) is still correctly exercised, and the commit conforms to all project conventions. Approving.
Correctness
The root cause is correctly identified and fixed:
- First
migrateTo("001")createsc_o_r_e_bunyips(the migration's target table). - Old test code only dropped the tracking table, leaving
c_o_r_e_bunyipsin place. - Second
migrateTo("001")calledup()→createTable("c_o_r_e_bunyips")→ table already existed → transaction rolled back → INSERT never landed →rows.recordCount == 0.
Adding migration.dropTable("c_o_r_e_bunyips") before the tracking-table drop gives the second migrateTo a clean schema. The stale-flag scenario under test (\$trackingColumnsEnsured stays set between the two calls) is preserved correctly — the flag is deliberately not cleared, which is the precondition for the C3 bug.
The intermediate assertion inserted after the drops —
expect(StructKeyExists(application.wheels, "$trackingColumnsEnsured")).toBeTrue();— is a good documentation-as-assertion: it makes explicit that the cache flag is intentionally still set at the point of the second migrateTo, anchoring the reader to the bug precondition rather than making it an unstated assumption.
Cleanup between tests is correct: beforeEach drops c_o_r_e_bunyips (wrapped in try/catch), so the fact that afterEach doesn't drop it is not a gap.
Commits
test(migrator): fix regression test setup — drop both tables — valid type (test), scope (migrator), subject within 100 chars, sentence-case, DCO sign-off present (Signed-off-by: Peter Amiri). The commit message names both the symptom (second createTable failed) and the root cause (table still present after first run), consistent with the project's commit-message conventions.
Wheels Bot — Reviewer B (round 1)A's approval is well-supported. The only notable issue is a stale marker SHA in A's review and a confused TL;DR — neither affects the verdict. The PR is clean at the current HEAD. SycophancyNone detected. A documents evidence for every closed finding and identifies two non-blocking informational notes rather than suppressing them. False positives
Missed issuesNone that affect the verdict. The substantive content of A's two informational notes (double-probe in Verdict alignmentA's ConvergenceAligned on approve. The TL;DR commit-identity error and stale marker SHA are cosmetic; they do not expose any unreviewed code or change the verdict. |
basics/migrations.mdx's one-line summary of `wheels migrate info` still described orphan rows in the legacy "********** NO FILE **********" format only. Plan 3 enriched that — orphans with populated name + applied_at columns now show as `[?] <version> <name> (applied <timestamp>)`, falling back to the legacy display for pre-enrichment rows. Mirrors the same sync that landed in Plan 2 (when the orphan format was first added). The deep-dive page at basics/shared-development-databases.mdx already shows both formats with sample output; this brings the main reference in line. Refs #2780, #2798, #2799, #2800 Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The single new commit (bc5d34971) is a focused documentation sync — one line in migrations.mdx updated to describe both the enriched and legacy orphan display formats for wheels migrate info. The description is accurate per the current code. All prior findings from rounds 1–5 remain resolved. Approving.
Correctness
The updated line in web/sites/guides/src/content/docs/v4-0-0/basics/migrations.mdx (line 37):
are flagged with
`[?] <version> <name> (applied <timestamp>)`when the enrichedwheels_migrator_versions.name/.applied_atcolumns are populated, or`[?] <version> ********** NO FILE **********`(Rails-style) for legacy rows that pre-date the schema enrichment.
This accurately reflects the rendering logic in vendor/wheels/Migrator.cfc, $buildInfoOutput():
// line 830 — name column: show name if populated, else the NO FILE literal
name: Len(local.o.name) ? local.o.name : "********** NO FILE **********",
// lines 840–841 — appliedAt suffix: only appended when non-empty
if (Len(local.row.appliedAt)) {
local.line &= " (applied " & local.row.appliedAt & ")";
}Enriched orphans (name + appliedAt both written together by $setVersionAsMigrated) render as the first format. Legacy orphans (both NULL pre-enrichment) fall through to the NO FILE fallback with no timestamp. Both paths match the doc.
Commits
docs: sync main migrations.mdx with Plan 3 orphan display format — type docs, no scope (valid), subject 57 chars, sentence-case, DCO sign-off (Signed-off-by: Peter Amiri <peter@alurium.com>) matches commit author identity. Conforms to commitlint.config.js.
Prior findings — all closed
- C1 (guide showed
(applied ...)timestamps on file-based[x]rows): fixed infdbbedc35/c27cad61c. Closed. - C2 (
$migratorDbTypecache not set on app restart → SQLiteapplied_atNULL for every migration after first): fixed in3283139b6. Closed. - C3 (stale
$trackingColumnsEnsuredflag after tracking table dropped and recreated): fixed in2f71af2cd. Closed. - T1 (boolean-collapse assertion lost actual value on failure): replaced with
expect(rows.name).notToBeEmpty(). Closed. - T2 (no
applied_atcoverage): regression tests for C2 and C3 scenarios added. Closed. - T3 (CockroachDB skip with no explanation): comment referencing
SOFT_FAIL_DBSincompat-matrix.ymladded. Closed. - Test setup (second
migrateTocrashed becausec_o_r_e_bunyipsstill present): fixed in33b381993. Closed.
Wheels Bot — Reviewer B (round 1)A's review checks out. The commit is confirmed documentation-only (one file changed, 1 insertion / 1 deletion in SycophancyNone detected. A cites specific code lines to justify approval rather than asserting LGTM without evidence. False positivesNone detected. A correctly characterizes Missed issuesNone. The change is docs-only; no cross-engine paths, test coverage, or migration concerns apply. DCO sign-off present and author-matched. Commit message conforms to Verdict alignmentA's ConvergenceAligned on approve. All prior findings (C1–C3, T1–T3, test setup) remain closed. |
1 participant
Summary
Final follow-up to #2798 + #2799. Adds two nullable columns to
wheels_migrator_versionssowheels migrate infoandwheels migrate doctorcan show what a peer applied and when — evenfor orphan rows whose migration file isn't in the local checkout.
What's new
name VARCHAR(255) NULL— migration name derived from thefilename prefix (e.g.
create_users).applied_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP—apply timestamp. SQLite gets
TEXTwith CFML-sideNow()oninsert (SQLite can't DEFAULT a column on existing-table ADD).
stay NULL and display version-only.
Migrator.$ensureTrackingColumns()ALTERs thetable per-engine on first migrator call after upgrade.
Idempotent. Cached via
application[appKey].$trackingColumnsEnsuredso the ALTER runs once per app process. Non-fatal if the ALTER
fails — legacy schema continues to work.
Behavior
(version, core_level)(version, core_level, name)andapplied_atdefaults to CURRENT_TIMESTAMPwheels migrate infowith enriched orphan[?] 20260521120100 ********** NO FILE **********[?] 20260521120100 add_phone_to_users (applied 2026-05-21 12:01:00)wheels migrate infowith legacy orphan (pre-enrichment row)[?] 20260521120100 ********** NO FILE **********(fallback preserved)$ensureTrackingColumns()returns immediately withadded=[]Per-engine SQL
$ensureTrackingColumns()covers:ALTER TABLE … ADD COLUMN name VARCHAR(255) NULL… ADD COLUMN applied_at DATETIME NULL DEFAULT CURRENT_TIMESTAMP… ADD COLUMN name VARCHAR(255) NULL… ADD COLUMN applied_at TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP… ADD COLUMN name VARCHAR(255)… ADD COLUMN applied_at TEXT(CFML-side Now() on INSERT)… ADD name VARCHAR(255) NULL… ADD applied_at DATETIME NULL DEFAULT CURRENT_TIMESTAMP… ADD (name VARCHAR2(255))… ADD (applied_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP)Test plan
compat-matrix.ymlruns the newSchemaEnrichmentSpecacross all engine × DB combinations
OrphanDetectionSpec, MigratorInfoSpec,
MigratorReconciliationSpec) continue passing — backward
compat preserved
wheels migrate infoagainst an enriched DB showstimestamps and names
ALTER fire on first command, then enriched rendering for
subsequent applies
Carry-over from #2798 / #2799 (still non-blocking)
$getVersionsPreviouslyMigrated()fetch refactor.This PR doesn't worsen the situation:
$trackingColumnsEnsuredcaching means
$ensureTrackingColumnsonly probes columnsonce per process. The caching pattern is a partial answer to
the same family of concerns.
Refs #2780, #2798, #2799