Make apply_patch streaming parser stateful#19160
Conversation
7bf74ff to
6374f9f
Compare
6374f9f to
4c0c1b7
Compare
Wires patch_updated events through app_server. These events are parsed and streamed while apply_patch is being written by the model. Also adds 500ms of buffering to the patch_updated events in the diff_consumer. The eventual goal is to use this to display better progress indicators in the codex app.
…patch-streaming # Conflicts: # codex-rs/app-server-protocol/schema/json/ServerNotification.json # codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json # codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json # codex-rs/app-server-protocol/schema/typescript/ServerNotification.ts # codex-rs/core/src/tools/handlers/apply_patch.rs # codex-rs/core/src/tools/handlers/apply_patch_tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2924bf70c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let hunks = self.parser.push_delta(delta).ok()??; | ||
| let changes = convert_apply_patch_hunks_to_protocol(&hunks); | ||
| self.last_progress = Some(hunks); | ||
| let event = PatchApplyUpdatedEvent { call_id, changes }; |
There was a problem hiding this comment.
Skip emitting unchanged apply_patch progress events
ApplyPatchArgumentDiffConsumer::push_delta now converts and emits an update for every successful parser call, but StreamingPatchParser::push_delta can return the same hunk set even when the incoming delta contains no newline (so no new patch line was actually parsed). In streamed custom-tool calls with long lines, this produces repeated identical PatchApplyUpdated payloads every buffer window, which the app-server forwards as redundant FileChangePatchUpdated notifications and can cause avoidable event/UI churn; reintroducing a last_progress equality guard (as before) prevents this regression.
Useful? React with 👍 / 👎.
|
I think the streaming parser also requires a trailing newline which the old parser didn't |
ec11648 to
9ebf25c
Compare
9ebf25c to
6bd78a5
Compare
|
|
||
| #[test] | ||
| fn test_parse_one_hunk() { | ||
| assert_eq!( |
There was a problem hiding this comment.
should we keep this one?
This reverts commit ebd7923.
Adds an incremental "streaming" parser for apply_patch. Instead of reparsing the entire patch every time, the incremental parser holds the current state and only parses the delta (line by line). Local n=1 benchmark shows this is 10-15x faster on reasonably sized patches.