fix(agent-core-v2): mint interaction ids engine-side - #2911
Conversation
Self-hosted OpenAI-compatible endpoints may renumber tool call ids on every response (Bash_0, Bash_1, ...). The approval/question/user_tool facades used the provider toolCallId as the interaction id, so a repeated id was silently swallowed by client-side pending-interaction dedupe: the approval prompt never appeared and the turn parked forever (#2908). Interaction ids are now minted by the engine (approval_<uuid> / question_<uuid> / user_tool_<uuid>); the provider toolCallId stays on the payload for correlation. This matches v1 semantics, where the approval id was already a daemon-minted id independent of the tool call id.
🦋 Changeset detectedLatest commit: edb9971 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
commit: |
…estion Self-hosted OpenAI-compatible endpoints may renumber tool call ids on every response (Bash_0, Bash_1, ...), and every downstream keying assumes an id identifies exactly one call: context rebuild silently drops the second tool result with a duplicated id, the strict projector discards duplicate calls, transcript frames merge, and approval/activity correlation misfires. A per-agent ToolCallIdNormalizer in the llmRequester stream boundary now tracks ids already claimed (seeded from the restored context). The first occurrence passes through unchanged; later occurrences — across responses or within one — are rewritten to a readable <id>__<n> suffix, kept consistent between streamed deltas and the finalized message, and logged for provenance. A failed attempt rolls its claims back so a projection retry re-streams the same logical calls under the same ids.
|
@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: 87220c0c92
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| function requestId(req: ApprovalRequest): string { | ||
| return req.id ?? req.toolCallId ?? `${req.toolName}:${String(Date.now())}`; | ||
| return req.id ?? `approval_${randomUUID()}`; |
There was a problem hiding this comment.
When normal tool approvals omit req.id, this now parks the interaction under a random approval_<uuid>, but the permission.approval.* events emitted by AgentToolApprovalService still only carry toolCallId; AgentActivityView therefore continues to expose approvalId: toolCallId, and sessionEventBroadcaster forwards that in agent.status.updated.phase.approval. In that common path, status subscribers receive an approval id that cannot be resolved by POST /sessions/{sid}/approvals/{approval_id}, while the list/requested-event surfaces use the random interaction id.
Useful? React with 👍 / 👎.
…status
The permission.approval.requested/resolved events only carried the
provider toolCallId, so AgentActivityView exposed approvalId =
toolCallId and the agent.status.updated approval phase forwarded an id
that POST /sessions/{sid}/approvals/{id} cannot resolve — the kernel
parks under the minted approval_<uuid>.
Mint the interaction id at the agent call site and include it in the
approval request payload: the kernel honors the explicit id, the
events carry it, and the activity view keys pendingApprovals by it
(falling back to the toolCallId for id-less events).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 038c91aed6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| function requestId(req: QuestionRequest): string { | ||
| return req.id ?? req.toolCallId ?? `question:${String(Date.now())}`; | ||
| return req.id ?? `question_${randomUUID()}`; |
There was a problem hiding this comment.
When AskUserQuestionTool calls question.request it does not pass req.id, so this new fallback parks the interaction under an opaque question_<uuid> while listPending() still returns only the original payload without that id. Hosts that use the typed ISessionQuestionService.listPending() surface can no longer discover the id needed for answer()/dismiss(), and falling back to the provider toolCallId no longer resolves the interaction; store { ...req, id } as the payload or merge i.id into the listed request.
Useful? React with 👍 / 👎.
The approval/question facades returned only the original payload from listPending(), so once the kernel id stopped deriving from the provider toolCallId, hosts listing pending requests had no id to feed back into decide()/answer()/dismiss() without reaching into the kernel. Merge the parked interaction id into each returned request — the klient contract schemas already carry the optional id field, so the RPC surface becomes round-trippable as well.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
2 participants
Related Issue
Resolve #2908
Problem
On self-hosted OpenAI-compatible endpoints that renumber tool call ids on every response (
Bash_0,Bash_1, ...), unrelated tool calls reach the engine sharing one id, while every downstream keying assumes an id identifies exactly one call. The linked issue reported the most visible symptom — the approval prompt never appears from the second approval onward and the turn hangs forever (the client bridge dedupes pending interactions by id with a grow-only set, so the second request with an already-seen id was silently skipped). The same id reuse also corrupts state all the way down: context rebuild silently drops the second tool result with a duplicated id, the strict request projector discards duplicate calls, google-genai outbound hard-fails on duplicate response ids, transcript frames merge two calls into one, and the TUI/ACP tool cards mis-attribute streaming args and results.What changed
Three layers, all in
agent-core-v2:ToolCallIdNormalizerin thellmRequesterstream path). A per-agent set of already-claimed ids (seeded from the restored context) decides: the first occurrence of an id passes through unchanged; later occurrences — across responses or within one — are rewritten to a readable<id>__<n>suffix. Streamed deltas and the finalized message get the same assignment (tracked per stream index), each rewrite is logged with raw/assigned ids for provenance, and a failed attempt rolls its claims back so a projection retry re-streams the same logical calls under the same ids. Healthy endpoints (globally uniquecall_...ids) see zero behavior change. This removes every downstream collision at once instead of migrating ~20 keying sites onto a second id namespace.approval_<uuid>/question_<uuid>/user_tool_<uuid>) instead of deriving them from the provider toolCallId — the direct fix for the reported deadlock. This matches v1 semantics, where the approval id was already a daemon-minted id independent of the tool call id. The provider toolCallId stays on the interaction payload, so TUI prompt correlation, the RESTtool_call_idfield, and the transcript approval back-link are unaffected.AgentToolApprovalServiceincludes the minted id in the request payload, sopermission.approval.requested/resolvedcarry it andAgentActivityViewkeyspendingApprovalsby it — theagent.status.updatedapproval phase now exposes an id thatPOST /sessions/{sid}/approvals/{approval_id}actually resolves (it previously forwarded the bare toolCallId, which diverged from the kernel id once minting moved engine-side).Deliberately out of scope: the v1 engine (
agent-core) — its approval broker already mints daemon-side ids so it does not deadlock, and it is the legacy engine being replaced; and rewriting provider ids back on the outbound path — unnecessary, because context persists the normalized ids and outbound requests are serialized from context, so call/result referential integrity holds end to end.Tests: normalizer unit tests (pass-through, cross-/intra-response rewrites, stream↔finalized consistency, restore seeding, rollback); service-level integration through the request pipeline (streamed parts and finish message stay consistent, rollback on mid-stream failure, restored-context seeding); approval id threading (broker request, events, and activity view share the minted id); interaction-id regression tests at the engine level (approval / question / user-tool: repeated toolCallId gets distinct ids, sequential and parallel) and at the REST level (two approvals sharing a tool call id resolve independently). Full suites: agent-core-v2 5054, kap-server 1036, node-sdk 363, acp-server 144 — all green.
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.