Skip to content

fix(imessage): retry watch.subscribe startup failures#65482

Merged
vincentkoc merged 2 commits intomainfrom
fix/65393-imessage-watch-subscribe
Apr 12, 2026
Merged

fix(imessage): retry watch.subscribe startup failures#65482
vincentkoc merged 2 commits intomainfrom
fix/65393-imessage-watch-subscribe

Conversation

@vincentkoc
Copy link
Copy Markdown
Member

Summary

  • Problem: a transient iMessage watch.subscribe startup timeout or early RPC close immediately fails the monitor and bounces the channel.
  • Why it matters: issue Bonjour advertiser failure cascades into iMessage channel teardown #65393 showed these failures next to bonjour churn, but the source-backed fragility is the iMessage startup path treating subscribe-time transport stalls as fatal on the first hit.
  • What changed: the iMessage monitor now retries bounded watch.subscribe startup failures locally before surfacing a fatal monitor error, and a focused regression test locks that behavior in.
  • What did NOT change (scope boundary): this does not change bonjour/mDNS behavior, channel restart policy, or post-subscription runtime handling.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: monitorIMessageProvider treated subscribe-time RPC startup failures as fatal on the first attempt, so a transient watch.subscribe timeout or early RPC close tore down the monitor immediately.
  • Missing detection / guardrail: there was no test covering bounded startup retry behavior for watch.subscribe.
  • Contributing context (if known): the issue report correlated these failures with bonjour advertiser churn, but the code-backed failure path is inside iMessage monitor startup, not direct bonjour teardown logic.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: extensions/imessage/src/monitor.watch-subscribe-retry.test.ts
  • Scenario the test should lock in: transient watch.subscribe startup timeout retries locally and succeeds; repeated startup timeouts still fail after the bounded retry budget.
  • Why this is the smallest reliable guardrail: the regression lives entirely in the monitor startup seam, so mocking the RPC client and transport-ready gate gives deterministic coverage without booting live iMessage tooling.
  • Existing test that already covers this (if any): None.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

  • iMessage channels now tolerate brief watch.subscribe startup stalls instead of immediately tearing down on the first timeout.

Diagram (if applicable)

Before:
[start iMessage monitor] -> [watch.subscribe timeout] -> [monitor failed] -> [channel restart]

After:
[start iMessage monitor] -> [watch.subscribe timeout] -> [bounded local retry] -> [subscribe succeeds or final failure]

Security Impact (required)

  • New permissions/capabilities? (No)
  • Secrets/tokens handling changed? (No)
  • New/changed network calls? (No)
  • Command/tool execution surface changed? (No)
  • Data access scope changed? (No)
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS: macOS
  • Runtime/container: Node 22 / pnpm
  • Model/provider: N/A
  • Integration/channel (if any): iMessage
  • Relevant config (redacted): default local dev config

Steps

  1. Start the iMessage monitor.
  2. Make the first watch.subscribe request fail with imsg rpc timeout (watch.subscribe).
  3. Observe monitor behavior before and after this patch.

Expected

  • The monitor retries bounded startup failures locally and only tears down after the retry budget is exhausted.

Actual

  • Before this patch the first subscribe-time startup failure immediately failed the monitor.

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

  • Verified scenarios: OPENCLAW_LOCAL_CHECK=0 pnpm test:serial extensions/imessage/src/monitor.watch-subscribe-retry.test.ts; OPENCLAW_LOCAL_CHECK=0 pnpm build
  • Edge cases checked: retries are limited to subscribe-time transport failures; abort during retry backoff exits cleanly without spinning a new client.
  • What you did not verify: live iMessage/bonjour reproduction on a real machine pair.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes)
  • Config/env changes? (No)
  • Migration needed? (No)
  • If yes, exact upgrade steps:

Risks and Mitigations

  • Risk: retrying the wrong class of startup errors could mask a real configuration problem.
    • Mitigation: retries are limited to subscribe-time transport/timeout errors and still fail after a small bounded budget.

@vincentkoc vincentkoc self-assigned this Apr 12, 2026
@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot Bot commented Apr 12, 2026

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

# Severity Title
1 🟡 Medium Terminal/log injection and potential sensitive data exposure via unsanitized startup error logging in iMessage monitor
1. 🟡 Terminal/log injection and potential sensitive data exposure via unsanitized startup error logging in iMessage monitor
Property Value
Severity Medium
CWE CWE-117
Location extensions/imessage/src/monitor/monitor-provider.ts:579-585

Description

In monitorIMessageProvider, errors from watch.subscribe startup are interpolated directly into log strings using String(err) and then passed to warn(...) / danger(...).

  • err can be derived from RPC responses (parsed.error.data) in extensions/imessage/src/client.ts, where non-string data is formatted with JSON.stringify(details, null, 2) (multi-line, attacker-controlled content from the RPC process)
  • The resulting string may contain newlines and other control characters, enabling log forging (extra lines), and if ANSI/control sequences appear (or are introduced by danger/warn formatting), terminal escape injection
  • The error text may also include sensitive local details contained in RPC error payloads, which would then be emitted to logs unredacted

