Skip to content

fix: derive report identifiers from bundle content so runs are reproducible - #430

Merged
tylervick merged 1 commit into
mainfrom
tylervick/reproducible-reports-411
Aug 11, 2026
Merged

fix: derive report identifiers from bundle content so runs are reproducible#430
tylervick merged 1 commit into
mainfrom
tylervick/reproducible-reports-411

Conversation

@tylervick

Copy link
Copy Markdown
Member

Fixes #411.

The problem

Five call sites minted a fresh UUID() at parse time for use as DOM ids and JavaScript handles, so the same binary run twice on the same .xcresult produced different HTML. Reproduced before touching anything:

$ diff a/index.html b/index.html | head -2
616c616
<   <ul class="device-info" onclick="selectDevice('95D46FBA-398C-489C-981F-CE7D72A3A993', this);">
---
>   <ul class="device-info" onclick="selectDevice('1F066FF7-7ECF-41F6-90E4-B798C2C7746A', this);">

The fix

Identifiers are now a digest of the element's path through the report, built from the bundle's own content (device identifier, target name, suite and test identifiers) plus enough positional information to keep them unique. IdentifierPath carries the full argument; the short version:

node path component why it is unique among siblings
run bundle index + action index position in the argument list
device device + device identifier one device per run
target summary index + target name position among testable summaries
suite index + suite identifier position among sibling groups
test case case + test identifier TestGroup dedupes its cases on exactly this key
iteration index within the test case assigned after merging and sorting

Components are length-prefixed when joined, so the component list → string mapping is injective and a test identifier containing the separator (MySuite/testFoo() does) cannot forge another node's path.

Why not hash the names alone. A report can hold several runs against the same device — merged bundles, or one bundle passed twice — and a duplicate id fails silently: getElementById and querySelectorAll(...)[0] both return the first match, so the second device's rows stop responding without anything failing to parse. There is a test for this, and it is not hypothetical: swapping in the naive hash(record.identifier) derivation makes all three device entries resolve to the same run element, and the test catches it.

Why a digest rather than the path. These land unescaped in both an HTML id attribute and a single-quoted JavaScript string. Test names can contain quotes, angle brackets and spaces; a 32-character lowercase hex digest cannot.

Two more sources of drift, found while verifying

Fixing the UUIDs alone did not make the report reproducible. Both remaining causes are seeded once per process by Swift's hash seed, which is why they only show up across separate processes:

  1. Per-status counts on a mixed test case came out of a Dictionary. The same fixture rendered 1 failed, 1 succeeded on four runs out of five and 1 succeeded, 1 failed on the fifth — user-visible text. Now sorted by status class.
  2. Set iteration order and non-total sort comparators. Ties (equal test names across test-plan configurations, equal titles, equal or absent repetition numbers) resolved differently between runs. Each comparator now has a deterministic tiebreaker; the pre-existing order is preserved wherever the old comparator already decided it.

Verification

Renders are byte-identical, not merely equivalent:

$ diff x1/index.html x2/index.html
$ echo $?
0
$ shasum -a 256 x1/index.html x2/index.html
46a27ef2e20c87671bd9f4056e955d757fb9907191507f457f1dd0d12f28fad1  x1/index.html
46a27ef2e20c87671bd9f4056e955d757fb9907191507f457f1dd0d12f28fad1  x2/index.html

20 renders of each case, one distinct output each (5 would have been enough for the UUIDs, but the ordering drift was only sampled — it needed repetition to surface reliably):

TestResults      : 20 runs -> 1 distinct output(s)
RetryResults     : 20 runs -> 1 distinct output(s)
SanityResults    : 20 runs -> 1 distinct output(s)
all three merged : 20 runs -> 1 distinct output(s)
same bundle twice: 20 runs -> 1 distinct output(s)

Rendering is otherwise unchanged. Diffing this branch's output against main's with identifiers normalised reports no differences, for both TestResults and RetryResults. The one visible consequence is that the device panel's Identifier: line now shows a stable digest instead of a fresh random UUID — the same place, the same shape, and it was never stable to begin with.

The report still navigates. Checked in Chrome against a four-run report (including the same bundle twice, so two runs share a device identifier):

  • clicking the 4th device entry activates the 4th run, exactly one run carries .active, and the clicked handle equals the active run's element id;
  • clicking a test row's disclosure triangle expands that row's activities;
  • expanding iteration 1 of the retried test leaves iteration 2 collapsed, and collapsing 1 leaves 2 open — i.e. the iteration ids address their own elements;
  • no console errors.

The new tests fail without the fix. They run the executable in separate processes on purpose, since two of the three causes cannot be observed inside one process. Both guards were mutation-checked:

  • naive hash(device identifier)("3") is not equal to ("1") - selectDevice('…') must resolve to exactly one run element
  • dropping the iteration renumbering after a merge → 2 identifier(s) used more than once

The uniqueness and escaping tests assert that each kind of identifier (device, target summary, suite, test case, iteration) was actually found, so a selector that stops matching fails the test instead of quietly making it vacuous.

swift test: 28 tests, 1 skipped, 0 failures — the 23-test baseline plus 5 new. SwiftFormat, SwiftLint and shellcheck clean; no lint findings in the changed files beyond the ones already on main.

Note, not fixed here

Passing literally the same bundle twice also duplicates id="activities-<uuid>" on activities, whose ids are read from the bundle (ActionTestActivitySummary.uuid) rather than minted here. That is pre-existing on main and out of scope for this issue; genuinely merged bundles from different runs carry distinct activity uuids and are unaffected.

🤖 Generated with Claude Code

…ucible

Five call sites minted a fresh `UUID()` at parse time for use as DOM ids
and JavaScript handles, so the same binary run twice on the same
`.xcresult` produced different HTML (#411). Rendering was unaffected —
these are internal handles — but it blocked diffing two reports,
content-addressing them, and any golden-file testing.

Identifiers are now a digest of the element's path through the report,
built from the bundle's own content (device identifier, target name,
suite and test identifiers) plus enough positional information to keep
them unique. `IdentifierPath` documents the uniqueness argument in full;
in short, every node appends a component that distinguishes it from its
siblings, and components are length-prefixed when joined so a test
identifier containing the separator cannot forge another node's path.

Uniqueness matters because names alone are not unique: a report can hold
several runs against the same device (merged bundles, or one bundle
passed twice), and a duplicate id fails silently — `getElementById` and
`querySelectorAll(...)[0]` both return the first match, so the second
device's rows would stop responding without anything failing to parse.

A digest rather than the path itself because these land unescaped in
both an HTML `id` attribute and a single-quoted JavaScript string, and
test names can contain quotes, angle brackets and spaces.

Fixing the UUIDs alone left the report non-reproducible, because two
orderings were also seeded per process by Swift's hash seed:

- the per-status counts on a mixed test case came out of a `Dictionary`,
  rendering "1 failed, 1 succeeded" on some runs and "1 succeeded, 1
  failed" on others. Now sorted by status class.
- `Set` iteration order and non-total sort comparators (equal test names,
  equal titles, equal repetition numbers) left ties resolving differently
  between runs. Each comparator now has a deterministic tiebreaker.

Verified: 20 renders of each fixture, and of the merged and
duplicate-bundle cases, produce one distinct output each; `diff` between
two runs is empty. Rendering is otherwise unchanged — diffing against
main's output with identifiers normalised shows no differences.

Fixes #411
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@tylervick, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2c5dc001-ff65-484d-8e41-5dbdd09491bb

📥 Commits

Reviewing files that changed from the base of the PR and between 822129f and 99752ba.

📒 Files selected for processing (9)
  • Sources/XCTestHTMLReportCore/Classes/Helpers/IdentifierPath.swift
  • Sources/XCTestHTMLReportCore/Classes/Models/Iteration.swift
  • Sources/XCTestHTMLReportCore/Classes/Models/Run.swift
  • Sources/XCTestHTMLReportCore/Classes/Models/RunDestination.swift
  • Sources/XCTestHTMLReportCore/Classes/Models/Summary.swift
  • Sources/XCTestHTMLReportCore/Classes/Models/TargetDevice.swift
  • Sources/XCTestHTMLReportCore/Classes/Models/Test.swift
  • Sources/XCTestHTMLReportCore/Classes/Models/TestSummary.swift
  • Tests/XCTestHTMLReportTests/ReproducibilityTests.swift

Comment @coderabbitai help to get the list of available commands.

@tylervick
tylervick merged commit a28b131 into main Aug 11, 2026
8 checks passed
@tylervick
tylervick deleted the tylervick/reproducible-reports-411 branch August 11, 2026 06:34
tylervick added a commit that referenced this pull request Aug 11, 2026
Four amendments to the design spec and implementation plan, none of which
change the strategy.

**Milestone.** Both documents targeted 3.0, inherited from #391's label.
3.0.0 shipped 2026-08-07, so the breaking changes here — the `--json` schema
and the declared output diff — land in 4.0. Notes that the report redesign is
a sibling workstream in that same major rather than a later release, since
`activityType`'s removal breaks the visual contract either way, and that the
two are strictly sequenced: templates stay frozen until the differential is
proven.

**#430 replaces Task 1's normalizer, but does not retire it.** Task 1 had
rediscovered #411 independently and solved it in the harness with a UUID
regex. #430 fixes it in the product via `IdentifierPath`. The subtlety worth
recording: identifiers are a digest of each element's *structural path*, and
the two backends disagree on structure — the modern tree drops the "All tests"
and "<bundle>.xctest" wrapper levels — so cross-backend identifiers still
diverge. The normalizer survives, retargeted from RFC-4122 to `[0-9a-f]{32}`;
a regex left matching UUIDs would silently match nothing and the differential
would compare raw digests and fail on every run.

Task 1 also collided with #430 on `ReproducibilityTests.swift`. It now appends
to that file instead of creating it, and #430 is a stated prerequisite.

**Task 2 captures raw renders.** Same-backend renders are byte-identical after
#430, so normalizing the baseline is unnecessary — and harmful: Task 5 moves
the renderer onto `ParsedResult`, and a refactor that perturbed the tree would
move every affected digest, which a normalized baseline would hide. Phase 1's
gate is correspondingly strengthened to exact equality with no normalization.

**HTMLTemplates.swift is not generated.** Both documents asserted it was.
Its `DO NOT EDIT … autogenerated by createTemplates.sh` header is stale —
that script was deleted in #295 — and both linters exempt the file on that
basis, which is how `HTML/*.html` drifted 28 hunks behind while the "generated"
file was hand-edited. No consequence for this work, which touches neither; it
is a live trap for the redesign workstream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 11, 2026
`ParsedResult` is the one artifact this migration could build twice. Shaped so
the current templates render unchanged, it is not backend-neutral — it is
legacy-shaped, and `ModernResultReader` spends its life supplying nil for
fields that exist only because the old UI reads them. The proposed model
already shows the pattern: `activityType`, `finish`, `name`, and
`uniformTypeIdentifier` are each documented as "nil on the modern backend",
and `statusRawValue` would have the modern reader emit legacy spellings it
never saw.

Adds Task 2.5 between the baseline capture and the model: a decision task, not
a code task, with eight questions and a recommended answer for each. The
redesign's visual work stays a sibling workstream and does not gate this one —
but its information model is exactly what the port encodes, and that is an
afternoon rather than a design phase.

Every answer either removes a field from the port or an entry from the
differential allow-list, because holding the legacy backend down to the modern
backend's capability makes the two agree and an unmasked diff proves more than
a masked one. On the recommended defaults, `activityTypeClasses` and
`durations` leave the allow-list entirely: with no `finish` and no
`activityType` in the model, there is no divergence left to mask. The cost is
one-way and stated — the legacy backend stops rendering some things it could
have — but that is a 4.0 behaviour change made once and visible in the model.

Two answers go the other way for reasons worth keeping: Swift Testing
`Arguments` is added now because it is a reshape *inside* the tree and
expensive to retrofit, while insights and metrics are left out because they
attach at the top level beside `runs` and are cheap to add when something
renders them.

Also records the rule the task exists to enforce: no reader code whose only
purpose is to satisfy the render-level diff. The current templates are a
verification scaffold with a retirement date, not a compatibility target.

Updates both #430 prerequisite notes — merged 2026-08-11 as a28b131.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 11, 2026
Four amendments to the design spec and implementation plan, none of which
change the strategy.

**Milestone.** Both documents targeted 3.0, inherited from #391's label.
3.0.0 shipped 2026-08-07, so the breaking changes here — the `--json` schema
and the declared output diff — land in 4.0. Notes that the report redesign is
a sibling workstream in that same major rather than a later release, since
`activityType`'s removal breaks the visual contract either way, and that the
two are strictly sequenced: templates stay frozen until the differential is
proven.

**#430 replaces Task 1's normalizer, but does not retire it.** Task 1 had
rediscovered #411 independently and solved it in the harness with a UUID
regex. #430 fixes it in the product via `IdentifierPath`. The subtlety worth
recording: identifiers are a digest of each element's *structural path*, and
the two backends disagree on structure — the modern tree drops the "All tests"
and "<bundle>.xctest" wrapper levels — so cross-backend identifiers still
diverge. The normalizer survives, retargeted from RFC-4122 to `[0-9a-f]{32}`;
a regex left matching UUIDs would silently match nothing and the differential
would compare raw digests and fail on every run.

Task 1 also collided with #430 on `ReproducibilityTests.swift`. It now appends
to that file instead of creating it, and #430 is a stated prerequisite.

**Task 2 captures raw renders.** Same-backend renders are byte-identical after
#430, so normalizing the baseline is unnecessary — and harmful: Task 5 moves
the renderer onto `ParsedResult`, and a refactor that perturbed the tree would
move every affected digest, which a normalized baseline would hide. Phase 1's
gate is correspondingly strengthened to exact equality with no normalization.

**HTMLTemplates.swift is not generated.** Both documents asserted it was.
Its `DO NOT EDIT … autogenerated by createTemplates.sh` header is stale —
that script was deleted in #295 — and both linters exempt the file on that
basis, which is how `HTML/*.html` drifted 28 hunks behind while the "generated"
file was hand-edited. No consequence for this work, which touches neither; it
is a live trap for the redesign workstream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 11, 2026
`ParsedResult` is the one artifact this migration could build twice. Shaped so
the current templates render unchanged, it is not backend-neutral — it is
legacy-shaped, and `ModernResultReader` spends its life supplying nil for
fields that exist only because the old UI reads them. The proposed model
already shows the pattern: `activityType`, `finish`, `name`, and
`uniformTypeIdentifier` are each documented as "nil on the modern backend",
and `statusRawValue` would have the modern reader emit legacy spellings it
never saw.

Adds Task 2.5 between the baseline capture and the model: a decision task, not
a code task, with eight questions and a recommended answer for each. The
redesign's visual work stays a sibling workstream and does not gate this one —
but its information model is exactly what the port encodes, and that is an
afternoon rather than a design phase.

Every answer either removes a field from the port or an entry from the
differential allow-list, because holding the legacy backend down to the modern
backend's capability makes the two agree and an unmasked diff proves more than
a masked one. On the recommended defaults, `activityTypeClasses` and
`durations` leave the allow-list entirely: with no `finish` and no
`activityType` in the model, there is no divergence left to mask. The cost is
one-way and stated — the legacy backend stops rendering some things it could
have — but that is a 4.0 behaviour change made once and visible in the model.

Two answers go the other way for reasons worth keeping: Swift Testing
`Arguments` is added now because it is a reshape *inside* the tree and
expensive to retrofit, while insights and metrics are left out because they
attach at the top level beside `runs` and are cheap to add when something
renders them.

Also records the rule the task exists to enforce: no reader code whose only
purpose is to satisfy the render-level diff. The current templates are a
verification scaffold with a retirement date, not a compatibility target.

Updates both #430 prerequisite notes — merged 2026-08-11 as a28b131.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 11, 2026
Applies Task 2.5's eight recommendations as written and propagates them
through the port, the readers, the renderer task, and the differential.

Three fields leave ParsedResult (activity finish, activity type, attachment
UTI), one arrives (Swift Testing arguments), one is retyped (status raw string
becomes a neutral enum), and ObjectClass is deleted rather than threaded
through. The direction is the point: each removal makes the two backends agree
by construction instead of by mask, so the differential allow-list drops from
five entries to three. An unmasked diff proves more than a masked one, and the
masked region is exactly where a regression can hide.

Two constraints surfaced while applying the answers, both recorded rather than
assumed away. Answer 6's `arguments` is unexercised: `Arguments` is in the
published TestNodeType enum, but SwiftTestingSuite has no parameterized case
and all three fixtures contain zero such nodes, so Task 8 gains a step that
adds one and a test that fails until it lands. Answer 4 cannot use
UTType(filenameExtension:), which is macOS 11+ against a 10.15 floor, so the
mapping keeps an explicit table.

Rebasing onto #430 also exposed stale guidance in Task 5: it had Activity.uuid
becoming UUID().uuidString, which would compile and silently undo the
reproducibility #430 just established. Activity now mints from IdentifierPath
like every other model.

Removing the activity-type and duration fields leaves two template
placeholders fed with empty strings rather than deleted, because
createTemplates.sh — the generator named in HTMLTemplates.swift's own
DO-NOT-EDIT header — is not in this repository. That leaves a cosmetic empty
paren on activity rows, noted for the redesign workstream, and is still better
than rendering a fabricated (0.00s).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 12, 2026
An xhigh multi-agent review found 30 distinct defects in #435, most of them
downstream of two wrong decisions. Correcting the decisions and re-deriving,
rather than patching the symptoms.

Decision 3 becomes replace-not-delete. Deleting ObjectClass empties ITEM_CLASS
and renders <div class=" failed"> on every row, which breaks the report's own
"show only failures" filter (showElementsWithSelector on .test-summary.failed
and siblings), group expand/collapse (querySelectorAll on .test-summary-group),
the stylesheet rules keyed on both classes, and four test call sites -- one in
CoreTests and three in ReproducibilityTests. The raw IDESchemeActionTest*
values were the legacy part and still go; the emitted class names are the
report's own contract and stay, behind a neutral renderer-side NodeKind. The
port needs no field, since ParsedNode already distinguishes group from case.

Decision 1 still removes the field, with two corrections. `start` is now named
as the replacement ordering key: the sort it fed interleaves failure rows among
activities so a failure renders where it occurred, and deleting it rather than
re-keying it would silently append every failure to the end. And the `durations`
allow-list entry is restored -- deleting it assumed removing `finish` removed
all duration divergence, but the surviving divergence is in group durations.
Verified: durationInSeconds is null on every Test Suite, Test Plan and test
bundle node in all three fixtures, while legacy reports FirstSuite 0.699s,
SecondSuite 0.126s, ThirdSuite 0.132s, SampleAppUnitTests 0.213s. wrapperGroups
only drops wrapper lines, so real suite headings were diverging unmasked.

Task 5 is split. It conflated a pure refactor with mandated behaviour changes
and then demanded a byte-identical gate, so the gate could only ever be waived
-- leaving the migration's largest refactor with no behaviour check. 5a moves
the renderer onto ParsedResult and must be byte-identical; 5b applies the
decisions that change output against an enumerated three-shape diff.

Also fixed, from the same review: failure rows were double-counted on modern
because both documents describe the same failure; the failureTitlePrefix mask
stripped only the legacy shape so every failing test still differed after
masking; read() handed every device the same testables array; an empty devices
list returned a non-nil empty result that exited 0 where legacy exits 3;
PayloadProviding omitted three members the call sites use, including the
downsize path from #428; Task 5 deleted three accessors LegacyResultReader
calls; Summary.init dropped the resultIndex/actionIndex seeding #430 needs;
ValidationError was thrown from a non-throwing init in a target without
ArgumentParser; legacyCapability returned false where the type wanted .unknown;
run??.activities double-chained a flattened optional; and Task 1 called a
render helper that does not exist.

