fix(cli): wheels test --ci emits GitHub Actions error annotations for failures - #3132
Conversation
… failures The --ci flag was parsed and threaded into runTests() but never consumed, so `wheels test --ci` produced byte-identical output to a plain run despite testing.mdx documenting it as tightening output for GitHub Actions and similar runners (#3113). displayTestResults() now takes a ciMode argument and, when set, emits one GitHub Actions `::error` workflow-command annotation per failed/errored spec via a new pure $buildCiAnnotations() helper (newlines/percent encoded so each annotation stays a single line). The verbose per-spec tree is regression-locked by a new ModuleOutputCapture-based spec. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…rence `wheels test --ci` now emits one GitHub Actions `::error` workflow-command annotation per failed or errored spec (issue #3113). Three guide pages previously described the flag as a no-op or forward-compat placeholder; update each to reflect the actual behavior. Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This PR wires the previously-dead --ci flag into displayTestResults() so wheels test --ci emits one GitHub Actions ::error workflow-command annotation per failed/errored spec, via a new pure $buildCiAnnotations() helper plus encoding helpers, with unit and observable-output specs. The implementation is correct and faithfully follows established in-file patterns; I attempted to refute every concern against the actual code and none survived as blocking. Verdict: comment — only docs/coverage nits below, and CI (queued at review time) still needs to confirm the suite passes, which the author honestly disclosed in the checklist.
What I verified (so the next reviewer doesn't have to)
public $buildCiAnnotationsis safe. Public$-prefixed helpers are the documented "public for specs" carve-out (cli/CLAUDE.md§ Code Style), andmcpHiddenTools()'s structural sweep (cli/lucli/Module.cfc:203-217) auto-hides every public$-function from MCPtools/list— no denylist update needed.suite.globalException ?: ""treated as a string matches prior art —emitTapResults()does the identical thing atcli/lucli/Module.cfc:5272, and the browser-test walker at:7431.- The walker genuinely mirrors
emitTapResults()(cli/lucli/Module.cfc:5248-5285), including the suite-level empty-specStatsbranch and thectx-struct-by-reference closure pattern (CLAUDE.md cross-engine invariant; CLI runs on bundled Lucee where recursive closure self-reference works — same idiom at:5248and:7400). - Encoding order is correct:
%→%25first, then\n→%0A, so the escape sequences themselves aren't double-escaped; the title additionally escapes:and,, matching GitHub'sactions/toolkitproperty rules. Dropping\r(instead of%0D) is intentional, documented in the docstring, and functionally equivalent for CRLF text. - No call-site breakage:
ciModewas already parsed and threaded (Module.cfc:732,:5094); the diff only consumes it, and the newdisplayTestResultsparameter is optional and last. - Tests are grounded: the
[PASS]assertion in the--verbosespec matchesdisplaySuite()output atModule.cfc:5569;renderResults()resets the capture buffer per call; the encoding spec exercises newline + percent and asserts no raw newline survives. - Commit conforms to commitlint (
fix(cli): …, ≤ 100 chars) and the DCOSigned-off-byemail matches the commit author. Changelog fragment present with a validfixedtype.
Docs
- Two guide pages now affirmatively claim the opposite of this PR's behavior and should be on the docs-bot's hit list (the checklist defers docs to
bot-update-docs.yml, which is fine — but these are false statements, not just missing coverage, so calling out the exact lines):web/sites/guides/src/content/docs/v4-0-0/testing/index.mdx:84— "wheels test --ci— accepted for forward-compatibility, but currently a no-op"web/sites/guides/src/content/docs/v4-0-0/testing/ci-integration.mdx:81— "Accepted for forward-compatibility — currently changes nothing"web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/testing.mdx:62should gain a sentence about the::errorannotations.
Conventions
- Annotations only fire on the
simple(default) reporter. The emission lives insidedisplayTestResults(), sowheels test --reporter=json --ciand--reporter=tap --ciprint no annotations (cli/lucli/Module.cfc:5179-5188). That's a defensible design — injecting::errorlines into the JSON stream would corrupt| jqconsumers, and TAP has its own diagnostics — but it's currently undocumented. Suggest one sentence in the testing guide (or the--ciflag description intestArgSpec,Module.cfc:279) stating that annotations apply to the default reporter.
Tests
-
Non-blocking follow-up: compile-failed spec files get no annotation. The
specsFailedToLoadprobe (Module.cfc:5403-5411) prints a WARN for bundles TestBox silently skipped, but under--cithose produce no::errorline — and (pre-existing, not introduced here) they don't flip the exit code at:5194since they contribute nothing tototalFail/totalError. For a CI-surfacing feature, annotating eachunloadedSpecPathsentry would be a natural next increment. Fine to land without it; consider a follow-up issue. -
The "Test runner passes" checklist item is honestly unchecked — the CLI suite wasn't executed in the bot sandbox. The PR is a draft and the
Lucee 7 + SQLite (LuCLI)check plus the Bot PR TDD Gate were queued at review time; this should not leave draft until they're green. Nothing in my static trace suggests they won't be.
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This re-review covers the new head (docs commit f82f6d687 on top of the previously-reviewed implementation commit 752b302f7). The --ci implementation itself is unchanged and remains sound, and the docs commit correctly fixes three guide pages. However, the CLI test suite fails to compile at this head — the PR's own spec file contains a block-comment that self-terminates on a */ inside a glob path, crashing the entire Lucee 7 + SQLite (LuCLI) check with HTTP 500. That is a blocking finding. Verdict: request changes — the fix is a one-line comment reword. (Submitted as a comment-state review because GitHub rejects a REQUEST_CHANGES event from the PR's own author — wheels-bot opened this PR; the red Lucee 7 + SQLite (LuCLI) required check is the effective merge block.)
Correctness / Tests (blocking)
-
cli/lucli/tests/specs/commands/TestCommandSpec.cfc:435— docblock terminates early on*/inside a path, breaking compilation of the whole CLI suite.* JSONReporter getMemento() the CLI deserializes from /wheels/*/tests.CFML block comments don't nest and have no escaping, so the comment opened by
/**at line 433 ends at the*/embedded in/wheels/*/tests. The trailingtests.becomes code and line 436's now-orphaned*/is what Lucee reports:[CLI Tests] HTTP status: 500 {"success":false,"error":"Invalid identifier; Failed at [.../cli/lucli/tests/specs/commands/TestCommandSpec.cfc:436]","detail":""}(workflow run 27424634422 at this head; the prior run at
752b302f7failed identically, so this came in with the implementation commit). Because the runner compiles every spec in the directory, this one comment takes down the entire CLI suite — the same all-or-nothing failure mode the rootCLAUDE.mdtesting section warns about for compile errors in spec directories.Fix: reword to avoid the
*/byte sequence, e.g. "…deserializes from/wheels/app/testsand/wheels/core/tests" or/wheels/<suite>/tests. I scanned the full diff for other mid-comment*/sequences — this is the only occurrence, so this one-line change should make the suite compile.Credit where due: the PR body's unchecked "Test runner passes" item honestly disclosed that the suite was never executed in the sandbox; this is precisely the class of bug only a real run catches. The PR must not leave draft until
Lucee 7 + SQLite (LuCLI)is green.
Docs (non-blocking)
-
web/sites/guides/src/content/docs/v4-0-0/testing/index.mdx:84still affirmatively claims the opposite of this PR's behavior. The docs commit fixedtesting.mdx,ci-integration.mdx, andrunning-tests-locally.mdx, but the fourth page flagged in the previous review was missed; at this head it still reads:- `wheels test --ci` — accepted for forward-compatibility, but currently a no-op: …Since this PR now updates the guides itself rather than deferring to the docs bot, please update this line in the same pass (keep the second sentence about
WHEELS_CI/WHEELS_BROWSER_CI_ENABLE— that part is still accurate). -
Reporter scoping still undocumented (carried from the previous review): annotations only fire on the default
simplereporter — theciMode-awaredisplayTestResults()call sits in thecase "simple": default:branch (cli/lucli/Module.cfc:5188), so--reporter=json --ciand--reporter=tap --ciemit no annotations. That design is defensible (don't corrupt JSON/TAP streams), but the new--cirow inci-integration.mdx's reporter table invites pairing the flag with any reporter. One sentence ("annotations are emitted by the defaultsimplereporter") would close the gap.
Commits
Both commits conform to commitlint (fix(cli): …, docs(web/guides): …; Validate Commit Messages check is green) and each carries a Signed-off-by: trailer matching its committer.
Carried verification (implementation unchanged since the 752b302f7 review)
The implementation hunks are byte-identical to what was already traced in the previous review. I independently re-verified the two load-bearing claims rather than trusting them: suite.globalException ?: "" treated as a string matches the emitTapResults() prior art at cli/lucli/Module.cfc:5272, and mcpHiddenTools()'s structural sweep (Module.cfc:203-217) auto-hides any public $-prefixed function from MCP, so public $buildCiAnnotations does not leak as a tool. The encoding helpers, walker pattern, and specs were reviewed in the prior round and stand.
…minate the comment The $passingResult() docblock contained a literal '/wheels/*/tests' — the '*/' inside the path closed the block comment early, leaving stray tokens that fail compilation. Because the CLI suite compiles every CFC in the directory, the whole suite returned HTTP 500. Reword to '/wheels/app|core/tests'. Signed-off-by: Peter Amiri <peter@alurium.com>
# Conflicts: # web/sites/guides/src/content/docs/v4-0-0/command-line-tools/wheels-commands/testing.mdx
…rt behavior Two live audits (issue #3113 and the #3124 behavioral audit) found 'wheels test --verbose' output byte-identical to a plain run — the LuCLI picocli root declares -v/--verbose as a global runtime option, so the flag never reaches the module's renderer. The command reference (testing.mdx) already carries the audited wording from #3124; bring the ci-integration reporter table in line instead of claiming the bundle/suite/spec tree is printed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This re-review covers head 2faa9d429 (commits cf94e32e8 docblock fix, merge of origin/develop, and 2faa9d429 --verbose docs alignment, on top of the previously-reviewed 752b302f7/f82f6d687). The previous round's single blocking finding — the spec docblock self-terminating on a */ inside a glob path and crashing the whole CLI suite — is resolved, and the Lucee 7 + SQLite (LuCLI) required check now passes at this head, which means the PR's own $buildCiAnnotations and ModuleOutputCapture specs ran green for the first time. The --ci implementation is unchanged and remains correct. What's left is two carried docs nits (one of which has now been flagged in two prior reviews). Verdict: comment — nothing blocking remains.
Resolved since last review (audit trail)
-
Spec-file compile crash — fixed.
cli/lucli/tests/specs/commands/TestCommandSpec.cfc:435now reads:* JSONReporter getMemento() the CLI deserializes from /wheels/app|core/tests.The
*/byte sequence is gone (commitcf94e32e8), and I re-scanned the file for any other mid-comment*/— none. Evidence it's truly resolved: theLucee 7 + SQLite (LuCLI)check, which failed with HTTP 500 (Invalid identifier … TestCommandSpec.cfc:436) at the two prior heads, passes at this head (run 27430242170, 2m32s). That green run is also the first real execution of this PR's new specs — the unchecked "Test runner passes" item in the PR body is now effectively satisfied by CI. -
--verbosedocs contradiction — fixed. Commit2faa9d429bringsci-integration.mdx:82in line with the audited reality (the LuCLI picocli root intercepts-v/--verbosebefore the module — seecli/CLAUDE.md"LuCLI reserves some tokens"), replacing the old claim that the bundle/suite/spec tree is printed.testing.mdxalready carried the audited wording.
Carried verification (implementation unchanged)
The Module.cfc hunks are byte-identical to what was traced in the previous two reviews; re-confirmed the load-bearing points at this head rather than trusting them:
ciModeis parsed atcli/lucli/Module.cfc:732and threadedtest()→runTests()(line 5157) →displayTestResults()(line 5251) — the flag is genuinely consumed now.- The
$buildCiAnnotations()walker (lines 5547–5599) mirrors the battle-testedemitTapResults()walker (lines 5310–5348) structurally, including the suite-level synthetic failure and thesuite.globalException ?: ""string assumption. Readingspec.error.messageforError-status specs matchesdisplaySuite()'s own Error branch (line 5642). - Encoding neutralizes workflow-command injection: a
failMessagecontaining a newline +::errorcannot start a second annotation because LF is collapsed to%0A(line 5609) with%escaped first (line 5607). public $buildCiAnnotationscannot leak as an MCP tool: themcpHiddenTools()structural sweep (lines 199–221) auto-hides every public$-prefixed function.
Tests
- (nit)
TestCommandSpec.cfc:415— the spec title slightly overstates what--verbosecan do end-to-end.it("--verbose prints per-spec PASS lines")exercisesdisplayTestResults(verboseOutput=true)directly through the fixture — real, worthwhile regression coverage of the renderer — but per this PR's own head commit (2faa9d429) the--verboseflag never reaches the module, so the flag itself prints nothing today (and the PR's docs now say exactly that). Consider renaming to something like"verboseOutput renders per-spec PASS lines"so the spec name doesn't read as evidence the flag works. Non-blocking.
Docs
-
web/sites/guides/src/content/docs/v4-0-0/testing/index.mdx:84still affirmatively claims the opposite of this PR's behavior — third time flagged (both prior reviews). At this head it still reads:- `wheels test --ci` — accepted for forward-compatibility, but currently a no-op: …This PR updates three other guide pages itself rather than deferring to the docs bot, so merging it leaves shipped docs that directly contradict shipped behavior on the fourth page. Please update this line in the same pass (keep the second sentence about
WHEELS_CI/WHEELS_BROWSER_CI_ENABLE— still accurate). Non-blocking, but it's the one substantive thing left to do. -
(nit, carried) Reporter scoping undocumented. Annotations only fire on the default
simplereporter — theciMode-aware call sits in thecase "simple": default:branch (cli/lucli/Module.cfc:5249-5251), so--reporter=json --ciand--reporter=tap --ciemit none. Defensible design (don't corrupt JSON/TAP streams), but the--cirow inci-integration.mdx's reporter table (line 81) invites pairing it with any reporter. One sentence closes the gap.
Commits
All five commits at this head conform to commitlint (fix(cli): … ×2, docs(web/guides): … ×2, plus a merge commit, which commitlint exempts) — the Validate Commit Messages check is green — and the non-merge commits carry matching Signed-off-by: trailers. The changelog fragment changelog.d/3113-cli-test-ci-annotations.fixed.md is present with the correct fixed type.
Summary
wheels test --ciwas parsed and threaded all the way intorunTests()but theflag was never consumed — so
wheels test --ciproduced byte-identical stdout to aplain run, even though
testing.mdxdocuments--cias tightening output forGitHub Actions and similar runners.
This wires the flag:
displayTestResults()now accepts aciModeargument and, whenset, emits one GitHub Actions
::errorworkflow-command annotation per failed/erroredspec (via a new pure
$buildCiAnnotations()helper). Messages are encoded per theworkflow-command rules (
%→%25, newlines →%0A, plus:/,in thetitleproperty) so a multi-line failMessage stays a single annotation line and surfaces
inline in CI logs and PR-check annotations.
The
--verbosehalf of the issue is already satisfied ondevelop:displayTestResults()renders a per-spec bundle/suite tree when
bundleStatsis present (added 2026-03-13),and the JSON reporter returns
getMemento(includeDebugBuffer=true)which includes it.This PR adds a
ModuleOutputCapture-based spec that regression-locks that behavior, butbecause I could not exercise it end-to-end against a live server in the bot sandbox, this
ships as a partial-scope fix — see the note below.
Refs #3113
Type of Change
Feature Completeness Checklist
Signed-off-by:(committed withgit commit -s)cli/lucli/tests/specs/commands/TestCommandSpec.cfc: failing-then-passingunit specs for
$buildCiAnnotations(empty/failure/encoding cases) plusModuleOutputCapture-based observable-output specs for--ci(annotations) and--verbose(per-spec PASS lines)bot-update-docs.ymlbot-update-docs.ymlbot-update-docs.ymlchangelog.d/3113-cli-test-ci-annotations.fixed.mdbash tools/test-cli-local.sh) could NOT beexecuted in the bot sandbox (the script and
lucliwere not granted execution, and notest server was running). The fix was statically reviewed against the existing
emitTapResults()walker pattern and the established$-helper unit-test conventions.CI must verify the failing-then-passing run before this leaves draft.
Test Plan
bash tools/test-cli-local.sh # runs cli/lucli/tests/specs/**, including TestCommandSpecwheels test --ciagainst a suite with failures should now print lines like:while a plain
wheels testprints no such lines (byte-for-byte unchanged from before).