This is inconsistent with the newly introduced sanitizeIMessageWatchErrorPayload(...) path for watch error notifications, which sanitizes and truncates untrusted error messages.

Vulnerable code:

runtime.error?.(danger(`imessage: monitor failed: ${String(err)}`));

runtime.log?.(
  warn(
    `imessage: watch.subscribe startup failed (attempt ${attempt}/${WATCH_SUBSCRIBE_MAX_ATTEMPTS}): ${String(err)}; retrying`,
  ),
);

Recommendation

Sanitize and bound error text before logging.

  • Use sanitizeTerminalText(...) and truncateUtf16Safe(...) on String(err) (or better: a safe error formatter that does not include multi-line JSON by default)
  • Consider logging structured fields (e.g., name, code) separately rather than raw error text

Example fix:

import { sanitizeTerminalText, truncateUtf16Safe } from "openclaw/plugin-sdk/text-runtime";

const MAX_ERR_CHARS = 300;
const safeErr = (() => {
  const s = sanitizeTerminalText(String(err));
  return s.length > MAX_ERR_CHARS ? `${truncateUtf16Safe(s, MAX_ERR_CHARS - 1)}…` : s;
})();

runtime.error?.(danger(`imessage: monitor failed: ${safeErr}`));
runtime.log?.(warn(`imessage: watch.subscribe startup failed ...: ${safeErr}; retrying`));

If RPC error data may contain sensitive values, consider redacting known keys or only logging error code plus a generic message at non-verbose log levels.


Analyzed PR: #65482 at commit 2408d10

Last updated on: 2026-04-12T18:43:58Z

@openclaw-barnacle openclaw-barnacle Bot added channel: imessage Channel integration: imessage size: M maintainer Maintainer-authored PR labels Apr 12, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: defb51239b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread CHANGELOG.md Outdated
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 12, 2026

Greptile Summary

This PR adds a bounded 3-attempt retry loop to the watch.subscribe startup path in monitorIMessageProvider, with 1 s abort-aware backoff between retries. A focused regression test (monitor.watch-subscribe-retry.test.ts) locks in both the success-on-retry and exhausted-budget paths.

  • The CHANGELOG fix entry (line 213) was appended inside the already-released ## 2026.4.10 block instead of ## Unreleased > ### Fixes, misattributing the fix to a shipped release.

Confidence Score: 5/5

Safe to merge; implementation is correct and tests are solid. One P2 CHANGELOG placement issue to fix.

All code logic is sound — retry loop, abort handling, and client cleanup are correctly implemented. The only finding is a P2 doc placement issue (CHANGELOG entry in the wrong released section), which does not affect runtime behavior.

CHANGELOG.md — entry needs to move from ## 2026.4.10 to ## Unreleased > ### Fixes.

Prompt To Fix All With AI
This is a comment left during a code review.
Path: CHANGELOG.md
Line: 213

Comment:
**CHANGELOG entry placed in released version block**

This fix entry landed at line 213, which is inside the `## 2026.4.10` block (lines 76–214), not in `## Unreleased`. Per `CLAUDE.md`: *"Changelog placement: in the active version block, append new entries to the end of the target section"* — the active block for in-flight work is `## Unreleased > ### Fixes`. The entry should be moved there to avoid misattributing this fix to an already-shipped release.

**Context Used:** CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=fd949e91-5c3a-4ab5-90a1-cbe184fd6ce8))

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(imessage): retry watch.subscribe sta..." | Re-trigger Greptile

Comment thread CHANGELOG.md Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 10d15644a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread extensions/imessage/src/monitor/monitor-provider.ts
@vincentkoc vincentkoc force-pushed the fix/65393-imessage-watch-subscribe branch from 1b660f7 to 2408d10 Compare April 12, 2026 18:38
@vincentkoc vincentkoc merged commit 35a784c into main Apr 12, 2026
36 of 40 checks passed
@vincentkoc vincentkoc deleted the fix/65393-imessage-watch-subscribe branch April 12, 2026 18:40
zhonghe0615 pushed a commit to zhonghe0615/openclaw that referenced this pull request Apr 27, 2026
* fix(imessage): retry watch.subscribe startup failures

* fix(imessage): sanitize watch error logging
lovewanwan pushed a commit to lovewanwan/openclaw that referenced this pull request Apr 28, 2026
* fix(imessage): retry watch.subscribe startup failures

* fix(imessage): sanitize watch error logging
ogt-redknie pushed a commit to ogt-redknie/OPENX that referenced this pull request May 2, 2026
* fix(imessage): retry watch.subscribe startup failures

* fix(imessage): sanitize watch error logging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: imessage Channel integration: imessage maintainer Maintainer-authored PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bonjour advertiser failure cascades into iMessage channel teardown

1 participant