Repository navigation
fix(artifacts): store text payloads byte-exact in FileArtifactService - #7917
Closed
TINGyu123644 wants to merge 4 commits into
Closed
TINGyu123644 wants to merge 4 commits into
TINGyu123644 wants to merge 4 commits into
Conversation
FileArtifactService rewrote the line endings of text artifacts. The
payload was read with Path.read_text, whose universal-newline mode
translates "\r\n" and "\r" to "\n", and written with Path.write_text,
which translates "\n" to os.linesep - so on Windows every "\n" was
stored as "\r\n". InMemoryArtifactService and GcsArtifactService return
the saved text unchanged, so the same artifact loaded back differently
depending on the backend, and FileArtifactService is the default for
`adk web` and `adk run`. CSV files (RFC 4180 uses CRLF), patches, and
Windows-authored text therefore loaded back different from what was
saved.
Read the payload with read_bytes().decode("utf-8") and write it with
write_bytes(text.encode("utf-8")) so the round trip is byte-exact on
every platform, matching the in-memory backend.
Adds a regression test parametrized over all three backends asserting
"id,name\r\n1,a\r\n2,b\rc\n" round-trips unchanged.
Fixes google#7914
The change was drafted with an AI coding assistant and reviewed before
submission.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
GWeale
self-requested a review
November 4, 2026 18:40
Collaborator
|
thanks for sending this over, @TINGyu123644. #7915 makes the same change to FileArtifactService to read and write text artifacts as raw utf-8 bytes, and since that one is already approved in review, we're going ahead with it and closing this one. please open a new issue if you still see this once it lands. |
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
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
FileArtifactServicechanges the line endings of text artifacts. A text artifact containing\r\nor a lone\rloads back with\nin their place: the payload is read withPath.read_text, whose universal-newline mode rewrites both, and written withPath.write_text, which translates\ntoos.linesep(so on Windows every\nis stored as\r\n).InMemoryArtifactServiceandGcsArtifactServicereturn the saved text unchanged, so the same artifact differs only on the file backend - which is whatadk webandadk runuse by default (use_local_storage=True, viacreate_local_artifact_service). CSV files (RFC 4180 uses CRLF), patches, and Windows-authored text a tool saves therefore load back different from what was saved.Solution:
Read the payload with
read_bytes().decode("utf-8")and write it withwrite_bytes(text.encode("utf-8"))so the round trip is byte-exact on every platform, matching the in-memory and GCS backends. The staging write already usedwrite_bytesforinline_data; this makes the text path consistent with it. The metadata document is JSON and is unaffected by newline translation, so its reader is left as is.Testing Plan
Unit Tests:
Added
test_save_load_preserves_crlf_and_cr_line_endings, parametrized overIN_MEMORY,GCS, andFILE, asserting"id,name\r\n1,a\r\n2,b\rc\n"round-trips unchanged. Local results:ruff check(repo config) andpyink --checkare clean for both touched files.Manual End-to-End (E2E) Tests:
The issue's reproducer, run against
mainand against this branch:Before:
FileArtifactService 'id,name\n1,a\n2,b\nc\n' False/InMemoryArtifactService ... TrueAfter: both print
True.Checklist
Additional context
The change was drafted with an AI coding assistant and reviewed before submission.