fix: reject unusable relayed call IDs - #237
Merged
franciscojavierarceo merged 2 commits intoSep 5, 2026
Merged
Conversation
StevenWang-CY
requested review from
bbrowning,
franciscojavierarceo,
jiahuei,
leseb,
maralbahari,
noobHappylife,
qandrew and
tjtanaa
as code owners
September 3, 2026 16:41
franciscojavierarceo
requested
a lite review from Copilot
and removed request for
Copilot
September 3, 2026 19:05
There was a problem hiding this comment.
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_callcall_ids (non-empty + unique within the committed response) before the first persistence operation. - Add integration tests covering malformed
call_idscenarios 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.
StevenWang-CY
force-pushed
the
fix-split-call-id-validation
branch
from
September 3, 2026 19:51
71e66ca to
52d6117
Compare
Signed-off-by: Chuyue Wang <stevenwang0805@outlook.com>
Signed-off-by: Chuyue Wang <stevenwang0805@outlook.com>
StevenWang-CY
force-pushed
the
fix-split-call-id-validation
branch
from
September 4, 2026 04:40
60aa5dc to
7f9bf99
Compare
franciscojavierarceo
approved these changes
Sep 5, 2026
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
function_callorcustom_tool_calloutput has a missing/emptycall_id, repeats one within the response, or reuses one from the rehydrated response chain or the current replayed input.call_idacrossresponse.output_item.added, function-argument events,response.output_item.done, and terminal output, rejecting later changes before persistence.Problem and contract
The split
commitpath deserialized an omittedcall_idas 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:response.output_item.addedand persisting ID B fromresponse.output_item.doneA fresh counterexample against the first implementation also failed:
response.function_call_arguments.donecould 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
Test Plan
Exact final commit
7f9bf9964209de75acebe72a0fe2fb854a12d975was tested from a clean detached worktree:cargo fmt --all -- --checkcargo check --workspace --all-targets --all-featurescargo clippy --workspace --all-targets --all-features -- -D warningscargo 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 requireTEST_POSTGRES_URL, and 1 doctest is repository-ignored)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