Conversation
📝 WalkthroughWalkthroughThis PR removes several TypeScript channel integrations and adds equivalent Rust worker binaries, updates the Cargo workspace members, and adjusts/adds channel-related test suites to reflect the migration from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 1/3 review remaining, refill in 34 minutes and 1 second. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if !resp.status().is_success() { | ||
| return Ok(None); |
There was a problem hiding this comment.
🔴 Webex fetch_message masks upstream API errors by returning 200 instead of 502
In the old TypeScript implementation (src/channels/webex.ts:39-43), when the Webex API returned a non-OK response for message fetching, the webhook handler returned status_code: 502 with an error body, properly signaling an upstream failure to the webhook provider. In the new Rust implementation, fetch_message returns Ok(None) on a non-success status (workers/channel-webex/src/main.rs:82-83), and the caller at line 158-160 treats None identically to an empty/missing text — returning status_code: 200. This masks upstream API errors (auth failures, rate limiting, server errors) as success, which prevents webhook providers from retrying failed deliveries and causes silent message loss.
Prompt for agents
In workers/channel-webex/src/main.rs, the fetch_message function returns Ok(None) when the Webex API returns a non-success status code (line 82-83). The caller in handle_webhook (lines 158-160) then treats this the same as empty text, returning status_code 200. This differs from the old TypeScript implementation in src/channels/webex.ts which returned status_code 502 with an error message when the API call failed.
To fix this, fetch_message should return an error (Err) on non-success status codes, or return a distinct variant that handle_webhook can differentiate from 'no text found'. The handle_webhook function should then return status_code 502 with an appropriate error body when the Webex API fetch fails, matching the old behavior. For example, you could change the Ok(None) to Err(IIIError::Handler(...)) and handle it in handle_webhook, or use a three-variant return type.
Was this helpful? React with 👍 or 👎 to provide feedback.
rohitg00
force-pushed
the
feat/port-channels-batch-2
branch
from
095af8d to
2f7eeee
Compare
| describe("Matrix channel", () => { | ||
| beforeAll(async () => { | ||
| process.env.TEAMS_APP_ID = "test-app-id"; | ||
| process.env.TEAMS_APP_PASSWORD = "test-password"; | ||
| await import("../channels/teams.js"); | ||
| process.env.MATRIX_HOMESERVER = "https://matrix.example.org"; | ||
| process.env.MATRIX_TOKEN = "test-matrix-token"; | ||
| await import("../channels/matrix.js"); |
There was a problem hiding this comment.
🔴 channels-teams.test.ts replaced with duplicate Matrix tests instead of being deleted
The file src/__tests__/channels-teams.test.ts was modified to test channel::matrix::webhook by importing ../channels/matrix.js — but src/channels/matrix.ts does not exist (the Matrix channel is a Rust-only worker at workers/channel-matrix/). This makes the file an exact, character-for-character duplicate of the newly-created src/__tests__/channels-matrix.test.ts. Both files will fail at await import("../channels/matrix.js") since the module doesn't exist on disk. The file should have been deleted when src/channels/teams.ts was removed (ported to workers/channel-teams/).
Prompt for agents
The file src/__tests__/channels-teams.test.ts was accidentally overwritten with Matrix channel test content that is identical to the new src/__tests__/channels-matrix.test.ts. The original Teams TypeScript channel (src/channels/teams.ts) was deleted and ported to Rust (workers/channel-teams/src/main.rs), so this TypeScript test file should be deleted entirely. Both this file and channels-matrix.test.ts import '../channels/matrix.js' which does not exist as a TypeScript module (the Matrix channel is a Rust worker at workers/channel-matrix/), so both will fail at runtime. Delete src/__tests__/channels-teams.test.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| process.env.BLUESKY_HANDLE = "test.bsky.social"; | ||
| process.env.BLUESKY_PASSWORD = "test-password"; | ||
| await import("../channels/bluesky.js"); |
There was a problem hiding this comment.
🔴 channels-webex.test.ts replaced with duplicate Bluesky tests instead of being deleted
The file src/__tests__/channels-webex.test.ts was modified to test channel::bluesky::webhook by importing ../channels/bluesky.js — but src/channels/bluesky.ts does not exist (the Bluesky channel is a Rust-only worker at workers/channel-bluesky/). This makes the file an exact, character-for-character duplicate of the newly-created src/__tests__/channels-bluesky.test.ts. Both files will fail at await import("../channels/bluesky.js") since the module doesn't exist on disk. The file should have been deleted when src/channels/webex.ts was removed (ported to workers/channel-webex/).
Prompt for agents
The file src/__tests__/channels-webex.test.ts was accidentally overwritten with Bluesky channel test content that is identical to the new src/__tests__/channels-bluesky.test.ts. The original Webex TypeScript channel (src/channels/webex.ts) was deleted and ported to Rust (workers/channel-webex/src/main.rs), so this TypeScript test file should be deleted entirely. Both this file and channels-bluesky.test.ts import '../channels/bluesky.js' which does not exist as a TypeScript module (the Bluesky channel is a Rust worker at workers/channel-bluesky/), so both will fail at runtime. Delete src/__tests__/channels-webex.test.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/channels-webex.test.ts (1)
69-164:⚠️ Potential issue | 🟠 MajorRestore Webex coverage in this file.
channels-webex.test.tsnow sets Bluesky credentials, imports../channels/bluesky.js, and assertschannel::bluesky::webhook. That leaves the Webex adapter added in this PR without any test coverage here and duplicates the Bluesky suite instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/channels-webex.test.ts` around lines 69 - 164, The test file mistakenly contains Bluesky tests instead of Webex coverage; restore Webex tests by changing the setup in beforeAll to set Webex-specific env vars and import ../channels/webex.js (not ../channels/bluesky.js), update the suite name and all assertions to target the Webex handler id "channel::webex::webhook" (references: handlers map, call helper, beforeAll), and adjust test cases to simulate Webex payloads and expected interactions with mocks used elsewhere (mockFetch, mockTrigger, mockTriggerVoid) so the Webex adapter added in this PR has proper coverage.
🧹 Nitpick comments (7)
workers/channel-whatsapp/src/main.rs (1)
166-173: Consider consistency: empty reply handling differs from Signal worker.The WhatsApp handler silently succeeds when
reply.is_empty(), only skipping the send. The Signal worker (lines 166-172 there) returns a 500 error for empty agent responses. While both approaches are valid, inconsistent behavior across channel adapters may complicate debugging.If the silent success is intentional (e.g., WhatsApp allows acknowledgment without reply), a brief comment would clarify the design choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-whatsapp/src/main.rs` around lines 166 - 173, The WhatsApp handler currently silently skips sending when reply (from chat.get("content")) is empty, which differs from the Signal worker that returns a 500; either make the behavior consistent or document it: either (A) change the empty-reply branch to mirror Signal by logging an error and returning a 500-style error response (propagate an Err from this handler after tracing::error!(to = %from, "empty agent response") so the request fails), or (B) if silent-acknowledgement is intentional, add a concise comment above the reply check explaining why empty replies are ok for WhatsApp; update code around reply, get_secret, and send_message accordingly so the chosen behavior is explicit.workers/channel-signal/src/main.rs (1)
9-70: Significant code duplication with WhatsApp worker.The
get_secret,resolve_agent, andsplit_messagefunctions are identical to those inchannel-whatsapp/src/main.rs. Consider extracting these into a shared crate (e.g.,channel-commonor withiniii-sdk) to reduce maintenance burden and ensure consistent behavior across all channel adapters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-signal/src/main.rs` around lines 9 - 70, The three identical helper functions get_secret, resolve_agent, and split_message should be extracted into a shared library to eliminate duplication; create a new crate/module (e.g., channel_common or add to iii-sdk) containing these functions (preserving signatures that accept &III and the same return types), export them publicly, then remove the local implementations and import the shared versions in both channel adapters; finally add the new crate as a dependency in each adapter's Cargo.toml and update use/import lines to reference the shared module so both workers call the single canonical implementations.workers/channel-email/src/main.rs (1)
166-216: These tests do not exercise the webhook logic you just ported.The current cases only restate local formatting and missing-field checks; they never call
handle_webhook, mockIII, or verify theagent::chat/audit/send-mail flow. That leaves the critical behavior in this file largely unprotected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-email/src/main.rs` around lines 166 - 216, Tests in main.rs only validate local formatting and missing-field logic and never exercise the webhook path; add unit tests that invoke handle_webhook and assert the end-to-end behavior by mocking the III interface and dependencies (agent::chat, audit, send-mail). Specifically, write tests that: call handle_webhook with JSON payloads covering cases (missing from, missing text, normal message), inject a mock III implementation that records calls to agent::chat and audit, and a mock mail sender that records outgoing emails, then assert the mocks received the expected calls (session id, reply subject, chat prompt/results, and audit/send-mail invocations). Use the existing test helpers/setup pattern in this file to initialize the runtime and dependency injection so the new tests exercise the webhook logic rather than only local formatting.src/__tests__/channels-bluesky.test.ts (1)
10-79: Use the shared test harness instead of re-creating it locally.This file duplicates the
handlersregistry,call()helper, and ad hoc mocks rather than using the repository's shared test setup. Please switch this suite to the common helpers/mocks so it stays aligned with the other channel tests.As per coding guidelines, "Use shared test helpers from
src/__tests__/helpers.tsincluding KV mock and request builders" and "Mock shared modulesutils.js,logger.js,metrics.js,errors.js."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/channels-bluesky.test.ts` around lines 10 - 79, This test duplicates the shared test harness (handlers registry, call helper, fetch and module mocks) instead of using the repo-wide helpers; remove the local handlers, call(), mockFetch and the ad-hoc vi.mock(...) blocks and instead import and invoke the common test helpers from src/__tests__/helpers.ts (use the harness setup function that provides the handlers registry and call() helper plus the KV and request builders), and replace local module mocks with the shared mocks for utils, logger, metrics, and errors so the channel test uses the centralized mock setup and then import "../channels/bluesky.js" after initializing the shared harness.workers/channel-webex/src/main.rs (2)
245-292: Unit tests cover core logic but could benefit from additional coverage.The tests effectively cover event filtering, JSON extraction, chunking, and session ID formatting. Consider adding a test for the newline-splitting behavior in
split_messageto verify chunks don't start with leading newlines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-webex/src/main.rs` around lines 245 - 292, Add a unit test in the tests module that asserts split_message does not produce chunks beginning with a newline when given input containing newline boundaries; call split_message with a string containing deliberate newline boundaries (e.g., many newlines or newline-separated paragraphs) and verify each returned chunk does not start_with('\n') and that the concatenation equals the original input; reference the existing split_message function and add the test alongside the other #[test]s in the tests module.
180-184: Silent failure on send error may mask issues in production.When
send_messagefails, the error is logged but the function still returns HTTP 200 OK. While this prevents the webhook from being retried by Webex, consider whether the audit payload should include a failure indicator or if a different status code would be more appropriate for observability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-webex/src/main.rs` around lines 180 - 184, The current code logs send_message failures (send_message, room_id, reply) but still returns HTTP 200, which can hide delivery problems; modify the webhook handler so that when send_message returns Err(e) you not only tracing::error! but also propagate that failure into the response/audit payload (e.g., add a "send_status":"failed" and error details) or return a non-200 response (e.g., propagate the error to produce a 5xx) instead of silently returning 200; update the code path that calls send_message to convert the Err into the chosen failure signal and ensure the audit payload or HTTP response includes the error and room_id for observability.workers/channel-teams/src/main.rs (1)
8-69: Code duplication:get_secret,resolve_agent, andsplit_messageare duplicated across multiple channel workers with implementation variants.These three functions are repeated across 10 channel workers (all except
channel-emailforget_secret/split_message;channel-emailandchannel-slackhave variant signatures forresolve_agent). However, they are not uniformly identical:
get_secret: Two variants (guard pattern vs. nested-if)resolve_agent: Three variants (match pattern, guard pattern, and a Slack-specific variant with different signature)split_message: Two variants (String-based vs. &str-based)
channel-teamsuses the guard pattern variant forget_secretandresolve_agent, and the String-based variant forsplit_message—matchingchannel-webex,channel-slack,channel-signal, andchannel-whatsapp.Extracting these functions into a shared crate (e.g.,
workers/channel-common) would reduce maintenance burden, though the implementation variants across workers suggest considering whether a single shared implementation or a small abstraction supporting both patterns is more appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-teams/src/main.rs` around lines 8 - 69, The three duplicated helpers (get_secret, resolve_agent, split_message) should be moved into a shared crate (e.g., workers/channel-common) and referenced from each channel worker to remove duplication; implement a single canonical API there: async fn get_secret(iii: &III, key: &str) -> String, async fn resolve_agent(iii: &III, channel: &str, channel_id: &str) -> String, and fn split_message(text: &str, max_len: usize) -> Vec<String> (use the guard-style result checks already in channel-teams for get_secret/resolve_agent and the &str-based split_message), then update channel-specific crates to call these shared functions or add tiny adapter wrappers where a variant signature is needed (e.g., Slack’s differing resolve_agent signature) so all workers import the same implementation from channel-common.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/channels-matrix.test.ts`:
- Around line 177-181: The test currently guards the Authorization header check
with "if (putCall)" which allows the assertion to be skipped silently; change
the test to explicitly assert that a PUT call was made (e.g.,
expect(putCall).toBeDefined() or expect(putCall).not.toBeUndefined()) before
verifying putCall[1].headers.Authorization equals "Bearer test-matrix-token",
locating the check around the mockFetch.mock.calls lookup for putCall in the
channels-matrix.test.ts test so the header assertion always runs only after
confirming putCall exists.
- Around line 148-153: The test currently guards the PUT assertion with if
(putCall) which allows the test to silently pass when no PUT occurred; update
the test in channels-matrix.test.ts to assert the PUT happened before inspecting
it by adding expect(putCall).toBeDefined() (or
expect(putCall).not.toBeUndefined()) right after finding
mockFetch.mock.calls.find(...), then safely parse JSON from putCall[1].body and
assert expect(body.msgtype).toBe("m.text"); keep references to mockFetch and
putCall so the check fails loudly when the PUT request is missing.
In `@src/__tests__/channels-teams.test.ts`:
- Around line 52-61: The test file channels-teams.test.ts contains duplicated
Matrix tests (it registers "channel::matrix::webhook") and should be removed or
renamed to reflect its actual purpose; either delete channels-teams.test.ts to
avoid duplication with the existing channels-matrix.test.ts, or rename the file
and update any test descriptions so the filename and the describe/it blocks
match (e.g., rename to channels-matrix.test.ts or change the tested channel to
Teams and the handler key), and ensure only one test covers the
"channel::matrix::webhook" registration.
In `@workers/channel-email/src/main.rs`:
- Around line 29-37: The SMTP config in build_transport currently reads
SMTP_HOST, SMTP_PORT, SMTP_SECURE, SMTP_USER, SMTP_PASS directly from env;
change it to first attempt reading each secret via the project's vault-fallback
helper (same helper used by other adapters) and only if the vault call returns
None/failure fall back to std::env::var or the existing defaults; apply the same
change to the other SMTP env reads later in the file (the same SMTP_* keys
referenced around lines 53-55) and ensure you still parse port to u16 and parse
secure as a boolean after obtaining the value from the vault-or-env result.
- Around line 102-107: The code currently always calls send_mail with reply
derived from chat.get("content"), which can be empty/whitespace and causes blank
emails; update the handler to trim and check the reply string (from
chat.get("content").and_then(|v| v.as_str()).unwrap_or("")) and only call
send_mail(from, &reply_subject, reply).await when reply.trim().is_empty() is
false (i.e., skip sending when reply.trim().is_empty()); keep the existing
reply_subject/reply variables and error logging around send_mail unchanged when
you early-return or skip the send.
In `@workers/channel-signal/src/main.rs`:
- Around line 166-172: The current branch that handles reply.is_empty() returns
a 500 JSON status which may trigger webhook retries; change the response to a
non-retry status (e.g., return status_code: 200 with an error body or a 4xx)
instead of 500. Update the return in the reply.is_empty() block (the branch
referencing reply.is_empty() and channel_key) to emit a safe non-retry status
(recommended: status_code: 200 and a clear body { "error": "Empty agent
response" } or an appropriate 4xx) so webhook sources won’t retry; keep the
existing log tracing::warn!(channel_key = %channel_key, ...) intact. Ensure the
returned JSON shape stays consistent with other responses expected by the
caller.
In `@workers/channel-teams/src/main.rs`:
- Around line 61-66: split_message can produce chunks that start with a '\n'
because when you split at a newline index (split_at) you push
remaining[..split_at] but then set remaining = remaining[split_at..], leaving
the newline at the start of the next chunk; change the update to skip the
newline (set remaining = remaining[split_at + 1..].to_string()) and ensure you
only add +1 when the chosen split_at came from a newline match (i.e., when using
window.rfind('\n')), guarding against out-of-bounds; reference the
variables/functions window, split_at, remaining, and chunks in your change.
In `@workers/channel-webex/src/main.rs`:
- Around line 44-69: In split_message, when you choose a newline split via
split_at you must advance past the newline delimiter for the next remaining
slice to avoid producing chunks that begin with '\n'; update the assignment of
remaining (and any places that push the current chunk) to use split_at + 1 as
the start of the next remaining string when the split point was a newline (i.e.,
detect that window.rfind('\n') returned Some and then set remaining =
remaining[split_at + 1..].to_string()) while leaving non-newline cuts unchanged.
In `@workers/channel-whatsapp/src/main.rs`:
- Around line 62-68: The split logic leaves a leading newline at the start of
the next chunk when split_at was chosen from window.rfind('\n'); update the code
that updates remaining to skip the newline character when present. Specifically,
after computing split_at (from window.rfind or fallback), push
remaining[..split_at] as before, then set remaining to start at split_at+1 if
remaining.as_bytes()[split_at] == b'\n' (or otherwise detect the '\n' at
split_at) so the next chunk does not begin with '\n' — otherwise keep remaining
starting at split_at as currently done; adjust uses of variables window,
split_at, chunks, and remaining accordingly.
- Around line 18-23: The new let-chain syntax used in main.rs (the if let
Ok(value) = result && let Some(v) = value.get("value").and_then(|v| v.as_str())
&& !v.is_empty() { ... }) requires Rust 1.88.0+ with the 2024 edition, so add an
explicit minimum Rust version: set rust-version = "1.88" (or "1.88.0") in the
root Cargo.toml [package] and/or ensure each crate’s Cargo.toml that contains
main.rs declares rust-version = "1.88" so the workspace MSRV matches the syntax
used; update Cargo.toml entries accordingly and run cargo metadata/cargo check
to verify the workspace accepts the let-chain in function containing
result/value/v usage.
---
Outside diff comments:
In `@src/__tests__/channels-webex.test.ts`:
- Around line 69-164: The test file mistakenly contains Bluesky tests instead of
Webex coverage; restore Webex tests by changing the setup in beforeAll to set
Webex-specific env vars and import ../channels/webex.js (not
../channels/bluesky.js), update the suite name and all assertions to target the
Webex handler id "channel::webex::webhook" (references: handlers map, call
helper, beforeAll), and adjust test cases to simulate Webex payloads and
expected interactions with mocks used elsewhere (mockFetch, mockTrigger,
mockTriggerVoid) so the Webex adapter added in this PR has proper coverage.
---
Nitpick comments:
In `@src/__tests__/channels-bluesky.test.ts`:
- Around line 10-79: This test duplicates the shared test harness (handlers
registry, call helper, fetch and module mocks) instead of using the repo-wide
helpers; remove the local handlers, call(), mockFetch and the ad-hoc
vi.mock(...) blocks and instead import and invoke the common test helpers from
src/__tests__/helpers.ts (use the harness setup function that provides the
handlers registry and call() helper plus the KV and request builders), and
replace local module mocks with the shared mocks for utils, logger, metrics, and
errors so the channel test uses the centralized mock setup and then import
"../channels/bluesky.js" after initializing the shared harness.
In `@workers/channel-email/src/main.rs`:
- Around line 166-216: Tests in main.rs only validate local formatting and
missing-field logic and never exercise the webhook path; add unit tests that
invoke handle_webhook and assert the end-to-end behavior by mocking the III
interface and dependencies (agent::chat, audit, send-mail). Specifically, write
tests that: call handle_webhook with JSON payloads covering cases (missing from,
missing text, normal message), inject a mock III implementation that records
calls to agent::chat and audit, and a mock mail sender that records outgoing
emails, then assert the mocks received the expected calls (session id, reply
subject, chat prompt/results, and audit/send-mail invocations). Use the existing
test helpers/setup pattern in this file to initialize the runtime and dependency
injection so the new tests exercise the webhook logic rather than only local
formatting.
In `@workers/channel-signal/src/main.rs`:
- Around line 9-70: The three identical helper functions get_secret,
resolve_agent, and split_message should be extracted into a shared library to
eliminate duplication; create a new crate/module (e.g., channel_common or add to
iii-sdk) containing these functions (preserving signatures that accept &III and
the same return types), export them publicly, then remove the local
implementations and import the shared versions in both channel adapters; finally
add the new crate as a dependency in each adapter's Cargo.toml and update
use/import lines to reference the shared module so both workers call the single
canonical implementations.
In `@workers/channel-teams/src/main.rs`:
- Around line 8-69: The three duplicated helpers (get_secret, resolve_agent,
split_message) should be moved into a shared crate (e.g.,
workers/channel-common) and referenced from each channel worker to remove
duplication; implement a single canonical API there: async fn get_secret(iii:
&III, key: &str) -> String, async fn resolve_agent(iii: &III, channel: &str,
channel_id: &str) -> String, and fn split_message(text: &str, max_len: usize) ->
Vec<String> (use the guard-style result checks already in channel-teams for
get_secret/resolve_agent and the &str-based split_message), then update
channel-specific crates to call these shared functions or add tiny adapter
wrappers where a variant signature is needed (e.g., Slack’s differing
resolve_agent signature) so all workers import the same implementation from
channel-common.
In `@workers/channel-webex/src/main.rs`:
- Around line 245-292: Add a unit test in the tests module that asserts
split_message does not produce chunks beginning with a newline when given input
containing newline boundaries; call split_message with a string containing
deliberate newline boundaries (e.g., many newlines or newline-separated
paragraphs) and verify each returned chunk does not start_with('\n') and that
the concatenation equals the original input; reference the existing
split_message function and add the test alongside the other #[test]s in the
tests module.
- Around line 180-184: The current code logs send_message failures
(send_message, room_id, reply) but still returns HTTP 200, which can hide
delivery problems; modify the webhook handler so that when send_message returns
Err(e) you not only tracing::error! but also propagate that failure into the
response/audit payload (e.g., add a "send_status":"failed" and error details) or
return a non-200 response (e.g., propagate the error to produce a 5xx) instead
of silently returning 200; update the code path that calls send_message to
convert the Err into the chosen failure signal and ensure the audit payload or
HTTP response includes the error and room_id for observability.
In `@workers/channel-whatsapp/src/main.rs`:
- Around line 166-173: The WhatsApp handler currently silently skips sending
when reply (from chat.get("content")) is empty, which differs from the Signal
worker that returns a 500; either make the behavior consistent or document it:
either (A) change the empty-reply branch to mirror Signal by logging an error
and returning a 500-style error response (propagate an Err from this handler
after tracing::error!(to = %from, "empty agent response") so the request fails),
or (B) if silent-acknowledgement is intentional, add a concise comment above the
reply check explaining why empty replies are ok for WhatsApp; update code around
reply, get_secret, and send_message accordingly so the chosen behavior is
explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bac55c28-90a2-4fb9-a336-543dee150b0d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomlsrc/__tests__/channels-bluesky.test.tssrc/__tests__/channels-matrix.test.tssrc/__tests__/channels-mattermost.test.tssrc/__tests__/channels-teams.test.tssrc/__tests__/channels-webex.test.tssrc/__tests__/channels-whatsapp-signal.test.tssrc/channels/email.tssrc/channels/signal.tssrc/channels/teams.tssrc/channels/webex.tssrc/channels/whatsapp.tsworkers/channel-email/Cargo.tomlworkers/channel-email/iii.worker.yamlworkers/channel-email/src/main.rsworkers/channel-signal/Cargo.tomlworkers/channel-signal/iii.worker.yamlworkers/channel-signal/src/main.rsworkers/channel-teams/Cargo.tomlworkers/channel-teams/iii.worker.yamlworkers/channel-teams/src/main.rsworkers/channel-webex/Cargo.tomlworkers/channel-webex/iii.worker.yamlworkers/channel-webex/src/main.rsworkers/channel-whatsapp/Cargo.tomlworkers/channel-whatsapp/iii.worker.yamlworkers/channel-whatsapp/src/main.rs
💤 Files with no reviewable changes (7)
- src/tests/channels-mattermost.test.ts
- src/tests/channels-whatsapp-signal.test.ts
- src/channels/whatsapp.ts
- src/channels/webex.ts
- src/channels/email.ts
- src/channels/teams.ts
- src/channels/signal.ts
| const putCall = mockFetch.mock.calls.find(c => c[1]?.method === "PUT"); | ||
| if (putCall) { | ||
| const body = JSON.parse(putCall[1].body); | ||
| expect(body.msgtype).toBe("m.text"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Conditional assertion may silently pass without verifying the PUT call.
The if (putCall) guard means the assertion is skipped entirely if no PUT call was made, causing the test to pass without validating the expected behavior. Consider using expect(putCall).toBeDefined() before accessing properties.
Proposed fix
const putCall = mockFetch.mock.calls.find(c => c[1]?.method === "PUT");
- if (putCall) {
- const body = JSON.parse(putCall[1].body);
- expect(body.msgtype).toBe("m.text");
- }
+ expect(putCall).toBeDefined();
+ const body = JSON.parse(putCall![1].body);
+ expect(body.msgtype).toBe("m.text");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const putCall = mockFetch.mock.calls.find(c => c[1]?.method === "PUT"); | |
| if (putCall) { | |
| const body = JSON.parse(putCall[1].body); | |
| expect(body.msgtype).toBe("m.text"); | |
| } | |
| }); | |
| const putCall = mockFetch.mock.calls.find(c => c[1]?.method === "PUT"); | |
| expect(putCall).toBeDefined(); | |
| const body = JSON.parse(putCall![1].body); | |
| expect(body.msgtype).toBe("m.text"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/channels-matrix.test.ts` around lines 148 - 153, The test
currently guards the PUT assertion with if (putCall) which allows the test to
silently pass when no PUT occurred; update the test in channels-matrix.test.ts
to assert the PUT happened before inspecting it by adding
expect(putCall).toBeDefined() (or expect(putCall).not.toBeUndefined()) right
after finding mockFetch.mock.calls.find(...), then safely parse JSON from
putCall[1].body and assert expect(body.msgtype).toBe("m.text"); keep references
to mockFetch and putCall so the check fails loudly when the PUT request is
missing.
| fn build_transport() -> Result<AsyncSmtpTransport<Tokio1Executor>, IIIError> { | ||
| let host = std::env::var("SMTP_HOST").unwrap_or_else(|_| "localhost".to_string()); | ||
| let port: u16 = std::env::var("SMTP_PORT") | ||
| .ok() | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(587); | ||
| let secure = std::env::var("SMTP_SECURE").map(|s| s == "true").unwrap_or(false); | ||
| let user = std::env::var("SMTP_USER").unwrap_or_default(); | ||
| let pass = std::env::var("SMTP_PASS").unwrap_or_default(); |
There was a problem hiding this comment.
Read SMTP secrets through the same vault fallback as the other adapters.
This worker only consults process env for SMTP_*. In a vault-backed deployment, outbound email will never be configured, so replies degrade into logged send failures instead of working. Please resolve these settings via vault first and fall back to env only when the secret is absent.
Also applies to: 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-email/src/main.rs` around lines 29 - 37, The SMTP config in
build_transport currently reads SMTP_HOST, SMTP_PORT, SMTP_SECURE, SMTP_USER,
SMTP_PASS directly from env; change it to first attempt reading each secret via
the project's vault-fallback helper (same helper used by other adapters) and
only if the vault call returns None/failure fall back to std::env::var or the
existing defaults; apply the same change to the other SMTP env reads later in
the file (the same SMTP_* keys referenced around lines 53-55) and ensure you
still parse port to u16 and parse secure as a boolean after obtaining the value
from the vault-or-env result.
| let reply = chat.get("content").and_then(|v| v.as_str()).unwrap_or(""); | ||
| let reply_subject = format!("Re: {subject}"); | ||
|
|
||
| if let Err(e) = send_mail(from, &reply_subject, reply).await { | ||
| tracing::error!(to = %from, error = %e, "failed to send email reply"); | ||
| } |
There was a problem hiding this comment.
Skip the SMTP send when agent::chat returns no content.
reply is always passed to send_mail, so an empty or whitespace-only model response becomes a blank email to the user. That is a user-visible regression for the email channel.
Suggested fix
let reply = chat.get("content").and_then(|v| v.as_str()).unwrap_or("");
let reply_subject = format!("Re: {subject}");
- if let Err(e) = send_mail(from, &reply_subject, reply).await {
- tracing::error!(to = %from, error = %e, "failed to send email reply");
+ if !reply.trim().is_empty() {
+ if let Err(e) = send_mail(from, &reply_subject, reply).await {
+ tracing::error!(to = %from, error = %e, "failed to send email reply");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let reply = chat.get("content").and_then(|v| v.as_str()).unwrap_or(""); | |
| let reply_subject = format!("Re: {subject}"); | |
| if let Err(e) = send_mail(from, &reply_subject, reply).await { | |
| tracing::error!(to = %from, error = %e, "failed to send email reply"); | |
| } | |
| let reply = chat.get("content").and_then(|v| v.as_str()).unwrap_or(""); | |
| let reply_subject = format!("Re: {subject}"); | |
| if !reply.trim().is_empty() { | |
| if let Err(e) = send_mail(from, &reply_subject, reply).await { | |
| tracing::error!(to = %from, error = %e, "failed to send email reply"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-email/src/main.rs` around lines 102 - 107, The code currently
always calls send_mail with reply derived from chat.get("content"), which can be
empty/whitespace and causes blank emails; update the handler to trim and check
the reply string (from chat.get("content").and_then(|v|
v.as_str()).unwrap_or("")) and only call send_mail(from, &reply_subject,
reply).await when reply.trim().is_empty() is false (i.e., skip sending when
reply.trim().is_empty()); keep the existing reply_subject/reply variables and
error logging around send_mail unchanged when you early-return or skip the send.
| if reply.is_empty() { | ||
| tracing::warn!(channel_key = %channel_key, "signal: agent returned empty response"); | ||
| return Ok(json!({ | ||
| "status_code": 500, | ||
| "body": { "error": "Empty agent response" } | ||
| })); | ||
| } |
There was a problem hiding this comment.
Returning 500 for empty agent response may trigger webhook retries.
If the Signal webhook source retries on 5xx responses, this could cause duplicate processing attempts. Consider whether a 200 with an error body (matching WhatsApp's approach) or a 4xx status would be more appropriate for this "no response available" scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-signal/src/main.rs` around lines 166 - 172, The current
branch that handles reply.is_empty() returns a 500 JSON status which may trigger
webhook retries; change the response to a non-retry status (e.g., return
status_code: 200 with an error body or a 4xx) instead of 500. Update the return
in the reply.is_empty() block (the branch referencing reply.is_empty() and
channel_key) to emit a safe non-retry status (recommended: status_code: 200 and
a clear body { "error": "Empty agent response" } or an appropriate 4xx) so
webhook sources won’t retry; keep the existing log tracing::warn!(channel_key =
%channel_key, ...) intact. Ensure the returned JSON shape stays consistent with
other responses expected by the caller.
| let split_at = match window.rfind('\n') { | ||
| Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, | ||
| _ => cutoff, | ||
| }; | ||
| chunks.push(remaining[..split_at].to_string()); | ||
| remaining = remaining[split_at..].to_string(); |
There was a problem hiding this comment.
Same newline edge case in split_message as in webex worker.
When splitting at a newline, the remaining slice starts from the newline character itself, potentially producing chunks that begin with \n.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-teams/src/main.rs` around lines 61 - 66, split_message can
produce chunks that start with a '\n' because when you split at a newline index
(split_at) you push remaining[..split_at] but then set remaining =
remaining[split_at..], leaving the newline at the start of the next chunk;
change the update to skip the newline (set remaining = remaining[split_at +
1..].to_string()) and ensure you only add +1 when the chosen split_at came from
a newline match (i.e., when using window.rfind('\n')), guarding against
out-of-bounds; reference the variables/functions window, split_at, remaining,
and chunks in your change.
| fn split_message(text: &str, max_len: usize) -> Vec<String> { | ||
| if text.chars().count() <= max_len { | ||
| return vec![text.to_string()]; | ||
| } | ||
| let mut chunks: Vec<String> = Vec::new(); | ||
| let mut remaining = text.to_string(); | ||
| while !remaining.is_empty() { | ||
| if remaining.chars().count() <= max_len { | ||
| chunks.push(remaining); | ||
| break; | ||
| } | ||
| let cutoff = remaining | ||
| .char_indices() | ||
| .nth(max_len) | ||
| .map(|(idx, _)| idx) | ||
| .unwrap_or(remaining.len()); | ||
| let window = &remaining[..cutoff]; | ||
| let split_at = match window.rfind('\n') { | ||
| Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, | ||
| _ => cutoff, | ||
| }; | ||
| chunks.push(remaining[..split_at].to_string()); | ||
| remaining = remaining[split_at..].to_string(); | ||
| } | ||
| chunks | ||
| } |
There was a problem hiding this comment.
Edge case: splitting on newline may produce a leading newline in subsequent chunks.
When splitting at a newline index (Line 65), the remaining slice starts from the newline character itself. If the intent is to skip the newline delimiter, split_at + 1 should be used for the new remaining to avoid chunks beginning with \n.
Proposed fix
chunks.push(remaining[..split_at].to_string());
- remaining = remaining[split_at..].to_string();
+ remaining = if split_at < cutoff && remaining.as_bytes().get(split_at) == Some(&b'\n') {
+ remaining[split_at + 1..].to_string()
+ } else {
+ remaining[split_at..].to_string()
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-webex/src/main.rs` around lines 44 - 69, In split_message,
when you choose a newline split via split_at you must advance past the newline
delimiter for the next remaining slice to avoid producing chunks that begin with
'\n'; update the assignment of remaining (and any places that push the current
chunk) to use split_at + 1 as the start of the next remaining string when the
split point was a newline (i.e., detect that window.rfind('\n') returned Some
and then set remaining = remaining[split_at + 1..].to_string()) while leaving
non-newline cuts unchanged.
| if let Ok(value) = result | ||
| && let Some(v) = value.get("value").and_then(|v| v.as_str()) | ||
| && !v.is_empty() | ||
| { | ||
| return v.to_string(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if rust-toolchain.toml or Cargo.toml specifies an MSRV
fd -t f 'rust-toolchain' --exec cat {}
rg -n 'rust-version|msrv' Cargo.tomlRepository: iii-experimental/agentos
Length of output: 50
🏁 Script executed:
# Find all Cargo.toml files and check their structure
find . -name "Cargo.toml" -type f | head -20Repository: iii-experimental/agentos
Length of output: 711
🏁 Script executed:
# Check the Cargo.toml in the workers/channel-whatsapp directory specifically
cat workers/channel-whatsapp/Cargo.tomlRepository: iii-experimental/agentos
Length of output: 543
🏁 Script executed:
# Also check the root Cargo.toml to see workspace configuration
head -100 Cargo.tomlRepository: iii-experimental/agentos
Length of output: 1763
🌐 Web query:
Rust let-chains stabilization version release date 1.76
💡 Result:
Let-chains (#![feature(let_chains)]) were not stabilized in Rust 1.76.0. An earlier stabilization attempt was made for Rust 1.64 but was reverted due to issues. Let-chains were fully stabilized in Rust 1.88.0 for the 2024 edition only (earlier editions require the feature gate or error out). Rust 1.76.0 was released on February 8, 2024, and its release notes do not mention let-chains stabilization.
Citations:
- 1: Stabilize
let_chainsin Rust 1.64 rust-lang/rust#94927 - 2: Stabilize let chains in the 2024 edition rust-lang/rust#132833
- 3: Tracking issue for release notes of #132833: Stabilize let chains in the 2024 edition rust-lang/rust#139951
- 4: https://blog.rust-lang.org/2024/02/08/Rust-1.76.0/
- 5: https://releases.rs/docs/1.76.0/
- 6: https://github.com/rust-lang/rust/releases/tag/1.76.0
- 7: https://doc.rust-lang.org/beta/edition-guide/rust-2024/let-chains.html
Let-chains require Rust 1.88.0+ with the 2024 edition.
The if let ... && let ... syntax was stabilized in Rust 1.88.0 for the 2024 edition only. The workspace is correctly configured with edition = "2024" in the root Cargo.toml, so this syntax is valid. However, ensure the MSRV is set to 1.88.0 or higher—currently, no explicit MSRV is declared in the Cargo.toml files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-whatsapp/src/main.rs` around lines 18 - 23, The new let-chain
syntax used in main.rs (the if let Ok(value) = result && let Some(v) =
value.get("value").and_then(|v| v.as_str()) && !v.is_empty() { ... }) requires
Rust 1.88.0+ with the 2024 edition, so add an explicit minimum Rust version: set
rust-version = "1.88" (or "1.88.0") in the root Cargo.toml [package] and/or
ensure each crate’s Cargo.toml that contains main.rs declares rust-version =
"1.88" so the workspace MSRV matches the syntax used; update Cargo.toml entries
accordingly and run cargo metadata/cargo check to verify the workspace accepts
the let-chain in function containing result/value/v usage.
| let window = &remaining[..cutoff]; | ||
| let split_at = match window.rfind('\n') { | ||
| Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, | ||
| _ => cutoff, | ||
| }; | ||
| chunks.push(remaining[..split_at].to_string()); | ||
| remaining = remaining[split_at..].to_string(); |
There was a problem hiding this comment.
Newline character included at start of next chunk after split.
When split_at is set to a newline index, the newline character itself remains at the start of remaining in the next iteration. This can cause chunks to begin with \n.
🔧 Proposed fix to skip the newline
let split_at = match window.rfind('\n') {
Some(idx) if window[..idx].chars().count() > max_len / 2 => idx,
_ => cutoff,
};
chunks.push(remaining[..split_at].to_string());
- remaining = remaining[split_at..].to_string();
+ remaining = if split_at < cutoff && remaining.as_bytes().get(split_at) == Some(&b'\n') {
+ remaining[split_at + 1..].to_string()
+ } else {
+ remaining[split_at..].to_string()
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let window = &remaining[..cutoff]; | |
| let split_at = match window.rfind('\n') { | |
| Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, | |
| _ => cutoff, | |
| }; | |
| chunks.push(remaining[..split_at].to_string()); | |
| remaining = remaining[split_at..].to_string(); | |
| let window = &remaining[..cutoff]; | |
| let split_at = match window.rfind('\n') { | |
| Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, | |
| _ => cutoff, | |
| }; | |
| chunks.push(remaining[..split_at].to_string()); | |
| remaining = if split_at < cutoff && remaining.as_bytes().get(split_at) == Some(&b'\n') { | |
| remaining[split_at + 1..].to_string() | |
| } else { | |
| remaining[split_at..].to_string() | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-whatsapp/src/main.rs` around lines 62 - 68, The split logic
leaves a leading newline at the start of the next chunk when split_at was chosen
from window.rfind('\n'); update the code that updates remaining to skip the
newline character when present. Specifically, after computing split_at (from
window.rfind or fallback), push remaining[..split_at] as before, then set
remaining to start at split_at+1 if remaining.as_bytes()[split_at] == b'\n' (or
otherwise detect the '\n' at split_at) so the next chunk does not begin with
'\n' — otherwise keep remaining starting at split_at as currently done; adjust
uses of variables window, split_at, chunks, and remaining accordingly.
2f7eeee to
0d4fc49
Compare
Replaces src/channels/email.ts (75 LOC) with workers/channel-email/ (~210 LOC Rust + tests). Inbound webhook + outbound SMTP via lettre. Function IDs preserved: - channel::email::webhook HTTP trigger: POST /webhook/email Auth env: SMTP_HOST, SMTP_PORT, SMTP_SECURE, SMTP_USER, SMTP_PASS Continues #41 phase 3 channel migration.
Replaces src/channels/whatsapp.ts (93 LOC) with workers/channel-whatsapp/ (~290 LOC Rust + tests). Inbound webhook + outbound WhatsApp Business Graph API via reqwest. Function IDs preserved: - channel::whatsapp::webhook HTTP trigger: POST /webhook/whatsapp Auth env: WHATSAPP_TOKEN, WHATSAPP_PHONE_ID (resolved via vault::get fallback to env var). Continues #41 phase 3 channel migration.
Replaces src/channels/teams.ts (121 LOC) with workers/channel-teams/ (~310 LOC Rust + tests). Inbound webhook + outbound Microsoft Bot Framework reply API with OAuth client_credentials token exchange. Function IDs preserved: - channel::teams::webhook HTTP trigger: POST /webhook/teams Auth env: TEAMS_APP_ID, TEAMS_APP_PASSWORD (resolved via vault::get fallback to env var). Also folds in deferred cleanup of channels-signal.test.ts (whatsapp describe block removed; missed in prior commit due to git rename detection). Continues #41 phase 3 channel migration.
Replaces src/channels/webex.ts (96 LOC) with workers/channel-webex/ (~290 LOC Rust + tests). Inbound webhook + outbound Cisco Webex REST API via reqwest. Function IDs preserved: - channel::webex::webhook HTTP trigger: POST /webhook/webex Auth env: WEBEX_TOKEN (resolved via vault::get fallback to env var). Continues #41 phase 3 channel migration.
Replaces src/channels/signal.ts (107 LOC) with workers/channel-signal/ (~310 LOC Rust + tests). Inbound webhook + outbound REST bridge to signal-cli-rest-api (no Node-only deps required — pure HTTP). Function IDs preserved: - channel::signal::webhook HTTP trigger: POST /webhook/signal Auth env: SIGNAL_API_URL, SIGNAL_PHONE (resolved via vault::get fallback to env var). Group routing via envelope.dataMessage.groupInfo.groupId. Outbound respects 10s send timeout (mirrors AbortController behavior from TS). Empty agent reply still returns 500 to surface upstream failures, matching TS port. Continues #41 phase 3 channel migration.
rohitg00
force-pushed
the
feat/port-channels-batch-2
branch
from
0d4fc49 to
893a60d
Compare
| "workers/channel-email", | ||
| "workers/channel-signal", |
There was a problem hiding this comment.
🔴 Cargo.toml accidentally removes browser and code-agent from workspace members
workers/browser and workers/code-agent were workspace members before this PR (confirmed at merge base b813556), and their Cargo.toml files still exist on disk. The new channel-email and channel-signal entries replaced them instead of being added alongside them. This means cargo build / cargo test will no longer compile these two crates as part of the workspace, silently breaking their builds.
| "workers/channel-email", | |
| "workers/channel-signal", | |
| "workers/browser", | |
| "workers/channel-email", | |
| "workers/channel-signal", | |
| "workers/channel-slack", | |
| "workers/channel-teams", | |
| "workers/channel-webex", | |
| "workers/channel-whatsapp", | |
| "workers/code-agent", |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (6)
workers/channel-email/src/main.rs (2)
102-107:⚠️ Potential issue | 🟠 MajorSkip the SMTP send when
agent::chatreturns no content.
replycan be empty or whitespace-only here, and we still send it as a reply email. That turns a no-answer model response into a blank user-visible email.Suggested fix
let reply = chat.get("content").and_then(|v| v.as_str()).unwrap_or(""); let reply_subject = format!("Re: {subject}"); - if let Err(e) = send_mail(from, &reply_subject, reply).await { - tracing::error!(to = %from, error = %e, "failed to send email reply"); + if !reply.trim().is_empty() { + if let Err(e) = send_mail(from, &reply_subject, reply).await { + tracing::error!(to = %from, error = %e, "failed to send email reply"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-email/src/main.rs` around lines 102 - 107, The code sends an email even when agent::chat produced no meaningful content; update the logic around chat.get("content"), reply, reply_subject and send_mail so that after computing reply (from chat.get("content").and_then(|v| v.as_str()).unwrap_or("")) you check if reply.trim().is_empty() and if so skip calling send_mail (and optionally emit a debug/trace log) instead of sending a blank email; ensure you still construct reply_subject only when you will send the mail or move that construction after the emptiness check.
28-37:⚠️ Potential issue | 🟠 MajorUse vault-or-env resolution for all
SMTP_*settings.This worker still reads SMTP config only from process env. In vault-backed deployments, email replies will stay unconfigured while the other adapters resolve secrets via vault first and fall back to env only when needed.
Also applies to: 52-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-email/src/main.rs` around lines 28 - 37, The build_transport function currently reads SMTP_HOST, SMTP_PORT, SMTP_SECURE, SMTP_USER, and SMTP_PASS directly from std::env; change it to use the same vault-or-env resolution used elsewhere (i.e., the project's secret resolver helper) so each SMTP_* key is fetched from Vault first and falls back to env when missing; update the lookups for host, port, secure, user, and pass in build_transport (and the same pattern used in the block around lines 52-55) to call the vault-or-env helper, parse port/secure as before, and propagate errors as Result<AsyncSmtpTransport<Tokio1Executor>, IIIError>.src/__tests__/channels-matrix.test.ts (1)
148-152:⚠️ Potential issue | 🟡 MinorFail fast when no Matrix PUT call is found.
Both tests conditionally assert with
if (putCall), so they can pass even when no PUT request happened.Proposed fix
const putCall = mockFetch.mock.calls.find(c => c[1]?.method === "PUT"); - if (putCall) { - const body = JSON.parse(putCall[1].body); - expect(body.msgtype).toBe("m.text"); - } + expect(putCall).toBeDefined(); + const body = JSON.parse(putCall![1].body); + expect(body.msgtype).toBe("m.text");const putCall = mockFetch.mock.calls.find(c => c[1]?.method === "PUT"); - if (putCall) { - expect(putCall[1].headers.Authorization).toBe("Bearer test-matrix-token"); - } + expect(putCall).toBeDefined(); + expect(putCall![1].headers.Authorization).toBe("Bearer test-matrix-token");Also applies to: 177-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/channels-matrix.test.ts` around lines 148 - 152, The tests currently swallow missing Matrix PUT requests by guarding with "if (putCall)"; change these to fail fast by asserting the PUT call exists before using it (e.g., replace the conditional with expect(putCall).toBeDefined() or throw an explicit error) so subsequent code parsing putCall[1].body is safe; apply this fix to the occurrences referencing mockFetch.mock.calls.find(...) and putCall at both the block using putCall and the similar block around lines 177-180 so the test fails when no PUT request was made.workers/channel-webex/src/main.rs (1)
61-66:⚠️ Potential issue | 🟡 MinorSkip the newline delimiter when rolling over to the next chunk.
When
split_atcomes fromrfind('\n'),remaining[split_at..]keeps the delimiter, so the next chunk can begin with a leading newline. Advance tosplit_at + 1for newline-based splits.Suggested fix
- let split_at = match window.rfind('\n') { - Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, - _ => cutoff, - }; + let (split_at, skip_delimiter) = match window.rfind('\n') { + Some(idx) if window[..idx].chars().count() > max_len / 2 => (idx, 1), + _ => (cutoff, 0), + }; chunks.push(remaining[..split_at].to_string()); - remaining = remaining[split_at..].to_string(); + remaining = remaining[split_at + skip_delimiter..].to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-webex/src/main.rs` around lines 61 - 66, The current rollover keeps the newline delimiter because when split_at was chosen by window.rfind('\n') you slice remaining from remaining[split_at..]; change the next-slice start to skip the delimiter: compute a next_start = if remaining.as_bytes()[split_at] == b'\n' { split_at + 1 } else { split_at } (or detect Some(idx) and use idx + 1) and use remaining[..split_at].to_string() for the chunk and remaining[next_start..].to_string() for the new remaining; update the code around split_at/window/remaining/chunks accordingly so newline-based splits advance past the '\n'.workers/channel-teams/src/main.rs (1)
61-66:⚠️ Potential issue | 🟡 MinorSkip the newline when
rfind('\n')chooses the split point.
remaining = remaining[split_at..].to_string()keeps the delimiter, so the next chunk can start with\n.✂️ Proposed fix
- let split_at = match window.rfind('\n') { - Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, - _ => cutoff, - }; + let (split_at, skip_delimiter) = match window.rfind('\n') { + Some(idx) if window[..idx].chars().count() > max_len / 2 => (idx, 1), + _ => (cutoff, 0), + }; chunks.push(remaining[..split_at].to_string()); - remaining = remaining[split_at..].to_string(); + remaining = remaining[split_at + skip_delimiter..].to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-teams/src/main.rs` around lines 61 - 66, When splitting at the newline found by window.rfind('\n'), the code currently keeps the '\n' in the next chunk by doing remaining = remaining[split_at..].to_string(); change this to skip the delimiter: after computing split_at, compute next_start = if remaining.as_bytes()[split_at] == b'\n' { split_at + 1 } else { split_at }; push remaining[..split_at].to_string() as before and set remaining = remaining[next_start..].to_string(); this ensures the newline is not preserved at the start of the next chunk (refer to window.rfind, split_at, chunks, remaining).workers/channel-whatsapp/src/main.rs (1)
63-68:⚠️ Potential issue | 🟡 MinorSkip the consumed newline when advancing
remaining.When
split_atcomes fromrfind('\n'), the next chunk still starts with\n, so outbound messages can begin with a blank line. This was already flagged earlier and still looks unresolved.🔧 Suggested fix
let split_at = match window.rfind('\n') { Some(idx) if window[..idx].chars().count() > max_len / 2 => idx, _ => cutoff, }; chunks.push(remaining[..split_at].to_string()); - remaining = remaining[split_at..].to_string(); + remaining = if remaining.as_bytes().get(split_at) == Some(&b'\n') { + remaining[split_at + 1..].to_string() + } else { + remaining[split_at..].to_string() + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-whatsapp/src/main.rs` around lines 63 - 68, The current logic pushes remaining[..split_at] and then advances remaining = remaining[split_at..], which leaves a leading '\n' when split_at came from window.rfind('\n'); update the advancement to skip the consumed newline: when split_at indexes a '\n' (the case from rfind('\n')), set remaining = remaining[split_at + 1..].to_string() (otherwise keep remaining = remaining[split_at..].to_string()); adjust the logic around the variables window, split_at, remaining, and chunks so that chunks.push still uses remaining[..split_at].to_string() but the next remaining does not start with a newline.
🧹 Nitpick comments (4)
workers/channel-email/src/main.rs (1)
166-216: Add handler-level tests for the actual webhook control flow.These tests only check local string formatting and field extraction, so they won't catch regressions in dispatch behavior like blank-reply sends or config resolution. A couple of
handle_webhooktests around “no content => no send” and “valid content => send attempt” would give this worker much better coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-email/src/main.rs` around lines 166 - 216, Add handler-level tests that exercise the actual webhook flow by invoking handle_webhook with controlled inputs and a mocked dispatcher/sender: create one test that calls handle_webhook with a body missing "from" or "text" and asserts the mock dispatcher's dispatch/send_email method is NOT called, and another test that calls handle_webhook with valid "from","to","text" and asserts the mock dispatcher's dispatch/send_email IS called once with expected args; implement the mock via the same trait/type your production code uses (e.g., Dispatcher or EmailProvider) and inject it into handle_webhook (or the handler context/config) so the tests verify dispatch behavior and config resolution rather than just string formatting.src/__tests__/channels-matrix.test.ts (3)
63-176: Use shared test helpers +call(id, input)helper instead of repeated direct handler invocations.Repeated direct
handlers[...]calls make setup verbose and drift from the expected shared test pattern.As per coding guidelines: "Test structure: Provide
call(id, input)helper to invoke registered handlers" and "Use shared test helpers fromsrc/__tests__/helpers.tsincluding KV mock and request builders".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/channels-matrix.test.ts` around lines 63 - 176, Tests call the handler directly via handlers["channel::matrix::webhook"] repeatedly which duplicates setup and violates the project test pattern; replace these direct invocations with the shared test helper call(id, input) (from src/__tests__/helpers.ts) and the common KV/request builders so each spec uses call("channel::matrix::webhook", { body: ... }) and the shared mocks (mockTrigger, mockFetch, mockTriggerVoid) instead of calling handlers[...] directly to keep tests consistent and DRY.
26-30: Align shared-module mocks with test convention.This file mocks
@agentos/shared/utilsonly; the test structure guideline expects mocking sharedutils.js,logger.js,metrics.js, anderrors.jsfor consistency and isolation.As per coding guidelines: "Test structure: Mock shared modules
utils.js,logger.js,metrics.js,errors.js".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/channels-matrix.test.ts` around lines 26 - 30, The test currently only mocks `@agentos/shared/utils` (httpOk, splitMessage, resolveAgent); update the test mocks to also stub shared logger.js, metrics.js, and errors.js per the test structure guideline by either expanding the existing vi.mock to include those modules or adding separate vi.mock calls for "logger.js" (e.g., stub info/error/debug methods), "metrics.js" (e.g., stub increment/record methods), and "errors.js" (e.g., stub createError or AppError export) so that all shared-module side effects are isolated; ensure the mocked symbols match usage in the test (e.g., info/error, increment, createError) and return no-op implementations or simple spies.
1-1: Drop file-level suppression comment.
//@ts-nocheck`` weakens test safety and also violates the no-comments rule forsrc/**.Proposed minimal change
-// `@ts-nocheck` import { describe, it, expect, vi, beforeEach, beforeAll } from "vitest";As per coding guidelines: "
src/**/*.{ts,tsx,js,jsx}: Code should be self-documenting; do not use code comments".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/channels-matrix.test.ts` at line 1, Remove the file-level "// `@ts-nocheck`" from src/__tests__/channels-matrix.test.ts and fix any resulting TypeScript errors by correcting types and adding proper type annotations in the test (e.g., for mocks, fixtures, function arguments, and return values) rather than reintroducing suppressive comments; run the test type-check (tsc or IDE) to locate offending symbols and update their declarations/usages so the file is type-safe and compliant with the no-comments rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workers/channel-email/src/main.rs`:
- Around line 105-106: The error log in the send_mail failure branch is
currently logging the raw sender email via "to = %from", exposing PII; update
the handler in main.rs (the block around send_mail(...).await and
tracing::error!) to stop logging the plaintext "from" value — either remove the
"to" field, replace it with a non-identifying placeholder, or log a
deterministic hash/redaction of the address (compute a short hash or mask
local-part) and include only the error and context in tracing::error!(error =
%e, "failed to send email reply"). Ensure the change is applied to the send_mail
error branch so no raw email appears in logs.
In `@workers/channel-signal/src/main.rs`:
- Around line 176-204: The current code logs send_message failures but still
returns Ok(json!({ "status_code": 200, "body": { "ok": true } })), so callers
get a success even when send_message (the call to send_message(client, &api_url,
&phone, &source, reply, group_id.as_deref()).await) fails; change the control
flow so that when that call returns Err(e) you propagate or return an error
response (e.g., return Err(e.into()) or return Ok(json!({ "status_code":
<non-200>, "body": { "ok": false, "error": format!("{}", e) } }))) instead of
continuing to spawn the audit and returning a 200; keep the tracing::error! log
but ensure the function returns early on send_message failure (use the same
send_message, channel_key, and agent_id symbols to locate the code).
In `@workers/channel-teams/src/main.rs`:
- Line 248: The reqwest client is created with reqwest::Client::new() which
leaves get_token and send_message calls without timeouts; replace creation of
the shared client (the variable client) with a reqwest::Client built via
reqwest::Client::builder() and set sensible connect_timeout and overall request
timeout (e.g., Duration::from_secs) so both get_token and send_message inherit
those limits; update any usages to keep the shared client and ensure the builder
call replaces the existing reqwest::Client::new() instantiation.
- Around line 150-172: The handler currently trusts incoming Teams activity
fields (conversation_id, service_url) leading to session hijack and token
leakage; add authentication and validation: implement Bot Framework JWT
validation for incoming requests (create a verify_teams_jwt(token,
expected_app_id) function and call it before using activity), reject requests
that fail, and only build sessionId (format!("teams:{conversation_id}")) and
call the Teams API with bearer_auth after successful validation; as a minimum,
validate service_url against a curated list or pattern of known Bot Framework
endpoints (or a configured allowed_service_urls set) and reject any request with
unknown serviceUrl before using service_url or conversation_id.
In `@workers/channel-webex/src/main.rs`:
- Around line 82-83: In handle_webhook, don't treat non-2xx HTTP responses from
the Webex fetch (the resp variable) as a benign "no message" by returning
Ok(None); instead propagate an error so the caller won't ack the webhook: change
the branch that currently does `if !resp.status().is_success() { return
Ok(None); }` to return an Err with context containing resp.status() (and ideally
body/text) so callers can surface failures (expired creds, 429/5xx) while
keeping Ok(None) only for the true "no text" case.
- Around line 144-183: The webhook handler extracts person_id but never ignores
messages from the bot itself; before calling
resolve_agent/iii.trigger("agent::chat") add a guard that fetches the bot's own
person ID (via the Webex /v1/people/me using the existing webex_token and
client, or from a cached value) and if person_id == bot_person_id return early
with the same Ok(json!({ "status_code": 200, "body": { "ok": true } }))
response; place this check after obtaining webex_token and person_id and before
calling resolve_agent/send_message so self-posted messages won’t trigger
agent::chat or sending replies.
In `@workers/channel-whatsapp/src/main.rs`:
- Around line 152-164: The code currently propagates failures from
iii.trigger(TriggerRequest { ... }) into an Err via map_err(IIIError::Handler),
making webhook responses depend on the downstream agent::chat call; change this
to swallow/log the error and proceed so delivery success isn't tied to agent
failures: replace the map_err chain on iii.trigger(...) (reference: iii.trigger,
TriggerRequest, IIIError::Handler, and the chat variable) with logic that awaits
the trigger and on Err logs the error (including context like agent_id/from) and
continues (e.g., set chat to None or a default) instead of returning Err,
ensuring the HTTP handler returns success regardless of agent::chat outcome.
- Around line 170-171: The error log currently prints the user identifier
variable `from` directly in the `tracing::error!` call inside the `if let Err(e)
= send_message(client, &token, &phone_id, &from, reply).await` block; replace
that direct exposure by computing a redacted or hashed representation (e.g.,
mask digits or compute a SHA256/hex snippet) into a new local variable (e.g.,
`redacted_from` or `from_hash`) and log that variable instead in the
`tracing::error!` invocation so the `send_message` failure still records the
recipient context without leaking the raw PII.
- Around line 216-220: The registration currently creates only an HTTP POST
trigger via iii.register_trigger with RegisterTriggerInput for function_id
"channel::whatsapp::webhook" and config { "http_method": "POST", "api_path":
"webhook/whatsapp" }, which will fail Meta's subscription verification that
sends a GET with mode=subscribe, hub.verify_token and hub.challenge; update the
trigger to accept GET as well (either change config to allow both methods or add
a second RegisterTriggerInput with "http_method": "GET" for the same api_path)
and ensure the webhook handler (channel::whatsapp::webhook) checks for
mode=subscribe and matching verify_token and returns hub.challenge when
verified.
- Around line 117-170: Add verification of the X-Hub-Signature-256 header before
trusting the webhook body: extract headers via req.get("headers").and_then(|h|
h.get("x-hub-signature-256")), get rawBody via req.get("rawBody"), fetch the app
secret (e.g. with get_secret(iii, "WHATSAPP_APP_SECRET")), compute
HMAC-SHA256(rawBody, app_secret) and compare it to the header using a
constant-time comparison (mirror the Slack signature logic in
workers/channel-slack's validation code) and return early (OK 200) if
verification fails; place this check at the top of the handler before extracting
message/from, resolve_agent, or calling iii.trigger/send_message so forged
requests are rejected.
---
Duplicate comments:
In `@src/__tests__/channels-matrix.test.ts`:
- Around line 148-152: The tests currently swallow missing Matrix PUT requests
by guarding with "if (putCall)"; change these to fail fast by asserting the PUT
call exists before using it (e.g., replace the conditional with
expect(putCall).toBeDefined() or throw an explicit error) so subsequent code
parsing putCall[1].body is safe; apply this fix to the occurrences referencing
mockFetch.mock.calls.find(...) and putCall at both the block using putCall and
the similar block around lines 177-180 so the test fails when no PUT request was
made.
In `@workers/channel-email/src/main.rs`:
- Around line 102-107: The code sends an email even when agent::chat produced no
meaningful content; update the logic around chat.get("content"), reply,
reply_subject and send_mail so that after computing reply (from
chat.get("content").and_then(|v| v.as_str()).unwrap_or("")) you check if
reply.trim().is_empty() and if so skip calling send_mail (and optionally emit a
debug/trace log) instead of sending a blank email; ensure you still construct
reply_subject only when you will send the mail or move that construction after
the emptiness check.
- Around line 28-37: The build_transport function currently reads SMTP_HOST,
SMTP_PORT, SMTP_SECURE, SMTP_USER, and SMTP_PASS directly from std::env; change
it to use the same vault-or-env resolution used elsewhere (i.e., the project's
secret resolver helper) so each SMTP_* key is fetched from Vault first and falls
back to env when missing; update the lookups for host, port, secure, user, and
pass in build_transport (and the same pattern used in the block around lines
52-55) to call the vault-or-env helper, parse port/secure as before, and
propagate errors as Result<AsyncSmtpTransport<Tokio1Executor>, IIIError>.
In `@workers/channel-teams/src/main.rs`:
- Around line 61-66: When splitting at the newline found by window.rfind('\n'),
the code currently keeps the '\n' in the next chunk by doing remaining =
remaining[split_at..].to_string(); change this to skip the delimiter: after
computing split_at, compute next_start = if remaining.as_bytes()[split_at] ==
b'\n' { split_at + 1 } else { split_at }; push remaining[..split_at].to_string()
as before and set remaining = remaining[next_start..].to_string(); this ensures
the newline is not preserved at the start of the next chunk (refer to
window.rfind, split_at, chunks, remaining).
In `@workers/channel-webex/src/main.rs`:
- Around line 61-66: The current rollover keeps the newline delimiter because
when split_at was chosen by window.rfind('\n') you slice remaining from
remaining[split_at..]; change the next-slice start to skip the delimiter:
compute a next_start = if remaining.as_bytes()[split_at] == b'\n' { split_at + 1
} else { split_at } (or detect Some(idx) and use idx + 1) and use
remaining[..split_at].to_string() for the chunk and
remaining[next_start..].to_string() for the new remaining; update the code
around split_at/window/remaining/chunks accordingly so newline-based splits
advance past the '\n'.
In `@workers/channel-whatsapp/src/main.rs`:
- Around line 63-68: The current logic pushes remaining[..split_at] and then
advances remaining = remaining[split_at..], which leaves a leading '\n' when
split_at came from window.rfind('\n'); update the advancement to skip the
consumed newline: when split_at indexes a '\n' (the case from rfind('\n')), set
remaining = remaining[split_at + 1..].to_string() (otherwise keep remaining =
remaining[split_at..].to_string()); adjust the logic around the variables
window, split_at, remaining, and chunks so that chunks.push still uses
remaining[..split_at].to_string() but the next remaining does not start with a
newline.
---
Nitpick comments:
In `@src/__tests__/channels-matrix.test.ts`:
- Around line 63-176: Tests call the handler directly via
handlers["channel::matrix::webhook"] repeatedly which duplicates setup and
violates the project test pattern; replace these direct invocations with the
shared test helper call(id, input) (from src/__tests__/helpers.ts) and the
common KV/request builders so each spec uses call("channel::matrix::webhook", {
body: ... }) and the shared mocks (mockTrigger, mockFetch, mockTriggerVoid)
instead of calling handlers[...] directly to keep tests consistent and DRY.
- Around line 26-30: The test currently only mocks `@agentos/shared/utils`
(httpOk, splitMessage, resolveAgent); update the test mocks to also stub shared
logger.js, metrics.js, and errors.js per the test structure guideline by either
expanding the existing vi.mock to include those modules or adding separate
vi.mock calls for "logger.js" (e.g., stub info/error/debug methods),
"metrics.js" (e.g., stub increment/record methods), and "errors.js" (e.g., stub
createError or AppError export) so that all shared-module side effects are
isolated; ensure the mocked symbols match usage in the test (e.g., info/error,
increment, createError) and return no-op implementations or simple spies.
- Line 1: Remove the file-level "// `@ts-nocheck`" from
src/__tests__/channels-matrix.test.ts and fix any resulting TypeScript errors by
correcting types and adding proper type annotations in the test (e.g., for
mocks, fixtures, function arguments, and return values) rather than
reintroducing suppressive comments; run the test type-check (tsc or IDE) to
locate offending symbols and update their declarations/usages so the file is
type-safe and compliant with the no-comments rule.
In `@workers/channel-email/src/main.rs`:
- Around line 166-216: Add handler-level tests that exercise the actual webhook
flow by invoking handle_webhook with controlled inputs and a mocked
dispatcher/sender: create one test that calls handle_webhook with a body missing
"from" or "text" and asserts the mock dispatcher's dispatch/send_email method is
NOT called, and another test that calls handle_webhook with valid
"from","to","text" and asserts the mock dispatcher's dispatch/send_email IS
called once with expected args; implement the mock via the same trait/type your
production code uses (e.g., Dispatcher or EmailProvider) and inject it into
handle_webhook (or the handler context/config) so the tests verify dispatch
behavior and config resolution rather than just string formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f73ca61-ee73-40a0-8bbc-8ef4fdcac5bd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomlsrc/__tests__/channels-bluesky.test.tssrc/__tests__/channels-matrix.test.tssrc/__tests__/channels-mattermost.test.tssrc/__tests__/channels-teams.test.tssrc/__tests__/channels-webex.test.tssrc/__tests__/channels-whatsapp-signal.test.tssrc/channels/email.tssrc/channels/signal.tssrc/channels/teams.tssrc/channels/webex.tssrc/channels/whatsapp.tsworkers/channel-email/Cargo.tomlworkers/channel-email/iii.worker.yamlworkers/channel-email/src/main.rsworkers/channel-signal/Cargo.tomlworkers/channel-signal/iii.worker.yamlworkers/channel-signal/src/main.rsworkers/channel-teams/Cargo.tomlworkers/channel-teams/iii.worker.yamlworkers/channel-teams/src/main.rsworkers/channel-webex/Cargo.tomlworkers/channel-webex/iii.worker.yamlworkers/channel-webex/src/main.rsworkers/channel-whatsapp/Cargo.tomlworkers/channel-whatsapp/iii.worker.yamlworkers/channel-whatsapp/src/main.rs
💤 Files with no reviewable changes (7)
- src/channels/teams.ts
- src/tests/channels-mattermost.test.ts
- src/channels/email.ts
- src/channels/whatsapp.ts
- src/channels/webex.ts
- src/channels/signal.ts
- src/tests/channels-whatsapp-signal.test.ts
✅ Files skipped from review due to trivial changes (7)
- workers/channel-signal/iii.worker.yaml
- workers/channel-teams/iii.worker.yaml
- workers/channel-webex/Cargo.toml
- workers/channel-whatsapp/iii.worker.yaml
- workers/channel-signal/Cargo.toml
- workers/channel-webex/iii.worker.yaml
- src/tests/channels-teams.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- workers/channel-email/iii.worker.yaml
- workers/channel-whatsapp/Cargo.toml
- workers/channel-email/Cargo.toml
- workers/channel-teams/Cargo.toml
- Cargo.toml
- src/tests/channels-bluesky.test.ts
- src/tests/channels-webex.test.ts
| if let Err(e) = send_mail(from, &reply_subject, reply).await { | ||
| tracing::error!(to = %from, error = %e, "failed to send email reply"); |
There was a problem hiding this comment.
Don't log the sender's raw email address on SMTP failures.
to = %from puts user PII directly into application logs. Please redact, hash, or drop that field and keep the error log non-identifying.
Suggested fix
- tracing::error!(to = %from, error = %e, "failed to send email reply");
+ tracing::error!(error = %e, "failed to send email reply");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Err(e) = send_mail(from, &reply_subject, reply).await { | |
| tracing::error!(to = %from, error = %e, "failed to send email reply"); | |
| if let Err(e) = send_mail(from, &reply_subject, reply).await { | |
| tracing::error!(error = %e, "failed to send email reply"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-email/src/main.rs` around lines 105 - 106, The error log in
the send_mail failure branch is currently logging the raw sender email via "to =
%from", exposing PII; update the handler in main.rs (the block around
send_mail(...).await and tracing::error!) to stop logging the plaintext "from"
value — either remove the "to" field, replace it with a non-identifying
placeholder, or log a deterministic hash/redaction of the address (compute a
short hash or mask local-part) and include only the error and context in
tracing::error!(error = %e, "failed to send email reply"). Ensure the change is
applied to the send_mail error branch so no raw email appears in logs.
| if let Err(e) = send_message(client, &api_url, &phone, &source, reply, group_id.as_deref()).await | ||
| { | ||
| tracing::error!(channel_key = %channel_key, error = %e, "failed to send Signal reply"); | ||
| } | ||
|
|
||
| let audit_iii = iii.clone(); | ||
| let source_for_audit = source.clone(); | ||
| let group_for_audit = group_id.clone(); | ||
| let agent_for_audit = agent_id.clone(); | ||
| tokio::spawn(async move { | ||
| let _ = audit_iii | ||
| .trigger(TriggerRequest { | ||
| function_id: "security::audit".to_string(), | ||
| payload: json!({ | ||
| "type": "channel_message", | ||
| "agentId": agent_for_audit, | ||
| "detail": { | ||
| "channel": "signal", | ||
| "source": source_for_audit, | ||
| "groupId": group_for_audit | ||
| }, | ||
| }), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) | ||
| .await; | ||
| }); | ||
|
|
||
| Ok(json!({ "status_code": 200, "body": { "ok": true } })) |
There was a problem hiding this comment.
Don’t return { ok: true } after Signal delivery fails.
Any missing config, timeout, or non-2xx from Signal lands in this branch, but Line 204 still acknowledges success. That makes reply delivery failures invisible to the caller and drops the response on the floor.
💡 Minimal fix
- if let Err(e) = send_message(client, &api_url, &phone, &source, reply, group_id.as_deref()).await
- {
- tracing::error!(channel_key = %channel_key, error = %e, "failed to send Signal reply");
- }
+ if let Err(e) =
+ send_message(client, &api_url, &phone, &source, reply, group_id.as_deref()).await
+ {
+ tracing::error!(channel_key = %channel_key, error = %e, "failed to send Signal reply");
+ return Ok(json!({
+ "status_code": 200,
+ "body": { "ok": false, "error": "Failed to deliver Signal reply" }
+ }));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Err(e) = send_message(client, &api_url, &phone, &source, reply, group_id.as_deref()).await | |
| { | |
| tracing::error!(channel_key = %channel_key, error = %e, "failed to send Signal reply"); | |
| } | |
| let audit_iii = iii.clone(); | |
| let source_for_audit = source.clone(); | |
| let group_for_audit = group_id.clone(); | |
| let agent_for_audit = agent_id.clone(); | |
| tokio::spawn(async move { | |
| let _ = audit_iii | |
| .trigger(TriggerRequest { | |
| function_id: "security::audit".to_string(), | |
| payload: json!({ | |
| "type": "channel_message", | |
| "agentId": agent_for_audit, | |
| "detail": { | |
| "channel": "signal", | |
| "source": source_for_audit, | |
| "groupId": group_for_audit | |
| }, | |
| }), | |
| action: None, | |
| timeout_ms: None, | |
| }) | |
| .await; | |
| }); | |
| Ok(json!({ "status_code": 200, "body": { "ok": true } })) | |
| if let Err(e) = | |
| send_message(client, &api_url, &phone, &source, reply, group_id.as_deref()).await | |
| { | |
| tracing::error!(channel_key = %channel_key, error = %e, "failed to send Signal reply"); | |
| return Ok(json!({ | |
| "status_code": 200, | |
| "body": { "ok": false, "error": "Failed to deliver Signal reply" } | |
| })); | |
| } | |
| let audit_iii = iii.clone(); | |
| let source_for_audit = source.clone(); | |
| let group_for_audit = group_id.clone(); | |
| let agent_for_audit = agent_id.clone(); | |
| tokio::spawn(async move { | |
| let _ = audit_iii | |
| .trigger(TriggerRequest { | |
| function_id: "security::audit".to_string(), | |
| payload: json!({ | |
| "type": "channel_message", | |
| "agentId": agent_for_audit, | |
| "detail": { | |
| "channel": "signal", | |
| "source": source_for_audit, | |
| "groupId": group_for_audit | |
| }, | |
| }), | |
| action: None, | |
| timeout_ms: None, | |
| }) | |
| .await; | |
| }); | |
| Ok(json!({ "status_code": 200, "body": { "ok": true } })) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-signal/src/main.rs` around lines 176 - 204, The current code
logs send_message failures but still returns Ok(json!({ "status_code": 200,
"body": { "ok": true } })), so callers get a success even when send_message (the
call to send_message(client, &api_url, &phone, &source, reply,
group_id.as_deref()).await) fails; change the control flow so that when that
call returns Err(e) you propagate or return an error response (e.g., return
Err(e.into()) or return Ok(json!({ "status_code": <non-200>, "body": { "ok":
false, "error": format!("{}", e) } }))) instead of continuing to spawn the audit
and returning a 200; keep the tracing::error! log but ensure the function
returns early on send_message failure (use the same send_message, channel_key,
and agent_id symbols to locate the code).
| let conversation_id = activity | ||
| .get("conversation") | ||
| .and_then(|c| c.get("id")) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
| let text = activity.get("text").and_then(|v| v.as_str()).unwrap_or("").to_string(); | ||
| let user_id = activity | ||
| .get("from") | ||
| .and_then(|f| f.get("id")) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
| let service_url = activity | ||
| .get("serviceUrl") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
| let activity_id = activity | ||
| .get("id") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n workers/channel-teams/src/main.rs | head -250Repository: iii-experimental/agentos
Length of output: 10008
🏁 Script executed:
cat -n workers/channel-teams/src/main.rs | sed -n '240,280p'Repository: iii-experimental/agentos
Length of output: 1672
🏁 Script executed:
cat -n workers/channel-teams/src/main.rs | sed -n '274,350p'Repository: iii-experimental/agentos
Length of output: 2072
🏁 Script executed:
find . -type f -name "*.rs" -o -name "*.md" | xargs grep -l "Bot Framework\|serviceUrl.*validation\|request.*auth" 2>/dev/null | head -20Repository: iii-experimental/agentos
Length of output: 140
🏁 Script executed:
cd workers/channel-teams && find . -type f -name "*.md" -o -name "*.toml" | xargs cat 2>/dev/nullRepository: iii-experimental/agentos
Length of output: 537
🏁 Script executed:
rg "request.*signature|verify.*jwt|bot.*framework.*auth" workers/channel-teams/ --type rsRepository: iii-experimental/agentos
Length of output: 95
🏁 Script executed:
rg "request.*signature|verify.*jwt|bot.*framework.*auth" workers/channel-teams/Repository: iii-experimental/agentos
Length of output: 197
🏁 Script executed:
find . -type f -name "*.rs" -path "*/iii-sdk/*" | head -5 && \
rg "Bot.*Framework\|serviceUrl.*validation\|request.*signature" . --max-count=5 2>/dev/null | grep -v "Binary"Repository: iii-experimental/agentos
Length of output: 50
🏁 Script executed:
find . -type f \( -name "*.md" -o -name "*.rs" \) | xargs grep -l "webhook\|trigger.*http" 2>/dev/null | head -10Repository: iii-experimental/agentos
Length of output: 395
🏁 Script executed:
cat ./workers/channel-slack/src/main.rs | head -150 | tail -50Repository: iii-experimental/agentos
Length of output: 1901
🏁 Script executed:
cat ./workers/channel-slack/src/main.rs | sed -n '1,100p'Repository: iii-experimental/agentos
Length of output: 3403
🏁 Script executed:
grep -n "verify.*signature\|request.*auth" ./workers/channel-teams/src/main.rsRepository: iii-experimental/agentos
Length of output: 50
🏁 Script executed:
cat ./workers/channel-slack/src/main.rs | sed -n '200,250p'Repository: iii-experimental/agentos
Length of output: 1875
Reject unauthenticated Teams activity payloads and validate serviceUrl before sending authenticated requests.
The Teams webhook accepts unvalidated POST requests. Unlike the Slack worker which verifies request signatures with verify_slack_signature(), this endpoint has no upstream authentication. An attacker can submit arbitrary activity with a malicious serviceUrl and conversation_id. This causes two problems:
- Session hijacking:
conversation_idis used unvalidated insessionId: format!("teams:{conversation_id}")(line 182), allowing the attacker to route agent responses to attacker-controlled sessions. - Token leakage:
service_urlis used unvalidated as the destination for authenticated requests (line 199 → line 110-112, line 118bearer_auth), allowing the attacker to receive the bot's Microsoft Bot Framework access token.
Implement request signature validation per Microsoft Bot Framework authentication standards, or at minimum validate that serviceUrl is a known Bot Framework service endpoint before sending authenticated requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-teams/src/main.rs` around lines 150 - 172, The handler
currently trusts incoming Teams activity fields (conversation_id, service_url)
leading to session hijack and token leakage; add authentication and validation:
implement Bot Framework JWT validation for incoming requests (create a
verify_teams_jwt(token, expected_app_id) function and call it before using
activity), reject requests that fail, and only build sessionId
(format!("teams:{conversation_id}")) and call the Teams API with bearer_auth
after successful validation; as a minimum, validate service_url against a
curated list or pattern of known Bot Framework endpoints (or a configured
allowed_service_urls set) and reject any request with unknown serviceUrl before
using service_url or conversation_id.
| let ws_url = | ||
| std::env::var("III_WS_URL").unwrap_or_else(|_| "ws://localhost:49134".to_string()); | ||
| let iii = register_worker(&ws_url, InitOptions::default()); | ||
| let client = reqwest::Client::new(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read around line 248
cat -n workers/channel-teams/src/main.rs | sed -n '240,260p'Repository: iii-experimental/agentos
Length of output: 975
🏁 Script executed:
# Let's also get an overview of the file structure
wc -l workers/channel-teams/src/main.rsRepository: iii-experimental/agentos
Length of output: 106
🏁 Script executed:
# Search for all reqwest::Client usages in the file
rg "reqwest::Client" workers/channel-teams/src/main.rs -n -A 3 -B 1Repository: iii-experimental/agentos
Length of output: 925
🏁 Script executed:
# Read the get_token function
cat -n workers/channel-teams/src/main.rs | sed -n '71,100p'Repository: iii-experimental/agentos
Length of output: 1435
🏁 Script executed:
# Read the send_message function
cat -n workers/channel-teams/src/main.rs | sed -n '102,135p'Repository: iii-experimental/agentos
Length of output: 1380
🏁 Script executed:
# Check the beginning of the file for imports
cat -n workers/channel-teams/src/main.rs | head -40Repository: iii-experimental/agentos
Length of output: 1687
🏁 Script executed:
# Search for any timeout or Duration usage
rg "timeout|Duration" workers/channel-teams/src/main.rs -nRepository: iii-experimental/agentos
Length of output: 206
Bind the outbound HTTP calls with timeouts.
reqwest::Client::new() leaves token exchange (in get_token) and message sends (in send_message) without a request timeout. If Microsoft Teams endpoints are slow or unresponsive, webhook handling will stall indefinitely. Build the shared client with explicit connect and request timeouts.
⏱️ Proposed fix
+use std::time::Duration;
...
- let client = reqwest::Client::new();
+ let client = reqwest::Client::builder()
+ .connect_timeout(Duration::from_secs(5))
+ .timeout(Duration::from_secs(15))
+ .build()?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let client = reqwest::Client::new(); | |
| let client = reqwest::Client::builder() | |
| .connect_timeout(Duration::from_secs(5)) | |
| .timeout(Duration::from_secs(15)) | |
| .build()?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-teams/src/main.rs` at line 248, The reqwest client is created
with reqwest::Client::new() which leaves get_token and send_message calls
without timeouts; replace creation of the shared client (the variable client)
with a reqwest::Client built via reqwest::Client::builder() and set sensible
connect_timeout and overall request timeout (e.g., Duration::from_secs) so both
get_token and send_message inherit those limits; update any usages to keep the
shared client and ensure the builder call replaces the existing
reqwest::Client::new() instantiation.
| if !resp.status().is_success() { | ||
| return Ok(None); |
There was a problem hiding this comment.
Propagate non-2xx fetch responses instead of treating them as “no message.”
Ok(None) here makes handle_webhook acknowledge the webhook with 200 for expired credentials, rate limits, and Webex 5xxs, so inbound messages get silently dropped. Return an error for unexpected statuses and keep None only for a real “no text” case.
Suggested fix
if !resp.status().is_success() {
- return Ok(None);
+ let status = resp.status();
+ let body = resp.text().await.unwrap_or_default();
+ return Err(IIIError::Handler(format!(
+ "Webex fetch failed ({status}): {}",
+ body.chars().take(300).collect::<String>()
+ )));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !resp.status().is_success() { | |
| return Ok(None); | |
| if !resp.status().is_success() { | |
| let status = resp.status(); | |
| let body = resp.text().await.unwrap_or_default(); | |
| return Err(IIIError::Handler(format!( | |
| "Webex fetch failed ({status}): {}", | |
| body.chars().take(300).collect::<String>() | |
| ))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-webex/src/main.rs` around lines 82 - 83, In handle_webhook,
don't treat non-2xx HTTP responses from the Webex fetch (the resp variable) as a
benign "no message" by returning Ok(None); instead propagate an error so the
caller won't ack the webhook: change the branch that currently does `if
!resp.status().is_success() { return Ok(None); }` to return an Err with context
containing resp.status() (and ideally body/text) so callers can surface failures
(expired creds, 429/5xx) while keeping Ok(None) only for the true "no text"
case.
| let person_id = body | ||
| .pointer("/data/personId") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
|
|
||
| let webex_token = get_secret(iii, "WEBEX_TOKEN").await; | ||
| if webex_token.is_empty() { | ||
| return Ok(json!({ | ||
| "status_code": 500, | ||
| "body": { "error": "WEBEX_TOKEN not configured" } | ||
| })); | ||
| } | ||
|
|
||
| let text = match fetch_message(client, &webex_token, &message_id).await? { | ||
| Some(t) if !t.is_empty() => t, | ||
| _ => return Ok(json!({ "status_code": 200, "body": { "ok": true } })), | ||
| }; | ||
|
|
||
| let agent_id = resolve_agent(iii, "webex", &room_id).await; | ||
|
|
||
| let chat = iii | ||
| .trigger(TriggerRequest { | ||
| function_id: "agent::chat".to_string(), | ||
| payload: json!({ | ||
| "agentId": agent_id, | ||
| "message": text, | ||
| "sessionId": format!("webex:{room_id}"), | ||
| }), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) | ||
| .await | ||
| .map_err(|e| IIIError::Handler(format!("agent::chat failed: {e}")))?; | ||
|
|
||
| let reply = chat.get("content").and_then(|v| v.as_str()).unwrap_or(""); | ||
| if !reply.is_empty() | ||
| && let Err(e) = send_message(client, &webex_token, &room_id, reply).await | ||
| { | ||
| tracing::error!(room = %room_id, error = %e, "failed to send Webex reply"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Cisco Webex send messages created webhook events for messages posted by the bot itself, and what payload field or API call should a bot use to identify and ignore its own messages?
💡 Result:
Yes, Cisco Webex sends messages:created webhook events for messages posted by the bot itself. The webhook payload's data field contains the message object, which includes a personId field identifying the sender. To identify and ignore its own messages, the bot should compare the data.personId from the webhook to its own person ID. To obtain the bot's own person ID, use the GET /v1/people/me API endpoint with the bot's access token.
Citations:
- 1: https://developer.webex.com/messaging/docs/api/guides/webhooks
- 2: https://developer-usgov.webex.com/docs/api/v1/messages/create-a-message
- 3: https://developer.webex.com/messaging/docs/api/v1/people/get-my-own-details
Add a guard to filter out messages from the bot itself to prevent recursion.
The personId is extracted but never used to filter messages. Cisco Webex sends messages:created webhook events for messages posted by the bot, so without a check, the handler will recurse into self-replies. Before triggering agent::chat, compare the personId from the webhook payload against the bot's own person ID (obtained via the /v1/people/me API endpoint). Return early if they match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-webex/src/main.rs` around lines 144 - 183, The webhook
handler extracts person_id but never ignores messages from the bot itself;
before calling resolve_agent/iii.trigger("agent::chat") add a guard that fetches
the bot's own person ID (via the Webex /v1/people/me using the existing
webex_token and client, or from a cached value) and if person_id ==
bot_person_id return early with the same Ok(json!({ "status_code": 200, "body":
{ "ok": true } })) response; place this check after obtaining webex_token and
person_id and before calling resolve_agent/send_message so self-posted messages
won’t trigger agent::chat or sending replies.
| let body = req.get("body").cloned().unwrap_or_else(|| req.clone()); | ||
|
|
||
| if body.get("object").and_then(|v| v.as_str()) != Some("whatsapp_business_account") { | ||
| return Ok(json!({ "status_code": 200, "body": { "ok": true } })); | ||
| } | ||
|
|
||
| let message = body | ||
| .get("entry") | ||
| .and_then(|e| e.get(0)) | ||
| .and_then(|e| e.get("changes")) | ||
| .and_then(|c| c.get(0)) | ||
| .and_then(|c| c.get("value")) | ||
| .and_then(|v| v.get("messages")) | ||
| .and_then(|m| m.get(0)); | ||
|
|
||
| let Some(message) = message else { | ||
| return Ok(json!({ "status_code": 200, "body": { "ok": true } })); | ||
| }; | ||
|
|
||
| let text = message | ||
| .get("text") | ||
| .and_then(|t| t.get("body")) | ||
| .and_then(|b| b.as_str()) | ||
| .unwrap_or(""); | ||
| if text.is_empty() { | ||
| return Ok(json!({ "status_code": 200, "body": { "ok": true } })); | ||
| } | ||
|
|
||
| let from = message | ||
| .get("from") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
| let agent_id = resolve_agent(iii, "whatsapp", &from).await; | ||
|
|
||
| let chat = iii | ||
| .trigger(TriggerRequest { | ||
| function_id: "agent::chat".to_string(), | ||
| payload: json!({ | ||
| "agentId": agent_id, | ||
| "message": text, | ||
| "sessionId": format!("whatsapp:{from}"), | ||
| }), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) | ||
| .await | ||
| .map_err(|e| IIIError::Handler(format!("agent::chat failed: {e}")))?; | ||
|
|
||
| let reply = chat.get("content").and_then(|v| v.as_str()).unwrap_or(""); | ||
| if !reply.is_empty() { | ||
| let token = get_secret(iii, "WHATSAPP_TOKEN").await; | ||
| let phone_id = get_secret(iii, "WHATSAPP_PHONE_ID").await; | ||
| if let Err(e) = send_message(client, &token, &phone_id, &from, reply).await { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Meta / WhatsApp Cloud API webhooks, should the callback validate the X-Hub-Signature-256 header against the raw request body before processing inbound messages?
💡 Result:
Yes, the callback should validate the X-Hub-Signature-256 header against the raw request body before processing inbound messages. This verifies the webhook originates from Meta's servers and prevents spoofed requests that could lead to unauthorized message processing.
Citations:
- 1: https://developers.facebook.com/docs/graph-api/webhooks/getting-started/#:~:text=In the App Dashboard, go,string in all Verification Requests.
- 2: https://github.com/theonlyhennygod/zeroclaw/issues/51
- 3: https://hookdeck.com/webhooks/platforms/guide-to-whatsapp-webhooks-features-and-best-practices
- 4: https://wa.expert/pages/whatsapp-webhook-guide.html
- 5: https://stackoverflow.com/questions/75422064/validate-x-hub-signature-256-meta-whatsapp-webhook-request
- 6: https://developers.facebook.com/community/threads/969752231235530/
🏁 Script executed:
cat -n workers/channel-whatsapp/src/main.rs | head -50Repository: iii-experimental/agentos
Length of output: 2110
🏁 Script executed:
wc -l workers/channel-whatsapp/src/main.rsRepository: iii-experimental/agentos
Length of output: 109
🏁 Script executed:
cat -n workers/channel-whatsapp/src/main.rs | head -150Repository: iii-experimental/agentos
Length of output: 5928
🏁 Script executed:
cat -n workers/channel-whatsapp/src/main.rs | sed -n '200,306p'Repository: iii-experimental/agentos
Length of output: 4141
🏁 Script executed:
cat -n workers/channel-whatsapp/src/main.rs | sed -n '150,210p'Repository: iii-experimental/agentos
Length of output: 2693
🏁 Script executed:
fd -t f "\.rs$" | xargs rg -l "RegisterTriggerInput|http.*trigger" | head -5Repository: iii-experimental/agentos
Length of output: 222
🏁 Script executed:
find . -type f -name "*.md" | xargs grep -l "X-Hub-Signature\|webhook.*header\|http.*header" 2>/dev/null | head -3Repository: iii-experimental/agentos
Length of output: 50
🏁 Script executed:
rg "get.*header|Header|X-Hub" --type rust | head -20Repository: iii-experimental/agentos
Length of output: 880
🏁 Script executed:
cat -n workers/channel-slack/src/main.rs | grep -A 20 "get.*headers"Repository: iii-experimental/agentos
Length of output: 961
🏁 Script executed:
cat -n workers/channel-telegram/src/main.rs | grep -A 20 "get.*headers"Repository: iii-experimental/agentos
Length of output: 851
🏁 Script executed:
cat -n workers/channel-slack/src/main.rs | sed -n '200,250p'Repository: iii-experimental/agentos
Length of output: 2232
🏁 Script executed:
rg "hmac|sha256|hash" workers/channel-whatsapp/src/main.rsRepository: iii-experimental/agentos
Length of output: 50
🏁 Script executed:
rg "verify.*signature" workers/channel-slack/src/main.rs -A 5Repository: iii-experimental/agentos
Length of output: 1492
🏁 Script executed:
web_search
Meta WhatsApp Cloud API X-Hub-Signature-256 HMAC SHA256 validation example codeRepository: iii-experimental/agentos
Length of output: 162
🌐 Web query:
Meta WhatsApp Cloud API X-Hub-Signature-256 HMAC SHA256 validation implementation example
💡 Result:
Meta's WhatsApp Cloud API sends webhook POST requests with an X-Hub-Signature-256 header containing an HMAC-SHA256 signature of the raw request body, using your app's App Secret (found in Meta App Dashboard > Settings > Basic > App Secret). Verify the signature before processing payloads to prevent spoofing. Key steps: 1. Capture the raw request body (before JSON parsing). 2. Extract signature from X-Hub-Signature-256 header (strip "sha256=" prefix). 3. Compute HMAC-SHA256 of raw body using App Secret. 4. Compare computed hash (hex) with header value using constant-time comparison. Handle GET requests for webhook verification: check hub.mode=subscribe and hub.verify_token, respond with hub.challenge. Examples: Node.js (Express): const crypto = require('crypto'); app.use(express.raw({type: 'application/json'})); // Capture raw body app.post('/webhook', (req, res) => { const signature = req.get('X-Hub-Signature-256'); const expected = 'sha256=' + crypto .createHmac('sha256', process.env.APP_SECRET) .update(req.body) .digest('hex'); if (signature !== expected) { return res.status(403).send('Invalid signature'); } // Process JSON.parse(req.body) res.sendStatus(200); }); Note: Some cases (Unicode/media) may require unicode escaping before hashing: function escapeUnicode(str) { return str.replace(/[\u00A0-\uffff]/gu, c => '\u' + c.charCodeAt(0).toString(16).padStart(4, '0')) .replace(///g, '\/'); } const escapedBody = escapeUnicode(req.body.toString); Python (Flask): import hmac import hashlib from flask import request @app.route('/webhook', methods=['POST']) def webhook: signature = request.headers.get('X-Hub-Signature-256', '').replace('sha256=', '') raw_body = request.get_data computed = hmac.new( APP_SECRET.encode('utf-8'), raw_body, hashlib.sha256 ).hexdigest if not hmac.compare_digest(signature, computed): return '', 403 # Process request.json return '', 200 Rust: use hmac::{Hmac, Mac}; use sha2::Sha256; fn verify_signature(secret: &str, body: &[u8], sig_header: &str) -> bool { let sig = sig_header.strip_prefix("sha256=").unwrap_or(""); let expected = hex::decode(sig).ok?; let mut mac = Hmac::::new_from_slice(secret.as_bytes).ok?; mac.update(body); mac.verify_slice(&expected).is_ok } Security notes: - Always verify in production. - Use raw body; JSON parsing changes bytes. - Constant-time compare prevents timing attacks. - Store App Secret securely (env vars). - Respond 200 quickly (<10s); Meta retries failures up to 7 days. Sources confirm consistent implementation across languages, emphasizing raw body and App Secret.
Citations:
- 1: https://hexdocs.pm/whatsapp_sdk/webhooks.html
- 2: https://wa.expert/pages/whatsapp-webhook-guide.html
- 3: How to use signature validation fbsamples/whatsapp-api-examples#13
- 4: https://github.com/theonlyhennygod/zeroclaw/issues/51
- 5: https://stackoverflow.com/questions/75422064/validate-x-hub-signature-256-meta-whatsapp-webhook-request
- 6: https://hookdeck.com/webhooks/platforms/guide-to-whatsapp-webhooks-features-and-best-practices
- 7: https://developers.facebook.com/docs/whatsapp/cloud-api/guides/set-up-webhooks/
- 8: https://developers.facebook.com/community/threads/969752231235530/
- 9: https://dualhook.com/docs/guide-nodejs-webhook
Add X-Hub-Signature-256 validation to prevent forged webhook requests.
This handler accepts any POST with WhatsApp-like JSON structure and uses the caller-supplied from field to route messages and trigger outbound replies. Meta's WhatsApp Cloud API signs all webhook notifications with an X-Hub-Signature-256 header (HMAC-SHA256 of the raw body using your App Secret), but this worker never verifies it. An attacker can POST a forged request to impersonate any sender, trigger agent::chat, and make the system message arbitrary phone numbers.
Extract req.get("headers").get("x-hub-signature-256") and req.get("rawBody"), then validate with constant-time HMAC comparison (similar to the existing Slack signature validation pattern in workers/channel-slack/src/main.rs lines 203–232).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-whatsapp/src/main.rs` around lines 117 - 170, Add
verification of the X-Hub-Signature-256 header before trusting the webhook body:
extract headers via req.get("headers").and_then(|h|
h.get("x-hub-signature-256")), get rawBody via req.get("rawBody"), fetch the app
secret (e.g. with get_secret(iii, "WHATSAPP_APP_SECRET")), compute
HMAC-SHA256(rawBody, app_secret) and compare it to the header using a
constant-time comparison (mirror the Slack signature logic in
workers/channel-slack's validation code) and return early (OK 200) if
verification fails; place this check at the top of the handler before extracting
message/from, resolve_agent, or calling iii.trigger/send_message so forged
requests are rejected.
| let chat = iii | ||
| .trigger(TriggerRequest { | ||
| function_id: "agent::chat".to_string(), | ||
| payload: json!({ | ||
| "agentId": agent_id, | ||
| "message": text, | ||
| "sessionId": format!("whatsapp:{from}"), | ||
| }), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) | ||
| .await | ||
| .map_err(|e| IIIError::Handler(format!("agent::chat failed: {e}")))?; |
There was a problem hiding this comment.
Don’t surface agent::chat failures as webhook failures.
Returning Err here makes delivery success depend on an internal downstream call. If the sender retries non-2xx deliveries, this path can process the same inbound message multiple times, and there’s no message-id dedupe in this worker to contain that.
🛠️ Suggested change
- let chat = iii
- .trigger(TriggerRequest {
- function_id: "agent::chat".to_string(),
- payload: json!({
- "agentId": agent_id,
- "message": text,
- "sessionId": format!("whatsapp:{from}"),
- }),
- action: None,
- timeout_ms: None,
- })
- .await
- .map_err(|e| IIIError::Handler(format!("agent::chat failed: {e}")))?;
+ let chat = match iii
+ .trigger(TriggerRequest {
+ function_id: "agent::chat".to_string(),
+ payload: json!({
+ "agentId": agent_id,
+ "message": text,
+ "sessionId": format!("whatsapp:{from}"),
+ }),
+ action: None,
+ timeout_ms: None,
+ })
+ .await
+ {
+ Ok(chat) => chat,
+ Err(e) => {
+ tracing::error!(error = %e, "agent::chat failed");
+ return Ok(json!({ "status_code": 200, "body": { "ok": true } }));
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let chat = iii | |
| .trigger(TriggerRequest { | |
| function_id: "agent::chat".to_string(), | |
| payload: json!({ | |
| "agentId": agent_id, | |
| "message": text, | |
| "sessionId": format!("whatsapp:{from}"), | |
| }), | |
| action: None, | |
| timeout_ms: None, | |
| }) | |
| .await | |
| .map_err(|e| IIIError::Handler(format!("agent::chat failed: {e}")))?; | |
| let chat = match iii | |
| .trigger(TriggerRequest { | |
| function_id: "agent::chat".to_string(), | |
| payload: json!({ | |
| "agentId": agent_id, | |
| "message": text, | |
| "sessionId": format!("whatsapp:{from}"), | |
| }), | |
| action: None, | |
| timeout_ms: None, | |
| }) | |
| .await | |
| { | |
| Ok(chat) => chat, | |
| Err(e) => { | |
| tracing::error!(error = %e, "agent::chat failed"); | |
| return Ok(json!({ "status_code": 200, "body": { "ok": true } })); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-whatsapp/src/main.rs` around lines 152 - 164, The code
currently propagates failures from iii.trigger(TriggerRequest { ... }) into an
Err via map_err(IIIError::Handler), making webhook responses depend on the
downstream agent::chat call; change this to swallow/log the error and proceed so
delivery success isn't tied to agent failures: replace the map_err chain on
iii.trigger(...) (reference: iii.trigger, TriggerRequest, IIIError::Handler, and
the chat variable) with logic that awaits the trigger and on Err logs the error
(including context like agent_id/from) and continues (e.g., set chat to None or
a default) instead of returning Err, ensuring the HTTP handler returns success
regardless of agent::chat outcome.
| if let Err(e) = send_message(client, &token, &phone_id, &from, reply).await { | ||
| tracing::error!(to = %from, error = %e, "failed to send WhatsApp reply"); |
There was a problem hiding this comment.
Redact the recipient phone number in error logs.
from is a user identifier. Logging it verbatim on send failures leaks PII into downstream log sinks and retention systems; mask or hash it before emitting.
🛡️ Suggested fix
let token = get_secret(iii, "WHATSAPP_TOKEN").await;
let phone_id = get_secret(iii, "WHATSAPP_PHONE_ID").await;
if let Err(e) = send_message(client, &token, &phone_id, &from, reply).await {
- tracing::error!(to = %from, error = %e, "failed to send WhatsApp reply");
+ let redacted_to = match from.char_indices().nth_back(3) {
+ Some((idx, _)) => format!("***{}", &from[idx..]),
+ None => "***".to_string(),
+ };
+ tracing::error!(to = %redacted_to, error = %e, "failed to send WhatsApp reply");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-whatsapp/src/main.rs` around lines 170 - 171, The error log
currently prints the user identifier variable `from` directly in the
`tracing::error!` call inside the `if let Err(e) = send_message(client, &token,
&phone_id, &from, reply).await` block; replace that direct exposure by computing
a redacted or hashed representation (e.g., mask digits or compute a SHA256/hex
snippet) into a new local variable (e.g., `redacted_from` or `from_hash`) and
log that variable instead in the `tracing::error!` invocation so the
`send_message` failure still records the recipient context without leaking the
raw PII.
| iii.register_trigger(RegisterTriggerInput { | ||
| trigger_type: "http".to_string(), | ||
| function_id: "channel::whatsapp::webhook".to_string(), | ||
| config: json!({ "http_method": "POST", "api_path": "webhook/whatsapp" }), | ||
| metadata: None, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Meta Graph / WhatsApp webhook setup, does the callback URL need a GET verification endpoint that validates hub.verify_token and echoes hub.challenge, or can a POST-only endpoint be used?
💡 Result:
The callback URL for Meta Graph / WhatsApp webhooks requires a GET verification endpoint that validates the hub.verify_token query parameter and echoes back the hub.challenge value in the response body with HTTP 200 status. A POST-only endpoint cannot be used because Meta sends a GET request during webhook configuration/subscription to verify ownership of the endpoint. If the GET request is not handled correctly (mode=subscribe, matching verify_token, return challenge), verification fails and webhooks will not be activated.
Citations:
- 1: https://wa.expert/pages/whatsapp-webhook-guide.html
- 2: https://developers.facebook.com/docs/graph-api/webhooks/getting-started/
- 3: https://hookdeck.com/webhooks/platforms/guide-to-whatsapp-webhooks-features-and-best-practices
- 4: https://developers.facebook.com/docs/whatsapp/flows/reference/flowswebhooks/#:~:text=In your App Dashboard, find,be sending the events to.
🏁 Script executed:
# First, let's examine the whatsapp worker directory structure
fd --type f . workers/channel-whatsapp/src/ | head -20Repository: iii-experimental/agentos
Length of output: 105
🏁 Script executed:
# Search for GET handling or verification logic in the whatsapp worker
rg -i "get|verify|challenge|hub\." workers/channel-whatsapp/src/ -A 3 -B 1Repository: iii-experimental/agentos
Length of output: 5838
🏁 Script executed:
# Search for "webhook/whatsapp" references across the codebase
rg "webhook/whatsapp" --type rust -B 3 -A 3Repository: iii-experimental/agentos
Length of output: 596
🏁 Script executed:
# Search for any mentions of hub.verify_token or hub.challenge
rg "hub\.(verify_token|challenge)" -A 2 -B 2Repository: iii-experimental/agentos
Length of output: 1400
🏁 Script executed:
# Check the main.rs file structure around lines 216-220 and nearby
cat -n workers/channel-whatsapp/src/main.rs | sed -n '200,250p'Repository: iii-experimental/agentos
Length of output: 2054
Add GET endpoint to handle Meta's webhook verification flow.
The webhook only registers a POST trigger, but Meta's subscription requires a GET request to validate hub.verify_token and echo hub.challenge. Without handling this verification request (mode=subscribe, matching verify_token, returning challenge), the webhook subscription will fail. Add a GET handler on the same path or update the trigger handler to support both GET and POST.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-whatsapp/src/main.rs` around lines 216 - 220, The
registration currently creates only an HTTP POST trigger via
iii.register_trigger with RegisterTriggerInput for function_id
"channel::whatsapp::webhook" and config { "http_method": "POST", "api_path":
"webhook/whatsapp" }, which will fail Meta's subscription verification that
sends a GET with mode=subscribe, hub.verify_token and hub.challenge; update the
trigger to accept GET as well (either change config to allow both methods or add
a second RegisterTriggerInput with "http_method": "GET" for the same api_path)
and ensure the webhook handler (channel::whatsapp::webhook) checks for
mode=subscribe and matching verify_token and returns hub.challenge when
verified.
Webhook security - LinkedIn: HMAC-SHA256 challengeCode GET handshake, X-LI-Signature verification on POST, notificationId dedup via state::*, process all elements (not just first), 10s HTTP timeout - Twitch: EventSub HMAC-SHA256 (Message-Id + Timestamp + raw body) signature verification, sender_id now uses TWITCH_BOT_USER_ID instead of broadcaster_id - WhatsApp: X-Hub-Signature-256 verification (HMAC-SHA256 of raw body via WHATSAPP_APP_SECRET) plus GET hub.challenge endpoint with constant-time verify_token compare - Teams: validate serviceUrl against Bot Framework host suffixes (with TEAMS_ALLOWED_SERVICE_URLS escape hatch) before sending authenticated outbound replies; reqwest connect/total timeouts (5s/15s) - Webex: cache bot personId from /people/me and drop self-posted webhook events to break reply loops; propagate non-2xx fetch errors instead of silently acking Reliability - Reddit: invalidate token cache on 401 and retry once with refresh - Signal: empty agent reply now returns 200 (with `ok:false`) so the bridge does not retry; send_message failures are surfaced to the caller rather than logged-and-acked - WhatsApp: agent::chat failures no longer fail the webhook response; caller-supplied phone numbers redacted (sha256:8) in error logs - Email: SMTP_* secrets resolved through vault::get with env fallback, recipient address redacted in send-failure logs, blank/whitespace agent replies no longer trigger empty outgoing email Hygiene - Newline-aware split_message no longer leaves a leading '\n' on the next chunk in teams/webex/whatsapp - Workspace MSRV pinned to 1.88 (let-chains stabilization) and the reddit/linkedin/twitch crates wired into workspace members
1 participant
Summary
Ports 5 channel adapters from TypeScript to Rust narrow workers, continuing #41 phase 3. Follows the gold-standard layout from
workers/channel-slack/(#46).Per-channel summary
channel-email
channel::email::webhookPOST /webhook/emailSMTP_HOST,SMTP_PORT,SMTP_SECURE,SMTP_USER,SMTP_PASSlettre = "0.11"(rustls-tls, async tokio1)channel-whatsapp
channel::whatsapp::webhookPOST /webhook/whatsappWHATSAPP_TOKEN,WHATSAPP_PHONE_ID(vault::get fallback to env var)v18.0/{phoneId}/messagesvia reqwestchannel-teams
channel::teams::webhookPOST /webhook/teamsTEAMS_APP_ID,TEAMS_APP_PASSWORD(vault::get fallback to env var)v3/conversations/{id}/activitieswith OAuth client_credentials token exchange againstlogin.microsoftonline.comchannel-webex
channel::webex::webhookPOST /webhook/webexWEBEX_TOKEN(vault::get fallback to env var)v1/messagesvia reqwest; inbound webhook payload only contains the message ID, so we fetch the body viaGET /v1/messages/{id}firstchannel-signal
channel::signal::webhookPOST /webhook/signalSIGNAL_API_URL,SIGNAL_PHONE(vault::get fallback to env var)signal-cli-rest-apiv2/send(no Node-only deps; pure HTTP)Deferred features
#[test].wiremockintegration could be added in a follow-up if useful.src/channels/for these — ports skipped, can be added when those channels are introduced.Cross-cutting
iii-sdk = "=0.11.4-next.4"(workspace dep, unchanged).http_method/api_pathper the iii engine convention.vault::getwith env-var fallback, mirroringcreateSecretGetterfrom the TS port.agent::chat,state::get,security::audit) useiii.trigger(TriggerRequest { ... }).security::auditcalls are fire-and-forget viatokio::spawn, matching the TSTriggerAction.Void()semantics.splitMessagereimplemented in Rust as UTF-8/codepoint-safe.Test plan
cargo check -p agentos-channel-{email,whatsapp,teams,webex,signal}cleancargo test -p agentos-channel-{email,whatsapp,teams,webex,signal}passes (35 tests total)cargo clippy -p agentos-channel-{email,whatsapp,teams,webex,signal} --all-targets -- -D warningscleancargo test --workspace --releasepassesnpx vitest runpasses for un-ported channels (1263 / 1263)Summary by CodeRabbit
Chores
Tests