Add model-eval Tools, theme customizer, and Labs UI polish - #9
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
…e quotes, and chat/eval UI polish. Co-authored-by: Cursor <cursoragent@cursor.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| let out = String(text); | ||
| // Drop full thought channels if the runtime leaked them into content. | ||
| out = out.replace(/<\|channel>thought[\s\S]*?<channel\|>/gi, ''); | ||
| out = out.replace(/<\|think\|>[\s\S]*?(?:<\|\/think\|>|$)/gi, ''); |
There was a problem hiding this comment.
CRITICAL: $ anchor in think-tag regex destroys all trailing streaming content
The regex /<\|think\|>[\s\S]*?(?:<\|\/think\|>|$)/gi uses $ as an alternation fallback for unclosed <|think|> tags. During streaming, when a partial chunk contains an opening <|think|> without a matching </|think|> closing tag, the lazy [\s\S]*? combined with $ consumes and deletes ALL text from the opening tag through the end of the string. This silently destroys entire model responses.
Fix by removing the $ fallback and instead stripping orphan opening tags on a separate pass:
| out = out.replace(/<\|think\|>[\s\S]*?(?:<\|\/think\|>|$)/gi, ''); | |
| out = out.replace(/<\|think\|>[\s\S]*?<\|\/think\|>/gi, ''); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Drop full thought channels if the runtime leaked them into content. | ||
| out = out.replace(/<\|channel>thought[\s\S]*?<channel\|>/gi, ''); | ||
| out = out.replace(/<\|think\|>[\s\S]*?(?:<\|\/think\|>|$)/gi, ''); | ||
| out = out.replace(/<think>[\s\S]*?(?:<\/think>|$)/gi, ''); |
There was a problem hiding this comment.
CRITICAL: $ anchor in HTML think-tag regex destroys all trailing streaming content
Same bug as line 324 but for the HTML-style <think> tags. An unclosed <think> tag in a streaming chunk will cause [\s\S]*? with the $ fallback to consume and delete everything to end-of-string.
| out = out.replace(/<think>[\s\S]*?(?:<\/think>|$)/gi, ''); | |
| out = out.replace(/<think>[\s\S]*?<\/think>/gi, ''); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| let salt = safeGet(storage, keys.salt); | ||
| if (!salt) { | ||
| salt = 'test-salt'; |
There was a problem hiding this comment.
CRITICAL: Hardcoded 'test-salt' instead of random salt in ensureHomeQuoteBag
ensureHomeQuoteBag sets salt = 'test-salt' when no salt is stored — a clear leftover debug artifact. This is the peek path called before pickNextHomeQuote, making every browser session that hits this path first get the same deterministic shuffle bag for the entire day, breaking the session-salt randomness guarantee.
| salt = 'test-salt'; | |
| salt = randomSalt(); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| model: modelSource, | ||
| mainExecutorSettings: { maxNumTokens: 4096 }, | ||
| mainExecutorSettings: { | ||
| maxNumTokens: option?.maxNumTokens || 4096, |
There was a problem hiding this comment.
WARNING: || 4096 treats 0 as falsy — use ?? for maxNumTokens fallback
option?.maxNumTokens || 4096 would incorrectly fall back to 4096 if a future model entry explicitly sets maxNumTokens: 0. While no current model has this, using nullish coalescing is more correct:
| maxNumTokens: option?.maxNumTokens || 4096, | |
| maxNumTokens: option?.maxNumTokens ?? 4096, |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * @param {string} reply | ||
| */ | ||
| export function scoreCase(testCase, reply) { | ||
| switch (testCase.scorer) { |
There was a problem hiding this comment.
WARNING: scoreCase has no null/undefined guard on testCase parameter
switch (testCase.scorer) throws a TypeError if testCase is null or undefined. As a public export called by external harness code, it should validate its input:
| switch (testCase.scorer) { | |
| if (!testCase) return { pass: false, reasons: ['missing test case'] }; | |
| switch (testCase.scorer) { |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| let used = 0; | ||
|
|
||
| for (const model of sorted) { | ||
| const size = model.approxBytes || GiB; |
There was a problem hiding this comment.
WARNING: || GiB treats approxBytes: 0 as falsy, defaulting to 1 GiB
model.approxBytes || GiB uses logical OR, so approxBytes: 0 (a valid size for a tiny model) evaluates to 0 || GiB = 1,073,741,824 bytes. Use nullish coalescing:
| const size = model.approxBytes || GiB; | |
| const size = model.approxBytes ?? GiB; |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| args: [ | ||
| '--enable-unsafe-webgpu', | ||
| '--ignore-gpu-blocklist', | ||
| '--disable-web-security', |
There was a problem hiding this comment.
WARNING: --disable-web-security disables all CORS and origin protections
This Chromium flag removes all cross-origin security in the headed browser instance used for eval. While mitigated by an isolated profile at .models/.model-eval-profile, this flag should be carefully documented with a warning comment and only enabled when strictly necessary (vs. the existing --disable-web-security requirement may be avoidable with proper CORS headers from Vite).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| credentials: 'same-origin', | ||
| }); | ||
| if (!res.ok) throw new Error(`HTTP ${res.status}`); | ||
| report.value = await res.json(); |
There was a problem hiding this comment.
WARNING: No JSON shape validation after fetch — non-array models will crash rendering
report.value = await res.json() assigns parsed JSON directly without validating that report.models is an array. If the server returns malformed JSON where models is truthy but non-array (e.g. a string or object), downstream computed properties calling .map()/.filter() on report.models will throw a runtime error, silently crashing the component with a blank page.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (typeof chat.reset === 'function') { | ||
| await chat.reset(); | ||
| } else { | ||
| chat.messages = []; |
There was a problem hiding this comment.
WARNING: Direct mutation of chat.messages = [] bypasses encapsulation
Assigning directly to an internal AiChat property skips intended side effects (state tracking, event emission, resource management). If AiChat changes its internal representation or adds getter/setter logic, this assignment will leave the instance in an inconsistent state. Prefer using chat.reset() (which already exists in the codebase) or add a dedicated public method for clearing messages.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Merge (minor suggestion only) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit a775f09)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a775f09)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (41 files)
Reviewed by deepseek-v4-pro · Input: 41.4K · Output: 15K · Cached: 331.1K |
…hromium flag. Unclosed think blocks no longer wipe trailing stream text via `$`-to-EOF; ensureHomeQuoteBag uses randomSalt(); note --disable-web-security is local-eval only. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed Kilo review P0/P1 from triage: Fixed
Deferred / wontfix (per triage): W1–W3/W5/W6 and remaining suggestions (null guards, |
| out = out.replace(/<\|think\|>[\s\S]*?<\|\/think\|>/gi, ''); | ||
| out = out.replace(/<think>[\s\S]*?<\/think>/gi, ''); | ||
| // Orphan open/close markers left mid-stream (no closed pair yet). | ||
| out = out.replace(/<\|think\|>/g, ''); |
There was a problem hiding this comment.
SUGGESTION: Missing i flag on <|think|> orphan regex — inconsistent with other case-insensitive think matches
All other think-related regexes on lines 326–327 and 330 use gi (case-insensitive). Line 329 uses only /g. If a model outputs <|THINK|> as an orphan token, the closed-block regex at line 326 would handle pairs, but this standalone strip would miss the case-variant, leaving a bare <|THINK|> in the output.
| out = out.replace(/<\|think\|>/g, ''); | |
| out = out.replace(/<\|think\|>/gi, ''); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
vdl-*localStorage remap/defaults so Labs prefs stay namespaced and restore cleanly.>=20.9.0for sharp@0.35.Test plan
pnpm test(Playwright unit suite) passes locallyvdl-*keys persist; reset uses Labs defaultsMade with Cursor