Design spec and implementation plan for migrating off xcresulttool --legacy (#391) - #426
Design spec and implementation plan for migrating off xcresulttool --legacy (#391)#426tylervick wants to merge 6 commits into
Conversation
Records the strategy decision and the evidence behind it: read the new xcresulttool format directly and drop XCResultKit, behind a reader abstraction that keeps the legacy path alive until Apple removes it. The central finding is that the new format is not a superset of the legacy one. Activity types and finish times, user-supplied attachment names, attachment UTIs, structured failure locations, and log emittedOutput all have no new-format equivalent, and exportRecursiveJson has no replacement at all. Byte-identical output across the two backends is therefore not achievable, so the bar is a declared and CI-asserted diff rather than an empty one. Every measurement in the spec was taken on freshly generated fixtures under Xcode 26.2 rather than inferred, including two that shape the plan: reports are not byte-reproducible (18 differing lines on the smallest fixture, all synthetic UUIDs, identical after one regex), and the two read paths are equal within noise on a same-runner interleaved A/B, so no performance claim is made in either direction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifteen tasks, each ending in a green test run and a commit, following the spec's phasing: extract a backend-neutral port, add the modern reader, prove parity differentially, then flip selection on. Two ordering decisions carry most of the risk reduction. The UUID normalizer and a pre-refactor baseline capture land before the large renderer refactor, so "behaviour-preserving" is something the implementer diffs rather than asserts. And the differential harness lands with the migration rather than after it, because it only works while xcresulttool still supports both formats. Fixtures are regenerated on every CI run, so checked-in golden HTML is impossible; every comparison in the plan is between two renders produced within one run. Both assertions that could pass on empty input are paired with an explicit non-vacuity guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a migration plan and design specification for replacing ChangesXCResultTool migration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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 `@docs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.md`:
- Around line 1749-1755: Update exportPayloadData to handle Data(contentsOf:)
failures explicitly instead of using try?, recording .payloadExportFailed with
the attachment reference and read error before returning nil. Preserve the
existing sourceURL failure handling and successful Data return.
- Around line 1797-1826: The attachment export flow around exportDirectory must
remove the UUID-named temporary directory after all payload reads finish. Track
the directory lifecycle so cleanup occurs only after the payload store no longer
needs the exported files, while retaining the current export and
manifest-processing behavior.
- Around line 823-830: Update the expected verification output in Step 4 to
account for tests added by Tasks 1, 2, and 4, using the cumulative test count;
alternatively, remove the fixed test count and state that the build and test
commands must complete successfully with zero failures and the expected
skipped-test behavior.
- Around line 420-424: Add pipefail handling to every shell block containing
piped verification commands such as swift build or test, including the
referenced locations, so the block returns a failure when the build or test
command fails rather than only reflecting tail’s status. Preserve the existing
output-trimming behavior.
- Around line 2344-2370: The testAllowListEntriesStillDescribeRealDifferences
test must compare normalized legacy and modern HTML across every fixture,
asserting that all differences are covered by the allow-list and no
unallowlisted changes remain. Add validation for entries with empty markers so
they cannot bypass coverage, while retaining the existing marker-presence checks
as stale-entry guards.
- Around line 2373-2397: Update
testAttachmentPayloadsAreByteIdenticalAcrossBackends so it compares attachment
records keyed by attachment identity and filename rather than Set<Data>.
Preserve duplicate attachments and their association with each file, and assert
that each record’s bytes match between legacy and modern summaries.
- Around line 1167-1170: Update TestResultsTests and the related test-results
models around testRuns so omitted collection keys decode as empty collections
rather than causing keyNotFound errors. Use optional or defaulted collection
properties while preserving the existing collection behavior when keys are
present, and add fixtures covering absent testNodes and testRuns keys.
- Around line 232-247: Update the baseline-capture loop around the resource list
to fail when any of TestResults, SanityResults, or RetryResults is missing
instead of continuing. After writing each normalized report, verify the
corresponding output file exists and is non-empty, and ensure exactly all three
baseline files are captured before the test succeeds.
- Around line 1728-1738: The exportPayload function must prevent attachment
names from escaping bundleURL. Reduce fileName to its basename, build and
standardize the destination, verify it remains within the standardized bundle
directory before any removal or copy, and record .payloadExportFailed with a nil
return when validation fails; add a test covering traversal and path-separator
input.
- Around line 401-404: Update the ResultReader.read contract to throws ->
ParsedResult and propagate genuine schema, subprocess, and activity-query
failures instead of converting them to nil or empty results. Ensure
modern-reader call sites record read errors through FaultCollector and only use
empty values for unsupported backend fields; apply the same behavior to the
referenced reader implementations and CLI flow.
- Around line 2097-2125: Update Summary.init’s backend setup and legacy-read
flow to catch failures from the actual LegacyResultReader operation. Preserve
explicit .legacy behavior by rethrowing its errors, but when the original
backend is .auto, construct the modern ModernPayloadStore/ModernResultReader
path and retry using it; do not rely solely on ResultBackend.resolved(), which
only checks availability.
- Around line 2132-2147: Thread SummaryOptions.resultReader through the
XCTestHtmlReport Summary construction so the selected backend is not ignored.
Resolve the backend with precedence explicit CLI option, then
XCHR_RESULT_READER, then auto, and pass that resolved value to Summary without
allowing the environment to override an explicit option. Add tests covering both
explicit-over-environment and environment-over-default precedence.
- Around line 1411-1433: Update ModernResultReader.read() to create a ParsedRun
for every destination in tests.devices rather than selecting
tests.devices?.first, preserving each destination’s associated testables. Before
any legacy/modern run comparison using zip, assert that legacy.runs.count equals
modern.runs.count so unmatched runs cannot be silently ignored.
- Around line 968-1002: Update run(_:) and legacyCommandsAvailable to execute
xcresulttool through a bounded Swift 5.5/macOS 10.15-compatible helper that
enforces a timeout, terminates timed-out processes, closes both stdout and
stderr pipe handles, and always reaps the process. Preserve normal output and
executionFailed behavior, but map an availability-probe timeout in
legacyCommandsAvailable to an unavailable backend. Add tests covering subprocess
timeout handling and the availability mapping.
- Around line 1172-1184: Update parseGroup and parseTestCase to preserve
TestNode entries whose nodeType is “Failure Message,” including their
details/text in ParsedActivity so RetryResults retains failure diagnostics;
alternatively, add a captured-response assertion proving activities exposes
equivalent message text.
In `@docs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md`:
- Line 66: Update the four fenced code blocks in the document, including the
blocks near the referenced sections, to use the text language identifier
(```text) instead of unlabeled fences. Preserve their existing contents.
🪄 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: e63391c0-9311-4111-8cbe-f19d69052803
📒 Files selected for processing (2)
docs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.mddocs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md
The differential test was the weak point. It only checked that declared markers appeared on legacy and vanished on modern, which says nothing about lines nobody declared — an undeclared regression would have passed. Checking the marker strings against HTMLTemplates.swift also showed three of them did not exist: activity durations render as a bare "(0.00s)" suffix and attachment names as bare text, with no class to key on. Replaced with masking: strip exactly the declared losses from both renders and require what remains to be byte-identical. That is implementable against the real markup and is a stronger claim than the marker match ever was. Each rule maps 1:1 to an allow-list entry, and the masked comparison asserts every test title survives masking so an over-broad mask cannot make it pass vacuously. Baseline capture no longer skips missing fixtures. It previously continued past one, which would let Task 5 diff two partial directories and report them identical — the same vacuous-verification shape. Also from review: the attachment export leaked its temp directory (a full copy of every screen recording) on every run; attachment comparison used Set<Data>, which collapses duplicates; testNodes and testRuns were non-optional and would throw keyNotFound on a bundle where xcresulttool omits the key; a failed activities query returned an empty list with no fault, so a visibly gutted report would exit 0; and Task 5 quoted a stale expected test count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three more from review. The --result-reader option was declared on SummaryOptions but never passed into Summary, so it would have parsed, validated, and done nothing; the end-to-end check now renders through both readers and requires the wrapper-group counts to differ, which fails if the flag is not reaching Summary. ResultBackend.resolved() only consulted the version string, so an explicit --result-reader legacy on a post-removal toolchain would have selected a backend that cannot work. It now demotes to modern with a warning, which is the degradation rule the spec already stated. The accompanying test is conditional on toolchain capability rather than asserting .legacy -> .legacy outright, since that assertion would start failing exactly when the fallback becomes load-bearing. The spec listed read() as throwing while the plan returned an optional. Settled on the optional, matching the existing getInvocationRecord() contract that Summary.init already guards with a fault, and documented why the sub-level failures are the ones that needed a new fault kind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Worked through CodeRabbit's 16 comments. Addressed 13, skipped 2 with reasons, 1 was a duplicate. The one that mattered most: the differential test didn't do what it claimed. It only checked that declared markers appeared on legacy and vanished on modern. That says nothing about lines nobody declared, so an undeclared regression would have passed — the exact vacuous-verification shape this repo has been bitten by before, in the central check of the whole migration. Checking the marker strings against Replaced with masking: strip exactly the declared losses from both renders, then require what remains to be byte-identical. That is implementable against the real markup and is a strictly stronger claim than the marker match. Each rule maps 1:1 to an allow-list entry, an entry naming a rule with no implementation fails Also fixed
Skipped
|
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 `@docs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md`:
- Around line 138-145: Update the design around ModernResultReader and
Summary.init to explicitly require recording .missingActivities on the passed
FaultCollector when the activities query fails before returning an empty list.
Add an integration test verifying the fault appears in summary.faults and causes
the expected CLI degradation, and describe Summary.validate() only as checking
unresolved attachments, not producing this fault.
🪄 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: 0b67ee49-7042-499f-b35a-2c26530f73ce
📒 Files selected for processing (2)
docs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.mddocs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md
The plan recorded .missingActivities when an activities query fails, but nothing asserted the fault reaches summary.faults. Since a failed query degrades to an empty activity list rather than aborting, unproven plumbing means the CLI could exit 0 on a report whose tests have no activities — the exact outcome the fault exists to prevent. Added an XCResultToolInvoking seam so a client that fails only on `activities` can be injected, and a test that reads through it and asserts both halves: the read still succeeds, and the fault lands on the caller's collector. The collector is the one Summary.init owns, so the existing exit-3 path covers the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three more from review, two of them substantive. The modern reader was sourcing failure text from the activities document. The tests tree's Failure Message nodes are strictly better: measured on TestResults, they give "FirstSuite.swift:66: XCTAssertTrue failed - Test failed" where the activity title gives only "XCTAssertTrue failed - Test failed". The plan was reading from the lossier of two available sources and the spec described that loss as unavoidable. Both corrected; skip reasons ride the same node. The modern reader also collapsed every destination to devices.first, where legacy emits one run per ActionRecord. Every fixture boots a single simulator, so no test would have caught the difference — and the differential's zip() truncates to the shorter sequence, so it would have compared the runs that did exist and passed. Now one run per device, with an explicit run-count assertion before the zip, and the coverage gap stated in both documents rather than implied to be tested. Also: the legacy-capability probe never drained stderr, which deadlocks if the pipe fills — the same bug the codebase already documents in TestSupport and XCResultToolClient.run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Second review round: 3 further findings, all addressed. Replied inline on all 17 threads. Failure text was coming from the lossier of two sources. The reader sourced failures from
The activities path drops file and line entirely — and the spec had described that as an unavoidable loss. It isn't. Now reads Multi-destination collapsed to one run, and the differential would have hidden it. The reader used Being explicit about the limit: every fixture boots a single simulator, so this path is not covered by the suite. The mapping is written from the format, not verified against a two-destination bundle. Both documents say so rather than implying otherwise. The capability probe could deadlock. Declined, with reasoning (also inline):
Across both rounds the pattern was consistent, and worth naming: the findings that mattered weren't bugs in the design, they were verification steps that would have passed on wrong input — a differential that didn't diff, a baseline that tolerated missing fixtures, a flag whose test passed without the flag working, a |
1 participant
Planning only for #391 — no implementation. The strategy choice is not cheaply reversible, so this lands the decision and its evidence for review before any code is written.
Closes nothing; #391 stays open for the implementation.
Decision
Read the new
xcresulttoolformat directly and drop XCResultKit, behind a reader abstraction that keeps the legacy path alive until Apple removes it.Under all three options in the issue we have to write a new-format reader — XCResultKit's public API is the legacy object graph, so there is no version of this where we swap a flag. Once that reader exists the 3,549-line dependency is dead weight, which makes the direct read strictly less work than vendoring or upstreaming for the same outcome.
The finding that reframed it
The new format is not a superset of the legacy one. Measured on freshly generated fixtures under Xcode 26.2, not inferred:
activityType(5 constants → CSS classes)isAssociatedWithFailurestartandfinishstartTimeonlyname(user-supplied)uniformTypeIdentifierActionTestFailureSummaryfile/line/issueTypeemittedOutputexportRecursiveJson()(drives--json)nodeType: "Repetition"— improvedSo byte-identical output across the two backends is not achievable. The bar is a declared, CI-asserted diff, not an empty one.
Two measurements that shaped the plan
Reports are not byte-reproducible. Rendering
SanityResultstwice with today's binary gives 18 differing lines, all synthesized UUIDs — byte-identical after one regex. Any diff harness must normalize first; without that step it reports a difference every run and proves nothing. The plan builds the normalizer as task 1 and pins it with a test that first asserts the raw renders differ, so the normalized comparison can't pass vacuously.Performance is a non-issue, and I'm claiming nothing. Same-runner interleaved A/B, 5 reps alternating per rep: legacy 27 spawns / 0.74s median, modern 20 spawns / 0.76s median. Equal within noise on one machine and one bundle size. Measured only to rule out the modern path being dramatically worse because
activitiesis per-test-id. Not a speedup, and not comparable to numbers from another runner.The parity trap worth reviewing closely
A Test Case node in the new format carries its own
result, which is not the legacy status:Reading the parent's
resultturns a mixed test green. The plan derives multi-repetition status from the children and ignores the parent, andRetryResultscovers it.Second trap:
FaultCollectorturns degradation into exit 3, so if the modern backend's structural nils were recorded as faults, every run would exit non-zero. Called out in the spec and asserted in task 11.Decisions taken (yours, recorded here)
Selected tests/*.xctestwrappers was rejected because one of those two labels would always be fabricated--jsonmoves to our own documented schema, identically on both backends — a breaking change landing in 3.0What's here
docs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md— decision, evidence, field-by-field mapping, parity rules, verification design, risksdocs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.md— 15 tasks, each ending in a green test run and a commitRemoving XCResultKit is deliberately not in scope: while the legacy commands exist, the differential is the only continuous evidence the modern reader is right, and deleting it early throws that away. Tracked as a follow-up gated on Apple actually removing them.
Verification
Docs-only diff. Baseline confirmed green on this branch before writing:
swift test→ 23 tests, 1 skipped, 0 failures. Every command, JSON shape, and number quoted above was executed against real fixtures in this worktree.🤖 Generated with Claude Code
Summary by CodeRabbit
xcresulttoolprocessing.