Repository navigation
Conversation
RemoteWorkspace.fileDownload let the HTTP client parse the application/octet-stream body in 'auto' mode, which decodes it as UTF-8 text, so fileDownload, downloadAsBlob and downloadAndSave replaced every invalid byte with U+FFFD. Request a Blob instead, add a regression test with a non-UTF-8 payload, and drop the known-bug marker from F34.workspace-binary-download. Fixes #5650 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
…ests fileDownload now always returns the raw bytes as a Blob, so the integration tests read the text back with Blob.text(). Fixes #5650 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
clients/typescript/ owns the browser-compatible TypeScript client for the Agent Server API, and this change stays inside it (plus the verify-agent-server map). The fix targets the TS client, so the scope gate passes and no repository move is needed.
Verified on head 07b46e0
- The behavior change is correct and minimal:
fileDownloadnow requestsresponseType: 'blob', so the existinginstanceof Blobbranch sets bothcontentandfile_sizefrom the raw bytes. Both supported transports preserve the bytes — the browser path (HttpClient.parseResponse->response.blob()) and the Node fetch path (parseNodeResponse->new Blob([new Uint8Array(buffer)])) — so the fix holds beyond the mocked test. - The regression test asserts the exact non-UTF-8 bytes and
file_size5 through bothdownloadAsBlobandfileDownload. - Locally:
remote-workspace-download.test.ts5/5, fullnpm test384/384,npm run lint0 errors (7 pre-existing warnings),npx tsc --noEmitclean,npm run format:checkclean. - Current-head CI for
07b46e0is fully green. - Linked issue #5650 is open with
bug+ready-for-devand describes exactly this symptom; the head addresses its acceptance criterion.
Material finding
The public contract changed without updating the docs that describe it. fileDownload().content now returns a Blob for every file (text included), where callers previously received a string. This is the change the PR itself flags as a "Decision needed" item, and it alters the runtime return shape of a public method on a published client (@openhands/typescript-client). The consumer-facing docs were not updated and still imply a string / a two-argument file-writing call:
clients/typescript/README.md(~line 168) showsfileDownload('/remote/path/file.txt', './downloaded-file.txt')— adestinationPathargument the method does not accept (fileDownload(sourcePath)).clients/typescript/README.md(~line 341) listsfileDownload(sourcePath, destinationPath).
A caller who follows the README and reads result.content as a string gets [object Blob] — the same silent-corruption class this PR fixes. Please either update those README entries to the Blob-always contract (pointing string callers at downloadAsText() / await content.text()) and record whether a release note is warranted, or confirm the accepted decision in the PR so a maintainer can settle the doc and release-note follow-up.
Note (not blocking)
Per the repository review guide, a fix whose symptom is only observable through the running service should carry authentic before-and-after runtime evidence. The PR body supplies that (the control-agent-server map run ... F34.workspace-binary-download run reporting xfail on main and pass on this head), but it lives only in the PR description, so I could not re-run it here. The unit and integration tests are sound regression protection regardless.
No other correctness, security, or compatibility issue found in the changed lines.
🔄 CHANGES REQUESTED
| params: { path: sourcePath }, | ||
| timeout: 60000, | ||
| // Keep the raw bytes: 'auto' would decode the octet-stream body as UTF-8 text. | ||
| responseType: 'blob', |
There was a problem hiding this comment.
This changes fileDownload().content to a Blob for every file, text included — a public contract change the PR flags as "Decision needed". The code path is correct on both transports, but the consumer-facing docs still describe the old shape: clients/typescript/README.md (~line 168) calls fileDownload('/remote/path/file.txt', './downloaded-file.txt') with a destinationPath argument this method does not accept, and the method list (~line 341) documents fileDownload(sourcePath, destinationPath). A caller who reads result.content as a string per the README now gets [object Blob]. Please update those README entries to the Blob-always contract and point string callers at downloadAsText() / await content.text(), and record whether the contract change needs a release note.
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
RemoteWorkspace treated binary downloads as UTF-8 text, changing invalid UTF-8 bytes before returning or saving them.
Summary
Request a Blob explicitly in the workspace download path. Binary content and file size survive unchanged; text conversion remains available through the text helper.
Source: remote-workspace.ts.
Issue Number
Fixes #5650
How to Test
Previously recorded verification
cd clients/typescript && npm ci && npm run lint && npx tsc --noEmit && npm test && npm run format:check: lint 0 errors (7 pre-existing warnings), typecheck clean, 384 tests pass. The new test fails on main (blob holds239, 191, 189, ...) and passes here.npx vitest run --config vitest.integration.config.ts workspace.integration.test.ts -t "File Download"against a localagent-server: 4 passed.control-agent-server map run --file F34 --only F34.workspace-binary-download --fresh --repeat 2 --checkout <main checkout>reportsxfailin both attempts.--checkoutreportsxpassin both attempts.control-agent-server map checkis ok andmap run --file F34 --only F34.workspace-binary-download --freshreportspass.Video/Screenshots
n/a
Type
Notes
The declared
string | Blobreturn type already allows a Blob, but all downloads now return one, including text files. Callers relying on a string must convert explicitly; the release-note decision remains relevant. The alternative of returning a string only for valid UTF-8 was not chosen. FileClient’s array-buffer path is unaffected. Supersedes #5676; umbrella #5653.Visual explanation by Codex using HumanLayer show-me.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:67aeb7f-pythonRun
All tags pushed for this build
About Multi-Architecture Support
67aeb7f-python) is a multi-arch manifest supporting both amd64 and arm6467aeb7f-python-amd64) are also available if needed