feat: Add image ingestion and update staging tests for Slack#461
Conversation
Parse Slack image shares, stage authenticated file downloads, and document the files:read scope required for multimodal image ingestion.
|
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:
📝 WalkthroughWalkthroughAdds Slack image ingestion: parses Slack Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client
participant Parser as Slack Parser
participant SlackAPI as Slack API
participant HTTP as HTTP Client
participant Media as Media Staging
Client->>Parser: parse_slack_message(payload)
Parser->>Parser: extract file IDs, mime, captions
Parser-->>Client: ContentPart::Image entries
Client->>SlackAPI: fetch_and_stage_image(file_id, mime, max_bytes)
SlackAPI->>SlackAPI: call files.info (bearer auth) to get url_private_download
SlackAPI->>HTTP: GET url_private_download (bearer auth, 30s timeout)
HTTP-->>SlackAPI: stream bytes
SlackAPI->>Media: stream_validate_and_stage(bytes, mime, max_bytes)
Media-->>SlackAPI: StagedImage
SlackAPI-->>Client: Result<StagedImage, RejectionReason>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-08 to 2026-04-08 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/agent-runtime/src/channels/slack.rs (1)
57-72:⚠️ Potential issue | 🟠 MajorNormalize Slack app-level error handling across auth, listen, and health.
Slack returns HTTP 200 with
"ok": falsefor auth/scope/channel failures.get_bot_user_id(),poll_history(), andhealth_check()currently treat those payloads as success, so bad credentials can look healthy andlisten()can spin forever with nomessages. Please reuse the sameok == truevalidation thatsend()andresolve_file_download_url()already apply, and add a regression test for a 200/"ok": falseresponse.Based on learnings, "Implement
Channeltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests."Also applies to: 89-104, 367-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/slack.rs` around lines 57 - 72, The code treats HTTP 200/"ok": false Slack responses as success; update get_bot_user_id, poll_history, and health_check to parse the JSON response and verify resp.get("ok").and_then(|v| v.as_bool()) == Some(true) (the same validation used by send and resolve_file_download_url), return None or an Err when "ok" is false and log the error message from resp.get("error"); add a regression test that mocks a 200 with {"ok": false, "error": "..."} for auth/listen/health to assert these methods fail/return None and that Channel trait implementations (send, listen, health_check) exhibit consistent auth/allowlist/health semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/config/schema.rs`:
- Line 277: The new constant MVP_VALID_MULTIMODAL_CHANNELS added "slack" but
code in channels::mod (specifically where stage_channel_images() is dispatched
on msg.channel) does not check config.multimodal.allowed_channels; update the
dispatch logic to consult the multimodal allowlist first and only call
stage_channel_images() for channels present in
config.multimodal.allowed_channels (or when multimodal.enabled and the channel
is explicitly allowed). Ensure the check references the same channel identifier
values used in MVP_VALID_MULTIMODAL_CHANNELS and deny by default when the
channel is not in the allowlist.
---
Outside diff comments:
In `@clients/agent-runtime/src/channels/slack.rs`:
- Around line 57-72: The code treats HTTP 200/"ok": false Slack responses as
success; update get_bot_user_id, poll_history, and health_check to parse the
JSON response and verify resp.get("ok").and_then(|v| v.as_bool()) == Some(true)
(the same validation used by send and resolve_file_download_url), return None or
an Err when "ok" is false and log the error message from resp.get("error"); add
a regression test that mocks a 200 with {"ok": false, "error": "..."} for
auth/listen/health to assert these methods fail/return None and that Channel
trait implementations (send, listen, health_check) exhibit consistent
auth/allowlist/health semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c000adf4-c7c2-418f-8429-ea8db8e5dda7
📒 Files selected for processing (6)
clients/agent-runtime/README.mdclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/channels/slack.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/onboard/wizard.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: report / Contributor Quality Report
- GitHub Check: pr-checks
- GitHub Check: sonar
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
clients/agent-runtime/README.md
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/README.mdclients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/onboard/wizard.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/onboard/wizard.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/onboard/wizard.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/channels/slack.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/onboard/wizard.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/src/channels/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Channeltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests
Files:
clients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/config/schema.rs
🧠 Learnings (5)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
clients/agent-runtime/README.mdclients/agent-runtime/src/config/schema.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/README.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/integrations/mod.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/schema.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/channels/slack.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/config/schema.rs
🔇 Additional comments (4)
clients/agent-runtime/README.md (1)
249-250: Slack scope docs update is accurate and aligned with ingestion behavior.Adding
files:readalongsidechat:writeandchannels:historyis consistent with the new Slack image/file ingestion flow.clients/agent-runtime/src/onboard/wizard.rs (1)
3409-3409: Onboarding prompt now correctly reflects required Slack scopes for multimodal ingestion.Good update—this should prevent misconfigured Slack setups when image/file ingestion is enabled.
clients/agent-runtime/src/integrations/mod.rs (1)
123-123: Integration help text is now consistent with Slack file/image ingestion requirements.The explicit scope list improves setup clarity and aligns with the updated Slack channel capabilities.
clients/agent-runtime/src/channels/mod.rs (1)
1612-1617: Slack image staging branch is correctly integrated.Line 1612-Line 1617 mirrors the existing channel-specific staging pattern, preserves fail-closed behavior, and correctly propagates fetch/staging errors via
ImageRejectionReason.
|
|
||
| /// Valid MVP channel names for multimodal image ingress. | ||
| const MVP_VALID_MULTIMODAL_CHANNELS: &[&str] = &["telegram", "whatsapp", "discord"]; | ||
| const MVP_VALID_MULTIMODAL_CHANNELS: &[&str] = &["telegram", "whatsapp", "discord", "slack"]; |
There was a problem hiding this comment.
Enforce multimodal.allowed_channels before expanding Slack support.
Adding "slack" here widens an existing policy bypass: clients/agent-runtime/src/channels/mod.rs currently dispatches stage_channel_images() on msg.channel without checking config.multimodal.allowed_channels. With multimodal.enabled = true, Slack images can now be fetched even when operators intentionally leave "slack" out of the allowlist. Please gate staging on the configured allowlist before treating Slack as supported.
As per coding guidelines, "Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/config/schema.rs` at line 277, The new constant
MVP_VALID_MULTIMODAL_CHANNELS added "slack" but code in channels::mod
(specifically where stage_channel_images() is dispatched on msg.channel) does
not check config.multimodal.allowed_channels; update the dispatch logic to
consult the multimodal allowlist first and only call stage_channel_images() for
channels present in config.multimodal.allowed_channels (or when
multimodal.enabled and the channel is explicitly allowed). Ensure the check
references the same channel identifier values used in
MVP_VALID_MULTIMODAL_CHANNELS and deny by default when the channel is not in the
allowlist.
Treat Slack JSON responses with ok=false as failures in auth, history polling, and health checks, and add regression tests for those cases.
Deploying corvus with
|
| Latest commit: |
352b328
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b156dcd1.corvus-42x.pages.dev |
| Branch Preview URL: | https://feat-slack-image-ingestion-3.corvus-42x.pages.dev |
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)
clients/agent-runtime/src/channels/slack.rs (1)
128-131:⚠️ Potential issue | 🟡 MinorInconsistent error sanitization.
poll_historylogs the reqwest error directly without usingsanitize_error(), unlikeget_bot_user_id()and other methods. While unlikely to leak the token in practice, it's inconsistent with the defensive pattern used elsewhere.Proposed fix
.send() .await .map_err(|e| { - tracing::warn!("Slack poll error: {e}"); - e + tracing::warn!("Slack poll error: {}", self.sanitize_error(&e)); + e })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/slack.rs` around lines 128 - 131, The poll_history map_err closure logs reqwest errors raw—make it consistent with get_bot_user_id by passing the error through sanitize_error() before logging; update the closure in poll_history (the .map_err(|e| { tracing::warn!(...) ; e })?) to call sanitize_error(&e) in the tracing::warn! message and return the original error afterwards, so the token/credentials aren't exposed and behavior matches other methods using sanitize_error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/channels/slack.rs`:
- Around line 253-265: The download request
(self.client.get(&download_url).bearer_auth(&self.bot_token).send().await) has
no timeout and can hang; wrap the send() future in a tokio::time::timeout (e.g.,
Duration::from_secs(30)) or use reqwest's per-request timeout if enabled, then
map a timeout error to the same media::ImageRejectionReason::FetchFailed and
emit a tracing::warn (use self.sanitize_error or a clear message) so
slow/unresponsive downloads fail fast; update the error mapping around the
send() and any subsequent body reads to handle elapsed timeouts consistently.
- Around line 226-233: Replace the duplicated Slack error-extraction logic with
the existing helper Self::slack_api_error(payload): call
Self::slack_api_error(&payload) to get the api error string (or default), use
that value in the tracing::warn message, and keep returning
media::ImageRejectionReason::FetchFailed; update the block that currently
manually reads payload.get("error") to delegate to Self::slack_api_error for
consistency and single-source parsing.
---
Outside diff comments:
In `@clients/agent-runtime/src/channels/slack.rs`:
- Around line 128-131: The poll_history map_err closure logs reqwest errors
raw—make it consistent with get_bot_user_id by passing the error through
sanitize_error() before logging; update the closure in poll_history (the
.map_err(|e| { tracing::warn!(...) ; e })?) to call sanitize_error(&e) in the
tracing::warn! message and return the original error afterwards, so the
token/credentials aren't exposed and behavior matches other methods using
sanitize_error.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0518d123-8411-4ccb-9884-a540afa2675a
📒 Files selected for processing (1)
clients/agent-runtime/src/channels/slack.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sonar
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (5)
clients/agent-runtime/src/channels/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Channeltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests
Files:
clients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/channels/slack.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/channels/slack.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/channels/slack.rs
🧠 Learnings (1)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/channels/slack.rs
🔇 Additional comments (10)
clients/agent-runtime/src/channels/slack.rs (10)
1-54: LGTM!The new
api_base_urlfield with test-only constructor is a clean pattern for mocking. Thesanitize_error()helper properly redacts the bot token from error messages, andslack_api_error()avoids unnecessary allocations by returning&str.
63-104: LGTM!Robust layered error handling: HTTP status → JSON parse → API
okfield. Token redaction viasanitize_error()is properly applied.
154-194: LGTM!Correctly handles both
filesarray and singlefilefield from Slack's API. MIME validation delegates toAllowedImageMime::from_mime_str()which enforces the exact allowlist (jpeg, png, webp). The emptyfile_idcheck is good defensive coding.
304-315: Asymmetric parts handling for text-only vs image messages.When
image_partsis empty,partsbecomes an emptyVec, so text-only messages have noContentPart::Textinparts(only incontent). Image messages get bothcontentandpartspopulated. This asymmetry appears intentional for backward compatibility, but ensure downstream consumers handle both patterns.
340-375: LGTM!The
send()method correctly usesapi_url()and checks both HTTP status and APIokfield.
414-454: LGTM!The
health_check()method follows the hardened pattern: HTTP status check → JSON parse → APIokvalidation. Token redaction is correctly applied.
729-791: LGTM!Well-structured mock server that validates the two-step metadata → download flow. The
Connection: closeheader ensures clean socket termination.
840-888: LGTM!Excellent test coverage: verifies bearer auth on both requests, validates staged image properties (MIME, size, content, prefix), and confirms cleanup removes the temp file.
890-959: LGTM!These regression tests verify that Slack API-level errors (
ok: false) are correctly propagated. Good coverage ofget_bot_user_id,poll_history, andhealth_check.
576-659: LGTM!Thorough test coverage for image parsing:
filesarray with caption, andfile_sharesubtype with empty text. Both verify theContentPart::Imagestructure correctly.
Add a timeout around Slack file downloads, sanitize polling transport errors, and reuse the shared Slack API error helper for metadata failures.
yacosta738
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/channels/slack.rs`:
- Around line 202-208: The call to Slack's files.info (the HTTP request built in
the client.get(...).bearer_auth(...).query(&[("file", file_id)]).send().await)
is executed before the 30s timeout started in resolve_file_download_url(), so a
stalled metadata call can hang ingestion; move or wrap the files.info request
and its response handling inside the same timeout used by
resolve_file_download_url() (i.e., ensure the await for files.info is performed
within the tokio::time::timeout span used by resolve_file_download_url()), avoid
unnecessary cloning of file_id or bot_token when passing into the timeout
closure, and propagate any timeout error upward so the Slack image ingestion
path is bounded end-to-end.
- Around line 68-80: The auth.test bot-ID lookup currently swallows
transport/auth errors by calling .ok() and returning None; change the auth.test
call in Slack client so it returns a Result (propagate the transport/auth error
using the sanitize_error message rather than collapsing to None) and validate
the response payload to return Err if user_id is missing or malformed rather
than Ok(None). Update the caller (listen()) to abort/return an error when the
bot-ID lookup yields None (do not convert None to empty string), so self-message
filtering remains enabled on failures; refer to the auth.test request block that
uses
self.client.get(self.api_url("auth.test")).bearer_auth(&self.bot_token).send().await.inspect_err(...).ok()?
and the listen() method where the bot ID is used.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d1081e3-c1b2-4967-a215-c8be47df8ba9
📒 Files selected for processing (1)
clients/agent-runtime/src/channels/slack.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pr-checks
- GitHub Check: sonar
- GitHub Check: submit-gradle
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (5)
clients/agent-runtime/src/channels/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Implement
Channeltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests
Files:
clients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/channels/slack.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/channels/slack.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/channels/slack.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/channels/slack.rs
🧠 Learnings (2)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
Applied to files:
clients/agent-runtime/src/channels/slack.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/channels/slack.rs
🔇 Additional comments (1)
clients/agent-runtime/src/channels/slack.rs (1)
210-213: The suggested fix has incorrect syntax;&strhas no.get()method for byte ranges.While UTF-8 panic on
&file_id[..file_id.len().min(8)]is a valid code smell, Slack file IDs are always ASCII (e.g., "F123" format), so the practical risk is minimal. If defensive truncation is desired, use.chars().take(8).collect::<String>()instead.
| let response = self | ||
| .client | ||
| .get("https://slack.com/api/auth.test") | ||
| .get(self.api_url("auth.test")) | ||
| .bearer_auth(&self.bot_token) | ||
| .send() | ||
| .await | ||
| .ok()? | ||
| .inspect_err(|err| { | ||
| tracing::warn!( | ||
| "Slack auth.test request failed: {}", | ||
| self.sanitize_error(err) | ||
| ); | ||
| }) | ||
| .ok()?; |
There was a problem hiding this comment.
Don't collapse bot-ID lookup failures into None.
Line 80 and Lines 96-101 make transport/auth failures indistinguishable from “no user_id present”. listen() still converts that to "", which disables self-message filtering and can let the bot consume its own posts after an auth.test failure. Please surface an error here, or explicitly abort listen() when this returns None.
Based on learnings: "Implement Channel trait in src/channels/ with consistent send, listen, and health_check semantics and cover auth/allowlist/health behavior with tests".
Also applies to: 96-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/channels/slack.rs` around lines 68 - 80, The
auth.test bot-ID lookup currently swallows transport/auth errors by calling
.ok() and returning None; change the auth.test call in Slack client so it
returns a Result (propagate the transport/auth error using the sanitize_error
message rather than collapsing to None) and validate the response payload to
return Err if user_id is missing or malformed rather than Ok(None). Update the
caller (listen()) to abort/return an error when the bot-ID lookup yields None
(do not convert None to empty string), so self-message filtering remains enabled
on failures; refer to the auth.test request block that uses
self.client.get(self.api_url("auth.test")).bearer_auth(&self.bot_token).send().await.inspect_err(...).ok()?
and the listen() method where the bot ID is used.
| let response = self | ||
| .client | ||
| .get(self.api_url("files.info")) | ||
| .bearer_auth(&self.bot_token) | ||
| .query(&[("file", file_id)]) | ||
| .send() | ||
| .await |
There was a problem hiding this comment.
Put files.info under the timeout too.
Line 251 starts the 30s budget only after resolve_file_download_url() has already awaited. A stalled metadata call on Lines 202-208 can still hang Slack image ingestion indefinitely, so this path is not actually bounded end-to-end.
Suggested fix
pub async fn fetch_and_stage_image(
&self,
file_id: &str,
declared_mime: Option<&str>,
max_bytes: u64,
) -> Result<media::StagedImage, media::ImageRejectionReason> {
- let download_url = self.resolve_file_download_url(file_id).await?;
tokio::time::timeout(SLACK_DOWNLOAD_TIMEOUT, async {
+ let download_url = self.resolve_file_download_url(file_id).await?;
let response = self
.client
.get(&download_url)
.bearer_auth(&self.bot_token)
.send()
@@
})
.await
.map_err(|_| {
tracing::warn!(
- "Slack file download timed out after {}s",
+ "Slack file fetch timed out after {}s",
SLACK_DOWNLOAD_TIMEOUT.as_secs()
);
media::ImageRejectionReason::FetchFailed
})?
}As per coding guidelines: "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency."
Also applies to: 251-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/channels/slack.rs` around lines 202 - 208, The call
to Slack's files.info (the HTTP request built in the
client.get(...).bearer_auth(...).query(&[("file", file_id)]).send().await) is
executed before the 30s timeout started in resolve_file_download_url(), so a
stalled metadata call can hang ingestion; move or wrap the files.info request
and its response handling inside the same timeout used by
resolve_file_download_url() (i.e., ensure the await for files.info is performed
within the tokio::time::timeout span used by resolve_file_download_url()), avoid
unnecessary cloning of file_id or bot_token when passing into the timeout
closure, and propagate any timeout error upward so the Slack image ingestion
path is bounded end-to-end.
|
1 participant
This pull request adds support for inbound image and file ingestion from Slack channels, making Slack a fully supported multimodal channel alongside Telegram, WhatsApp, and Discord. The changes include the implementation of Slack image/file fetching, staging, and parsing, as well as updates to configuration and documentation to reflect the new capability. Comprehensive tests have been added to ensure correct behavior and security.
Slack multimodal image/file ingestion support:
SlackChannel, including methods to fetch file metadata, download files, and stage images securely. [1] [2] [3] [4]stage_channel_imagesto support the "slack" channel, enabling image ingestion from Slack.Configuration and documentation updates:
README.mdto document required Slack OAuth scopes for image/file ingestion.Testing and code quality:
Security and maintainability improvements:
These changes collectively make Slack a first-class multimodal channel, allowing users to send images and files for processing securely and reliably.
Closes: ##329