{{ message }}
Repository navigation
Backend selection and the --result-reader flag (#391, Task 11) - #448
Merged
Merged
Conversation
ResultBackend resolves a requested backend against the toolchain's legacy capability: auto prefers legacy while the toolchain advertises it, and an explicit legacy request is never silently substituted — the CLI rejects it up front and Summary records a legacyReaderUnavailable fault for library consumers. Demotion stays scoped to capability detection; per-bundle failures propagate through FaultCollector exactly as before. Amends plan Task 11 (mkdir -p in the step-5 check, test count, added out-of-process CLI test for Task 12's differential) and the spec's Backend selection section, which still said explicit legacy falls back. Refs #391 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/XCTestHTMLReportTests/ResultBackendTests.swift`:
- Around line 42-61: Update testBothBackendsProduceAReportForTheSameBundle so
the modern backend is always tested, while ResultBackend.legacy is included only
when XCResultToolClient.legacyCapability is .available; do not substitute modern
for an unavailable legacy backend. Rename the test to reflect that both readers
are not guaranteed to run on every host.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67db38f3-0dd2-4d9e-ba72-a634c6a55f26
📒 Files selected for processing (7)
Sources/XCTestHTMLReport/XCTestHtmlReport.swiftSources/XCTestHTMLReportCore/Classes/Helpers/FaultCollector.swiftSources/XCTestHTMLReportCore/Classes/Models/Summary.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/ResultBackend.swiftTests/XCTestHTMLReportTests/ResultBackendTests.swiftdocs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.mddocs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md
The both-backends parity test looped over legacy unconditionally, which fails by design on a modern-only host: the explicit legacy arm records legacyReaderUnavailable. Same anti-time-bomb reasoning the resolution tests already use; parity stays fully asserted on every host that has both backends. Plan snippet amended to match. Refs #391 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/XCTestHTMLReportTests/ResultBackendTests.swift (1)
112-119: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the legacy report contains groups.
groupCount(reader: "legacy")returns0when the HTML has notest-summary-group. SincemodernGroupsis only required to be greater than zero,XCTAssertNotEqualcan pass for a missing legacy report.Store the legacy count and assert that it is greater than zero before comparing it with
modernGroups.Proposed fix
if XCResultToolClient.legacyCapability == .available { // Legacy interposes the wrapper levels the modern tree omits, so // equal counts mean the flag never reached `Summary` and both // runs used one backend. + let legacyGroups = try groupCount(reader: "legacy") + XCTAssertGreaterThan(legacyGroups, 0, "legacy rendered no test groups") XCTAssertNotEqual( - try groupCount(reader: "legacy"), modernGroups, + legacyGroups, modernGroups, "legacy and modern rendered identical group counts — the flag is not reaching Summary" ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/ResultBackendTests.swift` around lines 112 - 119, Update the legacy-versus-modern assertion in the XCResultToolClient.legacyCapability block to store groupCount(reader: "legacy") in a named value, assert that this legacy count is greater than zero, then compare it against modernGroups with the existing inequality assertion.
🧹 Nitpick comments (1)
Tests/XCTestHTMLReportTests/ResultBackendTests.swift (1)
13-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake capability-state tests deterministic.
XCResultToolClient.legacyCapabilityis a process-wide static value. Each test run executes only oneswitchbranch. A host with.availabledoes not test.unavailable, and a host with.unavailabledoes not test.availableor.unknown.Add an injectable capability parameter or a pure resolver helper. Table-test all three states for
.legacyand.auto.Based on learnings: capability detection is tri-state; only
automay fall back tomodern, while explicitlegacyuseslegacyfor.available/.unknownand returns.legacyUnavailablefor.unavailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/XCTestHTMLReportTests/ResultBackendTests.swift` around lines 13 - 24, Make backend resolution independently testable by adding an injectable capability parameter or pure resolver helper instead of relying on XCResultToolClient.legacyCapability. Table-test .legacy and .auto against .available, .unknown, and .unavailable, preserving that explicit legacy never resolves to modern, while auto falls back to modern only when legacy is unavailable.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Tests/XCTestHTMLReportTests/ResultBackendTests.swift`:
- Around line 112-119: Update the legacy-versus-modern assertion in the
XCResultToolClient.legacyCapability block to store groupCount(reader: "legacy")
in a named value, assert that this legacy count is greater than zero, then
compare it against modernGroups with the existing inequality assertion.
---
Nitpick comments:
In `@Tests/XCTestHTMLReportTests/ResultBackendTests.swift`:
- Around line 13-24: Make backend resolution independently testable by adding an
injectable capability parameter or pure resolver helper instead of relying on
XCResultToolClient.legacyCapability. Table-test .legacy and .auto against
.available, .unknown, and .unavailable, preserving that explicit legacy never
resolves to modern, while auto falls back to modern only when legacy is
unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10e8bb32-3d6f-423a-9696-d5a9e39d408d
📒 Files selected for processing (2)
Tests/XCTestHTMLReportTests/ResultBackendTests.swiftdocs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.md
This was referenced Aug 13, 2026
This was referenced Aug 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #391 (milestone 4.0) — plan Task 11.
What
ResultBackend(auto|legacy|modern) withresolve(), which checks the requested backend againstXCResultToolClient.legacyCapability(XCResultToolClient and new-format schema structs (#391) #441's probe):autopreferslegacywhile the toolchain advertises it, falls back tomodernotherwise (including an unparseable version string). Never resolves toauto.legacyis never silently substituted. On a toolchain that definitively lacks the legacy commands it resolves to.legacyUnavailable: the CLI rejects it invalidate()with a clear error, andSummary.init(which cannot throw) records a newlegacyReaderUnavailablefault that reaches exit 3 — otherwise the Task 12 differential could run modern-vs-modern and report parity against itself.unknowncapability treats legacy as available for explicitlegacy(not proof of absence); any resulting command failure surfaces as a fault, not a capability signal.FaultCollectorto exit 3 exactly as before; there is no retry-on-other-backend anywhere.Summary.init(..., backend: ResultBackend = .auto)appended afterfaultCollector, wiring per-bundle reader/payload-provider construction through the resolution (LegacyResultReader+ResultFilevsModernResultReader+ModernPayloadStore).--result-reader legacy|modern|auto(defaultauto) on the CLI, drivable out of process through the existingxchtmlreportCmdhelper — Task 12's differential forces each reader through the CLI.Tests
ResultBackendTestsper the plan (resolution table, both-backends-render-with-zero-faults through the realSummarypath), plustestResultReaderFlagWorksOutOfProcess: runs the built binary with--result-reader modern(andlegacywhere available) and asserts the rendered group counts differ, proving the flag reachesSummaryfrom a separate process.swift test: 71 tests, 0 failures (2 pre-existing skips)legacy exit=0 groups=14,modern exit=0 groups=10— counts differ as requiredPlan/spec amendments (in this PR)
mkdir -p—xchtmlreportdoes not create the output directory, so the snippet as written exited 1 for both readers.legacyfalls back tomodern; rewritten to the never-silently-substituted semantics the plan's tests encode.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests