fix(test): work around BoxLang catch-scope discard in lockingSpec#2754
Conversation
) `lockingSpec :: "releases lock even when callback throws an exception"` failed on BoxLang × MySQL / Postgres / SQLite — the missing leg of #2665 that #2670 intentionally deferred. The test tracked exception propagation through `local.exceptionThrown = true` inside the `catch` block; on BoxLang, writes to the `local` scope inside a catch don't survive past the block (the catch body runs under a nested `local` that gets discarded on exit), so the post-catch `expect(local.exceptionThrown)` read the un-touched outer value and failed with "Expected [false] to be true". Lucee 6 / 7 and Adobe CF 2023 / 2025 share the enclosing function's `local` across the catch, so the assignment stuck on every other engine and the gap was BoxLang-only. Switched to the same struct-field pattern TenantResolverSpec already uses for the equivalent assertion: var state = {exceptionThrown = false}; try { ... } catch (TestException e) { state.exceptionThrown = true; } expect(state.exceptionThrown).toBeTrue(); Struct field assignment targets a heap object and survives the scope transition on every engine. `vendor/wheels/model/locking.cfc` is unchanged — the lock-release contract was already correct via the existing `try { callback() } finally { release }`; the lock-release bug the issue title called out turned out not to exist. Documented the BoxLang catch-scope quirk in `.ai/wheels/cross-engine-compatibility.md` so future spec authors don't re-hit it. Both the `var struct.field` form and the `var bareName` form are called out as portable; the `local.X = ...` form is called out as the trap. Fixes #2744 Signed-off-by: Peter Amiri <peter@alurium.com>
bpamiri
force-pushed
the
peter/fix-2744-boxlang-locking-spec
branch
from
cdfe32c to
2ffb099
Compare
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly identifies the root cause (BoxLang-only catch-scope discard) and applies the right fix (struct-field state tracking). The production code (locking.cfc) is untouched and correctly uses try/finally for lock release. I'm marking this as a comment — the fix is sound, but there are three minor nits in the spec comment, the cross-engine doc, and CLAUDE.md coverage worth addressing before or shortly after merge.
Correctness
The fix is correct on all axes:
- The
var state = {exceptionThrown: false}; ... state.exceptionThrown = truepattern targets a heap-allocated struct, not thelocalscope chain, so it survives BoxLang's catch-scope discard. vendor/wheels/model/locking.cfclines 42–46 already usetry { callback() } finally { release }, so the lock-release contract was always correct and nothing on the production path needed changing.- The other
local.result = ...assignments in the spec (lines 26, 52, 59, 69, 80, 87, 93, 108, 114, 123–125) all sit outside catch blocks, so they are unaffected by the BoxLang quirk. No collateral fixes required. - The CHANGELOG and
.ai/doc are updated. ✓
Conventions
Spec comment is too long.
vendor/wheels/tests/specs/model/lockingSpec.cfc lines 33–40:
// Track state via a struct field, not `local.X`. On BoxLang,
// `local.X = value` inside a `catch` block writes to a
// catch-scoped local that gets discarded when the block exits,
// so a subsequent `expect(local.X)` reads the un-touched outer
// value. Struct field assignment (`state.X = ...`) targets a
// heap object and survives the scope transition on every
// engine. Same pattern as TenantResolverSpec's `result.threw`.
// Regression: issue #2744.CLAUDE.md says: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max. Only add one when the WHY is non-obvious." The WHY is non-obvious here (justified), but 8 lines is 7 too many now that .ai/wheels/cross-engine-compatibility.md carries the full explanation. Suggest collapsing to one line:
// BoxLang: local.X inside catch doesn't persist — struct field survives. See .ai/wheels/cross-engine-compatibility.md §"local.X inside catch".Minor: struct literal style inconsistency.
The spec fix (line 41) uses colon syntax {exceptionThrown: false}, while the prior-art patterns the PR cites use equals syntax — TenantResolverSpec line 231: var result = {threw = false}, and the doc's "RIGHT" example: var state = {caught = false}. Both are valid CFML, but consistency with the established style in sibling specs would be tidier.
Cross-engine
"ALSO RIGHT" example needs a WHY clause.
.ai/wheels/cross-engine-compatibility.md (the new section):
// ALSO RIGHT — var-declared name without `local.` prefix
var caught = false;
try { ... } catch (TestException e) { caught = true; }
expect(caught).toBeTrue();This example is accurate per the PR's diagnostic (var flag = false; flag = true survived on BoxLang), but a future reader will ask: "If var caught declares local.caught, why does the unscoped form survive when local.caught = true doesn't?" The doc doesn't answer this. A one-sentence note would close the gap, e.g.:
BoxLang's catch-scoped local only shadows keys set via explicit
local.X = ...; an unscoped write to avar-declared name appears to resolve through the var-declaration slot and escapes the catch-scope shadow. Prefer the struct-field form regardless — it is cleaner and the mechanism doesn't rely on this behaviour being preserved.
Docs
CLAUDE.md Cross-Engine Invariants not updated.
CLAUDE.md already lists 9 invariants in the "Cross-Engine Invariants" section. The catch-scope quirk documented here is exactly the kind of non-obvious, spec-breaking gotcha that belongs there — spec authors who stop reading at CLAUDE.md (the canonical checklist) won't see this rule. Suggest adding a 10th entry:
10. **`local.X = ...` inside `catch` doesn't persist on BoxLang.** Use a
struct field (`var state = {flag: false}; ... state.flag = true`) or a
plain `var bareName` assignment inside catch. Explicit `local.X = ...`
inside a catch block writes to a catch-scoped nested local that BoxLang
discards on block exit.
Commits
fix(test): work around BoxLang catch-scope discard in lockingSpec (#2744) — valid type, subject 72 chars (≤ 100), sentence-case, DCO Signed-off-by present. Commit body explains the why thoroughly. ✓
Security
No concerns. No user input, no SQL construction, no output encoding surfaces touched. ✓
… doc + spec)
- Collapse the 8-line in-spec comment to one line. The `.ai/` doc carries
the full WHY; the spec only needs the pointer. Matches CLAUDE.md's
"one short line max" rule for inline comments.
- Switch struct literal in `lockingSpec.cfc` from colon (`:`) to equals
(`=`) syntax to match the prior-art pattern in `TenantResolverSpec`
(`var result = {threw = false}`) and the WRONG/RIGHT examples in the
cross-engine doc. Both are valid CFML; consistency wins.
- Add a "Why the bare-`var` form survives" paragraph to the
`.ai/wheels/cross-engine-compatibility.md` BoxLang catch-scope section
so future readers don't ask why `var caught; caught = true` survives
when `local.caught = true` doesn't. Also recommends preferring the
struct-field form since the bare-var mechanism may not be a stable
contract across BoxLang releases.
- Add invariant #10 to CLAUDE.md's "Cross-Engine Invariants" checklist.
CLAUDE.md is the canonical drive-by checklist; spec authors who stop
there need this rule visible at the surface level. One-liner pattern
matches the existing 9 invariants.
Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-grounded. No sycophancy, no false positives. All four findings check out against the actual diff, the SycophancyNone detected. A's "comment" verdict is explicitly evidence-backed: the fix is correct, production code untouched, and the four nits are all cited to specific lines or rules. No "LGTM" without justification. False positivesNone detected. I verified each claim against the diff and docs:
Missed issuesNone detected. A verified the unaffected Verdict alignmentA's "comment" verdict with four concrete, line-cited findings is consistent. Per the loop protocol, "comment" with actionable findings maps to ConvergenceAligned. A's four findings are all valid and actionable:
|
|
Addressed all three nits from @wheels-bot's Reviewer A review in dbef5e9:
No inline-thread replies needed — the review was a single summary with no |
|
@wheels-bot Reviewer B — your four converged-changes findings match A's exactly, and all four are already addressed in dbef5e9, which landed before your review fired. Verified against current HEAD:
Re your slight overstatement on finding #2 — fair point that CLAUDE.md itself mixes Your review SHA ( |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Follow-up review on the nit-fixing commit (dbef5e9f). All four nits from the previous round are cleanly addressed. The fix to the spec was already correct; this commit is pure documentation and style cleanup. Approving.
Previous Nits — Resolved
1. 8-line in-spec comment collapsed to one-liner. vendor/wheels/tests/specs/model/lockingSpec.cfc line 31:
// BoxLang: local.X inside catch doesn't persist — struct field survives. See .ai/wheels/cross-engine-compatibility.md (#2744).The WHY is in the .ai/ doc; the spec carries only the pointer. Matches CLAUDE.md's one-short-line rule. ✓
2. Struct literal colon → equals syntax. Same file, line 32:
var state = {exceptionThrown = false};Now consistent with TenantResolverSpec's prior-art var result = {threw = false} and the WRONG/RIGHT examples in the cross-engine doc. ✓
3. "Why the bare-var form survives" paragraph added. .ai/wheels/cross-engine-compatibility.md:
BoxLang's catch-scoped local only shadows keys written via explicit
local.X = ...; an unscoped write to avar-declared name appears to resolve through the var-declaration slot and escapes the catch-scope shadow. Prefer the struct-field form anyway — it's cleaner, mirrors the prior-art TenantResolverSpec pattern, and doesn't rely on this behaviour being preserved across BoxLang releases.
Closes the reader question the previous review flagged. ✓
4. CLAUDE.md Cross-Engine Invariants updated. Invariant 10 added:
10. **`local.X = ...` inside `catch` doesn't persist on BoxLang.** Catch body runs under a nested
`local` that gets discarded on exit... Use a struct field: `var state = {flag = false}; ...
state.flag = true;`.
One-liner; matches the style of the existing nine invariants; correctly surfaces the trap at the canonical checklist level. ✓
Commits
fix(test): work around BoxLang catch-scope discard in lockingSpec (#2744) — reviewed and clean in the prior round. ✓
docs(claude): address Reviewer A nits on #2744 (CLAUDE.md invariant + doc + spec) — type docs, scope claude (unrestricted per commitlint config), subject 77 chars (≤ 100), sentence-case, DCO Signed-off-by present, body explains the why for each sub-change. ✓
Security
No concerns. No production code touched in either commit. ✓
Wheels Bot — Address Review (round 1)No bot commit needed — the PR author (@bpamiri) raced this run and applied all four consensus findings in Consensus findings verified present at HEAD (
Convergence loop is complete. Reviewer A's approval at SHA |
Wheels Bot — Reviewer B (round 1)A's review on the nit-fixing commit ( SycophancyNone detected. A cited specific lines and quoted the actual text for each resolved nit rather than asserting "LGTM" without evidence. The approve is earned. False positivesNone detected. I verified A's line citations and quoted text against the diff:
Missed issuesNone detected. This PR touches only a test spec, Verdict alignmentAPPROVE is consistent with the findings. All four nits are addressed, no production code was changed in either commit, and no new issues appear in the diff. ConvergenceAligned. A's APPROVE is correct. The two commits are clean, the BoxLang catch-scope quirk is now documented at both the surface level (CLAUDE.md invariant 10) and the deep reference level ( |
|
Convergence reached — both bot reviewers approve at
All 6 CI checks green: Auto-Label, Reviewer A, Reviewer B, Bot PR TDD Gate, Validate Commit Messages, Lucee 7 + SQLite. Status No inline-thread comments exist on this PR (both bot reviews are summary-style with no |
1 participant
Summary
Fixes #2744 —
lockingSpec :: "releases lock even when callback throws an exception"was failing on BoxLang × MySQL / Postgres / SQLite (the missing leg of #2665 that #2670 intentionally deferred).The lock-release bug the issue title called out turned out not to exist. The advisory-lock code path was already correct on every engine via the existing
try { callback() } finally { release }invendor/wheels/model/locking.cfc. What broke on BoxLang was the spec, not the production code.Root cause
BoxLang appears to run the body of a
catchblock under a nestedlocalscope that gets discarded when control leaves the block. Lucee 6/7 and Adobe CF 2023/2025 share the enclosing function'slocalacross the catch, so:local.caught = false; try { Throw(type="TestException", message="boom"); } catch (TestException e) { local.caught = true; } // ← stuck to nested local on BoxLang expect(local.caught).toBeTrue(); // ← reads outer local — still false…passes on every CFML engine except BoxLang. Verified via per-test diagnostics (
var state = {flag = false}; state.flag = truesurvived;var flag = false; flag = truesurvived;local.X = truedid not).Fix
Switched the spec to the struct-field pattern
TenantResolverSpecalready uses for the equivalent assertion:var state = {exceptionThrown = false}; try { ... } catch (TestException e) { state.exceptionThrown = true; } expect(state.exceptionThrown).toBeTrue();vendor/wheels/model/locking.cfcis untouched.Documented the BoxLang catch-scope quirk in
.ai/wheels/cross-engine-compatibility.mdso future spec authors don't re-hit it (bothvar struct.fieldandvar bareNameare called out as portable;local.Xinside a catch is called out as the trap).Test plan
lockingSpecall 11 specs pass locally (verified viatools/test-matrix.sh boxlang sqliteand direct HTTP run after the spec change).local.Xalready worked).