Merged
Conversation
- 新建 workspace 包 packages/@anthropic-ai/model-provider - 定义 ModelProviderHooks 接口（依赖注入：分析、成本、日志等） - 定义 ClientFactories 接口（Anthropic/OpenAI/Gemini/Grok 客户端工厂） - 搬入核心类型：Message 体系、NonNullableUsage、EMPTY_USAGE、SystemPrompt、错误常量 - 主项目 src/types/message.ts 等改为 re-export，保持向后兼容 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 搬入 OpenAI 消息转换（convertMessages）、工具转换（convertTools）、流适配（streamAdapter） - 搬入 OpenAI 和 Grok 模型映射（resolveOpenAIModel、resolveGrokModel） - 主项目文件改为 thin re-export proxy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 搬入 Gemini 类型定义、消息转换、工具转换、流适配、模型映射 - 主项目 gemini/ 目录下文件改为 thin re-export proxy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 搬入 formatAPIError、extractConnectionErrorDetails 等 errorUtils - 迁移 10 个消费者文件直接从 @anthropic-ai/model-provider 导入 - 更新 emptyUsage、sdkUtilityTypes、systemPromptType 为 re-export proxy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit e458d63.
* fix: 修复 Bun 的 polyfill 问题 * fix: 类型修复完成 * feat: 统一所有包的类型文件 * fix: 修复构建问题
Co-authored-by: chengzifeng <chengzifeng@meituan.com>
将 Anthropic 格式的工具定义转换为 Langfuse 兼容的 OpenAI 格式，
并在 generation 的 input 中以 { messages, tools } 结构传入，
以便在 Langfuse UI 中查看完整的工具定义信息。
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: 适配 zed acp 协议 * docs: 完善 acp 文档
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
📝 WalkthroughWalkthroughLarge-scale monorepo refactoring that extracts core model provider functionality into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/langfuse/tracing.ts (1)
80-96:⚠️ Potential issue | 🟡 MinorFix empty array truthiness check for tools parameter.
In JavaScript, an empty array
[]is truthy, so the conditionparams.tools ? ... : ...will incorrectly wrap the input as{ messages, tools: [] }whenconvertToolsToLangfusereturns an empty array. This changes the Langfuse recorded input shape inconsistently compared to requests without tools.Suggested fix
export function recordLLMObservation( rootSpan: LangfuseSpan | null, params: { @@ ): void { if (!rootSpan || !isLangfuseEnabled()) return try { const genName = PROVIDER_GENERATION_NAMES[params.provider] ?? `Chat${params.provider}` + const hasTools = Array.isArray(params.tools) ? params.tools.length > 0 : Boolean(params.tools) // Use the global startObservation directly instead of rootSpan.startObservation(). @@ { model: params.model, - input: params.tools + input: hasTools ? { messages: params.input, tools: params.tools } : params.input,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/langfuse/tracing.ts` around lines 80 - 96, The conditional that builds the Langfuse generation input uses params.tools truthiness, but an empty array is truthy and will incorrectly wrap input as { messages, tools: [] }; update the check where the input is constructed (inside the startObservation call in this tracing module using genName and params) to only include tools when they are a non-empty array (e.g., Array.isArray(params.tools) && params.tools.length > 0) or otherwise use params.input directly so empty tool arrays do not change the recorded input shape.
🟠 Major comments (19)
src/utils/__tests__/earlyInput.test.ts-72-91 (1)
72-91:⚠️ Potential issue | 🟠 MajorRegression tests are non-functional (asserting constants, not behavior).
Both cases pass even if filtering breaks, because they only compare local string literals and never invoke the code path under test.
Safer interim change (avoid false confidence)
- test('DA1 response sequence pattern is documented (CSI ? ... c)', () => { - // \x1b[?64;1;2;4;6;17;18;21;22c — previously leaked as "?64;1;2;4;6;17;18;21;22c" - // After fix: CSI sequences are fully consumed, nothing leaks - // We document the expected clean output here - const leakedBefore = '?64;1;2;4;6;17;18;21;22c' - const cleanAfter = '' - // The fix ensures processChunk produces cleanAfter, not leakedBefore - // (verified manually; this test documents the contract) - expect(leakedBefore).not.toBe(cleanAfter) // sanity: they differ - expect(cleanAfter).toBe('') // after fix: nothing leaks - }) + test.todo('DA1 response sequence is fully filtered in stdin → processChunk path') - test('XTVERSION DCS sequence pattern is documented (ESC P ... ESC \\)', () => { - // \x1bP>|iTerm2 3.6.4\x1b\\ — previously leaked as ">|iTerm2 3.6.4" - // After fix: DCS sequences are fully consumed via ST terminator - const leakedBefore = '>|iTerm2 3.6.4' - const cleanAfter = '' - expect(leakedBefore).not.toBe(cleanAfter) - expect(cleanAfter).toBe('') - }) + test.todo('XTVERSION DCS sequence is fully filtered in stdin → processChunk path')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/earlyInput.test.ts` around lines 72 - 91, The tests currently assert only local constants instead of exercising the real filter logic; update both tests to call the actual function under test (e.g., processChunk) with the raw escape sequences and assert its return/side-effect produces the empty string. Specifically, in the "DA1 response sequence pattern..." test call processChunk with "\x1b[?64;1;2;4;6;17;18;21;22c" and expect the result to be "" (and/or verify any buffer state is cleared), and in the "XTVERSION DCS sequence pattern..." test call processChunk (or the DCS-handling entrypoint) with "\x1bP>|iTerm2 3.6.4\x1b\\" and expect "" as the filtered output; ensure you import/require processChunk (or the correct exported function) and reset any parser state between assertions so tests are deterministic.src/commands/poor/__tests__/poorMode.test.ts-28-53 (1)
28-53:⚠️ Potential issue | 🟠 MajorThe “first call reads settings” path is not actually tested.
Line 45 and Line 51 call
setPoorMode(...)beforeisPoorModeActive(), so these cases validate setter effects, not initial-read behavior. Also,freshModule()(Line 29) is empty, so singleton reset is currently unimplemented.Suggested test adjustment
-/** Reset module-level singleton between tests by re-importing a fresh copy. */ -async function freshModule() { - // Bun caches modules; we manipulate the exported functions directly since - // the singleton `poorModeActive` is reset to null only on first import. - // Instead we test the observable behaviour through set/get pairs. -} +// Prefer an explicit test reset hook exported from poorMode.ts, e.g.: +// export function __resetPoorModeForTest() { poorModeActive = null } describe('isPoorModeActive — reads from settings on first call', () => { beforeEach(() => { lastUpdate = null + mockSettings = {} + // __resetPoorModeForTest() }) test('returns false when settings has no poorMode key', () => { mockSettings = {} - // Force re-read by setting internal state via setPoorMode then checking - setPoorMode(false) expect(isPoorModeActive()).toBe(false) }) test('returns true when settings.poorMode === true', () => { mockSettings = { poorMode: true } - setPoorMode(true) expect(isPoorModeActive()).toBe(true) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/poor/__tests__/poorMode.test.ts` around lines 28 - 53, freshModule() is currently a no-op so tests never exercise the "first-call reads settings" path; implement freshModule to reset the module-level singleton (clear module cache or re-require the module) and reset internal state (e.g., set lastUpdate = null and poorModeActive = null if present) so a fresh import will read mockSettings on first isPoorModeActive() call, then update the tests to call freshModule() before invoking isPoorModeActive() (remove the prior setPoorMode(...) calls used to force state) so the tests validate initial-read behavior; refer to freshModule, isPoorModeActive, setPoorMode, mockSettings and lastUpdate/poolModeActive in the test file when making changes.packages/remote-control-server/src/transport/sse-writer.ts-133-146 (1)
133-146:⚠️ Potential issue | 🟠 MajorPreserve string-form user messages when normalizing worker payloads.
If a user event comes through as
{ message: "..." }, this branch rewrites it to{ message: { content: "" } }because the stringmessagecase is never used as a fallback. That drops real user text from the SSE stream.🔧 Proposed fix
if (event.type === "user") { const message = payload.message; if (!message || typeof message !== "object" || !("content" in message)) { const content = typeof normalized?.content === "string" ? normalized.content + : typeof normalized?.message === "string" + ? normalized.message : typeof payload.content === "string" ? payload.content : typeof event.payload === "string" ? event.payload : ""; payload.content = content; payload.message = { content }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/transport/sse-writer.ts` around lines 133 - 146, In the SSE normalization branch handling event.type === "user" (in sse-writer.ts), the current fallback logic ignores when payload.message is a string and replaces it with an empty content object; update the fallback so a string message is preserved: if payload.message is a string use that string as content before checking normalized.content, payload.content, or event.payload, and then set payload.content and payload.message = { content } only after deriving the correct string content; modify references in this block (payload, message, normalized, event.payload) accordingly so real user text is not dropped.packages/remote-control-server/web/render.js-163-173 (1)
163-173:⚠️ Potential issue | 🟠 MajorDon't drop user text when the same event also contains embedded
tool_resultblocks.Both branches bail out as soon as
renderEmbeddedToolResultBlocks(payload)returns anything, so mixed-content user messages render only the tool result cards and lose their text. In the live path that also skips the loading state for a real user prompt.Suggested fix
case "user": { const toolResultEls = renderEmbeddedToolResultBlocks(payload); - if (toolResultEls.length > 0) { - els.push(...toolResultEls); - break; - } - if (!shouldRenderUserEvent(payload, direction, false)) return; - els.push(renderUserMessage(payload, direction)); - needLoading = true; + const text = extractText(payload).trim(); + if (text) { + if (!shouldRenderUserEvent(payload, direction, false)) return; + els.push(renderUserMessage(payload, direction)); + needLoading = true; + } + if (toolResultEls.length > 0) els.push(...toolResultEls); } break;Apply the same pattern in the replay branch so text and tool results can both render.
Also applies to: 234-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/web/render.js` around lines 163 - 173, The current case "user" branch drops user text whenever renderEmbeddedToolResultBlocks(payload) returns non-empty; update the logic so that both embedded tool result elements and the user message can be pushed to histEls: call renderEmbeddedToolResultBlocks(payload) and always push its results (if any) into histEls, then independently check shouldRenderUserEvent(payload, direction, true) and push renderUserMessage(payload, direction) if true; apply the same change to the replay branch (the similar code around the other user-case at the second location) so mixed-content messages render both tool_result blocks and the user text.packages/remote-control-server/src/services/session.ts-17-17 (1)
17-17:⚠️ Potential issue | 🟠 MajorClean up the event bus for
inactivesessions too.
inactiveis now classified as a closed status, but onlyarchiveSession()removes the event bus. If a session transitions to"inactive"throughupdateSessionStatus(), the bus stays registered and existing stream subscribers can remain attached to a session that the rest of this service already treats as closed.Suggested fix
export function updateSessionStatus(sessionId: string, status: string) { storeUpdateSession(sessionId, { status }); const bus = getAllEventBuses().get(sessionId); - if (!bus) return; - - bus.publish({ - id: uuid(), - sessionId, - type: "session_status", - payload: { status }, - direction: "inbound", - }); + if (bus) { + bus.publish({ + id: uuid(), + sessionId, + type: "session_status", + payload: { status }, + direction: "inbound", + }); + } + if (isSessionClosedStatus(status)) { + removeEventBus(sessionId); + } } export function archiveSession(sessionId: string) { updateSessionStatus(sessionId, "archived"); - removeEventBus(sessionId); }Also applies to: 78-80, 130-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/services/session.ts` at line 17, The event bus is only removed in archiveSession() so sessions marked "inactive" (now in CLOSED_SESSION_STATUSES) keep their event bus; factor the event-bus cleanup used in archiveSession() into a reusable helper (e.g., cleanupEventBusForSession or removeEventBus) and call it from updateSessionStatus() whenever the new status is a member of CLOSED_SESSION_STATUSES (and keep archiveSession() calling it too), so any transition to a closed status (including "inactive") unregisters the event bus and detaches stream subscribers.build.ts-93-110 (1)
93-110:⚠️ Potential issue | 🟠 MajorReset the regex state before scanning each file.
BUN_DESTRUCTUREis global, so.test(content)advanceslastIndex. After the first match, later files can be skipped and left unpatched, which makes the Node-compat pass flaky.Suggested fix
const BUN_DESTRUCTURE = /var \{([^}]+)\} = globalThis\.Bun;?/g const BUN_DESTRUCTURE_SAFE = 'var {$1} = typeof globalThis.Bun !== "undefined" ? globalThis.Bun : {};' for (const file of files) { if (!file.endsWith('.js')) continue const filePath = join(outdir, file) const content = await readFile(filePath, 'utf-8') - if (BUN_DESTRUCTURE.test(content)) { + BUN_DESTRUCTURE.lastIndex = 0 + const patchedContent = content.replace(BUN_DESTRUCTURE, BUN_DESTRUCTURE_SAFE) + if (patchedContent !== content) { await writeFile( filePath, - content.replace(BUN_DESTRUCTURE, BUN_DESTRUCTURE_SAFE), + patchedContent, ) bunPatched++ } } -BUN_DESTRUCTURE.lastIndex = 0Based on learnings, build output must be compatible with both Bun and Node.js.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.ts` around lines 93 - 110, The global RegExp BUN_DESTRUCTURE retains state between .test calls causing some files to be skipped; fix by resetting or re-instantiating the regex before testing each file in the files loop (e.g., recreate BUN_DESTRUCTURE or set BUN_DESTRUCTURE.lastIndex = 0 at the top of each iteration) so that the test against content always starts fresh before calling writeFile and incrementing bunPatched; ensure this change references BUN_DESTRUCTURE, the for (const file of files) loop, readFile, writeFile, and bunPatched so all JS outputs are reliably patched for Node compatibility.scripts/vite-plugin-feature-flags.ts-43-47 (1)
43-47:⚠️ Potential issue | 🟠 MajorOnly treat
FEATURE_*variables set to1as enabled.Right now any present variable enables the flag, so
FEATURE_ACP=0andFEATURE_ACP=falsestill turn ACP on. That breaks the documented override contract.Suggested fix
export function getEnabledFeatures(): Set<string> { - const envFeatures = Object.keys(process.env) - .filter((k) => k.startsWith("FEATURE_")) - .map((k) => k.replace("FEATURE_", "")); + const envFeatures = Object.entries(process.env) + .filter(([k, v]) => k.startsWith("FEATURE_") && v === "1") + .map(([k]) => k.replace("FEATURE_", "")); return new Set([...DEFAULT_BUILD_FEATURES, ...envFeatures]); }Based on learnings: Enable feature flags at runtime using environment variables:
FEATURE_<FLAG_NAME>=1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vite-plugin-feature-flags.ts` around lines 43 - 47, The current getEnabledFeatures treats any FEATURE_* env var as enabled; change it to only enable flags whose process.env value equals "1". In getEnabledFeatures(), iterate Object.entries(process.env) or filter Object.keys by checking process.env[k] === "1" (instead of only startsWith), extract the flag name via k.replace("FEATURE_", ""), and then return new Set([...DEFAULT_BUILD_FEATURES, ...envFeatures]) so only FEATURE_<NAME>=1 adds the feature while keeping DEFAULT_BUILD_FEATURES untouched.packages/@anthropic-ai/model-provider/src/providers/grok/modelMapping.ts-34-40 (1)
34-40:⚠️ Potential issue | 🟠 MajorValidate
GROK_MODEL_MAPvalues and honor exact model keys first.
JSON.parse()is asserted straight toRecord<string, string>, so non-string values can pass through unchecked, and the resolver only readsuserMap[family]. That meansGROK_MODEL_MAP='{"claude-opus-4-6":"grok-x"}'is ignored even though this option is documented as a model map.Suggested fix
function getUserModelMap(): Record<string, string> | null { const raw = process.env.GROK_MODEL_MAP if (!raw) return null try { - const parsed = JSON.parse(raw) + const parsed = JSON.parse(raw) as Record<string, unknown> if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { - return parsed as Record<string, string> + return Object.fromEntries( + Object.entries(parsed).filter(([, value]) => typeof value === 'string'), + ) as Record<string, string> } } catch { // ignore invalid JSON } return null } @@ const userMap = getUserModelMap() - if (userMap && family && userMap[family]) { + if (userMap?.[cleanModel]) { + return userMap[cleanModel] + } + if (userMap && family && userMap[family]) { return userMap[family] }As per coding guidelines: For unknown structure objects, use
Record<string, unknown>instead ofanytype.Also applies to: 59-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@anthropic-ai/model-provider/src/providers/grok/modelMapping.ts around lines 34 - 40, The GROK_MODEL_MAP parsing currently casts JSON.parse() to Record<string,string> and doesn't validate values or exact model keys; update getUserModelMap to parse into Record<string,unknown>, iterate entries and build a new Record<string,string> that only includes keys whose values are typeof 'string' (discard non-string entries), and return null if no valid mappings; also ensure the resolver logic that looks up userMap[family] first is preserved so exact model keys (e.g., "claude-opus-4-6") are honored before any family-based lookup—apply the same input validation/filtering approach to the similar code around the other mapping code referenced (lines ~59-61).scripts/vite-plugin-feature-flags.ts-7-38 (1)
7-38:⚠️ Potential issue | 🟠 MajorSource the default feature list from the canonical build config.
build:vitenow carries its own allowlist, so it can drift frombuild.tsand produce a different binary for the same commit. This list should come from the same source of truth instead of being duplicated here.Based on learnings: Feature flags are disabled by default. Dev mode enables all features by default; build mode enables 19 specific features (listed in build.ts). Centrally manage build defines (constants, version numbers) in
scripts/defines.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vite-plugin-feature-flags.ts` around lines 7 - 38, The hardcoded DEFAULT_BUILD_FEATURES array in vite-plugin-feature-flags.ts must be removed and the canonical feature-list imported from the single source of truth used by build.ts (e.g., the exported build feature constant in scripts/defines.ts or the build.ts module). Replace DEFAULT_BUILD_FEATURES with the imported symbol (e.g., BUILD_FEATURES or DEFAULT_FEATURE_FLAGS) and ensure the plugin still applies the same dev vs build semantics (dev: enable all; build: use the imported allowlist of 19 features) so the Vite build and the primary build pipeline use the identical feature set.src/services/acp/entry.ts-41-62 (1)
41-62:⚠️ Potential issue | 🟠 MajorGuard shutdown before the first agent instance exists.
If stdin closes or a signal arrives before the connection callback assigns
agent,shutdown()throws onagent.sessionsand the cleanup path fails.Suggested fix
- let agent!: AcpAgent + let agent: AcpAgent | undefined @@ async function shutdown(): Promise<void> { // Clean up all active sessions - for (const [sessionId] of agent.sessions) { + if (!agent) { + process.exit(0) + } + + for (const [sessionId] of agent.sessions) { try { await agent.unstable_closeSession({ sessionId }) } catch { // Best-effort cleanup }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/entry.ts` around lines 41 - 62, The shutdown function can run before the AgentSideConnection callback assigns the agent, causing a crash when accessing agent.sessions; modify shutdown to guard for an uninitialized agent by checking that the local variable agent is truthy before iterating its sessions (or return early if unset), and only call agent.unstable_closeSession for each session when agent is defined; reference the agent variable, the shutdown() function, the agent.sessions iteration, and unstable_closeSession when making the change.src/services/acp/entry.ts-30-51 (1)
30-51:⚠️ Potential issue | 🟠 MajorRedirect
console.*before any ACP startup work runs.
enableConfigs(),applySafeConfigEnvironmentVariables(), orAgentSideConnectionsetup can log before the redirect happens. Any such output lands on stdout and corrupts the NDJSON ACP stream.Suggested fix
export async function runAcpAgent(): Promise<void> { + // stdout is reserved for ACP messages + console.log = console.error + console.info = console.error + console.warn = console.error + console.debug = console.error + enableConfigs() @@ - // stdout is used for ACP messages — redirect console to stderr - console.log = console.error - console.info = console.error - console.warn = console.error - console.debug = console.error - async function shutdown(): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/entry.ts` around lines 30 - 51, Move the console redirection to the very start of runAcpAgent so no logging from enableConfigs, applySafeConfigEnvironmentVariables, createAcpStream or AgentSideConnection can write to stdout and corrupt the NDJSON ACP stream; specifically, set console.log/console.info/console.warn/console.debug to console.error before calling enableConfigs(), applySafeConfigEnvironmentVariables(), createAcpStream() or instantiating AgentSideConnection/AcpAgent so stdout remains reserved for ACP messages.scripts/vite-plugin-feature-flags.ts-104-110 (1)
104-110:⚠️ Potential issue | 🟠 MajorSymbol.dispose is dropped when transpiling
usingtoconst.Replacing
usingdeclarations withconstremoves automatic disposal semantics entirely. WhileslowLoggingExternalcurrently returns a no-op disposable with an empty[Symbol.dispose]()method, this safety relies onSLOW_OPERATION_LOGGINGremaining disabled. If the flag is ever enabled or the behavior ofslowLoggingchanges to return a real disposable, the transpiled code will silently fail to invoke cleanup logic that the source code expects. To maintain correct semantics, either keep theusingstatement or add a comment explaining why this specific case is safe to transpile unconditionally, and document the assumption being made.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vite-plugin-feature-flags.ts` around lines 104 - 110, The transpilation that replaces "using _" via transformed.replace removes disposal semantics and can drop calls to [Symbol.dispose] when SLOW_OPERATION_LOGGING or slowLoggingExternal change; update the transformation in vite-plugin-feature-flags.ts so it preserves disposal semantics instead of unconditionally swapping to "const": when you detect the "using _" pattern, emit code that still binds the disposable (the "_" symbol) and ensures disposal in a finally block (checking for _[Symbol.dispose] or a dispose method) or else leave the original using statement and add a comment asserting safety; reference the transformed string replacement logic that matches /\busing\s+(_\w*)\s*=/, the "using _" detection, the transformed variable name "$1", and the feature flag SLOW_OPERATION_LOGGING/slowLoggingExternal when making the change.scripts/vite-plugin-feature-flags.ts-50-52 (1)
50-52:⚠️ Potential issue | 🟠 MajorAvoid stub-based
feature()resolution in Vite—use Bun's compiler-resolved approach instead.The regex + stub pattern bypasses Bun's intended mechanism. According to Bun 1.2 documentation,
import { feature } from "bun:bundle"is compiler-resolved during bundling (calls are replaced with boolean literals via static analysis), and custom runtime stubs are unsupported—at runtime without Bun bundling, nobun:bundlemodule exists. Even though this plugin pre-transforms feature calls before bundling, the fallback stub (line 84) means any unmatchedfeature(...)call (non-string-literal arguments, template literals, variables) silently returnsfalseinstead of failing visibly. Non-string-literal usage already violates feature flag guidelines, but this approach masks errors rather than surfacing them. Align with Bun's standard pattern and let the bundler handle resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vite-plugin-feature-flags.ts` around lines 50 - 52, The plugin currently uses FEATURE_CALL_RE to pre-transform feature('FLAG') calls and provides a runtime stub that returns false for unmatched or non-string-literal calls; remove that stub-based fallback and stop masking invalid usage—only transform calls that match FEATURE_CALL_RE (string-literal args) and leave all other feature(...) calls untouched so Bun's compiler can resolve them, and emit a build-time error or warning when you detect non-string-literal feature(...) usages (so authors fix them instead of silently getting false). Ensure references to FEATURE_CALL_RE and the runtime stub removal are updated and that import { feature } from "bun:bundle" is not emulated by the plugin.packages/@anthropic-ai/model-provider/src/shared/openaiStreamAdapter.ts-239-271 (1)
239-271:⚠️ Potential issue | 🟠 MajorAlways emit a terminal
message_delta/message_stop.Right now the tail of the generator is gated on seeing
choice.finish_reason. If an OpenAI-compatible endpoint closes the stream without that final field, callers get content blocks but never a terminal event or final usage payload.💡 Minimal fix
- if (pendingFinishReason !== null) { + if (started) { + const effectiveFinishReason = pendingFinishReason ?? 'stop' const stopReason = - pendingFinishReason === 'length' + effectiveFinishReason === 'length' ? 'max_tokens' - : pendingHasToolCalls + : pendingHasToolCalls || toolBlocks.size > 0 ? 'tool_use' - : mapFinishReason(pendingFinishReason) + : mapFinishReason(effectiveFinishReason) yield { type: 'message_delta',Also applies to: 282-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@anthropic-ai/model-provider/src/shared/openaiStreamAdapter.ts around lines 239 - 271, The generator currently only emits the terminal message_delta/message_stop when choice.finish_reason is present, which can leave callers without a final event if the upstream stream closes early; update the end-of-stream handling in openaiStreamAdapter.ts (around the logic that sets pendingFinishReason/pendingHasToolCalls and the later 282-308 block) to always emit a terminal event and final usage payload even when choice.finish_reason is undefined: detect stream termination (e.g., when the loop exits or when no finish_reason is provided), set a default finish reason or leave it null but still yield a 'message_delta'/'message_stop' (and the corresponding usage/stop event), and ensure openBlockIndices/toolBlocks are closed and cleared before yielding so consumers always receive a single final terminal event.packages/@anthropic-ai/model-provider/src/providers/gemini/streamAdapter.ts-197-206 (1)
197-206:⚠️ Potential issue | 🟠 MajorReturn the final prompt token count in
message_delta.
usageMetadatacan arrive aftermessage_start, but the terminal payload only includesoutput_tokens. In that case callers never see the realinput_tokens, so accounting/cost code will stay at 0 for prompt usage.💡 Suggested fix
usage: { + input_tokens: inputTokens, output_tokens: outputTokens, + cache_creation_input_tokens: 0, + cache_read_input_tokens: 0, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@anthropic-ai/model-provider/src/providers/gemini/streamAdapter.ts around lines 197 - 206, The final emitted message_delta currently only includes stop_reason and output_tokens, which loses prompt token accounting if usageMetadata arrived after message_start; update the terminal yield in the streamAdapter (the block that checks stopped and calls mapGeminiFinishReason) to also include the final prompt token count by reading usageMetadata.input_tokens (or the tracked inputTokens variable) and adding it to delta (e.g., delta.input_tokens) so callers receive both input and output token counts even when usageMetadata arrives late.src/services/acp/__tests__/agent.test.ts-87-89 (1)
87-89:⚠️ Potential issue | 🟠 MajorChange mock specifier from
.tsto.js.The import in
src/services/acp/agent.tsuses'../../utils/model/modelOptions.js'(line 70), but the mock registers'../../../utils/model/modelOptions.ts'(line 87). Sincebun:testmock.module()matches the literal import string exactly, this.tsmock does not intercept the.jsimport, causing the test to execute the real module instead.Change
mock.module('../../../utils/model/modelOptions.js', () => ({ getModelOptions: mock(() => []), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/__tests__/agent.test.ts` around lines 87 - 89, The test mock for the model options is registered with the wrong file extension so it doesn't match the real import; update the mock.module call in the test (the mock.module(...) invocation in src/services/acp/__tests__/agent.test.ts) to use the same literal specifier the code imports ('../../utils/model/modelOptions.js' as used by src/services/acp/agent.ts) — i.e., change the specifier from '../../../utils/model/modelOptions.ts' to '../../../utils/model/modelOptions.js' so bun:test's mock.module() will intercept the import and return the mocked getModelOptions.packages/@anthropic-ai/model-provider/src/errorUtils.ts-233-237 (1)
233-237:⚠️ Potential issue | 🟠 MajorDon't fall back to raw HTML after sanitization.
sanitizeMessageHTML()intentionally returns an empty string for HTML without a<title>, but this branch then returns the original HTML page. That means proxy/Cloudflare blobs still leak into the UI even after the sanitization pass.💡 Suggested fix
const sanitizedMessage = sanitizeAPIError(error) - return sanitizedMessage !== error.message && sanitizedMessage.length > 0 - ? sanitizedMessage - : error.message + if (sanitizedMessage !== error.message) { + return sanitizedMessage.length > 0 + ? sanitizedMessage + : 'API returned an HTML error page' + } + return error.message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@anthropic-ai/model-provider/src/errorUtils.ts around lines 233 - 237, The current return expression falls back to the raw error.message when sanitizeAPIError returns an empty string (which is intentional for HTML without a <title>), leaking HTML into the UI; change the return logic in errorUtils.ts so that if sanitizeAPIError(error) returns a different value than error.message you return that sanitized value (even if it's an empty string) instead of falling back to the original HTML—i.e., use the sanitizedMessage !== error.message check alone (return sanitizedMessage when different, otherwise return error.message) referencing the sanitizeAPIError call and the sanitizedMessage variable.packages/@anthropic-ai/model-provider/src/shared/openaiStreamAdapter.ts-69-77 (1)
69-77:⚠️ Potential issue | 🟠 MajorRemove
as anyassertions for OpenAI compatibility fields.Lines 73 and 107 use
as anyto access provider-specific extensions (prompt_tokens_details.cached_tokensandreasoning_content) that aren't typed in the OpenAI SDK. Per strict mode enforcement, introduce a local compatibility interface or useRecord<string, unknown>with a type guard instead:// Option 1: Local interface for OpenAI extension fields interface OpenAICompat { prompt_tokens_details?: { cached_tokens?: number } } const details = (chunk.usage as OpenAICompat).prompt_tokens_details // Option 2: Type guard with Record const details = (chunk.usage as unknown as Record<string, unknown>).prompt_tokens_details🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@anthropic-ai/model-provider/src/shared/openaiStreamAdapter.ts around lines 69 - 77, The code is using unsafe `as any` casts on chunk.usage to access provider-specific fields (prompt_tokens_details.cached_tokens and reasoning_content); replace these with a typed compatibility approach: define a local interface (e.g., OpenAICompat with prompt_tokens_details?: { cached_tokens?: number } and reasoning_content?: string) and cast chunk.usage to that interface where you read those fields in openaiStreamAdapter.ts (references: chunk.usage, prompt_tokens_details, cached_tokens, reasoning_content), or alternatively cast to Record<string, unknown> and use a type guard to safely read and narrow those properties before assigning cachedReadTokens and reasoning content.packages/@anthropic-ai/model-provider/src/shared/openaiConvertMessages.ts-65-74 (1)
65-74:⚠️ Potential issue | 🟠 MajorThe turn-boundary scan still uses
any.This branch is deciding whether old reasoning gets stripped, so it should stay inside the same strict narrowing rules as the rest of the converter. Please narrow
bwithRecord<string, unknown>or a local block guard instead ofany.💡 Narrow without `any`
- const startsNewUserTurn = typeof content === 'string' - ? content.length > 0 - : Array.isArray(content) && content.some( - (b: any) => - typeof b === 'string' || - (b && - typeof b === 'object' && - 'type' in b && - b.type !== 'tool_result'), - ) + const startsNewUserTurn = typeof content === 'string' + ? content.length > 0 + : Array.isArray(content) && + content.some(block => { + if (typeof block === 'string') return true + if (!block || typeof block !== 'object') return false + const narrowed = block as Record<string, unknown> + return narrowed.type !== 'tool_result' + })As per coding guidelines
**/*.{ts,tsx}: TypeScript strict mode is enforced —bunx tsc --noEmitmust pass with zero errors. Noas anyallowed in production code, and for unknown structure objects useRecord<string, unknown>instead ofanytype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@anthropic-ai/model-provider/src/shared/openaiConvertMessages.ts around lines 65 - 74, The turn-boundary predicate for startsNewUserTurn is using an unsafe any for the array element `b`; change that to a stricter type (e.g. `Record<string, unknown>`) and add a local null/object guard before checking the `'type'` property so TypeScript narrows correctly. Concretely, in the Array.isArray(content) branch update the callback to accept `b: unknown`, first check `typeof b === 'string'` then `typeof b === 'object' && b !== null` and finally assert `('type' in b)` with `b as Record<string, unknown>` when comparing `b.type !== 'tool_result'` so no `any` is used and tsc strict mode passes for the `startsNewUserTurn` expression.
🟡 Minor comments (12)
src/utils/__tests__/imageResizer.test.ts-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorUse
src/alias import instead of relative path.Line 8 should use the repository
src/...alias for utility imports to match TS path conventions.Suggested patch
-import { detectImageFormatFromBase64, detectImageFormatFromBuffer } from '../imageResizer.js' +import { detectImageFormatFromBase64, detectImageFormatFromBuffer } from 'src/utils/imageResizer.js'As per coding guidelines, "
**/*.{ts,tsx}: Import utility functions from thesrc/path alias."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/imageResizer.test.ts` at line 8, Replace the relative import in the test that pulls detectImageFormatFromBase64 and detectImageFormatFromBuffer from '../imageResizer.js' with the repository path-alias import (e.g., 'src/utils/imageResizer') so the test uses the TS path alias; update the import statement for the symbols detectImageFormatFromBase64 and detectImageFormatFromBuffer accordingly to follow the project's src/ alias convention.src/keybindings/__tests__/confirmation-keybindings.test.ts-43-43 (1)
43-43:⚠️ Potential issue | 🟡 MinorUse English-only suite description to match test guideline.
The
describe(...)title includes Chinese text; please keep test descriptions fully English for consistency with repo standards.Suggested update
-describe('Confirmation context — n/y keys removed (fix: 修复 n 快捷键导致关闭的问题)', () => { +describe('Confirmation context — n/y keys removed (fix for accidental dismissal)', () => {As per coding guidelines: "
**/__tests__/**/*.test.ts: Use English for test descriptions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keybindings/__tests__/confirmation-keybindings.test.ts` at line 43, Update the test suite title used in the describe(...) call in confirmation-keybindings.test.ts to an English-only string to comply with the repo's test-guideline; locate the describe('Confirmation context — n/y keys removed (fix: 修复 n 快捷键导致关闭的问题)', ...) invocation and replace the Chinese characters and any non-English punctuation so the entire suite description is English-only while preserving its intent.src/utils/__tests__/earlyInput.test.ts-22-26 (1)
22-26:⚠️ Potential issue | 🟡 MinorUse
src/alias import in tests for utilities.The relative import should follow the repo alias convention for
ts/tsxfiles.Suggested change
import { seedEarlyInput, consumeEarlyInput, hasEarlyInput, -} from '../earlyInput.js' +} from 'src/utils/earlyInput'As per coding guidelines
**/*.{ts,tsx}: Import utility functions from thesrc/path alias. Use imports likeimport { ... } from 'src/utils/...'which are resolved via tsconfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/earlyInput.test.ts` around lines 22 - 26, Update the test import to use the repo tsconfig alias: replace the relative import of seedEarlyInput, consumeEarlyInput, and hasEarlyInput with the aliased module import from 'src/utils/earlyInput' so the test follows the project's src/ path convention.docs/features/remote-control-self-hosting.md-143-145 (1)
143-145:⚠️ Potential issue | 🟡 MinorAdd language identifiers to the new fenced blocks.
These two new fences are currently flagged by markdownlint (MD040). Adding a simple
textlanguage keeps the docs clean and avoids lint noise.📝 Proposed fix
-``` +```text https://rcs.example.com/code?bridge=<environmentId>-
+text
https://rcs.example.com/code/session_Also applies to: 149-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/remote-control-self-hosting.md` around lines 143 - 145, Add the language identifier "text" to the new fenced code blocks that contain the example URLs (the fences showing "https://rcs.example.com/code?bridge=<environmentId>" and "https://rcs.example.com/code/session_<id>") so they become ```text ... ``` to satisfy markdownlint MD040 and avoid lint noise.packages/remote-control-server/src/store.ts-203-205 (1)
203-205:⚠️ Potential issue | 🟡 MinorClean up owner bindings when deleting a session.
This function now removes
sessionWorkers, but it still leaves the per-session entry insessionOwners. On a long-lived process, deleted sessions will keep orphaned ownership sets around indefinitely.🧹 Proposed fix
export function storeDeleteSession(id: string): boolean { sessionWorkers.delete(id); + sessionOwners.delete(id); return sessions.delete(id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/store.ts` around lines 203 - 205, storeDeleteSession currently removes sessionWorkers and sessions but leaves the per-session entry in sessionOwners, causing orphaned ownership sets; update the storeDeleteSession function to also remove the sessionOwners entry for the given id (preferably clear the Set if present and then call sessionOwners.delete(id)) so both the ownership mapping and any held references are cleaned up when deleting a session.packages/remote-control-server/src/__tests__/routes.test.ts-1274-1284 (1)
1274-1284:⚠️ Potential issue | 🟡 MinorRead until the
client_eventframe instead of assuming it's the next chunk.Lines 1274-1284 currently assume the post-publish
reader.read()returns the event frame, but this stream already emits keepalives and can legally send another one first. That makes the test intermittently fail even when the endpoint is correct.Proposed test hardening
- const secondChunk = await reader.read(); - const frame = new TextDecoder().decode(secondChunk.value!); - expect(frame).toContain("event: client_event"); - expect(frame).toContain("\"payload\":{\"type\":\"user\",\"content\":\"hello\",\"message\":{\"content\":\"hello\"}}"); + const deadline = Date.now() + 2000; + let frame = ""; + while (Date.now() < deadline) { + const chunk = await reader.read(); + frame = new TextDecoder().decode(chunk.value!); + if (frame.includes("event: client_event")) break; + } + expect(frame).toContain("event: client_event"); + expect(frame).toContain("\"payload\":{\"type\":\"user\",\"content\":\"hello\",\"message\":{\"content\":\"hello\"}}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/__tests__/routes.test.ts` around lines 1274 - 1284, The test assumes the event is the next chunk after publishSessionEvent; instead change the read logic to loop calling reader.read() (replacing secondChunk) decoding each chunk into frame and continue until the decoded frame includes "event: client_event" (then assert payload and break), and finally call reader.cancel(); reference the reader.read() usage, publishSessionEvent(id, ...), and the variables firstChunk/keepalive/frame so the loop consumes intervening keepalive frames reliably.src/services/acp/permissions.ts-171-183 (1)
171-183:⚠️ Potential issue | 🟡 MinorMissing error handling in handleExitPlanMode.
Unlike the main permission flow (lines 90-131) which wraps
conn.requestPermissionin a try/catch,handleExitPlanModedoes not handle errors from the permission request. If the connection fails, this will throw and propagate up instead of returning a deny decision.🛡️ Proposed fix to add error handling
- const response = await conn.requestPermission({ - sessionId, - toolCall, - options, - }) + let response + try { + response = await conn.requestPermission({ + sessionId, + toolCall, + options, + }) + } catch { + return { + behavior: 'deny', + message: 'Permission request failed', + decisionReason: { type: 'mode', mode: 'plan' }, + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/permissions.ts` around lines 171 - 183, The handleExitPlanMode path calls conn.requestPermission (observed around the response/outcome check) without a try/catch, so connection errors will throw instead of returning a deny decision; wrap the call to conn.requestPermission in a try/catch inside handleExitPlanMode and on any error return the same shaped deny response used in the main flow (behavior: 'deny', decisionReason with type 'mode' and mode 'default', and a message conveying tool use was aborted or the error), ensuring you still handle the existing response.outcome.outcome === 'cancelled' branch after the try block.src/services/api/claude.ts-2919-2919 (1)
2919-2919:⚠️ Potential issue | 🟡 MinorUse full API tool set in Langfuse observation.
Line 2919 only logs
toolSchemas, but the request can includeextraToolSchemasviaallTools. This can make traces incomplete when advisor/server tools are appended.Suggested fix
- tools: convertToolsToLangfuse(toolSchemas as unknown[]), + tools: convertToolsToLangfuse(allTools as unknown[]),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/claude.ts` at line 2919, The Langfuse observation is only logging toolSchemas via convertToolsToLangfuse(toolSchemas as unknown[]) but may miss extraToolSchemas provided through allTools; update the observation to use the full combined tool set (e.g., merge toolSchemas and extraToolSchemas/allTools) and pass that combined array into convertToolsToLangfuse so traces include advisor/server-appended tools — modify the call site where convertToolsToLangfuse is invoked (referencing convertToolsToLangfuse, toolSchemas, extraToolSchemas/allTools) to construct and supply the complete tools list.docs/features/acp-zed.md-67-67 (1)
67-67:⚠️ Potential issue | 🟡 MinorSection numbering inconsistency: missing section 3.2.
Section numbering jumps from 3.1 to 3.3, skipping 3.2. Either add the missing section or renumber 3.3 → 3.2 and subsequent sections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/acp-zed.md` at line 67, The document's section numbering skips 3.2 (it shows 3.1 then 3.3 API 认证配置); update the headings to restore consistent numbering by either inserting the missing 3.2 content if it was omitted or renumbering "3.3 API 认证配置" (and all subsequent headings and any internal cross-references) to "3.2" (and adjust later numbers accordingly); search for the heading text "3.3 API 认证配置" and any references to "3.3" or later section numbers and update them to the corrected numbering to keep the whole document consistent.vite.config.ts-91-94 (1)
91-94:⚠️ Potential issue | 🟡 MinorRemove trailing slash from
src/alias key to align with standard Vite configuration patterns.The alias is currently configured as
"src/"with a trailing slash. While this works functionally, the standard Vite convention (and how tsconfig defines paths) is to use"src"without a trailing slash in the alias key:♻️ Standard alias pattern
alias: { // src/* path alias (mirrors tsconfig paths) - "src/": resolve(projectRoot, "src/"), + src: resolve(projectRoot, "src"), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 91 - 94, The alias key currently uses "src/" with a trailing slash; update the alias object so the key is "src" (no trailing slash) while keeping the resolved path value (resolve(projectRoot, "src/")) unchanged—i.e., change the alias entry in the alias map (the "alias" object in vite config) from "src/" to "src" to match standard Vite/tsconfig conventions.src/services/acp/agent.ts-661-670 (1)
661-670:⚠️ Potential issue | 🟡 MinorInconsistency between
validModesandavailableModes.
validModesincludes'bypassPermissions'but theavailableModesarray increateSession(lines 513-519) doesn't include this mode. This allows setting a mode viaapplySessionModethat isn't advertised to clients.Consider either adding
'bypassPermissions'toavailableModesor removing it fromvalidModesfor consistency.Option A: Remove from validModes
private applySessionMode(sessionId: string, modeId: string): void { - const validModes = ['auto', 'default', 'acceptEdits', 'bypassPermissions', 'dontAsk', 'plan'] + const validModes = ['auto', 'default', 'acceptEdits', 'dontAsk', 'plan'] if (!validModes.includes(modeId)) { throw new Error(`Invalid mode: ${modeId}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/agent.ts` around lines 661 - 670, The applySessionMode function permits 'bypassPermissions' (validModes) but createSession's availableModes array doesn't advertise it, causing inconsistency; update createSession (the availableModes array) to include 'bypassPermissions' so clients see all modes allowed by applySessionMode, or alternatively remove 'bypassPermissions' from validModes in applySessionMode and any related logic; locate createSession and applySessionMode to make the corresponding addition/removal so both arrays match.src/services/acp/agent.ts-316-325 (1)
316-325:⚠️ Potential issue | 🟡 MinorMissing
awaitonteardownSessionmay cause race conditions.
teardownSessionis an async method that callscancel()internally. Calling it withoutawaitmeans the error is thrown before the session cleanup completes, potentially leaving state inconsistent if the caller retries immediately.Proposed fix
if ( err instanceof Error && (err.message.includes('terminated') || err.message.includes('process exited')) ) { - this.teardownSession(params.sessionId) + await this.teardownSession(params.sessionId) throw new Error( 'The Claude Agent process exited unexpectedly. Please start a new session.', ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/agent.ts` around lines 316 - 325, The teardownSession call in the error-handling branch is asynchronous and currently invoked without awaiting, which can cause race conditions because teardownSession internally calls cancel(); change the handler to await this.teardownSession(params.sessionId) before throwing the new Error so cleanup completes first; locate the block checking err instanceof Error and err.message.includes(...) in the agent (function/method containing teardownSession usage) and add await to the teardownSession invocation and ensure the enclosing function is async or already supports awaiting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e8831d8-63c4-4fc6-9711-764926637ed5
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockcontributors.svgis excluded by!**/*.svg
📒 Files selected for processing (135)
.github/workflows/ci.ymlCLAUDE.mdREADME.mdbuild.tsdocs/diagrams/agent-loop-simple.mmddocs/diagrams/agent-loop.mmddocs/features/acp-zed.mddocs/features/chrome-use-mcp.mddocs/features/remote-control-self-hosting.mdpackage.jsonpackages/@ant/claude-for-chrome-mcp/tsconfig.jsonpackages/@ant/computer-use-input/src/backends/darwin.tspackages/@ant/computer-use-input/tsconfig.jsonpackages/@ant/computer-use-mcp/tsconfig.jsonpackages/@ant/computer-use-swift/src/backends/darwin.tspackages/@ant/computer-use-swift/src/backends/linux.tspackages/@ant/computer-use-swift/src/types.tspackages/@ant/computer-use-swift/tsconfig.jsonpackages/@ant/ink/tsconfig.jsonpackages/@anthropic-ai/model-provider/package.jsonpackages/@anthropic-ai/model-provider/src/client/index.tspackages/@anthropic-ai/model-provider/src/client/types.tspackages/@anthropic-ai/model-provider/src/errorUtils.tspackages/@anthropic-ai/model-provider/src/hooks/index.tspackages/@anthropic-ai/model-provider/src/hooks/types.tspackages/@anthropic-ai/model-provider/src/index.tspackages/@anthropic-ai/model-provider/src/providers/gemini/convertMessages.tspackages/@anthropic-ai/model-provider/src/providers/gemini/convertTools.tspackages/@anthropic-ai/model-provider/src/providers/gemini/modelMapping.tspackages/@anthropic-ai/model-provider/src/providers/gemini/streamAdapter.tspackages/@anthropic-ai/model-provider/src/providers/gemini/types.tspackages/@anthropic-ai/model-provider/src/providers/grok/modelMapping.tspackages/@anthropic-ai/model-provider/src/providers/openai/modelMapping.tspackages/@anthropic-ai/model-provider/src/shared/openaiConvertMessages.tspackages/@anthropic-ai/model-provider/src/shared/openaiConvertTools.tspackages/@anthropic-ai/model-provider/src/shared/openaiStreamAdapter.tspackages/@anthropic-ai/model-provider/src/types/errors.tspackages/@anthropic-ai/model-provider/src/types/index.tspackages/@anthropic-ai/model-provider/src/types/message.tspackages/@anthropic-ai/model-provider/src/types/systemPrompt.tspackages/@anthropic-ai/model-provider/src/types/usage.tspackages/@anthropic-ai/model-provider/tsconfig.jsonpackages/agent-tools/src/__tests__/compat.test.tspackages/agent-tools/tsconfig.jsonpackages/audio-capture-napi/tsconfig.jsonpackages/builtin-tools/src/tools/AgentTool/__tests__/agentDisplay.test.tspackages/builtin-tools/tsconfig.jsonpackages/color-diff-napi/src/__tests__/color-diff.test.tspackages/color-diff-napi/tsconfig.jsonpackages/image-processor-napi/tsconfig.jsonpackages/mcp-client/src/__tests__/InProcessTransport.test.tspackages/mcp-client/src/__tests__/discovery.test.tspackages/mcp-client/src/__tests__/manager.test.tspackages/mcp-client/tsconfig.jsonpackages/modifiers-napi/tsconfig.jsonpackages/remote-control-server/src/__tests__/disconnect-monitor.test.tspackages/remote-control-server/src/__tests__/routes.test.tspackages/remote-control-server/src/__tests__/services.test.tspackages/remote-control-server/src/__tests__/ws-handler.test.tspackages/remote-control-server/src/routes/v1/environments.tspackages/remote-control-server/src/routes/v1/environments.work.tspackages/remote-control-server/src/routes/v1/session-ingress.tspackages/remote-control-server/src/routes/v1/sessions.tspackages/remote-control-server/src/routes/v2/code-sessions.tspackages/remote-control-server/src/routes/v2/worker-events-stream.tspackages/remote-control-server/src/routes/v2/worker-events.tspackages/remote-control-server/src/routes/v2/worker.tspackages/remote-control-server/src/routes/web/auth.tspackages/remote-control-server/src/routes/web/control.tspackages/remote-control-server/src/routes/web/sessions.tspackages/remote-control-server/src/services/disconnect-monitor.tspackages/remote-control-server/src/services/session.tspackages/remote-control-server/src/services/transport.tspackages/remote-control-server/src/store.tspackages/remote-control-server/src/transport/sse-writer.tspackages/remote-control-server/src/transport/ws-handler.tspackages/remote-control-server/tsconfig.jsonpackages/remote-control-server/web/app.jspackages/remote-control-server/web/components.csspackages/remote-control-server/web/index.htmlpackages/remote-control-server/web/render.jspackages/remote-control-server/web/utils.jspackages/url-handler-napi/tsconfig.jsonscripts/dev.tsscripts/post-build.tsscripts/vite-plugin-feature-flags.tsscripts/vite-plugin-import-meta-require.tssrc/QueryEngine.tssrc/bridge/bridgeMessaging.tssrc/cli/handlers/auth.tssrc/cli/print.tssrc/commands/poor/__tests__/poorMode.test.tssrc/components/ConsoleOAuthFlow.tsxsrc/components/messages/SystemAPIErrorMessage.tsxsrc/entrypoints/cli.tsxsrc/entrypoints/sdk/sdkUtilityTypes.tssrc/keybindings/__tests__/confirmation-keybindings.test.tssrc/keybindings/defaultBindings.tssrc/services/acp/__tests__/agent.test.tssrc/services/acp/__tests__/bridge.test.tssrc/services/acp/__tests__/permissions.test.tssrc/services/acp/agent.tssrc/services/acp/bridge.tssrc/services/acp/entry.tssrc/services/acp/permissions.tssrc/services/acp/utils.tssrc/services/api/claude.tssrc/services/api/emptyUsage.tssrc/services/api/errorUtils.tssrc/services/api/gemini/convertMessages.tssrc/services/api/gemini/convertTools.tssrc/services/api/gemini/modelMapping.tssrc/services/api/gemini/streamAdapter.tssrc/services/api/gemini/types.tssrc/services/api/grok/modelMapping.tssrc/services/api/openai/__tests__/convertMessages.test.tssrc/services/api/openai/__tests__/queryModelOpenAI.test.tssrc/services/api/openai/convertMessages.tssrc/services/api/openai/convertTools.tssrc/services/api/openai/index.tssrc/services/api/openai/modelMapping.tssrc/services/api/openai/streamAdapter.tssrc/services/langfuse/__tests__/langfuse.test.tssrc/services/langfuse/convert.tssrc/services/langfuse/tracing.tssrc/types/message.tssrc/utils/__tests__/bunHashPolyfill.test.tssrc/utils/__tests__/earlyInput.test.tssrc/utils/__tests__/imageResizer.test.tssrc/utils/forkedAgent.tssrc/utils/sideQuestion.tssrc/utils/systemPromptType.tstsconfig.base.jsontsconfig.jsonvite.config.ts
💤 Files with no reviewable changes (1)
- src/keybindings/defaultBindings.ts
Comment on lines
+8
to
+13
| import { execFile, execFileSync } from 'child_process' | ||
| import { promisify } from 'util' | ||
| import type { FrontmostAppInfo, InputBackend } from '../types.js' | ||
|
|
||
| const execFileAsync = promisify(execFile) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify runtime guidance and this file's process-exec strategy.
set -euo pipefail
# 1) Locate Bun runtime guidance in repo docs.
fd -i 'CLAUDE.md' -x rg -n -C2 "Runtime must be Bun|Bun\\.build|bun:bundle|feature\\(" {}
# 2) Inspect current backend implementation for Node process APIs.
rg -n -C2 "from 'child_process'|from \"child_process\"|execFileAsync|execFileSync\\(" packages/@ant/computer-use-input/src/backends/darwin.tsRepository: claude-code-best/claude-code
Length of output: 3970
🏁 Script executed:
# Check if this is a separate package with its own configuration
cat packages/@ant/computer-use-input/package.json | head -30
# Check the directory structure and purpose
ls -la packages/@ant/computer-use-input/ | head -20
# Search for any documentation about this package
find packages/@ant/computer-use-input -maxdepth 2 -name "README*" -o -name "*.md" | xargs cat 2>/dev/null | head -100Repository: claude-code-best/claude-code
Length of output: 503
🏁 Script executed:
# Find where `@ant/computer-use-input` is imported
rg -n "@ant/computer-use-input" --type ts --type tsx
# Check if there's a workspace configuration or root tsconfig
cat package.json | head -50Repository: claude-code-best/claude-code
Length of output: 1459
🏁 Script executed:
# Search for imports of the package without type restrictions
rg "@ant/computer-use-input" -A 2
# Also check if there's a references to darwin.ts anywhere
rg "darwin\.ts|backends/darwin" -nRepository: claude-code-best/claude-code
Length of output: 9342
🏁 Script executed:
# Check how darwin backend is conditionally loaded or gated
rg -n "require.*darwin|import.*darwin|isSupported" packages/@ant/computer-use-input/src/
# Check the index.ts to see platform dispatch logic
cat packages/@ant/computer-use-input/src/index.tsRepository: claude-code-best/claude-code
Length of output: 2780
child_process and util imports prevent darwin backend from loading in Bun runtime.
Lines 8–9 import Node.js-only modules at the top level of darwin.ts. When Bun runtime executes require('./backends/darwin.js') in index.ts, these imports fail because Bun lacks child_process and util modules. The try-catch in loadBackend() masks this silently, leaving isSupported = false on macOS — breaking computer-use functionality.
Fix: Dynamically import child_process and util only when needed (lazy-load inside functions), or use Bun's native process APIs (Bun.spawn()) instead of Node.js execFile/execFileSync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@ant/computer-use-input/src/backends/darwin.ts around lines 8 - 13,
Top-level imports of Node-only modules (execFile, execFileSync from
child_process and promisify from util which creates execFileAsync) prevent the
darwin backend from loading in Bun; remove those top-level imports and lazy-load
the required APIs inside the functions that actually run commands (or replace
with Bun.spawn usage). Specifically, eliminate the top-level
execFile/execFileSync/promisify/execFileAsync symbols and instead require/import
child_process and util (or call Bun.spawn) within the command-running functions
in packages/@ant/computer-use-input/src/backends/darwin.ts so the module can be
loaded under Bun and only uses Node APIs when actually invoked.
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "extends": "../../tsconfig.json", | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All tsconfig.json files:"
fd -a '^tsconfig\.json$'
echo
python - <<'PY'
import json
from pathlib import Path
p = Path('packages/@anthropic-ai/model-provider/tsconfig.json')
cfg = json.loads(p.read_text())
ext = cfg.get("extends")
resolved = (p.parent / ext).resolve()
print("extends =", ext)
print("resolved =", resolved)
print("exists =", resolved.exists())
PYRepository: claude-code-best/claude-code
Length of output: 1140
Fix the extends path: target file does not exist.
The extends path ../../tsconfig.json resolves to packages/tsconfig.json, which does not exist in the repository. The repository root tsconfig.json is at tsconfig.json (root level). Change line 2 to:
"extends": "../../../tsconfig.json",This will correctly reference the base configuration at the repository root.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@anthropic-ai/model-provider/tsconfig.json at line 2, Update the
"extends" value in the package's tsconfig.json so it points to the repository
root base config instead of a non-existent packages/tsconfig.json; locate the
"extends" property in packages/@anthropic-ai/model-provider/tsconfig.json and
change its path to reference the root tsconfig.json (i.e., adjust the extends
string upward one extra directory level) so the package inherits the correct
base configuration.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
29-32: Harden Codecov step by pinning the action SHA and making LCOV input explicit.Using a mutable tag (
@v5) and implicit file discovery is less reproducible. Pin to a full-length commit SHA and pass the LCOV file directly. Since the build generates coverage viabun test --coverage --coverage-reporter=lcov, explicitly specify the output file.Proposed change
- name: Upload coverage to Codecov - uses: codecov/codecov-action@v5 + uses: codecov/codecov-action@<full-length-commit-sha-for-v5> with: token: ${{ secrets.CODECOV_TOKEN }} + files: ./coverage/lcov.info🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 29 - 32, The CI step "Upload coverage to Codecov" currently uses the mutable tag codecov/codecov-action@v5 and relies on implicit discovery; replace the mutable tag with the action's full commit SHA (pin to a specific full-length SHA for codecov/codecov-action) and add an explicit LCOV input (e.g., files: ./coverage/lcov.info) alongside the token to ensure deterministic behavior given your bun test --coverage --coverage-reporter=lcov output; update the step that references uses: codecov/codecov-action@v5 and the with: block to include the pinned SHA and the explicit LCOV file path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 29-32: The CI step "Upload coverage to Codecov" currently uses the
mutable tag codecov/codecov-action@v5 and relies on implicit discovery; replace
the mutable tag with the action's full commit SHA (pin to a specific full-length
SHA for codecov/codecov-action) and add an explicit LCOV input (e.g., files:
./coverage/lcov.info) alongside the token to ensure deterministic behavior given
your bun test --coverage --coverage-reporter=lcov output; update the step that
references uses: codecov/codecov-action@v5 and the with: block to include the
pinned SHA and the explicit LCOV file path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openai/index.ts (1)
243-243:⚠️ Potential issue | 🟡 MinorStep-number comment is duplicated.
Line 228 is already
// 12.; line 243 should be// 13.(and the subsequent inline blocks renumber accordingly). Pure documentation nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/openai/index.ts` at line 243, The inline step comment "// 12. Convert OpenAI stream to Anthropic events, then process into" is misnumbered (duplicating an earlier "// 12."); update that comment to "// 13." and then renumber any subsequent step-numbered inline comments accordingly so the sequence is correct; locate the comment string and adjust the numbering in the surrounding inline blocks to maintain increasing step numbers.
🧹 Nitpick comments (14)
packages/builtin-tools/src/tools/PowerShellTool/__tests__/gitSafety.test.ts (1)
11-20: Trim unused parser mock exports.
gitSafety.tsonly importsPS_TOKENIZER_DASH_CHARSfromsrc/utils/powershell/parser.js, so the other stubs (COMMON_ALIASES,commandHasArgAbbreviation,deriveSecurityFlags,getAllCommands,getVariablesByScope,hasCommandNamed,parsePowerShellCommandCached) aren't exercised here and just add noise / drift risk if the real exports change. Consider reducing the mock to only what's needed:♻️ Proposed trim
mock.module("src/utils/powershell/parser.js", () => ({ PS_TOKENIZER_DASH_CHARS: new Set(['-', '\u2013', '\u2014', '\u2015']), - COMMON_ALIASES: {}, - commandHasArgAbbreviation: () => false, - deriveSecurityFlags: () => ({}), - getAllCommands: () => [], - getVariablesByScope: () => [], - hasCommandNamed: () => false, - parsePowerShellCommandCached: () => ({ valid: false, errors: [], statements: [], variables: [], hasStopParsing: false, originalCommand: "" }), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/builtin-tools/src/tools/PowerShellTool/__tests__/gitSafety.test.ts` around lines 11 - 20, The mock in the test should only export what the test actually uses: replace the oversized mock.module stub with a minimal mock that exports only PS_TOKENIZER_DASH_CHARS (the constant imported by gitSafety.ts) so remove the unused stubs (COMMON_ALIASES, commandHasArgAbbreviation, deriveSecurityFlags, getAllCommands, getVariablesByScope, hasCommandNamed, parsePowerShellCommandCached); update the mock.module call in the test to return just PS_TOKENIZER_DASH_CHARS as a Set of dash chars to reduce noise and avoid drift.src/services/acp/__tests__/agent.test.ts (2)
33-38: Document the workaround more concretely, or consolidate on the alias form.The duplicate mock for
src/utils/config.js(alongside the relative-path mock on Line 29) is a reasonable workaround, but the comment references "agent.test.ts's relative-path mock … breaks Bun's src/* path alias for subsequent test files" without a linked issue or reproduction. If this is a known Bun quirk, a brief link/issue reference here will save future maintainers from deleting it as apparent duplication. If the relative-path mock is no longer strictly needed once the alias mock is present, consider dropping the relative form to avoid two sources of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/__tests__/agent.test.ts` around lines 33 - 38, Add a clearer comment or consolidate duplicate mocks: explain why mock.module('src/utils/config.js') is required (reference the Bun src/* alias resolution bug or link to its issue if known) and indicate whether the relative-path mock in agent.test.ts can be removed; alternatively, remove the relative-path mock and keep only the alias mock for 'src/utils/config.js' and ensure enableConfigs is still stubbed via mock(() => {}) so tests remain isolated.
23-27: Remove unusedTool.jsmocks.
agent.tsonly importsgetEmptyToolPermissionContextand theToolstype from../../../Tool.js. The mocks fortoolMatchesName,findToolByName,filterToolProgressMessages, andbuildToolare never referenced and add unnecessary maintenance burden.Additionally, the
buildToolidentity stub(def: any) => defdoes not match the real implementation's behavior (which spreadsTOOL_DEFAULTS). Ifagent.tsever usesbuildTool, this mock would silently return incomplete tools and mask bugs.♻️ Proposed cleanup
mock.module('../../../Tool.js', () => ({ getEmptyToolPermissionContext: mock(() => ({})), - toolMatchesName: mock(() => false), - findToolByName: mock(() => undefined), - filterToolProgressMessages: mock(() => []), - buildTool: mock((def: any) => def), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/__tests__/agent.test.ts` around lines 23 - 27, Remove the unused Tool.js mocks: delete the mock entries for toolMatchesName, findToolByName, filterToolProgressMessages, and buildTool from the test setup so only getEmptyToolPermissionContext and the Tools type remain mocked as in agent.ts; if you must keep buildTool for future use, replace the identity stub (buildTool) with a mock that mirrors the real implementation by spreading TOOL_DEFAULTS into the returned tool to avoid returning incomplete tool objects (refer to symbols toolMatchesName, findToolByName, filterToolProgressMessages, buildTool, getEmptyToolPermissionContext, Tools, and TOOL_DEFAULTS).packages/builtin-tools/src/tools/WebSearchTool/__tests__/bingAdapter.test.ts (1)
3-10: Consider mirroring the explanatory comment frombraveAdapter.test.ts.The sibling file
braveAdapter.test.ts(lines 3-5) explains why the defensivesrc/utils/errorsmock is required (agent.test.ts mockingconfig.jscan corrupt Bun'ssrc/*path alias resolution). Adding an equivalent comment here would help future maintainers understand this isn't a dead/duplicate mock and avoid accidental removal.📝 Proposed comment
+// Defensive mock: agent.test.ts mocks config.js which can corrupt Bun's +// src/* path alias resolution. Provide AbortError directly so the dynamic +// import in the adapter never needs to resolve the alias at runtime. const _abortMock = () => ({ AbortError: class AbortError extends Error { constructor(message?: string) { super(message); this.name = 'AbortError' } }, isAbortError: (e: unknown) => e instanceof Error && (e as Error).name === 'AbortError', }) mock.module('src/utils/errors.js', _abortMock) mock.module('src/utils/errors', _abortMock)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/builtin-tools/src/tools/WebSearchTool/__tests__/bingAdapter.test.ts` around lines 3 - 10, Add the same explanatory comment found in braveAdapter.test.ts above the defensive mock for src/utils/errors (the _abortMock and the two mock.module calls) explaining that agent.test.ts can mock config.js and corrupt Bun's src/* path alias resolution, so this duplicate defensive mock is required to prevent module resolution breaks; place the comment immediately before the _abortMock definition and the two mock.module(...) lines so future maintainers understand it is intentional, not dead code.packages/@ant/model-provider/src/types/message.ts (2)
82-82: Unused generic parameter.
NormalizedAssistantMessage<T = unknown>declaresTbut never uses it — it just aliasesAssistantMessage. Either drop the parameter or wire it into a narrowedcontentshape. As it stands it's a footgun for callers who think they're specializing the type.-export type NormalizedAssistantMessage<T = unknown> = AssistantMessage +export type NormalizedAssistantMessage = AssistantMessage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/types/message.ts at line 82, The type alias NormalizedAssistantMessage<T = unknown> declares an unused generic T; either drop the generic or make T control the message content. Fix by either (A) remove the generic and export type NormalizedAssistantMessage = AssistantMessage so callers can't mistakenly specialize T, or (B) wire T into the shape — replace the alias with a mapped type that preserves AssistantMessage except narrows the content field (e.g., Omit<AssistantMessage,'content'> & { content: T }) so NormalizedAssistantMessage<T> actually constrains the content type; update the exported type accordingly (reference: NormalizedAssistantMessage and AssistantMessage).
80-101: Manytype: 'system'subtypes are structurally identical.
SystemAPIErrorMessage,SystemFileSnapshotMessage,SystemAgentsKilledMessage,SystemApiMetricsMessage,SystemAwaySummaryMessage,SystemBridgeStatusMessage,SystemInformationalMessage,SystemMemorySavedMessage,SystemMicrocompactBoundaryMessage,SystemPermissionRetryMessage,SystemScheduledTaskFireMessage,SystemTurnDurationMessage,SystemThinkingMessageall resolve toMessage & { type: 'system' }with no distinguishing field. That means TypeScript cannot narrow between them — e.g.,msg: SystemMessageis assignable to every one of these, and code thatswitches on a discriminant beyond'system'will silently fail to narrow.If the migration intends to preserve the semantic distinctions from
src/types/message.ts, consider adding asubtypediscriminant (several already usesubtype: string). If these are intentional placeholders for the refactor, leaving a TODO marker here would help future readers. Non-blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/types/message.ts around lines 80 - 101, Several system message type aliases (e.g., SystemAPIErrorMessage, SystemFileSnapshotMessage, SystemAgentsKilledMessage, SystemApiMetricsMessage, SystemAwaySummaryMessage, SystemBridgeStatusMessage, SystemInformationalMessage, SystemMemorySavedMessage, SystemMicrocompactBoundaryMessage, SystemPermissionRetryMessage, SystemScheduledTaskFireMessage, SystemTurnDurationMessage, SystemThinkingMessage) are currently identical as Message & { type: 'system' }, which prevents TypeScript from discriminating between them; update each of these type aliases to include a unique discriminant (for example add a subtype: 'api_error' | 'file_snapshot' | 'agents_killed' etc. or a string literal per alias) so callers can narrow on msg.subtype, or if the sameness is intentional add a clear TODO comment on each alias indicating they are placeholders and why, and ensure the discriminant name matches existing patterns (e.g., subtype) used elsewhere in the file (refer to StopHookInfo and other types that use subtype).packages/@ant/model-provider/src/client/index.ts (1)
9-11: Consider guarding against duplicate registration.
registerClientFactoriessilently overwrites a prior registration. In app code that's fine, but in a test harness (or any host that initializes twice) this can let stale factories be replaced without signal. A cheap dev-mode guard (if (registeredFactories && process.env.NODE_ENV !== 'test') log/throw) would make accidental double-init visible. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/client/index.ts around lines 9 - 11, registerClientFactories currently overwrites a previous registration silently; add a guard in the function (registerClientFactories) that checks the module-level variable registeredFactories and, if it is already set and process.env.NODE_ENV !== 'test', emits a visible signal (e.g., processLogger.warn or throw an Error) instead of silently replacing it; keep behavior unchanged for tests by skipping the guard when NODE_ENV === 'test' and ensure the message names the function/variable (registerClientFactories / registeredFactories) so the origin is clear.packages/@ant/model-provider/src/hooks/types.ts (1)
8-48: Interface shape is reasonable; consider type parameters for better ergonomics.Using
Record<string, unknown>everywhere erases structure that callers almost always need (e.g.reportCost.usageis effectively always aBetaUsage). Since the whole point of this package boundary is to avoid depending on main-project types, this is defensible — but exposing a couple of generic type parameters (e.g.ModelProviderHooks<Usage = Record<string, unknown>, PermCtx = Record<string, unknown>>) would let the main project re-narrow withoutascasts at every call site.Not a blocker — mention for future iteration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/hooks/types.ts around lines 8 - 48, Add generic type parameters to ModelProviderHooks so callers can supply narrower types while preserving defaults; e.g. change the interface declaration to something like ModelProviderHooks<Usage = Record<string, unknown>, PermCtx = Record<string, unknown>, Notification = Record<string, unknown>, OauthInfo = Record<string, unknown>> and then update usage sites of reportCost.usage, getToolPermissionContext, addNotification, and getOauthAccountInfo to use those generic types (e.g., reportCost: (params: { usage: Usage; ... }), getToolPermissionContext: () => Promise<PermCtx>, addNotification?: (notification: Notification) => void, getOauthAccountInfo?: () => OauthInfo | undefined) so consumers can opt into stronger types without breaking defaults.packages/@ant/model-provider/src/hooks/index.ts (1)
3-25: Consider exposing a test/reset helper and guarding against accidental re-registration.The module-level
registeredHookssingleton is pragmatic, but two gaps worth addressing:
- Tests cannot reset state between runs — they must mock the module or leak state across test files.
registerHookssilently overwrites any previously registered hooks; a second caller (e.g. a bundled sub-app or a test) won't know they just clobbered production wiring.♻️ Suggested addition
export function registerHooks(hooks: ModelProviderHooks): void { + if (registeredHooks && process.env.NODE_ENV !== 'test') { + // Second registration in a running app is almost certainly a bug. + throw new Error('ModelProvider hooks already registered.') + } registeredHooks = hooks } + +/** Test-only: reset the registry. */ +export function resetHooks(): void { + registeredHooks = null +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/hooks/index.ts around lines 3 - 25, The module currently uses a module-level singleton (registeredHooks) that cannot be reset for tests and allows silent re-registration; add a test/reset helper (e.g., resetHooks or unregisterHooks) that clears registeredHooks for test isolation and modify registerHooks to guard against accidental re-registration by either throwing an error or returning a boolean/no-op if registeredHooks is already set; update getHooks to remain fail-fast by reading registeredHooks. Reference the existing symbols registeredHooks, registerHooks, and getHooks when making these changes.packages/@ant/model-provider/src/providers/gemini/convertMessages.ts (1)
14-22: LocalsafeParseJSONconflates parse errors with the literal"null".Both a JSON parse failure and a valid input of
"null"returnnull. Downstream,normalizeToolUseInput(line 244) treatsparsed === nullas the "parse failed, use empty object" path, so a valid JSON"null"input will be coerced to{}instead of{ value: null }. Low-severity edge case, but worth documenting or using a sentinel (e.g.undefinedon error) to disambiguate.♻️ Suggested disambiguation
-function safeParseJSON(json: string | null | undefined): unknown { - if (!json) return null - try { - return JSON.parse(json) - } catch { - return null - } -} +function safeParseJSON(json: string | null | undefined): unknown { + if (!json) return undefined + try { + return JSON.parse(json) + } catch { + return undefined + } +}Then adjust the
parsed === nullcheck at line 244 toparsed === undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@ant/model-provider/src/providers/gemini/convertMessages.ts around lines 14 - 22, The local safeParseJSON conflates a valid JSON "null" with a parse error by returning null for both; change safeParseJSON so it returns undefined on parse errors (preserve null for literal "null"), then update normalizeToolUseInput to treat parsed === undefined as the "parse failed, use empty object" branch instead of parsed === null; reference functions: safeParseJSON and normalizeToolUseInput.src/services/api/openai/requestBody.ts (1)
43-51:parseInt(...) || undefinedsilently drops0and accepts malformed input.Two edge cases:
OPENAI_MAX_TOKENS=0(or"0abc"→0) is coerced toundefinedvia|| undefined, falling through to the next override instead of surfacing as an invalid configuration. Likely harmless for max_tokens (0 is meaningless), but the pattern is worth noting.parseInt("64000abc", 10)returns64000— malformed env values are silently accepted. ConsiderNumber()+Number.isFinite+> 0validation, and log a warning on invalid input so misconfiguration is visible rather than silently ignored.♻️ Suggested hardening
+function parsePositiveInt(name: string, raw: string | undefined): number | undefined { + if (raw == null || raw === '') return undefined + const n = Number(raw) + if (!Number.isFinite(n) || !Number.isInteger(n) || n <= 0) { + // eslint-disable-next-line no-console + console.warn(`[OpenAI] Invalid ${name}=${raw}; ignoring.`) + return undefined + } + return n +} + export function resolveOpenAIMaxTokens( upperLimit: number, maxOutputTokensOverride?: number, ): number { return maxOutputTokensOverride - ?? (process.env.OPENAI_MAX_TOKENS ? parseInt(process.env.OPENAI_MAX_TOKENS, 10) || undefined : undefined) - ?? (process.env.CLAUDE_CODE_MAX_OUTPUT_TOKENS ? parseInt(process.env.CLAUDE_CODE_MAX_OUTPUT_TOKENS, 10) || undefined : undefined) + ?? parsePositiveInt('OPENAI_MAX_TOKENS', process.env.OPENAI_MAX_TOKENS) + ?? parsePositiveInt('CLAUDE_CODE_MAX_OUTPUT_TOKENS', process.env.CLAUDE_CODE_MAX_OUTPUT_TOKENS) ?? upperLimit }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/openai/requestBody.ts` around lines 43 - 51, In resolveOpenAIMaxTokens, replace the current parseInt(... ) || undefined pattern with robust parsing and validation: read OPENAI_MAX_TOKENS and CLAUDE_CODE_MAX_OUTPUT_TOKENS using Number(...) (or Number.parseInt then Number.isFinite), check Number.isFinite(value) and value > 0 (treat 0 as invalid), and only accept the parsed value when valid; if an env value is malformed or invalid, emit a warning (e.g., processLogger.warn or the project logger) naming the env var and the raw value, then fall through to the next override or upperLimit; ensure the function still returns the first valid override or upperLimit.src/services/api/openai/index.ts (1)
26-27: Collapse the import + re-export into a singleexport { ... } from.Minor — avoids the double reference and keeps this module's public surface obvious.
♻️ Suggested refactor
-import { isOpenAIThinkingEnabled, resolveOpenAIMaxTokens, buildOpenAIRequestBody } from './requestBody.js' -export { isOpenAIThinkingEnabled, resolveOpenAIMaxTokens, buildOpenAIRequestBody } +export { + isOpenAIThinkingEnabled, + resolveOpenAIMaxTokens, + buildOpenAIRequestBody, +} from './requestBody.js' +import { isOpenAIThinkingEnabled, resolveOpenAIMaxTokens, buildOpenAIRequestBody } from './requestBody.js'(Keep the second
importonly if these symbols are still used inside this file — they are, at lines 176, 215, 229.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/openai/index.ts` around lines 26 - 27, Collapse the paired import+re-export into a single export-from statement (export { isOpenAIThinkingEnabled, resolveOpenAIMaxTokens, buildOpenAIRequestBody } from './requestBody.js') only if those symbols are not referenced elsewhere in this file; if they are used locally (they are referenced later in this module), keep the current local import for use and remove the redundant separate re-export or leave the existing export pattern as-is so local bindings remain available for functions that call isOpenAIThinkingEnabled, resolveOpenAIMaxTokens, and buildOpenAIRequestBody.src/services/api/errorUtils.ts (1)
1-8: Delete this dead code file. The file is a pure re-export shim with no remaining importers in the codebase. All call-sites have already migrated to import directly from@ant/model-provider, making this shim unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/errorUtils.ts` around lines 1 - 8, Remove this dead re-export shim by deleting the module that only re-exports formatAPIError, extractConnectionErrorDetails, sanitizeAPIError, getSSLErrorHint, and the ConnectionErrorDetails type; ensure there are no remaining imports in the codebase (all call-sites should import directly from `@ant/model-provider`) and also remove any references to this file from project barrels/exports or build manifests so it is no longer published or included.src/types/message.ts (1)
53-69: Consolidate duplicate@ant/model-providerimports and drop inlineimport(...)types.
MessageandGroupedToolUseMessageare already re-exported above, yetRenderableMessageuses inlineimport('@ant/model-provider').Xtypes (Lines 66–69) while other shapes are brought in via a staticimport typeon Lines 53–59.StopHookInfois then imported separately on Line 61. Merge them into a single staticimport typefor consistency and readability.♻️ Proposed refactor
import type { AssistantMessage, CollapsibleMessage, + GroupedToolUseMessage, + Message, NormalizedAssistantMessage, NormalizedUserMessage, + StopHookInfo, UserMessage, } from '@ant/model-provider' import type { UUID } from 'crypto' -import type { StopHookInfo } from '@ant/model-provider' export type RenderableMessage = | AssistantMessage | UserMessage - | (import('@ant/model-provider').Message & { type: 'system' }) - | (import('@ant/model-provider').Message & { type: 'attachment'; attachment: { type: string; memories?: { path: string; content: string; mtimeMs: number }[]; [key: string]: unknown } }) - | (import('@ant/model-provider').Message & { type: 'progress' }) - | import('@ant/model-provider').GroupedToolUseMessage + | (Message & { type: 'system' }) + | (Message & { type: 'attachment'; attachment: { type: string; memories?: { path: string; content: string; mtimeMs: number }[]; [key: string]: unknown } }) + | (Message & { type: 'progress' }) + | GroupedToolUseMessage | CollapsedReadSearchGroup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/message.ts` around lines 53 - 69, The RenderableMessage union uses inline import(...) types for Message and GroupedToolUseMessage; consolidate these into the top static import list: add Message and GroupedToolUseMessage (and StopHookInfo if not already grouped) to the existing import type block that currently imports AssistantMessage, CollapsibleMessage, NormalizedAssistantMessage, NormalizedUserMessage, UserMessage from '@ant/model-provider', then replace the inline import('@ant/model-provider').Message and import('@ant/model-provider').GroupedToolUseMessage usages inside the RenderableMessage union with the statically imported Message and GroupedToolUseMessage types and remove the inline import(...) occurrences so all types from '@ant/model-provider' are imported consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/`@ant/model-provider/src/errorUtils.ts:
- Around line 226-237: sanitizeAPIError can return an empty string when
sanitizeMessageHTML stripped HTML with no <title>, but the current logic falls
back to error.message (raw HTML). Change the return logic in the block using
sanitizeAPIError (and similarly where extractNestedErrorMessage calls
sanitizeMessageHTML) so that an empty sanitizedMessage is treated as "sanitized
to empty" and you return a generic fallback like `API error (status
${error.status ?? 'unknown'})` instead of error.message; only return
error.message when no sanitization occurred (e.g., sanitizedMessage is
undefined/null) or when sanitizedMessage === error.message.
- Around line 103-112: sanitizeMessageHTML currently does case-sensitive checks
for '<!DOCTYPE html' and '<html' which misses lowercase/HTML5 variants; change
the detection to be case-insensitive (either by lowercasing message once and
using includes('<!doctype html') / includes('<html') or by using
case-insensitive regex like /<!doctype\s+html/i and /<html\b/i) and also make
the title extraction regex case-insensitive (use /<title>([^<]+)<\/title>/i) so
titleMatch works regardless of tag casing; keep the existing behavior of
returning the trimmed title or '' when none found and ensure you reference the
sanitizeMessageHTML function and its titleMatch logic when making the change.
In
`@packages/builtin-tools/src/tools/WebSearchTool/__tests__/braveAdapter.test.ts`:
- Around line 6-13: The test currently defines AbortError inside the _abortMock
factory causing two distinct class identities when calling
mock.module('src/utils/errors.js', _abortMock) and
mock.module('src/utils/errors', _abortMock); hoist a single AbortError class
declaration outside the _abortMock factory (e.g. const abortErrorClass = class
AbortError extends Error { ... }) and have _abortMock return that class
(AbortError: abortErrorClass) and the same isAbortError implementation so both
mock.module calls return the same class identity; apply the same change in the
corresponding bingAdapter.test.ts to ensure
instanceof/rejects.toThrow(AbortError) comparisons succeed.
In `@src/services/api/openai/requestBody.ts`:
- Around line 64-76: The function buildOpenAIRequestBody currently accepts
messages: any[], tools: any[], and toolChoice: any — replace those any types
with concrete OpenAI SDK types (e.g., use the SDK's
ChatCompletionRequestMessage[] for messages and the appropriate
Tool/ToolCollection type for tools and toolChoice imported from
openai/resources/chat/completions/completions.mjs); if the SDK types are not
directly compatible, use Record<string, unknown>[] or Record<string, unknown>
for tools/toolChoice and narrow with as unknown as <SDKType> (double assertion)
to satisfy the coding guideline against any, and update the function signature
and any callsites that construct messages/tools/toolChoice to match the new
types (refer to buildOpenAIRequestBody, parameter names messages, tools,
toolChoice).
---
Outside diff comments:
In `@src/services/api/openai/index.ts`:
- Line 243: The inline step comment "// 12. Convert OpenAI stream to Anthropic
events, then process into" is misnumbered (duplicating an earlier "// 12.");
update that comment to "// 13." and then renumber any subsequent step-numbered
inline comments accordingly so the sequence is correct; locate the comment
string and adjust the numbering in the surrounding inline blocks to maintain
increasing step numbers.
---
Nitpick comments:
In `@packages/`@ant/model-provider/src/client/index.ts:
- Around line 9-11: registerClientFactories currently overwrites a previous
registration silently; add a guard in the function (registerClientFactories)
that checks the module-level variable registeredFactories and, if it is already
set and process.env.NODE_ENV !== 'test', emits a visible signal (e.g.,
processLogger.warn or throw an Error) instead of silently replacing it; keep
behavior unchanged for tests by skipping the guard when NODE_ENV === 'test' and
ensure the message names the function/variable (registerClientFactories /
registeredFactories) so the origin is clear.
In `@packages/`@ant/model-provider/src/hooks/index.ts:
- Around line 3-25: The module currently uses a module-level singleton
(registeredHooks) that cannot be reset for tests and allows silent
re-registration; add a test/reset helper (e.g., resetHooks or unregisterHooks)
that clears registeredHooks for test isolation and modify registerHooks to guard
against accidental re-registration by either throwing an error or returning a
boolean/no-op if registeredHooks is already set; update getHooks to remain
fail-fast by reading registeredHooks. Reference the existing symbols
registeredHooks, registerHooks, and getHooks when making these changes.
In `@packages/`@ant/model-provider/src/hooks/types.ts:
- Around line 8-48: Add generic type parameters to ModelProviderHooks so callers
can supply narrower types while preserving defaults; e.g. change the interface
declaration to something like ModelProviderHooks<Usage = Record<string,
unknown>, PermCtx = Record<string, unknown>, Notification = Record<string,
unknown>, OauthInfo = Record<string, unknown>> and then update usage sites of
reportCost.usage, getToolPermissionContext, addNotification, and
getOauthAccountInfo to use those generic types (e.g., reportCost: (params: {
usage: Usage; ... }), getToolPermissionContext: () => Promise<PermCtx>,
addNotification?: (notification: Notification) => void, getOauthAccountInfo?: ()
=> OauthInfo | undefined) so consumers can opt into stronger types without
breaking defaults.
In `@packages/`@ant/model-provider/src/providers/gemini/convertMessages.ts:
- Around line 14-22: The local safeParseJSON conflates a valid JSON "null" with
a parse error by returning null for both; change safeParseJSON so it returns
undefined on parse errors (preserve null for literal "null"), then update
normalizeToolUseInput to treat parsed === undefined as the "parse failed, use
empty object" branch instead of parsed === null; reference functions:
safeParseJSON and normalizeToolUseInput.
In `@packages/`@ant/model-provider/src/types/message.ts:
- Line 82: The type alias NormalizedAssistantMessage<T = unknown> declares an
unused generic T; either drop the generic or make T control the message content.
Fix by either (A) remove the generic and export type NormalizedAssistantMessage
= AssistantMessage so callers can't mistakenly specialize T, or (B) wire T into
the shape — replace the alias with a mapped type that preserves AssistantMessage
except narrows the content field (e.g., Omit<AssistantMessage,'content'> & {
content: T }) so NormalizedAssistantMessage<T> actually constrains the content
type; update the exported type accordingly (reference:
NormalizedAssistantMessage and AssistantMessage).
- Around line 80-101: Several system message type aliases (e.g.,
SystemAPIErrorMessage, SystemFileSnapshotMessage, SystemAgentsKilledMessage,
SystemApiMetricsMessage, SystemAwaySummaryMessage, SystemBridgeStatusMessage,
SystemInformationalMessage, SystemMemorySavedMessage,
SystemMicrocompactBoundaryMessage, SystemPermissionRetryMessage,
SystemScheduledTaskFireMessage, SystemTurnDurationMessage,
SystemThinkingMessage) are currently identical as Message & { type: 'system' },
which prevents TypeScript from discriminating between them; update each of these
type aliases to include a unique discriminant (for example add a subtype:
'api_error' | 'file_snapshot' | 'agents_killed' etc. or a string literal per
alias) so callers can narrow on msg.subtype, or if the sameness is intentional
add a clear TODO comment on each alias indicating they are placeholders and why,
and ensure the discriminant name matches existing patterns (e.g., subtype) used
elsewhere in the file (refer to StopHookInfo and other types that use subtype).
In `@packages/builtin-tools/src/tools/PowerShellTool/__tests__/gitSafety.test.ts`:
- Around line 11-20: The mock in the test should only export what the test
actually uses: replace the oversized mock.module stub with a minimal mock that
exports only PS_TOKENIZER_DASH_CHARS (the constant imported by gitSafety.ts) so
remove the unused stubs (COMMON_ALIASES, commandHasArgAbbreviation,
deriveSecurityFlags, getAllCommands, getVariablesByScope, hasCommandNamed,
parsePowerShellCommandCached); update the mock.module call in the test to return
just PS_TOKENIZER_DASH_CHARS as a Set of dash chars to reduce noise and avoid
drift.
In
`@packages/builtin-tools/src/tools/WebSearchTool/__tests__/bingAdapter.test.ts`:
- Around line 3-10: Add the same explanatory comment found in
braveAdapter.test.ts above the defensive mock for src/utils/errors (the
_abortMock and the two mock.module calls) explaining that agent.test.ts can mock
config.js and corrupt Bun's src/* path alias resolution, so this duplicate
defensive mock is required to prevent module resolution breaks; place the
comment immediately before the _abortMock definition and the two
mock.module(...) lines so future maintainers understand it is intentional, not
dead code.
In `@src/services/acp/__tests__/agent.test.ts`:
- Around line 33-38: Add a clearer comment or consolidate duplicate mocks:
explain why mock.module('src/utils/config.js') is required (reference the Bun
src/* alias resolution bug or link to its issue if known) and indicate whether
the relative-path mock in agent.test.ts can be removed; alternatively, remove
the relative-path mock and keep only the alias mock for 'src/utils/config.js'
and ensure enableConfigs is still stubbed via mock(() => {}) so tests remain
isolated.
- Around line 23-27: Remove the unused Tool.js mocks: delete the mock entries
for toolMatchesName, findToolByName, filterToolProgressMessages, and buildTool
from the test setup so only getEmptyToolPermissionContext and the Tools type
remain mocked as in agent.ts; if you must keep buildTool for future use, replace
the identity stub (buildTool) with a mock that mirrors the real implementation
by spreading TOOL_DEFAULTS into the returned tool to avoid returning incomplete
tool objects (refer to symbols toolMatchesName, findToolByName,
filterToolProgressMessages, buildTool, getEmptyToolPermissionContext, Tools, and
TOOL_DEFAULTS).
In `@src/services/api/errorUtils.ts`:
- Around line 1-8: Remove this dead re-export shim by deleting the module that
only re-exports formatAPIError, extractConnectionErrorDetails, sanitizeAPIError,
getSSLErrorHint, and the ConnectionErrorDetails type; ensure there are no
remaining imports in the codebase (all call-sites should import directly from
`@ant/model-provider`) and also remove any references to this file from project
barrels/exports or build manifests so it is no longer published or included.
In `@src/services/api/openai/index.ts`:
- Around line 26-27: Collapse the paired import+re-export into a single
export-from statement (export { isOpenAIThinkingEnabled, resolveOpenAIMaxTokens,
buildOpenAIRequestBody } from './requestBody.js') only if those symbols are not
referenced elsewhere in this file; if they are used locally (they are referenced
later in this module), keep the current local import for use and remove the
redundant separate re-export or leave the existing export pattern as-is so local
bindings remain available for functions that call isOpenAIThinkingEnabled,
resolveOpenAIMaxTokens, and buildOpenAIRequestBody.
In `@src/services/api/openai/requestBody.ts`:
- Around line 43-51: In resolveOpenAIMaxTokens, replace the current parseInt(...
) || undefined pattern with robust parsing and validation: read
OPENAI_MAX_TOKENS and CLAUDE_CODE_MAX_OUTPUT_TOKENS using Number(...) (or
Number.parseInt then Number.isFinite), check Number.isFinite(value) and value >
0 (treat 0 as invalid), and only accept the parsed value when valid; if an env
value is malformed or invalid, emit a warning (e.g., processLogger.warn or the
project logger) naming the env var and the raw value, then fall through to the
next override or upperLimit; ensure the function still returns the first valid
override or upperLimit.
In `@src/types/message.ts`:
- Around line 53-69: The RenderableMessage union uses inline import(...) types
for Message and GroupedToolUseMessage; consolidate these into the top static
import list: add Message and GroupedToolUseMessage (and StopHookInfo if not
already grouped) to the existing import type block that currently imports
AssistantMessage, CollapsibleMessage, NormalizedAssistantMessage,
NormalizedUserMessage, UserMessage from '@ant/model-provider', then replace the
inline import('@ant/model-provider').Message and
import('@ant/model-provider').GroupedToolUseMessage usages inside the
RenderableMessage union with the statically imported Message and
GroupedToolUseMessage types and remove the inline import(...) occurrences so all
types from '@ant/model-provider' are imported consistently.
🪄 Autofix (Beta)
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
Run ID: 8bf5643d-c2e8-44ce-9c83-3a4cfe819464
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
package.jsonpackages/@ant/model-provider/package.jsonpackages/@ant/model-provider/src/client/index.tspackages/@ant/model-provider/src/client/types.tspackages/@ant/model-provider/src/errorUtils.tspackages/@ant/model-provider/src/hooks/index.tspackages/@ant/model-provider/src/hooks/types.tspackages/@ant/model-provider/src/index.tspackages/@ant/model-provider/src/providers/gemini/__tests__/convertMessages.test.tspackages/@ant/model-provider/src/providers/gemini/__tests__/convertTools.test.tspackages/@ant/model-provider/src/providers/gemini/__tests__/modelMapping.test.tspackages/@ant/model-provider/src/providers/gemini/__tests__/streamAdapter.test.tspackages/@ant/model-provider/src/providers/gemini/convertMessages.tspackages/@ant/model-provider/src/providers/gemini/convertTools.tspackages/@ant/model-provider/src/providers/gemini/modelMapping.tspackages/@ant/model-provider/src/providers/gemini/streamAdapter.tspackages/@ant/model-provider/src/providers/gemini/types.tspackages/@ant/model-provider/src/providers/grok/__tests__/modelMapping.test.tspackages/@ant/model-provider/src/providers/grok/modelMapping.tspackages/@ant/model-provider/src/providers/openai/__tests__/modelMapping.test.tspackages/@ant/model-provider/src/providers/openai/modelMapping.tspackages/@ant/model-provider/src/shared/__tests__/openaiConvertMessages.test.tspackages/@ant/model-provider/src/shared/__tests__/openaiConvertTools.test.tspackages/@ant/model-provider/src/shared/__tests__/openaiStreamAdapter.test.tspackages/@ant/model-provider/src/shared/openaiConvertMessages.tspackages/@ant/model-provider/src/shared/openaiConvertTools.tspackages/@ant/model-provider/src/shared/openaiStreamAdapter.tspackages/@ant/model-provider/src/types/errors.tspackages/@ant/model-provider/src/types/index.tspackages/@ant/model-provider/src/types/message.tspackages/@ant/model-provider/src/types/systemPrompt.tspackages/@ant/model-provider/src/types/usage.tspackages/@ant/model-provider/tsconfig.jsonpackages/builtin-tools/src/tools/PowerShellTool/__tests__/gitSafety.test.tspackages/builtin-tools/src/tools/PowerShellTool/__tests__/powershellSecurity.test.tspackages/builtin-tools/src/tools/WebSearchTool/__tests__/adapterFactory.test.tspackages/builtin-tools/src/tools/WebSearchTool/__tests__/bingAdapter.test.tspackages/builtin-tools/src/tools/WebSearchTool/__tests__/braveAdapter.test.tssrc/QueryEngine.tssrc/bridge/bridgeMessaging.tssrc/cli/handlers/auth.tssrc/cli/print.tssrc/components/ConsoleOAuthFlow.tsxsrc/components/messages/SystemAPIErrorMessage.tsxsrc/entrypoints/sdk/sdkUtilityTypes.tssrc/services/acp/__tests__/agent.test.tssrc/services/api/emptyUsage.tssrc/services/api/errorUtils.tssrc/services/api/gemini/client.tssrc/services/api/gemini/index.tssrc/services/api/grok/__tests__/client.test.tssrc/services/api/grok/index.tssrc/services/api/openai/__tests__/queryModelOpenAI.isolated.tssrc/services/api/openai/__tests__/queryModelOpenAI.test.tssrc/services/api/openai/__tests__/thinking.test.tssrc/services/api/openai/index.tssrc/services/api/openai/requestBody.tssrc/types/message.tssrc/utils/forkedAgent.tssrc/utils/sideQuestion.tssrc/utils/systemPromptType.ts
💤 Files with no reviewable changes (4)
- packages/@ant/model-provider/src/providers/gemini/modelMapping.ts
- packages/@ant/model-provider/src/providers/openai/modelMapping.ts
- src/services/api/openai/tests/queryModelOpenAI.test.ts
- src/services/api/openai/tests/queryModelOpenAI.isolated.ts
✅ Files skipped from review due to trivial changes (16)
- src/services/api/gemini/index.ts
- packages/@ant/model-provider/src/shared/tests/openaiConvertTools.test.ts
- packages/@ant/model-provider/src/shared/openaiConvertMessages.ts
- src/services/api/gemini/client.ts
- packages/@ant/model-provider/src/providers/grok/modelMapping.ts
- packages/@ant/model-provider/src/providers/gemini/tests/convertMessages.test.ts
- package.json
- src/services/api/openai/tests/thinking.test.ts
- src/cli/handlers/auth.ts
- src/QueryEngine.ts
- packages/@ant/model-provider/tsconfig.json
- src/bridge/bridgeMessaging.ts
- src/cli/print.ts
- packages/@ant/model-provider/package.json
- packages/@ant/model-provider/src/client/types.ts
- packages/@ant/model-provider/src/types/usage.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/utils/sideQuestion.ts
- src/entrypoints/sdk/sdkUtilityTypes.ts
- src/components/messages/SystemAPIErrorMessage.tsx
- src/utils/systemPromptType.ts
- src/services/api/emptyUsage.ts
- src/utils/forkedAgent.ts
Comment on lines
+103
to
+112
| function sanitizeMessageHTML(message: string): string { | ||
| if (message.includes('<!DOCTYPE html') || message.includes('<html')) { | ||
| const titleMatch = message.match(/<title>([^<]+)<\/title>/) | ||
| if (titleMatch && titleMatch[1]) { | ||
| return titleMatch[1].trim() | ||
| } | ||
| return '' | ||
| } | ||
| return message | ||
| } |
Contributor
There was a problem hiding this comment.
Case-sensitive HTML detection misses HTML5 lowercase doctype.
<!DOCTYPE html and <html are matched literally, but HTML5 responses (and many CDN/error pages) emit lowercase <!doctype html> or <HTML ...>. A CloudFlare/Nginx error page served with lowercase markup will slip past this guard and be surfaced verbatim via formatAPIError, exactly the case this helper was added to prevent.
🛡️ Proposed fix
function sanitizeMessageHTML(message: string): string {
- if (message.includes('<!DOCTYPE html') || message.includes('<html')) {
- const titleMatch = message.match(/<title>([^<]+)<\/title>/)
+ if (/<!doctype html|<html[\s>]/i.test(message)) {
+ const titleMatch = message.match(/<title>([^<]+)<\/title>/i)
if (titleMatch && titleMatch[1]) {
return titleMatch[1].trim()
}
return ''
}
return message
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@ant/model-provider/src/errorUtils.ts around lines 103 - 112,
sanitizeMessageHTML currently does case-sensitive checks for '<!DOCTYPE html'
and '<html' which misses lowercase/HTML5 variants; change the detection to be
case-insensitive (either by lowercasing message once and using
includes('<!doctype html') / includes('<html') or by using case-insensitive
regex like /<!doctype\s+html/i and /<html\b/i) and also make the title
extraction regex case-insensitive (use /<title>([^<]+)<\/title>/i) so titleMatch
works regardless of tag casing; keep the existing behavior of returning the
trimmed title or '' when none found and ensure you reference the
sanitizeMessageHTML function and its titleMatch logic when making the change.
Comment on lines
+226
to
+237
| if (!error.message) { | ||
| return ( | ||
| extractNestedErrorMessage(error) ?? | ||
| `API error (status ${error.status ?? 'unknown'})` | ||
| ) | ||
| } | ||
|
|
||
| const sanitizedMessage = sanitizeAPIError(error) | ||
| // Use sanitized message if it's different from the original (i.e., HTML was sanitized) | ||
| return sanitizedMessage !== error.message && sanitizedMessage.length > 0 | ||
| ? sanitizedMessage | ||
| : error.message |
Contributor
There was a problem hiding this comment.
Fallback path re-exposes raw HTML to the user.
When sanitizeMessageHTML detects HTML but finds no <title>, it returns ''. Here the guard sanitizedMessage.length > 0 then falls back to error.message — which is the raw HTML payload. The user ends up seeing the full CloudFlare/Nginx page in the terminal, defeating the sanitizer.
Consider distinguishing "was sanitized to empty" from "no sanitization performed" and returning a generic fallback (e.g., `API error (status ${error.status ?? 'unknown'})`) in the empty case. The same applies to extractNestedErrorMessage's use of sanitizeMessageHTML which already guards sanitized.length > 0.
♻️ Sketch
- const sanitizedMessage = sanitizeAPIError(error)
- // Use sanitized message if it's different from the original (i.e., HTML was sanitized)
- return sanitizedMessage !== error.message && sanitizedMessage.length > 0
- ? sanitizedMessage
- : error.message
+ const sanitizedMessage = sanitizeAPIError(error)
+ if (sanitizedMessage === error.message) return error.message
+ if (sanitizedMessage.length > 0) return sanitizedMessage
+ // HTML was stripped but nothing useful remained — avoid surfacing raw HTML.
+ return `API error (status ${error.status ?? 'unknown'})`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@ant/model-provider/src/errorUtils.ts around lines 226 - 237,
sanitizeAPIError can return an empty string when sanitizeMessageHTML stripped
HTML with no <title>, but the current logic falls back to error.message (raw
HTML). Change the return logic in the block using sanitizeAPIError (and
similarly where extractNestedErrorMessage calls sanitizeMessageHTML) so that an
empty sanitizedMessage is treated as "sanitized to empty" and you return a
generic fallback like `API error (status ${error.status ?? 'unknown'})` instead
of error.message; only return error.message when no sanitization occurred (e.g.,
sanitizedMessage is undefined/null) or when sanitizedMessage === error.message.
Comment on lines
+6
to
+13
| const _abortMock = () => ({ | ||
| AbortError: class AbortError extends Error { | ||
| constructor(message?: string) { super(message); this.name = 'AbortError' } | ||
| }, | ||
| isAbortError: (e: unknown) => e instanceof Error && (e as Error).name === 'AbortError', | ||
| }) | ||
| mock.module('src/utils/errors.js', _abortMock) | ||
| mock.module('src/utils/errors', _abortMock) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify both specifiers resolve to the same mock and check adapter's import specifier.
rg -nP -C2 "from\s+['\"](src/utils/errors(\.js)?)['\"]" packages/builtin-tools/src/tools/WebSearchTool/adapters/Repository: claude-code-best/claude-code
Length of output: 1143
🏁 Script executed:
cat -n packages/builtin-tools/src/tools/WebSearchTool/__tests__/braveAdapter.test.ts | head -220 | tail -150Repository: claude-code-best/claude-code
Length of output: 5257
🏁 Script executed:
rg -nP "(import.*AbortError|rejects\.toThrow)" packages/builtin-tools/src/tools/WebSearchTool/__tests__/braveAdapter.test.tsRepository: claude-code-best/claude-code
Length of output: 257
🏁 Script executed:
cat -n packages/builtin-tools/src/tools/WebSearchTool/__tests__/braveAdapter.test.ts | head -50Repository: claude-code-best/claude-code
Length of output: 2000
🏁 Script executed:
cat -n packages/builtin-tools/src/tools/WebSearchTool/adapters/braveAdapter.ts | grep -A 15 -B 5 "AbortError"Repository: claude-code-best/claude-code
Length of output: 2422
🏁 Script executed:
head -20 packages/builtin-tools/src/tools/WebSearchTool/__tests__/bingAdapter.test.tsRepository: claude-code-best/claude-code
Length of output: 886
🏁 Script executed:
rg -n "rejects\.toThrow.*AbortError" packages/builtin-tools/src/tools/WebSearchTool/__tests__/Repository: claude-code-best/claude-code
Length of output: 302
Hoist AbortError class above factory function to fix identity mismatch.
_abortMock creates separate class instances for each mock.module() call. The adapter imports from 'src/utils/errors.js' (line 7 of braveAdapter.ts) and throws instances of that mock's AbortError, but the test imports from 'src/utils/errors' (line 205) and verifies against that mock's AbortError. Since both specifiers resolve to distinct module records, instanceof checks in rejects.toThrow(AbortError) at line 208 will fail despite functional equivalence.
Define the class once outside the factory and return it from both mock.module() calls:
const abortErrorClass = class AbortError extends Error {
constructor(message?: string) { super(message); this.name = 'AbortError' }
}
const _abortMock = () => ({
AbortError: abortErrorClass,
isAbortError: (e: unknown) => e instanceof Error && (e as Error).name === 'AbortError',
})
mock.module('src/utils/errors.js', _abortMock)
mock.module('src/utils/errors', _abortMock)
Same issue exists in bingAdapter.test.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/builtin-tools/src/tools/WebSearchTool/__tests__/braveAdapter.test.ts`
around lines 6 - 13, The test currently defines AbortError inside the _abortMock
factory causing two distinct class identities when calling
mock.module('src/utils/errors.js', _abortMock) and
mock.module('src/utils/errors', _abortMock); hoist a single AbortError class
declaration outside the _abortMock factory (e.g. const abortErrorClass = class
AbortError extends Error { ... }) and have _abortMock return that class
(AbortError: abortErrorClass) and the same isAbortError implementation so both
mock.module calls return the same class identity; apply the same change in the
corresponding bingAdapter.test.ts to ensure
instanceof/rejects.toThrow(AbortError) comparisons succeed.
Comment on lines
+64
to
+76
| export function buildOpenAIRequestBody(params: { | ||
| model: string | ||
| messages: any[] | ||
| tools: any[] | ||
| toolChoice: any | ||
| enableThinking: boolean | ||
| maxTokens: number | ||
| temperatureOverride?: number | ||
| }): ChatCompletionCreateParamsStreaming & { | ||
| thinking?: { type: string } | ||
| enable_thinking?: boolean | ||
| chat_template_kwargs?: { thinking: boolean } | ||
| } { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tighten parameter types — avoid any[]/any in production code.
Per the repository coding guidelines (prohibit as any; use concrete types / Record<string, unknown>), messages: any[], tools: any[], and toolChoice: any should use the OpenAI SDK types already available in openai/resources/chat/completions/completions.mjs.
♻️ Suggested typing
+import type {
+ ChatCompletionCreateParamsStreaming,
+ ChatCompletionMessageParam,
+ ChatCompletionTool,
+ ChatCompletionToolChoiceOption,
+} from 'openai/resources/chat/completions/completions.mjs'
@@
export function buildOpenAIRequestBody(params: {
model: string
- messages: any[]
- tools: any[]
- toolChoice: any
+ messages: ChatCompletionMessageParam[]
+ tools: ChatCompletionTool[]
+ toolChoice?: ChatCompletionToolChoiceOption
enableThinking: boolean
maxTokens: number
temperatureOverride?: number
}): ChatCompletionCreateParamsStreaming & {
- thinking?: { type: string }
+ thinking?: { type: 'enabled' }
enable_thinking?: boolean
chat_template_kwargs?: { thinking: boolean }
} {As per coding guidelines: "Prohibit as any in production code — use as unknown as SpecificType double assertion or supplement interface definitions instead" and "Use Record<string, unknown> instead of any for objects with unknown structure".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/api/openai/requestBody.ts` around lines 64 - 76, The function
buildOpenAIRequestBody currently accepts messages: any[], tools: any[], and
toolChoice: any — replace those any types with concrete OpenAI SDK types (e.g.,
use the SDK's ChatCompletionRequestMessage[] for messages and the appropriate
Tool/ToolCollection type for tools and toolChoice imported from
openai/resources/chat/completions/completions.mjs); if the SDK types are not
directly compatible, use Record<string, unknown>[] or Record<string, unknown>
for tools/toolChoice and narrow with as unknown as <SDKType> (double assertion)
to satisfy the coding guideline against any, and update the function signature
and any callsites that construct messages/tools/toolChoice to match the new
types (refer to buildOpenAIRequestBody, parameter names messages, tools,
toolChoice).
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
3 participants
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.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores