Repository navigation
chore: print alert corpus surfaces through the Swift owner - #4186
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
![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 5 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: 12 finding(s): 2 minor, 10 nit. Verdict: approve. 🟡 Minor (2)
🔵 Nit (10)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 12 new findings on 7ee7dd8. The verdict and the full list are in the summary comment.
| let plain: [Node] | ||
| let honorModal: [Node] | ||
|
|
||
| func scope(_ nodes: [Node], allowEmpty: Bool) throws -> [ModalScopeNode] { |
There was a problem hiding this comment.
🟡 Minor · Load failures no longer name the file, tree, or node; README still promises they do
The removed TS script named every load failure precisely: <file>: "plain" node 5 does not load. Capture it again., <file>: "honorModal" is not a tree, and format 3 is not 2. The Swift port throws CocoaError(.coderReadCorrupt) from scope(_:allowEmpty:) and Rect.frame(), and main.swift prints only error.localizedDescription. For Cocoa error 4864 that string is the generic "The data doesn't have the correct format." (or "The operation couldn't be completed. (Cocoa error 4864.)"), naming neither the file, nor which tree, nor which node. The same applies to JSONDecoder failures: a recording missing format, with "format": "2" as a string, or with a node whose rect lacks x, throws a DecodingError whose localizedDescription is the generic "The data couldn't be read because it isn't in the correct format." with no key path or index (the detail lives in DecodingError's associated .errorDescription/.codingPath, which is never read). The README line the diff keeps — 'If the recording does not load in the replay, the command stops with one line that names the problem' (Tests/AlertSurfaceCorpus/README.md:48) — is no longer true for these cases, and the README flow depends on it: step 3 is how a contributor learns a recapture is malformed before they write expect. Failing input: any corpus JSON where a node's index differs from its position (a hand-trimmed recording, or one edited by a merge conflict) now prints one unhelpful CocoaError line instead of naming the node.
| func scope(_ nodes: [Node], allowEmpty: Bool) throws -> [ModalScopeNode] { | |
| func scope(_ nodes: [Node], name: String, allowEmpty: Bool) throws -> [ModalScopeNode] { | |
| func corrupt(_ position: Int, _ what: String) -> NSError { | |
| NSError(domain: "print:alert-surface", code: 1, userInfo: [ | |
| NSLocalizedDescriptionKey: "\(name) node \(position) does not load (\(what)). Capture it again." | |
| ]) | |
| } | |
| guard allowEmpty || !nodes.isEmpty else { throw corrupt(0, "\"\(name)\" is not a tree") } | |
| return try nodes.enumerated().map { position, node in | |
| guard node.index == position else { throw corrupt(position, "index \(node.index)") } | |
| if let parent = node.parentIndex { | |
| guard parent >= 0 && parent < position else { throw corrupt(position, "parentIndex \(parent)") } | |
| } else if position != 0 { | |
| throw corrupt(position, "no parentIndex") | |
| } | |
| let frame: CGRect | |
| do { | |
| frame = try node.rect.frame() | |
| } catch { | |
| throw corrupt(position, "rect") | |
| } | |
| return ModalScopeNode( | |
| element: node.element, | |
| parentElement: node.parentIndex.map { nodes[$0].element }, | |
| frame: frame | |
| ) | |
| } | |
| } |
Suggested change
Wrap each failure in a message that names the file, the tree name and the node index (mirroring the removed TS messages, e.g. an error type whose errorDescription is that sentence), or amend README line 48 to say the command stops with one line that does not identify the node.
Details
Also raised here
Also raised at this location by javascript: New Swift printer's failure lines no longer name the file, tree or node, breaking the README's promise — The removed TypeScript printer reported every load failure with the file, the tree name and the offending node index, e.g. apple/.../x.json: "honorModal" node 42 does not load. Capture it again. and x.json: format 1 is not 2. Capture it again. (see fail and the checks in the deleted scripts/print-alert-surface-tree.ts). The replacement throws instead, and main.swift prints only error.localizedDescription. Two reachable failures therefore lose the information the README documents: Recording.scope throws CocoaError(.coderReadCorrupt) when a node's index is not its position, when a parentIndex is out of range, or when a non-root node has no parent (PrintAlertSurface.swift:39-45). CocoaError(.coderReadCorrupt)'s localizedDescription is the fixed string "The data isn’t in the correct format." — it names no file, no tree and no node. …
persona: swift-ios, senior-developer, javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| var markers: [Int: String] = [:] | ||
| switch reading { | ||
| case let .modal(surfaces): | ||
| answer = "modal" |
There was a problem hiding this comment.
🟡 Minor · Reading line prints a module-qualified reflection description for unstable reasons and drops the node index
answer = "unknown(\(reason))" interpolates ModalSurfaceUnknownReason, which conforms only to Equatable (ModalSurface.swift:16-28). Payload-free cases print as the bare case name, so duplicatedChild, rootMismatch, unorderedRead and noSurvivingSurface match the unknown(<reason>) form README.md:46 and README.md:64-70 tell the contributor to read. But .unstable(ModalScopeInstability) has an associated value, so Swift's interpolation uses the mirror-based description and the line reads e.g. unknown(unstable(AgentDeviceSnapshotPresentation.ModalScopeInstability.movedElement)) instead of the unknown(unstable(movedElement)) the removed TS printer emitted (see its pairInstability, deleted in this PR).
| answer = "modal" | |
| case let .unknown(reason): | |
| switch reason { | |
| case let .unstable(instability): answer = "unknown(unstable(\(instability.rawValue)))" | |
| default: answer = "unknown(\(reason))" | |
| } | |
| } |
Suggested change
Print the reason in the stable form the README documents: switch over ModalSurfaceUnknownReason and write "unknown(\(name))" with unstable rendered as its ModalScopeInstability raw value (instability.rawValue), e.g. unknown(unstable(movedElement)). Either keep the node index/element id in the duplicatedChild line or state in the README how to find the offending node without it.
Details
The node index is also gone. The TS printer returned unknown(unstable(movedElement)): node 42 and unknown(duplicatedChild): node 7 kept 1 of 3 children with element id 99943:…, which the contributor used to locate the offending node in the printed tree and in the recording JSON. The new line names neither the node nor the element, while README.md:64-68 now only says to "Inspect repeated AX element ids under the same parent in the recording" — with no pointer to which parent. On a 248-node recording that is a search with no starting point.
Also raised here
Also raised at this location by senior-developer: unknown(...) printout drops the node index / element id the README tells contributors to search for — The old TS printer emitted the failing node (and, for duplicatedChild, the raw AX element id) in the first line: unknown(unstable(reparentedElement)): node 42 and unknown(duplicatedChild): node 5 kept 1 of 2 children with element id 99943:4829729792.1219. PrintAlertSurface.swift line 86 interpolates a ModalSurfaceUnknownReason with String(describing:), so the tool now prints only unknown(duplicatedChild) / unknown(unstable(reparentedElement)) / unknown(rootMismatch) — no node, no element id, no kept/listed counts. The README still routes the contributor through that missing data: it says "a node lists one element more than once … Inspect repeated AX element ids under the same parent in the recording" and warns not to write {"reading": "unknown", "reason": "duplicatedChild"} until those copies are confirmed. Without the offending node/element the contributor has to diff the two trees by hand in a tree with dozens of nodes, which is the work the command exists to save. …
persona: javascript, senior-developer
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| let width: Double? | ||
| let height: Double? | ||
|
|
||
| func frame() throws -> CGRect { |
There was a problem hiding this comment.
🔵 Nit · Rect.frame() maps nonFinite to CGRect.infinite, so the printed reading can differ from the replay's
Rect.frame() returns .infinite for infinite == true and builds CGRect(x: -.infinity, y: -.infinity, width: .infinity, height: .infinity) for nonFinite == true — the same CGRect for two different recorded facts (CGRect.infinite is exactly that rect). ModalScopePair.pair compares frames with ==, so a plain node flagged infinite and its honor-modal counterpart flagged nonFinite now pair as stable, while the removed TS predecessor keyed them differently ('infinite' vs 'nonFinite') and reported unknown(unstable(movedElement)): node N. Whether the printer or the predecessor matches the replay depends on how the replay's FixtureRect.cgRect maps those two flags; if it keeps them distinct, the printed first line disagrees with swift test for such a recording, and step 5 of the README ('compare the rule's reading on the first line with what you wrote') sends the contributor the wrong way.
Suggested change
Check FixtureRect.cgRect: if it distinguishes the infinite and nonFinite flags, build a distinct non-finite rect (or reject the recording) here so the printed reading matches what the replay computes.
Details
Also raised here
Also raised at this location by maintainability: nonFinite rect is decoded to all -infinity origin, which is not a valid frame and is not what the replay's invalidFrame produces — That is not a valid CGRect origin (an infinite origin is exactly what CGRect.infinite avoids by using -inf for x/y only as a special case), and the corpus's own invalidFrame unknown reason shows the replay path can reject a frame rather than invent one. If the package's recording loader maps the nonFinite flag differently, this tool prints a different ModalSurface answer than the corpus replay records for the same recording — the one contract the file exists to keep. Map the flag through the package's own recording decoding instead of reconstructing the rect here.
persona: swift-ios, maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| import Foundation | ||
|
|
||
| private struct Recording: Decodable { | ||
| struct Rect: Decodable { |
There was a problem hiding this comment.
🔵 Nit · Recording.Rect duplicates the corpus rect contract that FixtureRect already owns
Recording.Rect.frame() (PrintAlertSurface.swift:6-23) re-implements the JSON rect contract that FixtureRect (Tests/AgentDeviceSnapshotPresentationTests/FixtureRect.swift:7-29) already encodes: infinite == true wins over nonFinite == true, nonFinite == true yields CGRect(x: -.infinity, y: -.infinity, width: .infinity, height: .infinity), otherwise x/y/width/height are required. The printer reads the same corpus files the replay reads, so the two decoders must agree node-for-node or the printed reading is not the reading the tests check — which is the whole premise of the tool (README.md:44 "runs the Swift package's rule directly"). FixtureRect lives in the test target and is not public, so the executable target cannot reuse it; nothing enforces the two staying in step, and a future rect encoding change (a new flag, or the replay accepting a null rect) silently makes the printer's frames differ, which changes ModalScopePair.pair's movedElement comparison and thus the printed reading.
Suggested change
Move the rect decoding into the AgentDeviceSnapshotPresentation library target as a public type (or a public ModalScopeNode JSON decodable) and have both the corpus tests and the print mode decode through it, so the printed reading cannot diverge from the replayed one.
Details
persona: javascript
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| let format: Int | ||
| let plain: [Node] | ||
| let honorModal: [Node] | ||
|
|
There was a problem hiding this comment.
🔵 Nit · scope(_:allowEmpty:) does not scope; the flag is an unexplained asymmetry
scope returns [ModalScopeNode] for a whole recorded tree — nothing about it scopes — while the meaningful behavior is the allowEmpty flag, which is false for plain and true for honorModal with nothing to say why. Name it after what it does (modalScopeNodes(_:requireNodes:), or two small functions) and keep the deleted TS's comment stating that an empty honor-modal tree is left to the reading, which names it; that reason is now only implicit in a boolean argument.
| // The checks the corpus replay's loader makes. An empty honor-modal tree is left to the reading, | |
| // which names it; an empty plain tree is not a tree. | |
| func modalScopeNodes(_ nodes: [Node], requireNodes: Bool) throws -> [ModalScopeNode] { |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| case let .unknown(reason): | ||
| answer = "unknown(\(reason))" | ||
| } | ||
| print("ModalSurface reads (the rule's answer, not ground truth): \(answer)") |
There was a problem hiding this comment.
🔵 Nit · Tree choice reads arguments.last, so a recording named plain switches the default tree
let printPlain = arguments.last == "plain" picks the tree from the last argument, but the guard above only validates arguments[1] when there are two arguments. With a single argument, arguments.last is the file path, so pnpm print:alert-surface plain (a recording named plain in the working directory) silently prints the plain tree; the deleted script destructured [file, treeName = 'honorModal'], so a one-argument call always printed honorModal. Test the validated second argument instead of the last argument.
| print("ModalSurface reads (the rule's answer, not ground truth): \(answer)") | |
| let printPlain = arguments.count == 2 && arguments[1] == "plain" |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| let printPlain = arguments.last == "plain" | ||
| let nodes = printPlain ? recording.plain : recording.honorModal | ||
| var depths: [Int] = [] | ||
| let encoder = JSONEncoder() |
There was a problem hiding this comment.
🔵 Nit · Label is quoted by encoding a top-level String with JSONEncoder, which throws unless .fragmentsAllowed is set
let label = String(decoding: try encoder.encode(node.label), as: UTF8.self) encodes a bare String at the top level of a JSONEncoder. encoder.outputFormatting sets only .withoutEscapingSlashes; it does not set .fragmentsAllowed. Foundation's writer rejects a top-level JSON fragment ("Top-level String encoded as string JSON fragment. Consider adding 'fragmentsAllowed' to the encoder used to encode this value.") unless that option is present, so on a toolchain whose JSONEncoder still enforces it, the very first printed node throws, the error goes to stderr from main.swift and the command exits 1 for every recording — the print command prints nothing but the reading line. The removed TS predecessor used JSON.stringify(node.label), which has no such constraint, so this is a new failure mode on a line the diff adds. I could not run a Swift toolchain in this environment to settle the behaviour for the deployment toolchain (recent Foundation builds relaxed it), hence the lowered confidence; note that no test exercises printAlertSurface, so swift test (50 passed) would not catch it. The try on that line is also only there to satisfy this encode call — no other work in the print loop can throw — which is a hint the encoding was not meant to be fallible.
| let encoder = JSONEncoder() | |
| let encoder = JSONEncoder() | |
| encoder.outputFormatting = [.withoutEscapingSlashes, .fragmentsAllowed] |
Suggested change
Quote the label without a JSON encoder, or set the option the encode needs: either use encoder.outputFormatting = [.withoutEscapingSlashes, .fragmentsAllowed] (and keep try), or replace the encode with "\\\(node.label)\\" built from a small escaping helper, so the print path has no throwable step.
Details
persona: swift-ios
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| let depth = node.parentIndex.map { depths[$0] + 1 } ?? 0 | ||
| depths.append(depth) | ||
| let label = String(decoding: try encoder.encode(node.label), as: UTF8.self) | ||
| let index = String(repeating: " ", count: max(0, 4 - String(node.index).count)) + String(node.index) |
There was a problem hiding this comment.
🔵 Nit · Hand-rolled padStart with String(repeating:) and count
let index = String(repeating: " ", count: max(0, 4 - String(node.index).count)) + String(node.index) re-implements right-padding by hand, and String(node.index) is computed twice on the line. The max(0, ...) guard is only there because String(repeating:count:) traps on a negative count, which a reader has to know to see why it is safe. String(format: "%4d", node.index) says the same thing in one call.
| let index = String(repeating: " ", count: max(0, 4 - String(node.index).count)) + String(node.index) | |
| let index = String(format: "%4d", node.index) |
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| ) | ||
| FileHandle.standardOutput.write(output) | ||
| FileHandle.standardOutput.write(Data([0x0a])) | ||
| if CommandLine.arguments.dropFirst().first == "print-alert-surface" { |
There was a problem hiding this comment.
🔵 Nit · print-alert-surface literal is written in three places across two languages
The mode word appears as the dispatch comparison in main.swift and in the print:alert-surface usage string inside PrintAlertSurface.swift, plus the npm script. The usage text is also a pnpm invocation returned from a function that is reached through the raw swift run subcommand, so someone following it gets the pnpm wrapper's failure, not the tool's, when the package is not built. Keep one constant for the subcommand and phrase the usage line for the executable this file is compiled into.
Suggested change
Export one printAlertSurfaceCommand constant from PrintAlertSurface.swift, compare against it in main.swift, and make the usage line show the executable plus the subcommand.
Details
persona: maintainability
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
| `ModalSurface` reads, labeled as the rule's answer, not ground truth. It is `modal`, | ||
| `noExclusion`, or `unknown(<reason>)` with the node where the read failed, and nodes are | ||
| marked only when it is `modal`. To print the whole tree, add `plain` after the path. If the | ||
| The command runs the Swift package's rule directly. Each line after the first shows a node's |
There was a problem hiding this comment.
🔵 Nit · Rewritten README paragraph crams four sentences onto one overlong line
The replacement paragraph wraps differently from the rest of the file: line 47 ( It is \modal`, `noExclusion`, or `unknown()`. Hosts are marked only for `modal`. To print the whole tree, add `plain` after the path. If the`) runs well past the ~90-column wrap every other line in README.md keeps, while the two lines above it were re-wrapped. AGENTS.md asks agent docs to follow ASD-STE100 — 'Write one instruction in each sentence. Put the condition before the instruction' — and this line packs three unrelated statements (what the reading can be, when hosts are marked, how to print the whole tree) onto one line, which is also the sentence the reader needs when a recording shows no hosts.
Suggested change
The command runs the Swift package's rule directly. Each line after the first shows a node's
index, type, and label. [surface roots: 3] marks a modal host and lists its kept roots; hosts
are marked only when the reading is modal. The first line is what ModalSurface reads,
labeled as the rule's answer, not ground truth. It is modal, noExclusion, or
unknown(<reason>). To print the whole tree, add plain after the path. If the
recording does not load in the replay, the command stops with one line that names the problem.
Details
persona: swift-ios
Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.
Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.
|
At 7ee7dd8 two open threads still hold and need an answer or a fix before merge: the SwiftPM cleanup and the empty-tree policy (links below). The executable and the replay tests also load recordings with two separate policies, which should converge on one. Smoke Tests is still running, and there are no conflicts. Could the Swift print mode and TypeScript deletion stay, while package-local tooling support shares raw-recording tree and rectangle decoding and validation between the executable and replay tests? This would require separating the raw recording model from RecordedAlertSurface expectation fields; screenshot expectations can stay in the test wrapper, without exposing fixture formats in the production public API. The open SwiftPM cleanup and empty tree policy threads still apply, along with nine lower-priority threads. The rectangle distinction does not apply because both implementations construct the same cases. The error factory is optional because the domain is consistent. The JSON encoding suggestion uses an invalid option for JSONEncoder. The index padding already handles longer indexes. The mode token concern mistakes the pnpm command for a dispatch token. I read the code and did not run it; the old and new printer output was not available to compare. Existing corpus tests predate this change and do not establish print-mode dispatch or output regression coverage. SwiftPM leakage and toolchain-specific enum rendering were not reproduced. |
7ee7dd8 to
791e1ca
Compare
|
Fixed this review round: SwiftPM uses the cleanup wrapper; printer and replay share package-only raw recording/FixtureRect decoding, nonempty-tree validation, and tree construction. Screenshot expectations stay in tests. Malformed-node diagnostics now identify file/tree/position; decoding errors preserve their path. Also fixed explicit tree selection and unstable-reason rendering. Added print dispatch/output coverage. All four Cubic threads have replies and are resolved. The Swift unknown-reading API contains no failing-node index to preserve. No Apex replies. Sample Safari output: Settings also reads Rebase: old base |
|
Apex Sentinel could not review commit Apex Sentinel could not read or check out this pull request, which is usually temporary (GitHub was slow, or the branch moved while the review waited). If it keeps failing, tell whoever runs Sentinel for this repository, or check that the App still has access to it. Push a new commit or comment |
|
The packaging growth came from the development-only Validation:
Parent independent review found no issues: the packager excludes a target absent from Package.runner.swift, the regression proves exclusion, and runtime source/manifest contents stay intact. The package-local shared-loader design also matches the maintainer request without exposing fixture formats in the production library. |
|
The code looks good at b98908d. The printer and the replay tests now share one Swift recording loader that enforces the non-empty tree policy and reports actionable diagnostics, and the SwiftPM build runs through the toolchain temporary-directory wrapper. I read the code and did not run the tests, so the reported validation is yours. Smoke Tests and Coverage pass; the only check not passing is the external review bot status, which runs no repository code. There are no conflicts. The threads on the SwiftPM temporary directory, empty trees, diagnostics and the shared recording loader are fixed and can be resolved, and the remaining ones either no longer apply or are minor. |
thymikee
added
the
ready-for-human
Summary
pnpm print:alert-surfaceruns the Swift modal-surface owner instead of a duplicate TypeScript rule. Printer and replay tests share package-local raw-recording and rectangle decoding, validation, and actionable file/tree/node diagnostics. Empty-tree policy matches replay tests, and SwiftPM runs through the existing temporary-directory cleanup wrapper.The shared
RecordingSupporttarget stays development-only. The runtime source packager excludes it alongside the existing conformance/test targets, removing two unused files from installed packages while retaining the shared loader in the checkout.15 files changed; no linked issue supplied. The final packaging round changes two files (+10 lines).
Validation
Head
b98908d3a9e4ef026317631ed839adfda9e2d199; basec8ad8fb1257787ce0973cf74271cb9a4fafb51d9.791e1cadplus the new fixture: 1 failed/11 passed because tooling shipped; fixed owner: 12 passed. Experiment removed; checkout unchanged.pnpm check:affected --run --head b98908d3a9e4ef026317631ed839adfda9e2d199passed, receiptrun-XkVpW0,exactCommit=true,checkoutChanged=false; 41 passed, 25 skipped.791e1cad, all 51 Swift tests and both printer samples passed. Packaging-only follow-up leaves those sources unchanged.Parent independent review found no issues in the packaging boundary or shared-loader design. Hosted coverage/native/device obligations remain CI-owned. No merge is claimed.