-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: handle missing files in send_message_to_user #8105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import errno | ||
| import json | ||
| import os | ||
| import shlex | ||
|
|
@@ -118,6 +119,15 @@ async def _resolve_path_from_sandbox( | |
|
|
||
| return path, False | ||
|
|
||
| def _require_existing_file(self, resolved_path: str, original_path: str) -> str: | ||
| if os.path.isfile(resolved_path): | ||
| return resolved_path | ||
| raise FileNotFoundError( | ||
| errno.ENOENT, | ||
| os.strerror(errno.ENOENT), | ||
| original_path, | ||
| ) | ||
|
|
||
| async def call( | ||
| self, context: ContextWrapper[AstrAgentContext], **kwargs | ||
| ) -> ToolExecResult: | ||
|
|
@@ -157,6 +167,7 @@ async def call( | |
| local_path, _ = await self._resolve_path_from_sandbox( | ||
| context, path | ||
| ) | ||
| local_path = self._require_existing_file(local_path, path) | ||
|
Comment on lines
167
to
+170
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the suggested local_path = await self._resolve_and_require_file(context, path)References
|
||
| components.append(Comp.Image.fromFileSystem(path=local_path)) | ||
| elif url: | ||
| components.append(Comp.Image.fromURL(url=url)) | ||
|
|
@@ -169,6 +180,7 @@ async def call( | |
| local_path, _ = await self._resolve_path_from_sandbox( | ||
| context, path | ||
| ) | ||
| local_path = self._require_existing_file(local_path, path) | ||
|
Comment on lines
180
to
+183
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the suggested local_path = await self._resolve_and_require_file(context, path)References
|
||
| components.append(Comp.Record.fromFileSystem(path=local_path)) | ||
| elif url: | ||
| components.append(Comp.Record.fromURL(url=url)) | ||
|
|
@@ -181,6 +193,7 @@ async def call( | |
| local_path, _ = await self._resolve_path_from_sandbox( | ||
| context, path | ||
| ) | ||
| local_path = self._require_existing_file(local_path, path) | ||
|
Comment on lines
193
to
+196
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the suggested local_path = await self._resolve_and_require_file(context, path)References
|
||
| components.append(Comp.Video.fromFileSystem(path=local_path)) | ||
| elif url: | ||
| components.append(Comp.Video.fromURL(url=url)) | ||
|
|
@@ -199,6 +212,7 @@ async def call( | |
| local_path, _ = await self._resolve_path_from_sandbox( | ||
| context, path | ||
| ) | ||
| local_path = self._require_existing_file(local_path, path) | ||
|
Comment on lines
212
to
+215
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the suggested local_path = await self._resolve_and_require_file(context, path)References
|
||
| components.append(Comp.File(name=name, file=local_path)) | ||
| elif url: | ||
| components.append(Comp.File(name=name, url=url)) | ||
|
|
@@ -244,10 +258,19 @@ async def call( | |
| else: | ||
| return f"error: invalid session: {session}" | ||
|
|
||
| await context.context.context.send_message( | ||
| target_session, | ||
| MessageChain(chain=components), | ||
| ) | ||
| try: | ||
| await context.context.context.send_message( | ||
| target_session, | ||
| MessageChain(chain=components), | ||
| ) | ||
| except Exception as exc: | ||
| logger.warning( | ||
| "Failed to send proactive message to session %s: %s", | ||
| target_session, | ||
| exc, | ||
| exc_info=True, | ||
| ) | ||
| return f"error: {exc}" | ||
| return f"Message sent to session {target_session}" | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce code duplication across the different message types, consider combining the path resolution and existence check into a single helper method. This aligns with the general rule of refactoring similar functionality into shared helpers. Additionally, ensure that this new attachment handling functionality is accompanied by corresponding unit tests.
References