refactor(ios-runner): make the snapshot quality verdict state a closed enum - #2888
Conversation
Size Report
Startup median (7 runs, lower is better):
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Thermo-nuclear structural pass. The core move is right and feels inevitable in hindsight: the runner's free String becomes a closed enum, SnapshotQualityVerdict['state'] derives from one kernel tuple, both readers call one predicate, and a fixture pins the cross-language table. The three-way vocabulary is the correct (really only) mechanism and matches the house pattern (ios-system-surface-hosts.json); the path walks fail loudly (Data(contentsOf:) / readFileSync throw on a move); no file crossed 1k (kernel 642, Swift 729); and (TUPLE as readonly string[]).includes(value) is this file's existing idiom, not a new smell.
One change I would like before merge, plus two cheap on-brand items.
Also worth a look (did not inline):
packages/kernel/src/snapshot.ts:7module doc now says capture-kit'ssnapshot-quality-verdict.ts"owns reading a verdict off the wire", butsnapshot-capture-annotations.tsis a second wire reader this PR deepened. Name the real ownership (kernel owns the vocabulary; capture-kit parses an untrusted runner payload; contracts re-hydrates a response this repo published) so the sentence stops contradicting the code.wireTestCapture()inRunnerTests+SnapshotQualityStateTests.swiftis a ~28-line single-node builder that no assertion reads (the loop only decodessnapshotQuality.state) and is a near-4th copy of that builder in the target — build it from the existing helper orDataPayload(truncated: false)and drop the extra import.
|
Both non-inlined items taken in 7b8b463:
Evidence on |
|
Reviewed at 7b8b463. The closed Swift enum and the single kernel vocabulary look sound, and the wire JSON stays the same. All 16 checks were still queued at review time. I could not find the eager-closure budget entries for I didn't compile TypeScript or Swift myself, so the compile-time exhaustiveness and the Swift test-target membership are your reported results, not something I verified. Same for the runner side: the wire format is unchanged by construction and the host unit lane passed by your report, but there's no live iOS device run backing this, and iOS smoke CI is still queued. The contracts reader change means version-skewed Node clients (https://github.com/callstack/agent-device/blob/7b8b463/src/agent-device-client.ts#L544) now drop unknown state/backend verdicts instead of passing them through. There's no CHANGELOG entry for that, though the issue does scope it as intended, so that may be fine as is. Do the two readers actually agree on which payloads count as a verdict at all? The test at https://github.com/callstack/agent-device/blob/7b8b463/packages/capture-kit/src/snapshot-quality-verdict.test.ts#L352 says they must, but capture-kit gates on iOS-only backend capabilities while contracts accepts Could Not blocking: the paragraph-long comments justifying the local maps, the partial re-read, and the review history in both readers and the kernel test could shrink to a single-sentence constraint note each, and the inline No conflicts. The next step is your answer on the shared kernel predicate. |
|
Reviewed at 6e9ab31, following up on the evidence-pending review at 7b8b463 (#2888 (comment)). The enum change itself looks sound, but nothing new closes the evidence gap from that review, so it stays open. Not blocking: packages/contracts/src/snapshot-capture-annotations.ts#L120 (https://github.com/callstack/agent-device/blob/6e9ab31/packages/contracts/src/snapshot-capture-annotations.ts#L120) has a leftover word "Reading" at the end of a sentence before the next one starts; worth cleaning up but can wait. The runner's Swift stamping path is device-facing, and the only live run so far failed before reaching it, so this still needs a green iOS simulator smoke run on 6e9ab31, or a rerun of run 36009426964, that gets smoke:automation-input past "wait text Automation lab" and shows a successful snapshot with snapshotQuality.state stamped "healthy" or "recovered". The queued macOS smoke run also needs to finish green before this can merge. The iOS Smoke Tests run (36009426964) failed at "wait text Automation lab" in smoke:automation-input with COMMAND_FAILED invalid-viewport, because every backend timed out preparing the tree snapshot on the XCTest main thread (reasonCode capture-failed). The value stamped in the log is well formed (state "sparse", backend "private-ax"), so this looks like a main-thread capture timeout rather than an encoding bug from this change, though the route does pass through the stampedSnapshotPayload fallback this PR touches. I did not compile Swift or TypeScript or run any tests, so the enum's exhaustiveness and the new XCTest membership rest on your report. I also could not check whether this same capture timeout happens on main, or whether the iOS smoke passed at 7b8b463. No conflicts. Could you also confirm where the eager-closure budget entries land and whether the kernel predicate should be shared, since the previous review raised both and the delta doesn't answer them? The next step is to rerun the iOS Smoke Tests until they pass, let the macOS smoke finish, and answer those two questions. |
|
Answering the review at 7b8b463 (#2888 (comment)) — all four asks, on 1. Where the eager-closure budgets live (your main question). Nowhere as numbers, which is why you could not find them. 2. Shared kernel predicate: implemented, measured, reverted. I re-installed 3. The 4. CHANGELOG entry added under Unreleased for the client-side drop of an undeclared Non-blocking items taken: the justification comments in both readers and the kernel doc are now one constraint sentence each (the review-history narration is gone), and the kernel test keeps only its |
|
Answering the follow-up at 6e9ab31 (#2888 (comment)), on Leftover word at Smoke rerun: green. Run 36009426964 attempt 2 completed The What the green run does show device-facing: the runner stamped I also drove a simulator here (booted iPhone Duo): |
e905886 to
dedd8e8
Compare
|
Rebased on
Validation re-run on Worth noting for the failed lane: upstream landed #2902 ("wait out the launch an accepted deep-link confirmation releases") in this range, which is the |
dedd8e8 to
3a1d6ef
Compare
…lary The runner wrote the verdict `state` as a free String while the host accepts only healthy | recovered | sparse, and the contracts annotation reader cast any string state into the verdict type. A typo or a one-sided rename therefore dropped the verdict and its disclosure. `SnapshotQualityState` now owns the runner side, raw-value Codable keeps the wire JSON unchanged, and `reasonCode` stays open. The kernel states the vocabulary once as `SNAPSHOT_QUALITY_STATES` with `isSnapshotQualityState`; capture-kit and contracts both read through it, so neither keeps a second accepted-state set and the annotation reader rejects an unknown state instead of casting it. `contracts/fixtures/ios-snapshot-quality-states.json` is the table the Swift `allCases` order and the kernel tuple are each pinned to.
…reader The readers reached the vocabulary through `isSnapshotQualityState` in `kernel/snapshot.ts`, which made that module eager in six entry closures the eager-closure gate holds at its merge-base size — the contracts capture façade at 9 modules, capture-kit's verdict reader at 2. The only module those two closures already evaluate is the one whose edge the gate rejects, so no single runtime home for the set exists. Each reader now keys a `Record<SnapshotQualityState, true>` over the kernel union, the home the gate prescribes for code that has to live where it is already evaluated. Membership goes through `Object.hasOwn`, so an inherited key is never a state, and a state added to the tuple without a key in a reader is a compile error there: the guarantee the shared import was bought for, without the eager edge. Both reader tests walk the tuple and the kernel test still pins it to the fixture.
…y order The annotation reader was one predicate plus a blanket cast. `backend` names the recovery strategy in the warning line, so it goes through a declared map keyed against the kernel union now, and capture-kit lost both of its `as` casts on the way: the strategy is gated on the capability table it already imports, the reason code on an exhaustive map over its union. A full projection in contracts was tried and the repo's own gates refuse it — fallow reports a 4-group, 104-line clone family against capture-kit's normalizer, on top of the eager-closure gate that already forbids a shared reader. The re-read stays in the shape `readTargetActivation` in the same file uses: check the two names that decide presentation, forward what this module published, and pin the pair payload-by-payload from capture-kit's test. Order pinning drops out. The fixture is compared as a set on both sides, the stamping test asserts each case's own raw value, and the unread 28-line capture builder and its import are gone.
…eader Two same-named readers with different trust levels is how the half-migration read as validating more than it does. This module's version checks the two load-bearing names and forwards what this repo published, in the shape `readTargetActivation` uses, so it is now `readPublishedSnapshotQualityVerdict` and the strict per-field reading keeps the plain name for capture-kit's untrusted-payload reader.
…vidence The eager-closure refusal the review asked to be pointed at or filed is not a table of budgets: the gate ratchets every entry's closure against the committed merge-base tree, so the numbers to read are 9 for `facades/capture.ts` and 2 for `snapshot-quality-verdict.ts`. Re-installing one shared kernel predicate for both readers on this head fails six entries and names both of those, which is why the vocabulary stays a map per reader; recorded on #2872 as the accepted deviation. The `android-helper` asymmetry is not real: `SNAPSHOT_QUALITY_BACKEND_CAPABILITIES` merges the Android declaration, so both readers accept that strategy and the parity table now carries the row instead of omitting it. The client-side drop of an unknown state or backend gets its CHANGELOG entry, the justification comments shrink to one constraint sentence each, the kernel test keeps only its `@ts-expect-error`, and the word left dangling by the last rename goes.
|
The enum refactor at dedd8e8 looks clean. The delta since the earlier review touches only docs, the changelog, and one parity test row, so no runtime or Swift logic changed, and all 18 checks were green on that head. I did not run Swift compilation or the SnapshotQualityStateTests, so enum exhaustiveness and wire JSON parity rest on that green CI. The branch has since moved to 3a1d6ef, which this review does not cover. GitHub still shows a conflict with main: #2922 removed CHANGELOG.md, so the changelog entry needs to go too. Please rebase once more and get CI green on the new head. |
3a1d6ef to
6ccf3ae
Compare
|
Heads moved twice more while this was in review, and the branch is now on current Latest heads: One thing to note: Re-run on |
|
The closed-enum refactor at 6ccf3ae looks correct, and it fixes the gap flagged in the earlier review (6e9ab31). Three of the four Smoke Tests jobs and every other check passed on this commit; one Smoke Tests job (run 36029357255) is still queued, not failed, and the changed lines here (comments, one parity-test row, one kernel test) don't touch the Swift stamping path that smoke exercises, which is unchanged from 6e9ab31, where iOS smoke passed on attempt 2. I did not download the logs for run 36009426964 attempt 2, so the claim that the runner stamped a RECOVERED verdict there is the author's, and the six-entry gate failure for the shared kernel predicate is also the author's report; I compiled no Swift or TypeScript and ran no tests myself, and there's no full-tier device run that asserts snapshotQuality.state healthy/recovered, since the smoke tier doesn't reach that assertion. Not blocking: the commit message for 6ccf3ae says an unknown verdict state or backend drop "gets its CHANGELOG entry," but CHANGELOG.md was deleted in #2922 and the PR now documents this behavior change nowhere, so a release-note sentence in the PR body (the generated notes read from it) plus a fix to that stale claim on squash would help, though it can be taken or left. Once the queued Smoke Tests job finishes green on 6ccf3ae, there's nothing left in the code to stop this from merging. |
thymikee
added
the
ready-for-human
|
The red All 51 tests in that file passed and one was skipped; the timeout came from the No issue covers this yet, so the owning files are #2858's |
1 participant
Summary
The runner wrote the snapshot quality
stateas a free SwiftStringwhile the host accepts onlyhealthy | recovered | sparse, and the contracts annotation reader cast any string state into the verdict type. A one-sided rename therefore dropped the verdict and its disclosure.SnapshotQualityState: String, Codable, CaseIterableowns the runner side: the verdict field, every producer, the sparse-truncation rule. Raw-valueCodablekeeps the wire JSON unchanged;reasonCodestays open.SNAPSHOT_QUALITY_STATESandSnapshotQualityVerdict['state']derives from it. Each reader gatesstateandbackendwith aRecord<_, true>keyed over the kernel union, so a name added on one side fails to compile on the other. The maps sit in the readers: the eager-closure gate holds the contracts capture façade at 9 modules and capture-kit's verdict reader at 2, withkernel/snapshot.tsthe only module both already evaluate.contracts/fixtures/ios-snapshot-quality-states.jsonis the shared table, compared as a set by SwiftallCases, the kernel tuple, and both reader tests.Out of scope per the issue: a new state still needs its warning case.
Validation
On
6ccf3aef3(Swift lanes on159b11c45):pnpm check:affected --run: 6766 tests;eager-closure-budgets688, no entry grew;xctest-selection311 methods, 0 dark;packaged-runner-swift.AGENT_DEVICE_XCUITEST_INCLUDE_UNIT_TESTS=1): 3 focusedRunnerTestspassed;build:xcuitest:ioscompiles."heathy"; a fixture missing a state failed the Swift pin;case recovered = "rencovered"failed both Swift tests; a 4th tuple state broke both readers.Closes #2872