Conversation
|
Hit the same #9265 hot path from the issue side — the O(n^2) reparse is real, and the delta-only regression tests here are the right guard. On the design question you raised: the lazy |
gaoanze888
left a comment
There was a problem hiding this comment.
The delta-only optimization is real, and the submitted focused suites pass (40 tests). I found one terminal-state data-loss case in the OpenAI Responses path.
createSlot() copies item.arguments into block.partialJson, but initializes createPendingToolCall() with arguments: {} and does not call pending.setJson(block.partialJson). If response.output_item.added already contains a complete function-call argument string and the stream then emits response.failed (or otherwise exits before a delta / output_item.done), the finally finalizer materializes {} rather than the provider-supplied arguments.
Minimal sequence:
response.output_item.added(function_call, arguments='{"content":"already present"}')
response.failed(server_error)
On this exact head, processResponsesStream() rejects as expected, but output.content[0].arguments is {}. I added a local regression expecting { content: "already present" }; it fails exactly that way. This also conflicts with the new README statement that terminal messages materialize unread arguments on errors and aborts.
Please initialize the pending state from item.arguments (or call pending.setJson() when creating the function-call slot) and add a regression for the pre-populated-added-item + failed-before-done path. I also tested duplicate/end-late pi-messages sequences; those now error, but the PR explicitly documents that malformed-sequence behavior change, so I am not treating it as a separate blocker.
This comment is AI-generated by /wr
fixes #9265
Stops reparsing the full accumulated tool-call json on every delta. Instead moves reparsing into per-access of
.arguments(cached, so only first access on same version re-parses).It isn't complex code but it's not idiomatic in the codebase and I'm not sure it's worth it. It's done this way not to break anyone existing (mostly at least), but maybe making the system explicit in removing this field access and adding getter method instead (to indicate potential ad-hoc work) s.a.
getLastSnapshot()would be better. (It'll allow cleanup of some copy shenanigans which try to copy objects without triggering field reads. ).I chose not to do it not to break folks but with AI and changelogs and how pi approaches braking changes maybe it'd be better. Let me know or just change my PR / use it as base for your fix. Whichever works.
In any case, this removes unconditional whole-prefix argument parsing from the seven ingestion paths we identified. Reasoning-signature accumulation was already fixed, and plain text/thinking ingestion only appends deltas. It doesn't address frame-encoder catch-up, which can still reparse growing prefixes.
Do note: The current TUI still reads arguments on each update, so it can still incur quadratic parsing, this PR just makes it so other more conscious consumers don't have to.
A few visible differences: the proxy now ignores argument deltas after toolcall_end instead of letting alte data override them. Some other malformed event sequences (s.a. pi-messages argument deltas after tool end) can now error where main tolerated them. Live .arguments is also a getter/setter, so strict descriptor-based validators like isJsonValue() can reject it even though normal field access and JSON serialization work.