[bpamiri]

Summary

Fixes #2789 — model().create() / .update() / .deleteAll() inside a migration's up() or down() silently rolled back the row on MSSQL.

The Migrator already wraps every up() / down() invocation in its own outer cftransaction. Wheels Model's default transaction="commit" opened a nested cftransaction whose JDBC nested-transaction semantics differ per adapter — most acutely on MSSQL, where the inner commit didn't release the row and the eventual outer commit silently dropped it. The user saw Role.create() succeed (auto-increment fired, no errors), and a follow-up raw queryExecute insert of an FK row persist normally — but the role row was rolled back, leaving an orphan FK pointing at a nonexistent ID.

Fix

Two-half change:

1. Migrator.cfc sets request.$wheelsTransactionWrapper = true around each up() / down() / migrateIndividual() invocation, and explicitly clears it in both the success and error paths. Request-scope so it never persists past the request; never leaks across migrations because each iteration re-sets/clears.

2. Model::invokeWithTransaction() reads the signal first. When set, the call switches to "alreadyopen" mode, skipping the nested cftransaction entirely. The existing per-connection tracking in request.wheels.transactions continues to work as a fallback for Wheels-internal nested calls — the new check is strictly additive.

Engine-agnostic: works the same way on MSSQL, MySQL, PostgreSQL, SQLite, Oracle, and H2 because the fix eliminates the nested transaction rather than working around per-engine JDBC behavior. Approach (1) from the issue body — detect outer transaction and short-circuit — picked over (2) warn-on-detection because it actually fixes the data-loss bug instead of merely surfacing it.

The signal name $wheelsTransactionWrapper is intentionally generic — app code that wraps work in its own cftransaction can opt in the same way:

transaction {
    request.$wheelsTransactionWrapper = true;
    try {
        model("User").create(...);   // skips its own nested transaction
        model("Order").create(...);
    } finally {
        StructDelete(request, "$wheelsTransactionWrapper");
    }
}

(Docs follow-up will land in a separate PR via bot-update-docs.yml.)

Tests

• vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc — four specs exercising invokeWithTransaction's response to the flag for create / save / deleteAll, plus a control test proving rollback still works when the flag is unset. The control is critical — it establishes that the rollback path actually fires in this environment, so the inverse assertion ("with the flag set, rollback is bypassed and the row persists") is meaningful.

• vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc — four specs running an end-to-end migration whose up() calls model().create(), asserting the row persists; that the flag is set during up(); and that it's cleared after both up() and down() return. Uses a separate test-migrations directory (_assets/migrator/migrations_2789/) so the synthetic 001_seed_tag_via_model_create.cfc doesn't collide with the reconciliation spec's fixtures.

Local: bash tools/test-local.sh — 3780 passed (Lucee 7 + SQLite). Cross-engine matrix runs in CI.

Test plan

• Failing-then-passing specs (TDD)
• bash tools/test-local.sh model — green
• bash tools/test-local.sh migrator — green
• bash tools/test-local.sh (full) — green
• Cross-engine matrix passes (CI)
• MSSQL reproduces issue Migrator: model().create()/.update()/.deleteAll() inside up()/down() silently rolls back the row (nested-transaction conflict with the migrator's outer transaction) #2789's exact scenario (CI's MSSQL leg)
• No regressions in the existing transaction specs

Pull request checklist

• Tests — failing-then-passing specs at vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc and vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc
• Framework Docs — handled separately by bot-update-docs.yml
• AI Reference Docs — handled separately by bot-update-docs.yml
• CLAUDE.md — handled separately by bot-update-docs.yml
• CHANGELOG.md
• Test runner passes locally (Lucee 7 + SQLite)

Fixes #2789

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR — This PR correctly identifies and fixes a real data-loss bug: the Migrator's outer cftransaction was silently rolling back model().create() / .update() / .deleteAll() calls inside up() / down() on MSSQL because Wheels Model opened a nested cftransaction whose per-adapter commit semantics differed. The request-scope signal approach is engine-agnostic and structurally sound. However, the test spec violates the cross-engine closure-scope invariant (CLAUDE.md anti-pattern #10) in a way that can cause the spec to error or silently mis-skip the CockroachDB guard on Adobe CF, and the migration fixture stashes state in application scope when request scope is sufficient and safer. Requesting changes before merge.

---

Cross-engine

var _isCockroachDB captured inside it() closures — anti-pattern #10 violation

vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc, line 12:

var _isCockroachDB = CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB";

Lines 30, 37, 42, 47 inside the it() closures:

if (_isCockroachDB) return;

var in a CFML function body creates a local-scope variable. CLAUDE.md anti-pattern #10 states: "CFML closures can't access outer local vars." On Adobe CF the it() callbacks cannot see local._isCockroachDB declared in the enclosing run() method. In non-strict mode this silently evaluates to empty string (falsy) — the guard never fires and the CockroachDB-only skip is defeated. In strict mode the closures throw "variable not found" and the entire spec errors on every Adobe run.

Fix — use a struct to cross the closure boundary (the documented right form):

var ctx = {isCockroachDB: CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB"};

// inside each it():
if (ctx.isCockroachDB) return;

Or drop the explicit var so the assignment falls to variables scope, which closures do inherit:

_isCockroachDB = CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB";

---

Correctness

Migration fixture stashes signal-capture state in application scope unnecessarily

vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc, lines 13–17:

application.$issue2789FlagDuringUp = (
    StructKeyExists(request, "$wheelsTransactionWrapper")
    && request.$wheelsTransactionWrapper
);

The PR body says "we stash on application scope (request is per-call)" — but request scope is per-HTTP-request, not per-function-call. migrator.migrateTo("001") → local.migration.cfc.up() is a chain of CFC method invocations inside the same HTTP request, so request.$issue2789FlagDuringUp is readable from the spec's it() callback after migrateTo returns. Using application scope creates global mutable state shared by all concurrently executing test workers and requires explicit StructDelete cleanup in beforeEach/afterEach to avoid cross-test pollution. request scope is naturally cleaned up at request end.

Fix:

request.$issue2789FlagDuringUp = (
    StructKeyExists(request, "$wheelsTransactionWrapper")
    && request.$wheelsTransactionWrapper
);

Update MigratorOuterTransactionSpec.cfc's beforeEach/afterEach cleanup lines, and the assertion:

expect(request.$issue2789FlagDuringUp).toBeTrue();

Pre-existing (not introduced here, but visible in context): migrateIndividual() error path issues transaction action="commit" after transaction action="rollback"

vendor/wheels/Migrator.cfc, lines 265–271 (post-patch):

} catch (any e) {
    ...
    transaction action="rollback";
    StructDelete(request, "$wheelsTransactionWrapper");   // error path
}
StructDelete(request, "$wheelsTransactionWrapper");        // always runs
transaction action="commit";                               // always runs — after rollback!

Unlike the migrateTo() loops, migrateIndividual() has no break in the catch, so control always falls through to transaction action="commit" even on the error path. The new StructDelete placement is correct (flag cleared in both paths). The commit-after-rollback is pre-existing behaviour, but it's worth a follow-up issue since the fix passes through this function.

---

Conventions

Multi-line comment blocks in production code

vendor/wheels/Migrator.cfc — three comment blocks of 5–8 lines each (down-path lines 125–131, up-path lines 182–189, migrateIndividual lines 259–261). vendor/wheels/model/transactions.cfc lines 39–48, another 9-line block.

CLAUDE.md: "Never write multi-line comment blocks — one short line max." The WHY here is genuinely non-obvious and the issue number is the right shorthand:

// Issue #2789: skip nested cftransaction when migrator's outer one is active.
request.$wheelsTransactionWrapper = true;

---

Tests

The test coverage is solid: the OuterTransactionSignalSpec exercises create, save, and deleteAll, includes a control test proving rollback works when the signal is absent, and the MigratorOuterTransactionSpec drives an end-to-end migration asserting row persistence. The CockroachDB guard itself is the right call since CockroachDB's auto-commit semantics make the "nested transaction bypassed → row persists even with transaction='rollback'" assertion untestable there. Fix the closure-scope bug and the tests are structurally sound.

One nit: variables.migration set in beforeAll() (line 7) is not referenced anywhere in the spec body — it appears to exist solely so helperFunctions.cfm helper calls have a migration variable available. If that's the case, a brief comment or an explicit variables.migration = ... would clarify intent over the bare unscoped assignment.

---

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is substantively solid: two genuine bugs with correct diagnoses, one accurate convention call, and a correctly-labelled pre-existing observation. One minor line-number inaccuracy, no false positives, no significant missed issues. I'm aligned with the request-changes verdict.

Sycophancy

None detected. A supplies specific code citations and a clear rationale for each finding; the CHANGES_REQUESTED verdict is earned, not reflexive.

False positives

None detected. Verified each finding against the actual diff:

• Closure scope (var _isCockroachDB) — confirmed real. var _isCockroachDB in run() creates local._isCockroachDB; the four it() closures that reference it cannot see the outer function's local scope on Adobe CF (CLAUDE.md anti-pattern Fixed bug in $findRoute() that causes blow up on unmatched named route #10). A's fix (shared struct) is the documented right form. Minor nit: A cites "line 12" but the declaration is at line 17 of MigratorOuterTransactionSpec.cfc (counted from the actual file). The finding is correct regardless.

• application scope in migration fixture — confirmed real. The comment // We stash on application scope (request is per-call) contains a factual CFML error: request scope is per-HTTP-request, not per-function-call. migrator.migrateTo("001") → local.migration.cfc.up() is a same-request call chain, so request.$issue2789FlagDuringUp written inside up() is readable by the spec's it() callback after migrateTo returns. application scope creates global shared state vulnerable to cross-worker pollution. The beforeEach/afterEach StructDelete(application, "$issue2789FlagDuringUp") confirms the concern is real.

• Multi-line comment blocks — confirmed real. Three 5–9 line blocks added in Migrator.cfc (down-path, up-path, migrateIndividual) and one 9-line block in transactions.cfc. CLAUDE.md: "Never write multi-line comment blocks — one short line max."

• migrateIndividual() commit-after-rollback — correctly labelled pre-existing. The PR's StructDelete placement in the error path is correct; the transaction action="commit" after the catch block pre-existed the PR (only the StructDelete lines are new in the diff). A is right to note it for a follow-up issue, not this PR.

Missed issues

None of significance. Spot-checked:

• OuterTransactionSignalSpec.cfc uses g = application.wo (unscoped → variables.g), which closures can access — no scope bug there.
• Commit message (fix(model): bypass nested cftransaction when migrator owns the outer (#2789)) passes commitlint: valid type, optional scope, subject under 100 chars, not ALL-CAPS. A correctly did not flag it.
• No cross-engine concerns beyond what A already flagged.

Verdict alignment

A's CHANGES_REQUESTED is consistent with the findings: two genuine bugs in test code (closure scope defeat, application scope pollution) that could cause silent mis-skips on Adobe CF and inter-test pollution.

Convergence

Aligned. A's two blocking findings are correct and actionable: (1) replace var _isCockroachDB with a struct (var ctx = {isCockroachDB: ...}) and update the four if (ctx.isCockroachDB) return; guards; (2) change application.$issue2789FlagDuringUp → request.$issue2789FlagDuringUp in both the migration fixture and the spec assertion, and update beforeEach/afterEach cleanup from StructDelete(application, ...) to StructDelete(request, ...). Trimming the multi-line comment blocks to one-liners each should also be part of the address-review pass. Joint recommendation: apply those changes, then re-review.

[wheels-bot]

Wheels Bot — Address Review (round 1)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):

• Closure scope (anti-pattern Fixed bug in $findRoute() that causes blow up on unmatched named route #10) — Replaced var _isCockroachDB in vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc:17 with var ctx = {isCockroachDB: ...} and updated the four it() closure guards (lines 50, 61, 67, 73) from if (_isCockroachDB) to if (ctx.isCockroachDB). The previous form put the flag on local scope, invisible to closures on Adobe CF — the guard would silently no-op and the CockroachDB-only skip would defeat itself.

• application → request scope in fixture — Switched application.$issue2789FlagDuringUp to request.$issue2789FlagDuringUp in vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc:14 and the spec's assertion at MigratorOuterTransactionSpec.cfc:63. Updated the beforeEach/afterEach cleanups (lines 31, 45) to StructDelete(request, "$issue2789FlagDuringUp"). migrateTo("001") → migration.cfc.up() is a same-request call chain, so request scope is the right reach.

• Multi-line comment blocks → one-liners — Trimmed the three blocks in vendor/wheels/Migrator.cfc (down-path ~line 125, up-path ~line 182, migrateIndividual ~line 259) and the 9-line block in vendor/wheels/model/transactions.cfc:39 to one short line each, preserving the issue number. Per CLAUDE.md "one short line max."

Skipped (intentionally, per consensus scope):

• migrateIndividual() commit-after-rollback — Reviewer A explicitly labelled this pre-existing and recommended a follow-up issue, not action on this PR. Reviewer B confirmed.
• variables.migration set in beforeAll() but referenced only via included helpers — Reviewer A's nit, not in the consensus blocking list. Reviewer B did not echo it.

Test verification caveat: the local test runner bash tools/test-local.sh failed before reaching the specs due to a pre-existing infrastructure issue (BSD sed -i '' syntax in tools/test-local.sh:54 doesn't work on Linux runners; combined with a lucee.json {project} placeholder that was already modified in the workspace at session start). This is unrelated to the consensus changes. The changes applied here are: comment-only trims in Migrator.cfc and transactions.cfc (zero behavior delta), a closure-scope refactor in the spec (pure structural, no semantic change), and an application → request scope move in the fixture (CLAUDE.md anti-pattern guidance). The PR author's original bash tools/test-local.sh (3780 passed) validated the underlying fix. CI's cross-engine matrix will run the test suite on every supported engine × DB.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR — The round-1 fixup commit (93db8bde) correctly resolves all three findings from the prior Reviewer A review: the var _isCockroachDB closure-scope bug is fixed with the documented struct-capture pattern (var ctx = {isCockroachDB: ...}), the application.$issue2789FlagDuringUp global-state hazard is correctly moved to request scope, and multi-line comment blocks are trimmed to one-liners per CLAUDE.md. No new correctness, cross-engine, or security issues are introduced by the round-1 changes. Commenting with two minor nits; no blocking findings.

---

Correctness

Pre-existing migrateIndividual() commit-after-rollback (unchanged from round 1 — noted for a follow-up issue, not a blocker on this PR)

vendor/wheels/Migrator.cfc lines ~237–248 (post-patch):

} catch (any e) {
    local.rv = local.rv & "Error migrating #local.migration.version#...";
    transaction action="rollback";
    StructDelete(request, "$wheelsTransactionWrapper");   // new, correct
}
StructDelete(request, "$wheelsTransactionWrapper");        // success path, correct
transaction action="commit";                              // always runs — fires after rollback on error path

Unlike the migrateTo() loops, which break after the catch, migrateIndividual() has no enclosing loop to break out of, so transaction action="commit" always executes even after the error path issued transaction action="rollback". This is pre-existing behaviour that predates this PR — the new StructDelete placement is correct in both paths. Worth a dedicated follow-up issue rather than holding this PR.

---

Tests

variables.migration set in beforeAll() but never referenced by this spec's assertions

vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc, line 6:

function beforeAll() {
    migration = CreateObject("component", "wheels.migrator.Migration").init();

migration is correctly declared without var (so it lands in variables scope where closures can reach it), but none of this spec's describe/it blocks reference migration directly. It exists because helperFunctions.cfm uses migration.adapter.adapterName() in its engine-detection helpers (isDbCompatibleFor_SQLServer, etc.) — helpers that MigratorOuterTransactionSpec itself never calls. A one-liner would spare the next reader from wondering why it's there:

// Required by helperFunctions.cfm engine-detection helpers; migrator.paths.sql used by $cleanSqlDirectory.
migration = CreateObject("component", "wheels.migrator.Migration").init();

---

Commits

fix(model): address Reviewer A/B consensus findings (round 1) — type fix, scope model (slightly broader than the actual touch-set, which includes migrator/ and tests, but scope is optional and unrestricted per commitlint.config.js), subject < 100 chars, not ALL-CAPS, DCO Signed-off-by trailer present. Passes commitlint.

---

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's round-2 review is largely accurate: both observations are real and well-cited, and neither constitutes a blocker. However, A missed a class of multi-line comment violations in the test spec and migration fixture that A itself flagged in production code during round 1. The address-review bot fixed Migrator.cfc and transactions.cfc but left the same pattern standing in the new test files. Not aligned this round.

Sycophancy

None detected. A uses COMMENTED (not a premature approve), cites specific file/line locations, and withholds approval while the round-1 fixes are confirmed — correct posture.

False positives

None detected. Both observations verified against the diff:

• Pre-existing commit-after-rollback in migrateIndividual() — confirmed. transaction action="commit" at ~line 248 executes unconditionally in both the success and error paths; the error path already issued transaction action="rollback". The new StructDelete lines are in the correct positions; the commit-after-rollback predates this PR. A's recommendation (follow-up issue, not a blocker here) is correct.

• variables.migration unexplained in beforeAll() — the observation is accurate. migration is initialized without var so it lands in variables scope where helperFunctions.cfm engine-detection helpers (migration.adapter.adapterName()) can reach it, but none of the spec's own describe/it blocks reference it directly. A's suggested one-liner is reasonable (though very minor).

Missed issues

Multi-line comment blocks in OuterTransactionSignalSpec.cfc and 001_seed_tag_via_model_create.cfc

CLAUDE.md rule: "Never write multi-line comment blocks — one short line max." A correctly flagged this in Migrator.cfc (three 5–9 line blocks) and transactions.cfc (one 9-line block) in round 1, and the address-review trimmed those. The same violation persists in the new test files and was not flagged then or now.

Specific instances (cumulative diff, new files only):

• vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc — "treats invokeWithTransaction as 'alreadyopen'" it() block has a 9-line comment starting "With the outer-transaction signal set…"; "also bypasses the wrapper for update via save()" has a 3-line block; "control:" has a 2-line block; "also bypasses the wrapper for deleteAll" has a 2-line block. Each should be trimmed to one short line.

• vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc, down() — 2-line comment "removeRecord uses raw queryExecute (no nested transaction), safe for / every adapter regardless of whether the fix is in place." One line max.

These were in the original submission. A flagged the production-code equivalent, B confirmed, and the address-review fixed those — but not the test-file equivalents. A reviewing the new SHA should have caught the same pattern it flagged in round 1.

Verdict alignment

A's COMMENTED verdict is internally consistent with "no blocking findings, two minor observations." The verdict is not wrong, though APPROVED would also be defensible given both observations are cosmetic or pre-existing.

Convergence

Not aligned. A missed the multi-line comment violations in OuterTransactionSignalSpec.cfc and 001_seed_tag_via_model_create.cfc — the same class of finding A correctly identified in production code during round 1. A should either (a) flag these as a blocking style fix and emit CHANGES_REQUESTED, or (b) explicitly argue that the "one short line max" rule applies differently to test-spec comment blocks and provide a rationale — in which case B can reconsider. Once that's resolved, convergence on approve appears reachable given the core fix is sound.

[wheels-bot]

Wheels Bot — Reviewer A response (round 1)

B's critique is correct on the missed finding and wrong on nothing. I concede the multi-line comment violations in the two test files — B caught a genuine inconsistency in my round-2 review: I flagged and got fixed the exact same pattern in Migrator.cfc and transactions.cfc during round 1, but failed to check the new test files for the same violation when reviewing the round-1 fixup commit. The core fix remains sound; the only remaining work is style-only trimming of comment blocks in the spec and migration fixture.

Conceded points

• Multi-line comment blocks in OuterTransactionSignalSpec.cfc — B is right. Four violations confirmed in the diff: (1) a 10-line block starting // With the outer-transaction signal set… inside the "treats invokeWithTransaction as 'alreadyopen'" it() block; (2) a 2-line block (// Establishes the baseline…) in the "control:" case; (3) a 3-line block (// Same semantics for the update path…) in the save() case; (4) a 3-line block (// Same semantics for deleteAll…) in the deleteAll case. CLAUDE.md: "Never write multi-line comment blocks — one short line max." The 10-line block in particular re-describes what the surrounding code does, which CLAUDE.md explicitly prohibits. Each should compress to one line, e.g. // Issue #2789: signal set → model bypasses its own wrapper; INSERT persists despite transaction='rollback'.

• Multi-line comment block in 001_seed_tag_via_model_create.cfc down() — The 2-line block (// removeRecord uses raw queryExecute (no nested transaction), safe for / every adapter regardless of whether the fix is in place.) is the same violation. One line: // Issue #2789: raw queryExecute avoids nested transaction; safe across all adapters.

• Missed-issue consistency — B correctly notes I should have caught these when reviewing the new SHA, having flagged the identical pattern in production code in round 1. I did not re-scan the new test files for the same class. Conceded.

Defended points

• migrateIndividual() commit-after-rollback — B confirmed this is pre-existing and agreed it should be a follow-up issue. Position unchanged.

• variables.migration in beforeAll() nit — B confirmed the observation is accurate and the suggested one-liner is reasonable. Position unchanged; this was correctly labelled a nit, not a blocker.

Updated verdict

Updating to request-changes: the five multi-line comment blocks in OuterTransactionSignalSpec.cfc and 001_seed_tag_via_model_create.cfc need to be trimmed to one-liners each — identical to what the round-1 address-review applied to production code — before this merges. Once done, convergence on approve is straightforward.

[bpamiri]

Wheels Bot — Address Review (round 2)

Applied the consensus finding Reviewer B caught in round 1 that the round-1 address-review missed:

• Multi-line comment blocks in test files trimmed to one-liners. CLAUDE.md: "Never write multi-line comment blocks — one short line max." Reviewer A flagged the identical pattern in production code in round 1 and the address-review trimmed Migrator.cfc / transactions.cfc, but the same violation persisted in the new test files. Reviewer B's round-1 review caught the inconsistency; round-2 fixes it.
  • vendor/wheels/tests/specs/model/OuterTransactionSignalSpec.cfc:32-39, 41-60, 62-82, 84-97 — trimmed the 9-line block in treats invokeWithTransaction as 'alreadyopen', the 2-line block in control:, the 3-line block in the save() case, and the 3-line block in the deleteAll case to one-liners each, anchored on issue Migrator: model().create()/.update()/.deleteAll() inside up()/down() silently rolls back the row (nested-transaction conflict with the migrator's outer transaction) #2789.
  • vendor/wheels/tests/_assets/migrator/migrations_2789/001_seed_tag_via_model_create.cfc:14-17 — trimmed the 2-line block in down() to one line.

Also merged origin/develop to resolve the dirty merge state (PR #2808 / commit c21efb8cd had added an adjacent ### Fixed entry to CHANGELOG.md; kept both entries).

Skipped (intentionally, per consensus scope):

• migrateIndividual() commit-after-rollback — Reviewer A and Reviewer B both labelled this pre-existing in round 1 and explicitly recommended a follow-up issue, not action on this PR. The new StructDelete placement is correct in both paths.
• variables.migration set in beforeAll() but referenced only via included helpers — both reviewers labelled this a nit (Reviewer A's words), not a blocker. The bare-scope assignment is correct (closures inherit variables scope on every supported engine); the suggested explanatory one-liner is a future-readability nicety that can land separately.

Test verification:

• bash tools/test-local.sh model → 843 passed (Lucee 7 + SQLite)
• bash tools/test-local.sh migrator → 238 passed (Lucee 7 + SQLite)

Pure comment-trim changes with zero behavior delta. The cross-engine matrix will validate the broader fix on every supported engine × DB.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

[bpamiri]

Follow-up to the round-2 address-review: while in the spec, also picked up the two outstanding round-1 nits both reviewers had labelled non-blocking.

• variables.migration explanatory comment — Reviewer A's nit (B confirmed reasonable, both labelled it not-blocker). Added a one-liner above the bare assignment in vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc:6 naming helperFunctions.cfm's engine-detection helpers as the consumer. Also tightened the adjacent migrator-init two-liner to one line per CLAUDE.md.

• migrateIndividual() commit-after-rollback — both reviewers explicitly recommended a dedicated follow-up issue rather than expanding fix(model): bypass nested cftransaction when migrator owns the outer (#2789) #2810's scope. Filed as Migrator: migrateIndividual() error path falls through to transaction commit after rollback #2811 with the full diagnosis from both reviews and a suggested fix pattern. The new StructDelete placement from fix(model): bypass nested cftransaction when migrator owns the outer (#2789) #2810 stays correct in both paths; the follow-up is purely the pre-existing flow-control gap that's now visible in this code path.

Cross-engine matrix validation (MSSQL repro of the original #2789 scenario) still pending on CI for the new SHA a5c2c7f99.

Tests: bash tools/test-local.sh migrator → 238 passed (Lucee 7 + SQLite).

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR — All three round-1 blocking findings and all round-2 consensus findings are cleanly resolved in the two new commits (5c6d81bd and a5c2c7f9). The core fix (Migrator sets request.$wheelsTransactionWrapper around every up()/down() call; invokeWithTransaction reads it and switches to alreadyopen mode) is engine-agnostic and structurally sound. No new correctness, cross-engine, or security issues are introduced. Approving.

---

What the round-2 commits fix

fix(test): address Reviewer A/B consensus findings (round 2) (5c6d81bd) trims the five multi-line comment blocks that Reviewer B caught in the new test files — identical to what the round-1 address-review applied to production code. All four comment blocks in OuterTransactionSignalSpec.cfc (the 9-line block in the alreadyopen case, the 2-line baseline block, and the two 3-line update/deleteAll blocks) and the 2-line block in 001_seed_tag_via_model_create.cfc down() are now one-liners. CLAUDE.md: «Never write multi-line comment blocks — one short line max.» ✓

docs(test): explain why variables.migration is initialized in beforeAll (a5c2c7f9) adds the explanatory comment to MigratorOuterTransactionSpec.cfc's beforeAll() that Reviewer A and B both flagged as a nit in round 2. Also notes the follow-up issue (#2811) for the pre-existing migrateIndividual() commit-after-rollback that both reviewers agreed is out of scope for this PR. DCO Signed-off-by present on both commits. ✓

---

Correctness

The core logic is unchanged from rounds 1 and 2; no new behaviour delta in these commits. One nit (not a blocker):

Duplicate Migration instantiation in run()

vendor/wheels/tests/specs/migrator/MigratorOuterTransactionSpec.cfc, line 17:

var ctx = {isCockroachDB: CreateObject("component", "wheels.migrator.Migration").init().adapter.adapterName() == "CockroachDB"};

variables.migration is already set in beforeAll() and accessible from run(). This could read migration.adapter.adapterName() directly instead of constructing a second instance. One extra instantiation on test setup — not a blocker, but worth a tidy-up pass if the spec is ever revised.

---

Commits

Both commits pass commitlint.config.js:

• docs(test): explain why variables.migration is initialized in beforeAll — type docs, scope test, subject ≤ 100 chars, not ALL-CAPS, DCO trailer present. ✓
• fix(test): address Reviewer A/B consensus findings (round 2) — type fix, scope test, subject ≤ 100 chars, not ALL-CAPS, DCO trailer present. ✓

---

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's round-3 APPROVED review is accurate and well-supported. All blocking findings from rounds 1 and 2 are verifiably resolved, the one remaining nit is correctly labelled non-blocking, and no new issues were introduced by the two new commits. Aligned on approve.

Sycophancy

None detected. A cites the two specific commits, names the exact file/line for the nit, and does not approve without evidence.

False positives

None detected. Verified all claims against the diff:

• "All round-1 and round-2 consensus findings resolved" — confirmed. var ctx = {isCockroachDB: ...} replaces the old var _isCockroachDB declaration (closure-scope fix); request.$issue2789FlagDuringUp and the matching beforeEach/afterEach cleanups use request scope throughout the fixture; all spec and fixture comment blocks are one-liners.
• Duplicate Migration instantiation nit (line 17 of MigratorOuterTransactionSpec.cfc) — confirmed real. variables.migration is set in beforeAll() and is accessible from run(); the second CreateObject(...).init() inside run() is redundant. A's characterisation is accurate.

Missed issues

One very minor observation A didn't raise, but it is not blocking:

• Cleanup leak in the save() it-block — the test renames the row to outerSig_signal_update_modified before saving, then adds a manual DELETE at the end of the it-block. If the expect(refetched.name) assertion throws, that manual DELETE is skipped and afterEach's name-list won't match the modified name, leaving a stale row. This is a test-robustness nit only; it does not affect correctness in the passing path and does not change the fix being tested. Not a blocker.

No cross-engine, security, or commit-convention issues were missed.

Verdict alignment

A's APPROVED is consistent with the state of the diff: all three round-1 blockers (closure scope, application→request scope, multi-line comment blocks in test files) are resolved; the one remaining finding is a style nit A correctly did not elevate to a blocker.

Convergence

Aligned on approve. The core fix is engine-agnostic, structurally sound, and correctly tested. The outstanding nit (duplicate Migration instantiation) and the cleanup-leak observation above are both below the bar for blocking a merge. Joint recommendation: approve and merge after CI (cross-engine matrix, MSSQL repro of #2789) confirms green.