[bpamiri]

Summary

Identity retrieval after INSERT could return the wrong primary key on two adapters, silently corrupting subsequent update()/delete() calls made through the freshly created object:

• Oracle (vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc:146-149 pre-fix): resolved the new PK via WHERE ROWID = (SELECT MAX(ROWID) FROM <table>). ROWID encodes physical block location, not insertion order, so a concurrent committed insert (or block reuse) returns another session's row.
• SQL Server (vendor/wheels/databaseAdapters/MicrosoftSQLServer/MicrosoftSQLServerModel.cfc:288-295 pre-fix): resolved it via a standalone SELECT @@IDENTITY, which is session-scoped across all scopes — an audit/history trigger that inserts into another identity table makes it return the trigger's key (non-empty, so the existing SCOPE_IDENTITY empty-fallback never fired).

Source

Internal multi-agent framework review, 2026-06-09 — finding T10 (identity-retrieval-races), report "Top 10 #10" plus DA8/DA9.

Fix

Both adapters now prefer the driver-supplied generated key from the insert's own result struct, mirroring the proven CockroachDB adapter:

• MSSQL: ListFirst(result.generatedKey) when present — mssql-jdbc retrieves it in the insert's own batch, so it is scope-, trigger-, and concurrency-safe. The @@IDENTITY -> SCOPE_IDENTITY chain survives only as a documented last resort for engines that surface no generated key (currently BoxLang).
• Oracle: resolves result.generatedKey (Lucee) / result.rowid (ACF). A numeric value is used directly; a ROWID-shaped value is resolved with a race-free exact-row CHARTOROWID lookup, gated by a strict case-sensitive ^[A-Za-z0-9/+]{18}$ pattern (quote-free alphabet — $query has no parameter binding, so nothing unvalidated is ever interpolated). MAX(ROWID) survives only as a documented BoxLang fallback.
• Both adapters also gain the BoxLang ReplaceList column-parse workaround already used by the PostgreSQL/CockroachDB adapters, so the fallback path cannot falsely match the PK and skip key retrieval.

Deliberate deviation from the finding's recommendation: the suggested "default to SCOPE_IDENTITY()" reorder was NOT implemented. A standalone SELECT SCOPE_IDENTITY() over JDBC executes in its own batch (a batch is a scope per MS docs) and returns NULL — the documented BoxLang empty-value behavior — so the reorder would add a wasted round-trip per insert and still resolve via @@IDENTITY. The rationale is documented in the adapter comments.

Deferred scope — tracked in #2906: same-batch retrieval for the MSSQL BoxLang fallback, replacing Oracle's MAX(ROWID) last-resort with CURRVAL, and the DA3 six-way $identitySelect dedup ($parseInsertColumnList template + $lastIdLookup hook).

Contract unchanged for the sole caller (Base.cfc::$querySetup -> create.cfm); ACF on MSSQL is byte-identical via the untouched early identitycol return.

Tests

New DB-free unit specs, modeled on the matrix-proven CockroachDBUnitSpec:

• vendor/wheels/tests/specs/database/MicrosoftSQLServerUnitSpec.cfc
• vendor/wheels/tests/specs/database/OracleUnitSpec.cfc

They construct each adapter directly and drive $identitySelect with synthetic result structs (driver key present/absent, list-valued keys, PK-in-column-list, non-INSERT). Verified red against the pre-fix adapters via stash-and-rerun (4 pass / 2 errors per bundle on Lucee 7 + SQLite) and green with the fix; the database spec area passes 44 / 0 fail / 0 error locally.

Cross-engine notes

