Give the image rendering path its first test coverage - #428

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>

* 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>