Skip to content

fix(window): prevent hosts crashing when closing their own windows - #730

Merged
steipete merged 6 commits into
mainfrom
codex/fix-host-window-close-upstream-20260913
Sep 13, 2026
Merged

steipete merged 6 commits into
mainfrom
codex/fix-host-window-close-upstream-20260913

Conversation

@steipete

@steipete steipete commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Fixes embedded macOS hosts crashing when background window automation closes one of their own windows. Same-process Accessibility calls synchronously invoke AppKit; running them from a detached task can trap an actor-isolated window delegate.

Host-owned AX lookup, close, restore, geometry, and close verification now run on MainActor. Other apps retain detached messaging, per-element deadlines, and exact-window/process-generation validation.

A dedicated AppKit child exercises the public close service, releases the target as a real window owner does, and checks main-actor delegate delivery, confirmed background completion, WindowServer disappearance, and sibling survival. The parent requires a successful exit plus an explicit completion marker; CI also requires suite and test-run completion, preventing early process exits from appearing green.

Direct macOS 27 proof: the same Developer ID-signed fixture ran under a sandbox protecting user configuration, preferences, Keychain services, and network access while participating in the normal Peekaboo coordination locks. Original dependency 44eff916 exited with SIGTRAP and “Incorrect actor executor assumption”; the fixed source exited 0 with HOST_WINDOW_CLOSE_FIXTURE_COMPLETED. The fixture source was byte-identical in both builds.

Production and test builds, SwiftLint, SwiftFormat, docs checks, and independent P0–P2 review pass. Final-head CI passed: macOS, full validation, and CodeQL. The full-safe lane completed 1,977 tests across 234 suites; the guarded native lane completed all 51 selected tests. OpenClaw integration is openclaw/openclaw#147114, pinned to the minimal runtime backport 13ffdea67297ba28adc53c2f83c394059322155e. The two modified production files match this repair exactly, and the backport source branch is retained while OpenClaw depends on it.

The broader suite exposed stale guidance assertions from the already-merged skill update #729. The test-only repair recognizes the selected "$PB" CLI alias, preserves exact-target and read-only requirements, and removes the obsolete requirement to list a clipboard example. All 12 guidance tests pass.

@clawsweeper

clawsweeper Bot commented Sep 13, 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.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 13, 2026
@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 13, 2026, 12:19 PM ET / 16:19 UTC (Revision 6).

ClawSweeper review

What this changes

Runs host-owned window close, restore, maximize, and verification on MainActor, with a native regression fixture, CI completion checks, documentation, and updated CLI guidance tests.

Merge readiness

Ready for maintainer review

This remains a useful, focused fix: current main and v4.3.4 retain the crashing execution path. No actionable introduced defect was found, and the supplied native before/after evidence supports the repair.

Priority: P2
Reviewed head: b873bbcf804ccd52eb3fcd9f20232bb6e77e4844

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused implementation with meaningful native before/after evidence and guarded regression completion; no blocking findings remain.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The supplied macOS 27 before/after output exercises WindowManagementService through a real AppKit host: the original build traps, while the repaired build reaches the marker after main-thread delegate delivery, confirmed background close, target disappearance, and sibling-survival assertions.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The supplied macOS 27 before/after output exercises WindowManagementService through a real AppKit host: the original build traps, while the repaired build reaches the marker after main-thread delegate delivery, confirmed background close, target disappearance, and sibling-survival assertions.
Evidence reviewed 8 items Repository policy and review scope: Read the complete root AGENTS.md. No nested AGENTS.md files were found under the changed directories, and .agents contains no maintainer-notes directory. Applied shared-service boundaries, regression coverage, and behavior-proof guidance within the read-only review contract.
Executor change preserves target validation: The four raw AX operations now share an executor selector: current-process targets use MainActor.run; other processes retain Task.detached. Existing window identity, process-generation, bounds, child-element deadlines, and completion verification remain intact. The change does not alter authorization, public API, stored configuration, or dependency versions.
Current main still needs the repair: GitHub's main endpoint returned the pinned base SHA. Inspection of that source and the introduced diff confirms that main still dispatches host-owned AX operations through detached tasks.
Findings None None.
Security None None.

