feat(coderd/x/chatd): identify tool calls in the dispatcher and dedupe execute replays - #27370
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@codex review |
![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: 7aad78e8b8
ℹ️ 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".
7aad78e to
db15eaa
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db15eaae70
ℹ️ 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".
db15eaa to
a0e39db
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0e39db035
ℹ️ 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".
a0e39db to
a19b772
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
d3be23b to
c6f4fb5
Compare
a19b772 to
b89e001
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
d9b87ba to
f3c8095
Compare
f75158c to
30f965a
Compare
f3c8095 to
3dfed86
Compare
30f965a to
054e270
Compare
ibetitsmike
changed the title
|
Done on all four points. Identity in the common dispatch path. Comparison fields. The fingerprint is command, requested workdir, env, background, and chat ownership. Display intent and chatd's wait budget are excluded.
Machine-readable conflicts. One thing I got wrong in the original description, corrected here and in Also worth noting for #27369: its comment says "tool-call ID" where this one correctly says the composed identity. The composed key is what landed, since bare provider tool-call IDs repeat across regenerations.
|
3dfed86 to
6c58dd6
Compare
054e270 to
6aefea3
Compare
6c58dd6 to
7bedd8a
Compare
6aefea3 to
dbdb8fe
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Didn't have time to properly look through the changes here, but at a cursory glance it seems alright, although could be simplified a bit. Comments and writing does need a bit of work though, as seems to be the case with this whole stack. 🙈
| if !ok { | ||
| return "" | ||
| } | ||
| sum := sha256.Sum256(fmt.Appendf(nil, "%s|%d|%s", identity.ChatID, identity.AssistantMessageID, identity.ToolCallID)) |
There was a problem hiding this comment.
I don't really see the need for sha256 here. We could do identityAssistantMessageID.String() + identity.ToolCallID.String(). Although I suspect even message ID isn't necessary? We already transmit chat ID as part of the communication between chatd and workspace agent.
If we can't trust the chat ID already transmitted, we have bigger problems.
There was a problem hiding this comment.
Both parts taken.
The key is now fmt.Sprintf("%d-%s", identity.AssistantMessageID, identity.ToolCallID). No SHA-256: it is not a secret and does not need fixed width. No chat ID either, for exactly the reason you give, and it composes with your composite-key suggestion on #27369: the agent now keys reservations by {chatID, idempotencyKey}, so the chat is already the namespace and carrying it in the key too was redundant.
I kept the assistant message ID. A regenerated call reuses its provider tool call ID, but a history edit re-inserts the assistant message under a new row ID, so the message ID is what stops a regenerated call from aliasing the original. Dropping it would rest on an unverified provider guarantee that tool call IDs never repeat, which is the one thing here I did not want to assume.
The message ID is decimal digits, so the first - splits the pair unambiguously no matter what the tool call ID contains. TestIdempotencyKeyFromContext/DistinctPairsCannotCollide proves it with adversarial pairs (4+2-x vs 42+-x, 1+1-1 vs 11+-1).
One consequence worth flagging: the key is no longer guaranteed hex, and #27372 addresses processes by it. I moved that route's key from the URL path into the request body rather than escaping it, so no key value can break routing. Reasoning is in that PR's description.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
7bedd8a to
121599a
Compare
dbdb8fe to
202fa50
Compare
121599a to
3f1ac82
Compare
202fa50 to
a3f2866
Compare
|
Revised. Both parts of your key comment are in, and I took the "comments and writing need work" note as applying here too rather than only where you flagged it. Key Now I kept the assistant message ID and said so explicitly rather than dropping it silently. A regenerated call reuses its provider tool call ID, but a history edit re-inserts the assistant message under a new row ID, so the message ID is what stops a regenerated call from aliasing the original. Dropping it would rest on an unverified provider guarantee. The message ID is decimal digits, so the first Consequence worth flagging The key is no longer guaranteed hex, and #27372 addresses processes by it over HTTP. I measured chi rather than assuming: routing works with Comments Added comments in
|
…e execute replays A resumed turn replays tool calls that history still shows as unresolved, which for the execute tool means running the shell command a second time. Interrupts and edits are not the cause: both commit synthetic cancellation results, which marks the calls handled. What remains is the crash window between running a tool and committing its result, including a lost response. The local tool dispatcher now attaches the identity of the call it is running (chat, assistant message row, and provider tool call ID) to the execution context, so any tool that must run at most once per persisted call can derive a key from it without its own identity path. The execute tool is the only consumer today: it hashes the identity into the client token it sends with StartProcess, and a replay of the same persisted call derives the same token and attaches to the process already running or recently exited. Start conflicts are classified by the code the agent returns rather than by matching its error text. A parameter mismatch is permanent and yields an error result; a start still pending means the outcome is unresolved, so the tool retries while its budget lasts.
3f1ac82 to
22dc279
Compare
a3f2866 to
0599aa1
Compare
2 participants
Stack context
Part 2 of the CODAGT-757 stack. Builds on the run-once reservation mechanism from #27369 by identifying the tool call a reservation belongs to.
Correcting when replay happens
The earlier version of this PR claimed replay follows an interrupt, an edit, or a worker crash. The first two are wrong:
Both mark those calls handled, and
handledToolCallIDssuppresses replay for handled calls. The real driver is narrower: a worker that dies or an attempt that is retried between running a tool and committing its result, including a lost response. The honest framing for this stack is crash-window dedup, and the doc now says that.What
The identity of a tool call is now established by the local tool dispatcher rather than by generation code through an execute-specific channel.
chatloop.ExecuteLocalToolsreceives the chat and assistant message row for the batch, and attaches the identity of each call, including its provider tool call ID, before running it. Any tool that must run at most once per persisted call can read it without introducing another identity path.executeis the only consumer today.The key
Following review, the key is now the plain string
<assistantMessageID>-<toolCallID>. Both changes the reviewer asked for are in:The assistant message ID stays. A regenerated call reuses its provider tool call ID, but a history edit re-inserts the assistant message under a new row ID, so the message ID is what keeps a regenerated call from aliasing the original. Dropping it would rest on an unverified provider guarantee that tool call IDs never repeat.
The message ID is decimal digits, so the first
-splits the pair back apart unambiguously no matter what the tool call ID contains.TestIdempotencyKeyFromContextcovers that with adversarial pairs (4+2-xagainst42+-x) and with tool call IDs containing/,%, a space, and non-ASCII.Conflicts and attach on replay
Start conflicts are classified by the code from #27369 instead of by searching error text.
input_mismatchyields an error result and starts nothing;start_pendingis unresolved, so the tool retries while its budget lasts and only reports an unresolved outcome if the budget runs out.An attached foreground start also returns the process's original start time, so the replay waits only the remaining budget (
started_at + timeout - now) rather than a fresh full timeout. An attached process that already exited reports its real result even past the deadline; one still running past the deadline returns the usual timed-out result carrying thebackground_process_idhandle.