feat(discord): emit IMAGE markers for inbound image attachments (RMN-1826)#1826
feat(discord): emit IMAGE markers for inbound image attachments (RMN-1826)#1826xero7689 wants to merge 1171 commits intozeroclaw-labs:masterfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Image Attachment Processing src/channels/discord.rs |
Added image/* MIME type handling to process_attachments() to emit [IMAGE:<cdn_url>] markers for image attachments. Includes tests for single and multiple image attachment scenarios. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested labels
size: XS
Suggested reviewers
- theonlyhennygod
- chumyin
Poem
🐰 A hop, a skip, through Discord's door,
Images now shine where none before!
[IMAGE:link]in markers we weave,
Vision-bound channels now truly believe. 🖼️✨
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | The PR fully addresses all acceptance criteria from issue #1825: image/* attachments produce [IMAGE:<cdn_url>] markers, non-image attachments are skipped, tests cover single and multiple images, and the 2-line implementation matches the proposed solution. |
| Out of Scope Changes check | ✅ Passed | All changes are in scope: only src/channels/discord.rs modified with the image/* branch addition and corresponding tests, matching the stated 2-line architecture impact. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Description check | ✅ Passed | The pull request description is comprehensive and well-structured, covering all required template sections including summary, linked issues, labels, change metadata, validation evidence, security/privacy impact, compatibility, i18n assessment, and rollback plan. |
| Title check | ✅ Passed | The title accurately and specifically describes the main change: adding IMAGE marker emission for inbound image attachments in the Discord channel handler. |
✏️ 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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/channels/discord.rs (1)
1453-1454: Tighten multi-image assertion to enforce output contract.
containschecks can miss regressions in delimiter/order/extra content. Prefer asserting the full expected string.As per coding guidelines, "Keep tests deterministic (no flaky timing/network dependence without guardrails)".Suggested test tightening
- assert!(result.contains("[IMAGE:https://cdn.discordapp.com/attachments/1/2/a.jpg]")); - assert!(result.contains("[IMAGE:https://cdn.discordapp.com/attachments/1/2/b.png]")); + assert_eq!( + result, + "[IMAGE:https://cdn.discordapp.com/attachments/1/2/a.jpg]\n---\n[IMAGE:https://cdn.discordapp.com/attachments/1/2/b.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 1453 - 1454, The test currently uses loose checks on the variable result via assert!(result.contains(...)) for two image tokens; tighten it by asserting the exact expected output string (including delimiters and order) instead of contains so regressions in formatting or extra content fail the test—construct the exact expected value containing "[IMAGE:https://cdn.discordapp.com/attachments/1/2/a.jpg][IMAGE:https://cdn.discordapp.com/attachments/1/2/b.png]" (or with the exact spacing/newline/delimiter your contract requires) and replace the two contains assertions with a single equality assertion (e.g., assert_eq!(result, expected)) referring to result in the same test.
🤖 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 1453-1454: The test currently uses loose checks on the variable
result via assert!(result.contains(...)) for two image tokens; tighten it by
asserting the exact expected output string (including delimiters and order)
instead of contains so regressions in formatting or extra content fail the
test—construct the exact expected value containing
"[IMAGE:https://cdn.discordapp.com/attachments/1/2/a.jpg][IMAGE:https://cdn.discordapp.com/attachments/1/2/b.png]"
(or with the exact spacing/newline/delimiter your contract requires) and replace
the two contains assertions with a single equality assertion (e.g.,
assert_eq!(result, expected)) referring to result in the same test.
dwillitzer
left a comment
There was a problem hiding this comment.
Reviewed by ZeroClaw-Nigel team. Discord-only addition for IMAGE markers. Follows existing patterns. Safe to merge.
PR intake checks found warnings (non-blocking)Fast safe checks found advisory issues. CI lint/test/build gates still enforce merge quality.
Action items:
Detected Linear keys: RMN-1826 Run logs: https://github.com/zeroclaw-labs/zeroclaw/actions/runs/22543656370 Detected blocking line issues (sample):
Detected advisory line issues (sample):
|
…irst-extraction refactor(workspace): scaffold M4-5 PR-1 crate shells and CI lanes [RMN-248]
…d-wiring fix(web-fetch): wire html2md feature dependency (RMN-310)
…blue-live feat(web): Electric Blue dashboard with polished UI
fix(security): deny approval-required tools on non-CLI channels
…main-rebased feat(hardware): replay pico toolchain + prompt wiring on main [RMN-1837]
…rm-heartbeat feat(heartbeat): add lightweight proactive task dedupe and per-tick caps
…kill-invocation-links fix(skills): allow safe cross-skill markdown references
…odex-ws-sse-fallback fix(provider): fallback to sse on codex websocket no-response
…roup-sender-identity fix(channels): include telegram sender identity in group LLM prompts
…ctivation-warning fix(config): report initialized state correctly on load
…apcat-channel feat(channels): add napcat/onebot onboarding and config UI
…nhanced-recall-safe feat(memory): multi-query expansion with error-safe recall
…emantic-vectordb-guard feat(security): add semantic vectordb guard and corpus updater
…udit-log-init fix(audit): initialize log file when audit logging is enabled
…g_readme_i18n_20260304 feat(site): add es/pt/it locale support with UI selector
…odeowners-tri-owner-20260304 chore(codeowners): align main with dev tri-owner approver routing
…09-main-20260305 feat(integrations): support lmstudio custom connector endpoint
…e persistence (replay zeroclaw-labs#2797)
…97-main-20260305 fix(cron,security,onboarding): unify shell policy and custom-home-safe persistence
…09-main-20260305 chore: remove Linear and Hetzner integrations
theonlyhennygod
left a comment
There was a problem hiding this comment.
Approved for rebase/merge pass.
Discord's process_attachments() previously silenced image/* MIME types with a debug log. Add a branch that emits [IMAGE:<cdn_url>] markers so the multimodal pipeline can forward them to vision-capable providers. Follows the same pattern already used in qq.rs and linq.rs for inbound image handling. No new dependencies; reuses the existing HTTP client. Co-Authored-By: xero7689 <shzlee217@gmail.com>
8bb3a3c to
e496976
Compare
Summary
process_attachments()silently droppedimage/*attachments with a debug logimage/*branch that emits[IMAGE:<cdn_url>]markersqq.rsandlinq.rsLinked Issue
Closes #1825
Label Snapshot (required)
risk: lowsize: XSchannel,tool: discordChange Metadata
featurechannelValidation Evidence (required)
Manually verified: sent image attachment over Discord, agent received
[IMAGE:<cdn_url>]marker and forwarded to vision-capable provider successfully.Security Impact (required)
Privacy and Data Hygiene (required)
passcdn.discordapp.complaceholder paths and neutral filenames only.Compatibility / Migration
i18n Follow-Through (required)
Side Effects / Blast Radius (required)
src/channels/discord.rsonly.provider_capability_errorwhen sending images. This is the existing behaviour for all other channels; tracked separately in feat(discord): emit IMAGE markers for inbound image attachments (RMN-1826) #1826.Rollback Plan (required)
image/*branch inprocess_attachments()— single focused change, no config impact.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Linear
Refs RMN-1826