How this fits together

Peekaboo’s shared window automation service receives exact-window requests from CLI and embedded hosts, validates the target, and invokes macOS Accessibility. It verifies the resulting window state before reporting completion.

flowchart TD
    A[Exact-window request] --> B[Validate window and process identity]
    B --> C{Owned by this host?}
    C -->|Yes| D[Main-thread Accessibility calls]
    C -->|No| E[Detached calls with message deadlines]
    D --> F[Verify exact window state]
    E --> F
    F --> G[Report action outcome]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta Production source +22/-13; tests and fixture +209/-7 The small runtime increase centralizes executor selection; most added code exercises native host-window closure.

Technical review

Best possible solution:

Keep AppKit callbacks on the host’s main thread while preserving exact-window validation, remote AX deadlines, and native regression coverage.

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

Yes: the native fixture invokes the public background-close service on a host-owned NSWindow with a MainActor-isolated delegate. Current main retains the detached dispatch, and the supplied before/after run reports the expected trap and recovery; this review did not execute it.

Is this the best way to solve the issue?

Yes: selecting the executor at the shared raw AX boundary is a narrow repair that preserves remote execution and existing target checks without adding a competing window-management path.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 6434b7852d16.

Labels

Label justifications:

  • P2: Repairs a concrete crash in embedded hosts closing their own windows, with a bounded affected workflow.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The supplied macOS 27 before/after output exercises WindowManagementService through a real AppKit host: the original build traps, while the repaired build reaches the marker after main-thread delegate delivery, confirmed background close, target disappearance, and sibling-survival assertions.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied macOS 27 before/after output exercises WindowManagementService through a real AppKit host: the original build traps, while the repaired build reaches the marker after main-thread delegate delivery, confirmed background close, target disappearance, and sibling-survival assertions.

Evidence

What I checked:

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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.

History

Review history (5 earlier review cycles)
  • reviewed 2026-09-13T14:40:33.237Z sha e8e5ffa :: blocked before merge. :: none
  • reviewed 2026-09-13T14:57:23.744Z sha f80a539 :: blocked before merge. :: none
  • reviewed 2026-09-13T15:10:20.726Z sha d0a9b50 :: blocked before merge. :: none
  • reviewed 2026-09-13T15:23:24.967Z sha da4defe :: blocked before merge. :: none
  • reviewed 2026-09-13T15:55:42.118Z sha 14075c5 :: needs maintainer review before merge. :: none

@steipete
steipete marked this pull request as ready for review September 13, 2026 15:17
@steipete
steipete requested a review from a team as a code owner September 13, 2026 15:17
@cursor

cursor Bot commented Sep 13, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes threading for window close/restore/maximize when the target owns the host process; behavior for other apps is unchanged but this is core window automation with AppKit/AX interaction.

Overview
Fixes crashes when embedded Peekaboo hosts close their own windows via background AX automation. Same-process close, restore, maximize, geometry, and presence checks now run on MainActor instead of a detached task, because in-process Accessibility calls synchronously hit AppKit and window delegates; remote targets still use detached work with per-element AX deadlines and unchanged exact-window receipts.

Adds an AppKit HostWindowCloseFixture and SameProcessWindowCloseTests (gated by RUN_AUTOMATION_ACTIONS) to exercise closeWindowActionResult end-to-end—main-actor delegate delivery, confirmed background AX close, WindowServer disappearance, and sibling survival. macOS CI enables that env var, extends the contract test filter, and asserts suite/test-run success from piped logs.

Docs and changelog describe the host vs remote routing split; doc tests accept "$PB"-prefixed skill examples.

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

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Sep 13, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient Contributor real behavior proof is sufficient. label Sep 13, 2026
@steipete
steipete merged commit a744b91 into main Sep 13, 2026
17 checks passed
@steipete
steipete deleted the codex/fix-host-window-close-upstream-20260913 branch September 13, 2026 16:58
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. proof: sufficient Contributor real behavior proof is sufficient. 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