Skip to content

Fix: decode the Reasoning Engine raw body with setEncoding so multi-byte UTF-8 survives chunk boundaries - #472

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/reasoning-engine-raw-body-utf8
Open

Fix: decode the Reasoning Engine raw body with setEncoding so multi-byte UTF-8 survives chunk boundaries#472
AmaadMartin wants to merge 1 commit into
mainfrom
fix/reasoning-engine-raw-body-utf8

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

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):
    N/A — no public issue is open for this defect.
  2. Or, if no issue exists, describe the change:
    Problem: The raw-body fallback on POST /api/reasoning_engine corrupts any non-ASCII text.

dev/src/server/adk_api_server.ts accumulated the request body as a string:

let rawBody = '';
req.on('data', (chunk) => {
  rawBody += chunk;
});

req is an http.IncomingMessage with no encoding set, so every 'data' event
delivers a raw Buffer and rawBody += chunk implicitly calls
Buffer.prototype.toString() on each chunk in isolation. A multi-byte UTF-8
sequence that straddles a chunk boundary is therefore decoded as two truncated
fragments — the trailing partial bytes of chunk N and the leading partial bytes
of chunk N+1 each decode to U+FFFD REPLACEMENT CHARACTER. The bytes are gone
before JSON.parse ever runs.

Two consequences, in increasing order of visibility:

  1. Silent corruption (the common case). U+FFFD is a legal character inside a
    JSON string, so JSON.parse succeeds and the agent is invoked with mojibake in
    newMessage. Runner.runAsync then persists the corrupted text into the
    session history, so the damage outlives the request.
  2. Hard failure. If a boundary falls inside a JSON structural region, JSON.parse
    throws, the handler logs Failed to parse raw body as JSON and falls through to
    executeQuery({}), which answers 400 appName is required in input — a
    misleading error for a well-formed request.

Only the raw-body branch is affected. When express.json() accepts the media type
it decodes the body correctly and the handler takes the isParsed branch. The
raw-body branch runs when express.json() declines — notably for the doubled
Content-Type: application/json,application/json header that Agent Engine sends,
which the content-type parser rejects as an invalid media type, so type-is
reports no match and body-parser skips parsing.