• Lucee: result.generatedKey (case-insensitive StructKeyExists matches the lowercase generatedkey key). ACF: untouched early identitycol return on MSSQL; result.rowid on Oracle. BoxLang: takes the guarded fallbacks, now with the ReplaceList column-parse workaround copied verbatim from PostgreSQLModel.cfc.
• Live Lucee+MSSQL and BoxLang+MSSQL legs were not run locally — please confirm the PR CI compat-matrix sqlserver legs are green before merging. Oracle is soft-fail in CI as usual; its changes ship guarded behind the strict ROWID regex with the legacy query as fallback.
• No new closures-as-constructor-args, no attributeCollection = arguments, all mixin-visible helpers remain public $-prefixed.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR correctly fixes a real data-integrity race: Oracle was resolving the new PK via MAX(ROWID) (physical location, not insertion order) and MSSQL was using @@IDENTITY (trigger-unsafe) instead of the driver-supplied generated key. The fix mirrors the CockroachDB pattern already in the codebase and is well-reasoned. The code is safe and the logic is correct. Two minor gaps worth addressing before merge: a missing DB-free test for the ACF result.rowid surface in OracleUnitSpec, and a missing CHANGELOG.md entry. Verdict: comment (no correctness, cross-engine, or security blocks).

---

Tests

vendor/wheels/tests/specs/database/OracleUnitSpec.cfc — missing test for ACF result.rowid path

The Oracle adapter checks two distinct driver-key surfaces:

if (StructKeyExists(arguments.result, "generatedKey") && Len(arguments.result.generatedKey)) {
    local.generated = ListFirst(arguments.result.generatedKey);
} else if (StructKeyExists(arguments.result, "rowid") && Len(arguments.result.rowid)) {
    local.generated = arguments.result.rowid;
}

The result.rowid branch (the Adobe CF surface) is not exercised by any spec. A numeric rowid value returns before ever calling $query(), so this case is entirely DB-free:

it("uses a numeric result.rowid directly (ACF surface)", () => {
    var result = {
        sql = "INSERT INTO users (firstname) VALUES ('test')",
        rowid = "42"
    };
    var rv = adapter.$identitySelect(
        queryAttributes = {},
        result = result,
        primaryKey = "id",
        returningIdentity = ""
    );
    expect(rv).toBeStruct();
    expect(rv).toHaveKey("lastId");
    expect(rv.lastId).toBe("42");
});

The PR explicitly calls out the ACF result.rowid path as part of the fix (PR body: "ACF surfaces it as result.rowid"), so omitting coverage of this branch is an inconsistency worth closing. The CockroachDBUnitSpec pattern the PR says it models does not have this gap.

---

Docs

CHANGELOG.md — no [Unreleased] entry

This commit lands after the 4.0.3 tag (2026-06-09). The CHANGELOG has no [Unreleased] section above the 4.0.3 entry. A silent data-integrity bug (wrong PK returned after INSERT under concurrent load or a trigger) warrants a ### Fixed bullet under [Unreleased] so that the next release note captures it. This is a nit but one that will cause confusion at release-note time.

---

Security

Oracle adapter: local.generated interpolation is correctly gated

For completeness: the ROWID interpolation in

sql = "SELECT #arguments.primaryKey# AS lastId FROM #local.tbl# WHERE ROWID = CHARTOROWID('#local.generated#')",

is safe. local.generated only reaches this point after

