fix(test): browser skip path, dialog listener detach, launcher lifecycle and locking

[bpamiri]

Summary

Fixes 8 browser-test-infrastructure findings from the 2026-06-09 framework review (package test-browser-lifecycle): the browserDescribe() skip path now actually skips spec bodies (removing 30 hand-written per-spec guards), dialog listeners are detached from the Playwright Page after each armed action, the application-scoped BrowserLauncher is released on onApplicationEnd (no more orphaned headless browser/driver processes across reloads), acquireBrowser() gains double-check locking plus an isConnected() liveness probe, $startBrowserContext() throws a descriptive Wheels.BrowserTest.NotWired error, the triplicated WaitForOptions construction is extracted to $waitOptions() (surfacing the silent custom-timeout fallback), and the dead $jarPath/$verifyInstall pair pointing at a nonexistent CLI command is deleted.

Findings addressed

• T1 [Medium] browserDescribe skip path still executes spec bodies, forcing duplicated skip guards in every browser spec @ vendor/wheels/wheelstest/BrowserTest.cfc:143 — aroundEach now returns without calling arguments.spec.body() when browserTestSkipped is set; all 30 if (this.browserTestSkipped) return; guards removed across BrowserLoginSpec, BrowserRouteSpec, BrowserTestLifecycleSpec, and BrowserIntegrationSpec.
• T2 [Medium] Dialog listener is never detached from the Playwright Page; the "one-shot" comment is wrong @ vendor/wheels/wheelstest/BrowserClient.cfc:1014 — $clearDialogListener() now calls page.offDialog() with the identical proxy instance registered by $registerDialogListener (identity-based removal), invoked from finally blocks in click/press/keys; docblock corrected.
• T3 [Low] Dead $jarPath/$verifyInstall pair tells users to run a CLI command that does not exist @ vendor/wheels/wheelstest/BrowserLauncher.cfc (formerly :98-114) — pair deleted (zero callers outside their own unit tests); remaining install-guidance strings standardized on the canonical wheels browser setup.
• T5 [Low] WaitForOptions construction triplicated across waitFor/waitForText/waitForUrl with silent timeout fallback @ vendor/wheels/wheelstest/BrowserClient.cfc:259 — extracted $waitOptions(); a custom timeout on a launcher-less client now throws Wheels.BrowserTimeoutUnavailable instead of silently falling back to 30s.
• T8 [Medium] BrowserLauncher.release() is never called for the application-scoped launcher — browser/driver processes and JAR handles leak across reloads @ public/Application.cfc:118 and cli/lucli/templates/app/public/Application.cfc:107 — onApplicationEnd releases application.$wheelsBrowserLauncher (key matches $ensureLauncher), so applicationStop() reload cycles no longer orphan the headless browser, node driver, and JAR file handles; the false afterAll() scope-clear comment corrected.
• T9 [Low] acquireBrowser() check-then-act on shared application-scoped state has no lock @ vendor/wheels/wheelstest/BrowserLauncher.cfc:203 — now mirrors $ensureLauncher's double-check locking (named lock wheelsBrowserLauncherAcquire).
• T10 [Low] $startBrowserContext() calls newContext() on an unwired empty-string $browser when super.beforeAll() is skipped @ vendor/wheels/wheelstest/BrowserTest.cfc:172 — throws Wheels.BrowserTest.NotWired with actionable guidance instead of a cryptic string-method error.
• T13 [Low, perf] acquireBrowser() returns the cached Browser without a liveness check — a crashed browser poisons the application-scoped cache @ vendor/wheels/wheelstest/BrowserLauncher.cfc:229 — cache hits probe isConnected() and evict/relaunch when dead.

Findings verified already-fixed

None — all 8 packaged findings reproduced against origin/develop and required fixes. The review report's line references for this package were spot-checked (T1, T3) and confirmed accurate against develop. One report nit: T3's claim that wheels browser:install "never existed" is slightly off — the legacy CommandBox CLI at cli/src/commands/wheels/browser/install.cfc defines it — but standardizing on the canonical wheels browser setup (present in cli/lucli/Module.cfc) is correct regardless.

Source

Internal multi-agent framework review 2026-06-09, wave 2, package test-browser-lifecycle.

Tests

• New: vendor/wheels/tests/specs/wheelstest/BrowserDescribeSkipSpec.cfc (T1 — fails pre-fix: the old skip branch still called spec.body() outside try/catch) and vendor/wheels/tests/specs/wheelstest/BrowserTestNotWiredSpec.cfc (T10 — fails pre-fix with the cryptic string-method error).
• Updated: BrowserDialogSpec.cfc (T2 second-dialog-in-same-it coverage), BrowserIntegrationSpec.cfc (T5 launcher-less custom-timeout throw), BrowserLauncherSpec.cfc (T3 dead-pair removal, T9/T13 locking + liveness).
• Removed the 30 obsolete skip guards across 4 spec files (9+7+5+9).
• Local verification: Lucee 7 + SQLite worktree docker single-area run — wheelstest area 151 pass / 0 fail / 0 error; new specs confirmed red against pre-fix framework code; applicationStop() reload exercised post-change. Full engine x database matrix deferred to CI (the real gate).

Cross-engine notes

• Catch-block state uses the shared-struct pattern (local.X in catch does not persist on BoxLang).
• No inline closures as constructor named args; ## escapes correct; no literal CFML tag text in strings.
• private $launchBrowser is safe — BrowserLauncher is directly instantiated, never mixin-integrated.
• The named lock wheelsBrowserLauncherAcquire is server-global, so independent launcher instances serialize launches — harmless, and portable across Lucee/Adobe/BoxLang.
• Non-blocking observation from review: examples/starter-app and examples/tweet retain the old onApplicationEnd without the release block — acceptable drift for demo apps, candidate for a follow-up sync.

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR fixes eight browser-test-infrastructure findings from the June 2026 framework review. The logic is sound, the cross-engine patterns are correctly applied, and the test coverage is targeted and clear. Approving with two minor nits — neither blocks merge.

Correctness

All eight T-findings are correctly addressed:

• T1 (aroundEach skip path): Before the fix, arguments.spec.body() was unconditionally called before the early return, making the "skip" branch a no-op that relied on each spec's hand-written guard. After: aroundEach returns before touching spec.body(). The new BrowserDescribeSkipSpec self-test is clever — a throw in the body is the proof of failure; silence (no exception) is the proof of correctness.

• T2 (dialog listener leak): $clearDialogListener() now calls variables.page.offDialog(variables.$dialogProxy) with the same proxy instance registered in $registerDialogListener, so successive armed interactions in one it block no longer stack stale listeners.

• T5 ($waitOptions refactor): Extracting the triplicated option-building into a single helper and turning the silent-fallback into Wheels.BrowserTimeoutUnavailable is a real improvement. One minor test gap noted below.

• T8/T9/T10/T13: The onApplicationEnd release block, double-check locking in acquireBrowser(), Wheels.BrowserTest.NotWired guard in $startBrowserContext(), and isConnected() liveness probe all look correct.

Cross-engine

BoxLang invariant #11 ("local.X in catch doesn't persist") is correctly handled. The catch-block state in the new BrowserTestNotWiredSpec additions uses the shared-struct pattern:

// vendor/wheels/tests/specs/wheelstest/BrowserTestNotWiredSpec.cfc (added ~lines 475-486)
var state = {message: ""};
try {
    spec.$startBrowserContext();
} catch (Wheels.BrowserTest.NotWired e) {
    state.message = e.message;  // struct field, not local.X — correct for BoxLang
}

$liveCachedBrowser assigns alive inside try (not catch), so invariant #11 does not apply there. No inline closures as constructor named args; no obj["key"]() inside closures. The named lock wheelsBrowserLauncherAcquire and its double-check pattern in acquireBrowser() mirror the established $ensureLauncher prior art in BrowserTest.cfc. All good.

Tests

Coverage is strong. One minor gap:

