feat(policy): map --yolo to allowedTools wildcard policy#2
Open
laith-max wants to merge 1 commit into
Open
Conversation
This PR maps the `--yolo` flag natively into a wildcard policy array (`allowedTools: ["*"]`) and removes the concept of `ApprovalMode.YOLO` as a distinct state in the application, fulfilling issue google-gemini#11303. This removes the hardcoded `ApprovalMode.YOLO` state and its associated UI/bypasses. The `PolicyEngine` now evaluates YOLO purely via data-driven rules. - Removes `ApprovalMode.YOLO` - Removes UI toggle (`Ctrl+Y`) and indicators for YOLO - Removes `yolo.toml` - Updates A2A server and CLI config logic to translate YOLO into a wildcard tool - Rewrites policy engine tests to evaluate the wildcard - Enforces enterprise `disableYoloMode` and `secureModeEnabled` controls by actively preventing manual `--allowed-tools=*` bypasses. Fixes google-gemini#11303
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRemoves the YOLO approval mode and replaces it with wildcard allowed-tools. Updates policy engine logic, CLI/UI indicators and keybindings, server/config behavior, prompts, docs, tests, and removes a telemetry event and worktree/binary paths. ChangesApproval mode and policy engine overhaul
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/reference/configuration.md (1)
115-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale
--approval-mode=yoloreference from defaultApprovalMode docs.This now contradicts the new mode set (
default,auto_edit,plan) and can mislead users about valid CLI arguments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/configuration.md` around lines 115 - 116, Update the documentation text in the defaultApprovalMode section to remove the stale reference to "--yolo" and "--approval-mode=yolo" and replace it with the current valid modes and CLI usage; specifically, remove the phrase "YOLO mode (auto-approve all actions) can only be enabled via command line (--yolo or --approval-mode=yolo)" and instead mention the supported approval modes ("default", "auto_edit", "plan") and how to set them via the CLI using the --approval-mode flag (e.g., --approval-mode=auto_edit). Ensure the change is applied to the paragraph that currently mentions read-only/YOLO mode so the docs no longer reference the deprecated --yolo flag.packages/cli/src/config/config.ts (1)
564-620:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUntrusted-folder safety is bypassed by wildcard YOLO mapping.
With the new flow,
yolosetsapprovalModeto default and injects'*'intoallowedTools. The trust gate later only checksapprovalMode !== DEFAULT, so in untrusted folders the wildcard can remain active.Proposed fix
- // Force approval mode to default if the folder is not trusted. - if (!trustedFolder && approvalMode !== ApprovalMode.DEFAULT) { - debugLogger.warn( - `Approval mode overridden to "default" because the current folder is not trusted.`, - ); - approvalMode = ApprovalMode.DEFAULT; - } + // Force safe behavior if the folder is not trusted. + if (!trustedFolder) { + if (approvalMode !== ApprovalMode.DEFAULT || allowedTools.includes('*')) { + debugLogger.warn( + 'Approval mode and wildcard tool allowances were overridden because the current folder is not trusted.', + ); + } + approvalMode = ApprovalMode.DEFAULT; + allowedTools = allowedTools.filter((t) => t !== '*'); + isYoloRequested = false; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/config/config.ts` around lines 564 - 620, The bug is that rawApprovalMode 'yolo' sets approvalMode = ApprovalMode.DEFAULT but uses isYoloRequested to inject '*' into allowedTools, which lets untrusted-folder checks (they gate on approvalMode !== DEFAULT) be bypassed; change the 'yolo' branch so it sets a non-default approvalMode (e.g., ApprovalMode.ALLOWED_TOOLS or ApprovalMode.YOLO) instead of ApprovalMode.DEFAULT and keep isYoloRequested semantics so the later trust gate (which checks approvalMode !== DEFAULT) will correctly treat YOLO as a non-default, approved mode; update the switch case for rawApprovalMode === 'yolo' (and any dependent enum/consumers of ApprovalMode) to use that new/non-default ApprovalMode value and ensure allowedTools wildcard injection still only happens when approvalMode !== ApprovalMode.DEFAULT.packages/cli/src/ui/hooks/useApprovalModeIndicator.ts (1)
43-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a fallback for unsupported approval mode values in cycling.
When
config.getApprovalMode()returns an unexpected value, Shift+Tab currently does nothing. Falling back toApprovalMode.DEFAULTkeeps mode cycling recoverable.💡 Proposed fix
switch (currentMode) { case ApprovalMode.DEFAULT: nextApprovalMode = ApprovalMode.AUTO_EDIT; break; case ApprovalMode.AUTO_EDIT: nextApprovalMode = allowPlanMode ? ApprovalMode.PLAN : ApprovalMode.DEFAULT; break; case ApprovalMode.PLAN: nextApprovalMode = ApprovalMode.DEFAULT; break; default: + nextApprovalMode = ApprovalMode.DEFAULT; + break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/ui/hooks/useApprovalModeIndicator.ts` around lines 43 - 57, The switch over currentMode in useApprovalModeIndicator doesn't handle unexpected values, so add a fallback in the default branch to set nextApprovalMode = ApprovalMode.DEFAULT (using the same ApprovalMode symbol) so that when config.getApprovalMode() returns an unsupported value the cycle recovers; update the default case where currentMode is switched (referencing currentMode, nextApprovalMode, ApprovalMode, and allowPlanMode) to assign ApprovalMode.DEFAULT as the nextApprovalMode.packages/core/src/policy/policy-engine.test.ts (1)
1754-1782:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign wildcard parse-failure expectation with current engine fallback.
At Line 1779, this test expects
ALLOW, butPolicyEngine.checkShellCommand()returnsdefaultDecisionwhen parsing fails for non-DENY matches, which isASK_USERin this setup. This assertion is inconsistent with current behavior.💡 Suggested test fix
- it('should return ALLOW when using wildcard policy even if shell command parsing fails', async () => { + it('should return ASK_USER when shell command parsing fails even with wildcard policy', async () => { @@ - expect(result.decision).toBe(PolicyDecision.ALLOW); + expect(result.decision).toBe(PolicyDecision.ASK_USER); expect(result.rule).toBeDefined(); expect(result.rule?.priority).toBe(999); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/policy/policy-engine.test.ts` around lines 1754 - 1782, The test's assertion expecting ALLOW is inconsistent with PolicyEngine.checkShellCommand() behavior when splitCommands returns an empty array: the engine falls back to defaultDecision (here ASK_USER) for non-DENY matches; update the test to assert PolicyDecision.ASK_USER (and priority of the matching rule if needed) OR change the test fixture so defaultDecision is ALLOW; specifically adjust the expectations around result.decision and possibly result.rule to match the engine's fallback behavior (references: splitCommands, PolicyEngine.checkShellCommand(), defaultDecision, PolicyDecision.ALLOW/ASK_USER, engine.check).
🧹 Nitpick comments (5)
packages/a2a-server/src/config/config.ts (1)
254-261: 💤 Low valueUnused
path.resolve()result.The
path.resolve(adcFilePath)call computes a resolved path but discards the result. If the intent is to validate that the path can be resolved, this does nothing useful sincepath.resolve()doesn't throw for invalid paths. If the intent is to use the resolved path later, the result should be stored and passed to the auth flow.Suggested fix if validation is intended
try { if (adcFilePath) { - path.resolve(adcFilePath); + const resolved = path.resolve(adcFilePath); + // Optionally verify file exists: + // if (!fs.existsSync(resolved)) { ... } } } catch (e) {Or remove the try-catch block entirely if no validation is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/a2a-server/src/config/config.ts` around lines 254 - 261, The call to path.resolve(adcFilePath) in the config handling block discards its result and doesn't validate the path; update the code to capture the resolved path (e.g., const resolvedAdcPath = path.resolve(adcFilePath)) and use resolvedAdcPath where the GOOGLE_APPLICATION_CREDENTIALS path is consumed (or passed into your auth flow), or if no validation/transform is needed remove the entire try/catch and the path.resolve call; adjust logging to reference the resolvedAdcPath variable (or remove the block) accordingly so adcFilePath is actually used.packages/cli/src/ui/components/Composer.test.tsx (1)
658-669: ⚡ Quick winThis YOLO test currently doesn't assert YOLO-specific behavior.
Because the mocked
ApprovalModeIndicatorignoresisYoloMode, this test only verifies generic indicator presence and won’t catch regressions in wildcard→YOLO wiring. Please assert thatisYoloModeis passed astruein this scenario.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/ui/components/Composer.test.tsx` around lines 658 - 669, The test currently only checks that ApprovalModeIndicator renders but not that YOLO mode is enabled; update the test so the mocked ApprovalModeIndicator asserts it receives isYoloMode=true when getAllowedTools returns ['*'] and showApprovalModeIndicator is ApprovalMode.DEFAULT. Locate the mock for ApprovalModeIndicator (or add one) in Composer.test.tsx and either change it to capture props and expose them to the test or use a spy/vi.mock to intercept the component props, then add an assertion that the captured props.isYoloMode === true after calling renderComposer with the createMockConfig/getAllowedTools returning ['*'] and createMockUIState(showApprovalModeIndicator: ApprovalMode.DEFAULT).packages/cli/src/services/prompt-processors/shellProcessor.test.ts (1)
228-230: ⚡ Quick winAlign these test cases with wildcard-based YOLO semantics.
These overrides now set
ApprovalMode.AUTO_EDIT, but the cases are still framed as YOLO behavior. This makes the intent ambiguous and does not validate wildcard-driven YOLO behavior in this suite. Prefer removing these mode overrides (if irrelevant) or explicitly modeling YOLO via allowed-tools wildcard setup.Also applies to: 258-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/services/prompt-processors/shellProcessor.test.ts` around lines 228 - 230, The tests currently force ApprovalMode.AUTO_EDIT by calling (mockConfig.getApprovalMode as Mock).mockReturnValue(ApprovalMode.AUTO_EDIT) which conflicts with the intended wildcard-based YOLO semantics; either remove those mockReturnValue overrides so the tests exercise the default/Yolo flow, or explicitly simulate YOLO by setting up the allowed-tools wildcard (e.g., mockConfig.getAllowedTools / allowedTools config) to include a wildcard entry before invoking the ShellProcessor tests; update any assertions that assume YOLO behavior accordingly and do the same change for the other occurrence around lines 258-260.packages/cli/src/ui/hooks/useGeminiStream.test.tsx (1)
268-269: ⚡ Quick winAvoid sharing a mutable history array across tests.
Using one module-level
emptyHistorycan leak state between tests if any path mutates it. Create a fresh array per render.🧪 Proposed refactor
- const emptyHistory: any[] = []; + const createEmptyHistory = (): any[] => []; let capturedOnComplete: any = null;- history: emptyHistory, + history: createEmptyHistory(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/ui/hooks/useGeminiStream.test.tsx` around lines 268 - 269, The test currently exposes a module-level mutable array emptyHistory (and mutable capturedOnComplete) which can leak state between tests; instead, create a fresh history array (and reset capturedOnComplete) per render or per test—move the creation of emptyHistory into the renderHook call or into a beforeEach so each test gets its own [] and ensure capturedOnComplete is re-initialized (e.g., set to null) before each test; update references to use the local/history-from-render variable and remove reliance on the shared module-level emptyHistory and capturedOnComplete.packages/cli/src/utils/handleAutoUpdate.test.ts (1)
207-222: ⚡ Quick winAdd explicit positive coverage for
PackageManager.BINARYupdate flow.This table now validates the “not suppressed” set, but there’s no direct assertion that
BINARYactually proceeds (emits update-received and/or spawns update). Add one dedicated test to lock in the new behavior.Proposed test addition
+ it('should not suppress update handling when running via BINARY', () => { + mockGetInstallationInfo.mockReturnValue({ + updateCommand: 'npm i -g `@google/gemini-cli`@latest', + updateMessage: 'Binary install can auto-update.', + isGlobal: false, + packageManager: PackageManager.BINARY, + }); + + handleAutoUpdate(mockUpdateInfo, mockSettings, '/root', mockSpawn); + + expect(updateEventEmitter.emit).toHaveBeenCalledWith('update-received', { + ...mockUpdateInfo, + message: 'An update is available!\nBinary install can auto-update.', + isUpdating: true, + }); + expect(mockSpawn).toHaveBeenCalledOnce(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/handleAutoUpdate.test.ts` around lines 207 - 222, Add a unit test that explicitly covers the PackageManager.BINARY path: call handleAutoUpdate with mockGetInstallationInfo returning packageManager: PackageManager.BINARY (and appropriate updateCommand/updateMessage), then assert that updateEventEmitter.emit was called (with the expected update event) and that mockSpawn was invoked to run the binary update; reference the existing test harness variables (handleAutoUpdate, mockUpdateInfo, mockSettings, mockSpawn, updateEventEmitter) so the new test mirrors the NPX/PNPX/BUNX tests but asserts the positive "proceed with update" behavior for PackageManager.BINARY.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/configuration.md`:
- Around line 2328-2330: The sentence describing the limit for the
--include-directories flag is awkward; replace the line "- 5 directories can be
added at maximum." with clearer grammar such as "- You can add up to 5
directories." (refer to the "--include-directories" option in the same list) so
the documentation reads naturally and consistently with the surrounding
examples.
In `@docs/reference/policy-engine.md`:
- Around line 151-154: The example final priorities in the doc are using the
wrong tier bases; update the three examples that show Workspace `2.010`, User
`3.100`, and Admin `4.020` to use the correct bases so they read Workspace
`3.010`, User `4.100`, and Admin `5.020` (leave Default `1.050` as-is); change
the literal priority example strings in the policy examples to the corrected
values so the examples match the table/formula.
In `@packages/cli/src/config/config.test.ts`:
- Line 2705: Tests currently only assert config.getApprovalMode() ===
ServerConfig.ApprovalMode.DEFAULT; also assert the YOLO migration sets wildcard
access by checking the allowed tools list contains '*' (e.g.,
expect(config.getAllowedTools()).toContain('*') or equivalent). Update each
affected test case that uses config.getApprovalMode() in the YOLO path (where
CLI flags --yolo / --approval-mode=yolo are exercised) to include this
additional assertion so the migration contract (unrestricted => allowedTools
includes '*') is validated alongside the mode check.
In `@packages/cli/src/ui/components/InputPrompt.test.tsx`:
- Around line 4179-4181: The assertion is checking for the literal string
"{chalk.inverse(' ')}" instead of the evaluated inverted-space output; update
the test so it checks the actual rendered value using chalk.inverse(' '), e.g.
replace expect(stdout.lastFrame()).not.toContain(`{chalk.inverse(' ')}`) with
expect(stdout.lastFrame()).not.toContain(chalk.inverse(' ')), referencing the
stdout.lastFrame() call and the chalk.inverse utility to validate the cursor
inversion behavior.
In `@packages/core/src/config/config.test.ts`:
- Around line 1753-1755: The two tests conflict about whether
config.setApprovalMode(ApprovalMode.PLAN) should throw in an untrusted folder;
update one of the assertions so both tests assert the same policy: either both
expect config.setApprovalMode(ApprovalMode.PLAN) to throw with the message
'Cannot enable privileged approval modes in an untrusted folder.' or both expect
it to succeed (no throw). Locate the two assertions referencing
config.setApprovalMode and ApprovalMode.PLAN and make their expectations
consistent (modify the expect(...).toThrow(...) or expect(...).not.toThrow()
accordingly) to reflect the intended trusted/untrusted behavior.
In `@packages/core/src/policy/persistence.test.ts`:
- Around line 281-286: The test still expects the legacy yolo mode after
publishing modes [ApprovalMode.DEFAULT, ApprovalMode.AUTO_EDIT]; update the
assertion in persistence.test.ts that checks persisted modes (the assertion
around the post-publish check following messageBus.publish) to remove
ApprovalMode.YOLO/'yolo' and assert only ApprovalMode.DEFAULT and
ApprovalMode.AUTO_EDIT (matching the published array) so the test reflects the
new behavior.
In `@packages/core/src/tools/mcp-tool.ts`:
- Around line 83-87: The code currently emits duplicate encodings for the global
wildcard when serverName === '*' and toolName === '*'; update the logic that
builds MCP tool keys so that any time serverName === '*' you always return
`${MCP_TOOL_PREFIX}*` (i.e., ignore/normalize toolName when serverName is the
global wildcard) instead of emitting `${MCP_TOOL_PREFIX}*_${toolName}`; change
the branches around serverName and toolName (referencing serverName, toolName,
and MCP_TOOL_PREFIX) so the serverName === '*' case canonicalizes to a single
`mcp_*` value.
---
Outside diff comments:
In `@docs/reference/configuration.md`:
- Around line 115-116: Update the documentation text in the defaultApprovalMode
section to remove the stale reference to "--yolo" and "--approval-mode=yolo" and
replace it with the current valid modes and CLI usage; specifically, remove the
phrase "YOLO mode (auto-approve all actions) can only be enabled via command
line (--yolo or --approval-mode=yolo)" and instead mention the supported
approval modes ("default", "auto_edit", "plan") and how to set them via the CLI
using the --approval-mode flag (e.g., --approval-mode=auto_edit). Ensure the
change is applied to the paragraph that currently mentions read-only/YOLO mode
so the docs no longer reference the deprecated --yolo flag.
In `@packages/cli/src/config/config.ts`:
- Around line 564-620: The bug is that rawApprovalMode 'yolo' sets approvalMode
= ApprovalMode.DEFAULT but uses isYoloRequested to inject '*' into allowedTools,
which lets untrusted-folder checks (they gate on approvalMode !== DEFAULT) be
bypassed; change the 'yolo' branch so it sets a non-default approvalMode (e.g.,
ApprovalMode.ALLOWED_TOOLS or ApprovalMode.YOLO) instead of ApprovalMode.DEFAULT
and keep isYoloRequested semantics so the later trust gate (which checks
approvalMode !== DEFAULT) will correctly treat YOLO as a non-default, approved
mode; update the switch case for rawApprovalMode === 'yolo' (and any dependent
enum/consumers of ApprovalMode) to use that new/non-default ApprovalMode value
and ensure allowedTools wildcard injection still only happens when approvalMode
!== ApprovalMode.DEFAULT.
In `@packages/cli/src/ui/hooks/useApprovalModeIndicator.ts`:
- Around line 43-57: The switch over currentMode in useApprovalModeIndicator
doesn't handle unexpected values, so add a fallback in the default branch to set
nextApprovalMode = ApprovalMode.DEFAULT (using the same ApprovalMode symbol) so
that when config.getApprovalMode() returns an unsupported value the cycle
recovers; update the default case where currentMode is switched (referencing
currentMode, nextApprovalMode, ApprovalMode, and allowPlanMode) to assign
ApprovalMode.DEFAULT as the nextApprovalMode.
In `@packages/core/src/policy/policy-engine.test.ts`:
- Around line 1754-1782: The test's assertion expecting ALLOW is inconsistent
with PolicyEngine.checkShellCommand() behavior when splitCommands returns an
empty array: the engine falls back to defaultDecision (here ASK_USER) for
non-DENY matches; update the test to assert PolicyDecision.ASK_USER (and
priority of the matching rule if needed) OR change the test fixture so
defaultDecision is ALLOW; specifically adjust the expectations around
result.decision and possibly result.rule to match the engine's fallback behavior
(references: splitCommands, PolicyEngine.checkShellCommand(), defaultDecision,
PolicyDecision.ALLOW/ASK_USER, engine.check).
---
Nitpick comments:
In `@packages/a2a-server/src/config/config.ts`:
- Around line 254-261: The call to path.resolve(adcFilePath) in the config
handling block discards its result and doesn't validate the path; update the
code to capture the resolved path (e.g., const resolvedAdcPath =
path.resolve(adcFilePath)) and use resolvedAdcPath where the
GOOGLE_APPLICATION_CREDENTIALS path is consumed (or passed into your auth flow),
or if no validation/transform is needed remove the entire try/catch and the
path.resolve call; adjust logging to reference the resolvedAdcPath variable (or
remove the block) accordingly so adcFilePath is actually used.
In `@packages/cli/src/services/prompt-processors/shellProcessor.test.ts`:
- Around line 228-230: The tests currently force ApprovalMode.AUTO_EDIT by
calling (mockConfig.getApprovalMode as
Mock).mockReturnValue(ApprovalMode.AUTO_EDIT) which conflicts with the intended
wildcard-based YOLO semantics; either remove those mockReturnValue overrides so
the tests exercise the default/Yolo flow, or explicitly simulate YOLO by setting
up the allowed-tools wildcard (e.g., mockConfig.getAllowedTools / allowedTools
config) to include a wildcard entry before invoking the ShellProcessor tests;
update any assertions that assume YOLO behavior accordingly and do the same
change for the other occurrence around lines 258-260.
In `@packages/cli/src/ui/components/Composer.test.tsx`:
- Around line 658-669: The test currently only checks that ApprovalModeIndicator
renders but not that YOLO mode is enabled; update the test so the mocked
ApprovalModeIndicator asserts it receives isYoloMode=true when getAllowedTools
returns ['*'] and showApprovalModeIndicator is ApprovalMode.DEFAULT. Locate the
mock for ApprovalModeIndicator (or add one) in Composer.test.tsx and either
change it to capture props and expose them to the test or use a spy/vi.mock to
intercept the component props, then add an assertion that the captured
props.isYoloMode === true after calling renderComposer with the
createMockConfig/getAllowedTools returning ['*'] and
createMockUIState(showApprovalModeIndicator: ApprovalMode.DEFAULT).
In `@packages/cli/src/ui/hooks/useGeminiStream.test.tsx`:
- Around line 268-269: The test currently exposes a module-level mutable array
emptyHistory (and mutable capturedOnComplete) which can leak state between
tests; instead, create a fresh history array (and reset capturedOnComplete) per
render or per test—move the creation of emptyHistory into the renderHook call or
into a beforeEach so each test gets its own [] and ensure capturedOnComplete is
re-initialized (e.g., set to null) before each test; update references to use
the local/history-from-render variable and remove reliance on the shared
module-level emptyHistory and capturedOnComplete.
In `@packages/cli/src/utils/handleAutoUpdate.test.ts`:
- Around line 207-222: Add a unit test that explicitly covers the
PackageManager.BINARY path: call handleAutoUpdate with mockGetInstallationInfo
returning packageManager: PackageManager.BINARY (and appropriate
updateCommand/updateMessage), then assert that updateEventEmitter.emit was
called (with the expected update event) and that mockSpawn was invoked to run
the binary update; reference the existing test harness variables
(handleAutoUpdate, mockUpdateInfo, mockSettings, mockSpawn, updateEventEmitter)
so the new test mirrors the NPX/PNPX/BUNX tests but asserts the positive
"proceed with update" behavior for PackageManager.BINARY.
🪄 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 Plus
Run ID: 645c3fc8-953b-4c81-b02a-783cb346abeb
⛔ Files ignored due to path filters (3)
packages/cli/src/ui/components/__snapshots__/ApprovalModeIndicator.test.tsx.snapis excluded by!**/*.snappackages/cli/src/ui/components/__snapshots__/InputPrompt.test.tsx.snapis excluded by!**/*.snappackages/cli/src/ui/components/__snapshots__/ShortcutsHelp.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (87)
.gemini/skills/behavioral-evals/references/fixing.mddocs/admin/enterprise-controls.mddocs/cli/cli-reference.mddocs/cli/plan-mode.mddocs/extensions/reference.mddocs/reference/configuration.mddocs/reference/keyboard-shortcuts.mddocs/reference/policy-engine.mddocs/tools/planning.mdpackages/a2a-server/src/agent/task-event-driven.test.tspackages/a2a-server/src/agent/task.tspackages/a2a-server/src/config/config.test.tspackages/a2a-server/src/config/config.tspackages/a2a-server/src/http/app.test.tspackages/a2a-server/src/utils/testing_utils.tspackages/cli/src/acp/acpClient.test.tspackages/cli/src/acp/acpClient.tspackages/cli/src/acp/acpResume.test.tspackages/cli/src/config/config.test.tspackages/cli/src/config/config.tspackages/cli/src/config/extension.test.tspackages/cli/src/config/policy-engine.integration.test.tspackages/cli/src/gemini.tsxpackages/cli/src/services/prompt-processors/shellProcessor.test.tspackages/cli/src/ui/commands/policiesCommand.test.tspackages/cli/src/ui/commands/policiesCommand.tspackages/cli/src/ui/components/ApprovalModeIndicator.test.tsxpackages/cli/src/ui/components/ApprovalModeIndicator.tsxpackages/cli/src/ui/components/Composer.test.tsxpackages/cli/src/ui/components/Help.tsxpackages/cli/src/ui/components/InputPrompt.test.tsxpackages/cli/src/ui/components/InputPrompt.tsxpackages/cli/src/ui/components/ShortcutsHelp.tsxpackages/cli/src/ui/components/messages/ToolConfirmationMessage.tsxpackages/cli/src/ui/constants/tips.tspackages/cli/src/ui/hooks/useApprovalModeIndicator.test.tspackages/cli/src/ui/hooks/useApprovalModeIndicator.tspackages/cli/src/ui/hooks/useComposerStatus.tspackages/cli/src/ui/hooks/useGeminiStream.test.tsxpackages/cli/src/ui/hooks/useGeminiStream.tspackages/cli/src/ui/key/keyBindings.tspackages/cli/src/ui/key/keyMatchers.test.tspackages/cli/src/utils/handleAutoUpdate.test.tspackages/cli/src/utils/handleAutoUpdate.tspackages/cli/src/utils/installationInfo.test.tspackages/cli/src/utils/installationInfo.tspackages/core/.geminiignorepackages/core/.gitignorepackages/core/src/config/config.test.tspackages/core/src/config/config.tspackages/core/src/context/chatCompressionService.tspackages/core/src/core/prompts-substitution.test.tspackages/core/src/core/prompts.test.tspackages/core/src/hooks/hookAggregator.tspackages/core/src/policy/config.test.tspackages/core/src/policy/config.tspackages/core/src/policy/persistence.test.tspackages/core/src/policy/policies/plan.tomlpackages/core/src/policy/policies/read-only.tomlpackages/core/src/policy/policies/write.tomlpackages/core/src/policy/policies/yolo.tomlpackages/core/src/policy/policy-engine.test.tspackages/core/src/policy/policy-engine.tspackages/core/src/policy/toml-loader.test.tspackages/core/src/policy/topic-policy.test.tspackages/core/src/policy/types.tspackages/core/src/prompts/promptProvider.test.tspackages/core/src/prompts/promptProvider.tspackages/core/src/scheduler/policy.test.tspackages/core/src/scheduler/scheduler_hooks.test.tspackages/core/src/services/loopDetectionService.tspackages/core/src/skills/builtin/skill-creator/SKILL.mdpackages/core/src/telemetry/clearcut-logger/clearcut-logger.tspackages/core/src/telemetry/index.tspackages/core/src/telemetry/loggers.tspackages/core/src/telemetry/semantic.tspackages/core/src/telemetry/types.tspackages/core/src/test-utils/mock-message-bus.tspackages/core/src/tools/exit-plan-mode.test.tspackages/core/src/tools/exit-plan-mode.tspackages/core/src/tools/mcp-tool.tspackages/core/src/tools/trackerTools.test.tspackages/core/src/utils/approvalModeUtils.test.tspackages/core/src/utils/approvalModeUtils.tspackages/core/src/utils/editCorrector.tspackages/core/src/utils/googleErrors.tspackages/core/src/utils/oauth-flow.ts
💤 Files with no reviewable changes (17)
- packages/cli/src/utils/installationInfo.test.ts
- packages/core/src/policy/policies/yolo.toml
- packages/cli/src/acp/acpResume.test.ts
- packages/cli/src/ui/components/Help.tsx
- packages/core/src/telemetry/loggers.ts
- packages/core/src/policy/types.ts
- packages/cli/src/ui/hooks/useComposerStatus.ts
- packages/cli/src/utils/installationInfo.ts
- packages/core/src/telemetry/index.ts
- docs/reference/keyboard-shortcuts.md
- packages/core/src/telemetry/types.ts
- packages/cli/src/acp/acpClient.ts
- packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts
- packages/core/src/utils/approvalModeUtils.test.ts
- packages/core/src/policy/topic-policy.test.ts
- packages/cli/src/ui/key/keyBindings.ts
- packages/cli/src/gemini.tsx
Comment on lines
+2328
to
+2330
| - Can be specified multiple times or as comma-separated values. | ||
| - 5 directories can be added at maximum. | ||
| - Example: `--include-directories /path/to/project1,/path/to/project2` or |
There was a problem hiding this comment.
Fix grammar in include-directories limit sentence.
Current wording is awkward; e.g., “You can add up to 5 directories.”
🧰 Tools
🪛 LanguageTool
[style] ~2328-~2328: To form a complete sentence, be sure to include a subject.
Context: ...ce for multi-directory support. - Can be specified multiple times or as comma...
(MISSING_IT_THERE)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/configuration.md` around lines 2328 - 2330, The sentence
describing the limit for the --include-directories flag is awkward; replace the
line "- 5 directories can be added at maximum." with clearer grammar such as "-
You can add up to 5 directories." (refer to the "--include-directories" option
in the same list) so the documentation reads naturally and consistently with the
surrounding examples.
Comment on lines
+151
to
+154
| - A `priority: 50` rule in a Default policy file becomes `1.050`. | ||
| - A `priority: 10` rule in a Workspace policy policy file becomes `2.010`. | ||
| - A `priority: 100` rule in a User policy file becomes `3.100`. | ||
| - A `priority: 20` rule in an Admin policy file becomes `4.020`. |
There was a problem hiding this comment.
Correct tier math in priority examples.
The example final priorities don’t match the table/formula above. With bases 1/2/3/4/5, these should be: Workspace 3.010, User 4.100, Admin 5.020.
Proposed doc fix
-- A `priority: 10` rule in a Workspace policy policy file becomes `2.010`.
-- A `priority: 100` rule in a User policy file becomes `3.100`.
-- A `priority: 20` rule in an Admin policy file becomes `4.020`.
+- A `priority: 10` rule in a Workspace policy file becomes `3.010`.
+- A `priority: 100` rule in a User policy file becomes `4.100`.
+- A `priority: 20` rule in an Admin policy file becomes `5.020`.📝 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
| - A `priority: 50` rule in a Default policy file becomes `1.050`. | |
| - A `priority: 10` rule in a Workspace policy policy file becomes `2.010`. | |
| - A `priority: 100` rule in a User policy file becomes `3.100`. | |
| - A `priority: 20` rule in an Admin policy file becomes `4.020`. | |
| - A `priority: 50` rule in a Default policy file becomes `1.050`. | |
| - A `priority: 10` rule in a Workspace policy file becomes `3.010`. | |
| - A `priority: 100` rule in a User policy file becomes `4.100`. | |
| - A `priority: 20` rule in an Admin policy file becomes `5.020`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/policy-engine.md` around lines 151 - 154, The example final
priorities in the doc are using the wrong tier bases; update the three examples
that show Workspace `2.010`, User `3.100`, and Admin `4.020` to use the correct
bases so they read Workspace `3.010`, User `4.100`, and Admin `5.020` (leave
Default `1.050` as-is); change the literal priority example strings in the
policy examples to the corrected values so the examples match the table/formula.
| argv, | ||
| ); | ||
| expect(config.getApprovalMode()).toBe(ServerConfig.ApprovalMode.YOLO); | ||
| expect(config.getApprovalMode()).toBe(ServerConfig.ApprovalMode.DEFAULT); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert wildcard mapping, not just DEFAULT mode, in YOLO-path tests.
At Line 2705/2716/2749/2775/2926, these tests now only verify ApprovalMode.DEFAULT. That misses the core migration contract (--yolo/--approval-mode=yolo => allowedTools contains '*' when unrestricted), so a regression could slip through.
Suggested test assertion additions
expect(config.getApprovalMode()).toBe(ServerConfig.ApprovalMode.DEFAULT);
+expect(config.getAllowedTools() || []).toContain('*');Also applies to: 2716-2716, 2749-2749, 2775-2775, 2926-2926
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/config/config.test.ts` at line 2705, Tests currently only
assert config.getApprovalMode() === ServerConfig.ApprovalMode.DEFAULT; also
assert the YOLO migration sets wildcard access by checking the allowed tools
list contains '*' (e.g., expect(config.getAllowedTools()).toContain('*') or
equivalent). Update each affected test case that uses config.getApprovalMode()
in the YOLO path (where CLI flags --yolo / --approval-mode=yolo are exercised)
to include this additional assertion so the migration contract (unrestricted =>
allowedTools includes '*') is validated alongside the mode check.
Comment on lines
+4179
to
+4181
| await waitFor(() => { | ||
| expect(stdout.lastFrame()).not.toContain(`{chalk.inverse(' ')}`); | ||
| }); |
There was a problem hiding this comment.
Fix the literal-string assertion in the shell-focused cursor test.
The current check uses a literal `{chalk.inverse(' ')}` string, so it can pass without actually validating cursor inversion behavior.
Proposed fix
- await waitFor(() => {
- expect(stdout.lastFrame()).not.toContain(`{chalk.inverse(' ')}`);
- });
+ await waitFor(() => {
+ expect(stdout.lastFrame()).not.toContain(chalk.inverse(' '));
+ });📝 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
| await waitFor(() => { | |
| expect(stdout.lastFrame()).not.toContain(`{chalk.inverse(' ')}`); | |
| }); | |
| await waitFor(() => { | |
| expect(stdout.lastFrame()).not.toContain(chalk.inverse(' ')); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/ui/components/InputPrompt.test.tsx` around lines 4179 -
4181, The assertion is checking for the literal string "{chalk.inverse(' ')}"
instead of the evaluated inverted-space output; update the test so it checks the
actual rendered value using chalk.inverse(' '), e.g. replace
expect(stdout.lastFrame()).not.toContain(`{chalk.inverse(' ')}`) with
expect(stdout.lastFrame()).not.toContain(chalk.inverse(' ')), referencing the
stdout.lastFrame() call and the chalk.inverse utility to validate the cursor
inversion behavior.
Comment on lines
+1753
to
1755
| expect(() => config.setApprovalMode(ApprovalMode.PLAN)).toThrow( | ||
| 'Cannot enable privileged approval modes in an untrusted folder.', | ||
| ); |
There was a problem hiding this comment.
Conflicting PLAN trust expectations in this suite.
Line 1753 now expects ApprovalMode.PLAN to throw in untrusted folders, but Line 1775 asserts the opposite. This makes the contract ambiguous and can break CI depending on implementation. Align one of these tests to the intended policy behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/config/config.test.ts` around lines 1753 - 1755, The two
tests conflict about whether config.setApprovalMode(ApprovalMode.PLAN) should
throw in an untrusted folder; update one of the assertions so both tests assert
the same policy: either both expect config.setApprovalMode(ApprovalMode.PLAN) to
throw with the message 'Cannot enable privileged approval modes in an untrusted
folder.' or both expect it to succeed (no throw). Locate the two assertions
referencing config.setApprovalMode and ApprovalMode.PLAN and make their
expectations consistent (modify the expect(...).toThrow(...) or
expect(...).not.toThrow() accordingly) to reflect the intended trusted/untrusted
behavior.
Comment on lines
+281
to
+286
| // Now grant in DEFAULT mode, which should include [default, autoEdit] | ||
| await messageBus.publish({ | ||
| type: MessageBusType.UPDATE_POLICY, | ||
| toolName: 'test_tool', | ||
| persist: true, | ||
| modes: [ApprovalMode.DEFAULT, ApprovalMode.AUTO_EDIT, ApprovalMode.YOLO], | ||
| modes: [ApprovalMode.DEFAULT, ApprovalMode.AUTO_EDIT], |
There was a problem hiding this comment.
Update the expected persisted modes to remove legacy yolo.
This update now publishes [ApprovalMode.DEFAULT, ApprovalMode.AUTO_EDIT], but the assertion still expects yolo at Line 295. That keeps stale behavior in the test and can break this migration.
✅ Suggested fix
- expect(content).toContain('modes = [ "default", "autoEdit", "yolo" ]');
+ expect(content).toContain('modes = [ "default", "autoEdit" ]');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/policy/persistence.test.ts` around lines 281 - 286, The
test still expects the legacy yolo mode after publishing modes
[ApprovalMode.DEFAULT, ApprovalMode.AUTO_EDIT]; update the assertion in
persistence.test.ts that checks persisted modes (the assertion around the
post-publish check following messageBus.publish) to remove
ApprovalMode.YOLO/'yolo' and assert only ApprovalMode.DEFAULT and
ApprovalMode.AUTO_EDIT (matching the published array) so the test reflects the
new behavior.
Comment on lines
+83
to
+87
| if (serverName === '*' && !toolName) { | ||
| return `${MCP_TOOL_PREFIX}*`; | ||
| } else if (serverName === '*') { | ||
| return `${MCP_TOOL_PREFIX}*_${toolName}`; | ||
| } else if (toolName === undefined || toolName === '*') { | ||
| } else if (!toolName) { |
There was a problem hiding this comment.
Canonicalize global MCP wildcard output.
Line 85 now emits mcp_*_* when serverName === '*' and toolName === '*'. This introduces a second encoding for the same global wildcard and can break exact-match policy storage/lookups expecting mcp_*.
💡 Suggested fix
export function formatMcpToolName(
serverName: string,
toolName?: string,
): string {
- if (serverName === '*' && !toolName) {
+ if (serverName === '*' && (!toolName || toolName === '*')) {
return `${MCP_TOOL_PREFIX}*`;
} else if (serverName === '*') {
return `${MCP_TOOL_PREFIX}*_${toolName}`;
} else if (!toolName) {
return `${MCP_TOOL_PREFIX}${serverName}_*`;
} else {
return `${MCP_TOOL_PREFIX}${serverName}_${toolName}`;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (serverName === '*' && !toolName) { | |
| return `${MCP_TOOL_PREFIX}*`; | |
| } else if (serverName === '*') { | |
| return `${MCP_TOOL_PREFIX}*_${toolName}`; | |
| } else if (toolName === undefined || toolName === '*') { | |
| } else if (!toolName) { | |
| export function formatMcpToolName( | |
| serverName: string, | |
| toolName?: string, | |
| ): string { | |
| if (serverName === '*' && (!toolName || toolName === '*')) { | |
| return `${MCP_TOOL_PREFIX}*`; | |
| } else if (serverName === '*') { | |
| return `${MCP_TOOL_PREFIX}*_${toolName}`; | |
| } else if (!toolName) { | |
| return `${MCP_TOOL_PREFIX}${serverName}_*`; | |
| } else { | |
| return `${MCP_TOOL_PREFIX}${serverName}_${toolName}`; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/tools/mcp-tool.ts` around lines 83 - 87, The code currently
emits duplicate encodings for the global wildcard when serverName === '*' and
toolName === '*'; update the logic that builds MCP tool keys so that any time
serverName === '*' you always return `${MCP_TOOL_PREFIX}*` (i.e.,
ignore/normalize toolName when serverName is the global wildcard) instead of
emitting `${MCP_TOOL_PREFIX}*_${toolName}`; change the branches around
serverName and toolName (referencing serverName, toolName, and MCP_TOOL_PREFIX)
so the serverName === '*' case canonicalizes to a single `mcp_*` value.
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
2 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.
This PR maps the
--yoloflag natively into a wildcard policy array (allowedTools: ["*"]) and removes the concept ofApprovalMode.YOLOas a distinct state in the application, fulfilling issue google-gemini#11303.This removes the hardcoded
ApprovalMode.YOLOstate and its associated UI/bypasses. ThePolicyEnginenow evaluates YOLO purely via data-driven rules.ApprovalMode.YOLOCtrl+Y) and indicators for YOLOyolo.tomldisableYoloModeandsecureModeEnabledcontrols by actively preventing manual--allowed-tools=*bypasses.Fixes google-gemini#11303
Summary
Details
Related Issues
How to Validate
Pre-Merge Checklist
Summary by CodeRabbit
Documentation
Removed Features
Ctrl+Y) keyboard shortcut and related approval mode functionality.Changes