Feature/docker/run#1268
Merged
Merged
Conversation
- API 兼容层(openai/grok/gemini): 利用 BetaRawMessageStreamEvent 的 discriminated union 在 switch/case 中直接属性访问，消除 ~29 个 as any - ConsoleOAuthFlow: 用 as unknown as Parameters<typeof> 替代 as any - performanceShim: 用 Record<string, unknown> 和显式类型断言替代 as any - companionReact/auth: 直接访问已有类型属性消除 as any - sliceAnsi/textHighlighting: 用 as Char 替代 as any（Token 联合类型收窄） - ccrClient: 利用 RequestResult 类型收窄直接访问 retryAfterMs - outputsScanner: 用 TurnStartTime.turnStartTime 属性访问替代双重断言 - plans: 用显式数组类型替代 as any[] - FeedbackSurvey: 用 in 操作符和 Parameters<typeof> 替代 as any - messageQueueManager: 用 Record<string, unknown> 替代 as any - mcp.ts: 用 in 操作符类型守卫替代 as any precheck 通过: typecheck 零错误 + 5420 测试全部通过 + lint 通过
- AppStateStore: 添加 pipeIpc?: PipeIpcState 可选字段 - PromptInputFooter: 直接访问 s.pipeIpc - useBackgroundTaskNavigation: 直接访问 s.pipeIpc - usePipeRouter: 直接访问 store.getState().pipeIpc - REPL.tsx: 移除 getPipeIpc(s as any) 中的 as any precheck 通过
Ink Key 类型已包含 wheelDown/wheelUp 属性，直接访问即可。
- toolUse.name: 使用 as unknown as { name: string } 双重断言
- apiErr.error: 使用 as Parameters<typeof formatAPIError>[0] 类型参数
项目级 settings.local.json 的 env 字段在 trust dialog 之前只有 SAFE_ENV_VARS 白名单中的变量会被应用到 process.env。 OPENAI_API_KEY、OPENAI_BASE_URL 等关键变量不在白名单中， 导致容器中通过 settings.local.json 配置 OpenAI 协议时认证失败。
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTightens TypeScript typing across streaming adapters and utilities, adds optional Pipe IPC to AppState, introduces a periodic goal elapsed footer indicator, expands the SAFE_ENV_VARS allowlist for OpenAI/Grok/Gemini, and limits auto-dream consolidation to 20 turns. ChangesType Safety and Platform Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/PromptInput/PromptInputFooter.tsx (1)
278-281:⚠️ Potential issue | 🟡 MinorType
setAppStateupdaters inPromptInputFooterand remove the{}pipeIpc fallback
- In
src/components/PromptInput/PromptInputFooter.tsx, the threesetAppState((prev: any) => { ... })updaters (routeMode at ~278-281, selectedPipes toggle at ~317-322, selectorOpen close at ~325-328) should use(prev: AppState)to matchuseSetAppState’s(prev: AppState) => AppStatesignature.- Replace
const pIpc = prev.pipeIpc ?? {}withconst pIpc = getPipeIpc(prev)(importgetPipeIpcfromsrc/utils/pipeTransport.tsalongside the existing pipeTransport imports).PipeIpcStatehas only required fields, so spreading{}is type-unsound and can produce an incompletepipeIpcstate ifpipeIpcis ever missing at update time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PromptInput/PromptInputFooter.tsx` around lines 278 - 281, Update the three setAppState updaters in PromptInputFooter to type their prev parameter as (prev: AppState) instead of any and replace the unsafe fallback const pIpc = prev.pipeIpc ?? {} with const pIpc = getPipeIpc(prev); import getPipeIpc from src/utils/pipeTransport.ts alongside the existing pipeTransport imports so you always obtain a complete PipeIpcState; ensure this change is applied to the updaters that set routeMode, toggle selectedPipes, and close selectorOpen so the returned AppState conforms to (prev: AppState) => AppState.
🧹 Nitpick comments (3)
src/hooks/usePipeRouter.ts (1)
11-12: ⚡ Quick winReplace
anyin type aliases with proper imported types.The
StoreApiandSetAppStatetype aliases useanyfor their return and parameter types. Since this file has access toAppStatefrom../state/AppState.js, consider importing and using the proper types (e.g.,AppStateStoretype if exported, or properly typing the state parameter asAppState).This would complete the type safety improvements started at line 40 and align with the PR's objective to eliminate broad
anyusage.♻️ Suggested refactor
+import type { AppState } from '../state/AppState.js' + -type StoreApi = { getState: () => any } -type SetAppState = (updater: (prev: any) => any) => void +type StoreApi = { getState: () => AppState } +type SetAppState = (updater: (prev: AppState) => AppState) => voidThen update line 89:
- setAppState((prev: any) => { + setAppState((prev: AppState) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/usePipeRouter.ts` around lines 11 - 12, Replace the broad any usages in the type aliases StoreApi and SetAppState by importing and using the concrete AppState types from ../state/AppState.js (or the exported store type if available): change StoreApi.getState to return AppState (or AppStateStore) and change SetAppState to accept an updater whose prev param is typed as AppState; update the import to bring in AppState (or AppStateStore) and then adjust the usages of StoreApi and SetAppState (e.g., in the usePipeRouter hook and the updater passed at the location currently around the updater call) to use these concrete types.src/utils/messageQueueManager.ts (1)
371-373: ⚡ Quick winRemove redundant optional chaining.
The optional chaining
?.kindon line 373 is unnecessary because line 371 already guarantees thatorigin !== undefined. After confirmingoriginexists, you can use direct property access.♻️ Simplified access without optional chaining
if ( (feature('KAIROS') || feature('KAIROS_CHANNELS')) && (cmd as Record<string, unknown>).origin !== undefined && - ((cmd as Record<string, unknown>).origin as Record<string, unknown>) - ?.kind === 'channel' + ((cmd as Record<string, unknown>).origin as Record<string, unknown>) + .kind === 'channel' )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/messageQueueManager.ts` around lines 371 - 373, The conditional redundantly uses optional chaining on origin's kind; since the earlier check ensures (cmd as Record<string, unknown>).origin !== undefined, update the expression in messageQueueManager.ts to access origin.kind directly (remove the ?. from ?.kind) so the condition becomes a direct property comparison (referencing the same cast of cmd and the origin object used in the current conditional).src/utils/filePersistence/outputsScanner.ts (1)
67-74: ⚡ Quick winSimplify the type annotation by removing the redundant union.
The union type at lines 67-69 is immediately cast away at line 74. Since the cast is necessary anyway (to account for Node version differences in the
fs.readdirreturn type), the union adds no value and makes the code harder to read.♻️ Simplified type annotation
- let entries: - | Awaited<ReturnType<typeof fs.readdir>> - | { name: string; isFile(): boolean; isSymbolicLink(): boolean }[] try { - entries = (await fs.readdir(outputsDir, { + const entries = (await fs.readdir(outputsDir, { withFileTypes: true, recursive: true, })) as { name: string; isFile(): boolean; isSymbolicLink(): boolean }[]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/filePersistence/outputsScanner.ts` around lines 67 - 74, The declared variable entries uses an unnecessary union type then immediately casts the result of fs.readdir to one branch; remove the redundant union and declare entries with the single target type used in the cast (e.g. the array of Dirent-like objects), so set entries to the same type you cast to and keep the await fs.readdir(...) as that type; update references to entries accordingly (symbols: entries, fs.readdir, outputsDir).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/PromptInput/PromptInputFooterLeftSide.tsx`:
- Line 178: The color prop is incorrectly asserted to the literal 'ansi:green';
update the return in PromptInputFooterLeftSide to assert color as keyof Theme
(i.e., use color as keyof Theme) instead of 'ansi:green', and if Theme is not
already imported add Theme to the imports from '`@anthropic/ink`'; ensure the Text
component uses color={color as keyof Theme} so the union type ('ansi:green' |
'ansi:yellow' | 'ansi:red' | 'ansi:cyan' | undefined) is correctly typed.
- Around line 140-141: The conditional require for getGoal/getActiveElapsedMs is
being executed inside the component on every render; move that conditional
require to the top-level of the module (following the same pattern used for
coordinatorModule and proactiveModule) so it only runs once at load time, export
or keep the resulting binding in a top-level const (e.g., const { getGoal,
getActiveElapsedMs } = require(...) as typeof import(...);) and update the
component to use those top-level symbols instead of calling require on each
tick.
- Around line 140-142: The require call for ../../services/goal/goalState.js
used to import getGoal and getActiveElapsedMs in PromptInputFooterLeftSide.tsx
is pointing to a non-existent module and will throw MODULE_NOT_FOUND when GOAL
is enabled; fix it by either adding a new module that exports getGoal and
getActiveElapsedMs (matching the expected signatures) under
src/services/goal/goalState.{ts,js} or change the require/import to the correct
existing module that implements those functions (update the require in
PromptInputFooterLeftSide.tsx where getGoal and getActiveElapsedMs are
referenced). Ensure the exported names exactly match getGoal and
getActiveElapsedMs so the current usage continues to work.
In `@src/utils/managedEnvConstants.ts`:
- Around line 166-168: The change incorrectly adds sensitive provider
credentials and endpoints into SAFE_ENV_VARS, contradicting the module's own
security model; update the fix by removing OPENAI_API_KEY, OPENAI_BASE_URL,
GROK_API_KEY, GROK_BASE_URL, XAI_API_KEY, GEMINI_API_KEY, and GEMINI_BASE_URL
from the SAFE_ENV_VARS constant (and any duplicated additions elsewhere around
the file) so they remain classified as dangerous, or alternatively keep them in
SAFE_ENV_VARS only after updating the module documentation to explicitly justify
why third‑party API keys/base URLs are exempt from the trust dialog; locate and
modify the SAFE_ENV_VARS definition and accompanying documentation comments to
ensure consistency between the constant and the security docs.
---
Outside diff comments:
In `@src/components/PromptInput/PromptInputFooter.tsx`:
- Around line 278-281: Update the three setAppState updaters in
PromptInputFooter to type their prev parameter as (prev: AppState) instead of
any and replace the unsafe fallback const pIpc = prev.pipeIpc ?? {} with const
pIpc = getPipeIpc(prev); import getPipeIpc from src/utils/pipeTransport.ts
alongside the existing pipeTransport imports so you always obtain a complete
PipeIpcState; ensure this change is applied to the updaters that set routeMode,
toggle selectedPipes, and close selectorOpen so the returned AppState conforms
to (prev: AppState) => AppState.
---
Nitpick comments:
In `@src/hooks/usePipeRouter.ts`:
- Around line 11-12: Replace the broad any usages in the type aliases StoreApi
and SetAppState by importing and using the concrete AppState types from
../state/AppState.js (or the exported store type if available): change
StoreApi.getState to return AppState (or AppStateStore) and change SetAppState
to accept an updater whose prev param is typed as AppState; update the import to
bring in AppState (or AppStateStore) and then adjust the usages of StoreApi and
SetAppState (e.g., in the usePipeRouter hook and the updater passed at the
location currently around the updater call) to use these concrete types.
In `@src/utils/filePersistence/outputsScanner.ts`:
- Around line 67-74: The declared variable entries uses an unnecessary union
type then immediately casts the result of fs.readdir to one branch; remove the
redundant union and declare entries with the single target type used in the cast
(e.g. the array of Dirent-like objects), so set entries to the same type you
cast to and keep the await fs.readdir(...) as that type; update references to
entries accordingly (symbols: entries, fs.readdir, outputsDir).
In `@src/utils/messageQueueManager.ts`:
- Around line 371-373: The conditional redundantly uses optional chaining on
origin's kind; since the earlier check ensures (cmd as Record<string,
unknown>).origin !== undefined, update the expression in messageQueueManager.ts
to access origin.kind directly (remove the ?. from ?.kind) so the condition
becomes a direct property comparison (referencing the same cast of cmd and the
origin object used in the current conditional).
🪄 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: a8e957fb-c975-4231-b3a7-2b40a1ccb00d
📒 Files selected for processing (33)
dist-nosplit/cli.jsdist-nosplit/vendor/audio-capture/arm64-darwin/audio-capture.nodedist-nosplit/vendor/audio-capture/arm64-linux/audio-capture.nodedist-nosplit/vendor/audio-capture/arm64-win32/audio-capture.nodedist-nosplit/vendor/audio-capture/x64-darwin/audio-capture.nodedist-nosplit/vendor/audio-capture/x64-linux/audio-capture.nodedist-nosplit/vendor/audio-capture/x64-win32/audio-capture.nodedist-nosplit/vendor/ripgrep/arm64-linux/rgsrc/buddy/companionReact.tssrc/cli/transports/ccrClient.tssrc/components/ConsoleOAuthFlow.tsxsrc/components/FeedbackSurvey/useFrustrationDetection.tssrc/components/PromptInput/PromptInputFooter.tsxsrc/components/PromptInput/PromptInputFooterLeftSide.tsxsrc/components/ultraplan/UltraplanChoiceDialog.tsxsrc/entrypoints/mcp.tssrc/hooks/useBackgroundTaskNavigation.tssrc/hooks/usePipeRouter.tssrc/screens/REPL.tsxsrc/services/api/gemini/index.tssrc/services/api/grok/index.tssrc/services/api/openai/index.tssrc/services/autoDream/autoDream.tssrc/state/AppStateStore.tssrc/utils/auth.tssrc/utils/filePersistence/outputsScanner.tssrc/utils/managedEnvConstants.tssrc/utils/messageQueueManager.tssrc/utils/performanceShim.tssrc/utils/plans.tssrc/utils/sideQuestion.tssrc/utils/sliceAnsi.tssrc/utils/textHighlighting.ts
Comment on lines
+140
to
+141
| const { getGoal, getActiveElapsedMs } = | ||
| require('../../services/goal/goalState.js') as typeof import('../../services/goal/goalState.js'); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move conditional require() to top-level for consistency and efficiency.
The require() call inside the component body runs on every render (every second due to the tick interval). The existing pattern in this file (lines 5-7 for coordinatorModule, lines 51 for proactiveModule) conditionally requires modules at the top level, which evaluates once at module load time.
♻️ Move to top-level conditional require pattern
At the top of the file (after line 52):
const proactiveModule = feature('PROACTIVE') || feature('KAIROS') ? require('../../proactive/index.js') : null;
/* eslint-enable `@typescript-eslint/no-require-imports` */
+/* eslint-disable `@typescript-eslint/no-require-imports` */
+const goalModule = feature('GOAL')
+ ? (require('../../services/goal/goalState.js') as typeof import('../../services/goal/goalState.js'))
+ : undefined;
+/* eslint-enable `@typescript-eslint/no-require-imports` */
const NO_OP_SUBSCRIBE = (_cb: () => void) => () => {};Then in the component (lines 140-142):
- const { getGoal, getActiveElapsedMs } =
- require('../../services/goal/goalState.js') as typeof import('../../services/goal/goalState.js');
- const goal = getGoal();
+ if (!goalModule) return null;
+ const goal = goalModule.getGoal();
if (!goal) return null;
- const elapsedMs = getActiveElapsedMs(goal);
+ const elapsedMs = goalModule.getActiveElapsedMs(goal);And replace the condition at line 432:
- ...(feature('GOAL') &&
- (require('../../services/goal/goalState.js') as typeof import('../../services/goal/goalState.js')).getGoal()
+ ...(feature('GOAL') && goalModule?.getGoal()
? [<GoalElapsedIndicator key="goal-elapsed" />]
: []),🧰 Tools
🪛 GitHub Actions: CI / 0_ci.txt
[error] 141-141: TypeScript (tsc --noEmit) TS2307: Cannot find module '../../services/goal/goalState.js' or its corresponding type declarations.
🪛 GitHub Actions: CI / ci
[error] 141-141: TypeScript (tsc --noEmit) failed: TS2307 Cannot find module '../../services/goal/goalState.js' or its corresponding type declarations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/PromptInput/PromptInputFooterLeftSide.tsx` around lines 140 -
141, The conditional require for getGoal/getActiveElapsedMs is being executed
inside the component on every render; move that conditional require to the
top-level of the module (following the same pattern used for coordinatorModule
and proactiveModule) so it only runs once at load time, export or keep the
resulting binding in a top-level const (e.g., const { getGoal,
getActiveElapsedMs } = require(...) as typeof import(...);) and update the
component to use those top-level symbols instead of calling require on each
tick.
| break; | ||
| } | ||
|
|
||
| return <Text color={color as 'ansi:green'}>goal ({timeStr})</Text>; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use as keyof Theme instead of as 'ansi:green' for Ink color prop.
The color variable can be 'ansi:green' | 'ansi:yellow' | 'ansi:red' | 'ansi:cyan' | undefined, but the type assertion forces it to 'ansi:green'. According to coding guidelines for Ink color prop, use as keyof Theme. As per coding guidelines, "Ink color prop must use as keyof Theme rather than as any".
♻️ Fix type assertion
- return <Text color={color as 'ansi:green'}>goal ({timeStr})</Text>;
+ return <Text color={color as keyof Theme}>goal ({timeStr})</Text>;If Theme is not imported, add it to the imports from @anthropic/ink:
-import { Box, Text, Link } from '`@anthropic/ink`';
+import { Box, Text, Link, type Theme } from '`@anthropic/ink`';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return <Text color={color as 'ansi:green'}>goal ({timeStr})</Text>; | |
| return <Text color={color as keyof Theme}>goal ({timeStr})</Text>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/PromptInput/PromptInputFooterLeftSide.tsx` at line 178, The
color prop is incorrectly asserted to the literal 'ansi:green'; update the
return in PromptInputFooterLeftSide to assert color as keyof Theme (i.e., use
color as keyof Theme) instead of 'ansi:green', and if Theme is not already
imported add Theme to the imports from '`@anthropic/ink`'; ensure the Text
component uses color={color as keyof Theme} so the union type ('ansi:green' |
'ansi:yellow' | 'ansi:red' | 'ansi:cyan' | undefined) is correctly typed.
Source: Coding guidelines
Comment on lines
+166
to
+168
| 'OPENAI_API_KEY', | ||
| 'OPENAI_AUTH_MODE', | ||
| 'OPENAI_BASE_URL', |
Contributor
There was a problem hiding this comment.
Security policy violation: API keys and base URLs marked as safe contradict the security model.
The file's own documentation (lines 134-146) explicitly classifies base URLs and API keys as dangerous:
- "REDIRECT TO ATTACKER-CONTROLLED SERVER" includes base URLs
- "SWITCH TO ATTACKER-CONTROLLED PROJECT" includes API keys
Yet this change adds OPENAI_API_KEY, OPENAI_BASE_URL, GROK_API_KEY, GROK_BASE_URL, XAI_API_KEY, GEMINI_API_KEY, and GEMINI_BASE_URL to SAFE_ENV_VARS, which bypasses the trust dialog. An attacker controlling remote managed settings can now:
- Set
OPENAI_BASE_URLto redirect OpenAI traffic to their server - Set
OPENAI_API_KEYto bill usage to their account and exfiltrate prompts - Same for Grok/Gemini providers
This is the exact threat model the trust dialog was designed to prevent. Either these variables must remain dangerous (removed from SAFE_ENV_VARS), or the security documentation must be updated to explain why third-party provider credentials are exempt from the threat model that applies to Anthropic credentials.
Also applies to: 188-189, 195-195, 225-226
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/managedEnvConstants.ts` around lines 166 - 168, The change
incorrectly adds sensitive provider credentials and endpoints into
SAFE_ENV_VARS, contradicting the module's own security model; update the fix by
removing OPENAI_API_KEY, OPENAI_BASE_URL, GROK_API_KEY, GROK_BASE_URL,
XAI_API_KEY, GEMINI_API_KEY, and GEMINI_BASE_URL from the SAFE_ENV_VARS constant
(and any duplicated additions elsewhere around the file) so they remain
classified as dangerous, or alternatively keep them in SAFE_ENV_VARS only after
updating the module documentation to explicitly justify why third‑party API
keys/base URLs are exempt from the trust dialog; locate and modify the
SAFE_ENV_VARS definition and accompanying documentation comments to ensure
consistency between the constant and the security docs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/model/__tests__/providers.test.ts (1)
71-79: ⚡ Quick winConsider adding cross-tier precedence tests.
The new OpenAI and Grok tests verify provider selection when their respective env vars are set, but there are no tests verifying that first-tier providers (bedrock/vertex/foundry) take precedence over second-tier providers (openai/grok/gemini) when both are set simultaneously. For example:
test('bedrock takes precedence over openai', () => { process.env.CLAUDE_CODE_USE_BEDROCK = '1' process.env.CLAUDE_CODE_USE_OPENAI = '1' expect(getAPIProvider({})).toBe('bedrock') })This would validate the precedence ordering from the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/model/__tests__/providers.test.ts` around lines 71 - 79, Add tests that assert first-tier provider env vars (CLAUDE_CODE_USE_BEDROCK, CLAUDE_CODE_USE_VERTEX, CLAUDE_CODE_USE_FOUNDRY) take precedence over second-tier vars (CLAUDE_CODE_USE_OPENAI, CLAUDE_CODE_USE_GROK, CLAUDE_CODE_USE_GEMINI) when both are set; update src/utils/model/__tests__/providers.test.ts to include cases like calling getAPIProvider({}) with both env vars set and expecting 'bedrock'/'vertex'/'foundry' respectively, and ensure each test clears or resets other related env vars after running to avoid cross-test pollution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/model/__tests__/providers.test.ts`:
- Line 2: The test import uses a relative path for isEnvTruthy; update the
import in src/utils/model/__tests__/providers.test.ts to use the project alias
(src/*) instead of '../../envUtils.js' so it imports isEnvTruthy from the
aliased module (e.g., import { isEnvTruthy } from 'src/utils/envUtils' or the
correct aliased path used in the repo), ensuring the test references the same
exported symbol name isEnvTruthy.
---
Nitpick comments:
In `@src/utils/model/__tests__/providers.test.ts`:
- Around line 71-79: Add tests that assert first-tier provider env vars
(CLAUDE_CODE_USE_BEDROCK, CLAUDE_CODE_USE_VERTEX, CLAUDE_CODE_USE_FOUNDRY) take
precedence over second-tier vars (CLAUDE_CODE_USE_OPENAI, CLAUDE_CODE_USE_GROK,
CLAUDE_CODE_USE_GEMINI) when both are set; update
src/utils/model/__tests__/providers.test.ts to include cases like calling
getAPIProvider({}) with both env vars set and expecting
'bedrock'/'vertex'/'foundry' respectively, and ensure each test clears or resets
other related env vars after running to avoid cross-test pollution.
🪄 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: 4dac4698-1599-466c-a09c-2fea12e913d8
📒 Files selected for processing (1)
src/utils/model/__tests__/providers.test.ts
测试不再 import providers.ts（其默认参数触发 getInitialSettings 全链）， 改为内联纯函数逻辑，从根源消除 CI 上其他测试 mock.module 污染。
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/model/__tests__/providers.test.ts (1)
171-184: ⚡ Quick winConsider adding test coverage for full truthy value range.
Once the
isEnvTruthylogic is inlined, consider adding tests for the complete set of supported formats:
'yes'and'on'- Case variations:
'TRUE','True','YES','On'- Trimmed values:
' true ',' 1 'This would ensure production behavior for all supported env var formats is validated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/model/__tests__/providers.test.ts` around lines 171 - 184, Add unit tests for getAPIProviderTest to cover the full range of env-var truthy formats supported by isEnvTruthy: add cases setting process.env.CLAUDE_CODE_USE_BEDROCK to 'yes' and 'on', uppercase/lowercase variants like 'TRUE', 'True', 'YES', 'On', numeric '1', and trimmed values like ' true ' and ' 1 ' to assert they return 'bedrock', plus negative examples ('false','off','0','') to assert 'firstParty'; place these new cases alongside the existing tests in providers.test.ts referencing getAPIProviderTest so the inlined isEnvTruthy behavior is fully validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/model/__tests__/providers.test.ts`:
- Around line 23-62: getAPIProviderTest currently only treats env values equal
to '1' or 'true' as truthy, causing tests to diverge from production; update
getAPIProviderTest to use the same truthiness logic as isEnvTruthy
(case-insensitive, trimmed match for '1', 'true', 'yes', 'on') or call
isEnvTruthy directly, and apply it for CLAUDE_CODE_USE_BEDROCK,
CLAUDE_CODE_USE_VERTEX, CLAUDE_CODE_USE_FOUNDRY, CLAUDE_CODE_USE_OPENAI,
CLAUDE_CODE_USE_GEMINI, and CLAUDE_CODE_USE_GROK so values like 'yes', 'TRUE',
or ' true ' return the expected provider instead of 'firstParty'.
---
Nitpick comments:
In `@src/utils/model/__tests__/providers.test.ts`:
- Around line 171-184: Add unit tests for getAPIProviderTest to cover the full
range of env-var truthy formats supported by isEnvTruthy: add cases setting
process.env.CLAUDE_CODE_USE_BEDROCK to 'yes' and 'on', uppercase/lowercase
variants like 'TRUE', 'True', 'YES', 'On', numeric '1', and trimmed values like
' true ' and ' 1 ' to assert they return 'bedrock', plus negative examples
('false','off','0','') to assert 'firstParty'; place these new cases alongside
the existing tests in providers.test.ts referencing getAPIProviderTest so the
inlined isEnvTruthy behavior is fully validated.
🪄 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: 37a4be71-4a46-4bab-bd0d-18f8afb89eac
📒 Files selected for processing (1)
src/utils/model/__tests__/providers.test.ts
Comment on lines
+23
to
+62
| function getAPIProviderTest(settings: { modelType?: string }): APIProvider { | ||
| const modelType = settings.modelType | ||
| if (modelType === 'openai') return 'openai' | ||
| if (modelType === 'gemini') return 'gemini' | ||
| if (modelType === 'grok') return 'grok' | ||
|
|
||
| if ( | ||
| process.env.CLAUDE_CODE_USE_BEDROCK === '1' || | ||
| process.env.CLAUDE_CODE_USE_BEDROCK === 'true' | ||
| ) | ||
| return 'bedrock' | ||
| if ( | ||
| process.env.CLAUDE_CODE_USE_VERTEX === '1' || | ||
| process.env.CLAUDE_CODE_USE_VERTEX === 'true' | ||
| ) | ||
| return 'vertex' | ||
| if ( | ||
| process.env.CLAUDE_CODE_USE_FOUNDRY === '1' || | ||
| process.env.CLAUDE_CODE_USE_FOUNDRY === 'true' | ||
| ) | ||
| return 'foundry' | ||
|
|
||
| if ( | ||
| process.env.CLAUDE_CODE_USE_OPENAI === '1' || | ||
| process.env.CLAUDE_CODE_USE_OPENAI === 'true' | ||
| ) | ||
| return 'openai' | ||
| if ( | ||
| process.env.CLAUDE_CODE_USE_GEMINI === '1' || | ||
| process.env.CLAUDE_CODE_USE_GEMINI === 'true' | ||
| ) | ||
| return 'gemini' | ||
| if ( | ||
| process.env.CLAUDE_CODE_USE_GROK === '1' || | ||
| process.env.CLAUDE_CODE_USE_GROK === 'true' | ||
| ) | ||
| return 'grok' | ||
|
|
||
| return 'firstParty' | ||
| } |
Contributor
There was a problem hiding this comment.
Incomplete truthiness logic diverges from production behavior.
The inlined getAPIProviderTest only checks for exact matches of '1' or 'true', but production's isEnvTruthy (Context snippet 3) accepts '1', 'true', 'yes', 'on' with case-insensitive, trimmed matching. This creates critical test gaps:
- Setting
CLAUDE_CODE_USE_BEDROCK='yes'in production returns'bedrock', but the test would return'firstParty' - Setting
CLAUDE_CODE_USE_BEDROCK='TRUE'or' true 'would similarly diverge
The tests won't catch bugs when users rely on these supported formats.
🔧 Proposed fix to inline full truthiness logic
+function isEnvTruthy(envVar: string | undefined): boolean {
+ if (!envVar) return false
+ const normalizedValue = envVar.toLowerCase().trim()
+ return ['1', 'true', 'yes', 'on'].includes(normalizedValue)
+}
+
function getAPIProviderTest(settings: { modelType?: string }): APIProvider {
const modelType = settings.modelType
if (modelType === 'openai') return 'openai'
if (modelType === 'gemini') return 'gemini'
if (modelType === 'grok') return 'grok'
- if (
- process.env.CLAUDE_CODE_USE_BEDROCK === '1' ||
- process.env.CLAUDE_CODE_USE_BEDROCK === 'true'
- )
- return 'bedrock'
- if (
- process.env.CLAUDE_CODE_USE_VERTEX === '1' ||
- process.env.CLAUDE_CODE_USE_VERTEX === 'true'
- )
- return 'vertex'
- if (
- process.env.CLAUDE_CODE_USE_FOUNDRY === '1' ||
- process.env.CLAUDE_CODE_USE_FOUNDRY === 'true'
- )
- return 'foundry'
-
- if (
- process.env.CLAUDE_CODE_USE_OPENAI === '1' ||
- process.env.CLAUDE_CODE_USE_OPENAI === 'true'
- )
- return 'openai'
- if (
- process.env.CLAUDE_CODE_USE_GEMINI === '1' ||
- process.env.CLAUDE_CODE_USE_GEMINI === 'true'
- )
- return 'gemini'
- if (
- process.env.CLAUDE_CODE_USE_GROK === '1' ||
- process.env.CLAUDE_CODE_USE_GROK === 'true'
- )
- return 'grok'
+ if (isEnvTruthy(process.env.CLAUDE_CODE_USE_BEDROCK)) return 'bedrock'
+ if (isEnvTruthy(process.env.CLAUDE_CODE_USE_VERTEX)) return 'vertex'
+ if (isEnvTruthy(process.env.CLAUDE_CODE_USE_FOUNDRY)) return 'foundry'
+
+ if (isEnvTruthy(process.env.CLAUDE_CODE_USE_OPENAI)) return 'openai'
+ if (isEnvTruthy(process.env.CLAUDE_CODE_USE_GEMINI)) return 'gemini'
+ if (isEnvTruthy(process.env.CLAUDE_CODE_USE_GROK)) return 'grok'
return 'firstParty'
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/model/__tests__/providers.test.ts` around lines 23 - 62,
getAPIProviderTest currently only treats env values equal to '1' or 'true' as
truthy, causing tests to diverge from production; update getAPIProviderTest to
use the same truthiness logic as isEnvTruthy (case-insensitive, trimmed match
for '1', 'true', 'yes', 'on') or call isEnvTruthy directly, and apply it for
CLAUDE_CODE_USE_BEDROCK, CLAUDE_CODE_USE_VERTEX, CLAUDE_CODE_USE_FOUNDRY,
CLAUDE_CODE_USE_OPENAI, CLAUDE_CODE_USE_GEMINI, and CLAUDE_CODE_USE_GROK so
values like 'yes', 'TRUE', or ' true ' return the expected provider instead of
'firstParty'.
CI 中的 autonomy-lifecycle-user-flow 集成测试会执行 build.ts 打包 CLI。
此前 PromptInputFooterLeftSide.tsx 中 require('../../services/goal/goalState.js')
的路径在源码中不存在，打包器报 Could not resolve，导致 (unnamed) 测试失败。
新增 src/services/goal/goalState.ts 存根模块（getGoal 返回 null，组件不渲染），
让打包器在构建期可以解析该 require 路径。同时把 PromptInputFooterLeftSide.tsx
里两处 as unknown as 内联类型签名换成 as typeof import(...)，让类型直接来自
存根模块，避免类型定义重复。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests