Fix: treat an absent File.contentEncoding as base64 so sandbox output files are written as bytes - #678
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: treat an absent File.contentEncoding as base64 so sandbox output files are written as bytes#678AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
materializeFiles passed file.contentEncoding straight to Buffer.from, so an absent value fell back to Node's utf8 default. Every binary file the Agent Engine sandbox produced was written to disk as its base64 text. Put the rule in one place: decodeFileContent() in file_utils. Both in-repo producers that omitted the field now state it, so the default only ever governs File objects built outside this package. The sandbox input chunk now honors a declared encoding instead of assuming base64.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A
Problem:
File.contentEncodingis optional, and the repo disagreed on what an absent value meant.materializeFilespassed it straight toBuffer.from, which falls back to Node's utf8 default, whileAgentEngineSandboxCodeExecutorpushed base64 output files without the field. Every binary file the sandbox produced was written to disk as its base64 text, so a PNG from a skill script was ASCII, not an image. The sandbox input path had the mirror bug: it forwarded raw text asdatawhere the API wants base64.Solution: An absent
contentEncodingnow meansFileContentEncoding.BASE64. base64 is the lossless reading, it is what theFile.contentdocstring and two of the three consumers already assumed, and arbitrary bytes cannot survive a utf-8 round-trip. The rule lives in exactly one expression,decodeFileContent()incore/src/utils/file_utils.ts. Both in-repo producers that omitted the field now state it, so the default only ever governsFileobjects built outside this package.Behavior change: a caller that builds
CodeExecutionInput.inputFileswith plain text, omitscontentEncoding, and runsUnsafeLocalCodeExecutornow has that text decoded as base64. Such a caller must addcontentEncoding: FileContentEncoding.UTF8. This is the deliberate cost of having one rule. Input files that declare UTF8 are now base64-encoded before being sent to the sandbox; files that were already base64 go through a byte-identical round-trip, so no working caller breaks. The public API surface is unchanged: no signature, export, or enum value moves, anddecodeFileContentstays internal.Collision check: I listed all 577 open PRs on this fork and diffed every adjacent one. None lands this change. #677 fixes the separate artifact-save consumer (
postProcessCodeExecutionResult), which this change deliberately does not touch. #409, #523 and #371 rework collision and path containment inmaterializeFilesand all keep the buggyBuffer.from(file.content, file.contentEncoding)decode. #409 hoists that call above the collision loop, so whichever of the two lands second rebases. I based this onmain.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Six new tests, no existing test edited or deleted:
core/test/utils/file_utils_test.tscontentEncodingas decoded base64 bytescore/test/code_executors/agent_engine_sandbox_code_executor_test.tsThe first two file tests compare
Buffers, not strings, because a string comparison passes on mojibake. The fixture is the PNG signature, which is not valid utf-8.Mutation proof. I ran every new test against a mutated source line and recorded the failure.
decodeFileContentreverted toBuffer.from(file.content, file.contentEncoding)contentEncodingas decoded base64 bytesBuffer[105, 86, 66, 79, 82, 119, ...], the ASCII ofiVBORw, expectedBuffer[137, 80, 78, 71, 13, 10, ...]FileContentEncoding.UTF8FileContentEncoding.BASE64, ignoring the declared valuedata: file.contentdatawas"# hi", expected"IyBoaQ=="contentEncodingdropped from the output pushexpected undefined to be 'base64'Coverage.
decodeFileContentis at 100%: tests 1 and 2/3 cover both branches of the??.file_utils.tsmeasures 95.18% lines overall; the shortfall is the pre-existing second path-traversal guard at lines 92-95, which this change does not touch.Manual End-to-End (E2E) Tests:
I did not run the end-to-end test. It needs Google Cloud credentials and a live reasoning-engine sandbox, which I do not have. To run it: configure an
LlmAgentwithAgentEngineSandboxCodeExecutor, invoke a skill script that callsmatplotlibsavefig('plot.png'), then runfile plot.png. It must reportPNG image dataand the image must open. Before this change it reportedASCII text.I validated locally instead, on the exact commit pushed:
npm run build— passednpm run lint— passednpm run format:check— passednpm run docs:check— passednpx tsc --noEmit— 0 errorsChecklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.
CI note: the Windows job failed twice before passing, each time on a different unrelated integration test that timed out waiting on a spawned process (
build_setup_test.tsat 20s, thenapp_loader_test.tsat 40s). Neither test imports any module this change touches, and both live in theintegrationproject while the new tests areunit:core. The third run passed with no code change.