fix(api): normalize agent attachments through storage fixes NV-7456#10942
Merged
fix(api): normalize agent attachments through storage fixes NV-7456#10942
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
github-actions
Bot
changed the title
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:
📝 WalkthroughWalkthroughAdds AgentAttachmentStorage to validate, upload, and sign inbound attachments; extends StorageService with read-signed URL and existence checks; wires stored-attachment persistence and signing into AgentInboundHandler and BridgeExecutor (including reactions); adds Slack Changes
Sequence DiagramsequenceDiagram
participant Client
participant InboundHandler as AgentInboundHandler
participant AttachmentStorage as AgentAttachmentStorage
participant StorageProvider as StorageService (S3/GCS/Azure)
participant BridgeExecutor as BridgeExecutor
participant Bridge as Bridge
Client->>InboundHandler: inbound message with attachments
InboundHandler->>AttachmentStorage: storeInbound(attachments, context)
AttachmentStorage->>AttachmentStorage: validate, convert to Buffer, enforce caps
AttachmentStorage->>StorageProvider: upload(storageKey, data, contentType)
StorageProvider-->>AttachmentStorage: upload confirmation
AttachmentStorage->>StorageProvider: getReadSignedUrl(storageKey, ttl)
StorageProvider-->>AttachmentStorage: signed URL or error
AttachmentStorage-->>InboundHandler: StoredAttachment[] (storageKey, url?, metadata)
InboundHandler->>BridgeExecutor: buildPayload(message, storedAttachments)
BridgeExecutor->>AttachmentStorage: signRead(storageKey) for history/stored attachments
AttachmentStorage->>StorageProvider: fileExists(storageKey)
StorageProvider-->>AttachmentStorage: exists boolean
AttachmentStorage->>StorageProvider: getReadSignedUrl(...) if exists
StorageProvider-->>AttachmentStorage: signed URL or null
BridgeExecutor-->>Bridge: final payload with signed attachment URLs
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: 3
🧹 Nitpick comments (1)
packages/shared/src/consts/slack-agent-oauth-scopes.ts (1)
5-10: Plan a reconnect path for existing Slack installs.Adding
files:readchanges the Slack grant set. Already-connected workspaces will keep tokens without this scope until they re-authorize, so attachment downloads can still fail after deploy unless the rollout forces or clearly prompts a reconnect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/consts/slack-agent-oauth-scopes.ts` around lines 5 - 10, SLACK_AGENT_OAUTH_SCOPES was expanded to include 'files:read', which changes the OAuth grant set and will leave existing workspace tokens without that scope; add a reconnect/migration path by detecting missing scope when attempting attachment downloads (e.g., check token scopes or catch 403/insufficient_scope errors in the download flow), surface a clear reauthorization prompt or webhook to the workspace admin, and implement a retry after successful reauth; reference SLACK_AGENT_OAUTH_SCOPES and the 'files:read' scope and ensure the code that performs attachment downloads gracefully falls back and triggers the reauthorize flow when the scope is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/services/agent-attachment-storage.service.ts`:
- Around line 161-175: Split the upload and signing into two separate try/catch
blocks so a transient getReadSignedUrl failure doesn't discard the stored blob:
call this.storageService.uploadFile(storageKey, buffer, mimeType) inside its own
try and ensure you always construct and return the attachment metadata (type,
name, mimeType, size, storageKey) even if signing fails; then call
this.storageService.getReadSignedUrl(storageKey, READ_URL_TTL_SECONDS) in a
separate try, set url only if successful, and log a warning on signing errors
without returning null or deleting the uploaded object.
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts`:
- Around line 271-289: The reaction handler currently always calls
attachmentStorage.storeInbound and re-uploads event.message.attachments;
instead, find the corresponding source activity in the already-loaded history
(use the same id matching logic you use elsewhere for reactions, e.g., match
event.messageId or event.message.id against entries in history) and reuse its
persisted StoredAttachment metadata (storageKey etc.) to populate
sourceMessageStoredAttachments on the reactionPayload; only call
attachmentStorage.storeInbound when no stored attachments are found on the
history entry. Update the logic around
sourceMessageStoredAttachments/attachmentStorage.storeInbound and the
construction of reactionPayload so it prefers history-stored attachments and
falls back to storeInbound.
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 337-355: The current branch drops legacy attachments that lack
storageKey; modify the logic in the block that checks storageKey so that when
storageKey is a non-empty string you call signAttachmentForHistory(activityId)
and return the signed object (as now), but when storageKey is missing/empty you
should fall back to returning the existing attachment shape if a usable att.url
exists (preserve type, url, name, mimeType, size) instead of logging and
returning null; update the handling around storageKey, signAttachmentForHistory,
and att to return the legacy att object when url is present and only sign when
storageKey is present.
---
Nitpick comments:
In `@packages/shared/src/consts/slack-agent-oauth-scopes.ts`:
- Around line 5-10: SLACK_AGENT_OAUTH_SCOPES was expanded to include
'files:read', which changes the OAuth grant set and will leave existing
workspace tokens without that scope; add a reconnect/migration path by detecting
missing scope when attempting attachment downloads (e.g., check token scopes or
catch 403/insufficient_scope errors in the download flow), surface a clear
reauthorization prompt or webhook to the workspace admin, and implement a retry
after successful reauth; reference SLACK_AGENT_OAUTH_SCOPES and the 'files:read'
scope and ensure the code that performs attachment downloads gracefully falls
back and triggers the reauthorize flow when the scope is absent.
🪄 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: CHILL
Plan: Pro
Run ID: b0ba4e5d-b939-45dd-adc4-89118d8fb331
📒 Files selected for processing (8)
apps/api/src/app/agents/agents.module.tsapps/api/src/app/agents/services/agent-attachment-storage.service.spec.tsapps/api/src/app/agents/services/agent-attachment-storage.service.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.spec.tsapps/api/src/app/agents/services/bridge-executor.service.tslibs/application-generic/src/services/storage/storage.service.tspackages/shared/src/consts/slack-agent-oauth-scopes.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
commit: |
Contributor
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 `@apps/api/src/app/agents/services/agent-attachment-storage.service.ts`:
- Around line 46-64: In bufferFromAttachment ensure the 25MB byte cap is
enforced before allocating a Buffer: when attachment.fetchData or
blob.arrayBuffer would be used, either stream the data and accumulate while
rejecting if total bytes exceed the 25*1024*1024 limit, or pre-check
attachment.size and reject immediately if >25MB or missing/untrusted; update the
logic in bufferFromAttachment (and the other similar attachment-handling block
referenced later) to refuse downloads when size cannot be trusted rather than
calling fetchData()/arrayBuffer() unguarded, and surface an appropriate error
instead of returning a large Buffer.
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 239-248: The mapped.attachments mapping must not reuse
StoredAttachment.url; instead, re-sign each attachment using its storageKey
(e.g., call your storage signing helper in bridge-executor.service.ts) and build
attachment objects from the fresh signed URL plus type/name/mimeType/size; if
storageKey is missing or signing fails for an entry, omit that attachment from
mapped.attachments. Update the block that currently maps storedAttachments (the
mapped.attachments = storedAttachments.map(...) section) to derive URL via the
storage signing API, filter out entries that cannot be signed, and return the
filtered list.
🪄 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: CHILL
Plan: Pro
Run ID: b1cced5f-c7a5-47e9-ac71-495c290662c3
📒 Files selected for processing (6)
apps/api/src/app/agents/services/agent-attachment-storage.service.spec.tsapps/api/src/app/agents/services/agent-attachment-storage.service.tsapps/api/src/app/agents/services/agent-inbound-handler.service.spec.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.spec.tsapps/api/src/app/agents/services/bridge-executor.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/app/agents/services/agent-attachment-storage.service.spec.ts
- apps/api/src/app/agents/services/agent-inbound-handler.service.ts
Contributor
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 `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 323-378: The nested Promise.all in mapHistory and
mapRichContentForBridge causes unbounded parallel signing requests; limit
concurrency by replacing the raw Promise.all calls with a concurrency-limited
runner (e.g., use p-limit or a small async pool) so attachments are signed only
N at a time (choose a small number like 4); apply the limiter inside
mapRichContentForBridge when mapping rawAttachments (and also when mapping
activities in mapHistory if you want to cap parallel calls to
mapRichContentForBridge), invoking signAttachmentForHistory through the limiter
so storage signing requests are throttled and overall API-path latency/pressure
is controlled.
- Around line 353-376: The mapping assumes every rawAttachments item is a
non-null object and reads att.storageKey directly, which can throw for
null/primitive entries; update the Promise.all mapping in
bridge-executor.service.ts to first guard the item (e.g., if item == null or
typeof item !== 'object') and return null (and optionally warn with activityId),
then safely cast to Record<string, unknown> and proceed to check storageKey as
currently done before calling this.signAttachmentForHistory; ensure the same
guards are applied before accessing att.type/name/mimeType/size to avoid runtime
errors.
🪄 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: CHILL
Plan: Pro
Run ID: 0c0e700a-ce7c-468f-8f13-41f041335ce0
📒 Files selected for processing (4)
apps/api/src/app/agents/services/agent-attachment-storage.service.spec.tsapps/api/src/app/agents/services/agent-attachment-storage.service.tsapps/api/src/app/agents/services/bridge-executor.service.spec.tsapps/api/src/app/agents/services/bridge-executor.service.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/app/agents/services/agent-attachment-storage.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/app/agents/services/bridge-executor.service.spec.ts
- apps/api/src/app/agents/services/agent-attachment-storage.service.ts
Co-authored-by: Dima Grossman <dima@grossman.io>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/api/src/app/agents/services/bridge-executor.service.ts (1)
348-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe history throttle still multiplies across activities.
mapHistory()runs 4 activities in parallel, and eachmapRichContentForBridge()runs up to 4 attachment-signing tasks in parallel again. That means one payload build can still issue up to 16 concurrentsignRead()calls, so the new cap is not a real global throttle on the API path.Possible local fix
- return await mapWithConcurrency(reversed, ATTACHMENT_SIGNING_CONCURRENCY, async (activity) => ({ - role: activity.senderType, - type: activity.type, - content: activity.content, - richContent: await this.mapRichContentForBridge(activity.richContent, activity), - senderName: activity.senderName || undefined, - signalData: activity.signalData || undefined, - createdAt: activity.createdAt, - })); + const mapped: AgentHistoryEntry[] = []; + for (const activity of reversed) { + mapped.push({ + role: activity.senderType, + type: activity.type, + content: activity.content, + richContent: await this.mapRichContentForBridge(activity.richContent, activity), + senderName: activity.senderName || undefined, + signalData: activity.signalData || undefined, + createdAt: activity.createdAt, + }); + } + + return mapped;If you want to keep activity-level parallelism, the safer alternative is a single shared limiter that is passed down and used for every
signAttachmentForHistory()call instead of nesting two separate pools.Also applies to: 373-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 348 - 352, The history-level concurrency currently nests mapWithConcurrency in mapHistory with per-activity attachment signing inside mapRichContentForBridge, so ATTACHMENT_SIGNING_CONCURRENCY multiplies across activities; fix by creating a single shared limiter (e.g., a semaphore or limiter instance) in mapHistory and pass that limiter into mapRichContentForBridge and ultimately into signAttachmentForHistory/signRead so every attachment-signing call uses the same global limiter instead of creating new per-activity pools; update signatures of mapRichContentForBridge and signAttachmentForHistory (and any callers) to accept the limiter and replace internal mapWithConcurrency for attachment signing with limiter.run/await to enforce a true global cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 348-352: The history-level concurrency currently nests
mapWithConcurrency in mapHistory with per-activity attachment signing inside
mapRichContentForBridge, so ATTACHMENT_SIGNING_CONCURRENCY multiplies across
activities; fix by creating a single shared limiter (e.g., a semaphore or
limiter instance) in mapHistory and pass that limiter into
mapRichContentForBridge and ultimately into signAttachmentForHistory/signRead so
every attachment-signing call uses the same global limiter instead of creating
new per-activity pools; update signatures of mapRichContentForBridge and
signAttachmentForHistory (and any callers) to accept the limiter and replace
internal mapWithConcurrency for attachment signing with limiter.run/await to
enforce a true global cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ab90beb-2b83-4e4d-b93f-8180addcf989
📒 Files selected for processing (2)
apps/api/src/app/agents/services/bridge-executor.service.spec.tsapps/api/src/app/agents/services/bridge-executor.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/agents/services/bridge-executor.service.spec.ts
Co-authored-by: Dima Grossman <dima@grossman.io>
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.
What changed? Why was the change needed?
What changed
Inbound agent attachments are now normalized: provider-protected media (e.g., private Slack/WhatsApp files) are fetched via chat adapters, uploaded into Novu storage, and agent bridges receive short‑lived signed read URLs (15‑minute TTL) instead of raw provider URLs. This prevents broken or credentialed links and ensures stable, secure access for agent bridges. The change is API-only and adds safeguards around size/count limits and graceful degradation when signing fails.
Affected areas
Key technical decisions
Testing
Added targeted unit tests covering attachment upload/signing limits and failure modes; verified with a focused package build and Mocha specs for agent-attachment-storage and bridge executor mapping.
Normalizes inbound agent attachments by fetching provider-protected media through the chat adapter, storing it in Novu storage, and passing short-lived signed URLs to agent bridges. This fixes Slack and WhatsApp attachment handling where platform URLs are missing or require provider credentials.
Also adds
files:readto Slack agent OAuth scopes so private Slack file downloads can succeed.Screenshots
Not applicable, API-only change.
Expand for optional sections
Related enterprise PR
None.
Special notes for your reviewer
Focused tests cover attachment upload/signing behavior and bridge history degradation when signing fails. Verified with:
pnpm --filter @novu/application-generic buildagent-attachment-storage.serviceandbridge-executor.service