fix(agent): keep tool request before its response in session history - #9480
fix(agent): keep tool request before its response in session history#9480MukundaKatta wants to merge 1 commit into
Conversation
The response placeholder for a tool call is inserted before the tool runs, so it gets an earlier created timestamp than the request message, which is built after execution. Session history is ordered by created timestamp, so the tool request could sort after its own response (or tie and order nondeterministically by id). Retrieve the response first and backdate the request when it is not already earlier, so request-before-response ordering holds. Add a small helper plus unit tests for the ordering rule. Fixes aaif-goose#9461
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe109cbbb8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// when needed so request-before-response ordering is preserved. | ||
| fn ensure_request_precedes_response(request: &mut Message, response: &Message) { | ||
| if request.created >= response.created { | ||
| request.created = response.created.saturating_sub(1); |
There was a problem hiding this comment.
When the tool response placeholder is created in the same second as the user message that triggered the turn, this backdates the assistant tool request to one second before that user message. On reload, the session store orders by created_timestamp, id, so the tool request becomes a leading assistant message; fix_conversation then removes leading assistant messages, leaving the saved tool response without its request and corrupting the next provider call. This can happen with fast providers or skipped/denied tools because created only has second granularity, so the fix needs to avoid moving the request before earlier conversation messages (or move the response forward instead).
Useful? React with 👍 / 👎.
|
Thanks so much for digging into this and sending a fix — really appreciate it! 🙏 Sorry, somebody beat you to it on this one: the same ordering bug (#9461) was already being tackled in a few earlier PRs, including #9462 (from the person who reported the issue) and a maintainer attempt in #9739. To avoid duplicate effort I'm going to close this one in favour of those. One thing worth flagging for next time: this approach backdates the request to Please don't let this discourage you — would love to see more contributions! 🚀 |
2 participants
Summary
The response placeholder for a tool call is inserted into
request_to_response_mapbefore the tool runs, so it gets acreatedtimestamp from before execution. The matching request message is built after the tool finishes, so it gets a later timestamp. Session history is ordered bycreated, so the tool request can sort after its own response, or tie and order nondeterministically by id. That breaks request-before-response ordering when the conversation is reloaded.This retrieves the response first and backdates the request when it is not already earlier, so the request always sorts before its response. The logic lives in a small
ensure_request_precedes_responsehelper.Testing
Added two unit tests for the helper: equal and later request timestamps get backdated, and an already-earlier request is left alone.
cargo test -p goose --lib ensure_request_precedes_responsepasses andcargo fmt --checkis clean.Related Issues
Fixes #9461