feat(client): serve conversation workspace static files - #244
Conversation
Adds client coverage for the agent-server's static workspace routes:
- GET /api/conversations/{id}/workspace -> getWorkspaceRoot()
- GET /api/conversations/{id}/workspace/{path} -> getWorkspaceFile()
Both return the raw bytes as a Blob (call .text() for HTML/text) so
binary artifacts survive intact, mirroring FileClient.downloadTrajectory.
Includes a mocked unit test (both routes, blob round-trip) and an
integration test that round-trips a written root index.html and a nested
file, and asserts 404 for a missing file.
Endpoint audit❌ 8 off-contract call(s) — not on the agent-server · classifiers: cloud
❌ Not on agent-server (gated, 8)⛔ (no known backend) — served by no backend we can see (7)
|
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This change cleanly adds client coverage for the agent-server's static workspace-serving routes (getWorkspaceRoot / getWorkspaceFile), returning raw bytes as a Blob consistent with FileClient.downloadTrajectory. The unit and integration test coverage is appropriate (round-trip content, nested paths, 404 for missing files). No material bugs block merge.
Risk assessment
Low. The change is additive (two GET methods), reads-only, and scoped to existing server routes. The one correctness gap below (#/? in filePath) only affects files whose names contain those characters, which is uncommon in practice but worth fixing for robustness.
Findings
getWorkspaceFiledoes not percent-encodefilePath(see inline comment). A filename containing#or?is silently truncated by theURLconstructor inHttpClient.request, so the server receives the wrong path. Encoding each path segment withencodeURIComponentand joining on/resolves this and keeps nested paths working.- Minor: the integration test's 404 assertion relies on
getWorkspaceFilethrowing. Thetry/catchsetsmissingStatusonly insidecatch, so if the call ever succeeded theexpect(missingStatus).toBe(404)would fail withundefined- that's the intended guard, just noting it behaves as expected today.
| */ | ||
| async getWorkspaceFile(conversationId: string, filePath: string): Promise<Blob> { | ||
| const response = await this.client.get<Blob>( | ||
| `/api/conversations/${conversationId}/workspace/${filePath}`, |
There was a problem hiding this comment.
filePath is interpolated raw into the URL. The HttpClient builds the final URL with new URL(relativePath, baseUrl + '/'), so any # or ? in a filename is interpreted as a fragment/query separator and the path is silently truncated (e.g. weird#name.txt becomes weird). Consider encoding each segment and joining on /, e.g. filePath.split('/').map(encodeURIComponent).join('/'), so nested paths and unusual filenames are preserved. The rest of this client (e.g. FileClient.downloadTrajectory) uses encodeURIComponent for path components; applying the same here keeps the encoding contract consistent across the SDK.
2 participants
Summary
Split from #231 (one feature per PR). Adds client coverage for the agent-server's static workspace-serving routes, which the audit reported as missing.
GET /api/conversations/{id}/workspaceConversationClient.getWorkspaceRoot()GET /api/conversations/{id}/workspace/{path}ConversationClient.getWorkspaceFile()Details
Blob(call.text()for HTML/text) so binary artifacts survive intact, matchingFileClient.downloadTrajectory.getWorkspaceRootserves the workspace rootindex.html;getWorkspaceFileserves a workspace-relative file (nested paths supported). The agent-server only serves local workspaces — non-local workspaces, a missing directory/file, or the absence ofindex.htmlyield 404; path traversal is rejected with 400.Testing
src/__tests__/api-clients.test.ts, mockedfetch): both routes are GET, return aBlob, and round-trip text content; asserts the exact URLs (including the nested path).deterministic-api.integration.test.ts): writes a rootindex.htmland a nested file into the mounted workspace, serves both back verbatim through the static routes, and asserts404for a missing file.npm run build,lint(0 errors), andformat:checkpass; full unit suite green.