-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(cua): use native file interfaces for uploads #8069
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
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 |
|---|---|---|
|
|
@@ -232,6 +232,43 @@ def _has_component_method(root: Any, component_name: str, method_name: str) -> b | |
| return getattr(component, method_name, None) is not None | ||
|
|
||
|
|
||
| def _resolve_files_components(sandbox: Any) -> tuple[Any, ...]: | ||
| components: list[Any] = [] | ||
| seen_ids: set[int] = set() | ||
| for name in ("files", "filesystem"): | ||
| component = getattr(sandbox, name, None) | ||
| if component is None: | ||
| continue | ||
| component_id = id(component) | ||
| if component_id in seen_ids: | ||
| continue | ||
| seen_ids.add(component_id) | ||
| components.append(component) | ||
| return tuple(components) | ||
|
|
||
|
|
||
| def _resolve_files_method( | ||
| components: tuple[Any, ...], | ||
| method_names: str | tuple[str, ...], | ||
| ) -> Any | None: | ||
| for component in components: | ||
| method = _resolve_component_method(component, method_names) | ||
| if method is not None: | ||
| return method | ||
| return None | ||
|
|
||
|
|
||
| def _normalize_native_upload_result(raw: Any, file_name: str) -> dict[str, Any]: | ||
| payload = _maybe_model_dump(raw) | ||
| if not payload: | ||
| return {"success": True, "file_path": file_name} | ||
| if "file_path" not in payload and "path" not in payload: | ||
| payload["file_path"] = file_name | ||
| if "success" not in payload: | ||
| payload["success"] = not bool(payload.get("error") or payload.get("stderr")) | ||
| return payload | ||
|
|
||
|
|
||
| class CuaShellComponent(ShellComponent): | ||
| def __init__(self, sandbox: Any, os_type: str = "linux") -> None: | ||
| self._sandbox = sandbox | ||
|
|
@@ -360,7 +397,7 @@ def __init__( | |
| self, sandbox: Any, os_type: str = CUA_DEFAULT_CONFIG["os_type"] | ||
| ) -> None: | ||
| self._shell = CuaShellComponent(sandbox, os_type=os_type) | ||
| self._fs = getattr(sandbox, "filesystem", None) | ||
| self._fs_components = _resolve_files_components(sandbox) | ||
| self._os_type = os_type.lower() | ||
| self._fallback = _PosixShellFileSystem(self._shell, self._os_type) | ||
|
|
||
|
|
@@ -382,7 +419,9 @@ async def read_file( | |
| offset: int | None = None, | ||
| limit: int | None = None, | ||
| ) -> dict[str, Any]: | ||
| read_file = None if self._fs is None else getattr(self._fs, "read_file", None) | ||
| read_file = _resolve_files_method( | ||
| self._fs_components, ("read_file", "read_text") | ||
| ) | ||
| if read_file is None: | ||
| return await self._fallback.read_file(path, encoding, offset, limit) | ||
| else: | ||
|
|
@@ -405,19 +444,19 @@ async def write_file( | |
| encoding: str = "utf-8", | ||
| ) -> dict[str, Any]: | ||
| _ = mode | ||
| write_file = None if self._fs is None else getattr(self._fs, "write_file", None) | ||
| write_file = _resolve_files_method( | ||
| self._fs_components, ("write_file", "write_text") | ||
| ) | ||
| if write_file is None: | ||
| return await self._fallback.write_file(path, content, mode, encoding) | ||
| else: | ||
| await _maybe_await(write_file(path, content)) | ||
| return {"success": True, "path": path} | ||
|
|
||
| async def delete_file(self, path: str) -> dict[str, Any]: | ||
| delete = None | ||
| if self._fs is not None: | ||
| delete = getattr(self._fs, "delete", None) or getattr( | ||
| self._fs, "delete_file", None | ||
| ) | ||
| delete = _resolve_files_method( | ||
| self._fs_components, ("delete", "delete_file", "remove") | ||
| ) | ||
| if delete is None: | ||
| return await self._fallback.delete_file(path) | ||
| else: | ||
|
|
@@ -429,7 +468,7 @@ async def list_dir( | |
| path: str = ".", | ||
| show_hidden: bool = False, | ||
| ) -> dict[str, Any]: | ||
| list_dir = None if self._fs is None else getattr(self._fs, "list_dir", None) | ||
| list_dir = _resolve_files_method(self._fs_components, ("list_dir", "list")) | ||
| if list_dir is not None: | ||
| entries = await _maybe_await(list_dir(path)) | ||
| return {"success": True, "path": path, "entries": entries} | ||
|
|
@@ -802,6 +841,15 @@ async def upload_file(self, path: str, file_name: str) -> dict: | |
| return _maybe_model_dump( | ||
| await sandbox.upload_file(str(local_path), file_name) | ||
| ) | ||
|
Comment on lines
841
to
843
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. For consistency and to strictly adhere to the upload_file contract (which requires success and file_path keys), the legacy sandbox.upload_file result should also be processed by _normalize_native_upload_result. This refactors the logic into a shared helper function to avoid code duplication and ensures that new attachment handling functionality is properly normalized and tested. result = await sandbox.upload_file(str(local_path), file_name)
return _normalize_native_upload_result(result, file_name)References
|
||
| files_components = () if sandbox is None else _resolve_files_components(sandbox) | ||
| upload = _resolve_files_method(files_components, "upload") | ||
| if upload is not None: | ||
| result = await _maybe_await(upload(str(local_path), file_name)) | ||
| return _normalize_native_upload_result(result, file_name) | ||
| write_bytes = _resolve_files_method(files_components, "write_bytes") | ||
| if write_bytes is not None: | ||
| result = await _maybe_await(write_bytes(file_name, local_path.read_bytes())) | ||
| return _normalize_native_upload_result(result, file_name) | ||
| if not _is_posix_os_type(self.os_type): | ||
| return _non_posix_filesystem_result(file_name, self.os_type) | ||
| result = await _write_base64_via_shell( | ||
|
|
||
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.
The current logic fails to ensure the file_path key is present if the payload contains a path key instead. According to the ComputerBooter.upload_file contract, the returned dictionary must contain a file_path key. If path is present in the native response, it should be mapped to file_path to satisfy the interface requirements. Additionally, as this handles file attachments, ensure this logic is accompanied by unit tests.
References