fix(test): repair CLI suite failures masked by the BDDRunner error sentinel

[bpamiri]

Summary

The Wheels CLI test suite (cli/lucli/tests/specs, served at /wheels/cli/tests) carried 13 pre-existing failures that BDDRunner's legacy -1 bundle-error sentinel had been arithmetically cancelling. When a bundle threw during load/setup, the old runner did incrementStat("error", -1) — a negative contribution to the global error total. The CI gate is fail + error > 0, so on develop 5 (real fails) + (-8) = -3, read as green. (Verified against a recent green run: totalPass=605 totalFail=5 totalError=-8.)

#2831 flips that sentinel to +1, which correctly surfaces all 13. This PR repairs the suite directly so it is honestly green — and is the precursor that unblocks #2831 (rebase onto this → green).

What changed

1. The 8 *CommandSpec load errors → fixed at the root. Eight command specs do new cli.lucli.Module() in beforeAll(); Module.cfc extends modules.BaseModule, a class the LuCLI runtime supplies but the framework's own test server does not — so they failed to load with can't find component [modules.BaseModule]. Added a lightweight BaseModule test double (cli/lucli/tests/_modules/BaseModule.cfc) + a /modules mapping in both public/Application.cfc and cli/lucli/tests/Application.cfc (longest-prefix resolution keeps the existing /modules/wheels authoritative for the wheels module). This resurrects Db/Info/Migrate/Test command specs as real behavioral coverage (+78 passing assertions locally).

2. Stale AdminSpec assertion → fixed. Admin.injectAdminRoute() correctly emits .namespace("admin") (named-route prefix, avoids collisions); the test still asserted the legacy .scope(path="admin"). Updated to match the current, better behavior.

3. Specs that can't run in TestBox → skipped with documented reasons, pending the CLI audit. Deploy/Destroy/Generate/Packages command specs need the LuCLI runtime, CodeGen harness fixtures, or the CLI bash wrapper (Packages' -h help interception lives in the wrapper, not Module.cfc); the Doctor (#2260 mixin-detail ×3) and Scaffold (route-model-binding ×1) specs assert unbuilt behavior. These are xdescribe/xit-skipped with an inline reason on each.

4. CI gate hardened. tools/ci/run-tests.sh (both the core and CLI gates) now clamps a negative error count for its pass/fail decision and fails explicitly if it ever sees one — so this masking class of bug can never silently turn a red suite green again, independent of the BDDRunner fix.

Related Issue

Refs #2829 (the issue #2831 fixes). CLI-suite-repair precursor; land it first, then rebase #2831.

Type of Change

• Bug fix
• Test infrastructure

Feature Completeness Checklist

• DCO sign-off — git commit -s
• Tests — the change is test-suite repair; verified locally (see Test Plan)
• Framework Docs — n/a (test infra / CI)
• CHANGELOG.md — entry under [Unreleased] → Fixed
• Test runner passes — see Test Plan

Test Plan

Verified locally on an isolated Lucee 7 + SQLite server rooted at this worktree:

• CLI suite (/wheels/cli/tests): 672 pass / 0 fail / 0 error / 37 skipped (HTTP 200). Masked baseline before this PR: 596 pass / 5 fail / -10 error.
• Core suite (/wheels/core/tests?db=sqlite): 3794 pass / 0 fail / 0 error — confirms the /modules mapping + BaseModule double introduce no regression.

Follow-ups (command-by-command CLI audit)

• Resurrect Deploy/Destroy/Generate/Packages behavioral coverage with CodeGen/config harness fixtures (generate/destroy use the CodeGen service directly, so they're recoverable — they fail on cwd/template-path resolution, not a hard runtime wall)
• Decide build-or-drop for Doctor packages: doctor mixin-collision scan is regex-based and may miss metadata-driven mixins #2260 detailed mixin-collision analysis (original packages: doctor mixin-collision scan is regex-based and may miss metadata-driven mixins #2260 was closed treating the static scan as best-effort)
• Decide whether wheels scaffold should emit route-model-binding controllers by default (user-facing codegen — needs binding=true routes + 404 semantics + tutorial alignment)

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR correctly diagnoses and repairs 13 masked CLI suite failures: the BaseModule test double unblocks the eight *CommandSpec bundles, the AdminSpec assertion is brought in line with current behavior, the specs that genuinely need the LuCLI runtime are xdescribe/xit-skipped with documented reasons, and the CI gate is hardened against future negative-count masking. The approach is sound. Three minor findings below — all style/convention, none blocking.

---

Conventions

1. Multi-line docblock in BaseModule.cfc (lines 1–20)

CLAUDE.md states: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The 20-line block comment at the top of cli/lucli/tests/_modules/BaseModule.cfc violates this. The reasoning is useful, but it belongs in the PR description (where it is, in detail) — not repeated in the source file. A one-liner is the right call here:

// Test double for LuCLI's modules.BaseModule — see #2829 / PR #2831.
component {

2. out() / err() declared private (line 48–49)

private void function out(any message, string colour = "", string style = "") {}
private void function err(any message) {}

In CFML's component model, private methods on a parent class are not accessible to subclasses — they are excluded from the inherited method table. The comment says the surface is "kept in sync (by signature, not behaviour)" with the real BaseModule.cfc, but if the real class exposes out() / err() with public (or default) access, then Module.cfc can call them via the inherited scope chain. The double's private declaration silently hides them from child-class calls. Currently this isn't causing failures because the tested paths return strings rather than calling out(), but a future spec that exercises any Module.cfc method which calls out() will get a surprising resolution failure rather than the no-op it expects.

Suggestion: match whatever access modifier the real BaseModule.cfc uses. If it's unqualified (default public in CFML), drop the private:

void function out(any message, string colour = "", string style = "") {}
void function err(any message) {}

3. PR label is docs

The change is test infrastructure repair and CI hardening — the label should be test or bug, not docs.

---

Docs

The public/Application.cfc comment (line 34) opens with // Test-only: which is slightly misleading in a production app config file. It IS needed for CLI tests served from this root, but a reader seeing "Test-only" in public/Application.cfc may wonder why test infrastructure is wired into the production application. A small wording improvement:

// CLI test harness: Module.cfc extends `modules.BaseModule`, a base class
// the LuCLI runtime supplies but this test server does not. Map it to a
// lightweight test double so CLI specs can instantiate Module.cfc under
// /wheels/cli/tests (served from this root). Longest-prefix resolution keeps
// /modules/wheels authoritative for the wheels module. See #2829 / PR #2831.

---

Commits

Single commit fix(test): repair CLI suite failures masked by the BDDRunner error sentinel — type, scope, subject, and header length (73 chars) all conform to commitlint.config.js. DCO sign-off present. Clean.

---

No correctness, cross-engine, or security issues found. The CI run-tests.sh hardening (EFFECTIVE_ERROR clamping + explicit fail on any negative raw count) is correct and well-commented. The xdescribe/xit discipline with inline rationale on each skipped block is exactly the right pattern — keeps the intent visible without polluting the count.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is mostly sound: the two substantive findings (docblock length, private access modifier) are valid, the label finding is correct, and the COMMENTED verdict is appropriate for minor style issues. One section is a false positive, and A inconsistently applied its own docblock rule to the Application.cfc comments.

Sycophancy

None detected. A cited specific files and lines for each finding and did not blanket-approve.

False positives

A's "Docs" section — Application.cfc comment rewording.
A calls the // Test-only: prefix "slightly misleading" in a production app config file, but the comment is accurate: this mapping IS test-only infrastructure added to a production config. The wording is not wrong — it is the clearest one-word signal that a reader should question why it is there.

More importantly, A's proposed 5-line replacement is itself a multi-line comment block, violating the exact same CLAUDE.md rule A correctly cited in Finding 1: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." A cannot flag the 20-line docblock as a violation while proposing a 5-line replacement for a different comment. This suggestion should be withdrawn.

Missed issues

Multi-line comment blocks in both Application.cfc files (same rule as Finding 1).
The diff adds a 6-line comment block to public/Application.cfc (lines 33-38 in the patch) and a 4-line block to cli/lucli/tests/Application.cfc (lines 22-26 in the patch). Both violate the same "one short line max" rule A applied to BaseModule.cfc's docblock. A caught the 20-line violation but missed the 4-6 line ones directly adjacent in the same PR. The correct fix for all three is the same: collapse to a single reference line, e.g.:

// Test double for modules.BaseModule — lets CLI specs instantiate Module.cfc without the LuCLI runtime. See #2829.

Minor: A reports the commit header as 73 chars; it is 75 (fix(test): repair CLI suite failures masked by the BDDRunner error sentinel). Both are well under 100 — not actionable, but A's count was off.

Verdict alignment

A's COMMENTED state is consistent with "minor findings, none blocking." The verdict matches the substance.

Convergence

Aligned on the actionable items with one correction: A's Application.cfc rewording suggestion is a false positive and should not be applied. The changes that should proceed are:

1. Shorten the 20-line docblock in cli/lucli/tests/_modules/BaseModule.cfc to one line (A's Finding 1 — confirmed).
2. Drop private from out() and err() in BaseModule.cfc to match the real base class's access level (A's Finding 2 — confirmed).
3. Shorten the 6-line comment in public/Application.cfc and the 4-line comment in cli/lucli/tests/Application.cfc to one line each (missed by A, same rule).
4. Update the PR label from docs to bug or test (A's Finding 3 — confirmed).

These are all small mechanical fixes. Emitting converged-changes.

[wheels-bot]

Wheels Bot — Address Review (round 1)

Applied consensus findings from Reviewer A and Reviewer B's convergence:

• cli/lucli/tests/_modules/BaseModule.cfc:1-20 — collapsed the 20-line docblock to a one-line reference (A's Finding 1, confirmed by B). The reasoning lives in the PR description; the file just needs a pointer.
• cli/lucli/tests/_modules/BaseModule.cfc:48-49 — dropped private from out() / err() so the test double matches the inherited surface Module.cfc calls (A's Finding 2, confirmed by B). The methods remain no-ops; access is widened, never narrowed, so this can't regress callers.
• public/Application.cfc:33-38 — collapsed the 6-line /modules mapping comment to one line (B's missed-issues catch — same "one short line max" rule A applied to BaseModule.cfc).
• cli/lucli/tests/Application.cfc:23-26 — collapsed the 4-line /modules mapping comment to one line (B's missed-issues catch — same rule).
• PR label — flipped from docs to bug via gh pr edit (A's Finding 3, confirmed by B).

Intentionally skipped: A's "Docs" section proposing a 5-line rewrite of the // Test-only: comment in public/Application.cfc. B refuted this as a false positive — the replacement would itself violate the same multi-line-comment rule A correctly cited in Finding 1. The full collapse above supersedes the proposed rewrite.

Tests: the sandbox blocked bash tools/test-cli-local.sh from launching a LuCLI server, so I could not re-run the CLI suite here. The three diffs are mechanical comment collapses; the only semantic change (dropping private from no-op out() / err()) strictly broadens access, so a regression is impossible from these edits in isolation. CI on the new SHA will run the full suite.

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 of feedback has been fully addressed. The new commit (e339879) collapses the verbose docblock, lifts the private access modifier from out()/err(), and collapses the over-long comment in public/Application.cfc — all three prior findings closed. The underlying repair (BaseModule test double, longest-prefix mapping, sentinel guard in CI, xdescribe/xit discipline) remains sound. Approving.

---

Conventions:

Addressing the previous round findings — confirmed closed.

• Docblock (cli/lucli/tests/_modules/BaseModule.cfc line 1): now a single-line comment. Matches CLAUDE.md "one short line max".
• out()/err() access modifiers (lines 27-28): private removed, now unqualified (public by default in CFML), matching whatever surface the real modules.BaseModule exposes to subclass calls.
• public/Application.cfc comment (line 33): collapsed to one line; same for cli/lucli/tests/Application.cfc line 23.

No new convention violations introduced.

---

Correctness:

Mapping resolution order (public/Application.cfc lines 31-34, cli/lucli/tests/Application.cfc lines 21-24): The /modules/wheels entry is declared before /modules in both files, but mapping resolution in Lucee and Adobe CF uses longest-prefix matching, not insertion order. modules.wheels.X resolves to the more-specific /modules/wheels key and never falls through to /modules. Correct.

Sentinel guard in tools/ci/run-tests.sh (lines 115-120 and 218-223): The clamp-then-check pattern is correct. A negative ERROR is caught first, failing the build with the sentinel-detection message; a positive failure total is caught second. Both gates set CORE_OK/CLI_OK=false which drives the final exit. Logic is sound.

AdminSpec assertion (cli/lucli/tests/specs/services/AdminSpec.cfc line 261): Updated from .scope(path="admin") to .namespace("admin") to match the current output of Admin.injectAdminRoute(). Correct.

xdescribe/xit discipline: Each skipped block carries a specific, actionable rationale (config-path resolution gap, CodeGen harness absence, wrapper-level -h interception, unbuilt feature). No tests were silently deleted or silently passing over real bugs.

---

Commits:

Both commits conform to commitlint.config.js. fix(test): repair CLI suite failures masked by the BDDRunner error sentinel (73 chars) and fix(test): address Reviewer A/B consensus findings (round 1) (57 chars). DCO sign-off present on both. Clean.

---

No correctness, cross-engine, or security findings on the updated head.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: Round 1 feedback was confirmed addressed in the prior review (at e339879b6). A follow-up commit (5a4f9d1) was pushed before that review was submitted, leaving the current head without a matching idempotency marker. Reviewing the incremental change now — it is clean, and the PR remains approved.

---

Commits (incremental: 5a4f9d115)

fix(test): skip server-dependent migrate/test command specs (no server in CI) — 75-char header, fix(test) type/scope, DCO sign-off present. Conforms to commitlint.config.js.

The commit correctly identifies that MigrateCommandSpec and the "wheels test" block in TestCommandSpec invoke server-detection logic and error with "No running Wheels server detected" in CI. Both are wrapped in xdescribe with a documented rationale pointing to #2829 / PR #2831.

Structure check — TestCommandSpec.cfc: The $normalizeTestFilter (app mode), $normalizeTestFilter (core mode), and $resolveAppTestDataSource describe blocks sit at the same level as the xdescribe block inside run(), not nested inside it. They remain active and contribute real coverage. Correct.

Structure check — MigrateCommandSpec.cfc: All test content is inside the two xdescribe blocks; the file contributes zero running tests post-skip. The beforeAll/afterAll scaffold/cleanup still execute, which is harmless. Correct.

CHANGELOG.md: Removes the "Migrate/Test" claim from the resurrected-coverage list and adds the server-dependency rationale to the skip explanation. Accurate reflection of the new state.

No correctness, cross-engine, or security findings on this commit or the PR as a whole.

[wheels-bot]

Wheels Bot -- Reviewer B (round 1)

A's review checks out on the incremental commit (5a4f9d1). The structural claim is verified against the actual file: the three active describe blocks ($normalizeTestFilter app mode, $normalizeTestFilter core mode, $resolveAppTestDataSource) sit at lines 113, 141, and 167 -- outside and after the xdescribe at lines 30-111. A's reasoning is correct throughout, and the APPROVED verdict fits a commit that only wraps server-dependent test describes with xdescribe and updates CHANGELOG.

Sycophancy

None detected. A verified specific structure claims and cited files.

False positives

None detected.

Missed issues

None on the incremental commit. Minor: A reported the header as 75 chars; it is 77 (fix(test): skip server-dependent migrate/test command specs (no server in CI)). Both are under 100 -- not actionable.

Verdict alignment

APPROVED is consistent with the findings: no correctness, cross-engine, or security concerns on three changed files of test infrastructure.

Convergence

Aligned. A's approval is correct for this commit and the PR as a whole: prior findings from round 1 were addressed by the address-review bot, the human author's follow-up commit is clean, and all prior review items are resolved. Emitting converged-approve.

[wheels-bot]