feat: agent file transfers#128
Open
andyMrtnzP wants to merge 10 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR replaces the standalone ChangesFile Transfer Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPServer as MCP Server (index.ts)
participant UploadRoute as POST /upload
participant BrowserlessAgent as browserless_agent
participant BrowserlessAPI
participant DownloadRoute as GET /download/:id
participant DownloadStore as download-store
rect rgba(100, 149, 237, 0.5)
note over Client,DownloadStore: File Upload Flow
Client->>UploadRoute: POST /upload (multipart file + token)
UploadRoute->>UploadRoute: resolveBrowserlessAuth → 401 if invalid
UploadRoute->>DownloadStore: storeDownload(filename, mimeType, bytes)
DownloadStore-->>UploadRoute: StoredDownload {id, path}
UploadRoute-->>Client: {ok, handle:"browserless-download://id", filename, size}
end
rect rgba(144, 238, 144, 0.5)
note over Client,DownloadStore: Agent Upload + Auto-Download Flow
Client->>BrowserlessAgent: uploadFile [{handle:"browserless-download://id"}]
BrowserlessAgent->>DownloadStore: getDownload(handle) → base64 content
BrowserlessAgent->>BrowserlessAPI: uploadFile with base64 content
BrowserlessAPI-->>BrowserlessAgent: result
BrowserlessAgent->>BrowserlessAPI: getDownloads (auto-drain)
BrowserlessAPI-->>BrowserlessAgent: DownloadEntry[]
BrowserlessAgent->>DownloadStore: storeDownload (persist captured bytes)
BrowserlessAgent-->>Client: Content[] with curl/path instructions
end
rect rgba(255, 165, 0, 0.5)
note over Client,DownloadStore: Single-use File Download Flow
Client->>DownloadRoute: GET /download/:id + token
DownloadRoute->>DownloadStore: consumeDownload(id) → single-use record
DownloadRoute->>DownloadRoute: readFile(record.path)
DownloadRoute-->>Client: file bytes (Content-Type, Content-Disposition)
DownloadRoute->>DownloadStore: rm(record.path)
end
MCPServer->>DownloadStore: clearSession(sessionId) on disconnect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (2)
README.md (1)
34-34: ⚡ Quick winVerify the browserless_agent file-transfer description is complete.
Line 34 now claims that browserless_agent handles "file upload/download (captured downloads auto-surface as handles; bytes never enter context)." This aligns with the changelog's intent, but ensure that the README's brevity does not omit important caveats:
- The stdio vs. httpStream transport distinction mentioned in the changelog (file saved locally in stdio, HTTP endpoints for httpStream) is not reflected here.
- The "auto-surface" behavior and its mechanics (e.g., when downloads appear, what metadata is included) should be explained or linked to the file-transfers skill documentation if users need to understand it.
Consider whether a brief note or link to the
file-transfersskill section would improve clarity for users unfamiliar with the feature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 34, The browserless_agent description on line 34 of README.md is incomplete regarding file transfer functionality. Expand the file upload/download description to clarify the distinction between stdio and httpStream transports and their different behaviors (file saved locally in stdio versus HTTP endpoints for httpStream). Additionally, explain the "auto-surface" behavior mechanics including when downloads appear and what metadata is included. Consider adding a brief reference or link to the file-transfers skill documentation to guide users who need detailed understanding of this feature.src/resources/download-route.ts (1)
56-59: 💤 Low valueConsider RFC 5987 encoding for non-ASCII filenames.
The current sanitization only strips double quotes, which handles the immediate injection risk. However, filenames with non-ASCII characters or semicolons may display incorrectly in some browsers. Consider using the
filename*=parameter with UTF-8 encoding for broader compatibility.♻️ Optional: Improved Content-Disposition header
+ const safeFilename = record.filename.replace(/["\\]/g, ''); + const encodedFilename = encodeURIComponent(record.filename); c.header( 'Content-Disposition', - `attachment; filename="${record.filename.replace(/"/g, '')}"`, + `attachment; filename="${safeFilename}"; filename*=UTF-8''${encodedFilename}`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resources/download-route.ts` around lines 56 - 59, The Content-Disposition header in the download-route.ts file currently only strips double quotes from the filename, which doesn't properly handle non-ASCII characters or special characters like semicolons that may cause display issues in some browsers. Update the header construction to use RFC 5987 encoding by adding a filename* parameter with UTF-8 encoding alongside the standard filename parameter. This involves encoding the filename using appropriate character encoding and using the filename*= format as specified in RFC 5987 for better cross-browser compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/skills/file-transfers.md`:
- Line 81: The inline code span `< ` in the selector description line contains
invalid internal spacing that violates markdownlint rule MD038. Remove the space
inside the code span or reword the sentence to eliminate the space-padded inline
code. Instead of using backticks around `< `, restructure the text to describe
the deep selector syntax without spaces inside the code formatting, such as
using a different phrasing that avoids the problematic spacing pattern entirely.
In `@src/tools/agent.ts`:
- Around line 177-183: The code at line 177 interpolates the resolved `token`
value directly into the `tokenQ` variable, which then gets included in the error
message text. This can leak the actual Browserless API key to the user or model
output. Instead of interpolating the actual token value with `?token=${token ??
'<YOUR_BROWSERLESS_TOKEN>'}`, use only the placeholder
`?token=<YOUR_BROWSERLESS_TOKEN>` unconditionally so the real token is never
embedded in the error message. Alternatively, refactor the curl example to use
an Authorization header approach with Bearer token instead of query parameters,
which avoids the token in the URL entirely.
In `@src/tools/schemas.ts`:
- Around line 466-507: The file source validation in the z.object() schema for
the files array item currently allows zero or multiple sources to pass
validation, deferring failures to runtime. Add a schema-level constraint using
.refine() or .superRefine() to the z.object() that validates exactly one of the
three fields (content, handle, or path) is provided. The constraint should count
the number of defined fields among these three and ensure exactly one is
present, throwing a validation error with a clear message if zero or multiple
are provided.
---
Nitpick comments:
In `@README.md`:
- Line 34: The browserless_agent description on line 34 of README.md is
incomplete regarding file transfer functionality. Expand the file
upload/download description to clarify the distinction between stdio and
httpStream transports and their different behaviors (file saved locally in stdio
versus HTTP endpoints for httpStream). Additionally, explain the "auto-surface"
behavior mechanics including when downloads appear and what metadata is
included. Consider adding a brief reference or link to the file-transfers skill
documentation to guide users who need detailed understanding of this feature.
In `@src/resources/download-route.ts`:
- Around line 56-59: The Content-Disposition header in the download-route.ts
file currently only strips double quotes from the filename, which doesn't
properly handle non-ASCII characters or special characters like semicolons that
may cause display issues in some browsers. Update the header construction to use
RFC 5987 encoding by adding a filename* parameter with UTF-8 encoding alongside
the standard filename parameter. This involves encoding the filename using
appropriate character encoding and using the filename*= format as specified in
RFC 5987 for better cross-browser compatibility.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a5a2e64-ae83-4868-9c73-553a04dc1d67
📒 Files selected for processing (20)
CHANGELOG.mdREADME.mdsrc/@types/types.d.tssrc/index.tssrc/lib/download-store.tssrc/lib/http-auth.tssrc/resources/download-route.tssrc/resources/upload-route.tssrc/skills/file-transfers.mdsrc/skills/index.tssrc/skills/system-prompt.tssrc/tools/agent.tssrc/tools/download.tssrc/tools/schemas.tstest/lib/download-store.spec.tstest/lib/http-auth.spec.tstest/skills/skills.spec.tstest/tools/agent.spec.tstest/tools/annotations.spec.tstest/tools/download.spec.ts
💤 Files with no reviewable changes (3)
- test/tools/download.spec.ts
- src/tools/download.ts
- test/tools/annotations.spec.ts
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
1 participant
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 by CodeRabbit
Release Notes
file-transfersskill for detecting and guiding file upload/download workflows in agent runs.