Skip to content

Commit 703d2e0

Browse files
fix(patch): cherry-pick 37be162 to release/v0.23.0-preview.2-pr-15601 to patch version v0.23.0-preview.2 and create version 0.23.0-preview.3 (#15603)
Co-authored-by: Abhi <43648792+abhipatel12@users.noreply.github.com>
1 parent 7d8ab08 commit 703d2e0

File tree

6 files changed

+241
-22
lines changed

6 files changed

+241
-22
lines changed

packages/core/src/confirmation-bus/types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ export interface UpdatePolicy {
4141
toolName: string;
4242
persist?: boolean;
4343
argsPattern?: string;
44-
commandPrefix?: string;
44+
commandPrefix?: string | string[];
4545
mcpName?: string;
4646
}
4747

packages/core/src/policy/config.ts

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ interface TomlRule {
244244
mcpName?: string;
245245
decision?: string;
246246
priority?: number;
247-
commandPrefix?: string;
247+
commandPrefix?: string | string[];
248248
argsPattern?: string;
249249
// Index signature to satisfy Record type if needed for toml.stringify
250250
[key: string]: unknown;
@@ -258,26 +258,45 @@ export function createPolicyUpdater(
258258
MessageBusType.UPDATE_POLICY,
259259
async (message: UpdatePolicy) => {
260260
const toolName = message.toolName;
261-
let argsPattern = message.argsPattern
262-
? new RegExp(message.argsPattern)
263-
: undefined;
264261

265262
if (message.commandPrefix) {
266-
// Convert commandPrefix to argsPattern for in-memory rule
267-
// This mimics what toml-loader does
268-
const escapedPrefix = escapeRegex(message.commandPrefix);
269-
argsPattern = new RegExp(`"command":"${escapedPrefix}`);
270-
}
263+
// Convert commandPrefix(es) to argsPatterns for in-memory rules
264+
const prefixes = Array.isArray(message.commandPrefix)
265+
? message.commandPrefix
266+
: [message.commandPrefix];
267+
268+
for (const prefix of prefixes) {
269+
const escapedPrefix = escapeRegex(prefix);
270+
// Use robust regex to match whole words (e.g. "git" but not "github")
271+
const argsPattern = new RegExp(
272+
`"command":"${escapedPrefix}(?:[\\s"]|$)`,
273+
);
271274

272-
policyEngine.addRule({
273-
toolName,
274-
decision: PolicyDecision.ALLOW,
275-
// User tier (2) + high priority (950/1000) = 2.95
276-
// This ensures user "always allow" selections are high priority
277-
// but still lose to admin policies (3.xxx) and settings excludes (200)
278-
priority: 2.95,
279-
argsPattern,
280-
});
275+
policyEngine.addRule({
276+
toolName,
277+
decision: PolicyDecision.ALLOW,
278+
// User tier (2) + high priority (950/1000) = 2.95
279+
// This ensures user "always allow" selections are high priority
280+
// but still lose to admin policies (3.xxx) and settings excludes (200)
281+
priority: 2.95,
282+
argsPattern,
283+
});
284+
}
285+
} else {
286+
const argsPattern = message.argsPattern
287+
? new RegExp(message.argsPattern)
288+
: undefined;
289+
290+
policyEngine.addRule({
291+
toolName,
292+
decision: PolicyDecision.ALLOW,
293+
// User tier (2) + high priority (950/1000) = 2.95
294+
// This ensures user "always allow" selections are high priority
295+
// but still lose to admin policies (3.xxx) and settings excludes (200)
296+
priority: 2.95,
297+
argsPattern,
298+
});
299+
}
281300

282301
if (message.persist) {
283302
try {

packages/core/src/policy/persistence.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,9 @@ describe('createPolicyUpdater', () => {
121121
const addedRule = rules.find((r) => r.toolName === toolName);
122122
expect(addedRule).toBeDefined();
123123
expect(addedRule?.priority).toBe(2.95);
124-
expect(addedRule?.argsPattern).toEqual(new RegExp(`"command":"git status`));
124+
expect(addedRule?.argsPattern).toEqual(
125+
new RegExp(`"command":"git status(?:[\\s"]|$)`),
126+
);
125127

126128
// Verify file written
127129
expect(fs.writeFile).toHaveBeenCalledWith(
Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
/**
2+
* @license
3+
* Copyright 2025 Google LLC
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
8+
import * as fs from 'node:fs/promises';
9+
import { createPolicyUpdater } from './config.js';
10+
import { PolicyEngine } from './policy-engine.js';
11+
import { MessageBus } from '../confirmation-bus/message-bus.js';
12+
import { MessageBusType } from '../confirmation-bus/types.js';
13+
import { Storage } from '../config/storage.js';
14+
import toml from '@iarna/toml';
15+
import { ShellToolInvocation } from '../tools/shell.js';
16+
import { type Config } from '../config/config.js';
17+
import {
18+
ToolConfirmationOutcome,
19+
type PolicyUpdateOptions,
20+
} from '../tools/tools.js';
21+
import * as shellUtils from '../utils/shell-utils.js';
22+
23+
vi.mock('node:fs/promises');
24+
vi.mock('../config/storage.js');
25+
vi.mock('../utils/shell-utils.js', () => ({
26+
getCommandRoots: vi.fn(),
27+
stripShellWrapper: vi.fn(),
28+
}));
29+
interface ParsedPolicy {
30+
rule?: Array<{
31+
commandPrefix?: string | string[];
32+
}>;
33+
}
34+
35+
interface TestableShellToolInvocation {
36+
getPolicyUpdateOptions(
37+
outcome: ToolConfirmationOutcome,
38+
): PolicyUpdateOptions | undefined;
39+
}
40+
41+
describe('createPolicyUpdater', () => {
42+
let policyEngine: PolicyEngine;
43+
let messageBus: MessageBus;
44+
45+
beforeEach(() => {
46+
vi.resetAllMocks();
47+
policyEngine = new PolicyEngine({});
48+
vi.spyOn(policyEngine, 'addRule');
49+
50+
messageBus = new MessageBus(policyEngine);
51+
vi.spyOn(Storage, 'getUserPoliciesDir').mockReturnValue(
52+
'/mock/user/policies',
53+
);
54+
});
55+
56+
afterEach(() => {
57+
vi.restoreAllMocks();
58+
});
59+
60+
it('should add multiple rules when commandPrefix is an array', async () => {
61+
createPolicyUpdater(policyEngine, messageBus);
62+
63+
await messageBus.publish({
64+
type: MessageBusType.UPDATE_POLICY,
65+
toolName: 'run_shell_command',
66+
commandPrefix: ['echo', 'ls'],
67+
persist: false,
68+
});
69+
70+
expect(policyEngine.addRule).toHaveBeenCalledTimes(2);
71+
expect(policyEngine.addRule).toHaveBeenNthCalledWith(
72+
1,
73+
expect.objectContaining({
74+
toolName: 'run_shell_command',
75+
argsPattern: new RegExp('"command":"echo(?:[\\s"]|$)'),
76+
}),
77+
);
78+
expect(policyEngine.addRule).toHaveBeenNthCalledWith(
79+
2,
80+
expect.objectContaining({
81+
toolName: 'run_shell_command',
82+
argsPattern: new RegExp('"command":"ls(?:[\\s"]|$)'),
83+
}),
84+
);
85+
});
86+
87+
it('should add a single rule when commandPrefix is a string', async () => {
88+
createPolicyUpdater(policyEngine, messageBus);
89+
90+
await messageBus.publish({
91+
type: MessageBusType.UPDATE_POLICY,
92+
toolName: 'run_shell_command',
93+
commandPrefix: 'git',
94+
persist: false,
95+
});
96+
97+
expect(policyEngine.addRule).toHaveBeenCalledTimes(1);
98+
expect(policyEngine.addRule).toHaveBeenCalledWith(
99+
expect.objectContaining({
100+
toolName: 'run_shell_command',
101+
argsPattern: new RegExp('"command":"git(?:[\\s"]|$)'),
102+
}),
103+
);
104+
});
105+
106+
it('should persist multiple rules correctly to TOML', async () => {
107+
createPolicyUpdater(policyEngine, messageBus);
108+
vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' });
109+
vi.mocked(fs.mkdir).mockResolvedValue(undefined);
110+
vi.mocked(fs.writeFile).mockResolvedValue(undefined);
111+
vi.mocked(fs.rename).mockResolvedValue(undefined);
112+
113+
await messageBus.publish({
114+
type: MessageBusType.UPDATE_POLICY,
115+
toolName: 'run_shell_command',
116+
commandPrefix: ['echo', 'ls'],
117+
persist: true,
118+
});
119+
120+
// Wait for the async listener to complete
121+
await new Promise((resolve) => setTimeout(resolve, 0));
122+
123+
expect(fs.writeFile).toHaveBeenCalled();
124+
const [_path, content] = vi.mocked(fs.writeFile).mock.calls[0] as [
125+
string,
126+
string,
127+
];
128+
const parsed = toml.parse(content) as unknown as ParsedPolicy;
129+
130+
expect(parsed.rule).toHaveLength(1);
131+
expect(parsed.rule![0].commandPrefix).toEqual(['echo', 'ls']);
132+
});
133+
});
134+
135+
describe('ShellToolInvocation Policy Update', () => {
136+
let mockConfig: Config;
137+
let mockMessageBus: MessageBus;
138+
139+
beforeEach(() => {
140+
vi.resetAllMocks();
141+
mockConfig = {} as Config;
142+
mockMessageBus = {} as MessageBus;
143+
144+
vi.mocked(shellUtils.stripShellWrapper).mockImplementation(
145+
(c: string) => c,
146+
);
147+
});
148+
149+
it('should extract multiple root commands for chained commands', () => {
150+
vi.mocked(shellUtils.getCommandRoots).mockReturnValue(['git', 'npm']);
151+
152+
const invocation = new ShellToolInvocation(
153+
mockConfig,
154+
{ command: 'git status && npm test' },
155+
new Set(),
156+
mockMessageBus,
157+
'run_shell_command',
158+
'Shell',
159+
);
160+
161+
// Accessing protected method for testing
162+
const options = (
163+
invocation as unknown as TestableShellToolInvocation
164+
).getPolicyUpdateOptions(ToolConfirmationOutcome.ProceedAlways);
165+
expect(options!.commandPrefix).toEqual(['git', 'npm']);
166+
expect(shellUtils.getCommandRoots).toHaveBeenCalledWith(
167+
'git status && npm test',
168+
);
169+
});
170+
171+
it('should extract a single root command', () => {
172+
vi.mocked(shellUtils.getCommandRoots).mockReturnValue(['ls']);
173+
174+
const invocation = new ShellToolInvocation(
175+
mockConfig,
176+
{ command: 'ls -la /tmp' },
177+
new Set(),
178+
mockMessageBus,
179+
'run_shell_command',
180+
'Shell',
181+
);
182+
183+
// Accessing protected method for testing
184+
const options = (
185+
invocation as unknown as TestableShellToolInvocation
186+
).getPolicyUpdateOptions(ToolConfirmationOutcome.ProceedAlways);
187+
expect(options!.commandPrefix).toEqual(['ls']);
188+
expect(shellUtils.getCommandRoots).toHaveBeenCalledWith('ls -la /tmp');
189+
});
190+
});

packages/core/src/tools/shell.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,15 @@ export class ShellToolInvocation extends BaseToolInvocation<
8989
protected override getPolicyUpdateOptions(
9090
outcome: ToolConfirmationOutcome,
9191
): PolicyUpdateOptions | undefined {
92-
if (outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave) {
92+
if (
93+
outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave ||
94+
outcome === ToolConfirmationOutcome.ProceedAlways
95+
) {
96+
const command = stripShellWrapper(this.params.command);
97+
const rootCommands = [...new Set(getCommandRoots(command))];
98+
if (rootCommands.length > 0) {
99+
return { commandPrefix: rootCommands };
100+
}
93101
return { commandPrefix: this.params.command };
94102
}
95103
return undefined;

packages/core/src/tools/tools.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ export interface ToolInvocation<
6969
* Options for policy updates that can be customized by tool invocations.
7070
*/
7171
export interface PolicyUpdateOptions {
72-
commandPrefix?: string;
72+
commandPrefix?: string | string[];
7373
mcpName?: string;
7474
}
7575

0 commit comments

Comments
 (0)