fix: align relayed SSE validation with providers - #236
Open
franciscojavierarceo wants to merge 7 commits into
Open
fix: align relayed SSE validation with providers#236franciscojavierarceo wants to merge 7 commits into
franciscojavierarceo wants to merge 7 commits into
Conversation
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
franciscojavierarceo
requested review from
bbrowning,
jiahuei,
leseb,
maralbahari,
noobHappylife,
qandrew and
tjtanaa
as code owners
September 3, 2026 04:25
There was a problem hiding this comment.
🟡 Changes recommended
A critical normalization inconsistency can cause authoritative completed item content to be dropped.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Aligns relayed SSE validation with provider-compatible formats while consolidating lifecycle tracking in ResponseAccumulator.
Changes:
- Moves stateless validation into
events/validate.rs. - Reuses accumulator state for strict decoding.
- Adds provider-stream regression coverage.
File summaries
| File | Review |
|---|---|
crates/agentic-server-core/tests/relayed_stream_validation_test.rs |
Adds stream replay and validation tests. |
crates/agentic-server-core/src/executor/upstream.rs |
Delegates strict validation to the accumulator. |
crates/agentic-server-core/src/executor/accumulator.rs |
Tracks lifecycle and completed items. |
crates/agentic-server-core/src/events/validate.rs |
Implements stateless frame validation. |
crates/agentic-server-core/src/events/normalize.rs |
Adds compatible parsing and ID fallbacks, but must canonicalize retained done items to avoid dropping completed content. |
crates/agentic-server-core/src/events/mod.rs |
Exposes internal validation helpers. |
crates/agentic-llm-d/tests/split_execution_integration.rs |
Covers omitted function-call names. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
Collaborator
|
@franciscojavierarceo reviewing this now |
maralbahari
reviewed
Sep 4, 2026
6 tasks
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
franciscojavierarceo
pushed a commit
that referenced
this pull request
Sep 5, 2026
## Summary - Reject completed or incomplete split-execution responses whose `function_call` or `custom_tool_call` output has a missing/empty `call_id`, repeats one within the response, or reuses one from the rehydrated response chain or the current replayed input. - In caller-relayed SSE, bind each tool call's first non-empty `call_id` across `response.output_item.added`, function-argument events, `response.output_item.done`, and terminal output, rejecting later changes before persistence. - Validate before the first write so a rejected payload does not consume the reserved response ID and can be corrected and retried. Failed partial responses retain their existing returned-but-unstored behavior. ## Problem and contract The split `commit` path deserialized an omitted `call_id` as an empty string and persisted the terminal response. It also checked neither resolved IDs from the continued history nor changes between SSE representations. The resulting response ID was accepted but could fail on the next continuation, or a client could answer the originally announced ID while a different ID was persisted. The repository's continuation state treats call IDs as opaque, globally unique identifiers within the rehydrated sequence. The upstream types also require these IDs: - OpenAI response function calls: https://github.com/openai/openai-python/blob/main/src/openai/types/responses/response_function_tool_call.py - OpenAI custom tool calls: https://github.com/openai/openai-python/blob/main/src/openai/types/responses/response_custom_tool_call.py - vLLM call-ID construction: https://github.com/vllm-project/vllm/blob/31e9c13685b484d4ba67dc4e73019b23402609c7/vllm/entrypoints/openai/responses/utils.py#L93-L108 A scan of the recorded corpus found no missing, empty, or repeated finalized call ID. Across 62 recorded SSE call lifecycles, added/done/intermediate representations never changed `call_id`. Older cassettes with terminal representation differences already violate the split validator's item-identity contract and are not newly rejected by this patch. ## Red/green reproduction Against unmodified reviewed head `60aa5dc`, test-only controls failed because all of these payloads were accepted and persisted: - JSON and SSE responses reusing a resolved historical function-call ID in a later custom call - an SSE call announcing ID A in `response.output_item.added` and persisting ID B from `response.output_item.done` A fresh counterexample against the first implementation also failed: `response.function_call_arguments.done` could replace the announced ID even when done/terminal item representations later returned to the original ID. The final implementation observes that mutation as well. The final regressions cover JSON/SSE, function/custom cross-kind reuse, manually replayed current input, added/done/terminal and argument-event mutation, completed/incomplete/failed terminal states, initially empty IDs filled later, malicious CRLF markers, corrected retry, and unchanged failed-response persistence behavior. All rejection diagnostics use bounded indices and never echo the upstream ID. ## Compatibility and scope - The additional history read occurs only for a completed/incomplete payload containing a new function or custom call. - Failed responses remain returnable and unstored even if partial SSE representations disagree. - The normal in-process upstream accumulator is unchanged; stability enforcement is limited to caller-supplied split SSE. - No API, wire-format, or persistence schema changes are introduced. - PR #236 changes the strict SSE validator's architecture and does not currently enforce this invariant, so merge order may require retaining this regression while resolving that file mechanically. ## Test Plan Exact final commit `7f9bf9964209de75acebe72a0fe2fb854a12d975` was tested from a clean detached worktree: - `cargo fmt --all -- --check` - `cargo check --workspace --all-targets --all-features` - `cargo clippy --workspace --all-targets --all-features -- -D warnings` - `cargo test -p agentic-llm-d --test split_execution_integration` (19 passed) - `cargo test --workspace --all-features -- --test-threads=4` (all runnable tests passed; 5 PostgreSQL tests require `TEST_POSTGRES_URL`, and 1 doctest is repository-ignored) - Python suite (111 passed, 3 package-install scenarios skipped by the suite) - cassette validator (108/108 cassettes, 238 turns) - source-install CLI end-to-end test - strict MkDocs build - all-file pre-commit hooks; Apple Git deadlocked in `check-added-large-files --all-files`, so that hook was rerun successfully on the complete four-file diff while every other hook passed over all files --------- Signed-off-by: Chuyue Wang <stevenwang0805@outlook.com>
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
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.
Summary
events/validate.rsand keep cross-event lifecycle validation inResponseAccumulator.executor/upstream.rs.decode_upstreampayloads while leaving live upstream streaming lenient.item_idfallbacks, different terminal item IDs, omitted terminal output, and SSE data fields without a space after the colon.item_idfallbacks with and without terminal output.The broader typed pipeline, shared resolver, relay, and placement work is tracked in #241 and #243–#245. Deferral of the remaining architectural review comments is proposed, pending reviewer agreement.
Follow-up to #235, incorporating @maralbahari's review feedback.
Test Plan
CARGO_PROFILE_TEST_DEBUG=0 CARGO_INCREMENTAL=0 cargo test --workspace -j 2 --quiet(passed with local TCP listeners enabled; 9 tests ignored by their existing configuration).CARGO_PROFILE_TEST_DEBUG=0 CARGO_PROFILE_DEV_DEBUG=0 CARGO_INCREMENTAL=0 cargo clippy --workspace --all-targets -- -D warningscargo fmt --all -- --check/Users/farceo/.local/bin/uv run --no-project --python 3.12 --with pre-commit pre-commit run --all-files