Repository navigation
Conversation
ADR 0031 decides that a native macOS app screenshot captures only the app's front window through ScreenCaptureKit (#3188). When the front window's WindowServer ID was missing from the shareable candidates, the helper captured the app's largest window instead, so the image could show a window the user was not looking at. The helper now selects the front window by ID or refuses with a typed reason: `window-not-found` when the app has no on-screen window, `front-window-not-shareable` when ScreenCaptureKit does not offer the front window. The shared outcomes fixture pins both reasons for Swift and TypeScript, and the host adds a hint for each. Evidence: the audit at bd4fd68 (AppWindowScreenshot.swift:21); re-verified on 4c1c786. Part of #3744.
ADR 0025 decides that only CoreDevice facts identify a panel: the sole lit panel, otherwise the `primary` panel, and never array position. When no panel reported `primary` and power did not decide one lit panel, the inventory captured the first lit or first integrated panel in list order. That inventory now names no panel, and capture-target resolution refuses with COMMAND_FAILED, reason `capture-panel-unidentified`, and a hint. The sole-lit-panel path and the primary-panel path are unchanged. The foldable help topic states the refusal. Evidence: the audit at bd4fd68 (display-inventory.ts:261); re-verified on 4c1c786. Part of #3744.
![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 10 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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):
|
|
|
The Swift change looks right, but the native-backend screenshot route has no live evidence yet, so it needs one run on a real host before merge (reviewed at dec7c9b). AppWindowScreenshot.swift:19 changes the native-backend route for every macOS app capture. When there is no front window ID, or ScreenCaptureKit does not list the front ID, it now throws. It used to return the largest window. If a live app has a CGWindowList front window that SCShareableContent drops (for example a zero-area or transient layer-0 window), every screenshot of that app would now fail. No fixture can show whether that happens, and no live run reached this route because the author's host lacks Screen Recording permission. Could you run it on a host with Screen Recording granted? Set I did not run the Swift or Vitest suites. The regression value of the Swift tests relies on the author's planted-fallback report, because Smoke Tests failed in the iOS runner suite on RunnerTests.testSynthesizedReplacementLeavesAMaskedFieldUnverified (RunnerTests+SynthesizedTextEntryTests.swift:169), where the masked-field bullet count was 13 instead of 8. This PR changes nothing under apple/runner or the text-entry route, so the failure looks unrelated. No conflicts. Of the open inline threads, the two docs-wording notes still apply: system/index.ts:281 (lit panel wording) and helper.ts:157-158 (retry hint wording). The helper.test.ts:247 thread does not apply: the PR leaves screenshot argv unchanged, and the existing argv test owns that shape. You can resolve it. Before merge, this PR needs the live native-backend evidence above: a front-window PNG from on-screen TextEdit, and the window-not-found refusal with the hint when every TextEdit window is minimized. |
Choose the first on-screen normal-layer app window with positive alpha and nonempty bounds. Keep interaction window selection unchanged and refuse if an eligible front window is absent from ScreenCaptureKit. Review: #3761 (comment)
|
Follow-up head:
Previous-head CI: only iOS Smoke Tests failed, at the unchanged masked-field assertion (13 bullets vs 8). PR #3722 has the identical failure. That runner XCTest exercises neither changed capture route. Android/Linux/macOS smoke, Coverage, Integration, package/guards, docs/preview, CodeQL, Size, and cubic passed; the fixture-cache matrix job was skipped. New-head CI is pending. Live check remains with maintainer: preflight is false; native TextEdit screenshot returned |
|
Live macOS evidence at Setup:
Not run: a sheet or popover, a fullscreen Space, and the iOS 27.1 iPhone Duo (this host has no iOS 27.1 runtime). Cleanup: test documents closed, |
|
Addressed all three follow-up findings in separate commits: M1 At Live on this Mac with Prior-head CI is green; new-head CI is pending. |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Share the on-screen window-list query while preserving nil versus empty point refusals. Pin layer-8 alert exclusion at the shared selection boundary. Review: #3761 (comment)
|
Round 4 head:
Validation: exact-head affected gate passed ( Live native TextEdit, rebuilt helper: 1312×844 PNG visibly captures New-head CI pending; monitoring for up to ~40 minutes. No merge or approval. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Refuse screenshot and scroll with window-list-unreadable when the shared window-list read fails, and preserve a retry hint through the host helper client. Review: #3761 (comment)
|
Round 5 head: Screenshot and scroll now refuse nil window-list reads with Threads: 1 fixed and resolved (cubic); 0 rejected/open. Issue comments, submitted reviews, and inline threads reread; no new maintainer items. Follow-up self-review found no further issue; no separate reviewer used this round. Validation: 16 helper + 6 input Swift tests, 26 focused TypeScript tests, format, and XCTest selection passed. Current-head CI is queued/in progress, with no result yet. Previous-head completed checks passed; iOS/macOS smoke were pending. PR is conflict-free. No merge or approval. |
|
This PR is ready at 70acf88. The two points from the earlier review (#3761 (comment)) are fixed: a nil window list now throws window-list-unreadable instead of collapsing to window-not-found, and requireAppWindowPoint now reads appWindowInfo(), so one CGWindowListCopyWindowInfo definition remains. Not blocking, and you can take or leave these: requireAppWindowPoint (BackgroundInteraction.swift:309) still refuses an unreadable list as noAccessibleTarget, so press and fill never reach window-list-unreadable (the rule would be that every nil appWindowInfo() read refuses with window-list-unreadable), and the window-list hint in runMacOsHelper (helper.ts:386) is a literal, where a Record<WindowListRefusalReason,string> table beside MACOS_SCREENSHOT_REFUSAL_HINTS would give one hint mechanism. On the open threads, both Cubic threads are fixed at this head, so you can resolve them: #3761 (comment) (nil list now throws) and #3761 (comment) (single window-info read). All 20 checks pass at 70acf88, and no conflicts are known. I did not run the Swift or Vitest suites. I judged the nil-list test from the diff, since the old |
thymikee
added
the
ready-for-human
The helper owns each refusal's reason and remedy, so its details envelope carries the hint and the TypeScript client needs no lookup table, wrapper, or special case. Collapse the one-case WindowListRefusal enum and the screenshot Result mapping into the throw sites that already own them, and keep the front-window selection as one plain function. Press, fill, and their window fallback use the non-throwing path; requireAppWindowPoint has already refused an unreadable list there. The front-window help sentence states the layer-0 rule and the alert exclusion once.
|
Head
|
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/macos-helper/Sources/AgentDeviceMacOSHelper/BackgroundInteraction.swift">
<violation number="1" location="apple/macos-helper/Sources/AgentDeviceMacOSHelper/BackgroundInteraction.swift:370">
P2: `try?` here turns an unreadable window list into "no front window", so press and fill refuse with a generic `no-accessible-target` reason instead of `window-list-unreadable`. That contradicts the `frontWindow` doc comment. Let `actionWindow` throw again and propagate the error through `resolvePressTarget` and `fillInBackground`, or pass the window info read by `requireAppWindowPoint` into it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| if let window = elementAttribute(hit, attribute: kAXWindowAttribute as String) { return window } | ||
| } | ||
| guard let front = frontWindowFrame(pid: app.processIdentifier) else { return nil } | ||
| guard let front = try? frontWindowFrame(pid: app.processIdentifier) else { return nil } |
There was a problem hiding this comment.
P2: try? here turns an unreadable window list into "no front window", so press and fill refuse with a generic no-accessible-target reason instead of window-list-unreadable. That contradicts the frontWindow doc comment. Let actionWindow throw again and propagate the error through resolvePressTarget and fillInBackground, or pass the window info read by requireAppWindowPoint into it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apple/macos-helper/Sources/AgentDeviceMacOSHelper/BackgroundInteraction.swift, line 370:
<comment>`try?` here turns an unreadable window list into "no front window", so press and fill refuse with a generic `no-accessible-target` reason instead of `window-list-unreadable`. That contradicts the `frontWindow` doc comment. Let `actionWindow` throw again and propagate the error through `resolvePressTarget` and `fillInBackground`, or pass the window info read by `requireAppWindowPoint` into it.</comment>
<file context>
@@ -360,13 +360,14 @@ private func pressSearchChain(from hit: AXUIElement) -> [AXUIElement] {
if let window = elementAttribute(hit, attribute: kAXWindowAttribute as String) { return window }
}
- guard let front = try frontWindowFrame(pid: app.processIdentifier) else { return nil }
+ guard let front = try? frontWindowFrame(pid: app.processIdentifier) else { return nil }
return windows(of: AXUIElementCreateApplication(app.processIdentifier)).first { window in
guard let rect = rectAttribute(window) else { return false }
</file context>
There was a problem hiding this comment.
Not reachable as described: press and fill call requireAppWindowPoint before actionWindow (BackgroundInteraction.swift:98, 171), so an unreadable list already refused there (no-accessible-target, "the app's on-screen windows could not be read") before frontWindowFrame is asked. The try? only covers the race where the list turns unreadable between those two reads, and the approved design treats that as no fallback window. window-list-unreadable is owned by frontWindow and propagates through screenshot and scroll, which have no earlier read.
|
The code at 6d40fbc looks good, and I found nothing new to fix. The earlier review at 70acf88 was clean (#3761 (comment)). The delta since then removes the extra window-list definition and makes CI is still running. Smoke Tests, Repo Guards & Compatibility and Coverage are queued or in progress, with no failures so far. Repo Guards covers the deleted contracts fixture keys and the I did not re-run the live macOS evidence, and I did not check that the helper was rebuilt from this head, as the PR body says. I did not run the Swift tests. The Two cubic-dev-ai threads are fixed at this head, so please resolve them. #3761 (comment) is fixed because Before merge, the pending Smoke Tests, Repo Guards & Compatibility and Coverage jobs need to finish green. |
|
The delta since 70acf88 holds up. At 6d40fbc the code is still in good shape: captures now refuse when the window or panel identity is unknown, and I found no new problems. CI is green, with all 20 checks passing, and there are no conflicts. The Swift hint pins in assertRefusal and assertUnreadableList look right from the diff. They would fail if Swift dropped a hint, because the TS fallback that supplied hints is gone. I did not reproduce the live macOS native run or run the Swift tests. The PR body reports the live run at 6d40fbc: TextEdit captures, plus a window-not-found refusal that carries its hint. The capture-excluded and not-shareable refusals are covered by fixtures only. The author's live run therefore supplies the evidence that a real capture succeeds and that a real refusal carries its hint. No further run is needed from me. One question on simplicity. The app's front window (OnScreenWindow, frontWindowLayer, appWindowInfo, frontWindow and its window-list-unreadable refusal) is now shared by screenshot capture and point actions. Should it move out of BackgroundInteraction.swift into its own module, such as AppFrontWindow.swift, so AppWindowScreenshot.swift does not depend on the interaction file? And should requireAppWindowPoint use the same window-list-unreadable refusal? Today one condition, an unreadable WindowServer list, reports two typed reasons: no-accessible-target for press and fill, and window-list-unreadable for screenshot and scroll. The cubic-dev-ai P2 thread at #3761 (comment) does not apply, so you can resolve it. Press and fill call requireAppWindowPoint first, and it refuses on an unreadable list at BackgroundInteraction.swift:309. The try? at :370 only covers a list that becomes unreadable between the two reads, and the merge-base actionWindow already treated a nil frame as no window. Nothing else blocks this PR. Please answer the simplicity question above, or say why the current split is better, before it goes to human review. |
thymikee
removed
the
ready-for-human
This branch has not been deployed
1 participant
Summary
snapshotremedy. Each refusal builds its reason, hint, and front-window id in the Swift helper, so the TypeScript client forwards the details envelope without a lookup table. Screenshot and scroll reportwindow-list-unreadablewith a retry hint when the window-list query fails.capture-panel-unidentifiedwhen neither panel power nor primary identity selects a panel. Help warns that an ambiguous primary panel may be dark.10 files. Part of #3744. ADR 0031 and modal-panel eligibility remain outside this PR. Helper packaging (npm
filesand Package.swift test targets) is owned by a separate PR.Validation
6d40fbc0f2:pnpm check:affected --run --head 6d40fbc0f2passed (exactCommit=true) (receiptrun-ggnrE5).6d40fbc0f2: Swift debug build and 16 helper + 6 input tests passed, including nil-versus-empty reads and per-refusal hint pins. 12 focused helper tests and 1338 capture/platform-apple tests passed. Format, lint, typecheck, layering, fallow, and docs checks passed in the receipt above.COMMAND_FAILED,details.reason: "window-not-found", anddetails.hint: "Open or unminimize a window of the app on the current Space, then retry the screenshot."reaching the CLI untouched. Sessions closed, daemon stopped, state dir removed.