if (REFind("^[A-Za-z0-9/+]{18}$", local.generated) == 1) {

The regex correctly matches Oracle's extended-ROWID alphabet (base-64 over A-Z a-z 0-9 + /, 18 chars = 108 bits = the documented format). Anything outside that set — including UROWIDs, nulls, or unexpected driver output — falls through to the legacy MAX(ROWID) path without interpolation. No concern here; noting it explicitly because the PR body calls it out and it's worth confirming.

---

Correctness (no issues — contextual notes only)

• The endPar > 0 guard added around the column-list extraction is a quiet improvement over the pre-fix code: the original Mid(local.sql, startPar, endPar - startPar) with endPar == 0 would produce Mid(sql, N, -N) — a crash on Adobe CF (negative length). The guard makes both adapters consistent with the CockroachDB adapter.
• The "why SCOPE_IDENTITY() is not preferred" explanation in the comment block is the right call and is well-documented. The standalone-batch-scope behavior is subtle enough that future maintainers will thank the author.
• The StructKeyExists(server, "boxlang") column-parse branch matches the pattern already used verbatim in PostgreSQLModel.cfc and CockroachDBModel.cfc. Consistent; no concern.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is clean and well-reasoned. The missing result.rowid DB-free test is a real gap (A provided exact code to close it), the CHANGELOG nit is accurate, and the security analysis of the ROWID regex is verified against the diff. No sycophancy, no false positives, no missed issues. A's comment verdict with concrete actionable findings maps to aligned-changes-needed.

Sycophancy

None detected. A characterizes findings specifically, provides line-level evidence for every claim, and does not approve without basis.

False positives

None detected.

• ROWID regex claim: Verified in the diff. REFind("^[A-Za-z0-9/+]{18}$", local.generated) == 1 correctly gates before the CHARTOROWID interpolation. Oracle extended ROWID is base-64 over A-Za-z0-9+/, exactly 18 chars; that character class covers it.
• CockroachDB pattern claim: Confirmed — StructKeyExists(server, "boxlang") with the identical REReplace block is verbatim in CockroachDBModel.cfc. Not a novel pattern.
• endPar > 0 guard claim: Confirmed in the diff. The pre-fix code passed endPar - startPar directly to Mid(); with endPar == 0 that is a negative length, which crashes Adobe CF. The guard is a real improvement.

Missed issues

None detected. A caught the only substantive gap:

• OracleUnitSpec.cfc — missing result.rowid numeric path test: The fix adds two routes into local.generated — result.generatedKey (Lucee) and result.rowid (ACF). The spec exercises result.generatedKey with a numeric value and a list, but never sets only result.rowid. Because IsNumeric(local.generated) returns before any $query() call, this case is DB-free. A provided the exact code. This is the one actionable change.

• CHANGELOG.md: Confirmed no [Unreleased] section above the 4.0.3 entry. A correctly characterizes this as a nit, not a blocker.

Verdict alignment

A's comment verdict is consistent with the findings: the code is functionally correct (no correctness, cross-engine, or security blocks), and both items are addressable without changing the logic under test.

Convergence

Aligned. A's findings are accurate and the verdict is appropriate. The comment verdict carries concrete, actionable items (A even provided the exact test code for the result.rowid path), so this resolves as aligned-changes-needed: add the ACF result.rowid numeric test to OracleUnitSpec.cfc and add a [Unreleased] CHANGELOG entry capturing the data-integrity fix.

[wheels-bot]

Wheels Bot — Address Review (round 1)

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

• vendor/wheels/tests/specs/database/OracleUnitSpec.cfc:36-50 — Added a DB-free spec for the ACF result.rowid numeric path. Mirrors the existing result.generatedKey numeric-direct case structurally; only the stub result key differs (rowid vs generatedKey). A numeric rowid returns from $identitySelect before any $query() call, so the case stays fully DB-free per A's analysis.
• CHANGELOG.md:21-25 — Added a [Unreleased] section with a ### Fixed bullet describing the Oracle MAX(ROWID) race and SQL Server @@IDENTITY trigger-unsafety this PR closes, referencing the driver-supplied generated key reroute (result.generatedKey on Lucee, result.rowid on ACF), the strict ^[A-Za-z0-9/+]{18}$ regex gate on the Oracle ROWID interpolation, the SCOPE_IDENTITY() fallback on SQL Server, the endPar > 0 Adobe-CF crash guard, and the CockroachDB-pattern parity. PR #2908.

Tests not re-run locally: this is a human PR branch (peter/...), not fix/bot-*, so the strict address-review test-runner rail doesn't trigger. tools/test-local.sh also has a BSD-style sed -i '' invocation that fails on Linux runners (a pre-existing infra-layer issue, not introduced by this PR). The added spec is a structural clone of the adjacent passing generatedKey numeric-direct case, so a regression here would require breaking the existing case in the same file.

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: Round-1 addressed both findings from the previous review exactly as suggested: the DB-free spec for the ACF result.rowid numeric path is now in OracleUnitSpec.cfc, and the [Unreleased] CHANGELOG entry is present. The core logic — driver-key preference chain, ROWID regex gate, fallback documentation — remains correct and unchanged. No correctness, cross-engine, or security blockers. Two minor nits below. Verdict: comment.

---

Tests

Both findings from the prior round are resolved.

vendor/wheels/tests/specs/database/OracleUnitSpec.cfc:51-66 (round-1 addition) exercises the ACF result.rowid numeric path. A numeric rowid value returns before any $query() call, so this case is fully DB-free and will pass on every engine in the matrix. The CHARTOROWID/live-DB path is correctly left uncovered by unit specs. Coverage is now symmetric between the Lucee (generatedKey) and ACF (rowid) surfaces.

---

Docs

CHANGELOG.md now has an [Unreleased] section above 4.0.3 with a ### Fixed bullet. The horizontal-rule separator and header style match the rest of the file. ✓

Minor nit — entry length. The Fixed bullet is a single 350-word paragraph. Readers skimming the changelog at release time will struggle to absorb it. Consider splitting into 2-3 shorter bullets:

• Oracle: MAX(ROWID) race under concurrent inserts → CHARTOROWID exact-row lookup from driver-supplied ROWID.
• SQL Server: @@IDENTITY trigger-leak → driver-supplied generatedKey first; @@IDENTITY/SCOPE_IDENTITY chain only as BoxLang fallback.
• Added endPar > 0 guard fixing a latent ACF crash in both adapters (Mid(sql, N, -N) with a negative length).

Not blocking — the entry is accurate and present. It just reads better broken up.

---

Commits

Minor nit — round-1 commit subject. fix(model): address Reviewer A/B consensus findings (round 1) describes the process rather than the substance. CLAUDE.md convention: commit messages reflect the why, not the what. Since the repo squash-merges, the PR title is the landing commit and is fine; this second commit message matters less in practice. Still worth noting for future bot-authored fix commits: prefer substance over process in the subject line (e.g. "add OracleUnitSpec rowid ACF path and CHANGELOG entry").

Header length (58 chars) and DCO sign-off are both correct. ✓

---

Correctness / Cross-engine / Security (no new findings)

• The endPar > 0 guard is present in both adapters, eliminating the latent Adobe CF Mid(sql, N, -N) crash. ✓
• The BoxLang REReplace column-parse branch is an exact copy of the pattern already in PostgreSQLModel.cfc and CockroachDBModel.cfc. ✓
• The ROWID regex ^[A-Za-z0-9/+]{18}$ strictly gates the only interpolated value in the Oracle SQL; the quote-free base-64 alphabet means nothing in that 18-char window can break the string boundary. ✓
• StructKeyExists is case-insensitive, so Lucee's lowercase generatedkey result key correctly hits the generatedKey branch on both adapters. ✓

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate on all material points and the "comment" verdict is correct. One imprecision in the security reasoning worth noting; no false positives that change the verdict. No missed issues that would block the PR. Converging as approved.

Sycophancy

None detected. A verified concrete things (regex value, guard presence in both adapters, CHANGELOG format, test coverage symmetry) and cited specific lines throughout.

False positives

Minor imprecision — "only interpolated value" in the Oracle SQL. A states: "The ROWID regex strictly gates the only interpolated value in the Oracle SQL." Reading the diff at OracleModel.cfc line 156: the CHARTOROWID query interpolates three values (arguments.primaryKey, local.tbl, and local.generated), not one. A's security conclusion is still correct because primaryKey is model configuration and tbl is extracted from ORM-constructed INSERT SQL (neither is user input), while generated is the one originating from the JDBC driver result and correctly gated. The statement overstates the precision of the analysis but does not produce a wrong verdict.

Missed issues

None detected that would change the verdict. Two things I checked that A did not call out explicitly:

• endPar guard wording: A describes it as "endPar > 0 guard" but the code uses if (local.endPar) (truthiness, equivalent since Find() returns 0 on no-match in CFML). Not a defect.
• BoxLang column-parse branch: The StructKeyExists(server, "boxlang") detection is the same sniff pattern used elsewhere in the adapters. A did not call it out; not a blocking omission.

The MicrosoftSQLServerUnitSpec "void when result already contains identitycol" test correctly relies on the adapter returning "identitycol" as the generated key name for MSSQL, consistent with the separate $generatedKey describe block in the same file. A implicitly verified this through the test-coverage sign-off.

Verdict alignment

A's "comment" verdict is consistent with the findings: two non-blocking nits (changelog entry length, round-1 commit subject), no correctness or cross-engine blockers. Verdict is appropriate.

Convergence

Aligned. The correctness analysis is sound, cross-engine invariants are satisfied, the tests are appropriately scoped for a DB-dependent code path, and the two open nits are editorial rather than structural. Joint recommendation: this PR is safe to merge as-is; the changelog nit can be addressed pre-release or in a follow-up.

[bpamiri]

CHANGELOG entry deferred: with many parallel remediation PRs the per-branch [Unreleased] entries conflict on every sibling merge, so entries are being consolidated into a single changelog PR at the end of the campaign. The entry text is preserved verbatim by the merge captain.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: The single new commit on this round removes the [Unreleased] CHANGELOG entry that was added in response to the round-1 ### Docs finding. The rationale — conflict avoidance across a dozen parallel remediation PRs that all write to the same CHANGELOG line — is documented in the commit message and is operationally sound. The core adapter changes and unit specs are unchanged from round 2 and remain correct. No new correctness, cross-engine, or security concerns. Verdict: comment.

---

Docs

CHANGELOG.md — [Unreleased] entry intentionally removed

Round-1 requested a ### Fixed entry under [Unreleased]; round-2 confirmed its presence. The new commit removes it citing conflict avoidance across parallel remediation PRs that all converge on the same changelog insertion point, with the entry text preserved off-branch for a single consolidated changelog PR.

This is a sound operational call. One soft ask before merge: is the consolidation tracked in an issue or PR? The commit message says "The entry text is preserved off-branch" — confirming a concrete follow-up vehicle (even a sentence in the PR description pointing to an issue number) closes the documentation gap cleanly for future bisect readers. Not a blocker, but worth a line.

---

Commits

chore: defer changelog entry to campaign consolidation — clean. Header 53 chars, type chore is correct for a documentation bookkeeping change, subject describes the why (conflict avoidance during campaign), DCO signed by the PR author. ✓

The prior-round nit about fix(model): address Reviewer A/B consensus findings (round 1) describing process over substance stands for the record, but since the repo squash-merges, only the PR title lands as the commit — and the PR title (fix(model): prefer driver generated keys for Oracle and SQL Server identity retrieval, 73 chars) is correct and complete.

---

Correctness / Cross-engine / Security / Tests (no new findings — round-2 confirmations hold)

No new code changes to the adapter or spec files since round 2. All prior confirmations still apply:

• endPar > 0 guard present in both adapters, eliminating the latent Adobe CF Mid(sql, N, -N) crash. ✓
• ROWID regex ^[A-Za-z0-9/+]{18}$ strictly gates the only interpolated value in the Oracle SQL. ✓
• BoxLang REReplace column-parse branch is a verbatim copy of the pattern already in PostgreSQLModel.cfc and CockroachDBModel.cfc. ✓
• StructKeyExists case-insensitivity correctly handles Lucee's lowercase generatedkey result key on both adapters. ✓
• Unit specs cover the Lucee (generatedKey) and ACF (rowid) numeric paths, list-valued keys, PK-in-column-list, and non-INSERT statements on both adapters. ✓

[wheels-bot]

Wheels Bot Reviewer B round 1 - converged-approve. A review is accurate. No sycophancy: A does not blindly accept the consolidation rationale and correctly soft-asks for a tracking issue. No false positives: commit type chore is correct, DCO sign-off from PR author is valid, prior-round sign-offs carried forward appropriately as no code changed. No missed issues: I independently confirmed no open changelog consolidation PR exists, which supports A soft ask being the right instinct; the commit message provides forensic trail in git history so this is advisory not blocking. Verdict alignment: A comment verdict is consistent with no blockers and one non-blocking soft ask. Convergence: aligned. The substantive fix was approved on the prior SHA. The only new commit removes an editorial CHANGELOG entry with a documented rationale. No blockers remain. Joint recommendation: safe to merge.