Two refinements beyond the brief. The durations mask is necessarily
over-broad -- the duration sits on a different line from the group class, so it
cannot be scoped -- which also hides XCTest case durations that do agree, so a
targeted model-level assertion restores that coverage. And the empty-read rule
is stated for both readers rather than fixed only on the modern one, since
legacy has the identical shape.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 12, 2026
* docs: design spec for migrating off xcresulttool --legacy (#391)

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>

* docs: implementation plan for the xcresulttool legacy migration (#391)

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>

* docs: close verification gaps in the migration plan from PR review

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>

* docs: make the backend flag live and align the reader contract

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>

* docs: prove the partial-read fault actually reaches the report

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>

* docs: source failure text from the node that keeps file and line

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>

* docs: retarget the migration at 4.0 and rebase it onto #430

Four amendments to the design spec and implementation plan, none of which
change the strategy.

**Milestone.** Both documents targeted 3.0, inherited from #391's label.
3.0.0 shipped 2026-08-07, so the breaking changes here — the `--json` schema
and the declared output diff — land in 4.0. Notes that the report redesign is
a sibling workstream in that same major rather than a later release, since
`activityType`'s removal breaks the visual contract either way, and that the
two are strictly sequenced: templates stay frozen until the differential is
proven.

**#430 replaces Task 1's normalizer, but does not retire it.** Task 1 had
rediscovered #411 independently and solved it in the harness with a UUID
regex. #430 fixes it in the product via `IdentifierPath`. The subtlety worth
recording: identifiers are a digest of each element's *structural path*, and
the two backends disagree on structure — the modern tree drops the "All tests"
and "<bundle>.xctest" wrapper levels — so cross-backend identifiers still
diverge. The normalizer survives, retargeted from RFC-4122 to `[0-9a-f]{32}`;
a regex left matching UUIDs would silently match nothing and the differential
would compare raw digests and fail on every run.

Task 1 also collided with #430 on `ReproducibilityTests.swift`. It now appends
to that file instead of creating it, and #430 is a stated prerequisite.

**Task 2 captures raw renders.** Same-backend renders are byte-identical after
#430, so normalizing the baseline is unnecessary — and harmful: Task 5 moves
the renderer onto `ParsedResult`, and a refactor that perturbed the tree would
move every affected digest, which a normalized baseline would hide. Phase 1's
gate is correspondingly strengthened to exact equality with no normalization.

**HTMLTemplates.swift is not generated.** Both documents asserted it was.
Its `DO NOT EDIT … autogenerated by createTemplates.sh` header is stale —
that script was deleted in #295 — and both linters exempt the file on that
basis, which is how `HTML/*.html` drifted 28 hunks behind while the "generated"
file was hand-edited. No consequence for this work, which touches neither; it
is a live trap for the redesign workstream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: gate ParsedResult behind an information-model decision (Task 2.5)

`ParsedResult` is the one artifact this migration could build twice. Shaped so
the current templates render unchanged, it is not backend-neutral — it is
legacy-shaped, and `ModernResultReader` spends its life supplying nil for
fields that exist only because the old UI reads them. The proposed model
already shows the pattern: `activityType`, `finish`, `name`, and
`uniformTypeIdentifier` are each documented as "nil on the modern backend",
and `statusRawValue` would have the modern reader emit legacy spellings it
never saw.

Adds Task 2.5 between the baseline capture and the model: a decision task, not
a code task, with eight questions and a recommended answer for each. The
redesign's visual work stays a sibling workstream and does not gate this one —
but its information model is exactly what the port encodes, and that is an
afternoon rather than a design phase.

Every answer either removes a field from the port or an entry from the
differential allow-list, because holding the legacy backend down to the modern
backend's capability makes the two agree and an unmasked diff proves more than
a masked one. On the recommended defaults, `activityTypeClasses` and
`durations` leave the allow-list entirely: with no `finish` and no
`activityType` in the model, there is no divergence left to mask. The cost is
one-way and stated — the legacy backend stops rendering some things it could
have — but that is a 4.0 behaviour change made once and visible in the model.

Two answers go the other way for reasons worth keeping: Swift Testing
`Arguments` is added now because it is a reshape *inside* the tree and
expensive to retrofit, while insights and metrics are left out because they
attach at the top level beside `runs` and are cheap to add when something
renders them.

Also records the rule the task exists to enforce: no reader code whose only
purpose is to satisfy the render-level diff. The current templates are a
verification scaffold with a retirement date, not a compatibility target.

Updates both #430 prerequisite notes — merged 2026-08-11 as a28b131.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: settle the information model before writing ParsedResult

Applies Task 2.5's eight recommendations as written and propagates them
through the port, the readers, the renderer task, and the differential.

Three fields leave ParsedResult (activity finish, activity type, attachment
UTI), one arrives (Swift Testing arguments), one is retyped (status raw string
becomes a neutral enum), and ObjectClass is deleted rather than threaded
through. The direction is the point: each removal makes the two backends agree
by construction instead of by mask, so the differential allow-list drops from
five entries to three. An unmasked diff proves more than a masked one, and the
masked region is exactly where a regression can hide.

Two constraints surfaced while applying the answers, both recorded rather than
assumed away. Answer 6's `arguments` is unexercised: `Arguments` is in the
published TestNodeType enum, but SwiftTestingSuite has no parameterized case
and all three fixtures contain zero such nodes, so Task 8 gains a step that
adds one and a test that fails until it lands. Answer 4 cannot use
UTType(filenameExtension:), which is macOS 11+ against a 10.15 floor, so the
mapping keeps an explicit table.

Rebasing onto #430 also exposed stale guidance in Task 5: it had Activity.uuid
becoming UUID().uuidString, which would compile and silently undo the
reproducibility #430 just established. Activity now mints from IdentifierPath
like every other model.

Removing the activity-type and duration fields leaves two template
placeholders fed with empty strings rather than deleted, because
createTemplates.sh — the generator named in HTMLTemplates.swift's own
DO-NOT-EDIT header — is not in this repository. That leaves a cosmetic empty
paren on activity rows, noted for the redesign workstream, and is still better
than rendering a fabricated (0.00s).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: scope backend demotion, name the surviving allow-list entries, require a --json contract

Three findings from review, all against the spec.

The demotion rule was written as "any hard failure of a legacy command demotes
to modern", which is both broader than the plan implements and a hazard: a
corrupt bundle or permission error would silently retry on the modern reader
and produce a partial report where a clear failure belonged. Demotion is now
scoped to capability detection only, with everything else propagating through
FaultCollector to the exit-3 path.

The spec claimed the allow-list drops from five entries to three without saying
which three. Since this record is what Task 12 is read against, it now names
them with what still differs and which fixture exercises each, plus the
standing instruction to prefer deleting a field from the port over adding an
entry.

--json was specified only as "our schema". ParsedResult is an internal Swift
model, so deriving public output from a synthesized Encodable would make every
later field rename a silent breaking change. Task 14 gains a step requiring the
wire contract be written first -- field names, enum encoding, null-versus-
omitted, units, ordering, and a schema version -- with the encoder made to
match the document rather than the reverse.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: make backend detection tri-state and stop substituting an explicit legacy reader

Review caught a vacuous-verification hazard I had introduced. Making an
explicit --result-reader legacy fall back to modern meant a modern-only host
would run the modern reader twice and the differential would compare a backend
against itself and report parity -- the exact failure that suite exists to
prevent.

Detection is now tri-state (available / unavailable / unknown) and only `auto`
ever substitutes. An explicit legacy request that cannot be honoured is an
error; an unparseable version string is `unknown`, which degrades `auto` to
modern but lets an explicit legacy attempt proceed, since a string we cannot
read is not proof the commands are gone. requireBothBackends now asserts on the
backend it actually resolved rather than trusting the request.

--json parity was contradictory: the spec promised output "identical on both
backends" while the same document preserves three render-level differences.
Restated as schema identity -- same field names, nesting, enum encoding and
schemaVersion -- with the four permitted value differences enumerated and
everything else declared a reader bug. The accompanying test compared only
top-level keys, which cannot see a nested field present on one side; it now
compares full key paths.

Log write failures recorded no fault in either provider, so a report missing a
log it had successfully read would still exit 0. Both now record
.logExportFailed. The legacy one is pre-existing rather than introduced here,
but leaving one of two implementations silent is how that asymmetry survives.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: separate the arguments capability difference from the allow-list

Review caught that --json listed four permitted value differences while the
allow-list has three entries, leaving testCase.arguments as an implicit fourth
rule. It is not one: nothing renders arguments, so it cannot appear in the HTML
differential at all and needs no masking rule. Now classified separately as a
model-level capability difference, with the instruction that --json compares it
by asserting legacy is empty rather than asserting the two sides match -- and a
note that until the parameterized @test lands, both sides are empty and a naive
equality assertion would pass vacuously.

Also corrects reasoning I got wrong in the previous commit. Task 5 justified
leaving the orphaned TIME and ITEM_CLASS placeholders by claiming the template
generator is missing and hand-editing is forbidden. The spec already establishes
the opposite: createTemplates.sh was deleted in #295, HTMLTemplates.swift is
hand-maintained, and #349 edited it directly. The outcome is unchanged --
templates stay untouched -- but because the Global Constraints scope this plan
out of report markup and the redesign workstream must first settle which
template copy is the source of truth, not because the edit is impossible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: correct two Task 2.5 answers and re-derive the propagation

An xhigh multi-agent review found 30 distinct defects in #435, most of them
downstream of two wrong decisions. Correcting the decisions and re-deriving,
rather than patching the symptoms.

Decision 3 becomes replace-not-delete. Deleting ObjectClass empties ITEM_CLASS
and renders <div class=" failed"> on every row, which breaks the report's own
"show only failures" filter (showElementsWithSelector on .test-summary.failed
and siblings), group expand/collapse (querySelectorAll on .test-summary-group),
the stylesheet rules keyed on both classes, and four test call sites -- one in
CoreTests and three in ReproducibilityTests. The raw IDESchemeActionTest*
values were the legacy part and still go; the emitted class names are the
report's own contract and stay, behind a neutral renderer-side NodeKind. The
port needs no field, since ParsedNode already distinguishes group from case.

Decision 1 still removes the field, with two corrections. `start` is now named
as the replacement ordering key: the sort it fed interleaves failure rows among
activities so a failure renders where it occurred, and deleting it rather than
re-keying it would silently append every failure to the end. And the `durations`
allow-list entry is restored -- deleting it assumed removing `finish` removed
all duration divergence, but the surviving divergence is in group durations.
Verified: durationInSeconds is null on every Test Suite, Test Plan and test
bundle node in all three fixtures, while legacy reports FirstSuite 0.699s,
SecondSuite 0.126s, ThirdSuite 0.132s, SampleAppUnitTests 0.213s. wrapperGroups
only drops wrapper lines, so real suite headings were diverging unmasked.

Task 5 is split. It conflated a pure refactor with mandated behaviour changes
and then demanded a byte-identical gate, so the gate could only ever be waived
-- leaving the migration's largest refactor with no behaviour check. 5a moves
the renderer onto ParsedResult and must be byte-identical; 5b applies the
decisions that change output against an enumerated three-shape diff.

Also fixed, from the same review: failure rows were double-counted on modern
because both documents describe the same failure; the failureTitlePrefix mask
stripped only the legacy shape so every failing test still differed after
masking; read() handed every device the same testables array; an empty devices
list returned a non-nil empty result that exited 0 where legacy exits 3;
PayloadProviding omitted three members the call sites use, including the
downsize path from #428; Task 5 deleted three accessors LegacyResultReader
calls; Summary.init dropped the resultIndex/actionIndex seeding #430 needs;
ValidationError was thrown from a non-throwing init in a target without
ArgumentParser; legacyCapability returned false where the type wanted .unknown;
run??.activities double-chained a flattened optional; and Task 1 called a
render helper that does not exist.

Two refinements beyond the brief. The durations mask is necessarily
over-broad -- the duration sits on a different line from the group class, so it
cannot be scoped -- which also hides XCTest case durations that do agree, so a
targeted model-level assertion restores that coverage. And the empty-read rule
is stated for both readers rather than fixed only on the modern one, since
legacy has the identical shape.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 12, 2026
Fixture generation boots a simulator and runs the sample app's UI tests
on every CI run; even after #425 it dominates the test job's ~10-minute
wall time (#412). The bundles are deterministic for a given toolchain
and sample app (#423/#430), and every assertion in the suite compares
within one run, so identical bytes are safe to serve across runs.

Cache the three .xcresult bundles keyed on the exact Xcode build, the
newest installed iOS simulator runtime, prepareTestResults.sh itself,
and the sample-app sources. No restore-keys: an inexact match would
serve fixtures from a different toolchain, which the drift detector
(#392) and the legacy-vs-modern differential test cannot tolerate. A
verification step asserts all three bundles exist whether restored or
generated, so a cache hit that restores nothing fails instead of
passing as a suspiciously fast green run.

Local flow is untouched: prepareTestResults.sh is unchanged and swift
test still reads the same paths.

Fixes #412

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 13, 2026
…t + forced-modern CI leg (#391, Tasks 12–13) (#450)

* Parity rulings from the first differential run; fixes the #449 export 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 <noreply@anthropic.com>

* test: the differential — legacy/modern parity held to a declared allow-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 <noreply@anthropic.com>

* ci: forced-modern test leg via XCHR_RESULT_READER (Task 13)

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 <noreply@anthropic.com>

* docs: record the Task 12 rulings as rules, and the 4.0 release-note items

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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
tylervick added a commit that referenced this pull request Aug 15, 2026
…439) (#484)

* Give each view its own surface and the report one device picker (refs #439)

The three-pane shell dissolves. Tests and Logs each own the whole content
area below the tabs, toolbar included; device selection moves into a header
picker built from A1's device bars; attachments become a sheet the Tests view
summons rather than a pane the window reserves.

What that replaces, measured on a single-device run at 1440px: a 200px device
sidebar and a 400px attachment pane, ~42% of the window, spent on one device
card and the words "No Selected Attachment" (audit findings 1 and 3).

This is the PR the restyle-don't-rewire constraint was saving itself for, so
the scripts are restructured rather than worked around. Every test that pinned
the old contract is updated, each for a stated reason.

Shell
- Views are the outer level and runs nest inside them, so each view is one
  contiguous region a tab can point `aria-controls` at. A run contributes a
  `tests_<id>` slice and a `logs_<id>` slice; the picker activates the pair.
  This also fixes a live collision: every run used to emit `id="logs"`,
  `id="logs-header"` and `id="logs-iframe"`, so a two-bundle report emitted
  each of them twice and only behaved because the duplicates sat inside a
  hidden pane.
- Tests/Logs become a real tablist. They were two `<li onclick>` no keyboard
  could reach.
- The status filters become a radiogroup of buttons with `aria-checked`. The
  five filter functions are unchanged apart from their scope, which named the
  shell's per-run pane and now names the per-view slice; A3b upgrades them.
- The per-view toolbar has a right-aligned trailing slot, laid out and empty,
  for A3b's text filter and dropdowns (#460).
- The column header lifts out of the scroll container: it names the columns of
  every row, so scrolling it away was the one thing it could not afford.

Device picker
- One control, not two. A1's "Devices & Configurations" card stated each run's
  split and could not act; the sidebar could act and stated nothing. The bars
  are now inside the picker: every option is a button carrying the outcome
  glyph, the destination, the proportional bar, the spoken tally and the model.
- It lives in the title band, not the summary band, because the band stands
  down for the Logs view and every run has its own log.
- `<details>`, so the disclosure is the browser's: focusable, announced, and
  correct with no script. Escape closes it and returns focus.
- Options carry a run number when a report holds several runs. Two runs can
  have byte-identical destination fields — a bundle merged with itself, or two
  bundles from one simulator, which is what `ReproducibilityTests`' duplicate
  case is built from — and without it the picker offers two options a reader
  cannot tell apart. Derived from the run's index, so it agrees across
  backends by construction.
- Dropped: the sidebar's `Identifier:` line. Since #430 it has carried the
  report's own element handle, not the destination's identifier — a digest
  naming nothing a reader can use. The handle stays in `data-device`.
- `RunDestination.status` goes with it. It was hardcoded `.unknown` behind a
  standing TODO and the icon rule drew nothing for that case, so every card in
  every report ever rendered showed a blank status cell. The picker states the
  real outcome, from `Run.status`.

Attachments
- A2's <=700px bottom sheet generalises to every width and moves inside the
  Tests view. No placeholder state: a sheet with nothing in it is not in the
  layout. Nothing attachment-shaped exists while Logs is showing, because
  `display: none` on the view takes the subtree out of the a11y tree too.
- Docked as a flex item rather than `position: fixed`, so the tree gets shorter
  instead of needing `padding-bottom: 50vh` to stay reachable.
- One handler for all five kinds, taking the element, with `data-kind` written
  by the template that knows. The old script split the path on `.` and read the
  last piece, which cannot work for the `data:` URIs inline mode emits — and
  did not work for `.png` either, because the extension list was probed with
  `indexOf(...) > 0` and `png` sits at index 0. Clicking the eye always worked;
  selecting the row that held it sent a PNG to the download branch.
- The eye becomes a real `<button>`. It was the report's most consequential
  control and had no keyboard path. Its focus ring and its mask are separate
  elements: a mask clips everything its element paints, outline included.

Keyboard and a11y
- Roving focus for the tablist and the filter groups, so Tab steps past a
  toolbar in one press.
- The tree claims the arrow keys only while it is the thing being read. Before
  this, nothing in the page could take focus, so a document-level handler could
  not collide with anything.
- Switching view moves focus off any element inside the panel being hidden,
  which the browser would otherwise drop to <body>.
- The axe gate now runs in four states — as opened, picker open, sheet open,
  Logs — each asserting its own precondition, because three permanent panes
  became surfaces that come and go and a one-state gate would have gone green
  while seeing none of them. The contrast gate gets the same treatment and
  runs over both fixtures; opening the picker caught a defect this PR
  introduced, the selected option's run number at 1.31:1.

375px
- There is no longer a second layout to fold into: no sidebar to become a
  strip, no pane to become a sheet, no resizers to hide. The query is spacing,
  indentation and two caps. The picker's label is clipped rather than removed
  so the control keeps its accessible name.

Colour
- `--color-bg-sidebar` is renamed `--color-bg-chrome`; the values are
  unchanged in both themes, so every status figure measured against it carries
  over.
- One new token, `--color-border-control`, sized to clear the 3:1 non-text
  floor on the ground it is drawn on (3.24 light / 3.48 dark): the boundary of
  the picker's summary and of the sheet's Close button is part of what says
  "this is a button", where the sheet's other borders outline regions and are
  decorative under 1.4.11.

Differential green by construction: the shell is shared template text, so both
backends render it identically. `ReproducibilityTests`' duplicate-bundle case
is unchanged in what it asserts and strengthened in reach — it now requires a
device handle to address exactly one slice per view, which covers the log panes
the old single-selector check could not see.

Refs #439.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Fix the three behaviour findings from the A3a review (refs #439)

The xhigh review of #484 approved with findings; F1-F3 gate the merge.

F1 — a stale `.selected` survived a destination round-trip. `selectDevice`
dropped the script's handle on the selected row without removing the class,
so the row kept painting itself selected while hidden, came back still
painted, and the next selection added a second fill rather than moving the
first: two rows claiming a selection the attachment sheet answers for one of.

F2 — choosing a destination by keyboard dropped focus to `<body>`. Closing
the `<details>` takes the activated option out of the layout, which is the
failure `showView` already guards against for the view tabs and the reason
the Escape path refocuses the summary. The choose path lands in the same
place, guarded on the picker actually holding focus so boot and the digest
jump do not steal it.

F3 — the title band overflowed a 375px viewport with a real destination
name ("iPhone 17 Pro Max 26.2 Run 1" measured 405px in a 375px window).
`body` sets `overflow: hidden`, so the chevron, the summary's border and the
tail of the name were cut off rather than scrolled to, and `text-overflow`
never engaged: the picker is a flex item whose automatic minimum held it at
the width of a name that does not wrap. `min-width: 0` waives it, and the
run's status glyph — squeezed from 24px to nothing, being the one item in
the row with no content to protect it — is now `flex: none`.

F5 — the substitution-order comment was inverted. A chain of replacements
fills the placeholders an earlier replacement inserted, so being last made
`[[DEVICE_IDENTIFIER]]` reachable from a destination name rather than
protecting it. The identifier is now substituted first, being the one
machine-derived value in the chain, and `pickerHTML` fills its options
before its summary for the same reason. No XSS was reachable either way —
the digest is opaque hex and every leaf is escaped — but a destination named
after a placeholder rendered a digest, or a panel of buttons, inside its own
name.

Gates, each mutation-tested to prove it bites:
- `a destination round-trip leaves exactly one row highlighted` counts
  document-wide, since a stale fill in a hidden run is what a scoped count
  cannot see.
- the keyboard test now chooses as well as opening and escaping, on the
  two-bundle fixture, reading focus after two settled frames.
- `the title band fits a 375px viewport` measures the fixture's longer
  destination and asserts, as its precondition, that the name is at least as
  long as the real one that found this — so a short synthetic name can never
  hide the clip again.
- `testADestinationNamedAfterAPlaceholderIsNotFilledByIt` pins both orders.

Also: the evidence screenshots in `orca-artifacts/a3a-shell/` are re-shot.
The two real-picker files showed the pre-fix contrast defect the PR narrates
as caught and fixed; the four multi-picker files showed it too, and every
375px file showed the overflow this commit repairs.

Refs #439.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Give the placeholder-order pins their own file (refs #439)

Follow-up to the review fixes. The new pin pushed `HTMLEscapingTests` past
its type-body limit, and the failure it pins is not an escaping failure at
all: every value in the chain is correctly escaped and still changes the
markup around it, because a chain of replacements fills the placeholders an
earlier replacement inserted. `PlaceholderOrderTests` says that in its name.

Adds the boundary the reorder does not move. Two of the picker's
placeholders carry test-plan text and no order protects both; the one left
reachable is `[[CURRENT_DEVICE]]`, deliberately, because what it inserts is a
destination label rather than the panel of buttons the opposite order handed
out. The test asserts exactly that much — a name can garble text, never put
a control in the picker — and names what closing the class outright would
take: single-pass substitution across the whole `HTML` seam, which every
template shares and which predates this work.

Refs #439.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Close a hole in the 375px glyph assertion (refs #439)

"Width matches height" is satisfied by an element that has vanished in both
axes, and the run's glyph is sized in one axis by a rule and in the other by
the flex row it sits in — so removing the rule outright would have left the
shape assertion passing on nothing. Assert it is in the layout first.

Refs #439.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Point the substitution-order comment at the test that holds it (refs #439)

Comment only. `pickerHTML` states the exposure its ordering leaves open;
naming the file that asserts the boundary is what stops the next reader
having to take the comment's word for it.

Refs #439.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Scope the placeholder precondition to the span it lands in (refs #439)

A precondition a stray copy elsewhere in the document could satisfy is not
one. It still asserts only that the fixture arrived, not what the placeholder
after it became: that is the behaviour under test, and pinning today's answer
would make the test fail the day the seam stops rescanning what it inserted —
which is the outcome it argues for.

Refs #439.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reports are not reproducible: fresh UUIDs are minted on every run

1 participant