Conversation
veithly
left a comment
There was a problem hiding this comment.
Code Review: PR #31 — Add runtime file system related methods
Test Results: 59/59 PASSED
Ran comprehensive E2E test suite (54 additional tests beyond the 5 included in the PR) in an isolated worktree checked out from origin/feat/fs_methods at eaec29e.
Test coverage areas:
- Core FS operations (9 tests): Full lifecycle write→read→stat→remove, mkdir recursive, list pagination, read with offset/limit, write append mode, rename with/without overwrite, remove recursive
- Path resolution (4 tests): Relative paths, workspace root, empty path,
/workspaceprefix - Security boundaries (10 tests):
/etc/passwd,/etc/shadow,/tmpwrite, path traversal via../../, symlink escape (both stat and read), rename source/dest outside workspace - Error handling (13 tests): Nonexistent files, directory-as-file, invalid/unknown encoding, negative offset, zero limit, mkdir on file, rename to non-empty dir
- Edge cases (8 tests): Unicode filenames/content, empty dirs/files, 100KB files, filenames with spaces, sort order, mtime format, mkdir idempotency
- Watch service (6 tests): File creation/modification/deletion events, cleanup on close, nonexistent/outside-workspace path rejection, unknown watch_id handling
- Protocol & integration (4 tests): ClientMethod enum completeness, ServerEvent.SANDBOX_FILE_CHANGED, handler wiring, py_compile validation
Code Quality Assessment: Good
Strengths:
- Clean separation:
WorkspaceFSService(sync ops viaasyncio.to_thread) andWorkspaceWatchService(async polling) are well-isolated - Solid path security:
resolve(strict=False)+relative_to()double-check prevents traversal attacks; symlinks that escape workspace are correctly blocked - WebSocket lifecycle: Watch resources are properly cleaned up on connection disconnect via
_cleanup_resources() - Comprehensive RPC coverage: All 9
fs.*methods +fs.watch/fs.unwatchare wired into the handler
Minor Suggestions (non-blocking)
-
DRY path resolution —
workspace_fs.py:279-299andworkspace_watch.py:73-100contain near-identical path resolution logic. Consider extracting a shared utility. -
Duplicate constant —
SANDBOX_WORKSPACE_ROOT = "/workspace"is defined in bothworkspace_fs.py:12andworkspace_watch.py:16. Unify to a single source. -
Watch error notification —
workspace_watch.py:118-119: when_watch_loopcatches a non-cancellation exception, it only logs a warning. Consider emitting an error event to the WebSocket client so the frontend can react. -
Workspace path fallback —
handler.py:246:getattr(agent, "workspace", Path.home() / ".spoon-bot" / "workspace")silently falls back to a home directory path. A log or validation check would help catch misconfiguration.
Verdict
Approve — well-structured implementation with strong security boundaries. The suggestions above are all S3 (nice-to-have) improvements that can be addressed in follow-up PRs.
Summary
Implement sandbox filesystem RPC support in the runtime and route file watching through
spoon-botinstead of relying on the API server's local filesystem view.Changes
fs.list,fs.stat,fs.read,fs.write,fs.mkdir,fs.rename, andfs.removefs.*methods andsandbox.file.changedWorkspaceFSServiceto execute filesystem operations inside the runtime workspace with/workspacepath semanticsfs.watch/fs.unwatchhandling in the websocket handler/workspace/...createdevents reliablyWhy
The previous
fs.watchflow depended on the API server being able to see a mirrored host-side workspace, which breaks in local and sandboxed environments. This PR moves filesystem ownership to the runtime, aligns behavior with the documented sandbox protocol, and fills in the missingfs.*methods the frontend already expects.Validation
python -m py_compile spoon_bot/gateway/websocket/protocol.py spoon_bot/gateway/websocket/workspace_fs.py spoon_bot/gateway/websocket/workspace_watch.py spoon_bot/gateway/websocket/handler.py tests/test_workspace_fs_service.py tests/test_workspace_watch_service.pyuv run python -coveringmkdir,write,read,list,rename,remove, andfs.watch