Skip to content

fix: separate screen capture and accessibility ownership - #722

Merged
steipete merged 1 commit into
mainfrom
fix/phase5-screen-attribution-20260912
Sep 13, 2026
Merged

fix: separate screen capture and accessibility ownership#722
steipete merged 1 commit into
mainfrom
fix/phase5-screen-attribution-20260912

Conversation

@steipete

@steipete steipete commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Host-routed screen captures with Accessibility detection were rejected because the Bridge validator treated the AX application's identity as if it were the screen raster's target. Pixels-only capture worked; adding AX metadata produced the reported “unexpected application evidence” refusal.

Keep screen pixels globally scoped and validate the AX context as separate semantic evidence. Reuse the canonical process-generation/window-receipt validator, and independently check focused-element PID, window, role, and frame when no exact-window receipt is present. The capture target still rejects application/window claims, and unreceipted AX window hints never become exact-window pixel authority.

Addresses the attribution portions of #715 and #710. Premature artifact publication on rejected observations is separate work; this PR does not claim to fix it. ScreenCaptureKit coordination and fallback policies are unchanged.

Validation:

  • Signed native Bridge 1.38 before/after proof through a task-owned daemon and a separate public-API client with an explicit trusted signing team. Explicit classic capture used the same host throughout.
  • Before: pixels-only succeeded; screen+AX was refused with “response evidence ... requested unexpected application evidence.”
  • After: both succeeded; screen+AX returned 12 elements, a semantic PID and generation, and no application target on the screen raster. Captured PNGs remained local and were removed after structural verification.
  • swift test --package-path Core/PeekabooCore --scratch-path Apps/CLI/.build --filter 'PeekabooBridge.*Observation|RemoteDesktopObservation': 51 + 7 tests passed. New signed-bundle cases accept valid independent semantic ownership and reject missing generations, wrong receipt owners/bounds, and contradictory focused-element metadata.
  • Isolated Codex autoreview: complete candidate clean at P0–P2 after addressing its focused-element ownership finding.
  • CLI build, SwiftFormat, SwiftLint (zero violations), docs lint, and diff checks passed. Updated capture documentation and Unreleased.
  • Exact-head hosted checks passed: macOS CI, CodeQL, and Bugbot.

@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@cursor

cursor Bot commented Sep 12, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes signed Bridge observation receipt validation for screen+AX paths; incorrect rules could accept bad semantic evidence or still refuse valid captures, but scope is limited to screen-target binding and is covered by new tests.

Overview
Fixes host-routed screen observations with Accessibility detection being rejected when AX metadata carried an application/window owner while the capture target stayed display-scoped.

Bridge observation binding no longer treats elements.metadata.windowContext application/window fields as forbidden “application evidence” on .screen targets. For screen captures it runs a dedicated validateScreenSemanticOwner path: generation-bound semantic owner via DesktopTargetIdentityCoalescer, focused-element PID/window/role/frame checks, and rejection of inconsistent window receipts—without promoting screen pixels to an exact-window capture. Window evidence validation similarly allows AX window context on screen targets while capture metadata still must not claim a window target.

Adds signed-bundle tests for valid screen+AX combinations and for rejected invalid owners. see docs and CHANGELOG note that screen rasters stay global and window-bound follow-ups need explicit --window-id.

Reviewed by Cursor Bugbot for commit f04602e. Bugbot is set up for automated code reviews on this repo. Configure here.

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 12, 2026
@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 12, 2026, 7:19 PM ET / 23:19 UTC.

ClawSweeper review

What this changes

The PR separately validates the Accessibility application's identity in screen observations, adds signed-receipt regression tests, and documents the distinction between screen pixels and window authority.

Merge readiness

Ready for maintainer review

This fix remains necessary: current main and v4.3.4 still reject valid screen Accessibility evidence. No actionable patch defect was found. The related reports also contain separate problems that this PR appropriately leaves unresolved.

Priority: P1
Reviewed head: f04602e664bb8f5e73663a87b3b4a674fab8d27c

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with appropriate negative coverage, documented boundaries, and no blocking finding.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its body reports relevant signed native before/after screen+AX validation through a daemon and public client; source inspection found no unresolved change to principal authorization requiring additional authority proof.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its body reports relevant signed native before/after screen+AX validation through a daemon and public client; source inspection found no unresolved change to principal authorization requiring additional authority proof.
Evidence reviewed 9 items Current main still contains the rejection: The main-branch validator rejects elements.metadata.windowContext.applicationProcessId whenever the screen target has no application. The production Accessibility detector supplies that PID and its process generation.
Released behavior remains affected: The v4.3.4 source contains the same unconditional rejection of the Accessibility context PID.
Independent reporter isolation: The human follow-up isolates screen Accessibility detection: daemon screen capture succeeds with --no-elements and fails with elements, while caller-local capture succeeds. It separately confirms the frontmost and premature-publication problems. #710 (comment).
Findings None None.
Security None None.

How this fits together

Peekaboo's Bridge validates desktop observations returned by a macOS host to a CLI client. Screen observations combine display pixels with Accessibility metadata, which must retain its own application identity without granting window-specific authority to the screenshot.

flowchart TD
 A[CLI screen observation] --> B[Mac host capture]
 B --> C[Display pixels]
 B --> D[Accessibility metadata]
 C --> E[Screen target validation]
 D --> F[Semantic owner validation]
 E --> G[Verified observation]
 F --> G
 G --> H[Client and snapshot consumers]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +54/-4; tests +135/-0 Production growth is confined to semantic-owner validation and supported by focused acceptance and rejection coverage.

Technical review

Best possible solution:

Keep screen pixels display-scoped while accepting independently validated Accessibility ownership through the existing canonical identity rules.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes the failure: host-routed screen capture with Accessibility detection returns an application PID that current main rejects; --no-elements avoids that evidence. This review did not execute native capture.

Is this the best way to solve the issue?

Yes. The patch repairs the screen-only validator using the existing identity coalescer, preserves exact-window safeguards, and avoids changing capture engines or routing policy.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 3b6ea75b0002.

Labels

Label changes:

  • add P1: Independent reports on v4.3.0 and v4.3.4 establish a broken default host-routed screen observation workflow.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its body reports relevant signed native before/after screen+AX validation through a daemon and public client; source inspection found no unresolved change to principal authorization requiring additional authority proof.

Label justifications:

  • P1: Independent reports on v4.3.0 and v4.3.4 establish a broken default host-routed screen observation workflow.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its body reports relevant signed native before/after screen+AX validation through a daemon and public client; source inspection found no unresolved change to principal authorization requiring additional authority proof.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: Raw commit e68a46c adds Core/PeekabooCore/Sources/PeekabooBridge/PeekabooBridgeDesktopObservationBinding.swift:131 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: e68a46c227b9; files: Core/PeekabooCore/Sources/PeekabooBridge/PeekabooBridgeDesktopObservationBinding.swift)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix: separate screen capture and accessibility ownership This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant