feat: openai-compatible-strict-reasoning (2/2) - #1132
Open
myk1yt wants to merge 23 commits into
Open
Conversation
myk1yt
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
August 4, 2026 11:47
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds model-specific tool-call policies, ghost-call quarantine, enforcement telemetry, strict OpenAI tool schemas, provider request handling, settings UI, localization, typed provider tests, branch-cleanup reports, and a coverage diff analysis script. ChangesTool-call policy and provider behavior
Strict OpenAI tool schemas
Validation and repository reports
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 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__/mimo.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/task/__tests__/ghost-quarantine.spec.tsESLint skipped: the ESLint configuration for this file 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
![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.
myk1yt
force-pushed
the
pr/b17-provider-cost-v2
branch
from
August 5, 2026 08:45
225ebeb to
6d8176a
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
packages/types/src/telemetry.ts-222-250 (1)
222-250: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConstrain
policySourceandenforcementto their declared enum values.The tool-call policy domain values are finite, but these Zod schemas accept arbitrary strings. Invalid data can pass validation, including misspelled values such as
"providre". Use sharedz.enumschemas for both event variants and add parsing tests that reject invalid values.Proposed fix
+const toolCallPolicySourceSchema = z.enum([ + "model-capability", + "provider-default", + "user-setting", + "adaptive-circuit", +]) +const toolCallEnforcementSchema = z.enum(["provider", "local", "provider-and-local"]) + - policySource: z.string(), + policySource: toolCallPolicySourceSchema, - enforcement: z.string(), + enforcement: toolCallEnforcementSchema,🤖 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/telemetry.ts` around lines 222 - 250, Update the TOOL_CALL_POLICY_RESOLUTION and TOOL_CALL_ENFORCEMENT schemas in the telemetry event union so policySource and enforcement use the shared z.enum schemas for their declared domain values instead of z.string(). Add parsing tests covering both event variants that reject misspelled or otherwise invalid policySource and enforcement values.src/api/providers/__tests__/mimo.spec.ts-1915-1921 (1)
1915-1921: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the test title or the fixture status.
The title states "without status 400". The fixture sets
status: 400on line 1919. The test therefore does not cover the branch its name describes, because themessage.includes("parallel_tool_calls")clause and thestatus === 400clause both hold.Set the status to a non-400 value to cover the message-only clause, or rename the test.
💚 Proposed fix
- const rejectionError = Object.assign(new Error("400 - Unrecognized parameter: parallel_tool_calls"), { - status: 400, + const rejectionError = Object.assign(new Error("Unrecognized parameter: parallel_tool_calls"), { + status: 500, })🤖 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__/mimo.spec.ts` around lines 1915 - 1921, Update the test case title and fixture in the parallel_tool_calls retry test so they describe the same branch: use a non-400 status while retaining the message containing “parallel_tool_calls” to exercise the message-only condition.src/api/providers/__tests__/mistral.spec.ts-276-277 (1)
276-277: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an independent expected cost.
expectedCostis calculated withcalculateApiCostOpenAI. This test cannot detect a pricing regression in that helper because the expected value changes with the same implementation. Assert the numeric cost derived from thecodestral-latestprices, or calculate it independently.🤖 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 276 - 277, Update the test around mistralModels["codestral-latest"] so expectedCost is an independent fixed numeric value derived from that model’s pricing, rather than calling calculateApiCostOpenAI. Keep the existing assertion and input quantities unchanged.docs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.md-18-18 (1)
18-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the test-count arithmetic.
The report states “20 tests” and “18 existing + 3 new.” Those values total 21. Update Line 18 and the repeated total on Line 23 so the report contains one consistent count.
🤖 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/150700_code-b17-mistral-coverage-report.md` at line 18, Correct the test-count arithmetic in the coverage report by making the total on Line 18 and the repeated total on Line 23 consistent with “18 existing + 3 new,” which equals 21 tests.src/api/providers/__tests__/mistral.spec.ts-308-317 (1)
308-317: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the
includeMaxTokens = falsemissing-model path.
getModel()returnsinfofrommistralModels, andincludeMaxTokenscomes from request options, so this test can still exercisemaxTokens ?? info.maxTokenswith both values absent. Either add that fixture and make the fallback null-safe, or document thatincludeMaxTokensis required.🤖 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 308 - 317, Update the missing-model fixture around handler.getModel to set includeMaxTokens to false and omit maxTokens, so the test exercises the fallback with both values absent. Make the maxTokens fallback null-safe when model info is undefined, while preserving the existing behavior when model metadata is available.src/core/assistant-message/ToolCallRetentionPolicy.ts-216-217 (1)
216-217: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
callCountdisagrees with its documented contract at every emission site. The field is documented as the total tool calls in the turn including the ghost, but every producer computes a value that excludes the ghost. Pick one meaning and align the doc with all three producers.
src/core/assistant-message/ToolCallRetentionPolicy.ts#L216-L217: this is the contract. Either keep "including the ghost" and fix the producers, or reword to "tool calls remaining after the drop" and keep the producers.src/core/task/Task.ts#L3001-L3003: the filter runs after thespliceat Line 2975, so the ghost is already gone. To match the current wording, capture the count before the splice.src/core/task/Task.ts#L3092-L3094: the legacy path never pushes a block, so the count omits the ghost. Add 1 to match the current wording.src/core/task/Task.ts#L3492-L3495: same post-splice ordering as the streaming path; apply the same fix.Also update the assertion at
src/core/task/__tests__/ghost-quarantine.spec.tsLine 723, which currently encodes the post-splice value.🤖 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/core/assistant-message/ToolCallRetentionPolicy.ts` around lines 216 - 217, Keep ToolCallRetentionPolicy.callCount documented as the total tool calls including the ghost, and align every producer with that contract: in src/core/task/Task.ts lines 3001-3003 and 3492-3495 capture the count before the post-splice filter, and in lines 3092-3094 add the omitted ghost to the legacy-path count. Update the corresponding assertion in src/core/task/__tests__/ghost-quarantine.spec.ts line 723 to expect the pre-splice total.src/core/assistant-message/NativeToolCallParser.ts-1190-1219 (1)
1190-1219: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStore a valid error message for structural parser failures.
These throws are caught, but the legacy string channel only records
String(error). For the tagged plain objects, that becomes"[object Object]", soconsole.errorandconsumeParseError()show an unreadable message. Use a smallErrorsubclass that carries the same fields, or compute a descriptive string before storing the legacy error.🤖 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/core/assistant-message/NativeToolCallParser.ts` around lines 1190 - 1219, Update the structural failure throws in NativeToolCallParser’s parser path to preserve a readable legacy error message when consumed through console.error or consumeParseError(). Replace the tagged plain-object throws, or derive their messages before storage, so invalid_argument_shape and missing_required_arguments retain their existing fields while also producing descriptive String(error) output.Source: Coding guidelines
docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md-37-37 (1)
37-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language tags to the fenced blocks.
markdownlintreports MD040 for the commit-list and import-example fences at Lines 37, 46, 74, 101, 115, 123, 172, and 239. Addtextto commit-list fences andtsto the TypeScript import example.Also applies to: 46-46, 74-74, 101-101, 115-115, 123-123, 172-172, 239-239
🤖 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/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md` at line 37, Update the fenced code blocks in the runbook so every commit-list fence is tagged with text and the TypeScript import example fence is tagged with ts, covering all listed locations and preserving their contents.Source: Linters/SAST tools
docs/260730_0001_session_branch-cleanup/170000_debug-report.md-50-50 (1)
50-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSurround all Markdown tables with blank lines.
docs/260730_0001_session_branch-cleanup/170000_debug-report.md#L50-L50: add a blank line before the verification table.docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md#L192-L192: add blank lines around the first coverage table.docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md#L205-L205: add blank lines around the second coverage table.docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md#L213-L213: add blank lines around the third coverage table.🤖 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/260730_0001_session_branch-cleanup/170000_debug-report.md` at line 50, Surround every Markdown table with blank lines: add a blank line before and after the verification table in docs/260730_0001_session_branch-cleanup/170000_debug-report.md (lines 50-50), and before and after each of the three coverage tables in docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md (lines 192-192, 205-205, and 213-213).Source: Linters/SAST tools
scripts/coverage-diff-analysis.py-55-56 (1)
55-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the reported Ruff findings in the output loop.
Use
added_lines.get(f)for RUF019. Renamelto a descriptive name such asline_textfor E741.Suggested cleanup
- if f in added_lines and added_lines[f]: + if added_lines.get(f): ... - for i, l in enumerate(hunk["lines"]): - print(f" {start + i}: {l.rstrip()}") + for i, line_text in enumerate(hunk["lines"]): + print(f" {start + i}: {line_text.rstrip()}")Also applies to: 64-65
🤖 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-diff-analysis.py` around lines 55 - 56, Update the output loop over source_files to use added_lines.get(f) instead of directly indexing or checking added_lines[f], and rename the ambiguous loop variable l to a descriptive name such as line_text throughout the affected logic to resolve the Ruff findings.Source: Linters/SAST tools
docs/260730_0001_session_branch-cleanup/173230_execution-plan.md-33-42 (1)
33-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the stale Step 3 rebase command.
The first
git rebase --onto main d27153a25 feat/error-interception-middleware-cleanoperates on the branch created frommain. It does not replayfeat/error-interception-remote-src. Keep only the command that checks out and rebasesfeat/error-interception-remote-src.Suggested runbook correction
- git rebase --onto main d27153a25 feat/error-interception-middleware-clean - # (clean branch is at main; instead rebase the remote source series) - - **Corrected command** (rebase the source series, landing on the clean branch name): git checkout feat/error-interception-remote-src git rebase --onto main d27153a25 feat/error-interception-remote-src + git checkout feat/error-interception-remote-src + git rebase --onto main d27153a25 feat/error-interception-remote-src🤖 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/260730_0001_session_branch-cleanup/173230_execution-plan.md` around lines 33 - 42, Remove the stale `git rebase --onto main d27153a25 feat/error-interception-middleware-clean` command and its explanatory note from Step 3. Keep only the `git checkout feat/error-interception-remote-src` and corresponding rebase command that replays the remote source series.docs/260730_0001_session_branch-cleanup/184700_debug-report.md-11-11 (1)
11-11: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSecurity And Privacy (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Remove the developer-local absolute path from the report.
Line 11 commits
C:/Users/k1yt/OneDrive/Projects/ZooCode, which exposes the developer username and local OneDrive repository layout. Replace it with a repository-relative placeholder such as<repo-root>unless this report is guaranteed to remain private.🤖 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/260730_0001_session_branch-cleanup/184700_debug-report.md` at line 11, Replace the developer-local absolute path in the report’s “Workspace repo root” entry with a repository-relative placeholder such as <repo-root>, preserving the surrounding report text and formatting.
🧹 Nitpick comments (7)
src/api/providers/__tests__/mimo.spec.ts (4)
77-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse bracket notation for the private
optionsmember.The coding guidelines prefer bracket notation over a double assertion when a test reads a private member. Bracket notation also keeps the property type from the class instead of re-declaring it.
Apply the same change at line 85.
♻️ Proposed refactor
- expect((h as unknown as { options: { openAiBaseUrl: string } }).options.openAiBaseUrl).toBe( - "https://token-plan-sgp.xiaomimimo.com/v1", - ) + expect(h["options"].openAiBaseUrl).toBe("https://token-plan-sgp.xiaomimimo.com/v1")🤖 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__/mimo.spec.ts` around lines 77 - 85, Update the `MimoHandler` option assertions in the affected tests to access the private `options` member with bracket notation instead of casting to a re-declared `{ options: ... }` type; apply this consistently to both base-URL assertions, including the custom URL case.Source: Coding guidelines
127-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the double assertions.
These fixtures build content blocks that Anthropic's
MessageParamtype does not declare, such asreasoning. The double assertion is therefore necessary. The coding guidelines require a comment that explains each double assertion. Add one short comment per fixture.Also applies to: 215-230, 346-357, 693-695
🤖 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__/mimo.spec.ts` around lines 127 - 139, Explain each double type assertion in the Mimo test fixtures by adding a brief comment immediately before every affected assertion, including the fixtures around the reasoning content and the additional locations noted in the review. State that the fixture intentionally uses Anthropic content blocks, such as reasoning, that are not declared by MessageParam and therefore require the double assertion.Source: Coding guidelines
2110-2112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the hard-coded source line numbers from the comments.
These comments cite absolute line numbers in
src/api/providers/mimo.ts, such as "line 103-105", "lines 136-139", and "line 108". Any edit to that file makes the references wrong. Describe the branch instead, for example "the early return whendelta.tool_callsis absent or empty".Also applies to: 2142-2143, 2173-2174, 2218-2219
🤖 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__/mimo.spec.ts` around lines 2110 - 2112, Remove hard-coded source line references from the comments in the affected mimo provider tests. Update each comment near the tests around lines 2110, 2142, 2173, and 2218 to describe the relevant behavior or branch by symbol, such as the early return when delta.tool_calls is absent or empty, without citing line numbers.
469-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a failing retry.
Every retry test mocks the second
createcall as a success. No test covers the case where the retry also rejects. That path currently leaks the raw SDK error, as described in the comment onsrc/api/providers/mimo.tslines 243-262.Add a test that rejects both calls and asserts the thrown error carries the normalized MiMo message and the preserved
status.🤖 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__/mimo.spec.ts` around lines 469 - 519, The existing retry coverage only verifies a successful second create call; add a test near the current createMessage retry test that makes both mockCreate calls reject. Consume handler.createMessage and assert the final thrown error uses the normalized MiMo error message while preserving the original status value, covering the retry failure path in createMessage.src/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.ts (1)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the undocumented double assertions with
vi.mocked.Both lines use
as unknown as Mock. The coding guidelines allow a double assertion only as a last resort and require a comment.vi.mockedremoves the need for the assertion here and keeps the mock typing.♻️ Proposed fix
-const mockCaptureToolCallEnforcement = TelemetryService.instance.captureToolCallEnforcement as unknown as Mock -const mockHasInstance = TelemetryService.hasInstance as unknown as Mock +const mockCaptureToolCallEnforcement = vi.mocked(TelemetryService.instance.captureToolCallEnforcement) +const mockHasInstance = vi.mocked(TelemetryService.hasInstance)Remove the now-unused
import type { Mock } from "vitest"at Line 4 if no other reference remains.As per coding guidelines: "Use double assertions only as a last resort and explain them with a comment."
🤖 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/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.ts` around lines 23 - 24, Replace the double assertions in the mock declarations for captureToolCallEnforcement and hasInstance with vi.mocked, preserving their mocked typing. Remove the Mock type import if it is no longer referenced elsewhere in the test.Source: Coding guidelines
src/core/task/__tests__/ghost-quarantine.spec.ts (1)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the three
as anycasts by typingprovidercorrectly.
telemetryContext.provideris inferred asstring, so eachresolveToolCallPolicycall needs a cast. Declare the context with the provider identifier type thatresolveToolCallPolicyaccepts. All three casts then disappear.♻️ Proposed fix
+import type { ProviderSettings } from "`@roo-code/types`" + +type ApiProvider = NonNullable<ProviderSettings["apiProvider"]> + interface TelemetryContext { taskId: string - provider: string + provider: ApiProvider model: string modelInfo: ModelInfo }Use
TelemetryContextfor the three helper parameters, then:- const ghostPolicy1 = resolveToolCallPolicy(telemetryContext.modelInfo, telemetryContext.provider as any) + const ghostPolicy1 = resolveToolCallPolicy(telemetryContext.modelInfo, telemetryContext.provider)Apply the same change at Lines 135 and 198.
GhostDropTelemetry.providercan staystring, matching the telemetry payload.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards."Also applies to: 135-135, 198-198
🤖 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/core/task/__tests__/ghost-quarantine.spec.ts` at line 94, Update the test helper context parameters to use TelemetryContext, ensuring provider has the identifier type accepted by resolveToolCallPolicy. Apply this to all three helper call sites around ghostPolicy1 and the corresponding lines, then remove each provider as any cast while leaving GhostDropTelemetry.provider typed as string.Source: Coding guidelines
src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts (1)
393-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the other two failure kinds.
The suite asserts only
missing_required_arguments.ParserFailureKindhas three members. Add one case forjson_syntaxand one forinvalid_argument_shapeso a future change toclassifyParseFailurecannot silently collapse the kinds.💚 Proposed additional cases
it("classifies malformed JSON as json_syntax", () => { NativeToolCallParser.parseToolCall({ id: "call_syntax", name: "read_file", arguments: "{not valid json", }) const failure = NativeToolCallParser.consumeParseFailure("call_syntax") expect(failure?.kind).toBe("json_syntax") expect(failure?.toolName).toBe("read_file") }) it("classifies a non-object argument payload as invalid_argument_shape", () => { NativeToolCallParser.parseToolCall({ id: "call_shape", name: "read_file", arguments: "[1,2,3]", }) const failure = NativeToolCallParser.consumeParseFailure("call_shape") expect(failure?.kind).toBe("invalid_argument_shape") expect(failure?.emptyArguments).toBe(false) })🤖 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/core/assistant-message/__tests__/NativeToolCallParser.spec.ts` around lines 393 - 411, Extend the NativeToolCallParser tests with separate cases covering malformed JSON and non-object argument payloads. Use parseToolCall and consumeParseFailure to assert json_syntax with the tool name, and invalid_argument_shape with emptyArguments false, ensuring all ParserFailureKind classifications remain distinct.
🤖 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/260730_0001_session_branch-cleanup/175300_code-report.md`:
- Around line 42-43: Update the Result summary to accurately report validation
status: retain that type checks and targeted suites passed, but state that four
pre-existing task-persistence test failures remain. Remove the claim that all
tests pass while preserving the branch cleanup and force-push details.
In `@docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`:
- Around line 319-325: Update the Rollback instructions to restore or return to
feature/task-dnd-ux rather than checking out feat/error-interception-middleware.
Preserve the existing cherry-pick abort and cleanup commands, and ensure
subsequent recovery commands operate from the original DND branch.
- Around line 237-253: Correct the cherry-pick guidance in the ClineProvider.ts
conflict section: during cherry-pick, preserve the clean branch’s existing
ClineProvider.ts by using the ours/HEAD version, not theirs. Keep the
taskOrganizationModel.ts and spec changes from commit 78ba8218 staged separately
before continuing the cherry-pick.
In `@docs/260730_0001_session_branch-cleanup/184700_debug-report.md`:
- Around line 101-110: Update the “0. Preconditions” instructions to require an
entirely empty git status --porcelain result before running git switch -C,
removing the exception for untracked docs/ files. If untracked files exist,
instruct the user to move them outside the repository before rebuilding and
retain the later diff checks only after the worktree is clean.
In `@docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md`:
- Around line 170-186: Update the test commands in the coverage workflow so each
command executes from the repository root context independently. Use subshells
or explicitly return to the root after the `src`, `packages/types`, and
`packages/telemetry` commands, ensuring `python
scripts/coverage-diff-analysis.py` resolves from the repository root.
In `@scripts/coverage-diff-analysis.py`:
- Around line 40-53: Update the diff parser around the hunk handling and
added-line collection to maintain a new-file line cursor initialized from each
hunk’s +start position. Advance the cursor for context lines and added lines,
record each added line with its current cursor value, and leave it unchanged for
deleted lines so generated ranges map to the correct source lines.
- Around line 29-35: Update the subprocess.run invocation that obtains the git
diff to fail immediately when Git exits unsuccessfully, using check=True or
equivalent explicit return-code validation before assigning or parsing
result.stdout; preserve normal stdout handling for successful diffs.
In `@src/api/index.ts`:
- Around line 222-230: Update the capability handling in the branch checking
supportsParallelToolCalls === false so enforcement is "provider-and-local" only
when parallelToolCallsRequestControl is explicitly "openai" or "anthropic";
return "local" for "none", "unknown", and other unsupported values. Add a
regression test covering false support with "unknown" control.
In `@src/api/providers/mimo.ts`:
- Around line 252-258: Update the retry construction in the strict-schema
rejection branch of the chat completion flow to call stripStrictFromTools with
params.tools rather than the unconverted tools metadata. Preserve the
already-converted request payload and remove the redundant nullish fallback,
while keeping the existing paramsWithoutStrict retry behavior unchanged.
- Around line 243-262: Update the retry branches in the chat completion flow
around createMessage so failures from both the parallel_tool_calls retry and the
strict-schema retry are passed through handleProviderError(error, "MiMo") before
escaping. Wrap each retry call consistently with the existing non-retry path,
remove the undocumented as MiMoCompletionParams assertion from the parallel-tool
retry, and preserve the existing retry selection behavior.
In `@src/core/task/Task.ts`:
- Around line 3452-3501: Extract the duplicated ghost-drop handling into one
private quarantineGhostToolCall(callId: string): boolean method. Move
classification, assistantMessageContent/index-map cleanup, parser-state
disposal, and metadata-only telemetry into it, returning true only when a
provably empty ghost is dropped; then replace the duplicated logic at all three
tool_call_end sites with the helper and skip finalization when it returns true.
- Around line 2964-3011: When removing the ghost block in the ghost-disposition
branch, also decrement currentStreamingContentIndex when it is greater than or
equal to ghostIndex so the presenter cursor remains aligned after the splice.
Keep the existing streamingToolCallIndices re-indexing and cleanup unchanged.
In `@src/shared/tools.ts`:
- Line 97: Align the execute_command timeout contract across the tool schema and
NativeToolArgs. Either normalize or reject null in NativeToolCallParser before
resolveAgentTimeoutMs receives it, or remove null from the declared schema so
only numbers are accepted; ensure resolveAgentTimeoutMs never receives an
unhandled null value.
In `@webview-ui/src/i18n/locales/en/settings.json`:
- Around line 1043-1051: Remove the duplicate strictToolSchemas and
strictToolSchemasDescription entries from the modelInfo localization object,
keeping exactly one identical pair so the object has no duplicate keys and
preserves the existing translations.
---
Minor comments:
In `@docs/260730_0001_session_branch-cleanup/170000_debug-report.md`:
- Line 50: Surround every Markdown table with blank lines: add a blank line
before and after the verification table in
docs/260730_0001_session_branch-cleanup/170000_debug-report.md (lines 50-50),
and before and after each of the three coverage tables in
docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md (lines
192-192, 205-205, and 213-213).
In `@docs/260730_0001_session_branch-cleanup/173230_execution-plan.md`:
- Around line 33-42: Remove the stale `git rebase --onto main d27153a25
feat/error-interception-middleware-clean` command and its explanatory note from
Step 3. Keep only the `git checkout feat/error-interception-remote-src` and
corresponding rebase command that replays the remote source series.
In `@docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`:
- Line 37: Update the fenced code blocks in the runbook so every commit-list
fence is tagged with text and the TypeScript import example fence is tagged with
ts, covering all listed locations and preserving their contents.
In `@docs/260730_0001_session_branch-cleanup/184700_debug-report.md`:
- Line 11: Replace the developer-local absolute path in the report’s “Workspace
repo root” entry with a repository-relative placeholder such as <repo-root>,
preserving the surrounding report text and formatting.
In
`@docs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.md`:
- Line 18: Correct the test-count arithmetic in the coverage report by making
the total on Line 18 and the repeated total on Line 23 consistent with “18
existing + 3 new,” which equals 21 tests.
In `@packages/types/src/telemetry.ts`:
- Around line 222-250: Update the TOOL_CALL_POLICY_RESOLUTION and
TOOL_CALL_ENFORCEMENT schemas in the telemetry event union so policySource and
enforcement use the shared z.enum schemas for their declared domain values
instead of z.string(). Add parsing tests covering both event variants that
reject misspelled or otherwise invalid policySource and enforcement values.
In `@scripts/coverage-diff-analysis.py`:
- Around line 55-56: Update the output loop over source_files to use
added_lines.get(f) instead of directly indexing or checking added_lines[f], and
rename the ambiguous loop variable l to a descriptive name such as line_text
throughout the affected logic to resolve the Ruff findings.
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 1915-1921: Update the test case title and fixture in the
parallel_tool_calls retry test so they describe the same branch: use a non-400
status while retaining the message containing “parallel_tool_calls” to exercise
the message-only condition.
In `@src/api/providers/__tests__/mistral.spec.ts`:
- Around line 276-277: Update the test around mistralModels["codestral-latest"]
so expectedCost is an independent fixed numeric value derived from that model’s
pricing, rather than calling calculateApiCostOpenAI. Keep the existing assertion
and input quantities unchanged.
- Around line 308-317: Update the missing-model fixture around handler.getModel
to set includeMaxTokens to false and omit maxTokens, so the test exercises the
fallback with both values absent. Make the maxTokens fallback null-safe when
model info is undefined, while preserving the existing behavior when model
metadata is available.
In `@src/core/assistant-message/NativeToolCallParser.ts`:
- Around line 1190-1219: Update the structural failure throws in
NativeToolCallParser’s parser path to preserve a readable legacy error message
when consumed through console.error or consumeParseError(). Replace the tagged
plain-object throws, or derive their messages before storage, so
invalid_argument_shape and missing_required_arguments retain their existing
fields while also producing descriptive String(error) output.
In `@src/core/assistant-message/ToolCallRetentionPolicy.ts`:
- Around line 216-217: Keep ToolCallRetentionPolicy.callCount documented as the
total tool calls including the ghost, and align every producer with that
contract: in src/core/task/Task.ts lines 3001-3003 and 3492-3495 capture the
count before the post-splice filter, and in lines 3092-3094 add the omitted
ghost to the legacy-path count. Update the corresponding assertion in
src/core/task/__tests__/ghost-quarantine.spec.ts line 723 to expect the
pre-splice total.
---
Nitpick comments:
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 77-85: Update the `MimoHandler` option assertions in the affected
tests to access the private `options` member with bracket notation instead of
casting to a re-declared `{ options: ... }` type; apply this consistently to
both base-URL assertions, including the custom URL case.
- Around line 127-139: Explain each double type assertion in the Mimo test
fixtures by adding a brief comment immediately before every affected assertion,
including the fixtures around the reasoning content and the additional locations
noted in the review. State that the fixture intentionally uses Anthropic content
blocks, such as reasoning, that are not declared by MessageParam and therefore
require the double assertion.
- Around line 2110-2112: Remove hard-coded source line references from the
comments in the affected mimo provider tests. Update each comment near the tests
around lines 2110, 2142, 2173, and 2218 to describe the relevant behavior or
branch by symbol, such as the early return when delta.tool_calls is absent or
empty, without citing line numbers.
- Around line 469-519: The existing retry coverage only verifies a successful
second create call; add a test near the current createMessage retry test that
makes both mockCreate calls reject. Consume handler.createMessage and assert the
final thrown error uses the normalized MiMo error message while preserving the
original status value, covering the retry failure path in createMessage.
In `@src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts`:
- Around line 393-411: Extend the NativeToolCallParser tests with separate cases
covering malformed JSON and non-object argument payloads. Use parseToolCall and
consumeParseFailure to assert json_syntax with the tool name, and
invalid_argument_shape with emptyArguments false, ensuring all ParserFailureKind
classifications remain distinct.
In
`@src/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.ts`:
- Around line 23-24: Replace the double assertions in the mock declarations for
captureToolCallEnforcement and hasInstance with vi.mocked, preserving their
mocked typing. Remove the Mock type import if it is no longer referenced
elsewhere in the test.
In `@src/core/task/__tests__/ghost-quarantine.spec.ts`:
- Line 94: Update the test helper context parameters to use TelemetryContext,
ensuring provider has the identifier type accepted by resolveToolCallPolicy.
Apply this to all three helper call sites around ghostPolicy1 and the
corresponding lines, then remove each provider as any cast while leaving
GhostDropTelemetry.provider typed as string.
🪄 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: 115df466-f88b-453c-ae89-50e76729c63c
📒 Files selected for processing (56)
docs/260730_0001_session_branch-cleanup/170000_debug-report.mddocs/260730_0001_session_branch-cleanup/173200_debug-report.mddocs/260730_0001_session_branch-cleanup/173230_execution-plan.mddocs/260730_0001_session_branch-cleanup/175300_code-report.mddocs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.mddocs/260730_0001_session_branch-cleanup/182225_code-report.mddocs/260730_0001_session_branch-cleanup/184700_debug-report.mddocs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.mddocs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.mdpackages/telemetry/src/TelemetryService.tspackages/types/src/__tests__/provider-settings.test.tspackages/types/src/model.tspackages/types/src/provider-settings.tspackages/types/src/providers/mimo.tspackages/types/src/telemetry.tsscripts/coverage-diff-analysis.pysrc/api/index.tssrc/api/providers/__tests__/base-provider.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/mistral.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/base-provider.tssrc/api/providers/mimo.tssrc/api/providers/openai.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/ToolCallRetentionPolicy.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy.spec.tssrc/core/prompts/tools/native-tools/execute_command.tssrc/core/task/Task.tssrc/core/task/__tests__/ghost-quarantine.spec.tssrc/core/task/__tests__/tool-call-policy.spec.tssrc/core/tools/ExecuteCommandTool.tssrc/eslint-suppressions.jsonsrc/shared/tools.tswebview-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
🚧 Files skipped from review as they are similar to previous changes (23)
- src/api/providers/base-openai-compatible-provider.ts
- webview-ui/src/i18n/locales/ca/settings.json
- webview-ui/src/i18n/locales/es/settings.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/fr/settings.json
- webview-ui/src/i18n/locales/ko/settings.json
- webview-ui/src/i18n/locales/de/settings.json
- webview-ui/src/i18n/locales/it/settings.json
- packages/types/src/tests/provider-settings.test.ts
- webview-ui/src/i18n/locales/hi/settings.json
- webview-ui/src/i18n/locales/pt-BR/settings.json
- webview-ui/src/i18n/locales/ru/settings.json
- webview-ui/src/i18n/locales/pl/settings.json
- webview-ui/src/components/settings/providers/OpenAICompatible.tsx
- packages/types/src/provider-settings.ts
- webview-ui/src/i18n/locales/nl/settings.json
- src/api/providers/base-provider.ts
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/i18n/locales/tr/settings.json
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/i18n/locales/id/settings.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- src/api/providers/tests/base-provider.spec.ts
Comment on lines
+42
to
+43
| ## Result | ||
| ✅ Success. The branch `feat/error-interception-middleware` now contains 17 clean feature commits rebased onto `main`, with all 20 contamination commits auto-dropped. All type checks and tests pass. Force-pushed to remote `myk1yt/feat/error-interception-middleware`. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Report the pre-existing failures in the result summary.
Lines 29-32 record four failed task-persistence tests. The result currently says that all tests pass. State that type checks and targeted suites passed, while four pre-existing failures remain.
Suggested wording
-✅ Success. The branch `feat/error-interception-middleware` now contains 17 clean feature commits rebased onto `main`, with all 20 contamination commits auto-dropped. All type checks and tests pass.
+✅ Success. The branch `feat/error-interception-middleware` now contains 17 clean feature commits rebased onto `main`, with all 20 contamination commits auto-dropped. Type checks and the targeted suites passed; four pre-existing `task-persistence` failures remain.📝 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
| ## Result | |
| ✅ Success. The branch `feat/error-interception-middleware` now contains 17 clean feature commits rebased onto `main`, with all 20 contamination commits auto-dropped. All type checks and tests pass. Force-pushed to remote `myk1yt/feat/error-interception-middleware`. | |
| ## Result | |
| ✅ Success. The branch `feat/error-interception-middleware` now contains 17 clean feature commits rebased onto `main`, with all 20 contamination commits auto-dropped. Type checks and the targeted suites passed; four pre-existing `task-persistence` failures remain. Force-pushed to remote `myk1yt/feat/error-interception-middleware`. |
🤖 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/260730_0001_session_branch-cleanup/175300_code-report.md` around lines
42 - 43, Update the Result summary to accurately report validation status:
retain that type checks and targeted suites passed, but state that four
pre-existing task-persistence test failures remain. Remove the claim that all
tests pass while preserving the branch cleanup and force-push details.
Comment on lines
+237
to
+253
| ### Step 3a — Resolve the EXPECTED `78ba8218e` ClineProvider conflict | ||
| The `78ba8218e` ClineProvider hunk **removes** the lines: | ||
| ``` | ||
| import type { ..., TaskOrganizationStateV1 } from "@roo-code/types" | ||
| import { createEmptyTaskOrganizationState } from "@roo-code/types" | ||
| ``` | ||
| But remote `0453c3a70` **actively uses** both (multi-line import). That hunk is a *regression | ||
| artifact of the contaminated base* — NOT a real fix. **Resolution: keep the remote (theirs during | ||
| cherry-pick) version of `ClineProvider.ts`, i.e. DROP the ClineProvider hunk entirely and keep | ||
| only the `taskOrganizationModel.ts` + spec changes.** | ||
|
|
||
| During `git cherry-pick` the conflicted file is the *new* commit applying onto remote HEAD, so: | ||
| ```powershell | ||
| git checkout --theirs src/core/webview/ClineProvider.ts # keep remote 0453c3a70 version | ||
| git add src/core/webview/ClineProvider.ts | ||
| # ensure the taskOrganizationModel.ts + spec hunks from 78ba8218e ARE staged, then: | ||
| git cherry-pick --continue |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the clean branch side when resolving ClineProvider.ts.
During git cherry-pick 78ba8218e, --theirs selects the cherry-picked commit from the contaminated branch. It does not select remote squash 0453c3a70. This imports the unrelated ClineProvider.ts changes documented in 182225_code-report.md.
Use --ours or restore HEAD for this file, then stage it. Preserve the taskOrganizationModel.ts and spec changes separately.
Suggested correction
- git checkout --theirs src/core/webview/ClineProvider.ts
+ git checkout --ours src/core/webview/ClineProvider.ts
git add src/core/webview/ClineProvider.ts📝 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
| ### Step 3a — Resolve the EXPECTED `78ba8218e` ClineProvider conflict | |
| The `78ba8218e` ClineProvider hunk **removes** the lines: | |
| ``` | |
| import type { ..., TaskOrganizationStateV1 } from "@roo-code/types" | |
| import { createEmptyTaskOrganizationState } from "@roo-code/types" | |
| ``` | |
| But remote `0453c3a70` **actively uses** both (multi-line import). That hunk is a *regression | |
| artifact of the contaminated base* — NOT a real fix. **Resolution: keep the remote (theirs during | |
| cherry-pick) version of `ClineProvider.ts`, i.e. DROP the ClineProvider hunk entirely and keep | |
| only the `taskOrganizationModel.ts` + spec changes.** | |
| During `git cherry-pick` the conflicted file is the *new* commit applying onto remote HEAD, so: | |
| ```powershell | |
| git checkout --theirs src/core/webview/ClineProvider.ts # keep remote 0453c3a70 version | |
| git add src/core/webview/ClineProvider.ts | |
| # ensure the taskOrganizationModel.ts + spec hunks from 78ba8218e ARE staged, then: | |
| git cherry-pick --continue | |
| ### Step 3a — Resolve the EXPECTED `78ba8218e` ClineProvider conflict | |
| The `78ba8218e` ClineProvider hunk **removes** the lines: |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 239-239: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`
around lines 237 - 253, Correct the cherry-pick guidance in the ClineProvider.ts
conflict section: during cherry-pick, preserve the clean branch’s existing
ClineProvider.ts by using the ours/HEAD version, not theirs. Keep the
taskOrganizationModel.ts and spec changes from commit 78ba8218 staged separately
before continuing the cherry-pick.
Comment on lines
+319
to
+325
| ## 7. Rollback | ||
| If verification fails before Step 6: | ||
| ```powershell | ||
| git cherry-pick --abort # if mid-cherry-pick | ||
| git checkout feat/error-interception-middleware # or any other working branch | ||
| git branch -D feature/task-dnd-ux-clean | ||
| # original feature/task-dnd-ux + contaminated-backup remain untouched |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the DND branch during rollback.
git checkout feat/error-interception-middleware is unrelated to the feature/task-dnd-ux cleanup. If verification fails, this command moves the operator to the ERROR branch and can make subsequent recovery commands target the wrong refs. Restore feature/task-dnd-ux, or capture the original checkout before Step 1.
🤖 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/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`
around lines 319 - 325, Update the Rollback instructions to restore or return to
feature/task-dnd-ux rather than checking out feat/error-interception-middleware.
Preserve the existing cherry-pick abort and cleanup commands, and ensure
subsequent recovery commands operate from the original DND branch.
Comment on lines
+101
to
+110
| # 0. Preconditions | ||
| git fetch upstream | ||
| git rev-parse upstream/main # expect 569b43df991b5c56ee21cac5514eff36dd40d217 | ||
| git status --porcelain # expect clean (currently on feature/task-dnd-ux; docs/ untracked is fine) | ||
|
|
||
| # 1. Backup (only recovery point — fork has no copy) | ||
| git branch fix/mimo-parallel-tool-call-policy-backup-260730 fix/mimo-parallel-tool-call-policy | ||
|
|
||
| # 2. Rebuild from upstream/main | ||
| git switch -C fix/mimo-parallel-tool-call-policy upstream/main |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Require a clean worktree before rebuilding the branch.
The precondition says that untracked docs/ files are fine. git switch -C can fail when untracked files would be overwritten. The later git diff checks also ignore untracked files. Require empty git status --porcelain, or move the untracked files outside the repository before switching.
Also applies to: 124-126
🤖 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/260730_0001_session_branch-cleanup/184700_debug-report.md` around lines
101 - 110, Update the “0. Preconditions” instructions to require an entirely
empty git status --porcelain result before running git switch -C, removing the
exception for untracked docs/ files. If untracked files exist, instruct the user
to move them outside the repository before rebuilding and retain the later diff
checks only after the worktree is clean.
Comment on lines
+170
to
+186
| # Run src tests with coverage | ||
| cd src && npx vitest run --coverage --reporter=verbose \ | ||
| api/providers/__tests__/ \ | ||
| core/assistant-message/__tests__/ \ | ||
| core/task/__tests__/tool-call-policy.spec.ts | ||
| # Result: 60 test files, 1355 passed, 1 skipped | ||
|
|
||
| # Run packages/types tests with coverage | ||
| cd packages/types && npx vitest run --coverage --reporter=verbose | ||
| # Result: All tests pass, 100% coverage on new files | ||
|
|
||
| # Run packages/telemetry tests with coverage | ||
| cd packages/telemetry && npx vitest run --coverage --reporter=verbose | ||
| # Result: 3 test files, 46 passed | ||
|
|
||
| # Analyze diff for added lines per source file | ||
| python scripts/coverage-diff-analysis.py |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the repository root between test commands.
cd src && ... leaves the shell in src. The next cd packages/types, cd packages/telemetry, and python scripts/coverage-diff-analysis.py then resolve relative to the wrong directory. Use subshells or return to the repository root after each command.
Suggested command correction
- cd src && npx vitest run --coverage --reporter=verbose \
+ (cd src && npx vitest run --coverage --reporter=verbose \
api/providers/__tests__/ \
core/assistant-message/__tests__/ \
- core/task/__tests__/tool-call-policy.spec.ts
+ core/task/__tests__/tool-call-policy.spec.ts)
- cd packages/types && npx vitest run --coverage --reporter=verbose
+ (cd packages/types && npx vitest run --coverage --reporter=verbose)
- cd packages/telemetry && npx vitest run --coverage --reporter=verbose
+ (cd packages/telemetry && npx vitest run --coverage --reporter=verbose)
python scripts/coverage-diff-analysis.py📝 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
| # Run src tests with coverage | |
| cd src && npx vitest run --coverage --reporter=verbose \ | |
| api/providers/__tests__/ \ | |
| core/assistant-message/__tests__/ \ | |
| core/task/__tests__/tool-call-policy.spec.ts | |
| # Result: 60 test files, 1355 passed, 1 skipped | |
| # Run packages/types tests with coverage | |
| cd packages/types && npx vitest run --coverage --reporter=verbose | |
| # Result: All tests pass, 100% coverage on new files | |
| # Run packages/telemetry tests with coverage | |
| cd packages/telemetry && npx vitest run --coverage --reporter=verbose | |
| # Result: 3 test files, 46 passed | |
| # Analyze diff for added lines per source file | |
| python scripts/coverage-diff-analysis.py | |
| # Run src tests with coverage | |
| (cd src && npx vitest run --coverage --reporter=verbose \ | |
| api/providers/__tests__/ \ | |
| core/assistant-message/__tests__/ \ | |
| core/task/__tests__/tool-call-policy.spec.ts) | |
| # Result: 60 test files, 1355 passed, 1 skipped | |
| # Run packages/types tests with coverage | |
| (cd packages/types && npx vitest run --coverage --reporter=verbose) | |
| # Result: All tests pass, 100% coverage on new files | |
| # Run packages/telemetry tests with coverage | |
| (cd packages/telemetry && npx vitest run --coverage --reporter=verbose) | |
| # Result: 3 test files, 46 passed | |
| # Analyze diff for added lines per source file | |
| python scripts/coverage-diff-analysis.py |
🤖 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/151400_debug-coverage-b12.md` around
lines 170 - 186, Update the test commands in the coverage workflow so each
command executes from the repository root context independently. Use subshells
or explicitly return to the root after the `src`, `packages/types`, and
`packages/telemetry` commands, ensuring `python
scripts/coverage-diff-analysis.py` resolves from the repository root.
Comment on lines
+252
to
+258
| } else if (params.tools !== undefined && isStrictToolSchemaRejected(error)) { | ||
| // Fallback: if the endpoint rejects the strict tool flag or a | ||
| // hardened strict-mode schema, retry once with the original | ||
| // schemas and no strict flag. Build a new params object so the | ||
| // rejected request is left untouched. | ||
| const paramsWithoutStrict = { ...params, tools: stripStrictFromTools(tools ?? []) } | ||
| stream = await this.client.chat.completions.create(paramsWithoutStrict) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strip the strict flag from params.tools, not from the raw metadata.tools.
Line 227 sends this.convertToolsForOpenAI(tools) in the original request. Line 257 rebuilds the retry payload from tools, which is the unconverted metadata.tools value. The retry then sends a payload that differs from the rejected one by more than the strict flag, whenever convertToolsForOpenAI performs any other normalization.
The branch guard already proves params.tools is defined, so tools ?? [] is also redundant. Use params.tools.
The existing test at lines 542-582 of src/api/providers/__tests__/mimo.spec.ts cannot detect this. Its fixture converts to a form that differs only by the added strict flag.
🐛 Proposed fix
- const paramsWithoutStrict = { ...params, tools: stripStrictFromTools(tools ?? []) }
+ const paramsWithoutStrict = { ...params, tools: stripStrictFromTools(params.tools) }📝 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
| } else if (params.tools !== undefined && isStrictToolSchemaRejected(error)) { | |
| // Fallback: if the endpoint rejects the strict tool flag or a | |
| // hardened strict-mode schema, retry once with the original | |
| // schemas and no strict flag. Build a new params object so the | |
| // rejected request is left untouched. | |
| const paramsWithoutStrict = { ...params, tools: stripStrictFromTools(tools ?? []) } | |
| stream = await this.client.chat.completions.create(paramsWithoutStrict) | |
| } else if (params.tools !== undefined && isStrictToolSchemaRejected(error)) { | |
| // Fallback: if the endpoint rejects the strict tool flag or a | |
| // hardened strict-mode schema, retry once with the original | |
| // schemas and no strict flag. Build a new params object so the | |
| // rejected request is left untouched. | |
| const paramsWithoutStrict = { ...params, tools: stripStrictFromTools(params.tools) } | |
| stream = await this.client.chat.completions.create(paramsWithoutStrict) |
🤖 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/mimo.ts` around lines 252 - 258, Update the retry
construction in the strict-schema rejection branch of the chat completion flow
to call stripStrictFromTools with params.tools rather than the unconverted tools
metadata. Preserve the already-converted request payload and remove the
redundant nullish fallback, while keeping the existing paramsWithoutStrict retry
behavior unchanged.
Comment on lines
+2964
to
+3011
| if (ghostDisposition && isProvablyEmptyGhost(ghostDisposition)) { | ||
| // Silently drop the ghost: remove its partial block | ||
| // from assistantMessageContent and discard streaming | ||
| // state. It will NOT receive a tool_result. | ||
| const ghostIndex = this.streamingToolCallIndices.get(event.id) | ||
| if (ghostIndex !== undefined) { | ||
| // Remove the partial tool_use block that was pushed | ||
| // at tool_call_start. This is safe because the call | ||
| // never resolved a name or arguments — it carries | ||
| // no model intent and has not been presented to the | ||
| // user as a tool call. | ||
| this.assistantMessageContent.splice(ghostIndex, 1) | ||
| // Re-index remaining streaming tool call indices | ||
| // since we removed an element from the array. | ||
| for (const [cid, idx] of this.streamingToolCallIndices.entries()) { | ||
| if (idx > ghostIndex) { | ||
| this.streamingToolCallIndices.set(cid, idx - 1) | ||
| } | ||
| } | ||
| this.streamingToolCallIndices.delete(event.id) | ||
| } | ||
| // Discard streaming state (finalizeStreamingToolCall | ||
| // would also delete it, but we bypass that path). | ||
| NativeToolCallParser.discardStreamingToolCall(event.id) | ||
| // Emit telemetry for the ghost drop. Only counts and | ||
| // metadata are sent — no call ID, tool name, or args. | ||
| const ghostPolicy1 = resolveToolCallPolicy( | ||
| this.api.getModel().info, | ||
| this.apiConfiguration.apiProvider, | ||
| ) | ||
| emitGhostDropTelemetry({ | ||
| taskId: this.taskId, | ||
| provider: this.apiConfiguration.apiProvider ?? "unknown", | ||
| model: this.api.getModel().id, | ||
| policySource: ghostPolicy1.source, | ||
| maxCallsPerTurn: ghostPolicy1.maxCallsPerTurn, | ||
| enforcement: ghostPolicy1.enforcement, | ||
| callCount: this.assistantMessageContent.filter( | ||
| (b: AssistantMessageContent): b is ToolUse => b.type === "tool_use", | ||
| ).length, | ||
| ghostDroppedCount: 1, | ||
| errorResultCount: 0, | ||
| parallelToolCallsRequested: ghostPolicy1.generation === "parallel", | ||
| }) | ||
| // Do NOT call presentAssistantMessageSafe — there is | ||
| // nothing to present for a ghost. | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how presentAssistantMessage advances currentStreamingContentIndex and reads assistantMessageContent.
fd -t f 'presentAssistantMessage*' src | while IFS= read -r f; do
echo "=== $f ==="
rg -n -C6 'currentStreamingContentIndex|assistantMessageContent' "$f"
done
# Also list every other writer of currentStreamingContentIndex.
rg -n -C3 'currentStreamingContentIndex\s*(=|\+\+|--|\+=|-=)' srcRepository: Zoo-Code-Org/Zoo-Code
Length of output: 20409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate Task.ts ==="
fd 'Task\.ts$' src
echo "=== Task.ts relevant stream/presenter code around ghost quarantine ==="
sed -n '2860,3025p' src/core/task/Task.ts | cat -n -v | sed 's/^/Line /'
echo "=== Task.ts relevant stream/presenter code around later quarantine pattern ==="
sed -n '3420,3515p' src/core/task/Task.ts | cat -n -v | sed 's/^/Line /'
echo "=== presentAssistantMessage relevant function header and increment path ==="
sed -n '1,120p' src/core/assistant-message/presentAssistantMessage.ts | cat -n -v | sed 's/^/Line /'
sed -n '910,970p' src/core/assistant-message/presentAssistantMessage.ts | cat -n -v | sed 's/^/Line /'
echo "=== presentAssistantMessage invocations and locks ==="
rg -n -C5 'presentAssistantMessage(Async|Safe)?|presentAssistantMessageLocked|presentAssistantMessageHasPendingUpdates' src/core src/shared src -g '*.ts' -g '*.tsx'
echo "=== behavioral probe: splice semantics for index-shift hazard ==="
node - <<'JS'
for (const currentStreamingContentIndex of [0, 1]) {
const assistantMessageContent = ["text", "ghost", "realTool"]
const ghostIndex = 1
assistantMessageContent.splice(ghostIndex, 1)
if (currentStreamingContentIndex > ghostIndex) {
console.log(`currentStreamingContentIndex=${currentStreamingContentIndex}, after splice cursor points to`, assistantMessageContent[currentStreamingContentIndex])
}
}
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50378
Decrement currentStreamingContentIndex when removing a ghost block.
presentAssistantMessage holds only presentAssistantMessageLocked, a per-invocation lock; it can already be past the ghost block when spill(..., 1) shifts assistantMessageContent. If currentStreamingContentIndex > ghostIndex, the next presenter invocation reads the shifted block and skips a real tool call; if currentStreamingContentIndex === ghostIndex, it advances past the ghost. The re-index walk fixes streamingToolCallIndices, but the presenter cursor must move by the same amount.
🤖 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/core/task/Task.ts` around lines 2964 - 3011, When removing the ghost
block in the ghost-disposition branch, also decrement
currentStreamingContentIndex when it is greater than or equal to ghostIndex so
the presenter cursor remains aligned after the splice. Keep the existing
streamingToolCallIndices re-indexing and cleanup unchanged.
Comment on lines
+3452
to
+3501
| // Ghost quarantine (same logic as the streaming tool_call_end | ||
| // handler above): inspect streaming state BEFORE | ||
| // finalizeStreamingToolCall() deletes it. | ||
| const preFinalizeState = NativeToolCallParser.getStreamingToolCallState(event.id) | ||
| const ghostDisposition = preFinalizeState | ||
| ? classifyStreamedCall({ | ||
| callId: event.id, | ||
| toolName: preFinalizeState.name, | ||
| argumentsAccumulator: preFinalizeState.argumentsAccumulator, | ||
| streamEnded: true, | ||
| }) | ||
| : undefined | ||
|
|
||
| if (ghostDisposition && isProvablyEmptyGhost(ghostDisposition)) { | ||
| // Silently drop the ghost: remove its partial block | ||
| // from assistantMessageContent and discard streaming | ||
| // state. It will NOT receive a tool_result. | ||
| const ghostIndex = this.streamingToolCallIndices.get(event.id) | ||
| if (ghostIndex !== undefined) { | ||
| this.assistantMessageContent.splice(ghostIndex, 1) | ||
| for (const [cid, idx] of this.streamingToolCallIndices.entries()) { | ||
| if (idx > ghostIndex) { | ||
| this.streamingToolCallIndices.set(cid, idx - 1) | ||
| } | ||
| } | ||
| this.streamingToolCallIndices.delete(event.id) | ||
| } | ||
| NativeToolCallParser.discardStreamingToolCall(event.id) | ||
| // Emit telemetry for the ghost drop. Only counts and | ||
| // metadata are sent — no call ID, tool name, or args. | ||
| const ghostPolicy3 = resolveToolCallPolicy( | ||
| this.api.getModel().info, | ||
| this.apiConfiguration.apiProvider, | ||
| ) | ||
| emitGhostDropTelemetry({ | ||
| taskId: this.taskId, | ||
| provider: this.apiConfiguration.apiProvider ?? "unknown", | ||
| model: this.api.getModel().id, | ||
| policySource: ghostPolicy3.source, | ||
| maxCallsPerTurn: ghostPolicy3.maxCallsPerTurn, | ||
| enforcement: ghostPolicy3.enforcement, | ||
| callCount: this.assistantMessageContent.filter( | ||
| (b: AssistantMessageContent): b is ToolUse => b.type === "tool_use", | ||
| ).length, | ||
| ghostDroppedCount: 1, | ||
| errorResultCount: 0, | ||
| parallelToolCallsRequested: ghostPolicy3.generation === "parallel", | ||
| }) | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the duplicated ghost-drop body into one private method.
Lines 3455-3501 duplicate Lines 2952-3010 exactly, apart from the local variable name. A third partial copy exists at Lines 3069-3100. Any correction, including the presenter-cursor fix raised on Lines 2964-3011, must then be applied three times.
Extract one private method and call it from all three sites.
♻️ Proposed helper
/**
* Classify a streamed call and, when it is a provably empty ghost, remove it
* from `assistantMessageContent`, repair the index maps, discard parser state,
* and emit metadata-only telemetry.
*
* `@returns` true when the call was dropped and the caller must skip finalization.
*/
private quarantineGhostToolCall(callId: string): boolean {
const preFinalizeState = NativeToolCallParser.getStreamingToolCallState(callId)
if (!preFinalizeState) {
return false
}
const disposition = classifyStreamedCall({
callId,
toolName: preFinalizeState.name,
argumentsAccumulator: preFinalizeState.argumentsAccumulator,
streamEnded: true,
})
if (!isProvablyEmptyGhost(disposition)) {
return false
}
const ghostIndex = this.streamingToolCallIndices.get(callId)
if (ghostIndex !== undefined) {
this.assistantMessageContent.splice(ghostIndex, 1)
for (const [cid, idx] of this.streamingToolCallIndices.entries()) {
if (idx > ghostIndex) {
this.streamingToolCallIndices.set(cid, idx - 1)
}
}
this.streamingToolCallIndices.delete(callId)
}
NativeToolCallParser.discardStreamingToolCall(callId)
const policy = resolveToolCallPolicy(this.api.getModel().info, this.apiConfiguration.apiProvider)
emitGhostDropTelemetry({
taskId: this.taskId,
provider: this.apiConfiguration.apiProvider ?? "unknown",
model: this.api.getModel().id,
policySource: policy.source,
maxCallsPerTurn: policy.maxCallsPerTurn,
enforcement: policy.enforcement,
callCount: this.assistantMessageContent.filter(
(b: AssistantMessageContent): b is ToolUse => b.type === "tool_use",
).length,
ghostDroppedCount: 1,
errorResultCount: 0,
parallelToolCallsRequested: policy.generation === "parallel",
})
return true
}Both tool_call_end sites then reduce to:
if (this.quarantineGhostToolCall(event.id)) {
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 `@src/core/task/Task.ts` around lines 3452 - 3501, Extract the duplicated
ghost-drop handling into one private quarantineGhostToolCall(callId: string):
boolean method. Move classification, assistantMessageContent/index-map cleanup,
parser-state disposal, and metadata-only telemetry into it, returning true only
when a provably empty ghost is dropped; then replace the duplicated logic at all
three tool_call_end sites with the helper and skip finalization when it returns
true.
Comment on lines
+1043
to
+1051
| "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.", | ||
| "gemini": { | ||
| "freeRequests": "* Free up to {{count}} requests per minute. After that, billing depends on prompt size.", | ||
| "pricingDetails": "For more info, see pricing details.", | ||
| "billingEstimate": "* Billing is an estimate - exact cost depends on prompt size." | ||
| } | ||
| }, | ||
| "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." |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the duplicate localization keys.
strictToolSchemas and strictToolSchemasDescription are declared twice in modelInfo: at Lines 1043-1044 and Lines 1050-1051. Biome reports noDuplicateObjectKeys, so the file fails lint. Keep one pair and remove the other.
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.",
"gemini": {📝 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.", | |
| "gemini": { | |
| "freeRequests": "* Free up to {{count}} requests per minute. After that, billing depends on prompt size.", | |
| "pricingDetails": "For more info, see pricing details.", | |
| "billingEstimate": "* Billing is an estimate - exact cost depends on prompt size." | |
| } | |
| }, | |
| "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." | |
| "gemini": { | |
| "freeRequests": "* Free up to {{count}} requests per minute. After that, billing depends on prompt size.", | |
| "pricingDetails": "For more info, see pricing details.", | |
| "billingEstimate": "* Billing is an estimate - exact cost depends on prompt size." | |
| }, | |
| "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." |
🧰 Tools
🪛 Biome (2.5.6)
[error] 1043-1043: The key strictToolSchemas was already declared.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1044-1044: The key strictToolSchemasDescription was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 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` around lines 1043 - 1051,
Remove the duplicate strictToolSchemas and strictToolSchemasDescription entries
from the modelInfo localization object, keeping exactly one identical pair so
the object has no duplicate keys and preserves the existing translations.
Source: Linters/SAST tools
…penAI Compatible provider - Add openAiToolStrictMode boolean to provider settings (profile-scoped, default false) - Add strict toggle checkbox in OpenAICompatible settings UI - BaseProvider.convertToolsForOpenAI now accepts strictMode parameter - strictMode=true: strict:true + hardened schema - strictMode=false: strict:false + best-effort original schema - MCP tools: always strict:false regardless of setting - Wire setting into all 4 openai.ts request paths - Fix reasoning effort unsafe cast, add xhigh and max values - Make parallel_tool_calls conditional on tools being present
# Conflicts: # src/core/tools/error-interception/StructuralValidator.ts
# Conflicts: # src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts # src/core/assistant-message/__tests__/presentAssistantMessage-parser-dedup.integration.spec.ts # src/core/assistant-message/presentAssistantMessage.ts
# Conflicts: # src/core/assistant-message/__tests__/presentAssistantMessage-parser-dedup.integration.spec.ts
…ception refs from backup
MimoHandler was passing raw tool schemas to the API without the strict mode conversion that all other OpenAI-compatible providers use. This caused tool call errors due to missing required/strict fields. - Call this.convertToolsForOpenAI(tools) instead of raw assignment - Adds strict: true, required properties, additionalProperties: false
added 7 commits
August 5, 2026 18:58
An id-less argument-continuation chunk belongs to the most recent id chunk seen at its index. When a provider reuses index 0 with a NEW id (a disguised second parallel call), the new call's id chunk was dropped but its id-less argument fragments were still kept and concatenated into the FIRST call's accumulator, corrupting its JSON. Track dropped indexes in filterToFirstToolCall state and drop subsequent id-less fragments for those indexes. Also rewrite the function docblock, which referenced a non-existent error-interception retry loop.
The parseErrors/parseFailures docblocks claimed presentAssistantMessage routes recorded failures to an INVALID_JSON_ARGUMENTS error-interception pattern. No such routing exists on this codebase; describe the actual lifecycle (consumed via the consume* APIs, cleared on new API request). Comment-only change, no behavior difference.
parseErrors/parseFailures static maps accumulated an entry per malformed tool call and were never cleared in production (the consume* APIs have no production callers), slowly leaking for the extension-host lifetime. Add NativeToolCallParser.clearParseFailures() and call it in Task.recursivelyMakeClineRequests alongside clearAllStreamingToolCalls()/ clearRawChunkState(), where other per-stream state is reset. The consume* APIs keep working for tests.
MiMo sends tools through convertToolsForOpenAI(), which attaches a strict flag to every function tool. An OpenAI-compatible endpoint that doesn't support structured outputs rejects the request with a 400 and the turn fails outright. Mirror the existing parallel_tool_calls fallback: detect schema-rejection errors narrowly (400 status plus a mention of strict/additionalProperties in a tools context, so unrelated 400s like MiMo's missing-reasoning_content rejection are not retried) and retry once with the original schemas and no strict flag.
myk1yt
force-pushed
the
pr/b17-provider-cost-v2
branch
from
August 5, 2026 10:09
6d8176a to
040481d
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/260730_0001_session_branch-cleanup/182225_code-report.md`:
- Around line 29-32: The verification and next-step sections use inconsistent
pre-existing failure counts. Update the next-step references to state four
pre-existing src-test failures, including the flaky timestamp case, or
explicitly explain why that case is excluded.
In
`@docs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.md`:
- Around line 5-18: Update the test-count statements in the coverage report to
use one consistent accounting: explicitly state the actual pre-change test
count, the number of newly added tests, and the resulting final count. Remove or
clarify the separate “already there” qualification so the arithmetic is
unambiguous.
- Around line 15-16: The maxTokens fallback must handle both omitted maxTokens
and missing model info without throwing. Update the relevant max-token
calculation to use optional access on info, add a regression test covering both
values being absent, and revise the documented coverage case to omit maxTokens
and describe missing options/model info.
In `@docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md`:
- Around line 34-43: Reconcile the Task.ts metrics in the coverage report so the
summary and uncovered-lines detail use the same coverage artifact and
denominator, or explicitly label them when they represent different
measurements. Update the affected summary and detail sections, including the
additional referenced ranges, and ensure the stated 89% conclusion is supported
and auditable.
In `@scripts/coverage-diff-analysis.py`:
- Around line 64-65: Rename the ambiguous loop variable l in the hunk
line-printing loop to line or another clear name, and update its use in the
print statement so the script passes Ruff E741 without suppression.
In `@webview-ui/src/i18n/locales/pl/settings.json`:
- Around line 972-973: Translate the `strictToolSchemas` and
`strictToolSchemasDescription` values in the Polish locale to natural Polish,
preserving the original meaning and the existing JSON keys.
🪄 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: 41a98f93-3b60-435c-ba4f-b628f1015f9d
📒 Files selected for processing (56)
docs/260730_0001_session_branch-cleanup/170000_debug-report.mddocs/260730_0001_session_branch-cleanup/173200_debug-report.mddocs/260730_0001_session_branch-cleanup/173230_execution-plan.mddocs/260730_0001_session_branch-cleanup/175300_code-report.mddocs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.mddocs/260730_0001_session_branch-cleanup/182225_code-report.mddocs/260730_0001_session_branch-cleanup/184700_debug-report.mddocs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.mddocs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.mdpackages/telemetry/src/TelemetryService.tspackages/types/src/__tests__/provider-settings.test.tspackages/types/src/model.tspackages/types/src/provider-settings.tspackages/types/src/providers/mimo.tspackages/types/src/telemetry.tsscripts/coverage-diff-analysis.pysrc/api/index.tssrc/api/providers/__tests__/base-provider.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/mistral.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/base-provider.tssrc/api/providers/mimo.tssrc/api/providers/openai.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/ToolCallRetentionPolicy.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy.spec.tssrc/core/prompts/tools/native-tools/execute_command.tssrc/core/task/Task.tssrc/core/task/__tests__/ghost-quarantine.spec.tssrc/core/task/__tests__/tool-call-policy.spec.tssrc/core/tools/ExecuteCommandTool.tssrc/eslint-suppressions.jsonsrc/shared/tools.tswebview-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 skipped from review as they are similar to previous changes (43)
- webview-ui/src/components/settings/providers/OpenAICompatible.tsx
- src/api/providers/base-openai-compatible-provider.ts
- src/api/providers/tests/openai.spec.ts
- webview-ui/src/i18n/locales/de/settings.json
- src/core/assistant-message/tests/ToolCallRetentionPolicy-telemetry.spec.ts
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/pt-BR/settings.json
- webview-ui/src/i18n/locales/nl/settings.json
- webview-ui/src/i18n/locales/hi/settings.json
- src/api/providers/openai.ts
- webview-ui/src/i18n/locales/vi/settings.json
- src/shared/tools.ts
- src/core/task/tests/tool-call-policy.spec.ts
- src/core/prompts/tools/native-tools/execute_command.ts
- packages/telemetry/src/TelemetryService.ts
- packages/types/src/provider-settings.ts
- packages/types/src/model.ts
- src/core/assistant-message/ToolCallRetentionPolicy.ts
- src/core/assistant-message/tests/ToolCallRetentionPolicy.spec.ts
- packages/types/src/telemetry.ts
- webview-ui/src/i18n/locales/ko/settings.json
- webview-ui/src/i18n/locales/ru/settings.json
- webview-ui/src/i18n/locales/it/settings.json
- src/core/assistant-message/tests/NativeToolCallParser.spec.ts
- webview-ui/src/i18n/locales/fr/settings.json
- webview-ui/src/i18n/locales/id/settings.json
- webview-ui/src/i18n/locales/es/settings.json
- webview-ui/src/i18n/locales/ca/settings.json
- packages/types/src/tests/provider-settings.test.ts
- webview-ui/src/i18n/locales/zh-CN/settings.json
- src/api/providers/base-provider.ts
- docs/260730_0001_session_branch-cleanup/184700_debug-report.md
- docs/260730_0001_session_branch-cleanup/173200_debug-report.md
- webview-ui/src/i18n/locales/tr/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- src/api/providers/mimo.ts
- src/api/providers/tests/base-provider.spec.ts
- docs/260730_0001_session_branch-cleanup/173230_execution-plan.md
- packages/types/src/providers/mimo.ts
- src/core/assistant-message/NativeToolCallParser.ts
- src/core/tools/ExecuteCommandTool.ts
- src/core/task/tests/ghost-quarantine.spec.ts
- src/core/task/Task.ts
Comment on lines
+29
to
+32
| - **src tests** (`task-persistence/__tests__/`): 4 failures, all pre-existing | ||
| - Confirmed by running same tests on base squash commit: 3 of 4 fail identically | ||
| - 4th failure is a flaky timestamp off-by-1ms test (`updatedAt: 1785435668487` vs `1785435668486`) | ||
| - None introduced by our cherry-picks |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the pre-existing failure count consistent.
The verification section lists four failures: three deterministic failures and one timestamp-flaky failure. The next-step section refers to three failures. State four pre-existing failures, or explain why the timestamp case is excluded.
Also applies to: 52-59
🤖 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/260730_0001_session_branch-cleanup/182225_code-report.md` around lines
29 - 32, The verification and next-step sections use inconsistent pre-existing
failure counts. Update the next-step references to state four pre-existing
src-test failures, including the flaky timestamp case, or explicitly explain why
that case is excluded.
Comment on lines
+5
to
+18
| Added 3 test cases to `src/api/providers/__tests__/mistral.spec.ts` covering the uncovered cost-calculation block (lines 158-174) in `src/api/providers/mistral.ts` to resolve the `codecov/patch` failure on PR #1132. | ||
|
|
||
| ## Actions Taken | ||
|
|
||
| 1. Read coverage report `docs/260805_0001_session_ci-all-green/150000_debug-coverage-b17.md` identifying 9 uncovered lines (159-172) in `mistral.ts`. | ||
| 2. Read `src/api/providers/mistral.ts` to understand the cost-calculation logic in `createMessage`. | ||
| 3. Read existing test `src/api/providers/__tests__/mistral.spec.ts` and reference test `src/api/providers/__tests__/openai-usage-tracking.spec.ts` for patterns. | ||
| 4. Read `src/shared/cost.ts` and `packages/types/src/providers/mistral.ts` to understand `calculateApiCostOpenAI` and model pricing. | ||
| 5. Added 3 test cases to the `createMessage` describe block: | ||
| - **"should yield usage event with totalCost when stream contains usage data"**: Mocks a Mistral SSE stream with `usage: { promptTokens: 100, completionTokens: 50 }`, asserts a `usage` event with correct `totalCost` (computed via `calculateApiCostOpenAI` with `codestral-latest` pricing: inputPrice 0.3, outputPrice 0.9). | ||
| - **"should yield totalCost: 0 when modelInfo is not available"**: Spies on `getModel` to return `info: undefined`, asserts `totalCost: 0` fallback (line 166). Provides `maxTokens: 8192` to prevent crash at line 94 (`maxTokens ?? info.maxTokens`). | ||
| - **"should not yield usage event when stream has no usage data"**: Mocks a stream without `usage` field, asserts no `usage` event is yielded. | ||
| 6. Added imports for `ApiStreamUsageChunk`, `calculateApiCostOpenAI`, and `mistralModels`. | ||
| 7. Ran tests: all 20 tests pass (18 existing + 3 new, 1 was already there). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the test count consistent.
The report says three tests were added, then reports 20 total as 18 existing plus three new, with one already present. These statements do not define one unambiguous count. State the actual pre-change count, added count, and final count consistently.
Also applies to: 21-23
🧰 Tools
🪛 LanguageTool
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sage-tracking.spec.tsfor patterns. 4. Readsrc/shared/cost.tsandpackages/type...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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/150700_code-b17-mistral-coverage-report.md`
around lines 5 - 18, Update the test-count statements in the coverage report to
use one consistent accounting: explicitly state the actual pre-change test
count, the number of newly added tests, and the resulting final count. Remove or
clarify the separate “already there” qualification so the arithmetic is
unambiguous.
Comment on lines
+15
to
+16
| - **"should yield totalCost: 0 when modelInfo is not available"**: Spies on `getModel` to return `info: undefined`, asserts `totalCost: 0` fallback (line 166). Provides `maxTokens: 8192` to prevent crash at line 94 (`maxTokens ?? info.maxTokens`). | ||
| - **"should not yield usage event when stream has no usage data"**: Mocks a stream without `usage` field, asserts no `usage` event is yielded. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'mistral.ts|.*coverage-report.md' . | sed 's#^\./##' | head -200
echo
echo "Target lines:"
sed -n '1,80p' docs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.md 2>/dev/null || true
echo
echo "Find mistral.ts:"
fd -a 'mistral.ts' . | while read -r f; do
echo "--- $f"
rg -n 'maxTokens|getModel|totalCost|info\.maxTokens' "$f" || true
doneRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/api/providers/mistral.ts"
echo "--- outline $file"
ast-grep outline "$file" --view compact || true
echo
echo "--- lines 60-110: cost calculation"
sed -n '60,110p' "$file" | nl -ba -v60
echo
echo "--- lines 160-195: getModel / call path"
sed -n '160,195p' "$file" | nl -ba -v160
echo
echo "--- test coverage around new tests"
sed -n '1,120p' src/api/providers/__tests__/mistral.spec.ts 2>/dev/null | nl -ba -v1 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe: evaluate the exact fallback semantics in the same order as the source,
# then show whether omitting both maxTokens and info fails while omitting only maxTokens passes.
node - <<'JS'
for (const [maxTokens, info] of [
[undefined, undefined],
[undefined, { maxTokens: 8192 }],
[8192, undefined],
]) {
try {
const result = { maxTokens: maxTokens ?? info.maxTokens }
console.log(JSON.stringify({ maxTokens, hasInfo: typeof info === 'object' ? String(info.maxTokens) : 'undefined', result }))
} catch (err) {
console.log(JSON.stringify({ maxTokens, hasInfo: typeof info === 'object' ? String(info.maxTokens) : 'undefined', error: err.message, name: err.name }))
}
}
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/api/providers/__tests__/mistral.spec.ts | awk '{printf "%d\t%s\n", NR, $0}'
echo "---"
sed -n '120,220p' src/api/providers/__tests__/mistral.spec.ts | awk '{printf "%d\t%s\n", NR + 119, $0}'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 7064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- file line count"
wc -l docs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.md src/api/providers/__tests__/mistral.spec.ts src/api/providers/mistral.ts
echo
echo "--- changed files:"
git diff --name-status HEAD~1..HEAD 2>/dev/null || git status -s
echo
echo "--- mistral.spec.ts end:"
sed -n '220,320p' src/api/providers/__tests__/mistral.spec.ts | awk '{printf "%d\t%s\n", NR + 219, $0}'
echo
echo "--- mistral.ts relevant snippets:"
python3 - <<'PY'
from pathlib import Path
p=Path("src/api/providers/mistral.ts")
text=p.read_text()
for start,end in [(1,130),(130,220)]:
print(f"--- {p} lines {start}-{end}")
for i,line in enumerate(text.splitlines(),1):
if start <= i <= end and ("getModel\(","maxTokens ?? info.maxTokens","includeMaxTokens" in line or i in range(80,100) or i in range(180,200)):
print(f"{i}\t{line}")
elif start <= i <= end:
print(f"{i}\t{line}")
print()
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 11752
Guard info before reading maxTokens.
With includeMaxTokens: false, the returned value has no maxTokens, but the report case also sets info: undefined, which makes maxTokens ?? info.maxTokens throw Cannot read properties of undefined. Replace this with an info?.maxTokens fallback, add a regression test where both maxTokens and info are missing, and update this report to describe the regression as omitted options/model info rather than providing maxTokens.
🤖 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/150700_code-b17-mistral-coverage-report.md`
around lines 15 - 16, The maxTokens fallback must handle both omitted maxTokens
and missing model info without throwing. Update the relevant max-token
calculation to use optional access on info, add a regression test covering both
values being absent, and revise the documented coverage case to omit maxTokens
and describe missing options/model info.
Comment on lines
+34
to
+43
| | `src/core/task/Task.ts` | 191 | ~60 | ~131 | ~31% | | ||
| | `src/core/tools/ExecuteCommandTool.ts` | 1 | 0 | 1 | 0% | | ||
| | `src/shared/tools.ts` | 1 | 1 | 0 | 100% | | ||
| | **TOTAL** | **~1281** | **~1140** | **~141** | **~89%** | | ||
|
|
||
| ### Uncovered Lines Detail | ||
|
|
||
| #### 1. `src/core/task/Task.ts` — ~131 uncovered new lines (CRITICAL) | ||
|
|
||
| **Overall file coverage**: 0% (Task.ts has no dedicated test file; coverage comes only from integration via other test files, which don't exercise the new code paths). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reconcile the Task.ts coverage metrics before using the 89% conclusion.
The summary table reports about 60 of 191 new lines covered. The detail reports 0% file coverage and approximately 0% direct coverage for the same file. If these use different denominators, label them clearly. Otherwise, recompute from one coverage artifact. The current conclusion is not auditable.
Also applies to: 143-149, 199-200, 217-221
🤖 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/151400_debug-coverage-b12.md` around
lines 34 - 43, Reconcile the Task.ts metrics in the coverage report so the
summary and uncovered-lines detail use the same coverage artifact and
denominator, or explicitly label them when they represent different
measurements. Update the affected summary and detail sections, including the
additional referenced ranges, and ensure the stated 89% conclusion is supported
and auditable.
Comment on lines
+64
to
+65
| for i, l in enumerate(hunk["lines"]): | ||
| print(f" {start + i}: {l.rstrip()}") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the ambiguous loop variable.
Ruff reports E741 for l at Line 64. Use line or added_line so the script passes lint without a suppression.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 64-64: Ambiguous variable name: l
(E741)
🤖 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-diff-analysis.py` around lines 64 - 65, Rename the ambiguous
loop variable l in the hunk line-printing loop to line or another clear name,
and update its use in the print statement so the script passes Ruff E741 without
suppression.
Source: Linters/SAST tools
Comment on lines
+972
to
+973
| "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." |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new Polish locale entries.
Lines 972-973 are English. Polish users will see untranslated settings text. Add Polish translations for both values.
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."
+ "strictToolSchemas": "Ścisłe schematy narzędzi",
+ "strictToolSchemasDescription": "Aktywuje tryb ścisły dla schematów narzędzi funkcyjnych, zapewniając, że dane wyjściowe narzędzi dokładnie odpowiadają schematowi. Niektórzy dostawcy mogą nie obsługiwać trybu ścisłego. Narzędzia MCP zawsze pozostają nieścisłe, niezależnie od tego ustawienia."📝 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." | |
| "strictToolSchemas": "Ścisłe schematy narzędzi", | |
| "strictToolSchemasDescription": "Aktywuje tryb ścisły dla schematów narzędzi funkcyjnych, zapewniając, że dane wyjściowe narzędzi dokładnie odpowiadają schematowi. Niektórzy dostawcy mogą nie obsługiwać trybu ścisłego. Narzędzia MCP zawsze pozostają nieścisłe, niezależnie od tego ustawienia." |
🤖 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/pl/settings.json` around lines 972 - 973,
Translate the `strictToolSchemas` and `strictToolSchemasDescription` values in
the Polish locale to natural Polish, preserving the original meaning and the
existing JSON keys.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/api/providers/__tests__/mimo.spec.ts (2)
1027-1101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated parallel-call suppression test.
This test uses the same stream fixture and asserts the same behavior as
drops the second parallel tool call and keeps the firstat lines 1104-1168. The later test is stricter, because it also asserts the exact chunk count and the retained call id. Delete this test and keep the one inside theparallel tool call suppressionblock. If thetoolsmetadata path must stay covered, move thetoolsargument into the retained test instead.🤖 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__/mimo.spec.ts` around lines 1027 - 1101, Delete the duplicated test “should suppress parallel tool calls, keeping only the first” and retain the stricter test in the “parallel tool call suppression” block. If needed to preserve coverage for tool metadata, add the tools argument to the retained test rather than keeping a second fixture.
678-695: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the
toolsliteral and drop the double assertion.Line 562 passes
{ taskId: "test-task", tools }directly withtoolstyped asOpenAI.Chat.ChatCompletionTool[], so the metadata object type-checks without a cast. Apply the same shape here. This removes an unexplainedas unknown asdouble assertion. The coding guidelines require a comment for any double assertion that is truly unavoidable; this one is avoidable.♻️ Proposed change
- const tools = [ + const tools: OpenAI.Chat.ChatCompletionTool[] = [ { - type: "function" as const, + type: "function", function: { name: "read_file", description: "Read a file", parameters: { type: "object", properties: { path: { type: "string" } }, required: ["path"], }, }, }, ] - const stream = handler.createMessage("System prompt", messages, { - tools, - } as unknown as ApiHandlerCreateMessageMetadata) + const stream = handler.createMessage("System prompt", messages, { taskId: "test-task", tools })The same guideline applies to the remaining unexplained double assertions at lines 130, 132, 215, 228, 349, and 357. Add a short comment next to each one that must stay.
As per coding guidelines: "Use double assertions only as a last resort and explain them with a comment."
🤖 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__/mimo.spec.ts` around lines 678 - 695, Type the local tools literal as OpenAI.Chat.ChatCompletionTool[] and pass it directly in the createMessage metadata, removing the avoidable as unknown as ApiHandlerCreateMessageMetadata assertion. Also inspect the remaining double assertions at the referenced test locations and add a brief justification comment beside each assertion that cannot be removed.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.
Nitpick comments:
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 1027-1101: Delete the duplicated test “should suppress parallel
tool calls, keeping only the first” and retain the stricter test in the
“parallel tool call suppression” block. If needed to preserve coverage for tool
metadata, add the tools argument to the retained test rather than keeping a
second fixture.
- Around line 678-695: Type the local tools literal as
OpenAI.Chat.ChatCompletionTool[] and pass it directly in the createMessage
metadata, removing the avoidable as unknown as ApiHandlerCreateMessageMetadata
assertion. Also inspect the remaining double assertions at the referenced test
locations and add a brief justification comment beside each assertion that
cannot be removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 198512a3-3df4-44af-b9a3-224b3bef6a36
📒 Files selected for processing (3)
src/api/providers/__tests__/mimo.spec.tssrc/core/task/__tests__/ghost-quarantine.spec.tssrc/eslint-suppressions.json
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/task/tests/ghost-quarantine.spec.ts
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
New Features
Bug Fixes