Merged
Conversation
在 bypassPermissionsKillswitch.ts 的 useKickOffCheckAndDisableAutoModeIfNeeded 中，void fire-and-forget 调用缺少 .catch() 处理，导致 verifyAutoModeGateAccess 失败时产生 unhandled promise rejection。同时移除 permissionSetup.ts 中冗余的 null check。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
通过 Shift+Tab 统一循环：default → acceptEdits → plan → auto → bypassPermissions → default - 移除 USER_TYPE 分支判断，所有用户使用同一循环路径 - isBypassPermissionsModeAvailable 始终为 true - isAutoModeAvailable 初始化直接为 true - 移除 AutoModeOptInDialog 确认流程 - 简化 isAutoModeGateEnabled 仅保留快模式熔断器 - 简化 verifyAutoModeGateAccess 仅检查快模式 - 移除 GrowthBook/Statsig 远程门控 - bypass permissions killswitch 改为 no-op - 新增 24 个测试覆盖循环逻辑和门控不变量 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sideQuery 绕过了 claude.ts 的主 API 路径，导致所有走 sideQuery 的调用 （auto mode classifier、permission explainer、session search 等）都没有 Langfuse 记录。现在为每次 sideQuery 调用创建独立 trace 并记录 LLM observation， 未配置 Langfuse 时全部 no-op。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughPermission-mode gating and opt-in flows were removed: bypass permissions are now available by default and auto mode is universally enabled. The opt-in dialog and bypass killswitch logic were deleted/stubbed, permission cycling was unified, some state persistence was changed to in-memory overrides, and Langfuse tracing was added to side queries. 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
- ACP agent availableModes 按条件包含 bypassPermissions（非 root/sandbox） - 顺序对齐 REPL 循环：default → acceptEdits → plan → auto → bypassPermissions - 新增 2 个测试验证 availableModes 包含 bypassPermissions 及模式切换 - 修正 getNextPermissionMode.test.ts 和 permissionSetup.test.ts 的 import 路径 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils/settings/settings.ts (1)
891-899:⚠️ Potential issue | 🟡 MinorUpdate the stale opt-in/security JSDoc.
The function no longer checks trusted settings sources or an opt-in dialog, so the current comment is misleading.
📝 Proposed doc update
-/** - * Returns true if any trusted settings source has accepted the auto - * mode opt-in dialog. projectSettings is intentionally excluded — - * a malicious project could otherwise auto-bypass the dialog (RCE risk). - */ +/** + * Returns whether auto mode should be treated as opted in. + * Auto mode is now available without a per-user opt-in dialog. + */ export function hasAutoModeOptIn(): boolean { // Auto mode is available to all users — no opt-in needed return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/settings/settings.ts` around lines 891 - 899, The JSDoc above hasAutoModeOptIn() is outdated and mentions trusted settings, opt-in dialog, and RCE risk even though the function now unconditionally returns true; update the comment to accurately describe current behavior: state that auto mode is universally available (no opt-in), that the function always returns true, and remove references to trusted settings sources, projectSettings, opt-in dialog, or security rationale; edit the comment block immediately above the hasAutoModeOptIn declaration to a concise one-line summary reflecting this behavior.src/utils/permissions/permissionSetup.ts (2)
1175-1195:⚠️ Potential issue | 🟡 MinorStale doc comment: default is no longer
'disabled'.
AUTO_MODE_ENABLED_DEFAULTis now'enabled'(line 1175), but the docstring ongetAutoModeEnabledStatestill says “Defaults to 'disabled' if GrowthBook is unavailable or the field is unset.” Update the comment so the behavior is accurately documented for downstream surfaces (IDE/Desktop) that consult this value.📝 Proposed doc fix
/** * Reads the `enabled` field from tengu_auto_mode_config (cached, may be stale). - * Defaults to 'disabled' if GrowthBook is unavailable or the field is unset. + * Defaults to 'enabled' if GrowthBook is unavailable or the field is unset. * Other surfaces (IDE, Desktop) should call this to decide whether to surface * auto mode in their mode pickers. */ export function getAutoModeEnabledState(): AutoModeEnabledState {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/permissions/permissionSetup.ts` around lines 1175 - 1195, The docstring for getAutoModeEnabledState is stale — AUTO_MODE_ENABLED_DEFAULT is 'enabled' now; update the comment above getAutoModeEnabledState to state that it defaults to 'enabled' (not 'disabled') when GrowthBook is unavailable or the field is unset, and keep the existing note that the value is cached/stale; reference AUTO_MODE_ENABLED_DEFAULT, parseAutoModeEnabledState, and getFeatureValue_CACHED_MAY_BE_STALE to ensure the comment aligns with the actual fallback behavior implemented by the function.
1140-1234:⚠️ Potential issue | 🟠 MajorPolicy bypass confirmed: Shift+Tab circumvents org-level bypass-permissions killswitch that CLI still enforces.
initialPermissionModeFromCLI(lines 699–711, 778–792) enforcestengu_disable_bypass_permissions_mode(Statsig) andsettings.permissions.disableBypassPermissionsMode, and refuses--dangerously-skip-permissionswith a user-facing notification. But at runtime:
isBypassPermissionsModeDisabled()always returnsfalse(1232–1234),shouldDisableBypassPermissions()always returnsfalse(1140–1142),createDisabledBypassPermissionsContext()is unreachable (never called when parent check returns false).Callers still invoke these functions:
AppState.tsx(line 89): checksisBypassPermissionsModeDisabled()on mount → no-opapplySettingsChange.ts(line 63): checksisBypassPermissionsModeDisabled()on settings change → no-opuseReplBridge.tsx(line 528): checksisBypassPermissionsModeDisabled()on shift-tab cycle → no-opcli/print.ts(line 4738): checksisBypassPermissionsModeDisabled()in REPL bridge mode switch → no-opResult: the same user whose
--dangerously-skip-permissionswas denied at startup can Shift+Tab through the cycle and land inbypassPermissionsanyway.Please confirm intent:
- If the killswitch is retired, remove CLI-side gate logic (lines 699–711, 778–792), the orphaned
createDisabledBypassPermissionsContext()helper, and the now-stale check in the settings schema for consistency.- If the killswitch should remain an enforcement mechanism, restore
isBypassPermissionsModeDisabled()to consultcheckStatsigFeatureGate_CACHED_MAY_BE_STALE('tengu_disable_bypass_permissions_mode')andsettings.permissions.disableBypassPermissionsMode(mirroring the CLI-time check).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/permissions/permissionSetup.ts` around lines 1140 - 1234, The current bypass-permissions killswitch is effectively disabled because isBypassPermissionsModeDisabled() and shouldDisableBypassPermissions() always return false; restore enforcement by making these functions consult the cached Statsig gate and the settings flag: call checkStatsigFeatureGate_CACHED_MAY_BE_STALE('tengu_disable_bypass_permissions_mode') and read settings.permissions.disableBypassPermissionsMode (or the existing settings accessor used elsewhere) and return true if either indicates disabling; implement shouldDisableBypassPermissions() to return a Promise<boolean> that resolves the same combined result (or await any async gate check if available), leave createDisabledBypassPermissionsContext() reachable when the combined check is true, and keep callers (AppState.tsx, applySettingsChange.ts, useReplBridge.tsx, cli/print.ts) working without further changes.
🧹 Nitpick comments (3)
src/utils/sideQuery.ts (1)
193-253: End the trace in afinallypath after a successful response.If any success-path processing between Line 217 and Line 252 throws, the trace created at Line 185 is never ended. Keep the existing request error handling, but wrap post-response work so successful API calls always close their span.
🔒 Proposed trace lifecycle fix
} catch (error) { endTrace(langfuseTrace, undefined, 'error') throw error } - const requestId = - (response as { _request_id?: string | null })._request_id ?? undefined - const now = Date.now() - const lastCompletion = getLastApiCompletionTimestamp() - logEvent('tengu_api_success', { - requestId: - requestId as AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, - querySource: - opts.querySource as AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, - model: - normalizedModel as AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, - inputTokens: response.usage.input_tokens, - outputTokens: response.usage.output_tokens, - cachedInputTokens: response.usage.cache_read_input_tokens ?? 0, - uncachedInputTokens: response.usage.cache_creation_input_tokens ?? 0, - durationMsIncludingRetries: now - start, - timeSinceLastApiCallMs: - lastCompletion !== null ? now - lastCompletion : undefined, - }) - setLastApiCompletionTimestamp(now) - - // Record LLM observation in Langfuse (no-op if not configured) - recordLLMObservation(langfuseTrace, { - model: normalizedModel, - provider, - input: messages, - output: response.content, - usage: { - input_tokens: response.usage.input_tokens, - output_tokens: response.usage.output_tokens, - cache_creation_input_tokens: response.usage.cache_creation_input_tokens ?? undefined, - cache_read_input_tokens: response.usage.cache_read_input_tokens ?? undefined, - }, - startTime: new Date(start), - endTime: new Date(), - }) - endTrace(langfuseTrace) + try { + const requestId = + (response as { _request_id?: string | null })._request_id ?? undefined + const now = Date.now() + const lastCompletion = getLastApiCompletionTimestamp() + logEvent('tengu_api_success', { + requestId: + requestId as AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, + querySource: + opts.querySource as AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, + model: + normalizedModel as AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, + inputTokens: response.usage.input_tokens, + outputTokens: response.usage.output_tokens, + cachedInputTokens: response.usage.cache_read_input_tokens ?? 0, + uncachedInputTokens: response.usage.cache_creation_input_tokens ?? 0, + durationMsIncludingRetries: now - start, + timeSinceLastApiCallMs: + lastCompletion !== null ? now - lastCompletion : undefined, + }) + setLastApiCompletionTimestamp(now) + + // Record LLM observation in Langfuse (no-op if not configured) + recordLLMObservation(langfuseTrace, { + model: normalizedModel, + provider, + input: messages, + output: response.content, + usage: { + input_tokens: response.usage.input_tokens, + output_tokens: response.usage.output_tokens, + cache_creation_input_tokens: + response.usage.cache_creation_input_tokens ?? undefined, + cache_read_input_tokens: + response.usage.cache_read_input_tokens ?? undefined, + }, + startTime: new Date(start), + endTime: new Date(), + }) + } finally { + endTrace(langfuseTrace) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/sideQuery.ts` around lines 193 - 253, The trace (langfuseTrace) must be ended even if post-response processing throws: keep the existing try/catch around client.beta.messages.create (which currently calls endTrace(langfuseTrace, undefined, 'error') on error), but wrap the post-response work—everything that reads response and calls logEvent, setLastApiCompletionTimestamp, recordLLMObservation, etc.—in a try/finally (or add a finally after the outer try/catch) that calls endTrace(langfuseTrace) so the span is always closed after a successful API call; reference the client.beta.messages.create call, the response handling block, logEvent, setLastApiCompletionTimestamp, recordLLMObservation, and endTrace.src/utils/permissions/bypassPermissionsKillswitch.ts (1)
16-41: Nit: prefer a top-levelimport type { AppState }over inlineimport()types.The inline
import('../../state/AppState.js').AppStateappears twice in parameter signatures and hurts readability. A namedimport typeis idiomatic and consistent with howToolPermissionContextis already imported (line 8).♻️ Proposed refactor
-import { useAppState, useAppStateStore, useSetAppState } from '../../state/AppState.js' +import type { AppState } from '../../state/AppState.js' +import { useAppState, useAppStateStore, useSetAppState } from '../../state/AppState.js' import type { ToolPermissionContext } from '../../Tool.js' @@ export async function checkAndDisableBypassPermissionsIfNeeded( _toolPermissionContext: ToolPermissionContext, - _setAppState: (f: (prev: import('../../state/AppState.js').AppState) => import('../../state/AppState.js').AppState) => void, + _setAppState: (f: (prev: AppState) => AppState) => void, ): Promise<void> { @@ export async function checkAndDisableAutoModeIfNeeded( toolPermissionContext: ToolPermissionContext, - setAppState: (f: (prev: import('../../state/AppState.js').AppState) => import('../../state/AppState.js').AppState) => void, + setAppState: (f: (prev: AppState) => AppState) => void, fastMode?: boolean,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/permissions/bypassPermissionsKillswitch.ts` around lines 16 - 41, Replace the inline import() types with a top-level type import: add "import type { AppState } from '../../state/AppState.js'" and update all function signatures that currently use "import('../../state/AppState.js').AppState" to use the imported AppState type (e.g., in checkAndDisableBypassPermissionsIfNeeded, checkAndDisableAutoModeIfNeeded, and the setAppState parameter types). Leave the runtime imports/logic unchanged (these are only type changes) and ensure other symbols (ToolPermissionContext, autoModeCheckRan, resetBypassPermissionsCheck, useKickOffCheckAndDisableBypassPermissionsIfNeeded) remain as-is.src/components/PromptInput/PromptInput.tsx (1)
1879-1889: RedundantgetNextPermissionModecall.
cyclePermissionModealready invokesgetNextPermissionModeinternally (seegetNextPermissionMode.ts:56) and returnsnextModein its result — the separate call on line 1882 duplicates that work. DestructuringnextModefromcyclePermissionModeis both simpler and guarantees the two stay in sync.♻️ Proposed refactor
- // Compute the next mode without triggering side effects first - logForDebugging( - `[auto-mode] handleCycleMode: currentMode=${toolPermissionContext.mode}`, - ) - const nextMode = getNextPermissionMode(toolPermissionContext, teamContext) - - // Call cyclePermissionMode to apply side effects (e.g. strip - // dangerous permissions, activate classifier) - const { context: preparedContext } = cyclePermissionMode( - toolPermissionContext, - teamContext, - ) + logForDebugging( + `[auto-mode] handleCycleMode: currentMode=${toolPermissionContext.mode}`, + ) + // cyclePermissionMode computes nextMode AND applies side effects + // (e.g. strip dangerous permissions, activate classifier) + const { nextMode, context: preparedContext } = cyclePermissionMode( + toolPermissionContext, + teamContext, + )If
getNextPermissionModeis no longer needed here, also drop it from the import on line 155-157.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/PromptInput/PromptInput.tsx` around lines 1879 - 1889, The call to getNextPermissionMode is redundant because cyclePermissionMode already computes and returns nextMode; replace the separate getNextPermissionMode(toolPermissionContext, teamContext) call by destructuring nextMode from the result of cyclePermissionMode (e.g., const { context: preparedContext, nextMode } = cyclePermissionMode(toolPermissionContext, teamContext)) and use that nextMode going forward, and if getNextPermissionMode is no longer referenced elsewhere in this file remove it from the imports (original symbols to update: getNextPermissionMode, cyclePermissionMode, toolPermissionContext, teamContext, preparedContext, nextMode).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/onChangeAppState.ts`:
- Around line 93-99: AppState.mainLoopModel can be null but
setMainLoopModelOverride expects undefined to mean “no override”, so normalize
the value before calling it: in the onChangeAppState handler check
newState.mainLoopModel and convert null to undefined (e.g., use a local value
like modelOverride = newState.mainLoopModel ?? undefined) and pass that into
setMainLoopModelOverride; this preserves the intended semantic distinction
relied on by getUserSpecifiedModelSetting and any checks against !== undefined.
In `@src/utils/permissions/__tests__/getNextPermissionMode.test.ts`:
- Around line 101-142: The test mutates process.env.USER_TYPE but restores it
manually; change the test to capture originalUserType, then wrap the mutation
and assertions inside a try/finally so the original USER_TYPE is always restored
in the finally block; update the describe/test that calls getNextPermissionMode
and makeContext to use this try/finally pattern (restore originalUserType or
delete when undefined) to prevent leaking env changes to other tests.
- Around line 14-17: Update the stale inline-comment above the import of
getNextPermissionMode in the test: replace the note claiming the function is
inlined with a brief accurate comment (or remove it) that reflects that the test
now imports getNextPermissionMode rather than copying it; ensure the reference
to getNextPermissionMode remains and the comment no longer mentions the heavy
permissionSetup dependency or inlining.
- Line 11: The import for ToolPermissionContext in the test is pointing to the
wrong module; change the import in getNextPermissionMode.test.ts to import
ToolPermissionContext from ../../../Tool.js (matching src/Tool.ts compiled path)
so it resolves correctly, and then update the nearby comment that claims the
function is inlined (lines around the import) — either remove the outdated
comment or reword it to reflect that the function is being imported directly;
ensure references to ToolPermissionContext remain unchanged in the test.
In `@src/utils/permissions/__tests__/permissionSetup.test.ts`:
- Line 15: The import path for ToolPermissionContext in permissionSetup.test.ts
is incorrect and causing TS2307; update the import statement that currently
references '../../Tool.js' to the correct relative path '../../../Tool.js' so
that the named type ToolPermissionContext resolves to the actual Tool.js module
used elsewhere in this test file (e.g., where ToolPermissionContext is
referenced later on line 137).
In `@src/utils/sideQuery.ts`:
- Around line 239-249: The call to recordLLMObservation is currently sending raw
prompts and outputs (messages and response.content) to Langfuse; change this to
a safer default by redacting or truncating those fields or gating them behind an
explicit config flag (e.g., captureRawPayloads). Update the recordLLMObservation
invocation (referencing langfuseTrace, normalizedModel, provider, messages,
response.content) so that when captureRawPayloads is false it sends
redacted_input and redacted_output (or truncated versions) instead of messages
and response.content, and when true it forwards the full payloads; ensure the
new behavior is driven by a clearly named config/environment setting checked
before assembling the usage object.
---
Outside diff comments:
In `@src/utils/permissions/permissionSetup.ts`:
- Around line 1175-1195: The docstring for getAutoModeEnabledState is stale —
AUTO_MODE_ENABLED_DEFAULT is 'enabled' now; update the comment above
getAutoModeEnabledState to state that it defaults to 'enabled' (not 'disabled')
when GrowthBook is unavailable or the field is unset, and keep the existing note
that the value is cached/stale; reference AUTO_MODE_ENABLED_DEFAULT,
parseAutoModeEnabledState, and getFeatureValue_CACHED_MAY_BE_STALE to ensure the
comment aligns with the actual fallback behavior implemented by the function.
- Around line 1140-1234: The current bypass-permissions killswitch is
effectively disabled because isBypassPermissionsModeDisabled() and
shouldDisableBypassPermissions() always return false; restore enforcement by
making these functions consult the cached Statsig gate and the settings flag:
call
checkStatsigFeatureGate_CACHED_MAY_BE_STALE('tengu_disable_bypass_permissions_mode')
and read settings.permissions.disableBypassPermissionsMode (or the existing
settings accessor used elsewhere) and return true if either indicates disabling;
implement shouldDisableBypassPermissions() to return a Promise<boolean> that
resolves the same combined result (or await any async gate check if available),
leave createDisabledBypassPermissionsContext() reachable when the combined check
is true, and keep callers (AppState.tsx, applySettingsChange.ts,
useReplBridge.tsx, cli/print.ts) working without further changes.
In `@src/utils/settings/settings.ts`:
- Around line 891-899: The JSDoc above hasAutoModeOptIn() is outdated and
mentions trusted settings, opt-in dialog, and RCE risk even though the function
now unconditionally returns true; update the comment to accurately describe
current behavior: state that auto mode is universally available (no opt-in),
that the function always returns true, and remove references to trusted settings
sources, projectSettings, opt-in dialog, or security rationale; edit the comment
block immediately above the hasAutoModeOptIn declaration to a concise one-line
summary reflecting this behavior.
---
Nitpick comments:
In `@src/components/PromptInput/PromptInput.tsx`:
- Around line 1879-1889: The call to getNextPermissionMode is redundant because
cyclePermissionMode already computes and returns nextMode; replace the separate
getNextPermissionMode(toolPermissionContext, teamContext) call by destructuring
nextMode from the result of cyclePermissionMode (e.g., const { context:
preparedContext, nextMode } = cyclePermissionMode(toolPermissionContext,
teamContext)) and use that nextMode going forward, and if getNextPermissionMode
is no longer referenced elsewhere in this file remove it from the imports
(original symbols to update: getNextPermissionMode, cyclePermissionMode,
toolPermissionContext, teamContext, preparedContext, nextMode).
In `@src/utils/permissions/bypassPermissionsKillswitch.ts`:
- Around line 16-41: Replace the inline import() types with a top-level type
import: add "import type { AppState } from '../../state/AppState.js'" and update
all function signatures that currently use
"import('../../state/AppState.js').AppState" to use the imported AppState type
(e.g., in checkAndDisableBypassPermissionsIfNeeded,
checkAndDisableAutoModeIfNeeded, and the setAppState parameter types). Leave the
runtime imports/logic unchanged (these are only type changes) and ensure other
symbols (ToolPermissionContext, autoModeCheckRan, resetBypassPermissionsCheck,
useKickOffCheckAndDisableBypassPermissionsIfNeeded) remain as-is.
In `@src/utils/sideQuery.ts`:
- Around line 193-253: The trace (langfuseTrace) must be ended even if
post-response processing throws: keep the existing try/catch around
client.beta.messages.create (which currently calls endTrace(langfuseTrace,
undefined, 'error') on error), but wrap the post-response work—everything that
reads response and calls logEvent, setLastApiCompletionTimestamp,
recordLLMObservation, etc.—in a try/finally (or add a finally after the outer
try/catch) that calls endTrace(langfuseTrace) so the span is always closed after
a successful API call; reference the client.beta.messages.create call, the
response handling block, logEvent, setLastApiCompletionTimestamp,
recordLLMObservation, and endTrace.
🪄 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: 9957e89d-fd9b-432e-bbbe-f1ed7e89b2d3
📒 Files selected for processing (17)
src/Tool.tssrc/__tests__/Tool.test.tssrc/commands/login/login.tsxsrc/components/PromptInput/PromptInput.tsxsrc/interactiveHelpers.tsxsrc/main.tsxsrc/screens/REPL.tsxsrc/services/acp/__tests__/agent.test.tssrc/services/tips/tipRegistry.tssrc/state/onChangeAppState.tssrc/utils/permissions/__tests__/getNextPermissionMode.test.tssrc/utils/permissions/__tests__/permissionSetup.test.tssrc/utils/permissions/bypassPermissionsKillswitch.tssrc/utils/permissions/getNextPermissionMode.tssrc/utils/permissions/permissionSetup.tssrc/utils/settings/settings.tssrc/utils/sideQuery.ts
💤 Files with no reviewable changes (2)
- src/interactiveHelpers.tsx
- src/main.tsx
Comment on lines
+93
to
99
| // mainLoopModel: session-scoped only (do NOT persist to userSettings). | ||
| // Writing to settings.json would leak model changes into other running | ||
| // sessions (anthropics/claude-code#37596). Each process keeps its own | ||
| // model override in memory via setMainLoopModelOverride. | ||
| if (newState.mainLoopModel !== oldState.mainLoopModel) { | ||
| setMainLoopModelOverride(newState.mainLoopModel) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the override setter signature and the nullable AppState field/call sites.
rg -n -C4 'setMainLoopModelOverride|mainLoopModel'Repository: claude-code-best/claude-code
Length of output: 50387
🏁 Script executed:
#!/bin/bash
# Find the definition of setMainLoopModelOverride and its parameter type
rg -n "export.*setMainLoopModelOverride|function setMainLoopModelOverride|const setMainLoopModelOverride" -A 5Repository: claude-code-best/claude-code
Length of output: 361
🏁 Script executed:
#!/bin/bash
# Find ModelSetting type definition
rg -n "type ModelSetting|interface ModelSetting" -A 3Repository: claude-code-best/claude-code
Length of output: 1151
🏁 Script executed:
#!/bin/bash
# Check bootstrap/state file for the function definition
fd -n "state\.(ts|js)" src/bootstrap/ | head -20Repository: claude-code-best/claude-code
Length of output: 302
🏁 Script executed:
#!/bin/bash
# Check how getMainLoopModelOverride is used and its return type
rg -n "getMainLoopModelOverride" -B 2 -A 2 | head -100Repository: claude-code-best/claude-code
Length of output: 3874
🏁 Script executed:
#!/bin/bash
# Look for checks of getMainLoopModelOverride() !== undefined to see the semantic distinction
rg -n "getMainLoopModelOverride.*!==.*undefined|getMainLoopModelOverride.*!==\s*null" -B 2 -A 2Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Verify the return type of getMainLoopModelOverride and how it's actually used in the codebase
rg -n "const.*modelOverride.*=.*getMainLoopModelOverride\(\)" -A 3 | head -60Repository: claude-code-best/claude-code
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Check if there are any type errors running typecheck on the file
# First, find if there's a tsconfig or package.json that specifies strict mode
cd src/state && head -50 ../../tsconfig.json 2>/dev/null || head -50 ../../package.json 2>/dev/null | grep -A 20 "typescript\|strict"Repository: claude-code-best/claude-code
Length of output: 1322
🏁 Script executed:
#!/bin/bash
# Check how STATE.mainLoopModelOverride is actually used after being set
rg -n "STATE\.mainLoopModelOverride|mainLoopModelOverride" src/bootstrap/state.ts -B 2 -A 2Repository: claude-code-best/claude-code
Length of output: 679
🏁 Script executed:
#!/bin/bash
# Look for the actual initialization or usage of mainLoopModelOverride in the state
rg -n "mainLoopModelOverride\s*=" src/bootstrap/state.tsRepository: claude-code-best/claude-code
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Check what happens when mainLoopModel is set to null - any test fixtures or real code paths
rg -n "mainLoopModel.*null" src/screens/REPL.tsx | head -20Repository: claude-code-best/claude-code
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Look for how the override is actually used after being set via setMainLoopModelOverride
rg -n "STATE\.mainLoopModelOverride" src/bootstrap/state.ts -B 1 -A 1 | grep -v "^--$"Repository: claude-code-best/claude-code
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Check if there's actual divergence in how null vs undefined is handled for model overrides
rg -n "mainLoopModelOverride.*null|mainLoopModelOverride.*undefined" src/Repository: claude-code-best/claude-code
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Let me understand the semantic difference by checking how the override is actually consumed
# Look at all callers that check the override result
rg -n "getMainLoopModelOverride" -B 3 -A 3 | grep -E "(if|!==|===|\s\?\s)" -A 2 -B 2 | head -80Repository: claude-code-best/claude-code
Length of output: 2759
🏁 Script executed:
#!/bin/bash
# Check the specific line in onChangeAppState.ts to see the full context
sed -n '85,105p' src/state/onChangeAppState.tsRepository: claude-code-best/claude-code
Length of output: 929
🏁 Script executed:
#!/bin/bash
# Verify the critical check at src/utils/model/model.ts:75 in context
sed -n '70,85p' src/utils/model/model.tsRepository: claude-code-best/claude-code
Length of output: 636
🏁 Script executed:
#!/bin/bash
# Double-check what happens when mainLoopModel changes to null in the UI
rg -n "setAppState.*mainLoopModel.*null" -B 2 -A 2Repository: claude-code-best/claude-code
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check if strict type checking would catch this - look at the exact line in context
sed -n '93,99p' src/state/onChangeAppState.ts | cat -nRepository: claude-code-best/claude-code
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Verify the semantic issue by seeing what happens when null reaches getUserSpecifiedModelSetting
# The key issue: if modelOverride is null, line 75 check (modelOverride !== undefined) returns true
# So null gets treated as an active override instead of "no override"
rg -n "getUserSpecifiedModelSetting" -B 2 -A 2 | head -50Repository: claude-code-best/claude-code
Length of output: 3121
🏁 Script executed:
#!/bin/bash
# Final confirmation: check line 104 in model.ts to see the full logic
sed -n '100,115p' src/utils/model/model.tsRepository: claude-code-best/claude-code
Length of output: 574
Normalize cleared model overrides to undefined.
setMainLoopModelOverride expects undefined to indicate "no override" per its contract, but AppState.mainLoopModel can be null. Passing null causes it to be treated as an active override by code checking !== undefined (e.g., in getUserSpecifiedModelSetting()). This breaks the semantic distinction between "no override" and "an active override is set."
🐛 Proposed fix
if (newState.mainLoopModel !== oldState.mainLoopModel) {
- setMainLoopModelOverride(newState.mainLoopModel)
+ setMainLoopModelOverride(newState.mainLoopModel ?? undefined)
}📝 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
| // mainLoopModel: session-scoped only (do NOT persist to userSettings). | |
| // Writing to settings.json would leak model changes into other running | |
| // sessions (anthropics/claude-code#37596). Each process keeps its own | |
| // model override in memory via setMainLoopModelOverride. | |
| if (newState.mainLoopModel !== oldState.mainLoopModel) { | |
| setMainLoopModelOverride(newState.mainLoopModel) | |
| } | |
| // mainLoopModel: session-scoped only (do NOT persist to userSettings). | |
| // Writing to settings.json would leak model changes into other running | |
| // sessions (anthropics/claude-code#37596). Each process keeps its own | |
| // model override in memory via setMainLoopModelOverride. | |
| if (newState.mainLoopModel !== oldState.mainLoopModel) { | |
| setMainLoopModelOverride(newState.mainLoopModel ?? undefined) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/onChangeAppState.ts` around lines 93 - 99, AppState.mainLoopModel
can be null but setMainLoopModelOverride expects undefined to mean “no
override”, so normalize the value before calling it: in the onChangeAppState
handler check newState.mainLoopModel and convert null to undefined (e.g., use a
local value like modelOverride = newState.mainLoopModel ?? undefined) and pass
that into setMainLoopModelOverride; this preserves the intended semantic
distinction relied on by getUserSpecifiedModelSetting and any checks against !==
undefined.
| * distinction — all users share the same cycle order. | ||
| */ | ||
| import { describe, expect, test } from 'bun:test' | ||
| import type { ToolPermissionContext } from '../../Tool.js' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
test_file = Path("src/utils/permissions/__tests__/getNextPermissionMode.test.ts")
target = Path("src/Tool.ts")
print("current import line:")
print(test_file.read_text().splitlines()[10])
print("expected JS import path from test dir:")
print(Path("../../../Tool.js"))
print("resolved target exists:", target.exists())
PYRepository: claude-code-best/claude-code
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Read the test file to check all mentioned issues
head -20 src/utils/permissions/__tests__/getNextPermissionMode.test.ts
echo "---"
sed -n '100,145p' src/utils/permissions/__tests__/getNextPermissionMode.test.tsRepository: claude-code-best/claude-code
Length of output: 2225
Fix the ToolPermissionContext import path.
Line 11 imports from ../../Tool.js, which resolves to a non-existent file. The actual file is at src/Tool.ts, requiring the path ../../../Tool.js.
🐛 Proposed fix
-import type { ToolPermissionContext } from '../../Tool.js'
+import type { ToolPermissionContext } from '../../../Tool.js'Also update the comment on lines 14-17: it states the function is inlined to avoid imports, but the next line imports the function directly. Remove or clarify the comment.
🧰 Tools
🪛 GitHub Actions: CI
[error] 11-11: TypeScript (tsc --noEmit) failed with TS2307: Cannot find module '../../Tool.js' or its corresponding type declarations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/permissions/__tests__/getNextPermissionMode.test.ts` at line 11,
The import for ToolPermissionContext in the test is pointing to the wrong
module; change the import in getNextPermissionMode.test.ts to import
ToolPermissionContext from ../../../Tool.js (matching src/Tool.ts compiled path)
so it resolves correctly, and then update the nearby comment that claims the
function is inlined (lines around the import) — either remove the outdated
comment or reword it to reflect that the function is being imported directly;
ensure references to ToolPermissionContext remain unchanged in the test.
Comment on lines
+14
to
+17
| // Inline getNextPermissionMode to avoid importing the heavy permissionSetup | ||
| // dependency chain (growthbook, settings, etc.). | ||
| // The function under test is small and pure enough to copy for testing. | ||
| import { getNextPermissionMode } from '../getNextPermissionMode.js' |
Contributor
There was a problem hiding this comment.
Update the stale “inline” comment.
The test imports getNextPermissionMode; it no longer inlines the function.
📝 Proposed fix
-// Inline getNextPermissionMode to avoid importing the heavy permissionSetup
-// dependency chain (growthbook, settings, etc.).
-// The function under test is small and pure enough to copy for testing.
+// Import getNextPermissionMode directly; this module is small and pure and
+// avoids the heavier permissionSetup dependency chain.
import { getNextPermissionMode } from '../getNextPermissionMode.js'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/permissions/__tests__/getNextPermissionMode.test.ts` around lines
14 - 17, Update the stale inline-comment above the import of
getNextPermissionMode in the test: replace the note claiming the function is
inlined with a brief accurate comment (or remove it) that reflects that the test
now imports getNextPermissionMode rather than copying it; ensure the reference
to getNextPermissionMode remains and the comment no longer mentions the heavy
permissionSetup dependency or inlining.
Comment on lines
+101
to
+142
| describe('no USER_TYPE distinction', () => { | ||
| test('cycle order is the same regardless of USER_TYPE', () => { | ||
| // Save original | ||
| const originalUserType = process.env.USER_TYPE | ||
|
|
||
| // Test with no USER_TYPE | ||
| delete process.env.USER_TYPE | ||
| const cycleNoType: PermissionMode[] = [] | ||
| let ctx = makeContext('default') | ||
| for (let i = 0; i < 5; i++) { | ||
| const next = getNextPermissionMode(ctx) | ||
| cycleNoType.push(next) | ||
| ctx = makeContext(next) | ||
| } | ||
|
|
||
| // Test with USER_TYPE=ant | ||
| process.env.USER_TYPE = 'ant' | ||
| const cycleAnt: PermissionMode[] = [] | ||
| ctx = makeContext('default') | ||
| for (let i = 0; i < 5; i++) { | ||
| const next = getNextPermissionMode(ctx) | ||
| cycleAnt.push(next) | ||
| ctx = makeContext(next) | ||
| } | ||
|
|
||
| // Restore | ||
| if (originalUserType !== undefined) { | ||
| process.env.USER_TYPE = originalUserType | ||
| } else { | ||
| delete process.env.USER_TYPE | ||
| } | ||
|
|
||
| // Both should produce the same cycle | ||
| expect(cycleNoType).toEqual(cycleAnt) | ||
| expect(cycleNoType).toEqual([ | ||
| 'acceptEdits', | ||
| 'plan', | ||
| 'auto', | ||
| 'bypassPermissions', | ||
| 'default', | ||
| ]) | ||
| }) |
Contributor
There was a problem hiding this comment.
Restore USER_TYPE in a finally block.
If this test throws before the manual restore, later tests can inherit USER_TYPE='ant' or an unset value.
🧪 Proposed fix
test('cycle order is the same regardless of USER_TYPE', () => {
// Save original
const originalUserType = process.env.USER_TYPE
- // Test with no USER_TYPE
- delete process.env.USER_TYPE
- const cycleNoType: PermissionMode[] = []
- let ctx = makeContext('default')
- for (let i = 0; i < 5; i++) {
- const next = getNextPermissionMode(ctx)
- cycleNoType.push(next)
- ctx = makeContext(next)
- }
-
- // Test with USER_TYPE=ant
- process.env.USER_TYPE = 'ant'
- const cycleAnt: PermissionMode[] = []
- ctx = makeContext('default')
- for (let i = 0; i < 5; i++) {
- const next = getNextPermissionMode(ctx)
- cycleAnt.push(next)
- ctx = makeContext(next)
- }
-
- // Restore
- if (originalUserType !== undefined) {
- process.env.USER_TYPE = originalUserType
- } else {
- delete process.env.USER_TYPE
- }
-
- // Both should produce the same cycle
- expect(cycleNoType).toEqual(cycleAnt)
- expect(cycleNoType).toEqual([
- 'acceptEdits',
- 'plan',
- 'auto',
- 'bypassPermissions',
- 'default',
- ])
+ try {
+ // Test with no USER_TYPE
+ delete process.env.USER_TYPE
+ const cycleNoType: PermissionMode[] = []
+ let ctx = makeContext('default')
+ for (let i = 0; i < 5; i++) {
+ const next = getNextPermissionMode(ctx)
+ cycleNoType.push(next)
+ ctx = makeContext(next)
+ }
+
+ // Test with USER_TYPE=ant
+ process.env.USER_TYPE = 'ant'
+ const cycleAnt: PermissionMode[] = []
+ ctx = makeContext('default')
+ for (let i = 0; i < 5; i++) {
+ const next = getNextPermissionMode(ctx)
+ cycleAnt.push(next)
+ ctx = makeContext(next)
+ }
+
+ // Both should produce the same cycle
+ expect(cycleNoType).toEqual(cycleAnt)
+ expect(cycleNoType).toEqual([
+ 'acceptEdits',
+ 'plan',
+ 'auto',
+ 'bypassPermissions',
+ 'default',
+ ])
+ } finally {
+ if (originalUserType !== undefined) {
+ process.env.USER_TYPE = originalUserType
+ } else {
+ delete process.env.USER_TYPE
+ }
+ }
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/permissions/__tests__/getNextPermissionMode.test.ts` around lines
101 - 142, The test mutates process.env.USER_TYPE but restores it manually;
change the test to capture originalUserType, then wrap the mutation and
assertions inside a try/finally so the original USER_TYPE is always restored in
the finally block; update the describe/test that calls getNextPermissionMode and
makeContext to use this try/finally pattern (restore originalUserType or delete
when undefined) to prevent leaking env changes to other tests.
Comment on lines
+239
to
+249
| recordLLMObservation(langfuseTrace, { | ||
| model: normalizedModel, | ||
| provider, | ||
| input: messages, | ||
| output: response.content, | ||
| usage: { | ||
| input_tokens: response.usage.input_tokens, | ||
| output_tokens: response.usage.output_tokens, | ||
| cache_creation_input_tokens: response.usage.cache_creation_input_tokens ?? undefined, | ||
| cache_read_input_tokens: response.usage.cache_read_input_tokens ?? undefined, | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid exporting raw side-query content to Langfuse by default.
Line 242 and Line 243 send full prompts and model outputs to Langfuse. Side queries can include user/session/code content, so this should be redacted, truncated, or gated behind an explicit “capture raw payloads” setting.
🛡️ Proposed safer default
recordLLMObservation(langfuseTrace, {
model: normalizedModel,
provider,
- input: messages,
- output: response.content,
+ input: {
+ querySource: opts.querySource,
+ messageCount: messages.length,
+ },
+ output: {
+ contentBlockCount: response.content.length,
+ },
usage: {
input_tokens: response.usage.input_tokens,
output_tokens: response.usage.output_tokens,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/sideQuery.ts` around lines 239 - 249, The call to
recordLLMObservation is currently sending raw prompts and outputs (messages and
response.content) to Langfuse; change this to a safer default by redacting or
truncating those fields or gating them behind an explicit config flag (e.g.,
captureRawPayloads). Update the recordLLMObservation invocation (referencing
langfuseTrace, normalizedModel, provider, messages, response.content) so that
when captureRawPayloads is false it sends redacted_input and redacted_output (or
truncated versions) instead of messages and response.content, and when true it
forwards the full payloads; ensure the new behavior is driven by a clearly named
config/environment setting checked before assembling the usage object.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/acp/__tests__/agent.test.ts`:
- Around line 631-646: The new tests ('availableModes includes bypassPermissions
when not root' and 'can switch to bypassPermissions mode') assume bypass mode is
available; guard them by checking the same condition used in production
(isBypassAvailable via process.geteuid() !== 0 or process.env.IS_SANDBOX) or
force deterministic behavior by setting process.env.IS_SANDBOX = '1' before
calling AcpAgent.newSession and revert after; update the tests that call
AcpAgent.newSession, session.modes.availableModes and agent.setSessionMode to
either skip when running as root (process.geteuid && process.geteuid() === 0 &&
!process.env.IS_SANDBOX) or to set the env flag so bypassPermissions is present
and setSessionMode won't throw.
🪄 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: 287b2492-5d90-4ff5-9e02-5e25495448e1
📒 Files selected for processing (4)
src/services/acp/__tests__/agent.test.tssrc/services/acp/agent.tssrc/utils/permissions/__tests__/getNextPermissionMode.test.tssrc/utils/permissions/__tests__/permissionSetup.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/utils/permissions/tests/permissionSetup.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/permissions/tests/getNextPermissionMode.test.ts
Comment on lines
+631
to
+646
| test('availableModes includes bypassPermissions when not root', async () => { | ||
| const agent = new AcpAgent(makeConn()) | ||
| const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any) | ||
| const session = agent.sessions.get(sessionId) | ||
| const modeIds = session?.modes.availableModes.map((m: any) => m.id) | ||
| expect(modeIds).toContain('bypassPermissions') | ||
| }) | ||
|
|
||
| test('can switch to bypassPermissions mode', async () => { | ||
| const agent = new AcpAgent(makeConn()) | ||
| const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any) | ||
| await agent.setSessionMode({ sessionId, modeId: 'bypassPermissions' } as any) | ||
| const session = agent.sessions.get(sessionId) | ||
| expect(session?.modes.currentModeId).toBe('bypassPermissions') | ||
| expect(session?.appState.toolPermissionContext.mode).toBe('bypassPermissions') | ||
| }) |
Contributor
There was a problem hiding this comment.
Bypass tests will flake when the test runner executes as root.
Both new tests implicitly assume process.geteuid() !== 0 (or IS_SANDBOX set). In Docker/CI images that run as root without IS_SANDBOX=1, isBypassAvailable will be false, bypassPermissions will be absent from availableModes, and setSessionMode({ modeId: 'bypassPermissions' }) will throw Invalid mode — causing the second test to fail with a confusing error instead of being skipped.
Consider guarding the tests with the same condition used in production, or forcing it deterministically via process.env.IS_SANDBOX:
🛠️ Proposed fix
- test('availableModes includes bypassPermissions when not root', async () => {
+ test('availableModes includes bypassPermissions when not root', async () => {
+ const prev = process.env.IS_SANDBOX
+ process.env.IS_SANDBOX = '1'
const agent = new AcpAgent(makeConn())
const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any)
const session = agent.sessions.get(sessionId)
const modeIds = session?.modes.availableModes.map((m: any) => m.id)
expect(modeIds).toContain('bypassPermissions')
+ process.env.IS_SANDBOX = prev
})
test('can switch to bypassPermissions mode', async () => {
+ const prev = process.env.IS_SANDBOX
+ process.env.IS_SANDBOX = '1'
const agent = new AcpAgent(makeConn())
const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any)
await agent.setSessionMode({ sessionId, modeId: 'bypassPermissions' } as any)
const session = agent.sessions.get(sessionId)
expect(session?.modes.currentModeId).toBe('bypassPermissions')
expect(session?.appState.toolPermissionContext.mode).toBe('bypassPermissions')
+ process.env.IS_SANDBOX = prev
})📝 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
| test('availableModes includes bypassPermissions when not root', async () => { | |
| const agent = new AcpAgent(makeConn()) | |
| const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any) | |
| const session = agent.sessions.get(sessionId) | |
| const modeIds = session?.modes.availableModes.map((m: any) => m.id) | |
| expect(modeIds).toContain('bypassPermissions') | |
| }) | |
| test('can switch to bypassPermissions mode', async () => { | |
| const agent = new AcpAgent(makeConn()) | |
| const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any) | |
| await agent.setSessionMode({ sessionId, modeId: 'bypassPermissions' } as any) | |
| const session = agent.sessions.get(sessionId) | |
| expect(session?.modes.currentModeId).toBe('bypassPermissions') | |
| expect(session?.appState.toolPermissionContext.mode).toBe('bypassPermissions') | |
| }) | |
| test('availableModes includes bypassPermissions when not root', async () => { | |
| const prev = process.env.IS_SANDBOX | |
| process.env.IS_SANDBOX = '1' | |
| const agent = new AcpAgent(makeConn()) | |
| const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any) | |
| const session = agent.sessions.get(sessionId) | |
| const modeIds = session?.modes.availableModes.map((m: any) => m.id) | |
| expect(modeIds).toContain('bypassPermissions') | |
| process.env.IS_SANDBOX = prev | |
| }) | |
| test('can switch to bypassPermissions mode', async () => { | |
| const prev = process.env.IS_SANDBOX | |
| process.env.IS_SANDBOX = '1' | |
| const agent = new AcpAgent(makeConn()) | |
| const { sessionId } = await agent.newSession({ cwd: '/tmp' } as any) | |
| await agent.setSessionMode({ sessionId, modeId: 'bypassPermissions' } as any) | |
| const session = agent.sessions.get(sessionId) | |
| expect(session?.modes.currentModeId).toBe('bypassPermissions') | |
| expect(session?.appState.toolPermissionContext.mode).toBe('bypassPermissions') | |
| process.env.IS_SANDBOX = prev | |
| }) |
🤖 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 631 - 646, The new
tests ('availableModes includes bypassPermissions when not root' and 'can switch
to bypassPermissions mode') assume bypass mode is available; guard them by
checking the same condition used in production (isBypassAvailable via
process.geteuid() !== 0 or process.env.IS_SANDBOX) or force deterministic
behavior by setting process.env.IS_SANDBOX = '1' before calling
AcpAgent.newSession and revert after; update the tests that call
AcpAgent.newSession, session.modes.availableModes and agent.setSessionMode to
either skip when running as root (process.geteuid && process.geteuid() === 0 &&
!process.env.IS_SANDBOX) or to set the env flag so bypassPermissions is present
and setSessionMode won't throw.
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
Tests
Chores