Skip to content

fix(capture): avoid false-success screen captures#185

Merged
steipete merged 6 commits into
openclaw:mainfrom
VishalJ99:codex/peekaboo-screen-permission
Jun 12, 2026
Merged

fix(capture): avoid false-success screen captures#185
steipete merged 6 commits into
openclaw:mainfrom
VishalJ99:codex/peekaboo-screen-permission

Conversation

@VishalJ99

@VishalJ99 VishalJ99 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes legacy/CoreGraphics screen and area capture falsely reporting success with wallpaper-only or redacted pixels in non-Aqua/background sessions.

This PR now:

  • applies the Screen Recording permission gate to legacy capture
  • uses /usr/sbin/screencapture -D <display> for whole displays and -R for areas
  • maps display IDs to screencapture display numbers, excluding subordinate mirrored displays
  • removes unsafe CGDisplayCreateImage fallbacks for screen and area capture
  • preserves native screencapture errors and fails closed
  • adds peekaboo permissions request-screen-recording for the current local CLI binary
  • documents Bridge/local permission ownership and Homebrew TCC-path staleness
  • adds regression coverage and changelog entries

Root cause

Legacy capture skipped the normal Screen Recording gate and could fall back to CoreGraphics APIs that returned plausible image files containing only wallpaper or redacted pixels. Automation received a successful result even though the image did not represent the visible desktop.

The repaired path checks permission first, uses the native capture utility, and returns a permission/native capture error instead of accepting unsafe fallback pixels.

Validation

Final head: b0e9f76e6c8f8dd15495e5cfb8b89ae98bc3dfac

git diff --check origin/main...HEAD
pnpm run format
pnpm run lint
pnpm run test:safe
swift test --package-path Apps/CLI --filter CommanderBinderTests
PEEKABOO_INCLUDE_AUTOMATION_TESTS=true swift test --package-path Apps/CLI --filter PermissionsCommandTests
swift test --package-path Core/PeekabooCore --filter ScreenCapturePlannerMatchDisplayTests

Results:

  • safe suite: 491 tests in 68 suites passed
  • Commander binder suite: 28 tests passed
  • permission command suite: 4 tests passed
  • display planner suite: 24 tests passed
  • SwiftFormat: clean
  • SwiftLint: exit 0; four unrelated existing warnings, none in touched files
  • autoreview: clean; no actionable findings, patch correct at 0.86 confidence

Live proof

Active Aqua session:

  • full display capture succeeded at 3600x2338 and was visually inspected as the real multi-window desktop
  • area capture succeeded at 600x240 and was visually inspected as the real menu bar region

Background launchd session over localhost SSH:

  • full display capture exited 1 with PERMISSION_ERROR_SCREEN_RECORDING
  • area capture exited 1 with PERMISSION_ERROR_SCREEN_RECORDING
  • neither command created an image file

Baseline comparison:

  • shipped Peekaboo 3.4.1 in the same Background session exited 0 and created a wallpaper-only 1800x1169 image
  • direct /usr/sbin/screencapture in that Background session exited 1 with could not create image from display

The final permission-request command is pinned to the local in-process runtime, so it cannot silently request permission on a daemon or Bridge host.

Review follow-up

Maintainer review additionally:

  • replaced inferred whole-display -R geometry with documented -D selection
  • removed the remaining unsafe area fallback
  • preserved native stderr in errors
  • corrected display numbering for mirrored-display arrangements
  • added the local-runtime regression test

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 11, 2026, 10:38 AM ET / 14:38 UTC.

Summary
The PR makes legacy capture validate Screen Recording permission, replaces full-screen CoreGraphics capture with fail-closed system screencapture regions, centralizes system image capture, and adds a local permission-request command, tests, and documentation.

Reproducibility: yes. at source level: current main skips permission probing for legacy-first capture and accepts CoreGraphics output without detecting wallpaper-only pixels. This non-macOS review environment cannot independently execute the live reproduction.

Review metrics: 1 noteworthy metric.

  • Changed surface: 13 files, +288/-64. The compatibility-sensitive runtime change also adds a CLI operation, targeted tests, and four documentation updates.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Post redacted final-head evidence for both the unsafe background path and a successful supported capture path.
  • Include the exact command, runtime source, exit result, and visible image or diagnostic output.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The investigation includes credible live observations, but the successful patched capture predates the final head and the fresh final-head attempt was invalidated by a locked console and black frame; add redacted final-head terminal output or a recording, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P2] Existing full-screen legacy users can receive a new hard capture error whenever /usr/sbin/screencapture -R fails, because the branch intentionally removes the CoreGraphics fallback.

Maintainer options:

  1. Add final-head runtime evidence (recommended)
    Post redacted terminal output or a recording showing both the background-session failure and a successful supported Aqua or Bridge capture on edb73a3a.
  2. Accept the fallback removal
    Maintainers may intentionally own the new hard-failure mode after confirming the documented Bridge or Aqua recovery path is sufficient.

Next step before merge

  • [P1] The contributor must add final-head real behavior proof, after which capture-area owners should confirm the intentional fail-closed compatibility change.

Security
Cleared: No concrete security or supply-chain concern was found; the patch uses a fixed Apple system binary and does not change dependencies, workflows, secrets, or package resolution.

Review details

Best possible solution:

Keep the fail-safe behavior, but merge only after final-head evidence demonstrates the affected background-session error and a supported Aqua or Bridge success path, with capture owners accepting the fallback removal.

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

