Skip to content

fix: bound osascript wait with the shared process timeout - #197

Open
SebTardif wants to merge 4 commits into
openclaw:mainfrom
SebTardif:fix/osascript-wait-timeout
Open

fix: bound osascript wait with the shared process timeout#197
SebTardif wants to merge 4 commits into
openclaw:mainfrom
SebTardif:fix/osascript-wait-timeout

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

MessageSender.runOsascript (NSAppleScript authorization fallback) and ReactCommand.runAppleScript start /usr/bin/osascript and block on unbounded process.waitUntilExit(). A hung Messages automation stalls send/react CLI or RPC work indefinitely. Attachment conversion already fixed the same hang class for ffmpeg in #176; osascript was left unbounded.

Evidence

Patch

  • Shared ProcessTimeout (monotonic deadline, default 60s, SIGTERM then SIGKILL process/group).
  • Applied to MessageSender osascript fallback and ReactCommand osascript.
  • AttachmentResolver conversion reuses the same helper.
  • Linux: conditional Darwin/Glibc imports for read-core CI.
  • No release-owned CHANGELOG edit.

Live osascript route shape (head c0243a6)

Production launch shape: /usr/bin/osascript -l AppleScript - with AppleScript on stdin (same as MessageSender/ReactCommand). Source is delay 30; helper timeout 0.6s:

$ swift test --filter processTimeoutReapsHungOsascript
✔ Test processTimeoutReapsHungOsascript() passed after ~0.7s

PROOF osascript_route timedOut=true isRunning=false elapsed≈0.72s status=15
PROOF MessageSender/ReactCommand path shape: /usr/bin/osascript -l AppleScript - with delay 30 reaped

status=15 is SIGTERM from the shared terminate path. Full suite remains green on macOS; linux-read-core green after Glibc import fix.

Real behavior proof

  • Behavior or issue addressed: Unbounded waitUntilExit on osascript send-fallback and react automation; now deadline-bounded.

  • Real environment tested: macOS arm64, real /usr/bin/osascript, branch fix/osascript-wait-timeout at c0243a6.

  • Exact steps or command run after this patch:

    swift test --filter processTimeoutReapsHungOsascript
    make test
    make lint
  • Evidence after fix: real osascript child running delay 30 reaped in under 1s with timedOut=true; helper unit tests still pass.

  • Observed result after fix: same ProcessTimeout used by MessageSender/ReactCommand terminates hung osascript instead of blocking ~30s.

  • What was not tested: Full Messages.app Accessibility deadlock (requires intentional UI hang); NSAppleScript success path without osascript fallback.

MessagesLauncher helpers

killall Messages and csrutil status also used unbounded waitUntilExit. They now use ProcessTimeout with a 15s helper bound. Live success path:

$ swift test --filter processTimeoutAllowsCsrutilStatus
✔ Test processTimeoutAllowsCsrutilStatus() passed after 0.053 seconds

Test plan

  • swift test --filter processTimeoutReapsHungOsascript (real osascript)
  • swift test --filter processTimeoutKillsHungProcess
  • make test / make lint
  • CI macos + linux-read-core

Extract ProcessTimeout from the ffmpeg conversion wait and apply the
same monotonic deadline to MessageSender and ReactCommand osascript
paths so hung Messages automation cannot block indefinitely.

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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 27, 2026
@clawsweeper

clawsweeper Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 3:48 PM ET / 19:48 UTC.

ClawSweeper review

What this changes

The PR extracts the existing external-process timeout behavior into a shared helper, applies it to message-send and reaction AppleScript calls plus launcher helpers, and adds timeout coverage using real macOS tools.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

This is a focused reliability fix with sufficient real-process proof and no remaining line-level correctness finding. It should stay open for maintainer review because it deliberately changes the compatibility contract: previously unbounded osascript and launcher helper calls now fail after fixed deadlines.

Priority: P2
Reviewed head: 9d9fc3a6f92d747f0d41078faa68a3c2f51705bd
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The patch is focused, has strong real-process proof, and has no concrete correctness finding; the remaining item is explicit maintainer acceptance of the changed timeout contract.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR provides after-fix terminal proof using a real macOS osascript child in the same launch shape as the production callers, plus a live successful launcher-helper check.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR provides after-fix terminal proof using a real macOS osascript child in the same launch shape as the production callers, plus a live successful launcher-helper check.
Evidence reviewed 4 items Shared timeout implementation: The PR adds ProcessTimeout and routes message-send AppleScript, reaction AppleScript, attachment conversion, and Messages launcher subprocess waits through it; timeouts terminate the child and return caller-specific timeout/error behavior.
Existing policy precedent: The related merged change already established a 60-second monotonic timeout and termination policy for attachment conversion; this branch reuses that policy rather than introducing a second timeout mechanism.
Real behavior proof: The PR body reports a real macOS /usr/bin/osascript -l AppleScript - child executing delay 30 and being reaped in about 0.7 seconds under a short test deadline, plus a successful live csrutil status helper run.
Findings None None.
Security None None.

How this fits together

The iMessage CLI and RPC paths invoke macOS subprocesses to send messages, add reactions, convert attachments, and prepare Messages.app. Their exit results feed back into command handling, so a hung subprocess can otherwise stall a user operation indefinitely.

flowchart LR
  Request[CLI or RPC request] --> Command[Send, react, conversion, or launcher]
  Command --> Helper[External macOS helper]
  Helper --> Wait[Shared bounded process wait]
  Wait --> Outcome[Exit status or timeout error]
  Outcome --> Request
Loading

Decision needed

Question Recommendation
Should send, react, and Messages launcher operations adopt fixed 60-second and 15-second subprocess deadlines instead of retaining their former unbounded wait behavior? Accept bounded waits: Merge the shared timeout policy to prevent indefinite CLI and RPC stalls, accepting timeout errors for unusually slow helper runs.

