This repository was archived by the owner on Jul 27, 2026. It is now read-only.
Repository navigation
fix(upload): resolve relative working dirs against /api/file/home - #1106
Merged
Merged
Conversation
The agent-server's /api/file/upload endpoint requires an absolute path and `mkdir -p`'s the parent. The frontend's `toAbsoluteWorkspacePath` was naively prepending `/` to the default relative `workspace/project` working dir, producing `/workspace/project/<hex>/...`. On macOS and fresh Docker images that path lives under a read-only filesystem root, so uploads failed with `OSError: [Errno 30] Read-only file system: '/workspace'`. Conversations themselves kept working because the agent-server resolves relative `workspace.working_dir` against its own process CWD (which is writable in dev), so the worktree landed elsewhere and only uploads mistargeted the read-only root. Fix: introduce `getAgentServerHomeDir` (cached per backend, backed by `FileClient.getHome` → `GET /api/file/home`) and a `resolveAbsoluteAgentServerPath` helper. Both the conversation-start payload and the file-upload destination now go through this resolver, so they always agree on a single absolute path anchored at the agent-server's home directory (e.g. `~/workspace/project/<hex>`). Absolute paths pass through unchanged, so explicit workspace selections and `VITE_WORKING_DIR` overrides are unaffected. Spec: WUP-001 in specs/workspace-upload-path.md. Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
Summary
This PR fixes a real and annoying productivity bug: local conversations whose default working directory is relative would fail file uploads with Errno 30: Read-only file system on macOS and Docker. The fix introduces a FileClient.getHome() round-trip to anchor relative paths against the agent-server's actual home directory rather than the filesystem root.
The implementation is thoughtful and well-tested. A few items to address before merging:
🟠 Important: resolveConversationUploadWorkingDir doesn't resolve the working dir
src/api/workspace-upload-path.ts line 86–88:
export async function resolveConversationUploadWorkingDir(
conversationId?: string,
): Promise<string> {
const workspace = await getStoredConversationMetadata(conversationId);
if (workspace?.working_dir) {
return workspace.working_dir; // ← may still be relative!
}
return getAgentServerWorkingDir();
}If there's a stored conversation with a relative working_dir, this function returns it unchanged and then buildWorkspaceUploadPath resolves it — but only when the caller passes { conversationUrl, sessionApiKey } overrides. Callers in uploadFilesToConversation do this, but if any other caller invokes resolveConversationUploadWorkingDir directly and uses the result with the plain agent-server client (no overrides), they'll get the pre-fix absolute behavior.
Consider: either document this function as requiring overrides to be passed to buildWorkspaceUploadPath, or have it perform the full resolution itself.
🟡 Suggestion: Consider validating the relative path before joining
src/api/agent-server-home.ts joinPath function:
While getSafeUploadFileName sanitizes the file name component, the relative workingDir segment (workspace/project/<hex>) is directly concatenated without path-traversal validation. If a caller somehow passes ../../../etc as the working dir, it would resolve to <home>/../../../etc.
It's an unlikely path given how the value is constructed internally, but adding basic validation (e.g., rejecting paths containing ..) on the resolveAbsoluteAgentServerPath entry point would harden the API.
if (trimmed.includes("..")) {
throw new Error("Path traversal is not permitted in working directory");
}🟡 Suggestion: The isAbsolutePath regex doesn't cover all Windows edge cases
regex: /^([/\\]|[a-zA-Z]:[/\\])/
Windows UNC paths like \\server\share won't be treated as absolute by this regex. While this is unlikely in practice for the agent-server context, you could use Path.isAbsolute() from Node's path module in a non-browser environment, or extend the regex to: ^([/\\]|[a-zA-Z]:[/\\]|[/\\]{2}[^/\\]+) to be thorough.
✅ Everything else looks solid
- The spec in
specs/workspace-upload-path.mdis clear and well-scoped - The caching strategy (Promise-based sharing + no-caching-on-failure) is correct
- The test coverage is comprehensive
- The
@deprecatedjavadoc ontoAbsoluteWorkspacePathis appropriate - The PR description and risk/scope section are exemplary
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Contributor
Author
|
@OpenHands address review bot comments |
|
I'm on it! chuckbutkus can track my progress at all-hands.dev |
|
OpenHands encountered an error: Request timeout after 30 seconds to https://kvzlrnincfrmziip.prod-runtime.all-hands.dev/api/conversations/d947a8b7-083c-435e-90c3-7c58aa791285/ask_agent See the conversation for more information. |
…ly-relative) working dir Add JSDoc explaining that callers must funnel the result through buildWorkspaceUploadPath (which calls resolveAbsoluteWorkspacePath) to get an absolute path for the upload endpoint. Also document that the UNC path case is already covered by the existing isAbsolutePath regex. Addresses review comment on PR #1106. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR fixes a real bug (HTTP 500 on file uploads due to read-only filesystem) with a well-structured solution. The approach of introducing a single source of truth for home-directory-anchored paths is sound, and the test coverage is thorough.
Overall: This PR looks good to merge. I have two minor suggestions worth considering but neither blocks approval.
Inline Comments
🟡 Suggestion (src/api/workspace-upload-path.ts)
The new resolveConversationUploadWorkingDir JSDoc is clear about its contract, but there's a subtle risk: future callers might miss the caveat and use the return value directly for filesystem operations. Consider adding a runtime assertion or a separate internal variant (_resolveConversationUploadWorkingDirRaw) that explicitly signals "this must go through buildWorkspaceUploadPath" — though this may be over-engineering if the team discipline around the JSDoc is sufficient.
🟡 Suggestion (src/api/agent-server-home.ts)
The joinPath function always uses / as the separator. On Windows, the agent-server runs with Python's pathlib which normalizes path separators, but if Path.home() ever returns a Windows-style path (e.g. C:\Users\agent), joining with / produces C:\Users\agent/workspace/project. While pathlib.Path handles this internally, it could cause confusion in logs. Consider using path.sep or normalizing consistently.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
chuckbutkus
marked this pull request as ready for review
June 3, 2026 20:28
Member
|
@OpenHands please add an e2e mock llm test where the user uploads an image and that it gets processed and sent over as part of the llm completions/trajectories as expected maybe something like "what is in this image" for the mock llm trajectory (find or create a dummy image if need be) after pushing your changes, add the e2e-tests label to this PR, and watch only the npm e2e tests CI and make sure it runs successfully. continue to update the test until its passing |
Member
|
PR LGTM I'm going to add an e2e test here |
|
Uh oh! There was an unexpected error starting the job :( |
Adds an end-to-end mock-LLM test that exercises the full image-attachment pipeline: 1. Attaches a minimal 1×1 PNG to the home-page chat via the hidden file input (data-testid="upload-image-input") using Playwright's setInputFiles. 2. Submits "What is in this image?" — creating a conversation and sending the message via sendMessageWithAttachments. 3. Verifies the agent replies with IMAGE_REPLY_TOKEN in the chat UI. 4. Verifies the user MessageEvent in the conversation events API has image_urls populated (base64 data: URL). 5. Verifies at least one /v1/chat/completions call to the mock server contained an image_url content block — confirming the image was forwarded to the LLM as expected. Supporting changes: - mock-llm-server.py: add GET /admin/requests endpoint that exposes all captured completion request bodies since the last reset; reset also clears the history. - mock-llm-helpers.ts: add getMockLLMRequests(), IMAGE_REPLY_TOKEN, and MINIMAL_PNG_BASE64 exports. - AGENTS.md: document the new endpoint and test spec. Co-authored-by: openhands <openhands@all-hands.dev>
chuckbutkus
added
the
e2e-tests
OpenHands AI
Contributor
❌ Mock-LLM E2E Tests22/23 passed · 1 failed Commit:
🔍 Failure details (1)❌ mock-llm-image-upload.spec.ts › mock-LLM image upload › attaching an image embeds it as base64 in the LLM completion callPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
❌ Mock-LLM Docker E2E Test Results16/23 passed · 3 failed · 4 skipped Commit:
🔍 Failure details (3)❌ mock-llm-cross-connect.spec.ts › cross-connect: frontend-only → backend-only › frontend-only connects to a separate backend-only instance❌ mock-llm-image-upload.spec.ts › mock-LLM image upload › attaching an image embeds it as base64 in the LLM completion call❌ mock-llm-partial-stack.spec.ts › partial stack: --frontend-only › serves the frontend but returns 503 for backend routesPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
The agent-server makes one internal LLM call for skill-analysis before the main agent loop starts. The original 1-response trajectory was consumed by that internal call, leaving the agent with a 500 error and retry storm. Add 1 padding response (turn 0: empty text) + 1 safety buffer (turn 2) following the same pattern as mock-llm-automation.spec.ts. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
❌ Mock-LLM E2E Tests22/23 passed · 1 failed Commit:
🔍 Failure details (1)❌ mock-llm-image-upload.spec.ts › mock-LLM image upload › attaching an image embeds it as base64 in the LLM completion callPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
❌ Mock-LLM Docker E2E Test Results14/23 passed · 4 failed · 5 skipped Commit:
🔍 Failure details (4)❌ mock-llm-cross-connect.spec.ts › cross-connect: frontend-only → backend-only › frontend-only connects to a separate backend-only instance❌ mock-llm-image-upload.spec.ts › mock-LLM image upload › attaching an image embeds it as base64 in the LLM completion call❌ mock-llm-model-switch.spec.ts › mock-LLM /model slash command › step 1: configure LLM, create switch-target profile, register trajectory❌ mock-llm-partial-stack.spec.ts › partial stack: --frontend-only › serves the frontend but returns 503 for backend routesPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
litellm strips image_url content blocks for unknown model names like 'openai/mock-test-model'. Switch to 'openai/gpt-4o' so litellm knows the model accepts vision content and includes base64 image_url blocks in the completion request body. The base_url still points at the local mock server; the model name is only a hint to litellm's request formatter. Also improve assertion diagnostics to print all captured LLM requests (not just the first) when the assertion fails. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
✅ Mock-LLM E2E Tests23/23 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
❌ Mock-LLM Docker E2E Test Results17/23 passed · 2 failed · 4 skipped Commit:
🔍 Failure details (2)❌ mock-llm-cross-connect.spec.ts › cross-connect: frontend-only → backend-only › frontend-only connects to a separate backend-only instance❌ mock-llm-partial-stack.spec.ts › partial stack: --frontend-only › serves the frontend but returns 503 for backend routesPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM E2E Tests23/23 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
🔶 Mock-LLM Docker E2E Test Results18/23 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
This was referenced Jul 27, 2026
Contributor
|
🚀 Released in v1.2.0. |
Contributor
🔶 Mock-LLM Docker E2E Test Results18/23 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM E2E Tests23/23 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ No baseline found on main — all 73 snapshots are new and will become the baseline once this PR merges.
🆕 New snapshots (73)These snapshots have no baseline on main and will become the new baseline once this PR merges.
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2388.
What
Fix
HTTP 500 — [Errno 30] Read-only file system: '/workspace'when uploading a file to a local-backend conversation that uses the default (relative) working directory. This is the same path that handles inline image / video attachments in a chat message, so the symptom in #2388 (attachments hang and then fail to send) is the same root cause.Why
Three pieces lined up to silently re-root the upload destination at the filesystem root:
DEFAULT_WORKING_DIR = "workspace/project"is intentionally relative — the agent-server resolves it against its own process CWD.POST /api/file/uploadendpoint requires an absolute path andmkdir -p's the parent before writing.toAbsoluteWorkspacePath()insrc/api/workspace-upload-path.ts"absolutized" the working dir by just prepending/to it, which is not the same as resolving the path. The defaultworkspace/project/<hex>became the literal absolute path/workspace/project/<hex>— a completely different location on disk than the worktree the agent-server actually created.The conversation itself kept working because the agent-server's relative-path resolution put the worktree somewhere writable (under whatever CWD the launcher chose). Only the upload endpoint was redirected to
/workspace/..., which lives under the SIP-protected read-only root on macOS and on a read-only mount in fresh Docker images, hence theOSError.This works under
npm run devbecausescripts/dev-safe.mjssetsVITE_WORKING_DIRto~/.openhands/agent-canvas/workspaces(already absolute), so the resolver no-ops. The bug only bites the published binary (npx @openhands/agent-canvas),dev:static, and the Docker image, whereVITE_WORKING_DIRis unset and the bundled fallback is the relative default — which matches the report in #2388.The fix
Introduce a single source of truth for "resolve this (possibly relative) working dir against the agent-server's home directory":
src/api/agent-server-home.tsgetAgentServerHomeDir({ host?, sessionApiKey? })— async, cached per backend host, backed byFileClient.getHome()→GET /api/file/home. Failed lookups aren't cached so the next call retries.resolveAbsoluteAgentServerPath(workingDir, overrides?)— absolute paths (POSIX/foo, WindowsC:\foo) pass through; relative paths get prefixed with the home dir.Two callers go through it:
AgentServerConversationService.createConversation— resolves the working dir to absolute before sending the conversation-start payload, soworkspace.working_diris already absolute by the time it lands in the agent-server'sbase_state.json.uploadFilesToRuntime(and the dead-but-typeduseConversationUploadFileshook) — resolves at upload time too, so even pre-existing conversations whoseworking_diris relative get a sane absolute upload destination.Both legs use the same resolver, so the conversation's worktree and any later uploads always agree on a single absolute path anchored at the agent-server's home directory (e.g.
~/workspace/project/<hex>)./api/file/homereturnsPath.home()on the agent-server host, which is the most reliable absolute, writable anchor the agent-server currently exposes —/server_infodoes not include the process CWD. Resolving againstPath.home()puts uploads in a stable, discoverable place (~/workspace/project/...) instead of whatever transient directory the launcher happened to spawn from.The legacy
toAbsoluteWorkspacePathhelper is kept (annotated as@deprecated) for cosmetic/log use — no upload-path callers go through it anymore.Verification
npm run typecheck✅npm run lint✅npm test✅ (2901 passed, 12 skipped, 9 todo)getAgentServerHomeDirworkingDirOverridepasses through unchanged<home>/workspace/project/<file>instead of/workspace/project/<file>Risk / scope
VITE_WORKING_DIRset to an absolute path — the resolver short-circuits.search_subdirsalready returns absolute paths.workspace.working_diris relative will, on upload, target<home>/workspace/project/<hex>(where the new code expects them) while the agent-server's actual worktree is still at<old-cwd>/workspace/project/<hex>. Those conversations will still upload to the "wrong" place — same as before, but at least to a writable place. Workaround: start a new conversation.Spec:
WUP-001inspecs/workspace-upload-path.md.This PR was created by an AI agent (OpenHands) on behalf of @rbren.
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a68b692ef4b0496f38c0efa0ff4f0fb8da8675f0c7Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-8b692efRun
All tags pushed for this build
About Multi-Architecture Support
sha-8b692ef) is a multi-arch manifest supporting both amd64 and arm64sha-8b692ef-amd64) are also available if needed