Repository navigation
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Size Report
Startup median (7 runs, lower is better):
|
719fab6 to
dbae81f
Compare
|
dbae81f to
27035e5
Compare
27035e5 to
a083036
Compare
a083036 to
28076fc
Compare
28076fc to
14f5c0f
Compare
14f5c0f to
1e3f5eb
Compare
1e3f5eb to
16cf635
Compare
16cf635 to
54a46e7
Compare
|
This layer now carries the Swift half of the rule ( |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The runner change looks reasonable, but I cannot call it ready yet. I reviewed 54a46e7. The one gap is live evidence for the runner route. This PR changes a device-facing path: every runner tier now publishes Not blocking, and you can take or leave these: the docs (snapshots.md#L234) and the Swift comment at SnapshotPresentedSurface.swift lines 44-45 say Could the seam change stay as it is while SnapshotPresentedSurface.swift shrinks to the class-blind rule? That rule returns presented when any node type is a presenting type (named or Element(5..8)), and unknown otherwise. The parity test would pin Of the open review threads, two do not apply here: the engine.test.ts one (r4258846755) belongs on #3578 because that file is not in this diff, and the one on the unreachable system-window exclusion (r4260955100) cannot happen on the Swift route because class names are nil there. The third (r4258846775) is also not a defect, because the test does fail on a presented tree; only its "dropped every node" comment overstates, so please resolve it. CI is still pending: Smoke Tests and Bundle Size show cancelled runs, which look like superseded runs, and one Smoke Tests job is still running. The smoke result on this head matters, since this change touches the payload every runner-served snapshot goes through. I could not diff 719fab6 against this head, so the "byte-identical merged stack" claim is unchecked. The live runner capture above must land before merge. |
54a46e7 to
c06d8ec
Compare
|
Review round fixed at |
|
Re-ran the live device table at
|
Add the rule that answers whether an iOS capture read a modal presentation over the app's own content, the golden table that pins it, and the publication that carries its answer, so the rule has a production consumer the moment it lands. The rule reads a dimming view without its enablement, unlike the occlusion cut that shares the same container class: a sheet resting at an undimmed detent keeps a dimming view that takes no touches and is still presented. `absent` is the strong answer and never the default, because a consumer arms a dismissal confirmation on it. It requires that the producer stated the capture was not capped, that every node carries the UIKit class name the rule reads, that each node is numbered once so the parent links the walk follows name the node they mean, and that the one content window under the app root holds exactly one presentation container. Positive evidence — the dimming view a presentation owns, an alert's own root class, or an element type that names a presentation — answers `presented` from a single node, so a capped capture still reports what it read. Producers that publish no class names, which is every XCTest runner tier, answer `presented` or `unknown` and never `absent`. The rule lives in `geometry-policy.ts` beside the occlusion cut rather than in `engine.ts`, because that module already owns the UIKit presentation vocabulary both readers consume and `geometry.ts` already imports it on the engine's eager path, so the placement costs nothing and leaves one module to change when UIKit does. Both readers now match class names through the same normalization, so a producer that spells a class with a module prefix cannot reach one answer in the occlusion cut and another here. Evidence is matched on the node occurrence a walk stood on, not on the number it carries, so a keyboard host's subtree cannot swallow an app window's own dimming view; only `absent`, a claim about every window the producer read, requires numbering that identifies each node once. The publication is in this layer rather than one up because `check:production-exports` refuses an exported rule with no production consumer, and a layer that ships a decision nobody can read is not a layer that passes. The Apple runner's Swift twin and its own consumer land in the next layer, where the runner acquires trees; both languages read the one fixture published here.
c06d8ec to
81b9bf5
Compare
|
The code now looks right at 81b9bf5, but the live evidence is still missing, so this is not ready to merge yet. The earlier thread on per-occurrence evidence is fixed in the classifier (golden case 17 now expects presented), and the seam test now checks its own premise before it checks the answer. The remaining gap is the runner route in RunnerTests+SnapshotCapturePlan.swift. Every runner tier now stamps presentedSurface through makeSnapshotBackendCapture, so this is the device-facing payload. The re-run reports the private-AX result as prose ("unknown on every screen") with no quoted JSON and no snapshotQuality.backend. It also skips Alert.alert on this route. Without that, nothing shows that an Alert-typed AX node reaches the classifier, or that the field survives readPresentedSurfaceFact into snapshot --json. Please run Checks are still pending. The three non-passing checks were cancelled as superseded runs, not failed, so they give no signal at this head. The iOS Smoke Tests job does overlap this diff, because it runs the runner snapshot payload that this PR extends. Bundle Size does not overlap, since the diff is Swift and docs only. I did not run swift test on apple/snapshot-presentation at this head. The 23/23 parity claim predates the 19th fixture case and the Swift normalization, so please re-run it. On the other threads, the AX-node thread on the Alert classifier is fixed at this head, so you can resolve it. The engine.test.ts thread does not apply here, because that file belongs to base #3578 and is not in this diff. The seam test thread is fixed by the premise assertion, so you can resolve it too. Small notes, not required for merge. The head commit message carries the layer 2 subject instead of this PR's. The doc comment at SnapshotPresentedSurface.swift:47-48 still points to collectPresentedSurface in engine.ts, though it now lives in geometry-policy.ts. The PR body still says 18 cases. The next step before merge is the quoted runner-route JSON for Home, the sheet and Alert.alert. |
This branch has not been deployed
1 participant
Summary
Makes the Apple runner answer the presented-surface fact in Swift, the same way layer 2's TypeScript rule
answers it (#3565). Layer 3 of 3 and the layer that closes the issue: from here every iOS capture route
carries
presentedSurface— the daemon-side engines from layer 2, and the runner's own tiers from here —so #1831's dismissal confirmation can arm on a producer fact instead of tree shape. 6 files, +515/−13.
Consumer-side narrowing stays in #3470.
On the runner, the classification happens inside
makeSnapshotBackendCapture, which now takes theacquisition beside the presentation it produced. That seam is where a tier's own tree is in hand and
in-bundle unit tests reach it, so the fact cannot go missing by reading the presented tree or by deleting
one line there. It answers
unknownwhen the tier'snodesarray is missing or empty, andpresented/unknownwhen nodes carry no class names, which is every runner tier today. The fact crossesthe wire as text because a
DecodableBool?decodes"presented"intotrue, which the sharedreader refuses; the Swift test decodes through the real
JSONValue+ wire reader, so a round-tripfailure fails the build rather than a manual run.
The Swift rule mirrors layer 2's TypeScript rule exactly, including where evidence is matched on the node
occurrence a walk stood on rather than on the number it carries, so a keyboard host's subtree cannot
swallow an app window's own dimming view, while
absentstill requires numbering that identifies eachnode once.
The Swift rule and its parity test are here rather than with the TypeScript rule that shares their golden
table, for two reasons that are the repository's gates rather than preference: layer 2 needed the
publication to give its rule a production consumer (
check:production-exports), and carrying the Swifthalf as well would have put it at 1,242 gross lines against the 1,000-line budget. The table itself is in
layer 2, so this parity test is checked against a fixture that already exists — the Swift half cannot
silently redefine a case.
Evidence coverage (iPhone 17 simulator, RN test app, head
c06d8ec9a)Rows marked live were read off the device at this head. The rest are golden cases, said so plainly.
absent--rawand every projectionabsentpageSheetsheet — window holds 2 containers, dimming views at 4 and 134presentedpresentedunknown--actions, 0 class names published)unknownpresentedAlert.alert(own window, no dimming view)presentedElement(7)presentedunknown.sheetpresentedexpectedresolveBlockingSystemModalstamps.presented(layer 1)unknown, orpresentedon an element type aloneunknown, orpresentedon evidence it did readTwo things the live run is worth stating out loud. The dev-menu floating button puts a real
EXDevMenu.DevMenuFABWindowbeside the app window, and the honest answer for that screen isunknown—the four
absentrows above were taken with it disabled(
defaults write com.callstack.agentdevicelab EXDevMenuShowFloatingActionButton -bool NO), which is thesingle-content-window shape the rule reads absence from. And the interactive cut of the open sheet
publishes four nodes, none of them a dimming view, while the fact still answers
presented: that is thefailure mode this fact replaces, since a consumer reading the published tree would have had to answer
absenton a screen with a sheet over it.Validation
c06d8ec9a, verified at this head:swift testruns the parity suite at 23/23 against the layer-2fixture,
check:production-exportspasses,pnpm buildandpnpm typecheckare clean, and vitest overpackages/capture-kit,packages/kernelandpackages/platform-appleis 2,963 green, eager-closurebudgets included. The runner's own suite runs on the iPhone 17 simulator: 430 tests, with
testCommandthe one failure — it refuses to listen without the listener port and token the daemon injects, so it
cannot pass under a bare
xcodebuild testrun, and this stack does not go near that file. The livecapture table above is from the same head.
The repository-wide local gate for this head is deliberately not claimed here: four other worktrees on
this host run subprocess-heavy suites concurrently, and the earlier local failures in this stack — an ADB
connect failure, a daemon startup timeout, one abort-timing case — were contention from exactly that,
each passing in isolation (47/47, 6/6) while failing under load. GitHub CI is still queued at this head;
the
Compatibility & Provenancelane, which the previous head genuinely failed, is the one worth readingon this PR first — its check is reproduced locally above.
Both parity tests read
contracts/fixtures/ios-presented-surface.json, the single cross-language tablefrom layer 2, so either language answering one of these 18 cases differently fails its own suite, and the
cross-language claim is enforced rather than asserted. The runner seam is pinned in
RunnerTests+SnapshotCapturePlanTests.swift: a missingnodesarray answersunknownwithpresentedSurfacePresentednil, a bareSheetelement type answerspresented(a positive is strongeven with no class names), and an unnamed tree answers
presented/unknownand neverabsent. Thatseam's test now also asserts its own premise — that the presentation the tier produced does not carry the
node the evidence sits on — so the assertion below it cannot silently become a test that reads one tree
twice; swapping the evidence node for one the projection retains fails that premise assertion.