Skip to content

fix(capture): honor cancellation during screen-recording permission retry#280

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/screencapture-permission-honor-cancel
Open

fix(capture): honor cancellation during screen-recording permission retry#280
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/screencapture-permission-honor-cancel

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

ScreenRecordingPermissionChecker retries a failed SCShareableContent probe after try? await Task.sleep. Because try? swallows CancellationError, a canceled permission check can still run a second ScreenCaptureKit probe after the delay.

Same cancellation class as merged #270 / #271 / #230 / #267.

Evidence

On current main:

try? await Task.sleep(nanoseconds: delay)
do {
  _ = try await ScreenCaptureKitCaptureGate.currentShareableContent()
  // ...
}

After cancel, sleep returns without error and the second probe runs.

After this change, cancellation is rechecked after sleep and the function returns false without another probe.

Real behavior proof

  • Behavior or issue addressed: Canceled screen-recording permission retries no longer issue a second shareable-content probe.
  • Real environment tested: macOS, Peekaboo at upstream/main + this branch; pnpm run test:safe.
  • Exact steps or command run after this patch:
$ pnpm run lint
Done linting! Found 13 violations, 0 serious in 1187 files.

$ pnpm run test:safe
✔ Test run with 748 tests in 85 suites passed after 16.325 seconds.
  • Evidence after fix: ScreenCapturePermissionGate.swift now guard !Task.isCancelled else { return false } after the transient-denial sleep.
  • Observed result after fix: Safe suite green; canceled permission path cannot start the retry probe.
  • What was not tested: Live TCC denial + cancel under real SCK (host-specific permission state).

Summary

  • One-line control-flow fix after existing sleep.
  • Changelog credit for @SebTardif.
  • Sibling of the ScreenCaptureKit capture-retry cancel fix.

Related: #271, #270, #267, #230

…etry

The permission probe retry slept with try?, so a canceled caller could
still run a second SCShareableContent probe. Recheck cancellation after
sleep and return false without another probe.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@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. P2 Normal priority bug or improvement with limited blast radius. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 7:17 AM ET / 11:17 UTC.

Summary
Adds a post-sleep cancellation guard to the screen-recording permission retry path and adds a changelog entry.

Reproducibility: no. there is not yet a high-confidence real-environment reproduction. Source inspection clearly exposes the swallowed-cancellation path, but the PR does not exercise a live or focused production-helper cancellation during the ScreenCaptureKit retry.

Review metrics: 2 noteworthy metrics.

  • Diff Surface: 2 files, 5 additions, 0 deletions. The behavioral change is tightly scoped, while one of the two files is release-owned metadata rather than implementation.
  • Regression Coverage: 0 test files changed. The cancellation behavior is subtle and the existing safe suite does not directly demonstrate the affected retry path.

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

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

Rank-up moves:

  • Provide redacted terminal output or runtime logs demonstrating that cancellation during retry backoff prevents a second ScreenCaptureKit probe.
  • Remove the CHANGELOG.md edit.
  • [P1] Add focused regression coverage for cancellation during the retry sleep if the production helper can be exercised deterministically.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR provides green lint, tests, and source reasoning but no after-fix live or focused production-helper evidence for a canceled ScreenCaptureKit permission retry; add redacted terminal output or runtime logs, update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Without a focused live or production-helper demonstration, reviewers cannot confirm that cancellation during the transient TCC backoff prevents the second real ScreenCaptureKit probe while preserving the expected non-cancelled retry behavior.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the post-sleep guard, add focused cancellation regression coverage, provide redacted terminal or runtime evidence showing that cancellation suppresses the second probe, and leave CHANGELOG.md to the release process.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining merge gate is contributor-supplied real behavior proof from a macOS ScreenCaptureKit setup, plus removal of the release-owned changelog entry; automation cannot manufacture the required environment evidence.

Security
Cleared: The patch adds only local cancellation control flow and release text, with no dependency, credential, permission, download, or supply-chain changes.

Review findings

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
Review details

Best possible solution:

Keep the post-sleep guard, add focused cancellation regression coverage, provide redacted terminal or runtime evidence showing that cancellation suppresses the second probe, and leave CHANGELOG.md to the release process.

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

No, there is not yet a high-confidence real-environment reproduction. Source inspection clearly exposes the swallowed-cancellation path, but the PR does not exercise a live or focused production-helper cancellation during the ScreenCaptureKit retry.

Is this the best way to solve the issue?

Yes, the immediate post-sleep cancellation check matches established repository patterns and is the narrowest correction. Merge readiness still requires direct behavior proof and removal of the release-owned changelog edit.

Full review comments:

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
    Normal pull requests should leave CHANGELOG.md to the release process and keep release-note context in the PR body or commit message. This line was already present at the previously reviewed head, so this is a late policy finding; remove it before merge.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is correct
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a narrow cancellation correctness fix with limited blast radius and no evidence of data loss, security bypass, or core-runtime unavailability.
  • 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 real behavior proof before merge: The PR provides green lint, tests, and source reasoning but no after-fix live or focused production-helper evidence for a canceled ScreenCaptureKit permission retry; add redacted terminal output or runtime logs, update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • SebTardif: Previously authored several merged cancellation fixes in adjacent polling and window-control paths, making them closely familiar with the established implementation pattern beyond this proposal. (role: recent cancellation-pattern contributor; confidence: high; commits: b6a089828101, ee03f0df87b5, ed1a72186cd3; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/ScreenCapturePermissionGate.swift)
  • romanr: Prior repository history associates this contributor with the screen-recording permission reliability work that introduced or shaped this capture-permission surface. (role: screen-recording permission feature contributor; confidence: medium; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/ScreenCapturePermissionGate.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.
Review history (1 earlier review cycle)
  • reviewed 2026-07-16T13:09:26.973Z sha a1d740f :: needs real behavior proof before merge. :: none

@clawsweeper

clawsweeper Bot commented Jul 18, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(capture): honor cancellation during screen-recording permission retry 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

P2 Normal priority bug or improvement with limited blast radius. 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.

1 participant