✨ Feat: Add presigned URL support for external MCP tool file access and improve agent execution flow#2839
✨ Feat: Add presigned URL support for external MCP tool file access and improve agent execution flow#2839
Conversation
… improve agent execution flow
There was a problem hiding this comment.
Pull request overview
This PR adds presigned download URL support (default 24h) for externally hosted tools (e.g., MCP) to access uploaded files, updates agent prompts/execution guidance to avoid fabricated “observation” content, and extends agent context so historical attachments from conversation history are available during runs.
Changes:
- Backend: generate/propagate presigned URLs on upload, and extend default URL expiry from 1h → 24h across storage URL helpers/endpoints.
- Agent/runtime: include historical
minio_filesin the agent’s prompt context and update prompts to rely on system-providedObservation:outputs. - Frontend: persist/transmit
presigned_urlon attachments and pass historical attachment metadata inhistory.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/backend/database/test_attachment_db.py | Updates upload tests to cover presigned URL behavior and new flags. |
| frontend/types/chat.ts | Extends attachment types to include presigned_url. |
| frontend/lib/chatMessageExtractor.ts | Preserves presigned_url when hydrating messages from API responses. |
| frontend/lib/chat/chatAttachmentUtils.ts | Captures presigned URLs from upload results and attaches them to outgoing message metadata. |
| frontend/app/[locale]/chat/internal/chatInterface.tsx | Sends presigned URLs + historical attachments in the agent run payload. |
| backend/prompts/utils/prompt_generate_zh.yaml | Removes manual “Observe Results” pattern; relies on system Observation: marker. |
| backend/prompts/utils/prompt_generate_en.yaml | Same as above for English. |
| backend/prompts/manager_system_prompt_template_zh.yaml | Adds [MCP] markers and file URL usage guidance in manager prompt. |
| backend/prompts/manager_system_prompt_template_en.yaml | Same as above for English. |
| backend/prompts/managed_system_prompt_template_zh.yaml | Adds [MCP] markers and file URL usage guidance in managed prompt. |
| backend/prompts/managed_system_prompt_template_en.yaml | Same as above for English. |
| backend/database/client.py | Changes default presigned URL expiry to 24h. |
| backend/database/attachment_db.py | Adds presigned URL generation to uploads; updates URL expiry defaults and list behavior. |
| backend/apps/file_management_app.py | Updates API defaults/docs for URL expiry to 24h. |
| backend/agents/create_agent_info.py | Appends both S3 and presigned URLs to query context and includes historical files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @patch('backend.database.attachment_db.os.path.exists') | ||
| @patch('backend.database.attachment_db.os.path.getsize') | ||
| @patch('backend.database.attachment_db.os.path.basename') | ||
| def test_upload_file_nonexistent_file(self, mock_basename, mock_getsize, mock_exists): | ||
| """Test upload_file with nonexistent file""" | ||
| @patch('backend.database.attachment_db.get_file_url') | ||
| def test_upload_file_without_presigned_url(self, mock_get_file_url, mock_basename, mock_getsize, mock_exists): | ||
| """Test upload_file when generate_presigned_url is False""" | ||
| mock_basename.return_value = 'test.txt' | ||
| mock_exists.return_value = False | ||
| mock_getsize.return_value = 0 | ||
| mock_exists.return_value = True | ||
| mock_getsize.return_value = 1024 | ||
| minio_client_mock.upload_file.return_value = (True, '/bucket/attachments/test.txt') | ||
|
|
||
| result = upload_file('/path/to/test.txt', 'attachments/test.txt', 'bucket', generate_presigned_url=False) | ||
|
|
||
| assert result['success'] is True | ||
| assert 'url' in result | ||
| assert 'presigned_url' not in result | ||
| mock_get_file_url.assert_not_called() | ||
|
|
There was a problem hiding this comment.
The previous test that covered the “nonexistent file” path for upload_file() appears to have been replaced. That branch (file_size falling back to 0 when os.path.exists is false) is still present in upload_file(), so it would be good to keep a dedicated test for the nonexistent-file scenario in addition to the new presigned-URL tests.
| @patch('backend.database.attachment_db.generate_object_name') | ||
| def test_upload_fileobj_preserves_file_position(self, mock_generate): | ||
| @patch('backend.database.attachment_db.get_file_url') | ||
| def test_upload_fileobj_preserves_file_position(self, mock_get_file_url, mock_generate): | ||
| """Test upload_fileobj preserves original file position""" | ||
| mock_generate.return_value = 'attachments/test.txt' | ||
| minio_client_mock.upload_fileobj.return_value = (True, '/bucket/attachments/test.txt') | ||
| mock_get_file_url.return_value = {'success': True, 'url': 'http://minio:9000/presigned-url'} | ||
|
|
||
| file_obj = BytesIO(b'test data') | ||
| original_pos = 4 | ||
| file_obj.seek(original_pos) | ||
|
|
||
| result = upload_fileobj(file_obj, 'test.txt', 'bucket') | ||
|
|
||
| # File position should be restored | ||
| assert file_obj.tell() == original_pos |
There was a problem hiding this comment.
test_upload_fileobj_preserves_file_position verifies the stream position is restored, but it doesn’t assert that the upload reads from the beginning of the file (it uses a stubbed upload_fileobj that never reads). Since upload_fileobj() currently passes the stream through as-is, a non-zero starting position could lead to truncated uploads without this test catching it. Consider strengthening this test by using an upload_fileobj mock that reads file_obj and asserting the full content was uploaded while still restoring the original position.
| .map((msg) => { | ||
| const historyItem: any = { | ||
| role: msg.role, | ||
| content: | ||
| msg.role === ROLE_ASSISTANT | ||
| ? msg.finalAnswer?.trim() || msg.content || "" | ||
| : msg.content || "", | ||
| }; |
There was a problem hiding this comment.
The new history mapping uses any for historyItem, which drops type safety for the request payload (and makes it easy to accidentally send malformed history). Consider introducing an explicit type for the history items (including optional minio_files) instead of any.
| if (msg.attachments && msg.attachments.length > 0) { | ||
| historyItem.minio_files = msg.attachments.map((attachment) => ({ | ||
| object_name: attachment.object_name || "", | ||
| name: attachment.name, | ||
| type: attachment.type, | ||
| size: attachment.size, | ||
| url: attachment.url || "", | ||
| presigned_url: attachment.presigned_url || "", | ||
| description: attachment.description || "", | ||
| })); |
There was a problem hiding this comment.
When constructing historyItem.minio_files, optional fields are forced to empty strings (""). This increases payload size and makes it harder for the backend to distinguish “missing” vs “present but empty”. Prefer leaving fields as undefined (so they’re omitted from JSON) or conditionally adding keys only when values exist.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Feat: Add presigned URL support for external MCP tool file access and improve agent execution flow
upload_file()andupload_fileobj()to generate presigned URLsgenerate_presigned_urlandpresigned_url_expiresparametersUpdate agent prompts to prevent fabricated observation results(避免在思考阶段,模型没有执行代码,但模拟输出了结果造成模型幻觉的问题)
Include historical file attachments in agent context