Yes at source level: current main skips permission probing for legacy-first capture and accepts CoreGraphics output without detecting wallpaper-only pixels. This non-macOS review environment cannot independently execute the live reproduction.

Is this the best way to solve the issue?

Yes in principle: failing rather than returning misleading pixels is the narrowest safe behavior, and the coordinate conversion has focused layout coverage. Merge still needs final-head runtime proof and explicit compatibility acceptance.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 7c3862b03285.

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The investigation includes credible live observations, but the successful patched capture predates the final head and the fresh final-head attempt was invalidated by a locked console and black frame; add redacted final-head terminal output or a recording, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.
  • remove rating: 🌊 off-meta tidepool: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P1: False-success screenshots can cause active visual automation to inspect or target desktop state that was not actually captured.
  • merge-risk: 🚨 compatibility: The branch removes the full-screen CoreGraphics fallback and can convert previously completing legacy captures into errors.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The investigation includes credible live observations, but the successful patched capture predates the final head and the fresh final-head attempt was invalidated by a locked console and black frame; add redacted final-head terminal output or a recording, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • Peter Steinberger: Introduced the current capture orchestration and legacy-operator split and has the strongest recent history across the affected capture and permission UX paths. (role: recent capture-area contributor; confidence: high; commits: 922d31df9eff, 3a1cf0f32f9f, 8cf226c2def5; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/ScreenCaptureService+Operations.swift, Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/LegacyScreenCaptureOperator+ScreenArea.swift, docs/permissions.md)
  • Roman: Added and simplified the ScreenCaptureKit fallback used when CoreGraphics permission preflight is unreliable for CLI tools. (role: permission-check contributor; confidence: medium; commits: 554ccf444d3b, 1128b5684ccd; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/System/PermissionsService.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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 added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 11, 2026
@VishalJ99

Copy link
Copy Markdown
Contributor Author

Addressed the P1 fallback concern in 3c518b5d: full-screen legacy capture now fails closed if /usr/sbin/screencapture -R fails, instead of falling back to CGDisplayCreateImage and risking another wallpaper-only false success. Re-ran git diff --check, swift build --package-path Apps/CLI, and PEEKABOO_INCLUDE_AUTOMATION_TESTS=true swift test --package-path Apps/CLI --filter PermissionsCommandTests successfully. PR body updated with the validation and remaining proof limitations.

@VishalJ99

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Follow-up pushed in 3c518b5d: full-screen legacy capture now fails closed if /usr/sbin/screencapture -R fails, instead of falling back to CGDisplayCreateImage. PR body has been updated with the new validation results and remaining proof limitations.

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@VishalJ99

VishalJ99 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed the P1 coordinate finding in edb73a3a: full-screen legacy capture now converts the selected NSScreen.frame from AppKit lower-left coordinates into the top-left virtual desktop coordinate space expected by /usr/sbin/screencapture -R. Added focused planner coverage for primary, right, left, above, below, and mixed left+above display layouts.

I also ran the requested independent subagent review before pushing this update. It flagged my first local version as wrong because it only translated Y; I corrected that to desktopBounds.maxY - appKitRect.maxY before committing.

Validation on current head edb73a3a:

git diff --check
swift build --package-path Apps/CLI
PEEKABOO_INCLUDE_AUTOMATION_TESTS=true swift test --package-path Apps/CLI --filter PermissionsCommandTests
swift test --package-path Core/PeekabooCore --filter ScreenCapturePlannerMatchDisplayTests

Remaining known limitation: fresh inspectable GUI proof is still not included because the console was locked during the fresh attempt; the PR body calls that out explicitly.

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 11, 2026
@VishalJ99

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Retrying because the prior re-review run for edb73a3a reported an internal review failure / off-meta verdict rather than a code finding. No code changed since the last request.

Please re-check the current head edb73a3a, especially the coordinate fix and planner tests added for screencapture -R display regions.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 11, 2026
@steipete

Copy link
Copy Markdown
Collaborator

@clawsweeper re-review

Maintainer repair and live verification complete on exact head b0e9f76e6c8f8dd15495e5cfb8b89ae98bc3dfac.

Live behavior proof:

  • Active Aqua session: full display capture succeeded at 3600x2338; area capture succeeded at 600x240. Both images were visually inspected and contained the real desktop/menu bar.
  • Background launchd session over localhost SSH: full display and area capture both exited 1 with PERMISSION_ERROR_SCREEN_RECORDING; neither created an image.
  • Baseline Peekaboo 3.4.1 in the same Background session exited 0 and created a visually inspected wallpaper-only 1800x1169 image.
  • Direct /usr/sbin/screencapture in that Background session exited 1 with could not create image from display.

Final validation:

  • pnpm run test:safe: 491 tests in 68 suites passed
  • swift test --package-path Apps/CLI --filter CommanderBinderTests: 28 passed
  • PEEKABOO_INCLUDE_AUTOMATION_TESTS=true swift test --package-path Apps/CLI --filter PermissionsCommandTests: 4 passed
  • swift test --package-path Core/PeekabooCore --filter ScreenCapturePlannerMatchDisplayTests: 24 passed
  • pnpm run format: clean
  • pnpm run lint: exit 0; four unrelated existing warnings, none in touched files
  • branch autoreview: clean, no actionable findings

Maintainer fixes also replaced inferred display regions with documented screencapture -D, removed the unsafe area fallback, preserved native stderr, handled mirrored-display numbering, and forced request-screen-recording onto the local in-process runtime.

@clawsweeper

clawsweeper Bot commented Jun 12, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete steipete merged commit b873daf into openclaw:main Jun 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants