fix(cua): use native file interfaces for uploads#8069
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_normalize_native_upload_result, consider guarding against non-mapping payloads from_maybe_model_dump(e.g., booleans or strings) so that accessing keys like"file_path"and"success"does not raise when a native upload returns a primitive instead of a dict/model. - The new
_resolve_files_components/_resolve_files_methodhelpers are now called on every filesystem operation; if these code paths are hot, you may want to cache the resolved methods per operation (e.g., inCuaFileSystemComponentandCuaBooter) to avoid repeatedly walking the components tuple.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_normalize_native_upload_result`, consider guarding against non-mapping payloads from `_maybe_model_dump` (e.g., booleans or strings) so that accessing keys like `"file_path"` and `"success"` does not raise when a native upload returns a primitive instead of a dict/model.
- The new `_resolve_files_components`/`_resolve_files_method` helpers are now called on every filesystem operation; if these code paths are hot, you may want to cache the resolved methods per operation (e.g., in `CuaFileSystemComponent` and `CuaBooter`) to avoid repeatedly walking the components tuple.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions to standardize file system operations and upload result normalization within the CuaShellComponent, along with corresponding unit tests. The changes effectively decouple file system interactions and improve robustness. The reviewer provided actionable feedback suggesting an improvement to the file_path mapping logic in the normalization helper and recommended refactoring legacy upload calls to utilize this new helper for improved consistency.
| if "file_path" not in payload and "path" not in payload: | ||
| payload["file_path"] = file_name |
There was a problem hiding this comment.
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.
| if "file_path" not in payload and "path" not in payload: | |
| payload["file_path"] = file_name | |
| if "file_path" not in payload: | |
| payload["file_path"] = payload.get("path") or file_name |
References
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
| return _maybe_model_dump( | ||
| await sandbox.upload_file(str(local_path), file_name) | ||
| ) |
There was a problem hiding this comment.
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
- When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
Summary
files.uploadandfiles.write_bytesinterfaces before falling back to shell base64 writessandbox.filesand legacysandbox.filesystemso mixed SDK environments keep working without regressing to shell behaviorfiles/filesystemcompatibilityProblem
Issue #8054 failed after the CUA sandbox booted, during skill bundle sync. AstrBot packed local skills into
skills.zip, thenCuaBooter.upload_file()fell back to embedding the whole base64 payload into one shell command. On larger bundles that command exceeded the remote command length limit and the sandbox returned[Errno 7] Argument list too long.Root Cause
The CUA adapter only checked
sandbox.upload_fileand otherwise used the shell fallback. The current CUA SDK exposes file transfer and file IO throughsandbox.files, while some older environments may still exposesandbox.filesystem. Because AstrBot was not using those interfaces, it missed the native upload path and overused shell-based file writes.Fix
This change teaches the CUA adapter to discover file methods across both
sandbox.filesandsandbox.filesystemon a per-method basis.For uploads,
CuaBooter.upload_file()now tries:sandbox.upload_filefiles.uploadfiles.write_bytesFor filesystem operations,
CuaFileSystemComponentnow resolves each operation against both modern and legacy interfaces so mixed SDK environments still use the native method that actually exists.The native upload path also now preserves structured failure payloads instead of always reporting success.
Validation
uv run pytest tests/test_computer_skill_sync.py tests/unit/test_cua_computer_use.py -quv run ruff check astrbot/core/computer/booters/cua.py tests/unit/test_cua_computer_use.pySummary by Sourcery
Prefer native CUA sandbox file interfaces for uploads and filesystem operations before falling back to shell-based behavior.
New Features:
Bug Fixes:
Tests: