Skip to content

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

Merged
steipete merged 6 commits into
openclaw:mainfrom
SebTardif:fix/osascript-wait-timeout
Aug 2, 2026
Merged

fix: bound osascript wait with the shared process timeout#197
steipete merged 6 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 August 2, 2026, 1:08 AM ET / 05:08 UTC.

ClawSweeper review

What this changes

The PR extracts the existing attachment-conversion process timeout into a shared helper and applies bounded waits to AppleScript sends, reactions, and Messages launcher helper processes.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

This PR fixes current-main unbounded waits in AppleScript send and reaction paths, and its latest commit corrects the earlier 60-second regression by using the existing 150-second send-style deadline. No discrete patch defect remains; maintainer acceptance is needed because direct CLI fallback operations that previously waited indefinitely will now fail after 150 seconds.

Priority: P1
Reviewed head: 86f6d19d77a5d2c6e28602b52309b93b1f856016
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) This is a focused, well-proven reliability repair with no line-level correctness finding; acceptance depends on the maintainer’s compatibility decision.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body supplies after-fix macOS terminal output for a real /usr/bin/osascript process in the same argument and stdin shape as production, showing timeout and reaping; redact environment details in any additional proof.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body supplies after-fix macOS terminal output for a real /usr/bin/osascript process in the same argument and stdin shape as production, showing timeout and reaping; redact environment details in any additional proof.
Evidence reviewed 7 items Current-main send fallback is unbounded: The MessageSender fallback starts /usr/bin/osascript, closes stdin, then calls process.waitUntilExit() without a deadline.
Current-main reaction path is unbounded: The reaction command has the same direct waitUntilExit() after launching its production AppleScript process.
Existing timeout contract: The bridge defines 150 seconds for send-style actions because Messages can legitimately stall longer than short probe windows; the PR’s latest commit uses this constant for both AppleScript callers.
Findings None None.
Security None None.

How this fits together

The iMessage CLI starts AppleScript and small system helper processes to send messages, add reactions, convert attachments, and prepare Messages.app. Their exit status feeds back into CLI or bridge responses, so an indefinitely hung child can otherwise block the caller.

flowchart LR
  Request[CLI or bridge request] --> Operation[Send, react, convert, or launch]
  Operation --> Child[AppleScript or helper process]
  Child --> BoundedWait[Deadline and termination]
  BoundedWait --> Outcome[Exit status or timeout error]
  Outcome --> Response[CLI or bridge response]
Loading

Decision needed

Question Recommendation
Should direct AppleScript fallback send and reaction operations adopt the existing 150-second send-style deadline instead of retaining their previous unlimited wait? Accept the 150-second boundary: Merge with the existing bridge deadline so hung AppleScript no longer blocks send or reaction work indefinitely.

Why: The patch is mechanically sound and matches the bridge’s documented send-action timeout, but only maintainers can accept the user-visible change from an indefinite stall to a bounded failure for direct CLI fallback callers.

Before merge

  • Resolve merge risk (P2) - A direct send-fallback or reaction operation that previously waited indefinitely will now return an AppleScript failure after 150 seconds. That aligns with the existing bridge contract, but it is still a user-visible compatibility boundary for callers that rely on an unlimited wait.
  • Complete next step (P2) - The implementation and proof are sufficient, but a maintainer must explicitly accept the direct-CLI compatibility boundary before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 6 files affected; 184 added, 42 removed The patch centralizes one timeout implementation and extends it across send, reaction, conversion, and launcher subprocess paths.
Real process proof 2 live macOS subprocess scenarios The PR body reports a hung real osascript reaped under a short bound and a successful real csrutil status invocation.

Merge-risk options

Maintainer options:

  1. Accept the existing send deadline (recommended)
    Approve the 150-second limit for direct AppleScript fallback operations because it matches the bridge’s send-action contract and prevents indefinite stalls.
  2. Retain unbounded direct waits
    Pause this PR if direct CLI fallback operations must remain unlimited despite the existing bridge deadline.

Technical review

Best possible solution:

Merge the shared bounded-wait helper with the 150-second send/reaction deadline retained, preserving the existing conversion cleanup semantics and surfacing a clear timeout error instead of allowing a caller to stall forever.

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

Yes. Current main directly calls unbounded waitUntilExit() after starting the production send-fallback and reaction AppleScript processes; the PR body also shows a real macOS /usr/bin/osascript delay 30 process being reaped after timeout.

Is this the best way to solve the issue?

Yes. Reusing the established attachment-conversion deadline and termination pattern is the narrowest maintainable repair, and the latest revision correctly retains the existing 150-second send-style timeout for AppleScript callers.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P1: A hung AppleScript child can indefinitely block a real user-facing send or reaction workflow.
  • merge-risk: 🚨 compatibility: The patch intentionally changes direct fallback behavior from an unlimited wait to a 150-second failure.
  • 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 body supplies after-fix macOS terminal output for a real /usr/bin/osascript process in the same argument and stdin shape as production, showing timeout and reaping; redact environment details in any additional proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies after-fix macOS terminal output for a real /usr/bin/osascript process in the same argument and stdin shape as production, showing timeout and reaping; redact environment details in any additional proof.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: Current-main blame attributes the existing AppleScript waits, launcher helpers, and bridge timeout contract baseline to Peter Steinberger; the current main head also remains in this area’s recent history. (role: current behavior author and recent area contributor; confidence: high; commits: 1d8b679cc3a3, e22dfad8e54e; files: Sources/IMsgCore/MessageSender.swift, Sources/imsg/Commands/ReactCommand.swift, Sources/IMsgCore/MessagesLauncher.swift)
  • SebTardif: Authored the earlier merged attachment-conversion timeout implementation that this PR extracts and reuses, and authored the latest correction preserving the send-style deadline. (role: introduced shared timeout precedent; confidence: high; commits: 17e36970a58d, 86f6d19d77a5; files: Sources/IMsgCore/AttachmentResolver.swift, Sources/IMsgCore/ProcessTimeout.swift, Tests/IMsgCoreTests/ProcessTimeoutTests.swift)

Rank-up moves

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

  • Confirm the 150-second direct-CLI timeout boundary is intended, then merge without changing the established deadline.

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 (30 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-31T12:15:04.674Z sha 9d9fc3a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T12:29:45.241Z sha 9d9fc3a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T16:00:47.393Z sha 9d9fc3a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T23:13:09.827Z sha 9d9fc3a :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T05:08:59.889Z sha 9d9fc3a :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T11:55:21.304Z sha 9d9fc3a :: needs changes before merge. :: [P1] Align AppleScript waits with the send timeout contract | [P1] Align reaction waits with the send timeout contract
  • reviewed 2026-08-02T02:41:25.493Z sha 9d9fc3a :: needs changes before merge. :: [P1] Preserve the fallback send deadline | [P1] Preserve the reaction deadline
  • reviewed 2026-08-02T03:00:15.102Z sha 86f6d19 :: 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 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 rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. 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. and removed 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. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 1, 2026
Fallback send and reaction automation must match IMsgBridgeProtocol
defaultSendResponseTimeout rather than the 60s helper default.

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

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@SebTardif

Copy link
Copy Markdown
Contributor Author

Addressed the two P1 timeout findings: osascript waits in MessageSender fallback send and ReactCommand now use IMsgBridgeProtocol.defaultSendResponseTimeout (150s) instead of ProcessTimeout.defaultTimeout (60s).

@clawsweeper

clawsweeper Bot commented Aug 2, 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 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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 2, 2026
@steipete

steipete commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Independent maintainer verification on exact head 86f6d19 is clean; I recommend landing this PR as-is.

Proof on macOS arm64:

  • Focused timeout suite: all 4 tests passed, including real /usr/bin/osascript and csrutil status subprocesses.
  • make lint: passed with the repository’s pre-existing warnings only.
  • make test: all 489 tests passed.
  • make build ARCHES="$(uname -m)": release CLI and universal helper built successfully.
  • Live production-source integration: compiled ProcessTimeout.swift with a small driver, launched /usr/bin/osascript -e 'delay 10', and bounded it at 0.2s. Result: timed_out=true, running=false, elapsed 0.324s.
  • Live release CLI: ./bin/imsg --version reported 0.13.5 and ./bin/imsg send --help rendered normally.
  • Full-branch autoreview: clean, no actionable findings (0.98 correctness confidence).

GitHub CI is green on macOS and Linux, and the PR is currently cleanly mergeable.

Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 2, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix: bound osascript wait with the shared process timeout 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.

@steipete
steipete merged commit 02644cb into openclaw:main Aug 2, 2026
4 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. 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.

2 participants