Fix/pending tools and trust overrides#27854
Conversation
- task/executor: Add check to safely yield the agent turn when waiting for tool approvals, preventing premature state progression. - config: Extract logic to allow environment variables to properly override workspace trust settings. - scheduler: Enforce sequential execution for to prevent parallel modification race conditions. - : Add unit tests for verifying environment variable overrides. - : Add unit tests for and state tracking. - : Add integration test to verify is strictly executed sequentially within batches. - : Update mock implementations and add coverage for the new execution yielding logic.
|
📊 PR Size: size/L
|
gemini-cli
Bot
added
the
status/need-issue
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the agent's execution environment by addressing critical race conditions and state management issues. By enforcing sequential execution for file writes and implementing a proper waiting mechanism for pending tool approvals, the agent now handles complex task sequences more reliably. Additionally, the configuration logic has been improved to provide better control over workspace trust settings via environment variables. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several updates to the agent execution and scheduling logic. Specifically, it adds a new setIsTrusted utility in config.ts to determine folder trust based on the GEMINI_FOLDER_TRUST environment variable or agent settings, and integrates it into the CoderAgentExecutor. It also implements checks for pending tools in the executor loop to yield control back to the user when tools are awaiting approval. Additionally, the scheduler has been updated to force sequential execution of the write_file tool, preventing parallel writes. Unit tests have been added to cover these new behaviors. As there are no review comments provided, I have no additional feedback to offer.
galz10
left a comment
galz10
left a comment
There was a problem hiding this comment.
Code Review
Intent Summary: This PR prevents the agent from advancing while tool confirmations are pending, serializes file writes to avoid race conditions, and attempts to allow the GEMINI_FOLDER_TRUST environment variable to override workspace trust settings.
🚨 Critical Concerns (P0/P1)
Action required before merging.
-
Security Flaw / Fail-Open Behavior (
packages/a2a-server/src/config/config.ts:185): The trust override logic is additive (||), not an override. If a user or CI pipeline explicitly setsGEMINI_FOLDER_TRUST=falseto enforce a strict security boundary, this function ignores it ifagentSettings?.isTrustedistrue. Environment variables should act as an authoritative system-level override. The current implementation only allows the environment variable to escalate privileges (grant trust), but fails to let it revoke privileges (enforce distrust).// Suggested fix: export function setIsTrusted( agentSettings: AgentSettings | undefined, ): boolean { if (process.env['GEMINI_FOLDER_TRUST'] !== undefined) { return process.env['GEMINI_FOLDER_TRUST'] === 'true'; } return !!agentSettings?.isTrusted; }
(Note: You must also update the corresponding tests in
config.test.tswhich currently encode this flawed fallback behavior). -
Incomplete Race Condition Fix (
packages/core/src/scheduler/scheduler.ts:554): SerializingWRITE_FILE_TOOL_NAMEto prevent file modification race conditions is incomplete because it omitsEDIT_TOOL_NAME(thereplacetool). Concurrentreplacecalls or a mix ofreplaceandwrite_fileon the same file will still race and corrupt data. AddEDIT_TOOL_NAMEto the non-parallelizable list. -
Missing Test Coverage (
packages/a2a-server/src/agent/executor.ts:549): Added a new control flow branch forcurrentTask.hasPendingTools(), butpackages/a2a-server/src/agent/executor.test.tsonly mocks this to returnfalse. There are no test cases verifying that the executor properly yields the turn when tools are pending. This is a P0 test coverage violation.
🧹 Refactoring & Nits (P2/P3)
Recommended improvements.
packages/a2a-server/src/agent/task.ts:140: ReplacehasPendingTools()andgetPendingToolsCount()with TypeScript getters (e.g.,get hasPendingTools()) to adhere to standard class property access conventions, rather than creating distinct getter methods.
📝 Metadata Review
- The PR description is clear, but the "Trust Configuration" bullet incorrectly claims the environment variable takes "precedence" over internal settings. The implementation only allows the env var to elevate trust, not revoke it. Fix the code to match the PR description's intent.
…overage - Include the tool constant to ensure it is not parallelizable. - - Refactor and in the class into TypeScript getters ( and ) to adhere to standard property access conventions. - Add missing test coverage in executor.test.ts to verify the executor correctly yields the turn and transitions to input-required when tools are pending. - Improve the validation of environment variables within the function's logic. - - Expand test coverage in to fully verify the environment variable precedence logic in . - Add integration tests to validate the new constant in parallelizable validation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements sequential execution for file-writing and editing tools, adds a mechanism to yield the agent's turn when tools are pending user approval, and introduces a setIsTrusted helper to determine workspace trust. A critical security vulnerability was identified where workspace-level .env files can pollute global environment variables and bypass the folder trust mechanism, potentially leading to Remote Code Execution (RCE).
| export function setIsTrusted( | ||
| agentSettings: AgentSettings | undefined, | ||
| ): boolean { | ||
| if (process.env['GEMINI_FOLDER_TRUST'] !== undefined) { | ||
| return process.env['GEMINI_FOLDER_TRUST'] === 'true'; | ||
| } | ||
| return !!agentSettings?.isTrusted; | ||
| } |
There was a problem hiding this comment.
Folder Trust Bypass via Workspace .env Environment Pollution
Severity: critical
Sub-category: Broken Access Control / Environment Pollution
Description:
The setIsTrusted function determines if a workspace folder is trusted by checking process.env['GEMINI_FOLDER_TRUST'] and falling back to agentSettings.isTrusted.
However, in CoderAgentExecutor.getConfig (in packages/a2a-server/src/agent/executor.ts), loadEnvironment() is called immediately after setIsTrusted. loadEnvironment() loads environment variables from the workspace's .env file and overrides process.env globally using dotenv.config({ override: true }).
If an untrusted workspace contains a .env file with GEMINI_FOLDER_TRUST=true, this value will be loaded into process.env globally. On any subsequent task execution or configuration resolution, setIsTrusted will read this polluted environment variable and return true, completely bypassing the folder trust security mechanism.
Since trusted folders allow the agent to execute powerful tools (such as running arbitrary shell commands) without user confirmation, this bypass directly enables Remote Code Execution (RCE) when an untrusted folder is opened.
Remediation:
Do not allow workspace-level .env files to override system-level security environment variables like GEMINI_FOLDER_TRUST. Alternatively, resolve the folder trust status using only the system's initial environment variables or the explicit agentSettings.isTrusted passed by the client, and prevent loadEnvironment() from overriding security-critical environment variables.
const INITIAL_FOLDER_TRUST = process.env['GEMINI_FOLDER_TRUST'];
export function setIsTrusted(
agentSettings: AgentSettings | undefined,
): boolean {
if (INITIAL_FOLDER_TRUST !== undefined) {
return INITIAL_FOLDER_TRUST === 'true';
}
return !!agentSettings?.isTrusted;
}References
- Workspace-level configurations should be treated as untrusted by default. Security-sensitive settings, such as policy paths, must be loaded from trusted user-level configuration and should not be overridable by workspace settings unless trust is explicitly granted by the hosting environment.
…_TRUST in config.ts to evaluate it once upon initialization and Update config.test.ts to use vi.resetModules() and dynamic imports to properly mock module-level environment variables.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces tracking for pending tools to yield the agent's turn when awaiting approval, adds a helper to determine workspace trust, and forces sequential execution for file-modifying tools (WRITE_FILE_TOOL_NAME and EDIT_TOOL_NAME) in the scheduler. Feedback highlights a critical security vulnerability in setIsTrusted where client-controlled metadata can bypass trust checks and potentially lead to Remote Code Execution (RCE); this should be restricted to server-side configuration. Additionally, the scheduler should import and use the EDIT_TOOL_NAMES set rather than hardcoding individual file-modifying tool names to ensure robust sequential execution.
| export function setIsTrusted( | ||
| agentSettings: AgentSettings | undefined, | ||
| ): boolean { | ||
| if (INITIAL_FOLDER_TRUST !== undefined) { | ||
| return INITIAL_FOLDER_TRUST === 'true'; | ||
| } | ||
| return !!agentSettings?.isTrusted; | ||
| } |
There was a problem hiding this comment.
The setIsTrusted function is vulnerable to Remote Code Execution (RCE). An untrusted client can self-certify their workspace as trusted by sending isTrusted: true in userMessage.metadata, which is supplied by the client and is fully user-controlled. This bypasses security restrictions, allowing an attacker to execute arbitrary tools and potentially achieve RCE on the server. To remediate this, the trust status must be determined solely by server-side configuration, such as a strict allowlist of trusted paths, or by requiring the GEMINI_FOLDER_TRUST environment variable to be explicitly set on the server, and not be controlled by the client.
| export function setIsTrusted( | |
| agentSettings: AgentSettings | undefined, | |
| ): boolean { | |
| if (INITIAL_FOLDER_TRUST !== undefined) { | |
| return INITIAL_FOLDER_TRUST === 'true'; | |
| } | |
| return !!agentSettings?.isTrusted; | |
| } | |
| export function setIsTrusted( | |
| agentSettings: AgentSettings | undefined, | |
| ): boolean { | |
| if (INITIAL_FOLDER_TRUST !== undefined) { | |
| return INITIAL_FOLDER_TRUST === 'true'; | |
| } | |
| return false; | |
| } |
References
- Workspace-level configurations should be treated as untrusted by default. Security-sensitive settings, such as policy paths, must be loaded from trusted user-level configuration and should not be overridable by workspace settings unless trust is explicitly granted by the hosting environment.
| import { | ||
| EDIT_TOOL_NAME, | ||
| UPDATE_TOPIC_TOOL_NAME, | ||
| WRITE_FILE_TOOL_NAME, | ||
| } from '../tools/tool-names.js'; |
There was a problem hiding this comment.
Import EDIT_TOOL_NAMES instead of hardcoding individual file-modifying tool names to ensure all file-modifying tools are executed sequentially.
import {
EDIT_TOOL_NAMES,
UPDATE_TOPIC_TOOL_NAME,
} from '../tools/tool-names.js';| if ( | ||
| request.name === UPDATE_TOPIC_TOOL_NAME || | ||
| request.name === WRITE_FILE_TOOL_NAME || | ||
| request.name === EDIT_TOOL_NAME | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Instead of hardcoding specific file-modifying tool names like WRITE_FILE_TOOL_NAME and EDIT_TOOL_NAME, we should use the EDIT_TOOL_NAMES set (which contains all file-modifying/restorable tools, such as replace or others). This ensures that any new or existing file-modifying tools are automatically executed sequentially, preventing race conditions and potential file corruption.
if (
request.name === UPDATE_TOPIC_TOOL_NAME ||
EDIT_TOOL_NAMES.has(request.name)
) {
return false;
}…AME modifier tools using the EDIT_TOOL_NAMES array constant, which now includes both and supports future tool names, updated the scheduler_parallel.test to support the last scheduler modifications
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to yield the agent turn when tools are pending, adds a setIsTrusted helper to determine workspace trust based on the GEMINI_FOLDER_TRUST environment variable, and forces sequential execution for file write and edit tools in the scheduler. Feedback highlights a security vulnerability where loading workspace environment variables via loadEnvironment() before establishing trust could allow an untrusted workspace to override critical process-level environment variables.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for handling pending tools waiting for approval in the agent executor, yielding the turn when tools are pending. It also adds a setIsTrusted configuration helper that respects the GEMINI_FOLDER_TRUST environment variable, and updates the scheduler to force sequential execution for edit tools. Comprehensive unit tests have been added to verify these behaviors. I have no feedback to provide.
Note: Security Review did not run due to the size of the PR.
3 participants
Summary
This PR improves the agent's execution stability by preventing premature state progression when the agent is waiting for user tool approvals. It also eliminates race conditions during file modifications by forcing file writes to execute sequentially, and fixes a configuration bug to ensure environment variables correctly override workspace trust settings.
Details
hasPendingTools()andgetPendingToolsCount()to theTaskclass. TheCoderAgentExecutornow uses these to safely pause its execution loop and yield the turn to the user when tool confirmations are pending._isParallelizablein the scheduler to treatWRITE_FILE_TOOL_NAMEas non-parallelizable. This ensures multiple file edits requested in a single LLM batch do not conflict with each other.setIsTrustedhelper, allowing theGEMINI_FOLDER_TRUSTenvironment variable to take precedence over internal agent settings.Related Issues
Fixes b/521342062 # 400 error when using the write file tool
Fixes b/522312717 # The "Untrusted Folder" message appears in chat even when working within a trusted folder.
How to Validate
npm test -w @google/gemini-cli-a2a-serveryarn workspace @google/gemini-cli-core build yarn test -w @google/gemini-cli-corePre-Merge Checklist