Nit 1 — waitForText not tested for BrowserTimeoutUnavailable.
BrowserIntegrationSpec adds specs for waitFor and waitForUrl with a custom timeout on a launcher-less client, but not waitForText, which routes through the same $waitOptions helper. Not a blocking gap (the path is structurally identical), but a third case would complete T5 coverage:

// vendor/wheels/tests/specs/wheelstest/BrowserIntegrationSpec.cfc — suggested addition
it("waitForText() with a custom timeout but no launcher surfaces BrowserTimeoutUnavailable", () => {
    var c = new wheels.wheelstest.BrowserClient()
        .init(baseUrl="http://localhost");
    expect(() => {
        c.waitForText(text="never", seconds=5);
    }).toThrow(type="Wheels.BrowserTimeoutUnavailable");
});

Conventions

Nit 2 — Inline comment blocks in Application.cfc exceed one line.
CLAUDE.md says "one short line max" for inline comments. The four-line block added to both public/Application.cfc:119 and the template copy is justified (the "why" is genuinely non-obvious — CFML lacks destructors), but it could be condensed to something like:

// Release browser launcher on reload — CFML has no destructors; without this,
// applicationStop() cycles orphan the headless browser, node driver, and JAR handles.

Worth a follow-up tightening if the codebase enforces the one-line norm consistently.

Docs

Changelog entry intentionally omitted pending campaign consolidation — acknowledged in the PR body. No user-facing behavior changes (test infrastructure only), so no guides update is needed. The examples/ app divergence is called out explicitly and flagged as a follow-up candidate; acceptable for this PR's scope.

Commits

Single commit: fix(test): browser skip path, dialog listener detach, launcher lifecycle and locking — valid type, valid scope, 84-char header (under 100). Clean.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate and well-grounded. I re-scanned the diff and the cited lines; no sycophancy, no false positives, no missed issues. Both nits are real findings. Aligning on approve.

Sycophancy

None detected. The approval cites specific evidence for each of the 8 findings rather than a blanket LGTM.

False positives

None detected.

• T1 skip path: A correctly identified the old code called arguments.spec.body() before the return. The diff confirms it (the removed line is arguments.spec.body(); inside the if (me.browserTestSkipped) block). Fix is correct.
• BoxLang invariant New master #11: A correctly noted that alive is assigned inside try (not catch), so invariant New master #11 does not apply to $liveCachedBrowser. The catch block there is only for probe failure and writes nothing to local. Struct-field pattern in BrowserTestNotWiredSpec (state.message = e.message;) is the right form.
• $launchBrowser declared private: A did not flag it as a cross-engine issue, and correctly so -- CLAUDE.md invariant New master - readme #7 only applies to mixin-integrated methods; the PR body explicitly notes BrowserLauncher is directly instantiated, never mixin-integrated.
• Commit header length: Verified 84 chars independently. A's count is correct.

Missed issues

None detected. The one gap A identified -- waitForText missing a BrowserTimeoutUnavailable spec -- is already called out in Nit 1 with a concrete code suggestion. I checked the other structural risk areas:

• The url parameter in waitForUrl (Anti-Pattern New master #11 reserved scope shadow) is pre-existing, not introduced by this PR; all new usage is via arguments.url which is explicit and safe.
• No obj["key"]() inside closures; no inline closures as constructor named args.
• The == 30 default check in $waitOptions is safe -- the parameter is declared required numeric seconds.
• Adobe CF 2023/2025 attributeCollection concern (Anti-Pattern Fixed bug in $findRoute() that causes blow up on unmatched named route #10) does not apply; the Application.cfc changes use arguments.applicationScope.$key property access, not attributeCollection.

Verdict alignment

APPROVED is consistent with the findings: two non-blocking nits, browser test infrastructure only, no user-facing behavior changes, cross-engine patterns correctly applied.

Convergence

Aligned. A's review is accurate and the approve verdict is correct. The waitForText test gap is a nit, not a correctness issue; the comment-length nit is worth a follow-up but does not block merge. Joint recommendation: approve and merge.