Repository navigation
fix(channels): send pairing approvals from the approved account - #137268
Merged
Merged
Conversation
The pairing approval contract carries the account a request was selected on, but the LINE, Signal, IRC, and Synology Chat notifiers dropped it and resolved the default account instead. On a multi-account setup the approval went out from the wrong account with the wrong credentials while the gateway still reported the notification as sent. Each of the four notifiers now passes the approved account id to its account resolver. Omitting the account id still resolves the default account exactly as before.
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
openclaw-barnacle
Bot
added
channel: line
Contributor
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 6:20 AM ET / 10:20 UTC. ClawSweeper reviewWhat this changesSeven channel plugins preserve the approved account when sending pairing confirmations, reuse existing delivery functions, and report unsuccessful notifications. Merge readiness✅ Ready for maintainer review This repair remains necessary on current main. The previous iMessage text finding and Synology/Zalo delivery findings are addressed; no remaining actionable patch defect was found. Priority: P2 Review scores
Verification
How this fits togetherPairing lets an operator approve a sender’s access through the CLI or Gateway. After approval, the channel plugin sends an optional confirmation using the selected account and reports notification success or failure separately. flowchart TD
A[Operator approves sender] --> B[Pairing approval owner]
B --> C[Access approval recorded]
B --> D[Selected account and recipient]
D --> E[Channel notification adapter]
E --> F[Existing account sender]
F --> G[Confirmation delivered]
F --> H[Notification failure reported]
Before mergeNone. Agent review detailsSecurityNone. PR surfaceSource -4, Tests +466. Total +462 across 16 files. View PR surface stats
Review metrics
Technical reviewBest possible solution: Keep account selection in each plugin’s existing resolver and sender, preserving confirmation text and reporting delivery failure without undoing access approval. Do we have a high-confidence way to reproduce the issue? Yes: current source clearly drops the selected account before sending, so approving a named account with distinct transport credentials exercises the defect. This worker inspected source and supplied before/after results but did not execute a reproduction. Is this the best way to solve the issue? Yes: forwarding the existing account context and reusing account-aware delivery owners repairs the plugin boundary; changing the core callback or adding another notification API would duplicate behavior already supported. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against b5412c2a40b2. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
…Personal, and iMessage The Zalo, Zalo Personal (zalouser), and iMessage pairing notifiers dropped the approved account id the same way the first four did, so on a multi-account setup the approval went out from the default account. Each notifier now passes the approved account id to its account resolver or sender, and the Zalo and iMessage runtime facades type their params with an optional accountId so the id survives the module boundary. Omitting the account id still resolves the default account exactly as before.
openclaw-barnacle
Bot
added
channel: imessage
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf609fcdf
ℹ️ 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".
| notify: async ({ cfg, id, message }) => { | ||
| const account = resolveAccount(cfg); | ||
| notify: async ({ cfg, id, message, accountId }) => { | ||
| const account = resolveAccount(cfg, accountId); |
Contributor
There was a problem hiding this comment.
When the selected Synology account has no incomingUrl, this notifier returns normally; when the webhook POST fails, sendMessage likewise resolves false rather than throwing (extensions/synology-chat/src/client.ts:138-172). channels.pairing.approve treats either outcome as notification: "sent", so the newly routed named-account path can silently deliver nothing while reporting success. Throw for a missing URL and for a false send result.
AGENTS.md reference: AGENTS.md:L46-L46
Useful? React with 👍 / 👎.
| }) { | ||
| const { resolveZaloAccount } = await import("./accounts.js"); | ||
| const account = resolveZaloAccount({ cfg: params.cfg }); | ||
| const account = resolveZaloAccount({ cfg: params.cfg, accountId: params.accountId }); |
Contributor
There was a problem hiding this comment.
When the selected Zalo account depends on its per-account proxy, resolving the account here but passing only its token discards that proxy; sendMessageZalo applies the account proxy only when given cfg and accountId or an explicit proxy (extensions/zalo/src/send.ts:96-112). It also converts network and API errors into { ok: false }, which this notifier ignores, so the gateway reports notification: "sent" despite delivering nothing. Forward the selected account context and throw when the result is not successful.
AGENTS.md reference: AGENTS.md:L46-L46
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: sufficient
Preserve the approved account across all seven affected plugins. Reuse existing delivery owners for iMessage, Synology Chat, and Zalo so failed notifications remain visible and Zalo retains its account proxy. Keep the existing notification text and completed approval unchanged. Production decreases by five lines. Secretless hosted proof covers all seven original account mismatches, real notifier HTTP effects, 204 focused checks, and exact candidate file hashes. Co-authored-by: MarMar Labs <[email protected]>
Use the shared approval message already sent by the removed notifier. Assert the exact RPC text for both selected and default accounts. Hosted proof reproduced both generic-text regressions on a2014d6, then passed all 15 iMessage owner tests and verified all 16 candidate file hashes after this correction. Co-authored-by: MarMar Labs <[email protected]>
Contributor
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 7, 2026
…claw#137268) * fix(channels): send pairing approvals from the approved account The pairing approval contract carries the account a request was selected on, but the LINE, Signal, IRC, and Synology Chat notifiers dropped it and resolved the default account instead. On a multi-account setup the approval went out from the wrong account with the wrong credentials while the gateway still reported the notification as sent. Each of the four notifiers now passes the approved account id to its account resolver. Omitting the account id still resolves the default account exactly as before. * fix(channels): extend account-scoped pairing approvals to Zalo, Zalo Personal, and iMessage The Zalo, Zalo Personal (zalouser), and iMessage pairing notifiers dropped the approved account id the same way the first four did, so on a multi-account setup the approval went out from the default account. Each notifier now passes the approved account id to its account resolver or sender, and the Zalo and iMessage runtime facades type their params with an optional accountId so the id survives the module boundary. Omitting the account id still resolves the default account exactly as before. * fix(channels): reuse account-aware pairing notification senders Preserve the approved account across all seven affected plugins. Reuse existing delivery owners for iMessage, Synology Chat, and Zalo so failed notifications remain visible and Zalo retains its account proxy. Keep the existing notification text and completed approval unchanged. Production decreases by five lines. Secretless hosted proof covers all seven original account mismatches, real notifier HTTP effects, 204 focused checks, and exact candidate file hashes. Co-authored-by: MarMar Labs <[email protected]> * fix(imessage): preserve pairing approval welcome text Use the shared approval message already sent by the removed notifier. Assert the exact RPC text for both selected and default accounts. Hosted proof reproduced both generic-text regressions on a2014d6, then passed all 15 iMessage owner tests and verified all 16 candidate file hashes after this correction. Co-authored-by: MarMar Labs <[email protected]> --------- Co-authored-by: Peter Steinberger <[email protected]> Co-authored-by: MarMar Labs <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Pairing approvals on a named account could send the notification through the default account. The affected plugins are iMessage, IRC, LINE, Signal, Synology Chat, Zalo, and Zalo Personal. Synology Chat and Zalo could also report successful notification after an unsuccessful send.
Why This Change Was Made
The core approval callback already carries the selected account. Preserve it through each plugin's existing account resolver and sender. iMessage, Synology Chat, and Zalo reuse their existing outbound delivery functions, removing duplicate notification paths. This also preserves Zalo's account proxy and reports Synology/Zalo delivery failures through the existing notification-failure outcome.
The delivered notification text is unchanged. Pairing decisions and stored approvals are unchanged. Production is +42/−46, net −4 lines; tests are +467/−1.
User Impact
Approval notifications come from the approved account. An unsuccessful notification can be reported as failed while the completed approval remains valid.
Evidence
Corrected head: 2a54928c184. Fresh reviews, required CI and native landing checks passed. Merged as b75423c2fcc; all sixteen landed files match the reviewed source.
Original contribution by MarMar Labs (@marmar9615-cloud).