mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-04-24 03:54:43 -07:00
Fix: Enable write_file in Plan Mode via workspace policy
This commit is contained in:
@@ -713,9 +713,46 @@ export async function loadCliConfig(
|
||||
effectiveSettings,
|
||||
approvalMode,
|
||||
workspacePoliciesDir,
|
||||
cwd,
|
||||
);
|
||||
policyEngineConfig.nonInteractive = !interactive;
|
||||
|
||||
// FIX: Ensure tools allowed by high-priority policy are NOT excluded from the registry.
|
||||
// This allows users to re-enable tools like write_file in Plan Mode via policy.
|
||||
// We ALSO need to remove the conflicting 'Settings (Tools Excluded)' rule from policyEngineConfig,
|
||||
// otherwise PolicyEngine will still consider it excluded.
|
||||
const policyAllowedTools = new Set<string>();
|
||||
if (policyEngineConfig.rules) {
|
||||
for (const rule of policyEngineConfig.rules) {
|
||||
// Logic mirrors promptProvider.ts: Priority > 1.1 means user/admin tier (or high priority default)
|
||||
if (
|
||||
(rule.priority ?? 0) > 1.1 &&
|
||||
(rule.decision === 'allow' || rule.decision === 'ask_user') &&
|
||||
rule.toolName
|
||||
) {
|
||||
policyAllowedTools.add(rule.toolName);
|
||||
}
|
||||
}
|
||||
|
||||
// Filter out conflicting Settings Exclude rules
|
||||
policyEngineConfig.rules = policyEngineConfig.rules.filter((rule) => {
|
||||
if (
|
||||
rule.source === 'Settings (Tools Excluded)' &&
|
||||
rule.toolName &&
|
||||
policyAllowedTools.has(rule.toolName)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
});
|
||||
}
|
||||
|
||||
// If a tool is explicitly allowed by a high-priority policy, remove it from the exclusion list
|
||||
// so it gets registered in ToolRegistry.
|
||||
const finalExcludeTools = excludeTools.filter(
|
||||
(t) => !policyAllowedTools.has(t),
|
||||
);
|
||||
|
||||
const defaultModel = PREVIEW_GEMINI_MODEL_AUTO;
|
||||
const specifiedModel =
|
||||
argv.model || process.env['GEMINI_MODEL'] || settings.model?.name;
|
||||
@@ -776,8 +813,12 @@ export async function loadCliConfig(
|
||||
coreTools: settings.tools?.core || undefined,
|
||||
allowedTools: allowedTools.length > 0 ? allowedTools : undefined,
|
||||
policyEngineConfig,
|
||||
<<<<<<< HEAD
|
||||
policyUpdateConfirmationRequest,
|
||||
excludeTools,
|
||||
=======
|
||||
excludeTools: finalExcludeTools,
|
||||
>>>>>>> 45fcd9869 (Fix: Enable write_file in Plan Mode via workspace policy)
|
||||
toolDiscoveryCommand: settings.tools?.discoveryCommand,
|
||||
toolCallCommand: settings.tools?.callCommand,
|
||||
mcpServerCommand,
|
||||
|
||||
@@ -24,6 +24,7 @@ export async function createPolicyEngineConfig(
|
||||
settings: Settings,
|
||||
approvalMode: ApprovalMode,
|
||||
workspacePoliciesDir?: string,
|
||||
workspaceDir?: string,
|
||||
): Promise<PolicyEngineConfig> {
|
||||
// Explicitly construct PolicySettings from Settings to ensure type safety
|
||||
// and avoid accidental leakage of other settings properties.
|
||||
@@ -35,7 +36,12 @@ export async function createPolicyEngineConfig(
|
||||
workspacePoliciesDir,
|
||||
};
|
||||
|
||||
return createCorePolicyEngineConfig(policySettings, approvalMode);
|
||||
return createCorePolicyEngineConfig(
|
||||
policySettings,
|
||||
approvalMode,
|
||||
undefined,
|
||||
workspaceDir,
|
||||
);
|
||||
}
|
||||
|
||||
export function createPolicyUpdater(
|
||||
|
||||
@@ -2691,15 +2691,13 @@ describe('Plan Directory', () => {
|
||||
expect(config.getPlanDirectory()).toBe(absolutePath);
|
||||
});
|
||||
|
||||
it('should fallback to default plans directory when configured path is outside workspace', () => {
|
||||
it('should return the configured path even when it is outside the workspace', () => {
|
||||
const outsidePath = '/outside/workspace/plans';
|
||||
const params: ConfigParameters = {
|
||||
...baseParams,
|
||||
planDirectory: outsidePath,
|
||||
};
|
||||
const config = new Config(params);
|
||||
expect(config.getPlanDirectory()).toBe(
|
||||
config.storage.getProjectTempPlansDir(),
|
||||
);
|
||||
expect(config.getPlanDirectory()).toBe(outsidePath);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -2047,16 +2047,7 @@ export class Config {
|
||||
|
||||
getPlanDirectory(): string {
|
||||
if (this.planDirectory) {
|
||||
const resolvedPath = path.resolve(
|
||||
this.getTargetDir(),
|
||||
this.planDirectory,
|
||||
);
|
||||
if (isSubpath(this.getTargetDir(), resolvedPath)) {
|
||||
return resolvedPath;
|
||||
}
|
||||
debugLogger.warn(
|
||||
`Configured plan directory '${resolvedPath}' is outside the project root. Falling back to default temporary directory.`,
|
||||
);
|
||||
return path.resolve(this.getTargetDir(), this.planDirectory);
|
||||
}
|
||||
return this.storage.getProjectTempPlansDir();
|
||||
}
|
||||
|
||||
@@ -114,6 +114,10 @@ describe('Core System Prompt (prompts.ts)', () => {
|
||||
}),
|
||||
getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT),
|
||||
getApprovedPlanPath: vi.fn().mockReturnValue(undefined),
|
||||
getPlanDirectory: vi.fn().mockReturnValue('/tmp/project-temp/plans'),
|
||||
getPolicyEngine: vi.fn().mockReturnValue({
|
||||
getRules: vi.fn().mockReturnValue([]),
|
||||
}),
|
||||
} as unknown as Config;
|
||||
});
|
||||
|
||||
@@ -401,6 +405,10 @@ describe('Core System Prompt (prompts.ts)', () => {
|
||||
getSkills: vi.fn().mockReturnValue([]),
|
||||
}),
|
||||
getApprovedPlanPath: vi.fn().mockReturnValue(undefined),
|
||||
getPlanDirectory: vi.fn().mockReturnValue('/tmp/project-temp/plans'),
|
||||
getPolicyEngine: vi.fn().mockReturnValue({
|
||||
getRules: vi.fn().mockReturnValue([]),
|
||||
}),
|
||||
} as unknown as Config;
|
||||
|
||||
const prompt = getCoreSystemPrompt(testConfig);
|
||||
@@ -512,6 +520,7 @@ describe('Core System Prompt (prompts.ts)', () => {
|
||||
vi.mocked(mockConfig.storage.getProjectTempPlansDir).mockReturnValue(
|
||||
'/tmp/plans',
|
||||
);
|
||||
vi.mocked(mockConfig.getPlanDirectory).mockReturnValue('/tmp/plans');
|
||||
});
|
||||
|
||||
it('should include approved plan path when set in config', () => {
|
||||
@@ -554,6 +563,96 @@ describe('Core System Prompt (prompts.ts)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('Dynamic Plan Mode Tools', () => {
|
||||
beforeEach(() => {
|
||||
vi.mocked(mockConfig.getApprovalMode).mockReturnValue(ApprovalMode.PLAN);
|
||||
// Default: no extra rules
|
||||
mockConfig.getPolicyEngine = vi.fn().mockReturnValue({
|
||||
getRules: vi.fn().mockReturnValue([]),
|
||||
});
|
||||
// Ensure write_file is "enabled" in registry but not in PLAN_MODE_TOOLS by default
|
||||
vi.mocked(mockConfig.getToolRegistry().getAllToolNames).mockReturnValue([
|
||||
'write_file',
|
||||
'glob',
|
||||
'replace',
|
||||
]);
|
||||
});
|
||||
|
||||
it('should NOT include write_file by default in Plan Mode', () => {
|
||||
const prompt = getCoreSystemPrompt(mockConfig);
|
||||
expect(prompt).toContain('<tool>`glob`</tool>');
|
||||
expect(prompt).not.toContain('<tool>`write_file`</tool>');
|
||||
});
|
||||
|
||||
it('should include write_file if allowed by a high-priority user policy', () => {
|
||||
vi.mocked(mockConfig.getPolicyEngine().getRules).mockReturnValue([
|
||||
{
|
||||
toolName: 'write_file',
|
||||
decision: 'allow',
|
||||
priority: 2.1, // Tier 2 (User)
|
||||
modes: [ApprovalMode.PLAN],
|
||||
},
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
] as any);
|
||||
|
||||
const prompt = getCoreSystemPrompt(mockConfig);
|
||||
expect(prompt).toContain('<tool>`write_file`</tool>');
|
||||
});
|
||||
|
||||
it('should include write_file if allowed by a high-priority policy with global mode', () => {
|
||||
vi.mocked(mockConfig.getPolicyEngine().getRules).mockReturnValue([
|
||||
{
|
||||
toolName: 'write_file',
|
||||
decision: 'allow',
|
||||
priority: 2.1,
|
||||
modes: undefined, // Applies to all modes
|
||||
},
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
] as any);
|
||||
|
||||
const prompt = getCoreSystemPrompt(mockConfig);
|
||||
expect(prompt).toContain('<tool>`write_file`</tool>');
|
||||
});
|
||||
|
||||
it('should NOT include write_file if allowed by a low-priority policy (below Plan Mode restriction)', () => {
|
||||
vi.mocked(mockConfig.getPolicyEngine().getRules).mockReturnValue([
|
||||
{
|
||||
toolName: 'write_file',
|
||||
decision: 'allow',
|
||||
priority: 1.05, // Lower than 1.06 (Default Plan Mode Restriction)
|
||||
modes: [ApprovalMode.PLAN],
|
||||
},
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
] as any);
|
||||
|
||||
const prompt = getCoreSystemPrompt(mockConfig);
|
||||
expect(prompt).not.toContain('<tool>`write_file`</tool>');
|
||||
});
|
||||
|
||||
it('should include multiple tools if allowed by policy', () => {
|
||||
// This simulates toml-loader flattening ["write_file", "replace"] into two rules
|
||||
vi.mocked(mockConfig.getPolicyEngine().getRules).mockReturnValue([
|
||||
{
|
||||
toolName: 'write_file',
|
||||
decision: 'allow',
|
||||
priority: 2.1,
|
||||
modes: [ApprovalMode.PLAN],
|
||||
},
|
||||
{
|
||||
toolName: 'replace',
|
||||
decision: 'allow',
|
||||
priority: 2.1,
|
||||
modes: [ApprovalMode.PLAN],
|
||||
},
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
] as any);
|
||||
|
||||
const prompt = getCoreSystemPrompt(mockConfig);
|
||||
expect(prompt).toContain('<tool>`write_file`</tool>');
|
||||
expect(prompt).toContain('<tool>`replace`</tool>');
|
||||
});
|
||||
});
|
||||
|
||||
describe('Platform-specific and Background Process instructions', () => {
|
||||
it('should include Windows-specific shell efficiency commands on win32', () => {
|
||||
mockPlatform('win32');
|
||||
|
||||
@@ -31,6 +31,7 @@ import { SHELL_TOOL_NAMES } from '../utils/shell-utils.js';
|
||||
import { SHELL_TOOL_NAME } from '../tools/tool-names.js';
|
||||
|
||||
import { isDirectorySecure } from '../utils/security.js';
|
||||
import { GEMINI_DIR } from '../utils/paths.js';
|
||||
|
||||
const __filename = fileURLToPath(import.meta.url);
|
||||
const __dirname = path.dirname(__filename);
|
||||
@@ -73,6 +74,9 @@ export function getPolicyDirectories(
|
||||
dirs.push(workspacePoliciesDir);
|
||||
}
|
||||
|
||||
// Admin tier (highest priority)
|
||||
dirs.push(Storage.getSystemPoliciesDir());
|
||||
|
||||
// Default tier (lowest priority)
|
||||
dirs.push(defaultPoliciesDir ?? DEFAULT_CORE_POLICIES_DIR);
|
||||
|
||||
@@ -167,11 +171,20 @@ export async function createPolicyEngineConfig(
|
||||
settings: PolicySettings,
|
||||
approvalMode: ApprovalMode,
|
||||
defaultPoliciesDir?: string,
|
||||
workspaceDir?: string,
|
||||
): Promise<PolicyEngineConfig> {
|
||||
const workspacePoliciesDir = workspaceDir
|
||||
? path.join(workspaceDir, GEMINI_DIR, 'policies')
|
||||
: undefined;
|
||||
|
||||
const policyDirs = getPolicyDirectories(
|
||||
defaultPoliciesDir,
|
||||
settings.policyPaths,
|
||||
<<<<<<< HEAD
|
||||
settings.workspacePoliciesDir,
|
||||
=======
|
||||
workspacePoliciesDir,
|
||||
>>>>>>> 45fcd9869 (Fix: Enable write_file in Plan Mode via workspace policy)
|
||||
);
|
||||
const securePolicyDirs = await filterSecurePolicyDirectories(policyDirs);
|
||||
|
||||
|
||||
@@ -2373,4 +2373,90 @@ describe('PolicyEngine', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Relative vs Absolute Path Regex (Plan Mode Trap)', () => {
|
||||
const relativePath = 'conductor/product.md';
|
||||
const absolutePath = '/usr/local/home/user/conductor/product.md';
|
||||
|
||||
it('should FAIL with the brittle regex on relative paths', async () => {
|
||||
// The brittle regex used by the user: .*/conductor/
|
||||
// This requires a preceding slash or characters before 'conductor'
|
||||
const brittleRegex = /"(?:file_path|path)":".*\/conductor\/[^"]+"/;
|
||||
|
||||
const rules: PolicyRule[] = [
|
||||
{
|
||||
toolName: 'write_file',
|
||||
argsPattern: brittleRegex,
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: 100,
|
||||
},
|
||||
];
|
||||
|
||||
// Default behavior (simulating Plan Mode Deny fallback)
|
||||
const engine = new PolicyEngine({
|
||||
rules,
|
||||
defaultDecision: PolicyDecision.DENY,
|
||||
});
|
||||
|
||||
// Relative path fails matching because it starts with 'c', not '/'
|
||||
expect(
|
||||
(
|
||||
await engine.check(
|
||||
{ name: 'write_file', args: { file_path: relativePath } },
|
||||
undefined,
|
||||
)
|
||||
).decision,
|
||||
).toBe(PolicyDecision.DENY);
|
||||
|
||||
// Absolute path matches
|
||||
expect(
|
||||
(
|
||||
await engine.check(
|
||||
{ name: 'write_file', args: { file_path: absolutePath } },
|
||||
undefined,
|
||||
)
|
||||
).decision,
|
||||
).toBe(PolicyDecision.ALLOW);
|
||||
});
|
||||
|
||||
it('should PASS with the robust regex on relative paths', async () => {
|
||||
// The robust regex: (?:.*/)?conductor/
|
||||
// Makes the prefix optional
|
||||
const robustRegex = /"(?:file_path|path)":"(?:.*\/)?conductor\/[^"]+"/;
|
||||
|
||||
const rules: PolicyRule[] = [
|
||||
{
|
||||
toolName: 'write_file',
|
||||
argsPattern: robustRegex,
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: 100,
|
||||
},
|
||||
];
|
||||
|
||||
const engine = new PolicyEngine({
|
||||
rules,
|
||||
defaultDecision: PolicyDecision.DENY,
|
||||
});
|
||||
|
||||
// Relative path matches
|
||||
expect(
|
||||
(
|
||||
await engine.check(
|
||||
{ name: 'write_file', args: { file_path: relativePath } },
|
||||
undefined,
|
||||
)
|
||||
).decision,
|
||||
).toBe(PolicyDecision.ALLOW);
|
||||
|
||||
// Absolute path matches
|
||||
expect(
|
||||
(
|
||||
await engine.check(
|
||||
{ name: 'write_file', args: { file_path: absolutePath } },
|
||||
undefined,
|
||||
)
|
||||
).decision,
|
||||
).toBe(PolicyDecision.ALLOW);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -319,6 +319,16 @@ export class PolicyEngine {
|
||||
`[PolicyEngine.check] toolCall.name: ${toolCall.name}, stringifiedArgs: ${stringifiedArgs}`,
|
||||
);
|
||||
|
||||
if (toolCall.name === 'write_file') {
|
||||
for (const rule of this.rules) {
|
||||
if (!rule.toolName || rule.toolName === 'write_file') {
|
||||
debugLogger.debug(
|
||||
` - [Rule Dump] Tool=${rule.toolName}, P=${rule.priority}, Decision=${rule.decision}, Source=${rule.source}, Pattern=${rule.argsPattern}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Check for shell commands upfront to handle splitting
|
||||
let isShellCommand = false;
|
||||
let command: string | undefined;
|
||||
|
||||
@@ -64,9 +64,29 @@ export class PromptProvider {
|
||||
const contextFilenames = getAllGeminiMdFilenames();
|
||||
|
||||
// --- Context Gathering ---
|
||||
let planModeToolsList = PLAN_MODE_TOOLS.filter((t) =>
|
||||
enabledToolNames.has(t),
|
||||
)
|
||||
const allowedTools = new Set<string>(PLAN_MODE_TOOLS);
|
||||
|
||||
// Dynamically include tools allowed by policy in Plan Mode
|
||||
// The default Plan Mode restriction is priority 60 in Tier 1 (approx 1.06).
|
||||
// Any rule with priority > 1.1 (e.g. Tier 2/User rules) that allows a tool in Plan Mode should make that tool visible.
|
||||
const policyEngine = config.getPolicyEngine();
|
||||
const rules = policyEngine.getRules();
|
||||
for (const rule of rules) {
|
||||
const appliesToPlan =
|
||||
!rule.modes || rule.modes.includes(ApprovalMode.PLAN);
|
||||
// Priority is transformed: Tier 1 (1.xxx), Tier 2 (2.xxx).
|
||||
// We want to respect any user policy (Tier 2+) or high-priority default policy.
|
||||
const isHighPriority = (rule.priority ?? 0) > 1.1;
|
||||
const isAllowOrAsk =
|
||||
rule.decision === 'allow' || rule.decision === 'ask_user';
|
||||
|
||||
if (appliesToPlan && isHighPriority && isAllowOrAsk && rule.toolName) {
|
||||
allowedTools.add(rule.toolName);
|
||||
}
|
||||
}
|
||||
|
||||
let planModeToolsList = Array.from(allowedTools)
|
||||
.filter((t) => enabledToolNames.has(t))
|
||||
.map((t) => ` <tool>\`${t}\`</tool>`)
|
||||
.join('\n');
|
||||
|
||||
@@ -172,7 +192,7 @@ export class PromptProvider {
|
||||
'planningWorkflow',
|
||||
() => ({
|
||||
planModeToolsList,
|
||||
plansDir: config.storage.getProjectTempPlansDir(),
|
||||
plansDir: config.getPlanDirectory(),
|
||||
approvedPlanPath: config.getApprovedPlanPath(),
|
||||
}),
|
||||
isPlanMode,
|
||||
|
||||
Reference in New Issue
Block a user