Skip to content

fix(model): preserve database column case for auto-derived properties - #2852

Merged
bpamiri merged 4 commits into
developfrom
claude/distracted-allen-4a4c79
Jun 4, 2026
Merged

fix(model): preserve database column case for auto-derived properties#2852
bpamiri merged 4 commits into
developfrom
claude/distracted-allen-4a4c79

Conversation

@bpamiri

@bpamiri bpamiri commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Problem

Migrating from CFWheels 2.5 → Wheels 3/4, auto-derived model property names come back lowercased: an isHidden column surfaces as the property ishidden, breaking case-sensitive consumers of serialized model output (returnAs="structs", renderWith(), serializeJSON()). Reported on Slack; no prior issue. The same code worked in 2.5 on the same Lucee 7 + SQL Server, so it's a framework change — not a Lucee/driver change.

Root cause

When a model declares no property() mappings, Model.cfc auto-derives its properties from the database column metadata. Commit dbfbdda71 ("Oracle compatibility", 2025-07-15) replaced the raw column name with lCase(local.columns["column_name"][local.i]), force-lowercasing every auto-derived property name on every engine. Oracle reports unquoted identifiers in uppercase, so normalizing there made sense; applying it to all adapters regressed case preservation for SQL Server / MySQL / SQLite. Shipped in v3.0.0 (build +33) through the v4.0.x line.

Fix

Preserve the database's reported column casing by default; lowercase only on adapters whose database folds unquoted identifiers to a non-meaningful uppercase default.

  • New $lowerCaseColumnNames() capability on databaseAdapters/Base.cfc (default false), mirroring the existing $supportsAdvisoryLocks() pattern.
  • OracleModel and H2Model override it to true (both upper-fold unquoted identifiers).
  • Model.cfc consults it instead of unconditionally calling lCase().

Behavior by engine:

Engine Behavior
SQL Server, MySQL, SQLite preserve declared case (isHidden) — restores 2.5 behavior
PostgreSQL, CockroachDB DB folds to lowercase; reported name used as-is
Oracle, H2 DB folds to uppercase; lowercased (unchanged from today)

Models that explicitly declare property(name="isHidden", column="isHidden") were always unaffected and remain so.

Tests

Adds propertyCasePreservationSpec and a c_o_r_e_casepreservation fixture table with an undeclared mixed-case isHidden column. The assertion is adapter-aware (preserve-case engines expect isHidden; fold engines expect ishidden) and uses a case-sensitive ListFind so the regression can't hide behind case-insensitive lookups.

Verified locally on Lucee 7 across SQLite, H2, MySQL, and PostgreSQL — 0 failures, 0 errors on each (RED confirmed first: the spec fails pre-fix with actual [0] is not greater than [0]). Oracle + Adobe 2023/2025 + BoxLang + Lucee 6 run via the compat-matrix on this PR.

Pre-release stopgap for affected users

property(name="isHidden", column="isHidden") on the affected models restores casing today without waiting for the release.

🤖 Generated with Claude Code

