[bpamiri]

Summary

Wave-2 remediation for the migration-helpers package of the internal framework review. Seven migrator findings fixed in a single commit: a crash on the documented columnName path of addIndex(), two redundant-DB-round-trip hotspots (engine sniffing and table-metadata probes), a duplicated SQL pre-execution pipeline, an engine re-sniff in dropTable() plus a whole-schema FK probe, fourteen byte-identical typed column helpers, and diffAll() swallowing its own validation throws.

Findings addressed

• MG3 — addIndex() default indexName dereferences arguments.columnNames before $combineArguments resolves the columnName alias (undefined-key throw) @ vendor/wheels/migrator/Migration.cfc:418-432 — default now computed in the body after $combineArguments.
• MG5 — $execute() / $executeWithParams() duplicate the terminator / SQL-file / debug-capture pipeline, with a per-statement $dbinfo(version) round-trip @ vendor/wheels/migrator/Base.cfc:177 — extracted into $prepareMigrationSql() preserving the exact old pipeline order; Oracle-terminator check now uses memoized $getDBType(); $executeWithParams() gains the dataSource override its sibling already had (Base.cfc:159).
• MG7 — fourteen byte-identical typed column helpers in TableDefinition @ vendor/wheels/migrator/TableDefinition.cfc:133 — deduped into shared $addTypedColumns(), preserving the float() / uniqueidentifier() outlier defaults and timestamp()'s columnType pass-through verbatim.
• MG9 — diffAll() swallows deliberate validation throws in the per-model missing-table catch and reports "no drift" @ vendor/wheels/migrator/AutoMigrator.cfc:185-231 — heuristicThreshold validated up front (mirroring RenameDetector's range check); Wheels.InvalidThreshold, Wheels.InvalidRenameHint, Wheels.DuplicateRenameHint, Wheels.RenameHintTypeMismatch are rethrown.
• MG11 — every migration CFC init() triggers a fresh $dbinfo(version) round-trip during discovery @ vendor/wheels/migrator/Base.cfc:15 — $getDBType() memoizes the resolved adapter name per datasource on the application scope.
• MG13 — addRecord() probes table metadata twice per record @ vendor/wheels/migrator/Base.cfc:202 — $getColumns() gets a request-scoped cache keyed by datasource + table; $execute() (DDL) invalidates the cache (Base.cfc:148-150) while $executeWithParams() (data-only) does not.
• MG14 — dropTable() re-sniffs the engine and $getForeignKeys() lists every table in the schema @ vendor/wheels/migrator/Migration.cfc:119 + vendor/wheels/migrator/Base.cfc:103 — uses this.adapter.adapterName() (byte-identical to $getDBType() strings across all 7 adapters) and a zero-row single-table SELECT probe.

Findings verified already-fixed

None — all seven findings reproduced against current origin/develop (spot-confirmed by the reviewer for MG3 at Migration.cfc:416-420 and MG13).

Source

Internal multi-agent framework review 2026-06-09, wave 2, package migration-helpers.

Tests

• New BDD specs under vendor/wheels/tests/specs/migrator/ (written red against pre-fix code, green post-fix):
  • MigratorBaseCachingSpec.cfc — both caches; uses a poison-the-cache technique to distinguish cached reads from re-probes, and verifies $execute() invalidates the $getColumns() cache.
  • migrationCommandsSpec.cfc — addIndex() via the columnName alias (the MG3 crash path).
  • autoMigratorSpec.cfc — diffAll() throws on invalid threshold and rethrows rename-hint validation errors.
• Local verification: full migrator area on Lucee 7 + SQLite — 270 pass / 0 fail / 0 error / 6 skipped.
• Full engine x DB coverage deferred to the CI compat matrix (the real gate per repo policy).

Cross-engine notes

• Catch-body state in the new specs uses the shared-struct pattern (cross-engine invariant New master #11 — local.X in catch does not persist on BoxLang).
• argumentCollection = arguments.args forwarding in $addTypedColumns() has in-tree precedent (Injector.cfc:174, controller/filters.cfm:66).
• The single-table FK probe SQL is valid under every adapter's quoteTableName override; adapterName() returns are byte-identical to the old $getDBType() strings for all 7 adapters.
• Locally verified on Lucee 7 + SQLite only — the TableDefinition arguments-forwarding refactor and the new probe SQL should be confirmed by the adobe2023 / adobe2025 / boxlang legs of the CI matrix before merge.
• Known non-blocking follow-ups flagged in review: the same param-default pattern survives at databaseAdapters/Abstract.cfc:300 (unreachable via the fixed Migration path); $getForeignKeys() now maps any probe failure to "no FKs", shifting failure to the subsequent DROP; two parallel application-scope engine caches could be unified later.

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

This PR addresses seven internal migrator findings (MG3, MG5, MG7, MG9, MG11, MG13, MG14) from a multi-agent framework audit: an addIndex crash on the columnName alias, two redundant DB-round-trip hotspots ($getDBType and $getColumns), duplicate SQL pre-execution pipeline, dropTable re-sniff, fourteen byte-identical typed-column helpers, and silent swallowing of validation throws in diffAll. The changes are well-scoped, the cross-engine annotations are accurate, and the three new BDD specs cover the three riskiest paths. Verdict: comment — one low-severity correctness nit and one docs gap; no blocking issues.

Correctness

$getColumns() cache key folds to lowercase but the $dbinfo probe does not — case-sensitive database mismatch.

vendor/wheels/migrator/Base.cfc, line 211:

local.cacheKey = LCase(application[local.appKey].dataSourceName & "|" & arguments.tableName);

The cache key is case-folded, but the downstream $dbinfo(type="columns", table=arguments.tableName) call uses the original case. On a case-sensitive database (PostgreSQL with quoted identifiers, Oracle, case-sensitive SQLite), two callers using different case for the same table name — e.g., $getColumns("Authors") followed by $getColumns("authors") — share the same cache slot. The second call returns data from the first lookup even if the actual tables differ.

Low risk in practice (migration code usually passes consistent casing) but technically incorrect. Recommend keying on the original arguments.tableName without case folding, or documenting that the caller is responsible for consistent casing:

// consistent with $dbinfo(table=arguments.tableName)
local.cacheKey = application[local.appKey].dataSourceName & "|" & arguments.tableName;

Cross-engine

No blocking issues found.

The $addTypedColumns refactor calls column(argumentCollection = arguments.args) where arguments.args is a plain struct parameter — not the raw arguments scope itself. Cross-engine invariant #10 (Adobe 2023/2025 rejecting the arguments scope as attributeCollection on built-in tags) does not apply here: column() is a CFC method, not a built-in CFML tag, and the value passed is a regular struct. The $addTypedColumns private helper lives on TableDefinition.cfc, a standalone CFC instantiated with CreateObject, not a mixin — invariant #7 (private mixin functions not integrated) does not apply either.

The new $getForeignKeys table-existence probe uses SELECT 1 FROM #local.quotedTable# WHERE 1=0. The fallback branch StructKeyExists(this, "adapter") ? ... : arguments.table passes an unquoted table name into the SQL when no adapter is present. In practice this branch is unreachable because Migration.init() always populates this.adapter and $getForeignKeys is only called from dropTable(). But the dead code reads like a safety net that silently breaks on table names with spaces or dashes. Consider a Throw instead of the silent fallback, or remove the ternary and always require the adapter.

The BoxLang catch-body invariant (#11 — local.X = ... inside catch is discarded) is correctly addressed in both $getForeignKeys (local.state = {tableExists = true}) and the new BDD specs (shared-struct pattern throughout). Well done.

Tests

MigratorBaseCachingSpec.cfc line 38 poisons the application-scope $migratorAdapterNames cache with "SentinelAdapter" during the $getDBType test. The finally block at line 42 correctly restores the real value, so the isolation is sound.

No test covers the $addTypedColumns deduplication directly (e.g., verifying that float()'s default=""/allowNull=true outlier defaults survive the refactor). The existing TableDefinition specs exercise the public helpers indirectly, which is sufficient, but a single spot-check would make the preservation of those defaults explicit.

Docs

vendor/wheels/migrator/CLAUDE.md does not mention the two new caches introduced by this PR:

• application[appKey].$migratorAdapterNames (application-scoped, keyed by datasource name) — memoized adapter name per datasource, written by $getDBType()
• request.$wheelsMigratorColumns (request-scoped, keyed by dsName|tableName) — column list per table, written by $getColumns(), dropped wholesale by $execute()

A future agent debugging a stale-schema or wrong-adapter issue will grep for $migratorAdapterNames and find nothing in the reference doc. A short paragraph in the migrator CLAUDE.md under an "Internal caches" heading would make these discoverable.

No CHANGELOG entry — acknowledged in the PR body as deliberate (campaign consolidated). Acceptable.

Commits

fix(migrator): addIndex alias order, dbtype caching, helper dedup, diffAll rethrow — valid type, valid scope, header within 100 chars, sentence-case, reflects the why. Clean.

Security

No SQL injection vectors introduced. The $getForeignKeys probe routes the table name through this.adapter.quoteTableName() before interpolating it into SQL. The $getColumns cache key uses only the datasource name and table name from internal application config, not user input.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is well-calibrated. I verified every cited diff location, the cross-engine reasoning, and the test isolation claim. No sycophancy, no false positives. All findings are confirmed accurate and actionable. Verdict alignment is correct. Converging on changes needed.

Sycophancy

None detected. A issued a "comment" verdict (not "approve") and cited specific file/line evidence for each finding.

False positives

None detected.

• Cache key case-folding (Base.cfc / $getColumns()): verified in the diff — local.cacheKey = LCase(application[local.appKey].dataSourceName & "|" & arguments.tableName) folds on entry, but $dbinfo(type="columns", table=arguments.tableName) uses the original case. On a case-sensitive engine (PostgreSQL with quoted identifiers), "Authors" and "authors" share a cache slot but would probe different tables. A's description is accurate.
• $getForeignKeys dead-code ternary: StructKeyExists(this, "adapter") ? this.adapter.quoteTableName(arguments.table) : arguments.table — A correctly identifies the false-branch as unreachable (Migration.init() always sets this.adapter) and correctly notes it would silently pass an unquoted name to SQL if somehow reached. A's suggestion is sound.
• Cross-engine $addTypedColumns analysis: invariant 10 (arguments scope as attributeCollection on built-in tags) correctly does not apply — column() is a CFC method and arguments.args is a plain struct. Invariant 7 (private mixin functions not integrated) correctly does not apply — TableDefinition.cfc is a standalone CFC, not a mixin. Both claims verified.
• BoxLang catch-body handling: the shared-struct pattern (local.state = {tableExists = true}) correctly addresses invariant 11 in $getForeignKeys and in the new BDD specs. Confirmed.
• Test poison/restore isolation (MigratorBaseCachingSpec.cfc lines 38/42): line 38 poisons the cache, the finally at line 42 restores it. A's line numbers and analysis are accurate.

Missed issues

None of materiality. Two I checked and ruled out:

• $execute() deletes the column cache before checking dry-run mode — intentional; the test explicitly asserts this behavior under $wheelsDebugSQL = true.
• $executeWithParams() not clearing the cache — intentional per the PR description (data-only path, never DDL).

Verdict alignment

"Comment" is consistent with findings that are all non-blocking (low-severity correctness nit, dead-code cleanup, minor test gap, docs gap). Correct call.

Convergence

Aligned. Joint recommendation is to apply A's four concrete suggestions before merge: (1) remove LCase() from the $getColumns cache key, keying on arguments.tableName verbatim; (2) replace the unquoted-table fallback in $getForeignKeys with a Throw; (3) add a spot-check asserting float()/uniqueidentifier() outlier defaults survive the $addTypedColumns refactor; (4) add an "Internal caches" section to vendor/wheels/migrator/CLAUDE.md documenting $migratorAdapterNames (application-scoped) and $wheelsMigratorColumns (request-scoped).