Repository navigation
Carry a vendor-neutral tool kind on daemon tool-call envelopes - #793
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Summary by QodoCarry vendor-neutral tool kinds on canonical tool-call events
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
1. Users lose distinct web search records
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if (root.Str("cmd") is { } cmd) return cmd; | ||
| if (root.Str("command") is { } command) return command; | ||
| if (root.Arr("command") is not { } argv) return null; | ||
| var parts = argv.EnumerateArray().Where(p => p.ValueKind == JsonValueKind.String).Select(p => p.GetString()!).ToList(); |
There was a problem hiding this comment.
1. Tool parsing drifts from json helpers 📘 Rule violation ⚙ Maintainability
ToolCommandText.From(JsonElement) filters argv entries with p.ValueKind == JsonValueKind.String instead of the available p.IsString extension. Every array-form command reaches this filter, so a change to the shared shape-checking policy can leave this cross-vendor path behaving differently.
Agent Prompt
## Issue description
`ToolCommandText.From(JsonElement)` directly compares `JsonElement.ValueKind` even though the shared `IsString` helper provides the required check.
## Issue Context
Replace the direct string-kind comparison without changing how non-string argv entries are ignored.
## Fix Focus Areas
- src/Capacitor.Cli.Core/ToolCommandText.cs[17-17]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| /// The command a shell tool call was given, dug out of the tool input every vendor shapes | ||
| /// differently. Cross-vendor on purpose: Codex writes `cmd`, Claude writes `command` as a string, | ||
| /// and the older argv form is a `bash -lc <script>` triple. |
There was a problem hiding this comment.
2. Maintainers cannot retire old parsing 📘 Rule violation ⚙ Maintainability
ToolCommandText labels the supported command array as the older argv form without naming the persisted format or supported producer that can still emit it. When that compatibility branch is revisited, the comment gives no evidence for whether removing or retaining it will affect current sessions.
Agent Prompt
## Issue description
The comment calls the argv input form older without identifying the current source that requires compatibility support.
## Issue Context
Name the persisted format or supported producer that still emits this shape. If no current source exists, remove the historical qualifier and describe only the accepted input contract.
## Fix Focus Areas
- src/Capacitor.Cli.Core/ToolCommandText.cs[5-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "web_search_call" => WebSearch(payload, ts), | ||
| "tool_search_call" => [new AcpEventEnvelope(Kind: AcpEventKind.ToolCall, ToolCallId: payload.Str("call_id"), ToolName: "tool_search", ToolInputJson: payload.Obj("arguments")?.GetRawText() ?? "{}", ToolKind: AcpToolKind.Other, TimestampIso: ts)], | ||
| "tool_search_output" | ||
| => [new AcpEventEnvelope(Kind: AcpEventKind.ToolResult, ToolCallId: payload.Str("call_id"), ToolResult: Cap(payload.Arr("tools")?.GetRawText() ?? ""), TimestampIso: ts)], |
There was a problem hiding this comment.
3. Maintainers face split vendor parsing 📘 Rule violation ⌂ Architecture
CodexRolloutEvents.Project adds interpretation of raw Codex web_search_call, tool_search_call, and tool_search_output payloads in Capacitor.Cli.Core/Harness/Codex rather than the designated transcript harness assembly. A Codex format change must therefore be coordinated outside the canonical harness boundary, and downstream code can no longer assume vendor fields are confined there.
Agent Prompt
## Issue description
New raw Codex transcript record types and fields are interpreted in the CLI Core harness instead of the designated transcript harness assembly.
## Issue Context
Keep vendor-format parsing and canonical projection within `Capacitor.Models.Transcripts/Harness/Codex`; expose only normalized events to CLI Core consumers.
## Fix Focus Areas
- src/Capacitor.Cli.Core/Harness/Codex/CodexRolloutEvents.cs[32-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "tool_search_output" | ||
| => [new AcpEventEnvelope(Kind: AcpEventKind.ToolResult, ToolCallId: payload.Str("call_id"), ToolResult: Cap(payload.Arr("tools")?.GetRawText() ?? ""), TimestampIso: ts)], |
There was a problem hiding this comment.
4. Failed tool searches look successful 🐞 Bug ≡ Correctness
CodexRolloutEvents.Project creates each tool_search_output result without mapping its status or error to ToolIsError, so the field remains false. When Codex reports a failed tool-registry lookup, the chat consumer consequently marks the paired call Done, unlike the hosted Codex result path, which recognizes either an error payload or status == "failed".
Agent Prompt
## Issue description
`tool_search_output` records discard their failure status and error payload when creating a `ToolResult`, leaving `ToolIsError == false`. This makes failed tool-registry searches appear successful to consumers.
## Issue Context
The hosted Codex mapper already treats either an `error` payload or `status == "failed"` as failure, while the chat UI determines the displayed outcome solely from `ToolIsError` and maps false to `Done`. Apply equivalent failure semantics to rollout tool-search outputs, preserve useful error text when available, and cover failed-status behavior in the unit tests.
## Fix Focus Areas
- src/Capacitor.Cli.Core/Harness/Codex/CodexRolloutEvents.cs[26-36]
- src/Capacitor.Cli.Daemon/Harness/Codex/CodexNotificationMapper.cs[128-145]
- src/Capacitor.App/ViewModels/ChatTabViewModel.cs[395-400]
- test/Capacitor.Cli.Core.Tests.Unit/Harness/Codex/CodexRolloutEventsTests.cs[108-128]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| var action = payload.Obj("action"); | ||
| var status = payload.Str("status") ?? ""; | ||
| var id = $"web_search:{ts ?? action?.Str("query") ?? action?.Str("url") ?? "0"}"; |
There was a problem hiding this comment.
5. Users lose distinct web search records 🐞 Bug ≡ Correctness
WebSearch derives its synthesized ToolCallId only from timestamp, falling back to a query, URL, or the shared literal 0, and reuses that value for both envelopes. Two web-search records with the same timestamp, or timestamp-less records with the same action or no action, therefore reach ID-pairing transcript consumers as the same call.
Agent Prompt
## Issue description
Web-search calls without vendor IDs can receive the same synthesized `ToolCallId`, causing ID-based consumers to treat distinct calls as one.
## Issue Context
The rollout envelope already carries an `ordinal` field, but `WebSearch` currently builds its ID from timestamp/action data only. Preserve the same generated ID on the paired result while incorporating a per-record discriminator.
## Fix Focus Areas
- src/Capacitor.Cli.Core/Harness/Codex/CodexRolloutEvents.cs[24-32]
- src/Capacitor.Cli.Core/Harness/Codex/CodexRolloutEvents.cs[70-81]
- test/Capacitor.Cli.Core.Tests.Unit/Harness/Codex/CodexRolloutEventsTests.cs[77-106]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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: 6b60d3db31
ℹ️ 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".
| return One(new AcpEventEnvelope( | ||
| Kind: AcpEventKind.ToolCall, ToolCallId: id, ToolName: type ?? "unknown", | ||
| ToolInputJson: item.GetRawText(), ItemId: id, TimestampIso: ts)); | ||
| ToolInputJson: item.GetRawText(), ToolKind: AcpToolKind.Other, |
There was a problem hiding this comment.
When the Codex app server completes a normal webSearch thread item, MapItemCompleted has no matching case, so it reaches Unmapped and this line stamps the call as other. The rollout projection added by this commit classifies the equivalent web_search_call as fetch, so a live hosted session and a later import of that session report different tool kinds, defeating the promised vendor-neutral classification and skewing consumers of the field. Add explicit hosted webSearch handling that emits fetch.
Useful? React with 👍 / 👎.
| "tool_search_output" | ||
| => [new AcpEventEnvelope(Kind: AcpEventKind.ToolResult, ToolCallId: payload.Str("call_id"), ToolResult: Cap(payload.Arr("tools")?.GetRawText() ?? ""), TimestampIso: ts)], |
There was a problem hiding this comment.
When a tool_search_output carries a failed status, this result is still emitted with the default ToolIsError = false. That records a failed tool-registry lookup as successful and gives consumers an incorrect outcome; derive ToolIsError from the payload status, as the adjacent synthesized web-search result does.
Useful? React with 👍 / 👎.
ToolName stays the raw vendor name — the server pairs Codex's exec_command with write_stdin by it. A codex shell call has no name of its own for a read or a search, so its command decides. A hosted web search has no item/started row, so it surfaces as a pair: an orphan tool call renders as running forever. The projections cannot carry the kind yet — a tool call is a Kurrent.Agent.Schema ToolCallInfo, so that half needs a schema field first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6b60d3d to
044e9c2
Compare
alexeyzimarev
changed the title
1 participant
Closes #746 — AI-2426
What & why
Every consumer that wants to say what a tool call did keys on the vendor's raw tool name, and the
names disagree per vendor.
AcpEventEnvelopegains a nullableToolKindcarrying the ACP tool-kindvocabulary, populated by the ACP translator and the daemon's Codex app-server lane.
ToolNamekeepsraw vendor fidelity, since the server pairs
exec_commandwithwrite_stdinby it. Null means nolane classified the call;
othermeans none of the above.This is the daemon half only. #773 replaced the transcript-projection contract: a projected tool
call is now a
Kurrent.Agent.SchemaToolCallInfo, which has no kind field, so the Claude/Codexprojections cannot carry one until that package gains it. Tracked as #794 — see the follow-up
issue for the shape it needs.
Where to look
CodexNotificationMappergains awebSearchcase. Without it a hosted web search fell through toUnmappedand reportedother, which would have contradicted every other lane'sfetch. LikefileChangeit has noitem/startedrow, so the completed item emits the pair — an orphan tool callrenders as running forever, because only a result settles one.
The Codex kind table is not the one in the issue body; the comment on #746
records what the rollouts actually contain.
Verification
dotnet runper suite: Core 2936 (2926 passed, 9 skipped, 1 contention flake that passes inisolation —
ProcessRunnerTests.KillTree_cancelled_ct_kills_the_child_then_throws), plus the toucheddaemon classes —
AcpEventTranslatorTests20,CodexNotificationMapperTests18,AcpEventEnvelopeWireCompatTests11 — 0 failed.dotnet publish -c Releasegreps 0 IL2026/IL3050.