When a model declares no property() mappings, Wheels derives its properties from database column metadata. A change in the 3.0 line (Model.cfc, aimed at normalizing Oracle's fixed-case identifiers) began calling lCase() on every derived property name unconditionally, so an `isHidden` column surfaced as the property `ishidden` on SQL Server, MySQL, SQLite, etc. — silently breaking case-sensitive consumers of serialized model output (returnAs="structs", renderWith(), serializeJSON()) for apps upgrading from CFWheels 2.x.

Property names now preserve the database's reported column casing, gated by a new $lowerCaseColumnNames() adapter capability (Base default false). OracleModel and H2Model override it to true because their databases fold unquoted identifiers to a non-meaningful uppercase default, so those engines keep the lowercased behavior they have today. Models that explicitly declare property(name=..., column=...) were always unaffected.

Adds propertyCasePreservationSpec with an undeclared mixed-case `isHidden` fixture column; the assertion is adapter-aware to match each engine's identifier folding. Verified locally on Lucee 7 across SQLite, H2, MySQL, and PostgreSQL (0 failures); Oracle and Adobe/BoxLang via CI.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@github-actions github-actions Bot added the docs label Jun 3, 2026

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

TL;DR — This PR correctly fixes a 3.0-era regression where Model.cfc unconditionally force-lowercased all auto-derived property names, breaking case-sensitive consumers (JSON serialization, returnAs=\"structs\") for apps migrating from CFWheels 2.x. The adapter-capability pattern mirrors the existing $supportsAdvisoryLocks() approach cleanly. The test is well-constructed and the CHANGELOG is thorough. Verdict: comment (minor docs gaps only — no correctness, cross-engine, or security issues).


Correctness

The fix is sound. Key points verified:

  • vendor/wheels/Model.cfc line 148 compares variables.wheels.class.mapping[local.key].value == local.property — CFML's == is case-insensitive for strings, so "isHidden" == "ishidden" is true. Existing property(name=..., column=...) declarations continue to resolve correctly regardless of the reported column casing.
  • StructKeyExists(local.processedColumns, local.columnName) (line 141) is also case-insensitive on CFML structs, so the duplicate-column guard still works.
  • aliasedPropertyList in vendor/wheels/model/sql.cfc is consumed via ListFindNoCase (lines 527, 545), so mixed-case property names don't break JOIN aliasing.

No off-by-one, null-deref, or race concerns.

Conventions

Clean. No mixed positional/named argument issues, no misuse of query-vs-array in views, no route or migration concerns.

Cross-engine

The $lowerCaseColumnNames() pattern mirrors $supportsAdvisoryLocks() at vendor/wheels/databaseAdapters/Base.cfc:582 — a well-established capability hook in this codebase.

One note worth calling out explicitly: the fix relies on CFML engines (Lucee, Adobe CF, BoxLang) all reporting the same column casing via cfdbinfo for a given underlying database. If Adobe CF's cfdbinfo normalizes column names to lowercase internally, the adapter flag wouldn't help and the test would fail on Adobe+MySQL expecting isHidden. The PR says Adobe 2023/2025 run via the compat-matrix — confirm those matrix results are green before merge. The local Lucee 7 verification is strong but Adobe CF's cfdbinfo behavior for mixed-case column identifiers is the one gap not locally verified.

Tests

vendor/wheels/tests/specs/model/propertyCasePreservationSpec.cfc — solid construction:

  • Uses ListFind (case-sensitive) for the assertion, so the bug is genuinely invisible to a ListFindNoCase check. Good explicit choice.
  • get("adapterName") adapter-detection follows the same pattern as vendor/wheels/tests/specs/model/propertiesSpec.cfc:551 and others.
  • Fixture table + model in tests/_assets/models/CasePreservation.cfc follow the established convention.
  • Table cleanup added to populate.cfm's drop list. ✓

One minor concern: the adapter-detection list is

// vendor/wheels/tests/specs/model/propertyCasePreservationSpec.cfc:33
var preservesCase = ListFindNoCase("SQLiteModel,MySQLModel,MicrosoftSQLServerModel", get("adapterName")) GT 0;

This hard-codes the three preserve-case adapters rather than deriving from $lowerCaseColumnNames(). If a future adapter is added that also preserves case, the list needs a manual update. Not a bug today, but consider whether calling g.model("CasePreservation").$getAdapter().$lowerCaseColumnNames() (or the equivalent) would make the test self-consistent with the production logic. Keeping both in sync is a maintenance hazard.

Docs

Two gaps worth filling before or shortly after merge:

  1. .ai/wheels/cross-engine-compatibility.md not updated. The new $lowerCaseColumnNames() capability and the per-engine identifier-folding behavior (preserve / lowercase-fold / uppercase-fold-then-lowercase) is exactly the kind of cross-engine nuance that belongs in that reference. Future maintainers adding a new database adapter need to know to check this flag.

  2. Secondary migration note missing. The CHANGELOG entry accurately describes the 2.x→3.x regression. There is no note for apps that adapted to the regression — i.e., apps that, after upgrading to Wheels 3.x/4.x, built on lowercased property names in JSON outputs. For those apps, applying this patch silently changes {"ishidden": 1} back to {"isHidden": 1}. One sentence covering this case ("if your app adapted to the lowercased output and serializes model data to JSON, review your consumers after applying this patch") would be a useful heads-up.

Commits

Both commits conform to commitlint:

  • fix(model): preserve database column case for auto-derived properties — valid type, ≤100 chars. ✓
  • docs(changelog): note property-case preservation fix (#2852) — valid type, ≤100 chars. ✓

Both carry Signed-off-by: Peter Amiri <peter@alurium.com> — DCO satisfied. ✓

Security

No SQL injection, XSS, CSRF, or secret-leakage concerns. The change operates entirely on metadata returned from cfdbinfo, not user input.


@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

A's review holds up. The correctness claims are verified against the live code, the findings are concrete, and the comment verdict (docs gaps only, no blocking correctness issues) is right for this diff. Converging on changes-needed to address the two actionable docs improvements before merge.

Sycophancy

None detected. A did not say LGTM or approve without justification. Each section cites specific lines or patterns.

False positives

None detected. Verified each claim against the actual repo state:

  • variables.wheels.class.mapping[local.key].value == local.property (Model.cfc, the mapping-resolution loop after the fix): confirmed present in HEAD. CFML == is case-insensitive for strings — the claim is accurate.
  • StructKeyExists(local.processedColumns, local.columnName) (also in Model.cfc): CFML struct keys are case-insensitive by default — duplicate-column guard still works regardless of casing. Accurate.
  • aliasedPropertyList consumed via ListFindNoCase in vendor/wheels/model/sql.cfc: confirmed — both uses at lines ~527 and ~545 are ListFindNoCase. Accurate.
  • $lowerCaseColumnNames() mirrors $supportsAdvisoryLocks(): confirmed — $supportsAdvisoryLocks() is at line 582, new method lands at line 599, same file, same access modifier and return shape. The pattern claim is accurate.

Missed issues

None significant. One minor observation A did not raise: the multi-paragraph docblock added to Base.$lowerCaseColumnNames() is longer than CLAUDE.md's "one short line max" guideline for comments. However, the guideline targets Claude's own code generation, the surrounding adapter methods use the same style (see $supportsAdvisoryLocks()), and the WHY here is genuinely non-obvious cross-engine behavior — so this is a style judgment call, not a clear miss.

Verdict alignment

A's comment verdict is consistent with the findings: code correctness is clean, cross-engine handling is sound, the two actionable items are docs improvements only. Appropriate.

Convergence

Aligned with A. The two docs gaps A identified are concrete and addressable now:

  1. .ai/wheels/cross-engine-compatibility.md needs a note on $lowerCaseColumnNames() and the per-engine identifier-folding behavior (preserve / lowercase-fold / uppercase-fold-then-lowercase). This file is what future maintainers and agents read when adding a new database adapter.
  2. CHANGELOG should add one sentence covering the reverse-migration case: apps that upgraded to 3.x/4.x and adapted to the force-lowercased property names (e.g., built JSON consumers expecting ishidden) will see that output change back to the original declared casing after applying this patch.

A's note about hard-coding the adapter list in the spec is a maintenance concern worth tracking but not a correctness issue today — no action needed before merge.

Joint recommendation: address the two docs gaps above, then the PR is ready to merge pending green compat-matrix results (especially Adobe+MySQL) as A noted.

- Document the $lowerCaseColumnNames() adapter capability and per-engine
  identifier-folding behavior (preserve / lowercase-fold / uppercase-fold)
  in .ai/wheels/cross-engine-compatibility.md, including a table and a
  "when adding a new adapter" note so future maintainers know to check
  the flag.
- Extend the CHANGELOG entry (#2852) with a reverse-migration heads-up
  for apps that adapted to the 3.x/4.x force-lowercased property names
  in their JSON consumers — those outputs revert to declared casing on
  SQL Server / MySQL / SQLite after this patch.

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Address Review (round 1)

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

  • .ai/wheels/cross-engine-compatibility.md — added a new "Auto-Derived Property Casing — $lowerCaseColumnNames() Adapter Capability" section under "Database-Specific Gotchas". It documents the Base default (preserve case), the per-engine identifier-folding table (SQL Server / MySQL / SQLite preserve; PostgreSQL / CockroachDB lowercase-fold at the DB; Oracle / H2 fold to UPPERCASE and the adapter overrides to true), shows the override sites in OracleModel/H2Model, and ends with a "when adding a new adapter, check this flag" note plus references to Model.cfc, Base.cfc:599, and the regression spec. This is the cross-engine gap Reviewer A flagged in their Docs point New master #1 and that Reviewer B explicitly converged on.

  • CHANGELOG.md — extended the existing fix(model): preserve database column case for auto-derived properties #2852 [Unreleased] entry with a "Reverse-migration heads-up" sentence covering apps that upgraded to 3.x/4.x and adapted to the lowercased property names in JSON consumers / view templates / client-side code: those outputs revert to the originally declared casing on SQL Server / MySQL / SQLite after this patch. This is Reviewer A's Docs point New master #2 and Reviewer B's converged item New master #2.

Skipped (intentionally — not in consensus):

  • Hard-coded adapter list in propertyCasePreservationSpec.cfc (A's note in the Tests section about possibly deriving preservesCase from $lowerCaseColumnNames() directly). Reviewer A explicitly framed this as a maintenance concern worth tracking but "not a bug today" and "no action needed before merge"; Reviewer B agreed by not raising it. Leaving the test as-is for the merge of this PR.

No code paths were touched, so no test layer was affected — bash tools/test-local.sh was not re-run for this round. Both changed files are under documentation paths only.

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 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

TL;DR — Round 1 docs commit (12eccdb4c) correctly addresses both documentation gaps flagged in the previous review: .ai/wheels/cross-engine-compatibility.md now has the $lowerCaseColumnNames() table and new-adapter guidance, and the CHANGELOG entry carries the reverse-migration heads-up for case-adapted consumers. No correctness, cross-engine, or security issues in the new commit. Two open convention nits from the original diff remain; one open test-maintainability concern also remains. Verdict: comment.


Resolved since round 1

Both docs gaps from the previous review are closed:

  • .ai/wheels/cross-engine-compatibility.md — new section with the per-engine identifier-folding table, code examples for all three adapters, new-adapter authoring guidance, and an explicit note that explicit property(name=..., column=...) declarations bypass the flag entirely. ✓
  • CHANGELOG.md — reverse-migration heads-up for apps that adapted to the 3.x/4.x force-lowercased output is present and specific enough to be actionable. ✓

The new $lowerCaseColumnNames() docstring in Base.cfc (vendor/wheels/databaseAdapters/Base.cfc:586–598) follows the established /** multi-line pattern already used by $supportsAdvisoryLocks() at lines 569–581. Consistent. ✓


Conventions

Multi-line comment block in Model.cfc — still present

vendor/wheels/Model.cfc lines 128–135 contain an 8-line // block comment inside executable code:

//
// Preserve the database's reported column casing for the derived
// property name (so SQL Server / MySQL / SQLite keep `isHidden`),
// except on adapters whose database folds unquoted identifiers to
// a non-meaningful UPPERCASE default (Oracle, H2), where we
// lowercase to keep property names sane. Prior to 4.0 this was an
// unconditional lCase() that silently lowercased every
// auto-derived property on every engine.
local.columnName = local.columns["column_name"][local.i];

CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The WHY here is genuinely non-obvious and worth a comment, but the format violates the limit. Condense to one line:

// preserve reported case; unconditional lCase() regressed non-Oracle engines in 3.0 (see $lowerCaseColumnNames)
local.columnName = local.columns["column_name"][local.i];

Multi-line comment blocks in propertyCasePreservationSpec.cfc — still present

vendor/wheels/tests/specs/model/propertyCasePreservationSpec.cfc:

  • Lines 7–26: 20-line /* ... */ block inside describe().
  • Lines 30–32: 3-line // block before the ListFindNoCase line.

Both violate the same rule. The it() description string ("preserves the database column case for undeclared properties") and the existing describe() string already communicate intent. A single-line comment at the key decision point — e.g. // case-sensitive; the bug is invisible to ListFindNoCase — is sufficient.


Tests

Adapter-name list still hard-coded — open from round 1

vendor/wheels/tests/specs/model/propertyCasePreservationSpec.cfc line 33:

var preservesCase = ListFindNoCase("SQLiteModel,MySQLModel,MicrosoftSQLServerModel", get("adapterName")) GT 0;

This was flagged in round 1 and is still unaddressed. If a new adapter is added that preserves case (or if an existing one is renamed), the test diverges from production logic silently — it would expect ishidden but get isHidden, producing a false failure. The production gate is $lowerCaseColumnNames(), and the test should derive from the same source:

var preservesCase = !g.model("CasePreservation").$getAdapter().$lowerCaseColumnNames();
var expected = preservesCase ? "isHidden" : "ishidden";

This keeps the spec self-consistent with the adapter capability contract and survives new-adapter additions without a manual update.


Commits

New commit 12eccdb4c:

  • Header docs: address Reviewer A/B consensus findings (round 1) — 56 chars, valid type, ≤ 100 chars. ✓
  • Signed-off-by: claude[bot] — DCO satisfied. ✓

Merge gate reminder

As noted in round 1: the Adobe CF 2023/2025 matrix runs are the critical validation path. The PR body says they run via the compat-matrix; confirm those results are green before merging. The Lucee 7 + SQLite/H2/MySQL/PostgreSQL coverage is strong, but Adobe CF's cfdbinfo column-name reporting is the one behavior not locally verifiable.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

The round-2 review from A correctly closes the docs gaps from round 1. Both remaining convention findings rest on a misattributed rule, and the one test-maintainability concern was explicitly excluded from round-1 consensus. Stripping those leaves no actionable findings. Converging on approve.

Sycophancy

None detected. The COMMENTED state and per-finding justifications are appropriate.

False positives

Comment-style rule misattributed to CLAUDE.md (affects two findings)

A cites CLAUDE.md for the rule: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." That rule is not in the project CLAUDE.md. Searching the file produces no match for "multi-paragraph", "multi-line comment", or "one short line" — the phrase lives in the Claude AI system-prompt instructions, not in the checked-in project convention file. This matters because:

  • Pre-existing adapter code — $supportsAdvisoryLocks() at vendor/wheels/databaseAdapters/Base.cfc:569-581, present before this PR — uses the exact same /** ... */ multi-line docblock style that A objects to in the new $lowerCaseColumnNames() blocks. Applying the rule selectively to PR-added code while the surrounding file uses the same pattern is inconsistent.
  • The round-1 note from B already flagged this: "the guideline targets Claude code generation, the surrounding adapter methods use the same style." A re-raised it as a concrete finding rather than a minor observation, and still cites the wrong source.

Both the Model.cfc 8-line // block finding and the propertyCasePreservationSpec.cfc comment block finding are false positives on the same misattribution. Neither represents a real project-convention violation.

Missed issues

Suggested adapter-based fix references a non-existent API

A proposes replacing the hard-coded SQLiteModel,MySQLModel,MicrosoftSQLServerModel list with a call to $getAdapter().$lowerCaseColumnNames(). But $getAdapter() does not exist anywhere in vendor/wheels/. The adapter is an internal concern accessed via variables.wheels.class.adapter within model internals — there is no public or internal $getAdapter() method. The proposed replacement would throw at runtime. This weakens the force of the suggestion but does not change the round-1 determination (not a correctness issue, no action needed before merge); it does mean the concern is carrying an unreliable concrete example.

Verdict alignment

The COMMENTED verdict (non-blocking) is technically consistent with having no correctness issues, but — given that both convention findings are false positives and the adapter-list concern was explicitly excluded from round-1 consensus — the review should have moved to APPROVE. The non-approval follows directly from the misattributed rule.

Convergence

Aligned. With the false-positive comment-style findings stripped and the already-excluded adapter-list concern not re-opened, there are no remaining actionable findings. The PR is correctness-clean, the cross-engine handling is sound, the docs are complete. The compat-matrix (Adobe CF 2023/2025) is the only remaining gate — a CI concern, not a code concern. Joint recommendation: approve and let the compat-matrix results speak.

Address Reviewer A's comment-length nits: collapse the 8-line block in Model.cfc and the multi-line blocks in propertyCasePreservationSpec to single lines (CLAUDE.md one-line-comment guidance). Base.$lowerCaseColumnNames() keeps its docstring — it matches the existing $supportsAdvisoryLocks() convention, which both reviewers endorsed. The spec's adapter-name list is kept (deriving it from $lowerCaseColumnNames() needs a public model->adapter accessor that doesn't exist — getClass() isn't a model method and there is no $getAdapter()); reviewers flagged this as a non-blocking maintenance note.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri

bpamiri commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed Reviewer A/B (round 2) — 27fddae53

No inline review threads exist on this PR (the reviews are top-level), so summarizing here.

  • Condensed the inline comment blocks Reviewer A flagged — Model.cfc (8-line // block → 1 line) and propertyCasePreservationSpec.cfc (the /* */ + // blocks → single lines), per the one-line-comment guidance.
  • Kept Base.$lowerCaseColumnNames()'s docstring — it matches the existing $supportsAdvisoryLocks() /** */ convention, which both reviewers endorsed (Reviewer B explicitly called it a judgment call).
  • Spec adapter list kept as-is. I evaluated deriving preservesCase from $lowerCaseColumnNames() directly (Reviewer A's suggestion), but there's no public model→adapter accessor in the spec context: $getAdapter() doesn't exist, and getClass() isn't a model method (it errors The method getClass was not found in the CasePreservation model). Both reviewers framed the hard-coded list as a non-blocking maintenance note, so it stays; a public accessor would be a reasonable separate follow-up.

Re-verified locally after the change: Lucee 7 × SQLite (3795/0/0) and H2 (3793/0/0), both green.

On the red Lucee 7 + SQLite (LuCLI) check

The failure was the CLI module suite returning HTTP 500 after the app-reload phase — the core suite passed 3795 / 0 / 0 ([Core Tests] All tests passed!). It passed on the code commit (1bdda1113) and failed only on the docs-only commit (12eccdb4c), which can't change test behavior — so this is a flaky/pre-existing CLI-suite failure, not a regression from this change. The push above re-triggers the check.

🤖 Generated with Claude Code

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test body for auth check

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placeholder to test

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot --- Reviewer A (Round 3)

TL;DR -- Round 2 style commit (27fddae) correctly addresses the two comment-length violations flagged in the previous review: the 8-line block in Model.cfc is condensed to a single meaningful line, and the 20-line block plus multi-line inline comments in propertyCasePreservationSpec.cfc are condensed to single-line annotations. No correctness, cross-engine, or security concerns in this commit. The adapter-name list maintenance note from round 2 remains acknowledged but technically unresolvable without a public model->adapter accessor (see commit message justification). Verdict: comment -- PR is clear for merge once the compat-matrix results for Adobe CF 2023/2025 are confirmed green.


Resolved since round 2

vendor/wheels/Model.cfc line 128 is now a single-line comment explaining the WHY (a cross-engine regression with a prior-art reference), which earns its place under CLAUDE.md single-line-comment rule.

vendor/wheels/tests/specs/model/propertyCasePreservationSpec.cfc -- the 20-line block and the multi-line inline annotations are now four single-line comments, each explaining a non-obvious decision (fixture source, adapter folding, case-sensitivity rationale).

vendor/wheels/databaseAdapters/Base.cfc -- the $lowerCaseColumnNames() docstring is unchanged, consistent with both reviewers endorsement of the $supportsAdvisoryLocks() pattern.

Conventions -- Clean in this commit. No new multi-line blocks introduced.

Commits -- New commit 27fddae: header style(model): condense review-flagged inline comments is valid type, <= 100 chars, not ALL-CAPS. Signed-off-by present -- DCO satisfied. Commit message explains the adapter-list decision and the docstring exception.


Remaining open item (non-blocking, from round 2)

propertyCasePreservationSpec.cfc line 13 still hard-codes the adapter-name list (SQLiteModel,MySQLModel,MicrosoftSQLServerModel) rather than deriving from $lowerCaseColumnNames(). The commit message gives the right technical explanation: there is no public model->adapter accessor (getClass() is not a model method; $getAdapter() does not exist), so deriving from production logic would require adding a new accessor -- a scope change not warranted by this bug fix. This was marked non-blocking in round 2 and remains so. If a new adapter is added that overrides $lowerCaseColumnNames(), this list will need a manual update; that risk is understood.


Merge gate reminder -- As noted in rounds 1 and 2: confirm the Adobe CF 2023/2025 compat-matrix runs are green before merging. Lucee 7 + SQLite/H2/MySQL/PostgreSQL coverage is strong; Adobe CF cfdbinfo column-name reporting for mixed-case identifiers is the one behavior not locally verifiable.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

A's review filed on this SHA (27fddae53) is a placeholder stub — body reads "placeholder to test" with no inline comments or findings. There is nothing substantive to critique.

The commit itself (27fddae53) is a pure style cleanup: condenses the 8-line // block in Model.cfc and the multi-line /* */ / // blocks in propertyCasePreservationSpec.cfc to single lines, exactly as A requested in round 2. No logic was changed — the adapter capability, the Oracle/H2 overrides, the fixture table, and the populate script are all untouched. This is identical in substance to the SHA on which B previously emitted converged-approve (12eccdb4c).

Sycophancy

Not applicable — A made no findings.

False positives

Not applicable — A made no claims.

Missed issues

None. The style-only commit introduces no new correctness, cross-engine, or test concerns. Diff-confirmed: only vendor/wheels/Model.cfc (comment condensed, logic unchanged) and vendor/wheels/tests/specs/model/propertyCasePreservationSpec.cfc (comment blocks condensed) were touched.

Verdict alignment

A filed a COMMENTED state with no content. Given no findings, the appropriate verdict would have been APPROVE. The non-approval is a no-op in practice — nothing to act on.

Convergence

Aligned on approve. The PR is correctness-clean, cross-engine handling is sound, docs are complete (.ai/wheels/cross-engine-compatibility.md and CHANGELOG both updated in the prior address-review round), and the round-2 style nits have been addressed. The only remaining gate is the compat-matrix (Adobe CF 2023/2025, Oracle, BoxLang) — a CI concern, not a code concern.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

Review 4423209318 is a test/auth placeholder (body: "test body for auth check", zero line comments) — there is no substantive Reviewer A review on this SHA to critique. The previous cycle closed with converged-approve on SHA 12eccdb4c; the human's subsequent commit (27fddae53) makes only the style changes Reviewer A requested in round 2. I verified the diff directly.

Sycophancy

N/A — the submitted review is a test placeholder with no content.

False positives

N/A — no claims were made.

Missed issues

None in the new commit. The 27fddae53 diff touches only Model.cfc and propertyCasePreservationSpec.cfc:

  • Model.cfc lines 128–135: 8-line // block condensed to one line. ✓
  • propertyCasePreservationSpec.cfc: 20-line /* */ block and 3-line // block condensed to single-line comments. ✓
  • Base.$lowerCaseColumnNames() docblock retained — consistent with $supportsAdvisoryLocks() convention, endorsed by both reviewers in round 2. ✓
  • Commit style(model): condense review-flagged inline comments (#2852) — valid type, ≤100 chars, Signed-off-by present. ✓
  • No logic changes; no new cross-engine risks.

Verdict alignment

N/A — no verdict was issued by the placeholder review.

Convergence

Aligned. The prior convergence on 12eccdb4c established the substantive verdict (approve); the only open items were Reviewer A's comment-length nits, which 27fddae53 correctly addresses. No correctness, cross-engine, security, or docs issues remain. The compat-matrix (Adobe CF 2023/2025) is the sole remaining gate.

@wheels-bot

wheels-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Wheels Bot — Reviewer B (round 1)

A's round-3 review correctly characterizes the style commit as clean. No sycophancy, no false positives, no missed issues. Minor verdict note below, but converging on approve.

Sycophancy

None detected.

False positives

None detected. Verified A's claims against the diff:

  • Model.cfc inline comment condensed to one line: diff confirms a single // line replaces the prior block. Accurate.
  • propertyCasePreservationSpec.cfc comment blocks condensed: diff confirms four single-line annotations, each explaining a non-obvious decision. Accurate.
  • Base.$lowerCaseColumnNames() docblock unchanged: diff confirms, consistent with both reviewers' endorsement of the $supportsAdvisoryLocks() pattern in prior rounds. Accurate.
  • Commit conventions: header style(model): condense review-flagged inline comments is 51 chars, valid type, not ALL-CAPS. DCO trailer present on a human-authored commit. Accurate.

Missed issues

None detected.

Verdict alignment

A says "PR is clear for merge once the compat-matrix results for Adobe CF 2023/2025 are confirmed green" but uses COMMENTED rather than APPROVE. Both reviewers converged on approve for the substantive content on SHA 12eccdb4c, and this commit contains only the style condensations both reviewers explicitly requested. The compat-matrix is a CI gate, not a pending code-review concern. The verdict should be APPROVE; COMMENTED is a minor misalignment between A's stated position and their formal review state.

Convergence

Aligned. The style commit is correctness-clean and contains exactly the changes both reviewers requested. No new code paths, no new cross-engine concerns, no new docs gaps. The compat-matrix (Adobe CF 2023/2025) is the one remaining gate and is a CI concern, not a code-review concern. Joint recommendation: approve. The PR is ready to merge once compat-matrix results are confirmed green.

@bpamiri
bpamiri merged commit 2ed5568 into develop Jun 4, 2026
8 checks passed
@bpamiri
bpamiri deleted the claude/distracted-allen-4a4c79 branch June 4, 2026 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant