THE DIFFERENTIAL: legacy/modern parity held to the declared allow-list + forced-modern CI leg (#391, Tasks 12–13) - #450
Merged
Conversation
… race Running both readers over freshly generated fixtures surfaced divergences the spec's tables did not anticipate. Each lands here as a rule, recorded in the spec's "Task 12 execution rules" section; none adds an allow-list entry. - Content-addressed attachment exports: both backends name every exported payload <payloadId>.<ext> — the one attachment identifier the two formats share. Xcode 26.2 gives every auto screen recording in a session one shared display name, so name-keyed exports collapsed distinct payloads onto one path and raced concurrent copies (#449, reproduced with a live EEXIST trace and a 1-in-12 test flake). The export is now idempotent: an existing destination is the payload, never removed. - Symbol-annotation rows (no startTime) and attachment-shadow rows (leaf, non-failure, startTime == sibling attachment timestamp) are dropped by the modern reader; both are 26.2 bookkeeping the legacy tree never had. The shadow join's guards are load-bearing: a genuine failure row shares the attachment's millisecond on testWithSpecialChars(). - Swift Testing names come from the identifier's function form on both backends; the @test display name is a field only one backend can fill. - Legacy merges parameterized argument executions (duplicate siblings with no repetitionPolicySummary) into one iteration; true retries keep their numbers. - Expected failures are non-events on both backends: messages claim and remove their exact-title activity rows instead of joining. - Skip notices were a legacy reader gap: the reason was always on skipNoticeSummary; it now renders the same appended row modern emits. - Failure-row placement goes through one shared total-ordered interleave (ParsedActivity.interleavingFailureRows) fed by both readers; the modern reader hoists failure tips (isFailure now means "is the assertion row" — tip of the flagged chain). Re-nesting legacy rows by time window was tested and rejected: windows collide at millisecond granularity and misplace rows. - Run logs export as <run-identifier-digest>.log on both backends instead of backend-internal reference names. Refs #391. Fixes #449. Co-Authored-By: Claude Fable 5 <[email protected]>
…w-list Renders each fixture through both readers, forced explicitly, and asserts (Task 12): per-run counts, the identifier→status map, and exported attachment filenames AND bytes match exactly; after normalizing identifier digests (#430's [0-9a-f]{32}, .linking renders only) and masking exactly the four declared losses, the two renders are byte-identical. Skips loudly when the toolchain has no legacy commands, and proves the forced backend actually resolved rather than assuming it. Anti-rot runs in both directions: a diff outside the allow-list fails the build, and an entry that masks nothing real also fails — "fires" means omitting the rule (others still applied) leaves the renders unequal, so an entry cannot rot silently once Apple fills the gap. All four entries fire: durations on all three fixtures, wrapperGroups on all three, failureTitlePrefix and attachmentDisplayNames on TestResults and RetryResults. The masker diverged from the plan's snippets in four evidence-forced ways, recorded in the plan's Task 12 execution note: wrapperGroups is a structural SwiftSoup unwrap (line-filtering left the wrapper's div skeleton behind), the display-name anchor spans the icon line the plan's regex tripped on, line joins are canonicalized before comparing (template concatenation breaks lines differently across nesting depths), and XCTest-case duration coverage lost to the over-broad durations mask is restored by a model-level assertion. Refs #391. Co-Authored-By: Claude Fable 5 <[email protected]>
One matrix leg runs the whole suite with the modern reader forced, so the path that becomes the only path once Apple removes the legacy commands is exercised end to end on every PR — not only through the differential. The env var is the CI override, not a new control surface: it defaults both Summary.init's backend and the CLI's --result-reader (so CLI-driven tests pick it up through the spawned binary), the flag still wins when passed, and an unrecognised value degrades to auto. Verified locally: 92 tests green under both XCHR_RESULT_READER=modern and =auto. Fixture cache (#436) is shared across legs unchanged; no new action steps. Refs #391. Co-Authored-By: Claude Fable 5 <[email protected]>
…tems Spec: new "Task 12 execution rules (2026-08-12)" section — content-addressed exports (fixes #449), symbol-annotation and attachment-shadow drops, Swift Testing names from the identifier, parameterized-execution merge, expected failures as non-events, skip-notice fix, the shared failure-row interleave with the reversible hoist, and run-log naming — each with the evidence that forced it. The attachment-filename table row is superseded, the Tree-shape display-name paragraph becomes a model rule, and attachmentDisplayNames' Exercised-by cell gains RetryResults (measured). Plan: Task 12 execution note (where the shipped harness supersedes the snippets, and why); Task 15 gains a release-notes checklist so the 4.0 output changes accepted here cannot be missed by the docs task. Refs #391. Co-Authored-By: Claude Fable 5 <[email protected]>
📝 WalkthroughWalkthroughThe change adds environment-based backend selection, aligns legacy and modern result parsing, standardizes log and attachment exports, preserves existing payload files, and adds differential tests with an explicit allow-list. ChangesResult backend and export contracts
Legacy and modern reader behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to The PR changes result parsing and attachment export behavior, but concurrent exports can currently accept truncated files or remove an existing destination during replacement, producing successful reports with missing or corrupted attachments. Merge should be blocked until both export paths are made race-safe. Sequence Diagram(s)sequenceDiagram
participant TestMatrix
participant Summary
participant LegacyResultReader
participant ModernResultReader
participant DifferentialTests
TestMatrix->>Summary: set XCHR_RESULT_READER
Summary->>LegacyResultReader: parse legacy fixture
Summary->>ModernResultReader: parse modern fixture
LegacyResultReader->>DifferentialTests: return normalized summary
ModernResultReader->>DifferentialTests: return normalized summary
DifferentialTests->>DifferentialTests: compare renders, durations, and attachment bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 6
🧹 Nitpick comments (5)
Tests/XCTestHTMLReportTests/ModernReaderRuleTests.swift (1)
199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the temporary bundle copy after the test.
The test copies the fixture and then renders attachments and logs into that copy. Nothing deletes it, so each run leaves a full bundle plus exported payloads in
NSTemporaryDirectory().♻️ Proposed cleanup
try FileManager.default.copyItem(at: source, to: copy) + defer { + try? FileManager.default.removeItem(at: copy.deletingLastPathComponent()) + }🤖 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/ModernReaderRuleTests.swift` around lines 199 - 205, Remove the temporary bundle copy created in ModernReaderRuleTests after the test completes, including when assertions or rendering fail. Use the existing copy URL and ensure cleanup runs after attachments and logs are rendered, without deleting the original fixture.Tests/XCTestHTMLReportTests/KnownLossMasker.swift (1)
101-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAnchor the
failureTitlePrefixregexes to the failure-title element.Both patterns match anywhere in the document.
[A-Za-z ]+ at [^\s:]+:\d+:\s*matches ordinary prose in test output, and[\w.+-]+\.swift:\d+:\s*matches any Swift source reference, including one inside a log line or an activity title. A cross-backend divergence in those rows is then masked, and the byte-identity guarantee the allow-list rests on weakens silently.The
durationsrule documents its own over-breadth and namestestXCTestCaseDurationsAgreeAcrossBackendsas the compensating assertion. Apply the same discipline here: scope the pattern to the failure-title element, or add a compensating test that pins failure text on one backend.🤖 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/KnownLossMasker.swift` around lines 101 - 113, Restrict both `failureTitlePrefix` replacement patterns in `KnownLossMasker` to the failure-title element instead of matching anywhere in the HTML document. Preserve removal of both legacy and modern prefixes while ensuring ordinary prose, log lines, and activity titles remain unchanged; alternatively, add a focused compensating test that verifies backend-specific failure text is not masked outside that element.Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader.swift (1)
275-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two
startTimechecks into one binding.Line 275 already returns for a
nilstartTime, so the optional binding on Line 278 always succeeds. One binding states the two drop rules more directly.♻️ Proposed simplification
- if child.startTime == nil { + guard let start = child.startTime else { return nil // symbol annotation } - if let start = child.startTime, attachmentTimes.contains(start) { + if attachmentTimes.contains(start) { return nil // attachment shadow } return parsed🤖 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 `@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader.swift` around lines 275 - 280, In the child filtering logic, combine the nil check and attachment-time check into a single optional binding around child.startTime. Return nil when the bound start time is present in attachmentTimes, while preserving the existing behavior of dropping children with nil startTime.Tests/XCTestHTMLReportTests/ModernPayloadStoreTests.swift (1)
92-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a concurrent export assertion to cover the race the fix targets.
The test exports twice in sequence. The reported fault in
#449came from concurrent exports of one destination.RunandTestSummarybuild attachments on anOperationQueue, so the concurrent path is the one that regressed. Assert it directly.♻️ Proposed additional assertion
XCTAssertTrue( collector.faults.isEmpty, "Re-exporting an already-exported payload is success, not degradation" ) + + // The reported fault came from concurrent exports of one destination. + let concurrent = FaultCollector() + let (parallelStore, parallelBundle) = try store( + for: "TestResults", collector: concurrent + ) + let parallelUUID = try firstAttachmentUUID(in: parallelBundle) + DispatchQueue.concurrentPerform(iterations: 16) { _ in + XCTAssertNotNil( + parallelStore.exportPayload( + reference: parallelUUID, fileName: "0~parallel=.html" + ) + ) + } + XCTAssertTrue( + concurrent.faults.isEmpty, + "Concurrent exports of one content-addressed destination must not fault" + )🤖 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/ModernPayloadStoreTests.swift` around lines 92 - 115, Extend testExportPayloadIsIdempotentForTheSameDestination to perform two exports of the same reference and fileName concurrently, using the test’s existing concurrency utilities or an appropriate synchronized result collection. Assert both concurrent calls return the same destination, the destination remains valid, and collector.faults stays empty, while preserving the existing sequential idempotency assertions..github/workflows/test.yml (1)
17-26: 🚀 Performance & Scalability | 🔵 TrivialConsider warming the fixture cache before the matrix fans out.
Both legs compute the same fixture cache key. On a cold cache both legs boot a simulator and run
prepareTestResults.sh, and both then save under one key. The duplicate generation dominates job wall time, as the comment at Line 37 records for the single-leg case. A short prerequisite job that populates the cache would let both legs restore instead of regenerate.
[operational]🤖 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 @.github/workflows/test.yml around lines 17 - 26, Update the workflow matrix setup to add a prerequisite cache-warming job that computes the shared fixture cache key, restores it, and runs the fixture-generation flow only on a cache miss before the matrix job starts. Make both matrix legs depend on this warm-up job so they restore the populated cache instead of independently booting simulators and running prepareTestResults.sh; preserve the existing result_reader values and XCHR_RESULT_READER configuration.
🤖 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 451-468: The legacy export path in ResultFile.exportPayload must
preserve an existing content-addressed destination: return the destination when
it already exists and remove the pre-export deletion so existing payload files
are never removed or rewritten. Apply this behavior to the legacy backend,
verify both backends satisfy the documented idempotent contract, then update the
migration documentation.
In
`@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernPayloadStore.swift`:
- Around line 57-70: Update the export logic around the destination existence
check and FileManager.copyItem to record whether the destination existed before
this copy began. In the catch path, remove the destination only when it was
absent beforehand, then propagate the copy failure; preserve pre-existing
destinations without deleting or rewriting them and keep successful exports
unchanged.
In
`@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader`+FailureRows.swift:
- Around line 102-116: Update removeClaimed so a matched expected-failure
activity removes only its row while preserving and returning any attachments or
recursively processed subActivities it contains; continue returning nil only
when the matched activity has no content to retain, and leave unmatched activity
handling unchanged.
In `@Tests/XCTestHTMLReportTests/DifferentialTests.swift`:
- Around line 304-332: Update the differential test around the payloads helper
and legacy/modern summaries to fail when either summary reports export faults,
and require every attachment with a payload reference to resolve as .data rather
than silently skipping .none content. Preserve the existing filename-count and
byte comparisons after these validations.
- Around line 159-165: Update testEveryAllowListRuleIsImplemented to assert
allowList().knownLosses.count equals 4, then validate that the extracted rule
names contain no duplicates before comparing them with
KnownLossMasker.implementedRules. Preserve the existing equality assertion for
the complete rule set.
- Around line 110-150: Replace the aggregate statuses(_:) comparison with
per-run identifier-to-status comparisons after the run-count assertion, pairing
corresponding legacy and modern runs so duplicate test identifiers from
different destinations remain distinct. Preserve the existing status assertion
message and add a multi-destination fixture that exercises this behavior,
accounting for ModernResultReader producing one ParsedRun per reported device.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 17-26: Update the workflow matrix setup to add a prerequisite
cache-warming job that computes the shared fixture cache key, restores it, and
runs the fixture-generation flow only on a cache miss before the matrix job
starts. Make both matrix legs depend on this warm-up job so they restore the
populated cache instead of independently booting simulators and running
prepareTestResults.sh; preserve the existing result_reader values and
XCHR_RESULT_READER configuration.
In
`@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader.swift`:
- Around line 275-280: In the child filtering logic, combine the nil check and
attachment-time check into a single optional binding around child.startTime.
Return nil when the bound start time is present in attachmentTimes, while
preserving the existing behavior of dropping children with nil startTime.
In `@Tests/XCTestHTMLReportTests/KnownLossMasker.swift`:
- Around line 101-113: Restrict both `failureTitlePrefix` replacement patterns
in `KnownLossMasker` to the failure-title element instead of matching anywhere
in the HTML document. Preserve removal of both legacy and modern prefixes while
ensuring ordinary prose, log lines, and activity titles remain unchanged;
alternatively, add a focused compensating test that verifies backend-specific
failure text is not masked outside that element.
In `@Tests/XCTestHTMLReportTests/ModernPayloadStoreTests.swift`:
- Around line 92-115: Extend testExportPayloadIsIdempotentForTheSameDestination
to perform two exports of the same reference and fileName concurrently, using
the test’s existing concurrency utilities or an appropriate synchronized result
collection. Assert both concurrent calls return the same destination, the
destination remains valid, and collector.faults stays empty, while preserving
the existing sequential idempotency assertions.
In `@Tests/XCTestHTMLReportTests/ModernReaderRuleTests.swift`:
- Around line 199-205: Remove the temporary bundle copy created in
ModernReaderRuleTests after the test completes, including when assertions or
rendering fail. Use the existing copy URL and ensure cleanup runs after
attachments and logs are rendered, without deleting the original fixture.
🪄 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: db4c8ed3-4083-488f-bf33-27ccd918eb85
📒 Files selected for processing (24)
.github/workflows/test.ymlPackage.swiftSources/XCTestHTMLReport/XCTestHtmlReport.swiftSources/XCTestHTMLReportCore/Classes/Models/Run.swiftSources/XCTestHTMLReportCore/Classes/Models/Summary.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/Legacy/LegacyResultReader.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/Legacy/ResultFile.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernPayloadStore.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader+FailureRows.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/ParsedResult.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/ResultBackend.swiftSources/XCTestHTMLReportCore/Classes/ResultReading/ResultReader.swiftTests/XCTestHTMLReportTests/DifferentialTests.swiftTests/XCTestHTMLReportTests/FaultReportingTests.swiftTests/XCTestHTMLReportTests/KnownLossMasker.swiftTests/XCTestHTMLReportTests/ModernPayloadStoreTests.swiftTests/XCTestHTMLReportTests/ModernReaderRuleTests.swiftTests/XCTestHTMLReportTests/ModernResultReaderTests.swiftTests/XCTestHTMLReportTests/PortRuleTests.swiftTests/XCTestHTMLReportTests/Resources/differential-allowlist.jsonTests/XCTestHTMLReportTests/ResultBackendTests.swiftdocs/superpowers/plans/2026-08-10-xcresulttool-legacy-migration.mddocs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md
Comment on lines
+451
to
+468
| **Content-addressed attachment exports (fixes #449).** Xcode 26.2 gives every | ||
| auto screen recording in a session one shared display name | ||
| (`Screen Recording <timestamp>.mp4` — in the activities document *and* the | ||
| manifest's `suggestedHumanReadableName`), and the legacy pretty filename | ||
| embeds a legacy-only uuid the modern format cannot see. Naming exports after | ||
| either backend's pretty name therefore broke both ways at once: distinct | ||
| payloads collapsed onto one path (every video row played the same recording), | ||
| concurrent copies raced on it and intermittently recorded a spurious | ||
| `.payloadExportFailed` — root-caused as #449 with a live `EEXIST` trace whose | ||
| `fileExists` recovery check lost to a third thread's `removeItem`. The one | ||
| attachment identifier both formats share is the content-addressed payload id | ||
| (legacy `payloadRef.id` == modern `payloadId`, the same CAS id), so both | ||
| backends name every exported payload | ||
| `ParsedAttachment.exportFileName(payloadId:filenameExtension:)`. Names agree | ||
| by construction, the export is idempotent (a file already at the destination | ||
| *is* the payload — never removed, never rewritten), and byte-identical | ||
| payloads deduplicate to one file. 4.0 release note: on-disk attachment names | ||
| change for every user; display names in the report do not. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make legacy payload exports preserve existing destinations.
This section states that both backends never remove or rewrite an existing payload file. ResultFile.exportPayload still removes its destination before moving the legacy export. A concurrent report process can observe that deletion and reintroduce the export race that this rule documents as fixed.
Return the existing destination for a content-addressed filename. Do not remove it before export. Update the documentation only after both backends meet this contract.
🤖 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 `@docs/superpowers/specs/2026-08-10-xcresulttool-legacy-migration-design.md`
around lines 451 - 468, The legacy export path in ResultFile.exportPayload must
preserve an existing content-addressed destination: return the destination when
it already exists and remove the pre-export deletion so existing payload files
are never removed or rewritten. Apply this behavior to the legacy backend,
verify both backends satisfy the documented idempotent contract, then update the
migration documentation.
Comment on lines
+57
to
70
| // Destinations are content-addressed (`ParsedAttachment.exportFileName` | ||
| // names them by payload id), so a file already at the destination *is* | ||
| // this payload and the export is idempotent: never remove, never | ||
| // rewrite. The predecessor removed-then-copied, which under Xcode | ||
| // 26.2's shared screen-recording display names raced concurrent | ||
| // exports on one path and intermittently recorded a spurious | ||
| // `.payloadExportFailed` (#449). | ||
| if FileManager.default.fileExists(atPath: destination.path) { | ||
| return relativeURL.appendingPathComponent(resolved) | ||
| } | ||
| do { | ||
| try? FileManager.default.removeItem(at: destination) | ||
| try FileManager.default.copyItem(at: source, to: destination) | ||
| return relativeURL.appendingPathComponent(resolved) | ||
| } catch { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Distinguish a destination this copy created from one another writer created.
The new short circuit removes the previous remove-before-copy step. FileManager.copyItem can fail after it creates the destination and writes part of the payload. The catch arm then finds the destination and reports success, so the report links a truncated attachment and the CLI still exits 0. The next export of the same name also short circuits at Line 64, so the truncated file is never repaired.
Record whether the destination existed before the copy, and delete a destination that only this failed copy created.
🛡️ Proposed guard for the failed-copy case
if FileManager.default.fileExists(atPath: destination.path) {
return relativeURL.appendingPathComponent(resolved)
}
do {
try FileManager.default.copyItem(at: source, to: destination)
return relativeURL.appendingPathComponent(resolved)
} catch {
// A concurrent writer of the same payload can still beat us to the
// creation; its bytes are our bytes, so losing that race is
// success, not degradation.
+ // A destination this failed copy created itself is partial, not a
+ // rival's complete payload, so it must not be trusted.
+ if let sourceSize = try? FileManager.default
+ .attributesOfItem(atPath: source.path)[.size] as? Int,
+ let destinationSize = try? FileManager.default
+ .attributesOfItem(atPath: destination.path)[.size] as? Int,
+ sourceSize != destinationSize
+ {
+ try? FileManager.default.removeItem(at: destination)
+ }🤖 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
`@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernPayloadStore.swift`
around lines 57 - 70, Update the export logic around the destination existence
check and FileManager.copyItem to record whether the destination existed before
this copy began. In the catch path, remove the destination only when it was
absent beforehand, then propagate the copy failure; preserve pre-existing
destinations without deleting or rewriting them and keep successful exports
unchanged.
Comment on lines
+102
to
+116
| func removeClaimed(_ activities: [ParsedActivity]) -> [ParsedActivity] { | ||
| activities.compactMap { activity in | ||
| if let index = unclaimed.firstIndex(of: activity.title) { | ||
| unclaimed.remove(at: index) | ||
| return nil | ||
| } | ||
| return ParsedActivity( | ||
| title: activity.title, | ||
| isFailure: activity.isFailure, | ||
| start: activity.start, | ||
| attachments: activity.attachments, | ||
| subActivities: removeClaimed(activity.subActivities) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Removal of a matched expected-failure row also drops its attachments and children.
removeClaimed returns nil for the matched activity, so its attachments and subActivities disappear with it. The documented intent is to remove the expected-failure row, not user content that happens to hang below it. Keep the content when the matched row carries any.
♻️ Proposed guard
activities.compactMap { activity in
- if let index = unclaimed.firstIndex(of: activity.title) {
+ if let index = unclaimed.firstIndex(of: activity.title),
+ activity.attachments.isEmpty, activity.subActivities.isEmpty
+ {
unclaimed.remove(at: index)
return nil
}🤖 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
`@Sources/XCTestHTMLReportCore/Classes/ResultReading/Modern/ModernResultReader`+FailureRows.swift
around lines 102 - 116, Update removeClaimed so a matched expected-failure
activity removes only its row while preserving and returning any attachments or
recursively processed subActivities it contains; continue returning nil only
when the matched activity has no content to retain, and leave unmatched activity
handling unchanged.
Comment on lines
+110
to
+150
| func statuses(_ summary: Summary) -> [String: String] { | ||
| Dictionary( | ||
| summary.runs.flatMap(\.allTests) | ||
| .map { ($0.identifier, $0.status.rawValue) }, | ||
| uniquingKeysWith: { first, _ in first } | ||
| ) | ||
| } | ||
| XCTAssertEqual( | ||
| statuses(legacy), statuses(modern), | ||
| "\(fixture): identifier→status differs between backends" | ||
| ) | ||
|
|
||
| // Assert before zipping: `zip` truncates to the shorter sequence, | ||
| // so a backend that produced fewer runs would compare equal on the | ||
| // ones it did produce and pass. | ||
| XCTAssertEqual( | ||
| legacy.runs.count, modern.runs.count, | ||
| "\(fixture): backends disagree on the number of runs" | ||
| ) | ||
|
|
||
| for (legacyRun, modernRun) in zip(legacy.runs, modern.runs) { | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfTests, modernRun.numberOfTests, "\(fixture): total" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfPassedTests, modernRun.numberOfPassedTests, | ||
| "\(fixture): passed" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfFailedTests, modernRun.numberOfFailedTests, | ||
| "\(fixture): failed" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfSkippedTests, modernRun.numberOfSkippedTests, | ||
| "\(fixture): skipped" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyRun.numberOfMixedTests, modernRun.numberOfMixedTests, | ||
| "\(fixture): mixed" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Compare statuses within each run.
statuses(_:) keys only on Test.identifier. The same test identifier occurs on every destination. uniquingKeysWith: { first, _ in first } discards later-device statuses.
Compare identifier-to-status mappings per paired run after the run-count assertion, or include a stable destination key. Add a multi-destination fixture so this contract cannot regress.
Based on learnings: ModernResultReader creates one ParsedRun per reported device, while current fixtures boot only one simulator.
🤖 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/DifferentialTests.swift` around lines 110 - 150,
Replace the aggregate statuses(_:) comparison with per-run identifier-to-status
comparisons after the run-count assertion, pairing corresponding legacy and
modern runs so duplicate test identifiers from different destinations remain
distinct. Preserve the existing status assertion message and add a
multi-destination fixture that exercises this behavior, accounting for
ModernResultReader producing one ParsedRun per reported device.
Source: Learnings
Comment on lines
+159
to
+165
| func testEveryAllowListRuleIsImplemented() throws { | ||
| let declared = try Set(allowList().knownLosses.map(\.rule)) | ||
| XCTAssertEqual( | ||
| declared, KnownLossMasker.implementedRules, | ||
| "The allow-list and KnownLossMasker.implementedRules disagree. " | ||
| + "Every declared rule needs an implementation and vice versa." | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the required allow-list size.
This test verifies only that declared rules equal implemented rules. A fifth declared rule and masker implementation would pass, despite the requirement for exactly four allow-list entries.
Assert that knownLosses.count is 4, and reject duplicate rule names before comparing the rule set.
🤖 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/DifferentialTests.swift` around lines 159 - 165,
Update testEveryAllowListRuleIsImplemented to assert
allowList().knownLosses.count equals 4, then validate that the extracted rule
names contain no duplicates before comparing them with
KnownLossMasker.implementedRules. Preserve the existing equality assertion for
the complete rule set.
Comment on lines
+304
to
+332
| func payloads(_ summary: Summary) -> [String: [Data]] { | ||
| var byName: [String: [Data]] = [:] | ||
| for attachment in summary.allAttachments { | ||
| guard case let .data(data) = attachment.content else { | ||
| continue | ||
| } | ||
| byName[attachment.filename, default: []].append(data) | ||
| } | ||
| return byName.mapValues { $0.sorted { $0.count < $1.count } } | ||
| } | ||
|
|
||
| for fixture in Self.fixtures { | ||
| // Rendered inline so the bytes are in hand rather than on disk. | ||
| let legacy = try summaryInline(fixture, .legacy) | ||
| let modern = try summaryInline(fixture, .modern) | ||
| let legacyPayloads = payloads(legacy) | ||
| XCTAssertFalse( | ||
| legacyPayloads.isEmpty, | ||
| "\(fixture): no attachment bytes to compare — the assertion " | ||
| + "below would pass vacuously" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyPayloads.mapValues(\.count), | ||
| payloads(modern).mapValues(\.count), | ||
| "\(fixture): attachment counts differ between backends" | ||
| ) | ||
| XCTAssertEqual( | ||
| legacyPayloads, payloads(modern), | ||
| "\(fixture): attachment bytes differ between backends" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail when an attachment payload does not resolve.
The guard case let .data(data) branch skips .none content. If one attachment fails to export on both backends, this comparison can still pass because neither its filename nor its missing bytes enter either map.
Assert that both summaries have no export faults. Also require every attachment with a payload reference to resolve to data before comparing filename counts and byte collections.
🤖 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/DifferentialTests.swift` around lines 304 - 332,
Update the differential test around the payloads helper and legacy/modern
summaries to fail when either summary reports export faults, and require every
attachment with a payload reference to resolve as .data rather than silently
skipping .none content. Preserve the existing filename-count and byte
comparisons after these validations.
tylervick
added a commit
that referenced
this pull request
Aug 13, 2026
…elease notes (#391, Tasks 14–15) (#451) * feat!: emit our own schema from --json instead of the legacy object graph BREAKING: --json previously dumped xcresulttool's legacy object graph verbatim. That graph is Apple's internal shape and disappears with the legacy commands, so --json now emits a documented schema, identical on both backends. docs/json-schema.md is the contract, written before the encoder: field names and nesting with a complete worked example, enum spellings, one uniform null rule, duration and timestamp formats, ordering guarantees, and a semver schemaVersion policy. The encoder (JsonReport.swift) is an explicit layer rather than a synthesized Encodable on the internal model, so renaming a Parsed* property breaks compilation instead of silently renaming public output. ResultFile.exportJson() — the last exportRecursiveJson() call, kept since Task 5a — is deleted; XCResultKit is confined to ResultReading/Legacy/. JsonReportTests holds the output to the contract across both backends: recursive schema identity, values deeply equal outside the two permitted difference classes (JsonClassMask masks exactly the declared losses, with non-vacuity stats), and arguments compared per class 2 — legacy asserted == [] explicitly, modern asserted non-empty for the parameterized fixture. The value differential surfaced one reader-parity gap the HTML differential cannot see: under a retry-enabled plan, legacy stamps every summary "iteration 1" while modern reports repetition info only for real repetitions, and nothing renders a lone iteration number. The legacy reader now strips it (strippingLoneIterationNumber) — a single execution carries no repetition information on either backend. Recorded as a Task 14 execution rule in the spec. Refs #391 (Task 14). Co-Authored-By: Claude Fable 5 <[email protected]> * fix: fold in the carried review findings from #447 and #450 - Pin the no-startTime keep-guard (#450 review): a no-start activity row carrying an attachment, a failure flag, or surviving children is kept by the symbol-annotation drop; only the contentless row is an annotation. Crafted-document test, since no fixture produces the shape. - Suffix the failure-artifact name per matrix leg in test.yml (#450 review): upload-artifact v4 refuses duplicate names, so a run where both legs fail would have lost the second upload. - ModernPayloadStore takes XCResultToolInvoking instead of the concrete client (#447 review), so export-failure faulting is provable in-suite; the new test also pins that a failed one-shot export no longer leaks its temp directory (removed in the catch — deinit never saw it). - An out-of-range repetition index no longer falls back silently to runs.first (#447 review): the iteration renders no activities and records .missingActivities, instead of borrowing repetition 1's rows. Refs #391. Co-Authored-By: Claude Fable 5 <[email protected]> * docs: document --result-reader and the --json schema change README gains the --result-reader section, the --json break with a before/after snippet, and the known backend differences drawn from the allow-list. The 4.0 release notes are drafted at docs/release-notes/4.0.0.md in the 3.0.0 narrative style for the release to source, covering the full Task 15 checklist including the items added by rulings R1/R5/R7 and the #443/#450 reviews. The spec gains a status header (phases 1-5 landed, phase 6 deliberately deferred) and the Task 14 execution rules; the plan's Tasks 14-15 are ticked with implementation amendments recorded. Refs #391 (Task 15). Co-Authored-By: Claude Fable 5 <[email protected]> * docs: state schema identity, not value identity, in the README The --json section claimed the file is identical across readers; the contract's own headline is schema identity, with declared value differences. Point at those and at testCase.arguments, the modern-only capability. Addresses the valid half of CodeRabbit's review on #451. Refs #391. Co-Authored-By: Claude Fable 5 <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]>
This was referenced Aug 14, 2026
This was referenced Aug 14, 2026
Merged
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.
THE DIFFERENTIAL — the test this migration exists to pass, landed while both backends still exist to compare. Renders every fixture through both readers, asserts structural equality exactly, and holds the HTML diff to the four declared allow-list entries — nothing more, nothing less, enforced in both directions. Plus the forced-
modernCI leg so both paths run on every PR (Tasks 12–13 of the migration plan).Fixes #449. Refs #391. Milestone 4.0.
Differential results (Xcode 26.2 17C52, xcresulttool 24514, fresh fixture generation)
swift test --filter DifferentialTests— all 6 tests pass:[0-9a-f]{32}per #430)Allow-list anti-rot (necessity probe: a rule "fires" only if omitting it leaves the renders unequal):
durationsattachmentDisplayNameskXCTAttachmentScreenRecordingvsVideo)failureTitlePrefixwrapperGroupsAll four entries fire; a diff outside the list fails the build; an entry that fires nowhere also fails (
testEveryAllowListEntryStillMasksARealDivergence), so entries cannot rot once Apple fills a gap. The list stands at exactly the spec's four entries — the first differential run added none.#449: reproduced, root-caused, fixed
testModernRenderOfEveryFixtureRecordsNoFaultson a fresh RetryResults generation; two of five CLI renders degraded (exit 3).Screen Recording <timestamp>.mp4— verified in both the activities document and the manifest'ssuggestedHumanReadableName).ModernPayloadStoreused that name as its copy destination, so distinct payloads collapsed onto one path and concurrent test-case builds raced remove/copy on it. Live trace captured:copyItemthrewEEXIST, and by the time thefileExistsrecovery check ran, a third thread'sremoveItemhad deleted the file again → spurious.payloadExportFailed+unresolvedAttachment→ exit 3. Generation-variant because the collision needs same-second recording names; CI's Xcode 26.6 names differ.testExportPayloadIsIdempotentForTheSameDestination; the no-faults test now passes consistently.The rulings (all coordinator-approved; spec's new "Task 12 execution rules" section)
The first real differential run surfaced divergences the spec's tables didn't anticipate. Every resolution follows the settled doctrine — agree by construction over mask; never model what only one backend can fill:
<payloadId>.<ext>via one shared helper. Filenames agree by construction; byte-identical payloads deduplicate. Renderedsrc/datalinks resolve on disk with~/=in names (testLinkingModeSourcesResolveToExportedFiles).closure #1 in …) under failures; they are exactly the rows with nostartTime. Kept if they ever carry attachments/failure flags/children.testWithSpecialChars()— pinned by test.@Testdisplay name is a field only one backend can fill (spec's Tree-shape paragraph amended to a model rule).repetitionPolicySummary) into one iteration, durations summed. True retries keep numbers —RetryResultspins[1, 2]+.mixed.testInUnknownState().<run-identifier-digest>.logon both backends (the fix: derive report identifiers from bundle content so runs are reproducible #430 id scheme) instead of backend-internal names (<CAS id>.logvsaction.log); unique per action, so multi-action bundles can't last-writer-win (asserted by comment — no fixture has two actions).[start, finish]window was tested and rejected on evidence: at millisecond granularitySecondSuite's failure timestamp sits insideSet Up's window andThirdSuite's bare top-level assertion sits inside a sibling's window — the heuristic reconstruction the spec bans. Instead:ParsedActivity.isFailurenow means "this row IS the assertion row" (tip of the flagged chain), and the modern reader hoists tips intoParsedActivity.interleavingFailureRows— the same total-ordered function the legacy reader feeds ((start, activities-before-failures-on-tie, source index); nil-start rows sort last; totality unit-pinned after The port: ParsedResult model, LegacyResultReader, and the renderer migration (#391, Tasks 3–5b) #443's non-total-comparator history). The hoist is documented as a reversible scaffold constraint: when legacy dies, native nesting may return.skipNoticeSummary; the reader never read it. Both backends now render the identical appended row.Task 13: forced-modern CI leg
test.ymlruns aresult_reader: [auto, modern]matrix (fail-fast: false), threadingXCHR_RESULT_READERthrough the env; reuses the ci: cache generated test fixtures keyed on toolchain and inputs #436 fixture cache unchanged. No new action steps (nothing new to SHA-pin); zizmor: no findings; actionlint: clean.ResultBackend.fromEnvironment()defaults bothSummary.initand the CLI's--result-reader, soswift testand CLI-driven tests both honor the leg; flag still wins when passed. Unset/garbage →.auto(unit-pinned).Suite results (local, this generation)
swift test(auto): 92 tests, 2 skipped, 0 failuresXCHR_RESULT_READER=modern swift test: 92 tests, 2 skipped, 0 failuresXCHR_RESULT_READER=auto swift test: 92 tests, 2 skipped, 0 failuresModernResultReadersplit to stay under type-body limits)Docs amended in this PR (rules with reasoning, not changelog notes)
filenametable row superseded; Tree-shape display-name paragraph promoted to a model rule;attachmentDisplayNamesExercised-by cell gains RetryResults.New tests
DifferentialTests(6),PortRuleTests(8: interleave totality incl. the real ms-collision ties, argument-execution merge boundaries, export-name derivation),ModernReaderRuleTests(5: crafted-document tip extraction — multi-tip chains and flagged leaves — shadow-join survivor pin, expected-failure removal pin, content-addressed link resolution), store idempotency, env-var override. ExistingtestNestedRetryFailureIsRetitledInPlaceupdated to the hoist contract (…RetitledAndHoisted).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
XCHR_RESULT_READERenvironment setting.Bug Fixes
Tests