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.
2 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