Solution: Set the stream encoding once, before reading.

       } else {
+        // Decode through the stream's StringDecoder: a multi-byte UTF-8
+        // sequence straddling two chunks would otherwise become replacement
+        // characters on both sides of the boundary.
+        req.setEncoding('utf-8');
         let rawBody = '';
-        req.on('data', (chunk) => {
+        req.on('data', (chunk: string) => {
           rawBody += chunk;
         });

Readable.setEncoding(enc) installs a StringDecoder, which holds back an
incomplete multi-byte sequence until the following chunk completes it. The body is
therefore decoded once over the complete byte stream, so the string handed to
JSON.parse equals Buffer.concat(allChunks).toString('utf-8') regardless of how
the bytes were framed. The chunk parameter is now genuinely a string, so the
annotation is a type correction rather than a cast — no suppression is involved.

Why setEncoding and not a Buffer.concat accumulator: both fix the defect.
setEncoding is the remedy already used for the identical defect class in
core/src/code_executors/unsafe_local_code_executor.ts (stdout += data.toString()),
so both sites now read the same way. Exactly one of the two mechanisms is shipped —
adding a Buffer.concat accumulator on top would be redundant.

Deliberately out of scope (each would widen a one-defect diff):

  • The two // eslint-disable-next-line @typescript-eslint/no-explicit-any comments on
    let body: any / executeQuery(body: any) are left in place. Replacing any with a
    request interface makes newMessage correctly Content | undefined, which under
    strict is not assignable to executeAgentRun's newMessage: Content; relaxing
    that parameter pushes the same error onto Runner.runAsync in core/. Typing it
    honestly needs a core signature change, which does not belong in a UTF-8 fix.
    (PR Fix: narrow the reasoning engine request body instead of typing it any (Part 3/3) #232 on this fork is doing exactly that work.)
  • A raw body of non-object JSON (e.g. null) makes executeQuery throw
    TypeError: Cannot read properties of null (reading 'input') inside the un-awaited
    req.on('end') callback, so the request is never answered. Reproduced, but it is a
    separate defect and is not touched here.

Behaviour change: none beyond the fix. No public API, type, export, or wire-format
change; no new dependency and no package.json/lockfile change. Status codes, response
shapes, the isParsed branch, and the log lines are untouched — the existing
logger.info of the raw body simply logs correct text instead of mojibake. Callers
sending pure ASCII see byte-identical behaviour; the change strictly widens the set of
inputs handled correctly.

Collision check (run before implementing, over all 300 open PRs on the fork):
gh pr diff --name-only across every open PR found nine touching
dev/src/server/adk_api_server.ts#452, #432, #344, #332, #300, #278, #252, #232, #199.
None of them lands this fix. Two are worth flagging:

This PR is branched from main rather than stacked on either, because it depends on
neither, both are refactors of the same handler that would arbitrate the base, and a
stacked base means the pull_request: branches: [main] workflow never triggers. The
diff is 6 lines of source and rebases onto either refactor trivially.

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.

One test added to dev/test/server/adk_api_server_test.ts, in the existing
describe('Reasoning Engine', …) block:
'should not corrupt a multi-byte raw body split across chunk boundaries'.

It POSTs a body whose text is '世界你好'.repeat(16) (all 3-byte characters) over
node:http with no Content-Length, writing 7 bytes per write(). Node frames each
write() as its own chunked-transfer frame and the receiving parser emits one 'data'
event per frame, so the handler deterministically observes those byte boundaries; 7 is not
a multiple of 3, so boundaries land inside the multi-byte sequences. fetch is
deliberately not used — it sends the body as a single frame, so the boundary under test
never occurs and such a test would pass with or without the fix. The framing is
per-write(), not per-TCP-packet, so there are no sleeps and no timing assumptions.

The assertion is on the persisted session event, not the response body: the shared
TestAgent echoes fixed text and never reflects the input, whereas Runner.runAsync
writes the received newMessage into the session verbatim.

$ npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts
 ✓ dev/test/server/adk_api_server_test.ts (52 tests) 592ms
   Test Files  1 passed (1)
        Tests  52 passed (52)     # 51 pre-existing + 1 new

Proof the test can fail. Two independent mutations, each run against the new test:

  1. Revert the fix (drop req.setEncoding('utf-8'), i.e. the pre-fix code). Written and
    run in this state first, before the source was touched:
    AssertionError: expected +0 to be -1 // Object.is equality
     ❯ dev/test/server/adk_api_server_test.ts:1230
       expect(userMessage?.indexOf(REPLACEMENT_CHARACTER)).toBe(-1);
    
    indexOf is 0 — the persisted text starts with U+FFFD.
  2. Wrong encoding (setEncoding('latin1')), to show the test pins the encoding and not
    merely the presence of the call. latin1 produces no U+FFFD, so assertion 1 passes
    and the equality assertion catches it:
    AssertionError: expected 'ä¸\u0096ç\u0095\u008cä½ å¥½…' to be '世界你好世界你好…'
    
    Both assertions therefore earn their place.

Coverage of the changed lines — measured with @vitest/coverage-v8 scoped to
dev/src/server/adk_api_server.ts, per-statement hit counts:

line statement hits
863 req.setEncoding('utf-8'); 2
864 let rawBody = ''; 2
865 req.on('data', (chunk: string) => { 2
866 rawBody += chunk; 47

100% line coverage on every changed line (2 = the pre-existing single-frame raw-body test
plus the new one; 47 = 1 single frame + 46 chunked frames). The change adds no branches.

Other gates, all on the pushed commit:

$ npx eslint dev/src/server/adk_api_server.ts dev/test/server/adk_api_server_test.ts   # clean
$ npx prettier --check <same two files>   # All matched files use Prettier code style!
$ npm run lint          # clean
$ npm run format:check  # clean
$ npm run build         # succeeds
$ npm run ts:check      # 0 diagnostics in either touched file

npm run ts:check has a large pre-existing baseline elsewhere in the repo (mostly
core/test, the subject of separate PRs); neither file touched here contributes to it, and
it is not currently a CI gate.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

  1. npm install && npm run build at the repo root.
  2. Start an AdkApiServer with any agent registered as testApp.
  3. POST /api/reasoning_engine with header
    Content-Type: application/json,application/json (so express.json() declines and the
    raw-body branch runs) and no Content-Length, writing the UTF-8 bytes of
    {
      "input": {
        "appName": "testApp",
        "userId": "testUser",
        "sessionId": "utf8SessionId",
        "newMessage": {"parts": [{"text": "世界你好世界你好…"}], "role": "user"}
      }
    }
    in 7-byte pieces (request.write(bytes.subarray(offset, offset + 7))).
  4. Read the persisted user message back with
    sessionService.getSession({appName:'testApp', userId:'testUser', sessionId:'utf8SessionId'})
    and inspect events.find(e => e.author === 'user')?.content?.parts?.[0].text.

Before: "��界你好世���你��世界你好…"indexOf('\uFFFD') is 0.
After: the exact string that was sent — indexOf('\uFFFD') is -1.

No integration test was added: the behaviour is fully covered at the HTTP boundary by the
unit test above, which drives a real AdkApiServer over a real socket with no mocks.

Checklist

[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.

The raw-body fallback on POST /api/reasoning_engine accumulated the
request into a string with `rawBody += chunk`. `req` is an
IncomingMessage with no encoding set, so each 'data' event delivers a
Buffer and the `+=` decodes every chunk in isolation. A multi-byte UTF-8
sequence straddling a chunk boundary is therefore split into two
truncated fragments, each decoding to U+FFFD, and the bytes are gone
before JSON.parse runs.

Setting the stream encoding installs a StringDecoder, which holds back
an incomplete multi-byte sequence until the following chunk completes
it, so the body is decoded once over the whole byte stream.

Only the raw-body branch is affected; when express.json() accepts the
media type the handler takes the already-parsed branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant