Skip to content

fix: reject unusable relayed call IDs - #237

Merged
franciscojavierarceo merged 2 commits into
vllm-project:mainfrom
StevenWang-CY:fix-split-call-id-validation
Sep 5, 2026
Merged

fix: reject unusable relayed call IDs#237
franciscojavierarceo merged 2 commits into
vllm-project:mainfrom
StevenWang-CY:fix-split-call-id-validation

Conversation

@StevenWang-CY

@StevenWang-CY StevenWang-CY commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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:

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 fix: align relayed SSE validation with providers #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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR adds early validation to reject split-execution upstream responses that contain missing/empty or duplicate tool call_ids, preventing invalid terminal responses from being persisted and later breaking continuation.

Changes:

  • Validate function_call / custom_tool_call call_ids (non-empty + unique within the committed response) before the first persistence operation.
  • Add integration tests covering malformed call_id scenarios for both JSON and SSE relay paths, ensuring reserved response IDs remain reusable on retry.
  • Document the behavior change in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
crates/agentic-server-core/src/executor/persist.rs Adds pre-persistence validation for missing/duplicate tool call IDs in terminal responses.
crates/agentic-llm-d/tests/split_execution_integration.rs Adds regression tests for invalid call_ids in relayed JSON/SSE commit paths and verifies retry behavior.
CHANGELOG.md Notes the new rejection behavior and that reserved response IDs remain available for corrected retries.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/agentic-server-core/src/executor/persist.rs
Comment thread crates/agentic-server-core/src/executor/persist.rs
Comment thread crates/agentic-server-core/src/executor/persist.rs Outdated
Comment thread crates/agentic-server-core/src/executor/persist.rs Outdated
Comment thread crates/agentic-server-core/src/executor/persist.rs Outdated
Signed-off-by: Chuyue Wang <stevenwang0805@outlook.com>
Signed-off-by: Chuyue Wang <stevenwang0805@outlook.com>
@StevenWang-CY
StevenWang-CY force-pushed the fix-split-call-id-validation branch from 60aa5dc to 7f9bf99 Compare September 4, 2026 04:40
@franciscojavierarceo
franciscojavierarceo merged commit c04ac19 into vllm-project:main Sep 5, 2026
13 checks passed
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.

3 participants