feat: openai-compatible-strict-reasoning (2/2) - #1132
Open
myk1yt wants to merge 6 commits into
Open
Conversation
myk1yt
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
August 4, 2026 11:47
Contributor
📝 WalkthroughWalkthroughThe PR adds profile-scoped strict OpenAI tool schemas, propagates the setting across OpenAI-compatible providers, calculates usage costs across providers, adds localized settings controls, and introduces JSON duplicate-key and coverage-analysis utilities. ChangesStrict OpenAI tool schemas
Provider usage cost tracking
Validation utilities and reports
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsUI
participant OpenAIProvider
participant BaseProvider
SettingsUI->>OpenAIProvider: update openAiToolStrictMode
OpenAIProvider->>BaseProvider: convertToolsForOpenAI(tools, strictMode)
BaseProvider-->>OpenAIProvider: converted tool definitions
OpenAIProvider->>OpenAIProvider: omit parallel_tool_calls when tools are absent
sequenceDiagram
participant ProviderStream
participant ModelInfo
participant CostCalculator
participant UsageChunk
ProviderStream->>ModelInfo: resolve model pricing
ProviderStream->>CostCalculator: calculateApiCost(token counts, pricing)
CostCalculator-->>ProviderStream: totalCost
ProviderStream->>UsageChunk: emit normalized usage and totalCost
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/api/providers/__tests__/mistral.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
added 3 commits
August 5, 2026 05:29
Patch coverage checks were blocking 10+ PRs with 80%/70% thresholds. Changed to informational: true so patch coverage is reported but not a required status check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/providers/openai.ts (1)
367-387: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
parallel_tool_callswhen no tools exist.The standard OpenAI paths now omit this field without tools, but these paths still send
parallel_tool_calls: true. This violates the new compatibility contract and can cause no-tool requests to fail on providers that reject the field.
src/api/providers/openai.ts#L367-L387: apply the existing non-emptymetadata?.toolscondition to the O3 streaming request.src/api/providers/openai.ts#L418-L421: apply the same condition to the O3 non-streaming request.src/api/providers/deepseek.ts#L159-L161: apply the same condition to the DeepSeek request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/openai.ts` around lines 367 - 387, Guard parallel_tool_calls in the O3 streaming request within the OpenAI provider so it is included only when metadata?.tools is non-empty. Apply the same conditional handling to the O3 non-streaming request in src/api/providers/openai.ts lines 418-421 and the DeepSeek request in src/api/providers/deepseek.ts lines 159-161; these are all direct changes, preserving parallel_tool_calls: true when tools exist and omitting the field otherwise.
🧹 Nitpick comments (4)
scripts/find-dup-json-keys.js (2)
6-15: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
walkcan recurse without end through symlinked directories.
fs.statSyncresolves symlinks. If a scanned tree contains a symlink that points to an ancestor directory,walkre-enters the same directory and recursion continues until the stack overflows. A monorepo with symlinkednode_modulesmakes this reachable.Use
fs.readdirSync(target, { withFileTypes: true })andfs.lstatSyncso symlinks are not followed. The dirent form also removes onestatSynccall per entry.♻️ Proposed fix
function* walk(target) { - const stat = fs.statSync(target) + const stat = fs.lstatSync(target) + if (stat.isSymbolicLink()) return if (stat.isDirectory()) { - for (const entry of fs.readdirSync(target)) { - yield* walk(path.join(target, entry)) + for (const entry of fs.readdirSync(target, { withFileTypes: true })) { + if (entry.isSymbolicLink()) continue + yield* walk(path.join(target, entry.name)) } } else if (target.endsWith(".json")) { yield target } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/find-dup-json-keys.js` around lines 6 - 15, Update walk to use lstatSync for the target and readdirSync with withFileTypes enabled, identifying directories from dirents and recursing only into non-symlink directories. Preserve JSON file yielding while ensuring symlinked directories are never followed.
26-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
readStringcompares keys in their raw escaped form, so escaped duplicates are missed.Line 33 copies an escape sequence verbatim. The returned key keeps the source spelling.
"ab"and"a\u0062"name the same JSON member, butframe.keysstores two distinct strings, andparseObjectreports no duplicate. Locale JSON files in this repo carry non-ASCII text, so escaped keys are plausible.Decode the common escapes before returning the key, or state the limitation in the header comment.
♻️ Proposed fix
+ const escapes = { '"': '"', "\\": "\\", "/": "/", b: "\b", f: "\f", n: "\n", r: "\r", t: "\t" } + const readString = () => { // assumes text[i] === '"' i++ let out = "" while (i < n) { const c = text[i] if (c === "\\") { - out += text.slice(i, i + 2) - i += 2 + const e = text[i + 1] + if (e === "u") { + out += String.fromCharCode(parseInt(text.slice(i + 2, i + 6), 16)) + i += 6 + } else { + out += escapes[e] ?? e + i += 2 + } continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/find-dup-json-keys.js` around lines 26 - 45, Update readString to decode JSON escape sequences, including Unicode escapes, before returning the parsed key so equivalent spellings such as “ab” and “a\u0062” compare identically in parseObject and duplicate detection. Preserve handling of ordinary characters and continue throwing for unterminated strings.src/api/providers/__tests__/openai-compatible.spec.ts (1)
69-88: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert exact costs in pricing tests.
Type-only and positive-only assertions accept incorrect token rates and incorrect cache accounting. Assert the expected monetary values with
toBeCloseTo.
src/api/providers/__tests__/openai-compatible.spec.ts#L69-L88: Assert the expected baseline cost of0.0105.src/api/providers/__tests__/openai-compatible.spec.ts#L106-L128: Assert the expected cached-input cost of0.00996.src/api/providers/__tests__/anthropic-vertex.spec.ts#L207-L207: Assert the fixture-derived input and output cost instead ofexpect.any(Number).src/api/providers/__tests__/anthropic-vertex.spec.ts#L401-L401: Assert the fixture-derived cache-read and cache-write cost instead ofexpect.any(Number).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/openai-compatible.spec.ts` around lines 69 - 88, Replace weak cost assertions in the pricing tests with exact toBeCloseTo checks: in src/api/providers/__tests__/openai-compatible.spec.ts lines 69-88 assert totalCost is 0.0105, and lines 106-128 assert the cached-input cost is 0.00996; in src/api/providers/__tests__/anthropic-vertex.spec.ts lines 207 and 401 replace expect.any(Number) with the fixture-derived input/output and cache-read/cache-write cost values, respectively.src/api/providers/__tests__/openai.spec.ts (1)
828-832: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the unexplained double assertions.
as unknown as Anthropic.ContentBlockandas unknown as { status: number }bypass assignability checks. Use a precise test fixture or document why the first assertion is unavoidable. UseObject.assign(new Error(...), { status: 429 })for the rate-limit fixture.As per coding guidelines, use double assertions only as a last resort and explain them with a comment.
Also applies to: 882-886
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/openai.spec.ts` around lines 828 - 832, Replace the unexplained double assertions in the Anthropic content-block fixtures and the rate-limit error fixture with precisely typed test values; construct the rate-limit error using Object.assign(new Error(...), { status: 429 }). If the reasoning block still requires a double assertion, add a concise comment explaining why it is unavoidable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/types/src/__tests__/provider-settings.test.ts`:
- Around line 232-238: Update the Anthropic fixture in the
providerSettingsSchemaDiscriminated test to include openAiToolStrictMode, then
assert the schema’s intended behavior by verifying the parsed result removes or
rejects that non-OpenAI field rather than merely confirming its absence from the
input.
In `@scripts/find-dup-json-keys.js`:
- Around line 85-98: Update parseObject to validate each object key before
calling readString: reject the input with a parse error unless the current
character is a double quote, then after skipWs validate that the next character
is ':' before advancing and calling skipValue. Preserve duplicate-key tracking
only for valid key tokens and separators.
- Around line 141-160: Update the argument-processing flow around the top-level
loop to reject an empty process.argv.slice(2) with a nonzero exit and an error
written to stderr. Track duplicate-key occurrences separately from parse errors,
write parse-error diagnostics to stderr, and update the final summary and exit
status to report both counts accurately while preserving the existing
duplicate-key output.
- Around line 100-110: Update scripts/find-dup-json-keys.js at lines 100-110 in
parseObject to throw on loop termination without a closing brace; at lines
120-132 in parseArray, throw on missing closing bracket; and at lines 135-138 in
the top-level parser, reject empty input and trailing non-whitespace after the
parsed value. Ensure these errors propagate to findDuplicates so truncated or
otherwise invalid JSON is reported.
In `@src/api/providers/anthropic-vertex.ts`:
- Around line 126-150: Update the usage emission in the Anthropic Vertex
streaming flow to include output-token pricing once message_delta provides
output_tokens, rather than calculating totalCost only during message_start.
Reuse calculateApiCostAnthropic and the configured model info to emit either an
output-cost update or one consolidated usage record containing both input and
output costs, while preserving cache-token fields. Add a regression test
covering nonzero output tokens and verifying the emitted totalCost.
In `@src/api/providers/friendli.ts`:
- Around line 172-174: Update the request builders in
src/api/providers/friendli.ts lines 172-174, src/api/providers/kenari.ts lines
75-77, src/api/providers/lm-studio.ts lines 90-92, and
src/api/providers/opencode-go.ts lines 192-194 so parallel_tool_calls is
included only when metadata?.tools exists and has at least one item; preserve
the existing configured value when tools are present and omit the field for
tool-free requests.
In `@src/api/providers/mistral.ts`:
- Around line 162-166: Prevent fallback pricing from being charged when model
lookup fails: in src/api/providers/mistral.ts lines 162-166, strip pricing when
mistralModels[id] is missing; in src/api/providers/qwen-code.ts lines 319-323,
do the same for qwenCodeModels[id]; in src/api/providers/bedrock.ts lines
607-618 and 660-670, clear cloned default pricing for unresolved custom and
prompt-router models before assigning costModelConfig. Preserve fallback
capability metadata, but ensure calculateApiCostOpenAI reports totalCost 0 until
exact pricing is available.
In `@src/api/providers/openai-compatible.ts`:
- Around line 176-177: Update convertToolsForAiSdk and its call site in the
OpenAI-compatible provider to preserve each tool’s function.strict value when
creating AI SDK input schemas, using the AI SDK equivalent for strict and
non-strict modes. Ensure streamText receives the hardened schema produced by
convertToolsForOpenAI, and add request-level coverage for both strict mode
enabled and disabled.
In `@src/api/providers/openai.ts`:
- Around line 288-304: Update the usage parsing in the OpenAI handler around
calculateApiCostOpenAI to read cache-read tokens primarily from
usage.prompt_tokens_details.cached_tokens, retaining cache_read_input_tokens
only as a compatibility fallback. In src/api/providers/__tests__/openai.spec.ts
lines 1597-1604, mirror the prompt_tokens_details.cached_tokens usage shape and
assert the resulting totalCost.
In `@webview-ui/src/i18n/locales/ca/settings.json`:
- Around line 968-969: Translate both strictToolSchemas and
strictToolSchemasDescription from English into the target language at
webview-ui/src/i18n/locales/ca/settings.json lines 968-969 (Catalan),
webview-ui/src/i18n/locales/de/settings.json lines 968-969 (German),
webview-ui/src/i18n/locales/es/settings.json lines 968-969 (Spanish),
webview-ui/src/i18n/locales/fr/settings.json lines 968-969 (French),
webview-ui/src/i18n/locales/hi/settings.json lines 968-969 (Hindi),
webview-ui/src/i18n/locales/id/settings.json lines 968-969 (Indonesian), and
webview-ui/src/i18n/locales/it/settings.json lines 968-969 (Italian), preserving
the existing JSON keys and the complete meaning of each description.
In `@webview-ui/src/i18n/locales/en/settings.json`:
- Line 1044: Update the strictToolSchemasDescription text to identify tool-call
arguments, not tool outputs, as the values validated against the
function.parameters schema; preserve the existing provider, MCP, profile, and
OpenAI protocol details.
In `@webview-ui/src/i18n/locales/ja/settings.json`:
- Around line 968-969: Translate the strictToolSchemas and
strictToolSchemasDescription values in the Japanese locale, preserving the
original meaning and the existing JSON keys so the settings view is fully
localized.
In `@webview-ui/src/i18n/locales/ko/settings.json`:
- Around line 968-969: Translate the strictToolSchemas and
strictToolSchemasDescription entries into the appropriate locale language,
replacing the English values in
webview-ui/src/i18n/locales/ko/settings.json#L968-L969,
webview-ui/src/i18n/locales/nl/settings.json#L968-L969,
webview-ui/src/i18n/locales/pl/settings.json#L968-L969,
webview-ui/src/i18n/locales/pt-BR/settings.json#L968-L969,
webview-ui/src/i18n/locales/ru/settings.json#L968-L969,
webview-ui/src/i18n/locales/tr/settings.json#L968-L969,
webview-ui/src/i18n/locales/vi/settings.json#L968-L969,
webview-ui/src/i18n/locales/zh-CN/settings.json#L968-L969, and
webview-ui/src/i18n/locales/zh-TW/settings.json#L995-L996. Preserve both keys
and the full meaning of the strict-mode, provider-support, MCP exception, and
per-profile behavior.
---
Outside diff comments:
In `@src/api/providers/openai.ts`:
- Around line 367-387: Guard parallel_tool_calls in the O3 streaming request
within the OpenAI provider so it is included only when metadata?.tools is
non-empty. Apply the same conditional handling to the O3 non-streaming request
in src/api/providers/openai.ts lines 418-421 and the DeepSeek request in
src/api/providers/deepseek.ts lines 159-161; these are all direct changes,
preserving parallel_tool_calls: true when tools exist and omitting the field
otherwise.
---
Nitpick comments:
In `@scripts/find-dup-json-keys.js`:
- Around line 6-15: Update walk to use lstatSync for the target and readdirSync
with withFileTypes enabled, identifying directories from dirents and recursing
only into non-symlink directories. Preserve JSON file yielding while ensuring
symlinked directories are never followed.
- Around line 26-45: Update readString to decode JSON escape sequences,
including Unicode escapes, before returning the parsed key so equivalent
spellings such as “ab” and “a\u0062” compare identically in parseObject and
duplicate detection. Preserve handling of ordinary characters and continue
throwing for unterminated strings.
In `@src/api/providers/__tests__/openai-compatible.spec.ts`:
- Around line 69-88: Replace weak cost assertions in the pricing tests with
exact toBeCloseTo checks: in
src/api/providers/__tests__/openai-compatible.spec.ts lines 69-88 assert
totalCost is 0.0105, and lines 106-128 assert the cached-input cost is 0.00996;
in src/api/providers/__tests__/anthropic-vertex.spec.ts lines 207 and 401
replace expect.any(Number) with the fixture-derived input/output and
cache-read/cache-write cost values, respectively.
In `@src/api/providers/__tests__/openai.spec.ts`:
- Around line 828-832: Replace the unexplained double assertions in the
Anthropic content-block fixtures and the rate-limit error fixture with precisely
typed test values; construct the rate-limit error using Object.assign(new
Error(...), { status: 429 }). If the reasoning block still requires a double
assertion, add a concise comment explaining why it is unavoidable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16a6d0dc-0253-4041-a197-c4c7f1df2e2a
📒 Files selected for processing (47)
packages/types/src/__tests__/provider-settings.test.tspackages/types/src/provider-settings.tsscripts/find-dup-json-keys.jssrc/api/providers/__tests__/anthropic-vertex.spec.tssrc/api/providers/__tests__/base-provider.spec.tssrc/api/providers/__tests__/kenari.spec.tssrc/api/providers/__tests__/openai-compatible.spec.tssrc/api/providers/__tests__/openai-usage-tracking.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/anthropic-vertex.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/base-provider.tssrc/api/providers/bedrock.tssrc/api/providers/deepseek.tssrc/api/providers/friendli.tssrc/api/providers/kenari.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mistral.tssrc/api/providers/moonshot.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/poe.tssrc/api/providers/qwen-code.tssrc/api/providers/xai.tssrc/eslint-suppressions.jsonwebview-ui/src/components/settings/providers/OpenAICompatible.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
Comment on lines
+232
to
+238
| // Anthropic provider should not have this field | ||
| const anthropicResult = providerSettingsSchemaDiscriminated.parse({ | ||
| apiProvider: "anthropic", | ||
| apiKey: "sk-test", | ||
| }) | ||
| expect(anthropicResult.apiProvider).toBe("anthropic") | ||
| expect((anthropicResult as Record<string, unknown>).openAiToolStrictMode).toBeUndefined() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the non-OpenAI input in the scoping test.
The Anthropic fixture does not include openAiToolStrictMode, so the assertion only checks its default absence. Add the field to the Anthropic input and assert that parsing removes or rejects it.
Suggested test correction
const anthropicResult = providerSettingsSchemaDiscriminated.parse({
apiProvider: "anthropic",
apiKey: "sk-test",
+ openAiToolStrictMode: true,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Anthropic provider should not have this field | |
| const anthropicResult = providerSettingsSchemaDiscriminated.parse({ | |
| apiProvider: "anthropic", | |
| apiKey: "sk-test", | |
| }) | |
| expect(anthropicResult.apiProvider).toBe("anthropic") | |
| expect((anthropicResult as Record<string, unknown>).openAiToolStrictMode).toBeUndefined() | |
| // Anthropic provider should not have this field | |
| const anthropicResult = providerSettingsSchemaDiscriminated.parse({ | |
| apiProvider: "anthropic", | |
| apiKey: "sk-test", | |
| openAiToolStrictMode: true, | |
| }) | |
| expect(anthropicResult.apiProvider).toBe("anthropic") | |
| expect((anthropicResult as Record<string, unknown>).openAiToolStrictMode).toBeUndefined() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/types/src/__tests__/provider-settings.test.ts` around lines 232 -
238, Update the Anthropic fixture in the providerSettingsSchemaDiscriminated
test to include openAiToolStrictMode, then assert the schema’s intended behavior
by verifying the parsed result removes or rejects that non-OpenAI field rather
than merely confirming its absence from the input.
Comment on lines
+85
to
+98
| while (i < n) { | ||
| skipWs() | ||
| const keyLine = line | ||
| const key = readString() | ||
| const frame = stack[stack.length - 1] | ||
| if (frame.keys.has(key)) { | ||
| dups.push({ key, line: keyLine }) | ||
| } else { | ||
| frame.keys.add(key) | ||
| } | ||
| skipWs() | ||
| // expect ':' | ||
| i++ | ||
| skipValue() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
parseObject does not validate the key token or the colon.
Line 88 calls readString, which assumes text[i] === '"'. Line 97 advances one character and assumes it is :. Neither assumption is checked.
For an unquoted key such as { foo: 1 }, readString starts at f, consumes text until the next " anywhere later in the file, and returns a garbage key. The scanner then keeps parsing from a wrong offset. The file is reported with wrong keys and wrong lines, or with no finding at all, instead of a parse error.
Reject the input when the key does not start with " and when the following non-whitespace character is not :.
🐛 Proposed fix
while (i < n) {
skipWs()
const keyLine = line
+ if (text[i] !== '"') {
+ throw new Error(`expected '"' but found ${text[i]} at line ${line}`)
+ }
const key = readString()
const frame = stack[stack.length - 1]
if (frame.keys.has(key)) {
dups.push({ key, line: keyLine })
} else {
frame.keys.add(key)
}
skipWs()
- // expect ':'
- i++
+ if (text[i] !== ":") {
+ throw new Error(`expected ':' but found ${text[i]} at line ${line}`)
+ }
+ i++
skipValue()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (i < n) { | |
| skipWs() | |
| const keyLine = line | |
| const key = readString() | |
| const frame = stack[stack.length - 1] | |
| if (frame.keys.has(key)) { | |
| dups.push({ key, line: keyLine }) | |
| } else { | |
| frame.keys.add(key) | |
| } | |
| skipWs() | |
| // expect ':' | |
| i++ | |
| skipValue() | |
| while (i < n) { | |
| skipWs() | |
| const keyLine = line | |
| if (text[i] !== '"') { | |
| throw new Error(`expected '"' but found ${text[i]} at line ${line}`) | |
| } | |
| const key = readString() | |
| const frame = stack[stack.length - 1] | |
| if (frame.keys.has(key)) { | |
| dups.push({ key, line: keyLine }) | |
| } else { | |
| frame.keys.add(key) | |
| } | |
| skipWs() | |
| if (text[i] !== ":") { | |
| throw new Error(`expected ':' but found ${text[i]} at line ${line}`) | |
| } | |
| i++ | |
| skipValue() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/find-dup-json-keys.js` around lines 85 - 98, Update parseObject to
validate each object key before calling readString: reject the input with a
parse error unless the current character is a double quote, then after skipWs
validate that the next character is ':' before advancing and calling skipValue.
Preserve duplicate-key tracking only for valid key tokens and separators.
Comment on lines
+100
to
+110
| if (text[i] === ",") { | ||
| i++ | ||
| continue | ||
| } | ||
| if (text[i] === "}") { | ||
| i++ | ||
| stack.pop() | ||
| return | ||
| } | ||
| throw new Error(`unexpected char ${text[i]} at line ${line}`) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The scanner treats the end of the text as a valid terminator, so truncated JSON is reported as clean. Every container loop is bounded by while (i < n) and no branch throws when the text ends before the closing } or ]. findDuplicates then returns normally, the catch block in the main loop never runs, and the file is counted as having no problem. A file cut short by a bad merge is exactly the input this utility exists to catch.
scripts/find-dup-json-keys.js#L100-L110: after thewhile (i < n)loop inparseObject, throw an error such asunexpected end of input in object, instead of returning.scripts/find-dup-json-keys.js#L120-L132: after thewhile (i < n)loop inparseArray, throwunexpected end of input in array.scripts/find-dup-json-keys.js#L135-L138: throw when the text holds no value, and after the top-level value callskipWsand throw wheni < n, so trailing content is rejected.
🐛 Proposed fix
if (text[i] === "}") {
i++
stack.pop()
return
}
throw new Error(`unexpected char ${text[i]} at line ${line}`)
}
+ throw new Error("unexpected end of input in object")
}
@@
if (text[i] === "]") {
i++
return
}
throw new Error(`unexpected char ${text[i]} at line ${line}`)
}
+ throw new Error("unexpected end of input in array")
}
@@
skipWs()
+ if (i >= n) throw new Error("empty document")
if (text[i] === "{") parseObject()
else skipValue()
+ skipWs()
+ if (i < n) throw new Error(`trailing content at line ${line}`)
return dups📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (text[i] === ",") { | |
| i++ | |
| continue | |
| } | |
| if (text[i] === "}") { | |
| i++ | |
| stack.pop() | |
| return | |
| } | |
| throw new Error(`unexpected char ${text[i]} at line ${line}`) | |
| } | |
| if (text[i] === ",") { | |
| i++ | |
| continue | |
| } | |
| if (text[i] === "}") { | |
| i++ | |
| stack.pop() | |
| return | |
| } | |
| throw new Error(`unexpected char ${text[i]} at line ${line}`) | |
| } | |
| throw new Error("unexpected end of input in object") | |
| } | |
| while (i < n) { | |
| skipValue() | |
| skipWs() | |
| if (text[i] === ",") { | |
| i++ | |
| continue | |
| } | |
| if (text[i] === "]") { | |
| i++ | |
| return | |
| } | |
| throw new Error(`unexpected char ${text[i]} at line ${line}`) | |
| } | |
| throw new Error("unexpected end of input in array") | |
| } | |
| skipWs() | |
| if (i >= n) throw new Error("empty document") | |
| if (text[i] === "{") parseObject() | |
| else skipValue() | |
| skipWs() | |
| if (i < n) throw new Error(`trailing content at line ${line}`) | |
| return dups |
📍 Affects 1 file
scripts/find-dup-json-keys.js#L100-L110(this comment)scripts/find-dup-json-keys.js#L120-L132scripts/find-dup-json-keys.js#L135-L138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/find-dup-json-keys.js` around lines 100 - 110, Update
scripts/find-dup-json-keys.js at lines 100-110 in parseObject to throw on loop
termination without a closing brace; at lines 120-132 in parseArray, throw on
missing closing bracket; and at lines 135-138 in the top-level parser, reject
empty input and trailing non-whitespace after the parsed value. Ensure these
errors propagate to findDuplicates so truncated or otherwise invalid JSON is
reported.
Comment on lines
+141
to
+160
| let found = 0 | ||
| for (const target of process.argv.slice(2)) { | ||
| for (const file of walk(target)) { | ||
| const text = fs.readFileSync(file, "utf8") | ||
| let dups | ||
| try { | ||
| dups = findDuplicates(text) | ||
| } catch (e) { | ||
| console.log(`${file}: PARSE ERROR ${e.message}`) | ||
| found++ | ||
| continue | ||
| } | ||
| for (const d of dups) { | ||
| console.log(`${file}: duplicate key "${d.key}" at line ${d.line}`) | ||
| found++ | ||
| } | ||
| } | ||
| } | ||
| console.log(found === 0 ? "OK: no duplicate keys found" : `TOTAL: ${found} duplicate key occurrence(s)`) | ||
| process.exit(found === 0 ? 0 : 1) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
With no arguments the script reports OK and exits 0, and the summary line mislabels parse errors.
If a caller passes no path, the for loop body never runs, line 159 prints OK: no duplicate keys found, and line 160 exits 0. A CI step that loses its path argument then passes silently.
Line 150 also increments found for a parse error, but line 159 describes the total as duplicate key occurrences only. Count the two conditions separately. Write failures to stderr so a caller can separate them from the summary.
🐛 Proposed fix
+const targets = process.argv.slice(2)
+if (targets.length === 0) {
+ console.error("Usage: node find-dup-json-keys.js <file-or-dir> [...]")
+ process.exit(2)
+}
+
let found = 0
-for (const target of process.argv.slice(2)) {
+let errors = 0
+for (const target of targets) {
for (const file of walk(target)) {
const text = fs.readFileSync(file, "utf8")
let dups
try {
dups = findDuplicates(text)
} catch (e) {
- console.log(`${file}: PARSE ERROR ${e.message}`)
- found++
+ console.error(`${file}: PARSE ERROR ${e.message}`)
+ errors++
continue
}
for (const d of dups) {
- console.log(`${file}: duplicate key "${d.key}" at line ${d.line}`)
+ console.error(`${file}: duplicate key "${d.key}" at line ${d.line}`)
found++
}
}
}
-console.log(found === 0 ? "OK: no duplicate keys found" : `TOTAL: ${found} duplicate key occurrence(s)`)
-process.exit(found === 0 ? 0 : 1)
+console.log(
+ found === 0 && errors === 0
+ ? "OK: no duplicate keys found"
+ : `TOTAL: ${found} duplicate key occurrence(s), ${errors} parse error(s)`,
+)
+process.exit(found === 0 && errors === 0 ? 0 : 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let found = 0 | |
| for (const target of process.argv.slice(2)) { | |
| for (const file of walk(target)) { | |
| const text = fs.readFileSync(file, "utf8") | |
| let dups | |
| try { | |
| dups = findDuplicates(text) | |
| } catch (e) { | |
| console.log(`${file}: PARSE ERROR ${e.message}`) | |
| found++ | |
| continue | |
| } | |
| for (const d of dups) { | |
| console.log(`${file}: duplicate key "${d.key}" at line ${d.line}`) | |
| found++ | |
| } | |
| } | |
| } | |
| console.log(found === 0 ? "OK: no duplicate keys found" : `TOTAL: ${found} duplicate key occurrence(s)`) | |
| process.exit(found === 0 ? 0 : 1) | |
| const targets = process.argv.slice(2) | |
| if (targets.length === 0) { | |
| console.error("Usage: node find-dup-json-keys.js <file-or-dir> [...]") | |
| process.exit(2) | |
| } | |
| let found = 0 | |
| let errors = 0 | |
| for (const target of targets) { | |
| for (const file of walk(target)) { | |
| const text = fs.readFileSync(file, "utf8") | |
| let dups | |
| try { | |
| dups = findDuplicates(text) | |
| } catch (e) { | |
| console.error(`${file}: PARSE ERROR ${e.message}`) | |
| errors++ | |
| continue | |
| } | |
| for (const d of dups) { | |
| console.error(`${file}: duplicate key "${d.key}" at line ${d.line}`) | |
| found++ | |
| } | |
| } | |
| } | |
| console.log( | |
| found === 0 && errors === 0 | |
| ? "OK: no duplicate keys found" | |
| : `TOTAL: ${found} duplicate key occurrence(s), ${errors} parse error(s)`, | |
| ) | |
| process.exit(found === 0 && errors === 0 ? 0 : 1) |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 143-143: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(file, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/find-dup-json-keys.js` around lines 141 - 160, Update the
argument-processing flow around the top-level loop to reject an empty
process.argv.slice(2) with a nonzero exit and an error written to stderr. Track
duplicate-key occurrences separately from parse errors, write parse-error
diagnostics to stderr, and update the final summary and exit status to report
both counts accurately while preserving the existing duplicate-key output.
Comment on lines
+126
to
+150
| const inputTokens = usage.input_tokens || 0 | ||
| const outputTokens = usage.output_tokens || 0 | ||
| const cacheWriteTokens = usage.cache_creation_input_tokens || 0 | ||
| const cacheReadTokens = usage.cache_read_input_tokens || 0 | ||
|
|
||
| // Compute cost using user-configured pricing from model info. | ||
| // Anthropic semantics: inputTokens does NOT include cached tokens. | ||
| const modelInfo = this.getModel().info | ||
| const { totalCost } = modelInfo | ||
| ? calculateApiCostAnthropic( | ||
| modelInfo, | ||
| inputTokens, | ||
| outputTokens, | ||
| cacheWriteTokens, | ||
| cacheReadTokens, | ||
| ) | ||
| : { totalCost: 0 } | ||
|
|
||
| yield { | ||
| type: "usage", | ||
| inputTokens: usage.input_tokens || 0, | ||
| outputTokens: usage.output_tokens || 0, | ||
| cacheWriteTokens: usage.cache_creation_input_tokens || undefined, | ||
| cacheReadTokens: usage.cache_read_input_tokens || undefined, | ||
| inputTokens, | ||
| outputTokens, | ||
| cacheWriteTokens: cacheWriteTokens || undefined, | ||
| cacheReadTokens: cacheReadTokens || undefined, | ||
| totalCost, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include output-token cost in the emitted usage total.
message_delta emits chunk.usage!.output_tokens, but it emits no totalCost. The new calculation runs only for message_start. When output tokens arrive later, the reported cost excludes output pricing. Emit an output-side cost chunk, or emit one consolidated usage chunk after both token categories are available. Add a regression test with nonzero output tokens.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/anthropic-vertex.ts` around lines 126 - 150, Update the
usage emission in the Anthropic Vertex streaming flow to include output-token
pricing once message_delta provides output_tokens, rather than calculating
totalCost only during message_start. Reuse calculateApiCostAnthropic and the
configured model info to emit either an output-cost update or one consolidated
usage record containing both input and output costs, while preserving
cache-token fields. Add a regression test covering nonzero output tokens and
verifying the emitted totalCost.
Comment on lines
+288
to
+304
| const inputTokens = usage?.prompt_tokens || 0 | ||
| const outputTokens = usage?.completion_tokens || 0 | ||
| const cacheWriteTokens = usage?.cache_creation_input_tokens || 0 | ||
| const cacheReadTokens = usage?.cache_read_input_tokens || 0 | ||
|
|
||
| const modelInfo = _modelInfo ?? this.getModel().info | ||
| const { totalCost } = modelInfo | ||
| ? calculateApiCostOpenAI(modelInfo, inputTokens, outputTokens, cacheWriteTokens, cacheReadTokens) | ||
| : { totalCost: 0 } | ||
|
|
||
| return { | ||
| type: "usage", | ||
| inputTokens: usage?.prompt_tokens || 0, | ||
| outputTokens: usage?.completion_tokens || 0, | ||
| cacheWriteTokens: usage?.cache_creation_input_tokens || undefined, | ||
| cacheReadTokens: usage?.cache_read_input_tokens || undefined, | ||
| inputTokens, | ||
| outputTokens, | ||
| cacheWriteTokens: cacheWriteTokens || undefined, | ||
| cacheReadTokens: cacheReadTokens || undefined, | ||
| totalCost, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For OpenAI Node SDK version 5.12.2, what field in Chat Completions usage reports cached prompt tokens?
💡 Result:
For the OpenAI Node SDK (including version 5.12.2), cached prompt tokens in Chat Completions are reported in the cached_tokens field located within the usage.prompt_tokens_details object [1][2][3]. Specifically, in the response structure, this appears as: usage: { prompt_tokens_details: { cached_tokens: number } } This field indicates the number of input tokens that were read from the cache [1][4][3]. For certain newer model families (e.g., GPT-5.6 and later), the prompt_tokens_details object may also include a cache_write_tokens field, which reports the number of prompt tokens written to the cache [1][3].
Citations:
- 1: https://developers.openai.com/api/docs/guides/prompt-caching
- 2: https://github.com/openai/openai-node/blob/e228aaad/src/resources/completions.ts
- 3: https://developers.openai.com/api/docs/guides/prompt-caching.md
- 4: https://developers.openai.com/api/docs/guides/prompt-caching?prompt-cache-api=responses
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate files ---'
git ls-files | grep -E '(^|/)(openai\.ts|openai\.spec\.ts|apiCost.*|calculator|cost.*openai.*)$' || true
echo "
echo '--- opencai outline ---"
ast-grep outline src/api/providers/openai.ts --view expanded || true
echo '--- relevant openai.ts section ---'
sed -n '240,320p' src/api/providers/openai.ts
echo '--- provider tests prompt_tokens_details/cache_read ---'
rg -n "prompt_tokens_details|cache_read_input_tokens|cache_creation_input_tokens|cache_write_tokens" src/api/providers src/api -g '*.ts'
echo '--- test section around affected lines ---'
sed -n '1560,1630p' src/api/providers/__tests__/openai.spec.ts
echo '--- cost calculator definitions/usages ---'
rg -n "calculateApiCostOpenAI|cacheWriteTokens|cacheReadTokens|cache_read_input_tokens|cache_creation_input_tokens|cached_tokens" src -g '*.ts' | head -200Repository: Zoo-Code-Org/Zoo-Code
Length of output: 46287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cost calculator ---'
sed -n '1,180p' src/shared/cost.ts
echo '--- cost calculator tests ---'
sed -n '100,190p' src/utils/__tests__/cost.spec.ts
echo '--- package OpenAI SDK version references ---'
rg -n '"openai"|openai@|`@openai`' package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null || true
echo '--- run behavioral probe on cost function from source ---'
node - <<'JS'
const fs = require('node:fs')
const source = fs.readFileSync('src/shared/cost.ts', 'utf8')
const start = source.indexOf('export function calculateApiCostOpenAI')
const end = source.indexOf('\n}', start) + 2
const fnText = source.slice(start, end)
const fn = new Function(...['inputTokens', 'outputTokens', 'cacheWriteTokens', 'cacheReadTokens'],
source.slice(start).replace(/export function [^(]+?\(([\s\S]*?)\)/, 'function fake($1)')
.replace(/^const .*/, '')
.replace(/const .*?;.*?$/, '')
.trim())
console.log(fnText)
const cases = [
[1000, 500, 100, 0],
[1000, 500, 100, 200],
[1000, 500, 100, 0.00008 * 1000],
]
for (const [inputTokens, outputTokens, cacheWriteTokens, cacheReadTokens] of cases) {
console.log({ inputTokens, outputTokens, cacheWriteTokens, cacheReadTokens, totalCost: fn(inputTokens, outputTokens, cacheWriteTokens, cacheReadTokens) })
}
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 8617
Read OpenAI cache tokens from usage.prompt_tokens_details.cached_tokens.
OpenAI Chat Completions usage reports cached prompt tokens in prompt_tokens_details.cached_tokens, while this handler falls back to nonstandard cache_read_input_tokens. Use prompt_tokens_details.cached_tokens as the primary cache-read source, with nonstandard fields only as compatibility fallbacks. Update the OpenAI usage test to mirror this shape and assert the expected totalCost.
📍 Affects 2 files
src/api/providers/openai.ts#L288-L304(this comment)src/api/providers/__tests__/openai.spec.ts#L1597-L1604
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/openai.ts` around lines 288 - 304, Update the usage parsing
in the OpenAI handler around calculateApiCostOpenAI to read cache-read tokens
primarily from usage.prompt_tokens_details.cached_tokens, retaining
cache_read_input_tokens only as a compatibility fallback. In
src/api/providers/__tests__/openai.spec.ts lines 1597-1604, mirror the
prompt_tokens_details.cached_tokens usage shape and assert the resulting
totalCost.
Comment on lines
+968
to
+969
| "strictToolSchemas": "Strict tool schemas", | ||
| "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting. (This setting is saved per profile and also applies to other providers that use the OpenAI protocol within the same profile.)" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new strict-schema strings in each non-English locale.
The new keys contain English text in all seven affected locale files. Add accurate translations for both keys at each site.
webview-ui/src/i18n/locales/ca/settings.json#L968-L969: add Catalan translations.webview-ui/src/i18n/locales/de/settings.json#L968-L969: add German translations.webview-ui/src/i18n/locales/es/settings.json#L968-L969: add Spanish translations.webview-ui/src/i18n/locales/fr/settings.json#L968-L969: add French translations.webview-ui/src/i18n/locales/hi/settings.json#L968-L969: add Hindi translations.webview-ui/src/i18n/locales/id/settings.json#L968-L969: add Indonesian translations.webview-ui/src/i18n/locales/it/settings.json#L968-L969: add Italian translations.
📍 Affects 7 files
webview-ui/src/i18n/locales/ca/settings.json#L968-L969(this comment)webview-ui/src/i18n/locales/de/settings.json#L968-L969webview-ui/src/i18n/locales/es/settings.json#L968-L969webview-ui/src/i18n/locales/fr/settings.json#L968-L969webview-ui/src/i18n/locales/hi/settings.json#L968-L969webview-ui/src/i18n/locales/id/settings.json#L968-L969webview-ui/src/i18n/locales/it/settings.json#L968-L969
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/ca/settings.json` around lines 968 - 969,
Translate both strictToolSchemas and strictToolSchemasDescription from English
into the target language at webview-ui/src/i18n/locales/ca/settings.json lines
968-969 (Catalan), webview-ui/src/i18n/locales/de/settings.json lines 968-969
(German), webview-ui/src/i18n/locales/es/settings.json lines 968-969 (Spanish),
webview-ui/src/i18n/locales/fr/settings.json lines 968-969 (French),
webview-ui/src/i18n/locales/hi/settings.json lines 968-969 (Hindi),
webview-ui/src/i18n/locales/id/settings.json lines 968-969 (Indonesian), and
webview-ui/src/i18n/locales/it/settings.json lines 968-969 (Italian), preserving
the existing JSON keys and the complete meaning of each description.
| "useAzure": "Use Azure", | ||
| "azureApiVersion": "Set Azure API version", | ||
| "strictToolSchemas": "Strict tool schemas", | ||
| "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting. This setting is saved per profile and also applies to other providers that use the OpenAI protocol within the same profile.", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'strict|openAiToolStrictMode' src/api/providers/base-provider.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3570
Avoid naming tool outputs as the validation target.
strictMode is applied to function.parameters schema when converting tools. Use wording like “ensuring tool-call call arguments match the schema exactly” instead of “tool outputs.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/en/settings.json` at line 1044, Update the
strictToolSchemasDescription text to identify tool-call arguments, not tool
outputs, as the values validated against the function.parameters schema;
preserve the existing provider, MCP, profile, and OpenAI protocol details.
Comment on lines
+968
to
+969
| "strictToolSchemas": "Strict tool schemas", | ||
| "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting. (This setting is saved per profile and also applies to other providers that use the OpenAI protocol within the same profile.)" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new Japanese locale entries.
strictToolSchemas and strictToolSchemasDescription are English. Provide Japanese translations so the Japanese settings view remains localized.
Proposed fix
- "strictToolSchemas": "Strict tool schemas",
- "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting. (This setting is saved per profile and also applies to other providers that use the OpenAI protocol within the same profile.)"
+ "strictToolSchemas": "厳密なツールスキーマ",
+ "strictToolSchemasDescription": "関数ツールスキーマの厳密モードを有効にし、ツール出力がスキーマに完全に一致するようにします。一部のプロバイダーは厳密モードをサポートしていない場合があります。MCP ツールは、この設定にかかわらず常に非厳密のままです。この設定はプロファイルごとに保存され、同じプロファイル内で OpenAI プロトコルを使用する他のプロバイダーにも適用されます。"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "strictToolSchemas": "Strict tool schemas", | |
| "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting. (This setting is saved per profile and also applies to other providers that use the OpenAI protocol within the same profile.)" | |
| "strictToolSchemas": "厳密なツールスキーマ", | |
| "strictToolSchemasDescription": "関数ツールスキーマの厳密モードを有効にし、ツール出力がスキーマに完全に一致するようにします。一部のプロバイダーは厳密モードをサポートしていない場合があります。MCP ツールは、この設定にかかわらず常に非厳密のままです。この設定はプロファイルごとに保存され、同じプロファイル内で OpenAI プロトコルを使用する他のプロバイダーにも適用されます。" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/ja/settings.json` around lines 968 - 969,
Translate the strictToolSchemas and strictToolSchemasDescription values in the
Japanese locale, preserving the original meaning and the existing JSON keys so
the settings view is fully localized.
Comment on lines
+968
to
+969
| "strictToolSchemas": "Strict tool schemas", | ||
| "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting. (This setting is saved per profile and also applies to other providers that use the OpenAI protocol within the same profile.)" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new locale entries.
These non-English locale files add English values for both strict-schema strings. Users of these locales will see untranslated settings text.
webview-ui/src/i18n/locales/ko/settings.json#L968-L969: add Korean translations.webview-ui/src/i18n/locales/nl/settings.json#L968-L969: add Dutch translations.webview-ui/src/i18n/locales/pl/settings.json#L968-L969: add Polish translations.webview-ui/src/i18n/locales/pt-BR/settings.json#L968-L969: add Brazilian Portuguese translations.webview-ui/src/i18n/locales/ru/settings.json#L968-L969: add Russian translations.webview-ui/src/i18n/locales/tr/settings.json#L968-L969: add Turkish translations.webview-ui/src/i18n/locales/vi/settings.json#L968-L969: add Vietnamese translations.webview-ui/src/i18n/locales/zh-CN/settings.json#L968-L969: add Simplified Chinese translations.webview-ui/src/i18n/locales/zh-TW/settings.json#L995-L996: add Traditional Chinese translations.
📍 Affects 9 files
webview-ui/src/i18n/locales/ko/settings.json#L968-L969(this comment)webview-ui/src/i18n/locales/nl/settings.json#L968-L969webview-ui/src/i18n/locales/pl/settings.json#L968-L969webview-ui/src/i18n/locales/pt-BR/settings.json#L968-L969webview-ui/src/i18n/locales/ru/settings.json#L968-L969webview-ui/src/i18n/locales/tr/settings.json#L968-L969webview-ui/src/i18n/locales/vi/settings.json#L968-L969webview-ui/src/i18n/locales/zh-CN/settings.json#L968-L969webview-ui/src/i18n/locales/zh-TW/settings.json#L995-L996
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/ko/settings.json` around lines 968 - 969,
Translate the strictToolSchemas and strictToolSchemasDescription entries into
the appropriate locale language, replacing the English values in
webview-ui/src/i18n/locales/ko/settings.json#L968-L969,
webview-ui/src/i18n/locales/nl/settings.json#L968-L969,
webview-ui/src/i18n/locales/pl/settings.json#L968-L969,
webview-ui/src/i18n/locales/pt-BR/settings.json#L968-L969,
webview-ui/src/i18n/locales/ru/settings.json#L968-L969,
webview-ui/src/i18n/locales/tr/settings.json#L968-L969,
webview-ui/src/i18n/locales/vi/settings.json#L968-L969,
webview-ui/src/i18n/locales/zh-CN/settings.json#L968-L969, and
webview-ui/src/i18n/locales/zh-TW/settings.json#L995-L996. Preserve both keys
and the full meaning of the strict-mode, provider-support, MCP exception, and
per-profile behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/260805_0001_session_ci-all-green/055100_code-b13-coverage-tests-report.md (1)
66-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language identifiers to fenced code blocks.
Markdownlint reports MD040 for each fence in this range. Use
shellfor command blocks andtextfor commit and push output blocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/260805_0001_session_ci-all-green/055100_code-b13-coverage-tests-report.md` around lines 66 - 83, Update the fenced code blocks in the documented test, commit, and push sections with language identifiers: use shell for the command block and text for the commit and push output blocks, preserving their contents unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/260805_0001_session_ci-all-green/150000_debug-coverage-b17.md`:
- Around line 9-14: Update the coverage summary in the documented analysis to
state that two files contain uncovered lines, while identifying only
src/api/providers/mistral.ts as below the 80% threshold; keep
src/api/providers/openai-compatible.ts listed with its 92.9% coverage but remove
it from the below-threshold count.
In `@scripts/coverage-analysis.py`:
- Around line 35-56: Update the subprocess.run call in the added-line parsing
function to use check=True so Git failures raise instead of producing empty
output. Remove the broad exception handling that converts failures into [] and
let the error propagate, preserving [] only for a genuinely empty successful
diff.
In `@src/api/providers/__tests__/mistral.spec.ts`:
- Around line 311-314: Update the getModel mock fixture in the missing model
info test to avoid the `undefined as any` suppression: use a type-safe fixture
representing absent metadata, or make the model’s info property optional if that
is valid for providers. Then run the specified ESLint command and resolve any
resulting issues.
---
Nitpick comments:
In
`@docs/260805_0001_session_ci-all-green/055100_code-b13-coverage-tests-report.md`:
- Around line 66-83: Update the fenced code blocks in the documented test,
commit, and push sections with language identifiers: use shell for the command
block and text for the commit and push output blocks, preserving their contents
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb092ac3-28f2-4207-b2fa-d01eb9fa0dd6
📒 Files selected for processing (4)
docs/260805_0001_session_ci-all-green/055100_code-b13-coverage-tests-report.mddocs/260805_0001_session_ci-all-green/150000_debug-coverage-b17.mdscripts/coverage-analysis.pysrc/api/providers/__tests__/mistral.spec.ts
Comment on lines
+9
to
+14
| Coverage analysis of all 19 source files in the PR diff reveals **2 files with patch coverage below 80%**: | ||
|
|
||
| 1. **`src/api/providers/mistral.ts`** — 30.8% patch coverage (9 of 13 new lines uncovered) | ||
| 2. **`src/api/providers/openai-compatible.ts`** — 92.9% patch coverage (1 of 14 new lines uncovered) | ||
|
|
||
| All other 17 source files have **100% patch coverage** on new lines. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the threshold summary.
src/api/providers/openai-compatible.ts has 92.9% patch coverage in the table. It is not below the 80% threshold. State that two files have uncovered lines and that only src/api/providers/mistral.ts is below 80%.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260805_0001_session_ci-all-green/150000_debug-coverage-b17.md` around
lines 9 - 14, Update the coverage summary in the documented analysis to state
that two files contain uncovered lines, while identifying only
src/api/providers/mistral.ts as below the 80% threshold; keep
src/api/providers/openai-compatible.ts listed with its 92.9% coverage but remove
it from the below-threshold count.
Comment on lines
+35
to
+56
| result = subprocess.run( | ||
| ["git", "diff", BASE, "HEAD", "--unified=0", "--", filepath], | ||
| capture_output=True, text=True, cwd=os.getcwd() | ||
| ) | ||
| added_lines = [] | ||
| current_new_line = 0 | ||
| for line in result.stdout.splitlines(): | ||
| # Parse hunk header: @@ -old_start,old_count +new_start,new_count @@ | ||
| m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@', line) | ||
| if m: | ||
| current_new_line = int(m.group(1)) | ||
| continue | ||
| if line.startswith('+') and not line.startswith('+++'): | ||
| added_lines.append(current_new_line) | ||
| current_new_line += 1 | ||
| elif line.startswith('-') and not line.startswith('---'): | ||
| pass # removed line, don't advance new line counter | ||
| else: | ||
| current_new_line += 1 | ||
| return added_lines | ||
| except Exception as e: | ||
| return [] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail when git diff fails.
If git diff fails, this function returns []. Lines 148-151 then report 100% patch coverage for that file. This can hide an invalid BASE, a shallow checkout, or another Git failure.
Use check=True and propagate the failure. Do not convert a failed diff into an empty diff.
Proposed fix
result = subprocess.run(
["git", "diff", BASE, "HEAD", "--unified=0", "--", filepath],
- capture_output=True, text=True, cwd=os.getcwd()
+ capture_output=True, text=True, cwd=os.getcwd(), check=True
)
@@
- except Exception as e:
- return []🧰 Tools
🪛 Ruff (0.16.1)
[error] 35-35: subprocess call: check for execution of untrusted input
(S603)
[error] 36-36: Starting a process with a partial executable path
(S607)
[warning] 55-55: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/coverage-analysis.py` around lines 35 - 56, Update the subprocess.run
call in the added-line parsing function to use check=True so Git failures raise
instead of producing empty output. Remove the broad exception handling that
converts failures into [] and let the error propagate, preserving [] only for a
genuinely empty successful diff.
Comment on lines
+311
to
+314
| vi.spyOn(handler, "getModel").mockReturnValueOnce({ | ||
| id: "codestral-latest", | ||
| info: undefined as any, | ||
| maxTokens: 8192, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files related to mistral:"
git ls-files | rg '(^|/)mistral(\.spec)?\.ts$|api/providers/__tests__/mistral.spec.ts'
echo
echo "Target snippet and nearby context:"
sed -n '280,340p' src/api/providers/__tests__/mistral.spec.ts
echo
echo "Search for getModel and model fixture types in provider tests:"
rg -n 'getModel\($|getModel\(\)|info: undefined|codestral-latest' src/api/providers src -g '*.ts' | head -200Repository: Zoo-Code-Org/Zoo-Code
Length of output: 19857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target provider and base types:"
sed -n '1,240p' src/api/providers/mistral.ts
echo
sed -n '1,80p' src/api/index.ts
echo
sed -n '1,40p' packages/types/src/providers/mistral.ts
echo
echo "Relevant imports/types in mistral spec:"
sed -n '1,80p' src/api/providers/__tests__/mistral.spec.ts
echo
echo "Lint configuration suppressing or enforcing no-explicit-any / eslint tests:"
rg -n 'no-explicit-any|tslint|eslint|suppress' package.json pnpm-lock.yaml src .eslintrc* eslint.config.* --glob '!node_modules' --glob '!dist' --glob '!build' 2>/dev/null | head -120 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 20335
Use a typed getModel() fixture for the missing model info case.
The mock now suppresses the real return type; replace info: undefined as any with a typed value or make info optional if providers can legitimately return missing model metadata. Then run pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 api/providers/__tests__/mistral.spec.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/__tests__/mistral.spec.ts` around lines 311 - 314, Update
the getModel mock fixture in the missing model info test to avoid the `undefined
as any` suppression: use a type-safe fixture representing absent metadata, or
make the model’s info property optional if that is valid for providers. Then run
the specified ESLint command and resolve any resulting issues.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack Position
feat/openai-compatible-strict-reasoningDescription
Full Feature Description
feat/openai-compatible-strict-reasoningprovider-settings.ts,base-openai-compatible-provider.ts,base-provider.ts,OpenAICompatible.tsx,openai.ts,openai-compatible.ts,anthropic-vertex.ts,qwen-code.ts.cachedStatebefore saving. Cost calculation treats missing fields as unknown or zero per provider contract and does not produce negative tokens. Cached input/output tokens and provider-specific price units are not double-counted. B17 does not change request payload or tool-call policy.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Normalizes OpenAI/OpenAI-compatible/Anthropic Vertex/Qwen usage fields and cached tokens, and calculates cost according to provider price lookup. Does not change request payload, strict UI, or MiMo tool policy.
Included Files
src/api/providers/openai.tssrc/api/providers/openai-compatible.tssrc/api/providers/anthropic-vertex.tssrc/api/providers/qwen-code.tsExclusion Scope
Summary by CodeRabbit