Conversation
github-actions
Bot
added
size: M
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue preventing proactive Telegram messages from being sent via WASM-backed channels. The core change involves implementing the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the broadcast method for WasmChannel to enable proactive messaging, specifically for the Telegram channel, adding it to WasmChannel and SharedWasmChannel with regression tests. A security vulnerability of medium severity exists as the new broadcast path lacks authorization checks, potentially allowing unauthorized messaging and bypassing owner_id restrictions, which violates the rule regarding verifying authenticated user IDs for user-owned resources. Furthermore, the broadcast method in the generic WasmChannel contains Telegram-specific logic, which could impact maintainability, and one new test for the success path of broadcast is not fully effective due to using a mock channel without a WASM component. Positively, the fix to cap HTTP request timeouts and the refactoring of callback timeouts are solid improvements, with the implementation correctly handling Telegram-specific metadata and robust HTTP timeout mechanisms.
| async fn broadcast( | ||
| &self, | ||
| user_id: &str, | ||
| response: OutgoingResponse, | ||
| ) -> Result<(), ChannelError> { |
There was a problem hiding this comment.
The broadcast method allows the agent to send proactive messages to any Telegram chat_id provided in the user_id parameter without performing any authorization checks. This implementation bypasses the owner_id restriction and pairing-based access controls that are enforced for incoming messages in the Telegram channel's WASM guest.
If an attacker can influence the user_id passed to the broadcast method (for example, through prompt injection into a tool that calls this method), they could use the bot to send unauthorized messages or spam to arbitrary Telegram users.
Recommendation: Implement a check within the broadcast method to verify that the target user_id is authorized to receive messages from the bot, respecting the channel's configured privacy policy (e.g., checking against the owner_id or the pairing store).
References
- Tools that interact with user-owned resources, such as sandbox jobs, must verify that the authenticated user ID in the tool context matches the resource owner's ID (e.g., by using a
ContextManager) before performing any read or write operations to prevent unauthorized cross-user access.
| // Broadcast routing metadata is channel-specific. For now, support | ||
| // Telegram's expected metadata shape so proactive notifications can be | ||
| // delivered to a chat ID. | ||
| if self.name != "telegram" { | ||
| return Err(ChannelError::SendFailed { | ||
| name: self.name.clone(), | ||
| reason: "WASM broadcast is only implemented for telegram".to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The broadcast function contains logic specific to Telegram, such as parsing the user_id as a chat_id and constructing Telegram-specific metadata. This is implemented in the generic WasmChannel wrapper, which makes it difficult to support broadcasting for other WASM-backed channels in the future without adding more channel-specific if/else blocks.
For better maintainability, this logic should ideally be delegated to the WASM module itself. A potential improvement would be to add an on_broadcast function to the WIT interface. WasmChannel::broadcast would then call this function, and each WASM channel supporting broadcast would implement its own logic.
While the current approach is a pragmatic fix, this is a suggestion for future refactoring when expanding broadcast capabilities.
| #[tokio::test] | ||
| async fn test_broadcast_accepts_valid_telegram_chat_id() { | ||
| let channel = create_telegram_test_channel(); | ||
| let result = channel | ||
| .broadcast("146032821", OutgoingResponse::text("hello")) | ||
| .await; | ||
|
|
||
| assert!(result.is_ok()); | ||
| } |
There was a problem hiding this comment.
The test test_broadcast_accepts_valid_telegram_chat_id uses a test channel without a WASM component. In this scenario, call_on_status returns Ok(()) immediately without executing any WASM code. As a result, this test only verifies that the initial validation in broadcast passes, but it doesn't cover the full success path of the call_on_status invocation.
To make this test more robust, consider using a mock WASM component that can assert it was called correctly, or find another way to verify that call_on_status is invoked with the expected parameters.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Summary
This is a well-scoped, focused PR that fixes a real bug (WasmChannel/SharedWasmChannel silently dropping broadcasts due to the trait's no-op default). The implementation is pragmatic — piggybacking broadcast on the existing call_on_status path avoids a new WASM callback while reusing Telegram's retry logic. The three new tests cover the main validation paths.
The main concerns are: (1) the callback_timeout() refactoring silently changes timeout source for 8 existing callsites, which could be intentional but warrants documentation; (2) the happy-path test doesn't exercise actual WASM execution due to component: None; (3) the synthetic metadata (user_id = chat_id, is_private = true) hardcodes assumptions that may break for group chats. None of these are blocking — the PR achieves its stated goal correctly for the private-chat Telegram use case it was built for.
| reason: format!("Invalid telegram chat_id '{}': {}", user_id, e), | ||
| })?; | ||
|
|
||
| let metadata = serde_json::json!({ |
There was a problem hiding this comment.
user_id metadata field set to chat_id may be incorrect for group chats
The broadcast metadata hardcodes "user_id": chat_id. In Telegram, user_id and chat_id are distinct concepts: a group/supergroup chat_id is negative and is not a user_id. If the downstream WASM module's on_status handler uses the user_id field for any authorization, attribution, or routing logic, it will receive the chat_id instead, which is semantically wrong for non-private chats. Combined with is_private: true being hardcoded, a broadcast to a group chat_id would present contradictory metadata.
Suggested fix:
Either (a) validate that chat_id is positive (private chats only) and reject group chat IDs explicitly, or (b) accept an optional separate user_id parameter for group-chat broadcasts and set is_private accordingly. At minimum, add a comment documenting that this path is only valid for private chats.Severity: medium · Confidence: medium
| assert!(err.contains("Invalid telegram chat_id")); | ||
| } | ||
|
|
||
| #[tokio::test] |
There was a problem hiding this comment.
test_broadcast_accepts_valid_telegram_chat_id does not exercise WASM execution
The test creates a WasmChannel with component: None, which causes call_on_status to return Ok(()) immediately (line ~1253: if self.prepared.component().is_none() { return Ok(()); }). This means the test only validates the input-validation path (name check, chat_id parsing) but never tests that the WASM component actually receives and processes the broadcast. A valid chat_id could still fail when a real component is present (e.g., the WASM module rejects message_id: 0, or the status handler doesn't handle Status variant as a broadcast).
Suggested fix:
Add a comment to the test clarifying it only validates pre-WASM validation. Consider adding an integration test with a minimal WASM component that asserts the `on_status` callback is invoked with the expected `Status` payload and synthetic metadata.Severity: medium · Confidence: high
| impl WasmChannel { | ||
| #[inline] | ||
| fn callback_timeout(&self) -> Duration { | ||
| self.capabilities.callback_timeout |
There was a problem hiding this comment.
callback_timeout() silently changes timeout source from runtime config to per-channel capabilities across 8 callsites
The new callback_timeout() helper returns self.capabilities.callback_timeout instead of the previous self.runtime.config().callback_timeout. This changes all 8 existing callsites (call_on_start, call_on_respond, call_on_http_request, call_on_poll, call_on_status, handle_status_update, on_credentials_refreshed). If ChannelCapabilities::callback_timeout and WasmChannelRuntimeConfig::callback_timeout ever have different values, all callback timeout behavior changes. The PR description focuses on broadcast but this refactoring has broader impact.
Suggested fix:
This is likely intentional (per-channel timeout overrides global runtime timeout), but add a brief comment on the method explaining this design choice: that capabilities timeout takes precedence over runtime config timeout. Also verify that `ChannelCapabilities::for_channel()` initializes `callback_timeout` to a sane default that matches or improves on the runtime config default.Severity: medium · Confidence: medium
| } | ||
|
|
||
| let chat_id = user_id.parse::<i64>().map_err(|e| ChannelError::SendFailed { | ||
| name: self.name.clone(), |
There was a problem hiding this comment.
No validation that chat_id is non-zero
The code parses user_id as i64 but does not check for zero. A chat_id of 0 is invalid in Telegram's API and would cause a silent failure or error at the Telegram API level when the WASM module tries to send the message.
Suggested fix:
Add a check after parsing: `if chat_id == 0 { return Err(ChannelError::SendFailed { name: self.name.clone(), reason: "chat_id cannot be zero".into() }); }`Severity: low · Confidence: medium
| @@ -2435,6 +2521,40 @@ mod tests { | |||
| assert!(channel.health_check().await.is_err()); | |||
There was a problem hiding this comment.
Missing test for attachment rejection path
The broadcast implementation rejects non-empty attachments (lines 2025-2029), but there is no test covering this path. Only non-telegram rejection and invalid chat_id are tested.
Suggested fix:
Add a test: create a telegram test channel, call `broadcast` with `OutgoingResponse::text("hello").with_attachments(vec!["file.png".to_string()])`, and assert the error contains "does not support attachments".Severity: low · Confidence: high
| })?; | ||
|
|
||
| let metadata = serde_json::json!({ | ||
| "chat_id": chat_id, |
There was a problem hiding this comment.
Hardcoded is_private: true may cause issues if broadcast is extended to groups
The metadata hardcodes is_private: true. If broadcast is later used for group notifications (negative chat_id), this field will be incorrect. The Telegram WASM module may use this flag to decide reply behavior, formatting, or permission checks.
Suggested fix:
Derive `is_private` from the chat_id sign: `"is_private": chat_id > 0`. In Telegram, positive IDs are users/private chats, negative IDs are groups/supergroups/channels.Severity: low · Confidence: medium
| self.handle_status_update(status, metadata).await | ||
| } | ||
|
|
||
| async fn broadcast( |
There was a problem hiding this comment.
broadcast method lacks doc comment explaining contract and limitations
The broadcast implementation is non-trivial — it only supports telegram, rejects attachments, synthesizes metadata with hardcoded fields, and piggybacks on call_on_status. A doc comment would help future maintainers understand the design choices and constraints without reading the full implementation.
Suggested fix:
Add a doc comment above the `broadcast` method explaining: (1) only telegram is supported, (2) attachments are not supported, (3) broadcast uses the `on_status(Status)` callback path, (4) `message_id: 0` signals the WASM module to skip reply context, (5) assumes private chat targeting.Severity: low · Confidence: high
|
Thanks for the detailed review — addressed in the latest push ( What changed:
If useful, I can follow up with a separate integration test PR that uses a minimal WASM component to assert |
Yeah that would be great! Rest looks good now. |
481f4ab to
152d8e5
Compare
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Telegram Broadcast Path for WASM Channels
The code quality is good -- input validation, error handling, and test coverage are solid. But there's an architectural conflict that needs resolving.
Blocker: Conflicts with Existing Generic Broadcast
PR #398 (merged after this PR was branched) added a generic broadcast() implementation to WasmChannel at line 2103 that uses last_broadcast_metadata and calls call_on_respond. This PR adds a second Telegram-specific broadcast() at line ~2153 that hardcodes metadata from user_id and calls call_on_status.
These two implementations will conflict on rebase. More importantly, the project convention is "prefer generic/extensible architectures over hardcoding specific integrations."
The existing generic approach is the right architecture. The gap it has is: it fails with "No messages received yet" when no prior message has been received (so last_broadcast_metadata is empty). This PR solves a real problem -- proactive sends to users who haven't messaged yet -- but the fix should enhance the generic path, not add a Telegram-specific bypass.
Suggested Approach
Instead of hardcoding Telegram metadata construction, extend the existing generic broadcast to accept a fallback when last_broadcast_metadata is unavailable:
- Let the WASM channel module (Telegram, etc.) define how to construct broadcast metadata from a
user_idvia a new WIT callback (e.g.,build-broadcast-metadata(user-id: string) -> option<string>) - Or, simpler: have the broadcast try
last_broadcast_metadatafirst, then fall back to calling the WASM module'son_statuswith a minimal metadata envelope constructed from capabilities/config (not hardcoded per channel name)
Specific Hardcoding Issues
if self.name != "telegram"-- hardcodes channel name checkuser_id.parse::<i64>()-- assumes Telegram's numeric chat_id format- Hardcoded metadata JSON shape (
chat_id,message_id: 0,user_id,is_private) chat_id > 0for private-chat detection -- Telegram-specific semantics- Uses
call_on_statusinstead ofcall_on_respond-- different code path than the generic broadcast
What's Good
- Input validation is thorough (invalid chat_id, non-private, attachments)
- Error messages are clear and actionable
- 5 tests cover all the error paths plus the happy path
- The comment "This path is private-chat only" documents the limitation honestly
- SharedWasmChannel delegation is correct
Recommendation
Rebase onto main, resolve the conflict with the existing broadcast() from #398, and rework this as an enhancement to the generic broadcast path rather than a Telegram-specific bypass. The test suite can largely be kept as-is.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Clean implementation. Error handling follows IronClaw conventions (no .unwrap()/.expect(), ChannelError::SendFailed with context throughout). Guards for unsupported cases (attachments, non-telegram, non-private chats) are correct. SharedWasmChannel delegation is consistent with the existing pattern. Good test coverage on all error paths.
One minor observation: is_private: chat_id > 0 in the metadata JSON is always true at that point since the function returns early when chat_id <= 0. Not a bug -- it correctly communicates intent to the WASM module -- just a tautology. Fine to leave as-is.
CI FixMerged staging and resolved conflicts:
The fixed branch is at @davidpty — please pull from |
Pull request was closed
* channels/wasm: implement telegram broadcast path for message tool * channels/wasm: tighten telegram broadcast contract and tests * fix: resolve merge conflicts with staging for wasm broadcast - Remove duplicate broadcast() impls from WasmChannel and SharedWasmChannel (staging already has the generic call_on_broadcast path) - Remove obsolete telegram-specific test helpers and tests that tested the old telegram-only broadcast logic - Add test_broadcast_delegates_to_call_on_broadcast for the generic path - Fix missing fallback_deliverable field in job_monitor test SseEvents Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: davidpty <127684147+davidpty@users.noreply.github.com> Co-authored-by: firat.sertgoz <f@nuff.tech> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* channels/wasm: implement telegram broadcast path for message tool * channels/wasm: tighten telegram broadcast contract and tests * fix: resolve merge conflicts with staging for wasm broadcast - Remove duplicate broadcast() impls from WasmChannel and SharedWasmChannel (staging already has the generic call_on_broadcast path) - Remove obsolete telegram-specific test helpers and tests that tested the old telegram-only broadcast logic - Add test_broadcast_delegates_to_call_on_broadcast for the generic path - Fix missing fallback_deliverable field in job_monitor test SseEvents Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: davidpty <127684147+davidpty@users.noreply.github.com> Co-authored-by: firat.sertgoz <f@nuff.tech> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* channels/wasm: implement telegram broadcast path for message tool * channels/wasm: tighten telegram broadcast contract and tests * fix: resolve merge conflicts with staging for wasm broadcast - Remove duplicate broadcast() impls from WasmChannel and SharedWasmChannel (staging already has the generic call_on_broadcast path) - Remove obsolete telegram-specific test helpers and tests that tested the old telegram-only broadcast logic - Add test_broadcast_delegates_to_call_on_broadcast for the generic path - Fix missing fallback_deliverable field in job_monitor test SseEvents Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: davidpty <127684147+davidpty@users.noreply.github.com> Co-authored-by: firat.sertgoz <f@nuff.tech> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nearai#1460) * channels/wasm: implement telegram broadcast path for message tool * channels/wasm: tighten telegram broadcast contract and tests * fix: resolve merge conflicts with staging for wasm broadcast - Remove duplicate broadcast() impls from WasmChannel and SharedWasmChannel (staging already has the generic call_on_broadcast path) - Remove obsolete telegram-specific test helpers and tests that tested the old telegram-only broadcast logic - Add test_broadcast_delegates_to_call_on_broadcast for the generic path - Fix missing fallback_deliverable field in job_monitor test SseEvents Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: davidpty <127684147+davidpty@users.noreply.github.com> Co-authored-by: firat.sertgoz <f@nuff.tech> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nearai#1460) * channels/wasm: implement telegram broadcast path for message tool * channels/wasm: tighten telegram broadcast contract and tests * fix: resolve merge conflicts with staging for wasm broadcast - Remove duplicate broadcast() impls from WasmChannel and SharedWasmChannel (staging already has the generic call_on_broadcast path) - Remove obsolete telegram-specific test helpers and tests that tested the old telegram-only broadcast logic - Add test_broadcast_delegates_to_call_on_broadcast for the generic path - Fix missing fallback_deliverable field in job_monitor test SseEvents Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: davidpty <127684147+davidpty@users.noreply.github.com> Co-authored-by: firat.sertgoz <f@nuff.tech> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 participants
Summary
Fix proactive Telegram sends from the
messagetool when channel is WASM-backed.Root Cause
WasmChannel/SharedWasmChanneldid not implementbroadcast(), so the default trait implementation returnedOk(())without delivering anything.Changes
WasmChannel::broadcast()for Telegram proactive sends.broadcast()inSharedWasmChannel.src/channels/wasm/wrapper.rs.Validation
cargo test -q test_broadcast_ -- src/channels/wasm/wrapper.rsTEST-WASM-BROADCAST-1772150835to chat146032821.