fix: stop one undefined field killing every turn that calls a tool - #1867
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
P2. No test reproduces the original failure at the boundary that broke. The added tests exercise only the new pure function, which cannot have produced the bug; reverting the one-line call-site change in ai-sdk-backend.ts keeps the whole suite green. The harness already exists: packages/runtime/src/__tests__/ai-sdk-backend.test.ts:2416 streams a fake tool-call part with providerOptions and asserts on the persisted event. Add a case that streams { caller: { type: 'direct', toolId: undefined } }, then asserts the persisted tool_start passes encodeCanonicalRuntimeEvent. It should fail on main and pass on the branch.
P2. Arrays are not sanitized (raised independently by both reviewers). stripUndefinedDeep maps array entries but keeps undefined positions, so { items: [undefined] } still fails the canonical encoder and kills the turn; the new test pins [1, undefined, 3] as expected output even though it cannot round-trip through JSON. Convert array undefined entries to null (the JSON-faithful form).
P2. Object.entries rebuilds silently discard symbol and non-enumerable keys and execute getters (tool-args-identity.ts:157-162), contradicting the docstring's "only drops undefined keys" claim. Values the encoder was supposed to reject now persist as a trimmed, plausible-looking object. Restrict the transform to object properties whose value is undefined.
P3. The strip covers one of several producer sites. Thinking events persist raw stream providerOptions at ai-sdk-backend.ts:685/704/723 without stripping, so the PR's "the one place provider metadata enters an event" is inaccurate; the installed adapters are clean there today, so it is latent. A single choke point where stream events are first consumed would cover current and future callers.
P3. stripUndefinedDeep<T>(value: T): T promises the original type after deleting keys. Keep the helper private to the provider-metadata boundary instead of exporting it from @maka/core.
3fa0ff9 to
dfe8904
Compare
|
Three findings on this branch: two confirmed and fixed, one rejected with evidence. Array holes, confirmed. The comment was written entirely about holes, but map and some both skip them, so a sparse array came back untouched and canonicalizeStrictJson still refused it. Only explicit undefined entries were converted. I made the code do what the comment says rather than weakening the comment: the array branch now walks indices and writes null for a hole as well as for an undefined, which is what JSON would have persisted there anyway. Holes are unreachable from JSON-derived provider metadata, so this is not a live bug, but the encoder does refuse them, the fix is one construct wide, and a comment that has to be believed rather than checked is the more expensive of the two options. The old test was renamed to say it covers an explicit undefined, and a new one builds a real hole. Null prototypes, confirmed. The guard bailed on anything whose prototype was not Object.prototype, while canonicalizeStrictJson explicitly accepts a null prototype and then throws on the nested undefined. So metadata built with Object.create(null), which is what prototype-pollution-safe JSON parsing produces, reintroduced the original bug with the fix in place. The guard now matches the encoder exactly, and a rebuilt object keeps its null prototype, because that prototype is what keeps a proto key as data rather than a setter call. Negative control for both: with the guard back to Object.prototype only and the array branch back to map, exactly the two new tests fail and the other seven pass. The thinking_complete path, rejected. The claim was that part.providerOptions is carried from the stream into the persisted event the way a tool call is. It is not. translateChunk does not pass reasoning metadata through; openAiResponsesReasoningProviderOptionsFromChunk rebuilds it from two named fields, itemId and reasoningEncryptedContent, both type-guarded to strings, and the only other source is a Kimi literal. Nothing the provider sent reaches the event, so no omitted field can arrive as an undefined. Evidence rather than argument: I streamed a reasoning part whose metadata carries an undefined-valued field and encoded every persisted event. It passes with no sanitiser. The same harness, in the same file, reddens for the tool call when the sanitiser there is removed, so it can detect this failure and the reasoning path simply does not have it. That test is in the branch now, pinning the rebuild, and a comment on the reasoning push says why no sanitiser belongs there and what would have to change for one to be needed. Test delta against origin/main on this machine: core 703 passing before, 706 after, zero failing. Runtime failure set unchanged, with one pre-existing flake passing this time. |
Provider metadata reaches Maka as the SDK parsed it, and a field the response
did not carry arrives as an explicit `undefined` — Anthropic's `caller` comes
through as `{ type: 'direct', toolId: undefined }` when there is no tool id.
JSON drops such a property, so the value no longer round-trips and
`encodeCanonicalRuntimeEvent` refuses it. That refusal is correct: an
immutable event must mean the same thing after it is read back.
The cost of not handling it was total. The refused write marked the runtime
event store unavailable, the turn's terminal write then threw, and every turn
that called any tool died a tenth of a second after the tool returned —
`load_tools` succeeded, reported the group loaded, and the turn ended there.
`stripUndefinedDeep` removes the keys before they are persisted. It is
lossless in the only sense that matters: JSON cannot tell an absent property
from one set to `undefined`, so this writes down what would have been
persisted anyway. Applied at the one place provider metadata enters an event,
rather than by loosening what the encoder accepts — the encoder is the check
that caught this.
…t round-trip
Review found the first version proved nothing. Reverting the one-line call-site
change in `ai-sdk-backend.ts` left the whole suite green: the tests exercised
only the sanitiser, a pure function that could not have produced the bug.
`ai-sdk-backend.test.ts` now streams the shape a real provider sends —
`{ caller: { type: 'direct', toolId: undefined } }` on a tool-call part — and
asserts every persisted event survives `encodeCanonicalRuntimeEvent`. It fails
without the call-site change and passes with it, which is the only thing that
makes it a check.
Three defects in the sanitiser, all found in review:
- An array hole was left as `undefined`, and the test pinned that as the
expected output — a value that cannot round-trip, so the assertion was
protecting the bug. JSON writes `null` there regardless and removing the
entry would shift everything after it, so `null` is what gets written. The
same reasoning as for object keys: write down what would be persisted.
- Rebuilding every object dropped symbol keys and re-ran getters even when
nothing needed removing. A value that needs no change is now returned as it
came, and one that does is a plain-object spread, which keeps symbol keys.
- The docstring claimed it only dropped undefined keys, which the rebuild
contradicted. It now says what the function does on each path.
…past An array hole and a null-prototype object both survived the sanitiser and still failed the canonical encoder, so the turn-killing write is only fixed for the shapes the tests happened to use. A hole is not an explicit `undefined`: `map` and `some` skip it, so a sparse array came back exactly as it went in while the comment above said it was being written as `null`. It is now written as `null`, which is what JSON would have persisted there anyway. A null prototype is what prototype-pollution-safe JSON parsing produces, and `canonicalizeStrictJson` accepts it — then throws on the nested `undefined`. The guard skipped those objects, so being careful about how you parse reintroduced the original bug. The guard now matches the encoder exactly, and the rebuilt object keeps its null prototype, because that prototype is what keeps a `__proto__` key as data. Reasoning metadata was checked for the same seam and does not have it: `translateChunk` rebuilds it from two named string fields, so nothing the provider sent reaches the persisted event. A test pins that, and a comment says why no sanitiser belongs there.
The untouched-return path was described as keeping symbol keys, getters and object identity, in a paragraph about what this deliberately does not disturb. Two of those are worth keeping; the third is a hazard read as a feature. `canonicalizeStrictJson` throws on any accessor it reaches, so at the only call site a getter that survives kills the write exactly as the `undefined` this function exists to remove would have. Nothing hands it one today — provider metadata arrives parsed — but the sentence recommended a shape that would end the turn. Say that instead, and correct the note at the return, which claimed rebuilding would drop symbol keys and re-run getters. The spread keeps own enumerable symbols and has already read every accessor by the time that branch is taken. No behaviour changes.
4764229 to
c8f18ef
Compare
|
Fixed the comment, and rebased onto current main. The finding is right. The untouched-return path was described as a place where symbol keys, getters and object identity survive, in a paragraph listing what the function deliberately does not disturb. Two of those three are worth keeping. The third is a hazard being read as a feature: canonicalizeStrictJson in the same file throws on any property whose descriptor has no value, so at the only call site — providerOptions on its way into an immutable RuntimeEvent through encodeCanonicalRuntimeEvent — a getter that survives kills the write exactly as the surviving undefined this function exists to remove would have. Not reachable today, since provider metadata arrives as parsed plain objects, but the sentence recommended a shape that would end the turn. Behaviour is unchanged, as asked. The docstring now says accessors are out of scope and that out of scope is fatal downstream, and says why widening the function to rebuild them is not the right move here. I also corrected the note at the return, which claimed rebuilding a value that needed nothing would drop symbol keys and re-run getters. Neither holds: the spread copies own enumerable symbols, which the existing test for a symbol key on the rebuilt path already proves, and the spread has already read every accessor by the time that branch is reached. It now says what the branch actually buys, which is object identity. Verification: tool-args-identity tests 9, pass 9, fail 0; ai-sdk-backend tests 175, pass 175, fail 0. biome check clean on the touched file, check-console passes. Rebase onto origin/main was clean. The packages/core/src/index.ts overlap with #1927 merged without conflict; the export block there now carries stripUndefinedDeep alongside the two existing exports. |
2 participants
What happens without this
Provider metadata reaches Maka as the SDK parsed it, and a field the response did not carry arrives as an explicit
undefined— Anthropic'scallercomes through as{ type: 'direct', toolId: undefined }when there is no tool id.JSON drops such a property, so the value no longer round-trips and
encodeCanonicalRuntimeEventrefuses it. That refusal is correct: an immutable event must mean the same thing after it is read back.The cost was total, and it did not look like this from the outside:
The visible symptom was "the group loaded and then nothing happened".
The fix
stripUndefinedDeepremoves the keys before they are persisted. Lossless in the only sense that matters: JSON cannot tell an absent property from one set toundefined, so this writes down what would have been persisted anyway.Applied at the one place provider metadata enters an event, rather than by loosening what the encoder accepts — the encoder is the check that caught this, and it should keep catching things.
Verification
packages/corebuilds and itstool-args-identitytests pass on this branch against current main;@maka/runtimetypechecks.