Conversation
Greptile SummaryThis PR adds deterministic Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/simplification suggestions with no correctness impact. The core changes are straightforward and well-tested. The two P2 findings (non-contiguous IDs in the mixed provider-defined tool edge case, and duplicate ID-generation logic) don't affect real-world correctness since IDs only need to be unique within a request, not sequential. No security concerns. packages/internal/src/openai-compatible/chat/openai-compatible-prepare-tools.ts — the for...entries() index is from the unfiltered array, leading to non-sequential IDs when provider-defined tools are present.
|
| Filename | Overview |
|---|---|
| packages/internal/src/openai-compatible/chat/openai-compatible-prepare-tools.ts | Adds id: tool_${index + 1} to each mapped function tool; index is from the unfiltered array so IDs become non-contiguous when provider-defined tools are mixed in |
| packages/internal/src/openrouter-ai-sdk/chat/index.ts | Adds inline id: tool_${index + 1} generation to the tool mapping — duplicates the same pattern introduced in openai-compatible-prepare-tools.ts |
| packages/internal/src/openai-compatible/chat/convert-to-openai-compatible-chat-messages.ts | Adds name: toolResponse.toolName to tool-role messages; straightforward propagation of existing field |
| packages/internal/src/openrouter-ai-sdk/chat/convert-to-openrouter-chat-messages.ts | Same name addition as the OpenAI-compatible path — consistent and correct |
| packages/internal/src/openai-compatible/chat/openai-compatible-api-types.ts | Adds optional name field to OpenAICompatibleToolMessage to match the new message shape |
| packages/internal/src/openrouter-ai-sdk/types/openrouter-chat-completions-input.ts | Adds optional name to ChatCompletionToolMessageParam — mirrors the OpenAI-compatible type update |
| web/src/llm-api/types.ts | Introduces ChatCompletionTool interface and adds optional tools to ChatCompletionRequestBody — clean type-level change |
| packages/internal/src/openai-compatible/chat/openai-compatible-prepare-tools.test.ts | New test validates sequential IDs for all-function tool lists; does not cover the mixed provider-defined + function tool case |
Comments Outside Diff (1)
-
packages/internal/src/openrouter-ai-sdk/chat/index.ts, line 165-177 (link)The
filter + mapwithtool_${index + 1}here duplicates the ID-generation pattern that was just introduced inopenai-compatible-prepare-tools.ts. If the ID scheme ever changes (e.g., switching to a hash or the tool name), it will need to be updated in two places. Consider extracting a shared helper or reusingprepareTools(it already returns the mapped tools array and tool warnings).Prompt To Fix With AI
This is a comment left during a code review. Path: packages/internal/src/openrouter-ai-sdk/chat/index.ts Line: 165-177 Comment: **Duplicate ID-generation logic** The `filter + map` with `tool_${index + 1}` here duplicates the ID-generation pattern that was just introduced in `openai-compatible-prepare-tools.ts`. If the ID scheme ever changes (e.g., switching to a hash or the tool name), it will need to be updated in two places. Consider extracting a shared helper or reusing `prepareTools` (it already returns the mapped tools array and tool warnings). How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/internal/src/openai-compatible/chat/openai-compatible-prepare-tools.ts
Line: 54-59
Comment:
**Non-sequential IDs when provider-defined tools are present**
The ID is derived from the position in the original (unfiltered) `tools` array. If a `provider-defined` tool precedes a function tool, the first function tool receives `tool_2` instead of `tool_1`, making the IDs non-contiguous. The OpenRouter `index.ts` path doesn't share this issue because it filters first and then maps. To keep IDs sequential (and symmetric with the other path), maintain a separate counter for function tools only.
```suggestion
let functionToolIndex = 0;
for (const tool of tools) {
if (tool.type === 'provider-defined') {
toolWarnings.push({ type: 'unsupported-tool', tool });
} else {
functionToolIndex++;
openaiCompatTools.push({
id: `tool_${functionToolIndex}`,
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/internal/src/openrouter-ai-sdk/chat/index.ts
Line: 165-177
Comment:
**Duplicate ID-generation logic**
The `filter + map` with `tool_${index + 1}` here duplicates the ID-generation pattern that was just introduced in `openai-compatible-prepare-tools.ts`. If the ID scheme ever changes (e.g., switching to a hash or the tool name), it will need to be updated in two places. Consider extracting a shared helper or reusing `prepareTools` (it already returns the mapped tools array and tool warnings).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/internal/src/openai-compatible/chat/openai-compatible-prepare-tools.ts
Line: 59
Comment:
**Simplification: use `tool.name` as the ID**
Since two tools in the same request cannot share the same name, `tool.name` is already a stable, unique, human-readable identifier. Using `tool_${index + 1}` is less meaningful and adds a dependency on iteration order. Switching to the name would simplify both this file and the parallel logic in `openrouter-ai-sdk/chat/index.ts`.
```suggestion
id: tool.name,
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "include tool call ids" | Re-trigger Greptile
| for (const [index, tool] of tools.entries()) { | ||
| if (tool.type === 'provider-defined') { | ||
| toolWarnings.push({ type: 'unsupported-tool', tool }); | ||
| } else { | ||
| openaiCompatTools.push({ | ||
| id: `tool_${index + 1}`, |
There was a problem hiding this comment.
Non-sequential IDs when provider-defined tools are present
The ID is derived from the position in the original (unfiltered) tools array. If a provider-defined tool precedes a function tool, the first function tool receives tool_2 instead of tool_1, making the IDs non-contiguous. The OpenRouter index.ts path doesn't share this issue because it filters first and then maps. To keep IDs sequential (and symmetric with the other path), maintain a separate counter for function tools only.
| for (const [index, tool] of tools.entries()) { | |
| if (tool.type === 'provider-defined') { | |
| toolWarnings.push({ type: 'unsupported-tool', tool }); | |
| } else { | |
| openaiCompatTools.push({ | |
| id: `tool_${index + 1}`, | |
| let functionToolIndex = 0; | |
| for (const tool of tools) { | |
| if (tool.type === 'provider-defined') { | |
| toolWarnings.push({ type: 'unsupported-tool', tool }); | |
| } else { | |
| functionToolIndex++; | |
| openaiCompatTools.push({ | |
| id: `tool_${functionToolIndex}`, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/internal/src/openai-compatible/chat/openai-compatible-prepare-tools.ts
Line: 54-59
Comment:
**Non-sequential IDs when provider-defined tools are present**
The ID is derived from the position in the original (unfiltered) `tools` array. If a `provider-defined` tool precedes a function tool, the first function tool receives `tool_2` instead of `tool_1`, making the IDs non-contiguous. The OpenRouter `index.ts` path doesn't share this issue because it filters first and then maps. To keep IDs sequential (and symmetric with the other path), maintain a separate counter for function tools only.
```suggestion
let functionToolIndex = 0;
for (const tool of tools) {
if (tool.type === 'provider-defined') {
toolWarnings.push({ type: 'unsupported-tool', tool });
} else {
functionToolIndex++;
openaiCompatTools.push({
id: `tool_${functionToolIndex}`,
```
How can I resolve this? If you propose a fix, please make it concise.| toolWarnings.push({ type: 'unsupported-tool', tool }); | ||
| } else { | ||
| openaiCompatTools.push({ | ||
| id: `tool_${index + 1}`, |
There was a problem hiding this comment.
Simplification: use
tool.name as the ID
Since two tools in the same request cannot share the same name, tool.name is already a stable, unique, human-readable identifier. Using tool_${index + 1} is less meaningful and adds a dependency on iteration order. Switching to the name would simplify both this file and the parallel logic in openrouter-ai-sdk/chat/index.ts.
| id: `tool_${index + 1}`, | |
| id: tool.name, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/internal/src/openai-compatible/chat/openai-compatible-prepare-tools.ts
Line: 59
Comment:
**Simplification: use `tool.name` as the ID**
Since two tools in the same request cannot share the same name, `tool.name` is already a stable, unique, human-readable identifier. Using `tool_${index + 1}` is less meaningful and adds a dependency on iteration order. Switching to the name would simplify both this file and the parallel logic in `openrouter-ai-sdk/chat/index.ts`.
```suggestion
id: tool.name,
```
How can I resolve this? If you propose a fix, please make it concise.This reverts commit 8c63e6a.
Adds a Kimi-specific provider-boundary compatibility pass for Moonshot requests routed through CanopyWave or OpenRouter. The pass adds deterministic ids to outbound tool declarations and fills tool-result message names from prior assistant tool calls, without changing the standard OpenAI-compatible or OpenRouter client adapters. This avoids sending non-standard fields to stricter non-Kimi providers while satisfying Kimi’s tool-call requirements. Validated with targeted Kimi/OpenRouter/provider-adapter tests and the web typecheck.