Why: The implementation is mechanically coherent and follows the existing attachment-conversion policy, but applying that policy to established interactive command paths intentionally changes what slow existing workflows observe.

Before merge

  • Resolve merge risk (P2) - A slow but otherwise healthy osascript invocation that previously waited indefinitely will now return an AppleScript timeout after 60 seconds; similarly, launcher helpers can return failure or no result after 15 seconds.
  • Complete next step (P2) - No mechanical repair is indicated; maintainer acceptance of the deliberate timeout compatibility change is the remaining merge action.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 6 files affected; 1 helper added, 4 production paths changed, 1 test file added The branch centralizes one external-process policy across several user-facing command paths.

Merge-risk options

Maintainer options:

  1. Accept the timeout contract (recommended)
    Merge after confirming that 60 seconds for AppleScript and 15 seconds for launcher helpers are acceptable failure bounds for existing users.
  2. Reduce the scope before merge
    Keep the AppleScript reliability fix while splitting launcher-helper policy into a separately reviewed change.

Technical review

Best possible solution:

Adopt the shared bounded-wait helper if maintainers accept the explicit 60-second AppleScript and 15-second launcher-helper contracts, preserving the existing attachment-conversion timeout policy and its caller-specific errors.

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

Yes for the bounded-subprocess behavior: the supplied test launches real macOS osascript with the same invocation shape as the production paths and proves it is reaped under a deadline. The exact Messages.app accessibility deadlock was not intentionally reproduced.

Is this the best way to solve the issue?

Unclear: extracting the already-merged conversion timeout behavior is the narrowest maintainable implementation, but maintainers must decide whether its fixed deadline is the right compatibility contract for send, react, and launcher operations.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: The change addresses indefinite stalls in established macOS command paths without evidence of an active widespread outage.
  • merge-risk: 🚨 compatibility: Fixed deadlines replace prior indefinite waits and can turn slow existing subprocess runs into timeout failures.
  • 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 PR provides after-fix terminal proof using a real macOS osascript child in the same launch shape as the production callers, plus a live successful launcher-helper check.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR provides after-fix terminal proof using a real macOS osascript child in the same launch shape as the production callers, plus a live successful launcher-helper check.

Evidence

What I checked:

  • Shared timeout implementation: The PR adds ProcessTimeout and routes message-send AppleScript, reaction AppleScript, attachment conversion, and Messages launcher subprocess waits through it; timeouts terminate the child and return caller-specific timeout/error behavior. (Sources/IMsgCore/ProcessTimeout.swift:1, 9d9fc3a6f92d)
  • Existing policy precedent: The related merged change already established a 60-second monotonic timeout and termination policy for attachment conversion; this branch reuses that policy rather than introducing a second timeout mechanism. (Sources/IMsgCore/AttachmentResolver.swift:126, 17e36970a58d)
  • Real behavior proof: The PR body reports a real macOS /usr/bin/osascript -l AppleScript - child executing delay 30 and being reaped in about 0.7 seconds under a short test deadline, plus a successful live csrutil status helper run. (Tests/IMsgCoreTests/ProcessTimeoutTests.swift:1, c0243a6cacf4)
  • Review continuity: The immediately preceding review found no patch defect after the Linux portability and release-owned changelog concerns were addressed; it identified acceptance of the new command-level deadline as the remaining maintainer choice. (c0243a6cacf4)

Likely related people:

  • SebTardif: Authored the merged attachment-conversion timeout work and this follow-up shared implementation across the affected subprocess paths. (role: introduced related timeout behavior and recent area contributor; confidence: high; commits: 17e36970a58d, 6cb8ca677e4f, 9d9fc3a6f92d; files: Sources/IMsgCore/AttachmentResolver.swift, Sources/IMsgCore/ProcessTimeout.swift, Sources/IMsgCore/MessageSender.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Confirm the fixed 60-second AppleScript and 15-second launcher-helper bounds are acceptable for current users.

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 (6 earlier review cycles)
  • reviewed 2026-07-27T15:36:58.315Z sha 6cb8ca6 :: needs real behavior proof before merge. :: [P1] Make the new timeout files portable to Linux
  • reviewed 2026-07-27T15:46:12.230Z sha cfc229d :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-27T16:41:45.520Z sha cfc229d :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-07-27T16:46:30.894Z sha cfc229d :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-07-27T18:51:54.922Z sha c0243a6 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-27T18:58:13.702Z sha c0243a6 :: needs maintainer review before merge. :: none

Import Darwin or Glibc conditionally like AttachmentResolver so
linux-read-core does not fail on unconditional import Darwin.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 27, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated Real behavior proof with exact-head live evidence (not mock-only unit assertions only).

@clawsweeper

clawsweeper Bot commented Jul 27, 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: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

Remove Unreleased CHANGELOG entry (release-owned). Add a ProcessTimeout
test that launches real /usr/bin/osascript with delay 30 and reaps it
under a short bound (same launch shape as MessageSender/ReactCommand).

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

  • Removed release-owned CHANGELOG entry
  • Added real /usr/bin/osascript delay-30 timeout proof (same launch shape as MessageSender/ReactCommand) via processTimeoutReapsHungOsascript

@clawsweeper

clawsweeper Bot commented Jul 27, 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: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 27, 2026
Use ProcessTimeout for short helper processes so hung killall/csrutil
cannot stall launcher setup (same policy as osascript/ffmpeg).

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Also bound MessagesLauncher killall/csrutil via ProcessTimeout (15s helper timeout) plus processTimeoutAllowsCsrutilStatus live success proof.

@clawsweeper

clawsweeper Bot commented Jul 27, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

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. 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