fix(channel:discord): handle inbound image attachments as robust [IMAGE] markers#1923
Merged
theonlyhennygod merged 1 commit intozeroclaw-labs:mainfrom Feb 28, 2026
Conversation
|
Thanks for contributing to ZeroClaw. For faster review, please ensure:
See |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Discord channel src/channels/discord.rs |
Add image-marker emission [IMAGE:<url>] for image-like attachments; consolidate detection into private helpers is_image_attachment() and has_image_extension() (MIME-first, extension fallback); continue inlining text/* attachments and skipping others; tests added/updated for multiple image-detection scenarios. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- fix(discord): recover text attachments when content type is missing #1871 — Related changes to
process_attachments()and attachment classification helpers insrc/channels/discord.rs. - [supersede #1872] feat(discord): forward inbound image attachments as markers #1980 — Similar modifications to image-marker handling and attachment detection logic in
src/channels/discord.rs. - feat(discord): forward inbound image attachments as markers #1872 — Overlapping PR introducing
[IMAGE:<url>]emission for Discord attachments and related detection logic.
Suggested labels
channel: discord
Suggested reviewers
- chumyin
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The PR title clearly describes the main change: adding image attachment handling for Discord inbound messages as [IMAGE] markers. |
| Linked Issues check | ✅ Passed | The code changes fully address issue #1825 requirements: image attachments emit [IMAGE:] markers, text attachments are inlined, non-image/non-text types are skipped, and tests cover single/multiple images with edge cases. |
| Out of Scope Changes check | ✅ Passed | All changes are strictly scoped to src/channels/discord.rs inbound attachment handling with targeted tests; no unrelated modifications to providers, config, schema, or outbound behavior. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Description check | ✅ Passed | The pull request description comprehensively follows the template with all required sections completed, including summary, labels, change metadata, validation evidence, security impact, and rollback plan. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Comment @coderabbitai help to get the list of available commands and usage tips.
github-actions
bot
added
size: S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/channels/discord.rs (1)
145-160:⚠️ Potential issue | 🟡 MinorNormalize text MIME checks to avoid case-sensitive misses.
At Line 159,
ct.starts_with("text/")is case-sensitive, but MIME tokens are case-insensitive. AText/Plaincontent type would skip inlining unexpectedly.Suggested fix
- } else if ct.starts_with("text/") { + } else if ct + .split(';') + .next() + .unwrap_or("") + .trim() + .to_ascii_lowercase() + .starts_with("text/") + {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/channels/discord.rs` around lines 145 - 160, The MIME-type check uses the variable ct and currently calls ct.starts_with("text/") which is case-sensitive; change the check in the attachment handling block (the code that defines ct and later branches on is_image_attachment and ct.starts_with) to perform a case-insensitive comparison (e.g., normalize ct with ct.to_ascii_lowercase() or otherwise compare ASCII-case-insensitively) before calling starts_with("text/") so text/* types like "Text/Plain" are correctly detected and inlined.
🤖 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/channels/discord.rs`:
- Around line 135-138: Update the doc comment above the attachment-processing
code in src/channels/discord.rs to reflect that unsupported attachment types are
not silent: instead of saying they are "silently skipped", state that they are
skipped and a debug log is emitted for unsupported types (matching the runtime
behavior in the attachment handling code that logs at debug level). Mention that
image/* are passed as [IMAGE:<url>] and text/* are fetched/inlined, and clarify
that all other types are skipped with a debug-level log entry for visibility.
- Around line 1613-1659: Add a new async test (e.g.,
process_attachments_emits_multiple_image_markers_in_order) that builds a vector
of two or more image attachments (mix content_type-present and
filename-extension-only cases), calls process_attachments(&attachments,
&client).await, and asserts the output contains the expected image markers for
each URL and that the first marker appears before the second (check ordering via
string positions), and also assert the total number of "[IMAGE:" markers equals
the number of image attachments; reference process_attachments and
is_image_attachment when locating logic to validate ordering/join behavior.
---
Outside diff comments:
In `@src/channels/discord.rs`:
- Around line 145-160: The MIME-type check uses the variable ct and currently
calls ct.starts_with("text/") which is case-sensitive; change the check in the
attachment handling block (the code that defines ct and later branches on
is_image_attachment and ct.starts_with) to perform a case-insensitive comparison
(e.g., normalize ct with ct.to_ascii_lowercase() or otherwise compare
ASCII-case-insensitively) before calling starts_with("text/") so text/* types
like "Text/Plain" are correctly detected and inlined.
Comment on lines
+135
to
+138
| /// `image/*` attachments are passed through as `[IMAGE:<url>]` markers so the | ||
| /// downstream model/tooling can reason about visual inputs. | ||
| /// `text/*` attachments are fetched and inlined. | ||
| /// All other types are silently skipped. Fetch errors are logged as warnings. |
Contributor
There was a problem hiding this comment.
Align docs with runtime behavior for unsupported attachments.
The comment says unsupported types are “silently skipped,” but the code emits a debug log at Line 174-Line 178. Please update the doc text to avoid ambiguity.
As per coding guidelines "Never silently broaden permissions/capabilities; document fallback behavior when fallback is intentional and safe."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/discord.rs` around lines 135 - 138, Update the doc comment above
the attachment-processing code in src/channels/discord.rs to reflect that
unsupported attachment types are not silent: instead of saying they are
"silently skipped", state that they are skipped and a debug log is emitted for
unsupported types (matching the runtime behavior in the attachment handling code
that logs at debug level). Mention that image/* are passed as [IMAGE:<url>] and
text/* are fetched/inlined, and clarify that all other types are skipped with a
debug-level log entry for visibility.
src/channels/discord.rs
Outdated
Comment on lines
+1613
to
+1659
| #[tokio::test] | ||
| async fn process_attachments_emits_image_marker_for_image_content_type() { | ||
| let client = reqwest::Client::new(); | ||
| let attachments = vec![serde_json::json!({ | ||
| "url": "https://cdn.discordapp.com/attachments/123/456/photo.png", | ||
| "filename": "photo.png", | ||
| "content_type": "image/png" | ||
| })]; | ||
| let result = process_attachments(&attachments, &client).await; | ||
| assert_eq!( | ||
| result, | ||
| "[IMAGE:https://cdn.discordapp.com/attachments/123/456/photo.png]" | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn process_attachments_emits_image_marker_from_filename_without_content_type() { | ||
| let client = reqwest::Client::new(); | ||
| let attachments = vec![serde_json::json!({ | ||
| "url": "https://cdn.discordapp.com/attachments/123/456/photo.jpeg?size=1024", | ||
| "filename": "photo.jpeg" | ||
| })]; | ||
| let result = process_attachments(&attachments, &client).await; | ||
| assert_eq!( | ||
| result, | ||
| "[IMAGE:https://cdn.discordapp.com/attachments/123/456/photo.jpeg?size=1024]" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_image_attachment_prefers_non_image_content_type_over_extension() { | ||
| assert!(!is_image_attachment( | ||
| "text/plain", | ||
| "photo.png", | ||
| "https://cdn.discordapp.com/attachments/123/456/photo.png" | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_image_attachment_allows_octet_stream_extension_fallback() { | ||
| assert!(is_image_attachment( | ||
| "application/octet-stream", | ||
| "photo.png", | ||
| "https://cdn.discordapp.com/attachments/123/456/photo.png" | ||
| )); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Add a multi-image marker test to lock ordering/join behavior.
Current tests cover single-image and helper cases, but there’s no assertion for multiple image attachments being emitted in order with the expected delimiter.
Suggested test addition
+ #[tokio::test]
+ async fn process_attachments_emits_multiple_image_markers_in_order() {
+ let client = reqwest::Client::new();
+ let attachments = vec![
+ serde_json::json!({
+ "url": "https://cdn.discordapp.com/attachments/123/1/a.png",
+ "filename": "a.png",
+ "content_type": "image/png"
+ }),
+ serde_json::json!({
+ "url": "https://cdn.discordapp.com/attachments/123/2/b.jpg",
+ "filename": "b.jpg",
+ "content_type": "image/jpeg"
+ }),
+ ];
+
+ let result = process_attachments(&attachments, &client).await;
+ assert_eq!(
+ result,
+ "[IMAGE:https://cdn.discordapp.com/attachments/123/1/a.png]\n---\n[IMAGE:https://cdn.discordapp.com/attachments/123/2/b.jpg]"
+ );
+ }📝 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.
Suggested change
| #[tokio::test] | |
| async fn process_attachments_emits_image_marker_for_image_content_type() { | |
| let client = reqwest::Client::new(); | |
| let attachments = vec![serde_json::json!({ | |
| "url": "https://cdn.discordapp.com/attachments/123/456/photo.png", | |
| "filename": "photo.png", | |
| "content_type": "image/png" | |
| })]; | |
| let result = process_attachments(&attachments, &client).await; | |
| assert_eq!( | |
| result, | |
| "[IMAGE:https://cdn.discordapp.com/attachments/123/456/photo.png]" | |
| ); | |
| } | |
| #[tokio::test] | |
| async fn process_attachments_emits_image_marker_from_filename_without_content_type() { | |
| let client = reqwest::Client::new(); | |
| let attachments = vec![serde_json::json!({ | |
| "url": "https://cdn.discordapp.com/attachments/123/456/photo.jpeg?size=1024", | |
| "filename": "photo.jpeg" | |
| })]; | |
| let result = process_attachments(&attachments, &client).await; | |
| assert_eq!( | |
| result, | |
| "[IMAGE:https://cdn.discordapp.com/attachments/123/456/photo.jpeg?size=1024]" | |
| ); | |
| } | |
| #[test] | |
| fn is_image_attachment_prefers_non_image_content_type_over_extension() { | |
| assert!(!is_image_attachment( | |
| "text/plain", | |
| "photo.png", | |
| "https://cdn.discordapp.com/attachments/123/456/photo.png" | |
| )); | |
| } | |
| #[test] | |
| fn is_image_attachment_allows_octet_stream_extension_fallback() { | |
| assert!(is_image_attachment( | |
| "application/octet-stream", | |
| "photo.png", | |
| "https://cdn.discordapp.com/attachments/123/456/photo.png" | |
| )); | |
| } | |
| #[tokio::test] | |
| async fn process_attachments_emits_image_marker_for_image_content_type() { | |
| let client = reqwest::Client::new(); | |
| let attachments = vec![serde_json::json!({ | |
| "url": "https://cdn.discordapp.com/attachments/123/456/photo.png", | |
| "filename": "photo.png", | |
| "content_type": "image/png" | |
| })]; | |
| let result = process_attachments(&attachments, &client).await; | |
| assert_eq!( | |
| result, | |
| "[IMAGE:https://cdn.discordapp.com/attachments/123/456/photo.png]" | |
| ); | |
| } | |
| #[tokio::test] | |
| async fn process_attachments_emits_image_marker_from_filename_without_content_type() { | |
| let client = reqwest::Client::new(); | |
| let attachments = vec![serde_json::json!({ | |
| "url": "https://cdn.discordapp.com/attachments/123/456/photo.jpeg?size=1024", | |
| "filename": "photo.jpeg" | |
| })]; | |
| let result = process_attachments(&attachments, &client).await; | |
| assert_eq!( | |
| result, | |
| "[IMAGE:https://cdn.discordapp.com/attachments/123/456/photo.jpeg?size=1024]" | |
| ); | |
| } | |
| #[test] | |
| fn is_image_attachment_prefers_non_image_content_type_over_extension() { | |
| assert!(!is_image_attachment( | |
| "text/plain", | |
| "photo.png", | |
| "https://cdn.discordapp.com/attachments/123/456/photo.png" | |
| )); | |
| } | |
| #[test] | |
| fn is_image_attachment_allows_octet_stream_extension_fallback() { | |
| assert!(is_image_attachment( | |
| "application/octet-stream", | |
| "photo.png", | |
| "https://cdn.discordapp.com/attachments/123/456/photo.png" | |
| )); | |
| } | |
| #[tokio::test] | |
| async fn process_attachments_emits_multiple_image_markers_in_order() { | |
| let client = reqwest::Client::new(); | |
| let attachments = vec![ | |
| serde_json::json!({ | |
| "url": "https://cdn.discordapp.com/attachments/123/1/a.png", | |
| "filename": "a.png", | |
| "content_type": "image/png" | |
| }), | |
| serde_json::json!({ | |
| "url": "https://cdn.discordapp.com/attachments/123/2/b.jpg", | |
| "filename": "b.jpg", | |
| "content_type": "image/jpeg" | |
| }), | |
| ]; | |
| let result = process_attachments(&attachments, &client).await; | |
| assert_eq!( | |
| result, | |
| "[IMAGE:https://cdn.discordapp.com/attachments/123/1/a.png]\n---\n[IMAGE:https://cdn.discordapp.com/attachments/123/2/b.jpg]" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/discord.rs` around lines 1613 - 1659, Add a new async test
(e.g., process_attachments_emits_multiple_image_markers_in_order) that builds a
vector of two or more image attachments (mix content_type-present and
filename-extension-only cases), calls process_attachments(&attachments,
&client).await, and asserts the output contains the expected image markers for
each URL and that the first marker appears before the second (check ordering via
string positions), and also assert the total number of "[IMAGE:" markers equals
the number of image attachments; reference process_attachments and
is_image_attachment when locating logic to validate ordering/join behavior.
loydccc
force-pushed
the
fix/discord-image-markers-1825
branch
from
36c186a to
07b1214
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/channels/discord.rs (1)
1642-1649: Consider adding a URL-based extension fallback test.Current tests verify filename-based extension detection, but
is_image_attachmentalso falls back to the URL when filename has no extension. A targeted test would strengthen coverage for this path.Suggested test addition
+ #[test] + fn is_image_attachment_uses_url_extension_when_filename_has_none() { + assert!(is_image_attachment( + "", + "image", + "https://cdn.discordapp.com/attachments/123/456/image.png" + )); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/channels/discord.rs` around lines 1642 - 1649, Add a unit test for is_image_attachment that verifies the URL-based extension fallback: create a case where content_type is non-image (e.g., "text/plain"), filename has no extension or is empty (e.g., "photo"), and the URL ends with an image extension (e.g., "https://.../photo.png"); assert that is_image_attachment returns true for this input so the function's URL fallback path is covered. Reference the is_image_attachment function and add the new test alongside the existing tests in the same module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/channels/discord.rs`:
- Around line 1642-1649: Add a unit test for is_image_attachment that verifies
the URL-based extension fallback: create a case where content_type is non-image
(e.g., "text/plain"), filename has no extension or is empty (e.g., "photo"), and
the URL ends with an image extension (e.g., "https://.../photo.png"); assert
that is_image_attachment returns true for this input so the function's URL
fallback path is covered. Reference the is_image_attachment function and add the
new test alongside the existing tests in the same module.
Author
|
Maintainers, could you please provide one active Linear issue key for this PR (format: RMN-xxx / CDV-xxx / COM-xxx)?
|
github-actions
bot
added
ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/i18n/el/actions-source-policy.md (1)
1-80:⚠️ Potential issue | 🔴 CriticalCritical: Incomplete i18n coverage and undocumented scope bundling.
This PR updates
actions-source-policy.mdwith theSwatinem/rust-cache@*allowlist entry across multiple locales but incomplete: zh-CN, ja, ru, and fr locales are missing the update. Per coding guidelines, all supported locales (en, zh-CN, ja, ru, fr, vi, el) must be updated in the same PR.Additionally, the PR bundles unrelated concerns: Actions allowlist documentation changes (across 7 locales) plus Discord channel code changes (
src/channels/discord.rs,python/zeroclaw_tools/integrations/discord_bot.py). Per the pull request template and PR governance guidelines, if multiple concerns are intentionally bundled, the coupling rationale must be documented explicitly in the PR summary. Verify that the PR description clearly justifies this bundling; if undocumented, unbundle the changes into separate PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/i18n/el/actions-source-policy.md` around lines 1 - 80, The PR added Swatinem/rust-cache@* to the Greek allowlist (docs/i18n/el/actions-source-policy.md) but failed to update the same entry in the other locale files (docs/i18n/zh-CN/actions-source-policy.md, docs/i18n/ja/actions-source-policy.md, docs/i18n/ru/actions-source-policy.md, docs/i18n/fr/actions-source-policy.md); add the identical allowlist line (`Swatinem/rust-cache@*`) to each missing locale file so all supported locales (en, zh-CN, ja, ru, fr, vi, el) are consistent, and then address the bundled change concern by either updating the PR description to explicitly document the coupling rationale for including Actions policy changes plus Discord-related edits (src/channels/discord.rs and python/zeroclaw_tools/integrations/discord_bot.py) or split the Discord code changes into a separate PR per the governance template.
🧹 Nitpick comments (2)
src/tools/browser_open.rs (1)
148-174: Error message implies separation that doesn't match implementation.The error message states "default browser launchers; Brave compatibility fallback also failed" suggesting two distinct phases, but all launchers are tried in a single unified loop. Consider either:
- Updating the message to reflect the single-loop behavior, or
- Separating the loop into default launchers and Brave fallback (matching macOS/Windows structure).
💡 Option 1: Simplify the error message
// TODO(compat): remove Brave fallback commands (brave-browser/brave) once default launcher coverage is validated. anyhow::bail!( - "Failed to open URL with default browser launchers; Brave compatibility fallback also failed. Last error: {last_error}" + "Failed to open URL: no browser launcher succeeded (tried xdg-open, gio, sensible-browser, brave-browser, brave). Last error: {last_error}" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser_open.rs` around lines 148 - 174, The bail message claims separate "default browser launchers" and "Brave compatibility fallback" phases, but the code uses a single unified loop over the commands (see the command list, the loop that builds tokio::process::Command, the last_error variable, and the anyhow::bail! call); either update the bail text to reflect a single unified attempt (e.g., "Failed to open URL with any browser launcher. Last error: {last_error}") or refactor into two loops: one iterating a default_launchers array and a second iterating brave_fallbacks (preserving the same command/status handling and last_error accumulation) and then adjust the bail message to accurately report which phase failed..github/workflows/docs-deploy.yml (1)
213-285: Consider extracting shared HTML template generation.The preview/deploy jobs duplicate almost the same heredoc HTML. A shared script/template would reduce drift and simplify future edits.
Also applies to: 321-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docs-deploy.yml around lines 213 - 285, The workflow duplicates large heredoc HTML blocks that generate site/index.html and site/docs/index.html; extract the shared HTML generation into a reusable script or function (e.g., a shell script like generate_docs_index.sh or a workflow composite/action) and replace the heredocs in both the preview and deploy jobs with calls to that script. Implement the script to accept parameters (title, subtitle/file links, output path) or read a small template + variables so both invocations reuse the same template logic, then update the workflow steps that currently run the heredocs (the site/index.html and site/docs/index.html generation steps) to call the new script with appropriate arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci-build-fast.yml:
- Line 55: Update the repository documentation and PR notes to reflect the new
GitHub Action source: the workflow reference
Swatinem/rust-cache@779680da715d629ac1d338a641029a2f4372abb5 in
.github/workflows/ci-build-fast.yml must be added to the Actions allowlist
impact section in the PR description and the official allowlist document
docs/actions-source-policy.md; edit docs/actions-source-policy.md to include the
new action source entry (name, owner, and pinned commit) and add a short note in
the PR template or PR description indicating the allowlist change for reviewers
to approve.
In @.github/workflows/ci-run.yml:
- Line 24: The workflow uses broad runner selector values like the runs-on array
[self-hosted, Linux, X64] which can schedule sensitive jobs onto any matching
self-hosted node; update each runs-on entry that currently equals [self-hosted,
Linux, X64] (and other similar occurrences) to include a dedicated trust-tier
label (for example add a stable label such as "trusted" or "tier:trusted") so
only approved runners pick up these privileged jobs, and ensure the new label is
added consistently across all occurrences referenced in the workflow.
In @.github/workflows/docs-deploy.yml:
- Line 358: Replace the hardcoded branch name "main" in the deployment
provenance paragraph (the element with class "muted" that currently reads
'Automatically deployed from <code>main</code> via
<code>.github/workflows/docs-deploy.yml</code>.') with the pipeline output
variable so the message uses the actual source ref; specifically update that
paragraph to render the expression ${{ needs.docs-quality.outputs.source_ref }}
instead of the literal "main" so rollbacks or manual refs display correctly.
In @.github/workflows/pub-docker-img.yml:
- Line 42: The workflow now introduces/replaces external actions
docker/setup-buildx-action and docker/build-push-action; update the project
Actions source policy by adding these sources to docs/actions-source-policy.md
and document the allowlist impact in the PR notes, and also ensure any other
occurrences of the same actions in the workflow are captured in the same doc/PR
notes update.
In @.github/workflows/pub-release.yml:
- Line 177: The workflow uses a custom runner label "racknerd" in the
pub-release workflow matrix which may not exist; verify that the "racknerd"
runner label is registered and has required tools/permissions for MUSL and
Android NDK jobs, and if it is not available replace those matrix entries with
known-good labels (e.g., [self-hosted, Linux, X64] or other existing org runner
labels) so lanes do not queue indefinitely; update the matrix entries where
"racknerd" is used to the validated label set and confirm successful runs.
In @.github/workflows/sec-codeql.yml:
- Around line 60-68: The workflow step "Ensure native build tools"
unconditionally calls apt-get which fails on non-APT self-hosted runners; update
that step to first detect whether apt-get exists (e.g., test command -v apt-get)
and only run the SUDO/apt-get update and apt-get install lines when apt-get is
available, otherwise skip with a clear log message; keep the existing SUDO
detection (variable SUDO) and the install commands (build-essential pkg-config)
but wrap them behind the apt-get existence check so the job won’t hard-fail on
runners without APT.
In `@docs/actions-source-policy.md`:
- Line 26: Add the missing i18n updates to the zh-CN, ja, ru, and fr versions of
docs/actions-source-policy.md by inserting the allowlist entry
"Swatinem/rust-cache@*" in the same allowlist section where it appears in the
en/vi/el files, and append the 2026-02-26 history note (the same three-line
entry present in en/vi/el) to the history/changelog section so those locale
files match the English version; locate the allowlist section and history note
by searching for the string "Swatinem/rust-cache@*" and the 2026-02-26 history
entry and mirror the exact wording and placement from the updated locales.
In `@docs/i18n/vi/actions-source-policy.md`:
- Line 25: The Vietnamese docs added the `Swatinem/rust-cache@*` allowlist entry
and a 2026-02-26 history note in actions-source-policy.md but the same
runtime-contract change must be applied to the other locales; update the
corresponding files for Chinese (zh-CN), Japanese (ja), Russian (ru) and French
(fr) versions of actions-source-policy.md to include the exact allowlist entry
`Swatinem/rust-cache@*` and add the same 2026-02-26 history entry, then update
the PR description to mark i18n follow-through complete per the
docs/i18n-guide.md checklist (include which locales were updated).
In `@src/channels/mod.rs`:
- Line 728: There is a duplicate match arm for the string literal "/new" that
maps to ChannelRuntimeCommand::NewSession and is unreachable because "/new" is
already included in the combined arm that matches "/new" | "/clear"; remove the
redundant arm mapping `"/new" => Some(ChannelRuntimeCommand::NewSession)` from
the match expression (leave the existing combined arm intact) so the match has
no dead/unreachable branches.
In `@src/channels/whatsapp_web.rs`:
- Around line 660-664: The tracing::warn! call that currently logs sender_jid
and the full sender_candidates vector should be changed to avoid emitting
sensitive identifiers; update the warn in the code around
sender_jid/sender_candidates to log only non-sensitive metadata (e.g., number of
candidates via sender_candidates.len() and a brief source/type string) and keep
sender_jid redacted or removed, modify the tracing::warn! invocation accordingly
so it no longer prints the raw sender_candidates contents.
- Around line 282-285: The code is building phone tokens from a Jid by calling
sender.to_string(), which includes device IDs (e.g., "12345:7@s.whatsapp.net")
and leads normalize_phone_token to produce incorrect tokens; change the calls
that pass sender/to sender_alt into add_candidate to use the Jid.user field
(e.g., sender.user and alt.user) so normalize_phone_token receives only the
phone number portion; update the two places that call
add_candidate(Self::normalize_phone_token(&sender.to_string())) and
add_candidate(Self::normalize_phone_token(&alt.to_string())) to pass the
Jid.user string slice instead, ensuring allowlist matching uses the correct
phone token.
---
Outside diff comments:
In `@docs/i18n/el/actions-source-policy.md`:
- Around line 1-80: The PR added Swatinem/rust-cache@* to the Greek allowlist
(docs/i18n/el/actions-source-policy.md) but failed to update the same entry in
the other locale files (docs/i18n/zh-CN/actions-source-policy.md,
docs/i18n/ja/actions-source-policy.md, docs/i18n/ru/actions-source-policy.md,
docs/i18n/fr/actions-source-policy.md); add the identical allowlist line
(`Swatinem/rust-cache@*`) to each missing locale file so all supported locales
(en, zh-CN, ja, ru, fr, vi, el) are consistent, and then address the bundled
change concern by either updating the PR description to explicitly document the
coupling rationale for including Actions policy changes plus Discord-related
edits (src/channels/discord.rs and
python/zeroclaw_tools/integrations/discord_bot.py) or split the Discord code
changes into a separate PR per the governance template.
---
Nitpick comments:
In @.github/workflows/docs-deploy.yml:
- Around line 213-285: The workflow duplicates large heredoc HTML blocks that
generate site/index.html and site/docs/index.html; extract the shared HTML
generation into a reusable script or function (e.g., a shell script like
generate_docs_index.sh or a workflow composite/action) and replace the heredocs
in both the preview and deploy jobs with calls to that script. Implement the
script to accept parameters (title, subtitle/file links, output path) or read a
small template + variables so both invocations reuse the same template logic,
then update the workflow steps that currently run the heredocs (the
site/index.html and site/docs/index.html generation steps) to call the new
script with appropriate arguments.
In `@src/tools/browser_open.rs`:
- Around line 148-174: The bail message claims separate "default browser
launchers" and "Brave compatibility fallback" phases, but the code uses a single
unified loop over the commands (see the command list, the loop that builds
tokio::process::Command, the last_error variable, and the anyhow::bail! call);
either update the bail text to reflect a single unified attempt (e.g., "Failed
to open URL with any browser launcher. Last error: {last_error}") or refactor
into two loops: one iterating a default_launchers array and a second iterating
brave_fallbacks (preserving the same command/status handling and last_error
accumulation) and then adjust the bail message to accurately report which phase
failed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
.github/CODEOWNERS.github/actionlint.yaml.github/workflows/ci-build-fast.yml.github/workflows/ci-canary-gate.yml.github/workflows/ci-change-audit.yml.github/workflows/ci-connectivity-probes.yml.github/workflows/ci-provider-connectivity.yml.github/workflows/ci-reproducible-build.yml.github/workflows/ci-rollback.yml.github/workflows/ci-run.yml.github/workflows/ci-supply-chain-provenance.yml.github/workflows/docs-deploy.yml.github/workflows/feature-matrix.yml.github/workflows/main-promotion-gate.yml.github/workflows/nightly-all-features.yml.github/workflows/pr-auto-response.yml.github/workflows/pr-check-stale.yml.github/workflows/pr-check-status.yml.github/workflows/pr-intake-checks.yml.github/workflows/pr-label-policy-check.yml.github/workflows/pr-labeler.yml.github/workflows/pub-docker-img.yml.github/workflows/pub-prerelease.yml.github/workflows/pub-release.yml.github/workflows/sec-audit.yml.github/workflows/sec-codeql.yml.github/workflows/sec-vorpal-reviewdog.yml.github/workflows/sync-contributors.yml.github/workflows/test-benchmarks.yml.github/workflows/test-e2e.yml.github/workflows/test-fuzz.yml.github/workflows/test-rust-build.yml.github/workflows/workflow-sanity.ymlTESTING_TELEGRAM.mddocs/actions-source-policy.mddocs/i18n/README.mddocs/i18n/el/actions-source-policy.mddocs/i18n/vi/actions-source-policy.mdsrc/channels/mod.rssrc/channels/whatsapp_web.rssrc/tools/browser_open.rs
✅ Files skipped from review due to trivial changes (1)
- docs/i18n/README.md
.github/workflows/ci-build-fast.yml
Outdated
| toolchain: 1.92.0 | ||
|
|
||
| - uses: useblacksmith/rust-cache@f53e7f127245d2a269b3d90879ccf259876842d5 # v3 | ||
| - uses: Swatinem/rust-cache@779680da715d629ac1d338a641029a2f4372abb5 # v3 |
Contributor
There was a problem hiding this comment.
Document the new GitHub Action source in the allowlist policy.
Switching to Swatinem/rust-cache should be reflected in PR notes (allowlist impact) and docs/actions-source-policy.md when action sources change.
As per coding guidelines, “For any .github/workflows changes, include Actions allowlist impact in PR notes and update docs/actions-source-policy.md when sources change.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-build-fast.yml at line 55, Update the repository
documentation and PR notes to reflect the new GitHub Action source: the workflow
reference Swatinem/rust-cache@779680da715d629ac1d338a641029a2f4372abb5 in
.github/workflows/ci-build-fast.yml must be added to the Actions allowlist
impact section in the PR description and the official allowlist document
docs/actions-source-policy.md; edit docs/actions-source-policy.md to include the
new action source entry (name, owner, and pinned commit) and add a short note in
the PR template or PR description indicating the allowlist change for reviewers
to approve.
.github/workflows/ci-run.yml
Outdated
| changes: | ||
| name: Detect Change Scope | ||
| runs-on: blacksmith-2vcpu-ubuntu-2404 | ||
| runs-on: [self-hosted, Linux, X64] |
Contributor
There was a problem hiding this comment.
Constrain self-hosted runner trust boundary.
Using only [self-hosted, Linux, X64] across privileged CI jobs can route work onto any matching self-hosted node. Add a dedicated trust-tier label (or runner group) to avoid accidental broad scheduling.
Also applies to: 49-49, 73-73, 90-90, 111-111, 158-158, 167-167, 176-176, 231-231, 257-257, 278-278, 296-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-run.yml at line 24, The workflow uses broad runner
selector values like the runs-on array [self-hosted, Linux, X64] which can
schedule sensitive jobs onto any matching self-hosted node; update each runs-on
entry that currently equals [self-hosted, Linux, X64] (and other similar
occurrences) to include a dedicated trust-tier label (for example add a stable
label such as "trusted" or "tier:trusted") so only approved runners pick up
these privileged jobs, and ensure the new label is added consistently across all
occurrences referenced in the workflow.
.github/workflows/docs-deploy.yml
Outdated
| <main class="container"> | ||
| <section class="card"> | ||
| <h1>ZeroClaw Documentation</h1> | ||
| <p class="muted">Automatically deployed from <code>main</code> via <code>.github/workflows/docs-deploy.yml</code>.</p> |
Contributor
There was a problem hiding this comment.
Avoid hardcoding main in deploy provenance text.
This can become incorrect for rollback/manual source refs. Use ${{ needs.docs-quality.outputs.source_ref }} in the rendered message instead.
Suggested fix
- <p class="muted">Automatically deployed from <code>main</code> via <code>.github/workflows/docs-deploy.yml</code>.</p>
+ <p class="muted">Automatically deployed from <code>${{ needs.docs-quality.outputs.source_ref }}</code> via <code>.github/workflows/docs-deploy.yml</code>.</p>📝 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.
Suggested change
| <p class="muted">Automatically deployed from <code>main</code> via <code>.github/workflows/docs-deploy.yml</code>.</p> | |
| <p class="muted">Automatically deployed from <code>${{ needs.docs-quality.outputs.source_ref }}</code> via <code>.github/workflows/docs-deploy.yml</code>.</p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docs-deploy.yml at line 358, Replace the hardcoded branch
name "main" in the deployment provenance paragraph (the element with class
"muted" that currently reads 'Automatically deployed from <code>main</code> via
<code>.github/workflows/docs-deploy.yml</code>.') with the pipeline output
variable so the message uses the actual source ref; specifically update that
paragraph to render the expression ${{ needs.docs-quality.outputs.source_ref }}
instead of the literal "main" so rollbacks or manual refs display correctly.
.github/workflows/pub-docker-img.yml
Outdated
| - name: Setup Blacksmith Builder | ||
| uses: useblacksmith/setup-docker-builder@ef12d5b165b596e3aa44ea8198d8fde563eab402 # v1 | ||
| - name: Setup Buildx Builder | ||
| uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v1 |
Contributor
There was a problem hiding this comment.
Capture docker action source changes in workflow source policy docs.
This file introduces/replaces external action sources (docker/setup-buildx-action, docker/build-push-action). Please record allowlist impact in PR notes and update docs/actions-source-policy.md accordingly.
As per coding guidelines, “For any .github/workflows changes, include Actions allowlist impact in PR notes and update docs/actions-source-policy.md when sources change.”
Also applies to: 54-54, 86-86, 122-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pub-docker-img.yml at line 42, The workflow now
introduces/replaces external actions docker/setup-buildx-action and
docker/build-push-action; update the project Actions source policy by adding
these sources to docs/actions-source-policy.md and document the allowlist impact
in the PR notes, and also ensure any other occurrences of the same actions in
the workflow are captured in the same doc/PR notes update.
| - `softprops/action-gh-release@*` | ||
| - `sigstore/cosign-installer@*` | ||
| - `useblacksmith/*` (cơ sở hạ tầng self-hosted runner Blacksmith) | ||
| - `Swatinem/rust-cache@*` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the Vietnamese locale actions policy file exists and examine its structure
git ls-files docs/i18n/vi/ | head -20Repository: zeroclaw-labs/zeroclaw
Length of output: 734
🏁 Script executed:
# Verify the specific file and check line counts
if git ls-files docs/i18n/vi/actions-source-policy.md | grep -q .; then
echo "=== File found ==="
wc -l docs/i18n/vi/actions-source-policy.md
echo ""
echo "=== Line 25 context ==="
sed -n '20,30p' docs/i18n/vi/actions-source-policy.md
echo ""
echo "=== Lines 77-79 context ==="
sed -n '75,80p' docs/i18n/vi/actions-source-policy.md
else
echo "File not found in git"
fiRepository: zeroclaw-labs/zeroclaw
Length of output: 809
🏁 Script executed:
# Check if other locale versions have the same changes
echo "=== Checking English version ==="
grep -n "Swatinem/rust-cache@" docs/actions-source-policy.md 2>/dev/null || echo "Not found in en"
echo ""
echo "=== Checking Greek version ==="
grep -n "Swatinem/rust-cache@" docs/i18n/el/actions-source-policy.md 2>/dev/null || echo "Not found in el"
echo ""
echo "=== Checking all locale versions for this entry ==="
for locale in zh-CN ja ru fr vi; do
echo -n "$locale: "
grep -c "Swatinem/rust-cache@" "docs/i18n/$locale/actions-source-policy.md" 2>/dev/null || echo "0 or file missing"
doneRepository: zeroclaw-labs/zeroclaw
Length of output: 404
🏁 Script executed:
# Check which locale versions exist
echo "=== Available locale versions ==="
find docs/i18n -name "actions-source-policy.md" -type f | sortRepository: zeroclaw-labs/zeroclaw
Length of output: 330
🏁 Script executed:
# Check if zh-CN version exists and what its allowlist looks like
echo "=== Chinese (Simplified) version ==="
head -35 docs/i18n/zh-CN/actions-source-policy.md | tail -15
echo ""
echo "=== Japanese version ==="
head -35 docs/i18n/ja/actions-source-policy.md | tail -15
echo ""
echo "=== Russian version ==="
head -35 docs/i18n/ru/actions-source-policy.md | tail -15
echo ""
echo "=== French version ==="
head -35 docs/i18n/fr/actions-source-policy.md | tail -15Repository: zeroclaw-labs/zeroclaw
Length of output: 2386
🏁 Script executed:
# Double-check that en version has the entry properly dated
echo "=== English version history around 2026-02-26 ==="
grep -A 3 "2026-02-26" docs/actions-source-policy.md
echo ""
echo "=== Vietnamese version history ==="
grep -A 3 "2026-02-26" docs/i18n/vi/actions-source-policy.mdRepository: zeroclaw-labs/zeroclaw
Length of output: 769
Critical: i18n follow-through incomplete across all supported locales
While the Vietnamese translation is internally consistent with English and Greek versions, the Swatinem/rust-cache@* allowlist entry and 2026-02-26 history update are missing from Chinese (zh-CN), Japanese (ja), Russian (ru), and French (fr) locale versions.
Per coding guidelines, when a change touches runtime-contract references in shared docs, i18n follow-through must cover all supported locales (en, zh-CN, ja, ru, fr, vi, el) in the same PR. Add the corresponding allowlist entry and history note to the four missing locale versions before merge. Also include i18n completion status in PR notes per docs/i18n-guide.md checklist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/i18n/vi/actions-source-policy.md` at line 25, The Vietnamese docs added
the `Swatinem/rust-cache@*` allowlist entry and a 2026-02-26 history note in
actions-source-policy.md but the same runtime-contract change must be applied to
the other locales; update the corresponding files for Chinese (zh-CN), Japanese
(ja), Russian (ru) and French (fr) versions of actions-source-policy.md to
include the exact allowlist entry `Swatinem/rust-cache@*` and add the same
2026-02-26 history entry, then update the PR description to mark i18n
follow-through complete per the docs/i18n-guide.md checklist (include which
locales were updated).
src/channels/mod.rs
Outdated
| Some(ChannelRuntimeCommand::SetModel(model)) | ||
| } | ||
| } | ||
| "/new" => Some(ChannelRuntimeCommand::NewSession), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate `/new` arms and context in parse_runtime_command.
echo "=== All occurrences of \"/new\" in src/channels/mod.rs ==="
rg -n '"/new"' src/channels/mod.rs
echo ""
echo "=== Context around lines 700-732 ==="
sed -n '700,732p' src/channels/mod.rs | cat -nRepository: zeroclaw-labs/zeroclaw
Length of output: 2056
Remove the duplicate /new match arm at line 728.
Line 728 contains an unreachable pattern: "/new" => Some(ChannelRuntimeCommand::NewSession), is already matched by line 702's "/new" | "/clear" arm, making this branch dead code. Rust evaluates match arms in order, so this duplicate is unreachable and should be removed to maintain straightforward control flow.
♻️ Proposed fix
- "/new" => Some(ChannelRuntimeCommand::NewSession),📝 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.
Suggested change
| "/new" => Some(ChannelRuntimeCommand::NewSession), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/mod.rs` at line 728, There is a duplicate match arm for the
string literal "/new" that maps to ChannelRuntimeCommand::NewSession and is
unreachable because "/new" is already included in the combined arm that matches
"/new" | "/clear"; remove the redundant arm mapping `"/new" =>
Some(ChannelRuntimeCommand::NewSession)` from the match expression (leave the
existing combined arm intact) so the match has no dead/unreachable branches.
src/channels/whatsapp_web.rs
Outdated
Comment on lines
+282
to
+285
| add_candidate(Self::normalize_phone_token(&sender.to_string())); | ||
| if let Some(alt) = sender_alt { | ||
| add_candidate(Self::normalize_phone_token(&alt.to_string())); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the code at lines 282-285
cat -n src/channels/whatsapp_web.rs | sed -n '275,290p'Repository: zeroclaw-labs/zeroclaw
Length of output: 745
🏁 Script executed:
# Check if wa-rs-binary is used and what version
cat Cargo.toml | grep -A 5 -B 5 wa-rsRepository: zeroclaw-labs/zeroclaw
Length of output: 1990
🏁 Script executed:
# Search for Jid usage patterns in the codebase
rg "Jid::" src/channels/whatsapp_web.rs -A 2 -B 2Repository: zeroclaw-labs/zeroclaw
Length of output: 130
🌐 Web query:
wa-rs-binary v0.2.0 Jid::to_string() format PN device LID JID types
💡 Result:
In wa-rs-binary v0.2.0, Jid::to_string() uses the type’s Display impl (i.e., it renders a “normal” JID string, not the AD form). The formatting rules are: user[.agent?][:device?]@server, with a special case for server-only JIDs. [1]
Jid::to_string() / Display format
- If
jid.useris empty → output is just the server (no@). [1] - Otherwise:
- start with
user - append
.agentonly ifagent > 0andserveris nots.whatsapp.net, notlid, and nothosted[1] - append
:deviceifdevice > 0[1] - append
@server[1]
- start with
So, typical outputs are:
- PN (phone-number) JID:
12345@s.whatsapp.net - PN device JID:
12345:7@s.whatsapp.net - LID JID:
abcdef@lid - LID device JID:
abcdef:7@lid
(For PN/LID servers specifically, the .agent suffix is intentionally not appended by the formatter.) [1]
PN vs LID vs other JID “types” (by server)
The crate defines these servers (among others): [2]
- PN user server:
s.whatsapp.net(DEFAULT_USER_SERVER) - LID server:
lid(HIDDEN_USER_SERVER) - Groups:
g.us(GROUP_SERVER) - Legacy users:
c.us(LEGACY_USER_SERVER) - Newsletters:
newsletter, etc.
Note: AD string is different
If you need the “AD” encoding, that is to_ad_string(), which formats as user.agent:device@server (note the dot before agent). [3]
Sources:
[1] wa-rs-binary 0.2.0 impl Display for Jid (jid.rs) (docs.rs)
[2] wa-rs-binary 0.2.0 JID server constants (jid.rs) (docs.rs)
[3] wa-rs-binary 0.2.0 to_ad_string() (jid.rs) (docs.rs)
Citations:
- 1: https://docs.rs/wa-rs-binary/latest/src/wa_rs_binary/jid.rs.html
- 2: https://docs.rs/wa-rs-binary/latest/src/wa_rs_binary/jid.rs.html
- 3: https://docs.rs/wa-rs-binary/latest/src/wa_rs_binary/jid.rs.html
🏁 Script executed:
# Find normalize_phone_token implementation
rg "fn normalize_phone_token" src/channels/whatsapp_web.rs -A 10Repository: zeroclaw-labs/zeroclaw
Length of output: 380
🏁 Script executed:
# Get broader context around lines 282-285 to understand the function purpose
cat -n src/channels/whatsapp_web.rs | sed -n '260,300p'Repository: zeroclaw-labs/zeroclaw
Length of output: 1722
🏁 Script executed:
# Check what types sender and sender_alt are
rg "sender.*:" src/channels/whatsapp_web.rs | head -20Repository: zeroclaw-labs/zeroclaw
Length of output: 310
🏁 Script executed:
# Look for where this function is called and what context sender comes from
rg "normalize_phone_token\|sender.*Jid\|sender_alt" src/channels/whatsapp_web.rs -B 5 -A 2 | head -100Repository: zeroclaw-labs/zeroclaw
Length of output: 48
🏁 Script executed:
# Get the full normalize_phone_token function
rg "fn normalize_phone_token" src/channels/whatsapp_web.rs -A 25Repository: zeroclaw-labs/zeroclaw
Length of output: 898
🏁 Script executed:
# Also check where sender_phone_candidates is used to understand the context
rg "sender_phone_candidates" src/channels/whatsapp_web.rs -B 5 -A 5Repository: zeroclaw-labs/zeroclaw
Length of output: 993
Use the Jid's user field directly instead of to_string() to avoid device ID concatenation.
The current approach sender.to_string() produces formats like 12345:7@s.whatsapp.net (for device JIDs), and extracting only digits concatenates the device ID with the phone number, producing +123457 instead of +12345. This creates incorrect phone tokens for allowlist matching. Extract the user component directly from the Jid struct instead (e.g., sender.user) to avoid this issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/whatsapp_web.rs` around lines 282 - 285, The code is building
phone tokens from a Jid by calling sender.to_string(), which includes device IDs
(e.g., "12345:7@s.whatsapp.net") and leads normalize_phone_token to produce
incorrect tokens; change the calls that pass sender/to sender_alt into
add_candidate to use the Jid.user field (e.g., sender.user and alt.user) so
normalize_phone_token receives only the phone number portion; update the two
places that call add_candidate(Self::normalize_phone_token(&sender.to_string()))
and add_candidate(Self::normalize_phone_token(&alt.to_string())) to pass the
Jid.user string slice instead, ensuring allowlist matching uses the correct
phone token.
src/channels/whatsapp_web.rs
Outdated
Comment on lines
+660
to
+664
| tracing::warn!( | ||
| "WhatsApp Web: message from {} not in allowed list (candidates: {:?})", | ||
| sender_jid, | ||
| sender_candidates | ||
| ); |
Contributor
There was a problem hiding this comment.
Do not log full sender candidate identifiers at warn level.
sender_candidates can contain normalized phone identifiers; logging the full vector increases sensitive-data exposure. Log count/source type instead.
Suggested fix
- tracing::warn!(
- "WhatsApp Web: message from {} not in allowed list (candidates: {:?})",
- sender_jid,
- sender_candidates
- );
+ tracing::warn!(
+ "WhatsApp Web: message from {} not in allowed list (candidate_count={})",
+ sender_jid,
+ sender_candidates.len()
+ );As per coding guidelines, “Never log secrets, raw tokens, or sensitive payloads in Rust code.”
📝 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.
Suggested change
| tracing::warn!( | |
| "WhatsApp Web: message from {} not in allowed list (candidates: {:?})", | |
| sender_jid, | |
| sender_candidates | |
| ); | |
| tracing::warn!( | |
| "WhatsApp Web: message from {} not in allowed list (candidate_count={})", | |
| sender_jid, | |
| sender_candidates.len() | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/whatsapp_web.rs` around lines 660 - 664, The tracing::warn! call
that currently logs sender_jid and the full sender_candidates vector should be
changed to avoid emitting sensitive identifiers; update the warn in the code
around sender_jid/sender_candidates to log only non-sensitive metadata (e.g.,
number of candidates via sender_candidates.len() and a brief source/type string)
and keep sender_jid redacted or removed, modify the tracing::warn! invocation
accordingly so it no longer prints the raw sender_candidates contents.
|
Hi @loydccc, friendly automation nudge from PR hygiene. This PR has had no new commits for 48h and still needs an update before merge:
Recommended next steps
Maintainers: apply |
loydccc
force-pushed
the
fix/discord-image-markers-1825
branch
from
b11c22f to
3157ae7
Compare
Author
|
Update: I force-pushed a clean, scope-reduced branch to this PR. What changed
Validation run
Notes
If maintainers can provide a Linear key in-thread, I will update the PR title/body immediately and re-run CI. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/channels/discord.rs (2)
1613-1659:⚠️ Potential issue | 🟡 MinorTest coverage still misses multi-image ordering and hash-suffix fallback.
Great additions, but this block still doesn’t lock in multi-image marker ordering/join behavior, and it doesn’t explicitly cover
#fragmentsuffix handling for extension detection.Suggested test additions
+ #[tokio::test] + async fn process_attachments_emits_multiple_image_markers_in_order() { + let client = reqwest::Client::new(); + let attachments = vec![ + serde_json::json!({ + "url": "https://cdn.discordapp.com/attachments/123/1/a.png", + "filename": "a.png", + "content_type": "image/png" + }), + serde_json::json!({ + "url": "https://cdn.discordapp.com/attachments/123/2/b.jpeg?size=1024", + "filename": "b.jpeg" + }), + ]; + + let result = process_attachments(&attachments, &client).await; + assert_eq!(result.matches("[IMAGE:").count(), 2); + let first = result.find("a.png").expect("first marker"); + let second = result.find("b.jpeg?size=1024").expect("second marker"); + assert!(first < second, "markers should preserve attachment order"); + } + + #[test] + fn is_image_attachment_handles_hash_suffix_in_url_extension_fallback() { + assert!(is_image_attachment( + "", + "file", + "https://cdn.discordapp.com/attachments/123/456/photo.webp#view" + )); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/channels/discord.rs` around lines 1613 - 1659, Add tests to assert multi-attachment ordering and join behavior in process_attachments and to validate extension-detection when filenames include URL fragments (e.g., "#fragment") by exercising is_image_attachment and process_attachments with multiple attachments and with filenames/URLs that include fragments and no content_type; specifically, create a test that passes multiple attachments (mixed image and non-image) and asserts the resulting string preserves input order and joins image markers correctly, and create tests where filename or url has a trailing fragment (like "photo.png#hash" or URL "?size=1024#hash") with missing content_type to ensure is_image_attachment falls back to the extension after trimming fragments.
135-138:⚠️ Potential issue | 🟡 MinorDocstring is inconsistent with runtime behavior for unsupported attachments.
At Line 138, the docs say unsupported types are “silently skipped,” but the implementation logs a debug event for skipped unsupported types. Please align the comment with actual behavior.
Proposed doc fix
-/// All other types are silently skipped. Fetch errors are logged as warnings. +/// All other types are skipped with a debug log. Fetch errors are logged as warnings.As per coding guidelines "Keep unsupported paths explicit (error out) rather than adding partial fake support."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/channels/discord.rs` around lines 135 - 138, The docstring in src/channels/discord.rs incorrectly states that unsupported attachment types are "silently skipped"; update the comment to reflect runtime behavior: unsupported types are skipped but a debug-level log is emitted for each skipped attachment, while `image/*` are passed as `[IMAGE:<url>]` and `text/*` are fetched/inline with fetch errors logged as warnings; update the description near the existing discord attachment-handling doc comment to mention the debug log for skipped types so it matches the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/channels/discord.rs`:
- Around line 1613-1659: Add tests to assert multi-attachment ordering and join
behavior in process_attachments and to validate extension-detection when
filenames include URL fragments (e.g., "#fragment") by exercising
is_image_attachment and process_attachments with multiple attachments and with
filenames/URLs that include fragments and no content_type; specifically, create
a test that passes multiple attachments (mixed image and non-image) and asserts
the resulting string preserves input order and joins image markers correctly,
and create tests where filename or url has a trailing fragment (like
"photo.png#hash" or URL "?size=1024#hash") with missing content_type to ensure
is_image_attachment falls back to the extension after trimming fragments.
- Around line 135-138: The docstring in src/channels/discord.rs incorrectly
states that unsupported attachment types are "silently skipped"; update the
comment to reflect runtime behavior: unsupported types are skipped but a
debug-level log is emitted for each skipped attachment, while `image/*` are
passed as `[IMAGE:<url>]` and `text/*` are fetched/inline with fetch errors
logged as warnings; update the description near the existing discord
attachment-handling doc comment to mention the debug log for skipped types so it
matches the implementation.
theonlyhennygod
force-pushed
the
fix/discord-image-markers-1825
branch
from
3157ae7 to
2a5d922
Compare
# Conflicts: # src/channels/discord.rs
theonlyhennygod
force-pushed
the
fix/discord-image-markers-1825
branch
from
2a5d922 to
56430a9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe this PR in 2-5 bullets:
devfor normal contributions;mainonly fordevpromotion):devsrc/channels/discord.rs::process_attachmentsonly inlinedtext/*and dropped image attachments, so Discord inbound images were invisible to multimodal handling.Label Snapshot (required)
risk: low|medium|high):risk: mediumsize: XS|S|M|L|XL, auto-managed/read-only): expectedsize: Score|agent|channel|config|cron|daemon|doctor|gateway|health|heartbeat|integration|memory|observability|onboard|provider|runtime|security|service|skillforge|skills|tool|tunnel|docs|dependencies|ci|tests|scripts|dev, comma-separated):channel, tests<module>: <component>, for examplechannel: telegram,provider: kimi,tool: shell):channel: discordtrusted contributor|experienced contributor|principal contributor|distinguished contributor, auto-managed/read-only; author merged PRs >=5/10/20/50): auto-managedChange Metadata
bug|feature|refactor|docs|security|chore):bugruntime|provider|channel|memory|security|ci|docs|multi):channelLinked Issue
Supersede Attribution (required when
Supersedes #is used)#<pr> by @<author>, one per line):devbranch.Co-authored-bytrailers added for materially incorporated contributors? (Yes/No):NoNo, explain why (for example: inspiration-only, no direct code/design carry-over): independent implementation from currentupstream/dev, no direct code cherry-pick from superseded branches.\n):PassValidation Evidence (required)
Commands and result summary:
cargo fmt --all -- --check cargo clippy --all-targets -- -D warnings cargo testcargo fmt --all -- --check: passcargo clippy --all-targets -- -D warnings: fail due pre-existing upstreamdevissues unrelated to this diff (e.g.src/cron/scheduler.rsclippy::large_futures,src/config/schema.rsunused import).cargo test: full suite not run in this cycle; targeted tests below were run for touched behavior.cargo check --lib: passCARGO_INCREMENTAL=0 RUSTFLAGS='-C debuginfo=0 -C link-arg=/DEBUG:NONE' cargo test process_attachments --lib: pass (4/4)CARGO_INCREMENTAL=0 RUSTFLAGS='-C debuginfo=0 -C link-arg=/DEBUG:NONE' cargo test is_image_attachment --lib: pass (2/2)cargo testskipped for iteration speed; behavior coverage added and executed for changed code path.Security Impact (required)
Yes/No):NoYes/No):NoYes/No):NoYes/No):NoYes, describe risk and mitigation: N/APrivacy and Data Hygiene (required)
pass|needs-follow-up):passCompatibility / Migration
Yes/No):YesYes/No):NoYes/No):Noi18n Follow-Through (required when docs or user-facing wording changes)
Yes/No):NoYes, locale navigation parity updated inREADME*,docs/README*, anddocs/SUMMARY.mdfor supported locales (en,zh-CN,ja,ru,fr,vi)? (Yes/No): N/AYes, localized runtime-contract docs updated where equivalents exist (minimum forfr/vi:commands-reference,config-reference,troubleshooting)? (Yes/No/N.A.): N/AYes, Vietnamese canonical docs underdocs/i18n/vi/**synced and compatibility shims underdocs/*.vi.mdvalidated? (Yes/No/N.A.): N/ANo/N.A., link follow-up issue/PR and explain scope decision: N/AHuman Verification (required)
What was personally validated beyond CI:
image/*attachment emits[IMAGE:<url>]marker.content_typebut image extension emits marker.text/plain) does not emit marker even if filename ends with image extension.application/octet-streamallows extension fallback.Side Effects / Blast Radius (required)
src/channels/discord.rsinbound attachment parsing only.Agent Collaboration Notes (recommended)
AGENTS.md+CONTRIBUTING.md): confirmed.Rollback Plan (required)
git revert 36c186a7aff9890b2c043f6853376b211273c52e[IMAGE:...]in downstream prompt context.Risks and Mitigations
List real risks in this PR (or write
None).application/octet-streamcontent types.Summary by CodeRabbit
New Features
Improvements
Tests
Refs RMN-217