Skip to content

Commit 7bb4205

Browse files
LaZzyManLaZzyMan
committed
fix(permissions): align monitor + propagate warnings to ACP/non-interactive (#4386 review)
Addresses round-2 review findings from wenshao on PR #4386. Four related findings, all about sibling drift from the original #4093 fix: either the same hard-deny-on-substitution pattern in a sibling tool, or downstream consumers that don't propagate the new warnings field. 1. monitor.ts: `MonitorToolInvocation.getDefaultPermission()` had the identical `detectCommandSubstitution → 'deny'` pattern that the original PR removed from `PermissionManager.resolveDefaultPermission` — sibling drift the original audit missed. Same UX problems apply (YOLO unbypassable, opaque deny). Removed the deny branch, added the substitution warning in `getConfirmationDetails` (mirroring ShellToolInvocation), updated the 5 existing tests that asserted the old 'deny' to assert 'ask', and added a confirmation-warning test for the issue #4093 mirror case. Monitor still maintains its separate permission boundary (Monitor(...) rules don't share with Bash(...) — documented in the unchanged comment at the top of getConfirmationDetails); only the substitution-deny half is removed. 2. ACP `buildPermissionRequestContent` (permissionUtils.ts): had no `exec` branch, so the new `warnings` field never reached ACP clients (IDE integrations etc.). Added an `exec` branch that emits one `⚠ <warning>` text content per warning, alongside the existing `edit` (diff) and `plan` (text) branches. Two regression tests added: warning present → ⚠ content entry; no warnings → empty. 3. Non-interactive `permissionController.buildPermissionSuggestions`: the `case 'exec'` description string didn't read `warnings`, so daemon/API consumers that delegate the approval decision back to a user lost the substitution context. Appended warnings as a parenthesized `⚠ ...` suffix to the description string. (No test file exists for this controller — change is small and verifiable by inspection; adding test scaffolding is out of scope for this round.) 4. Test coverage: the `command substitution (issue #4093)` describe blocks in `shell.test.ts` and `permission-manager.test.ts` covered `$()`, backticks, and `<()` but not `>()` — even though `detectCommandSubstitution` and the warning text both list it. Added a `>()` case to each block to close the regression gap.
1 parent c672223 commit 7bb4205

7 files changed

Lines changed: 163 additions & 19 deletions

File tree

‎packages/cli/src/acp-integration/session/permissionUtils.test.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@
66

77
import { describe, expect, it } from 'vitest';
88
import { ToolConfirmationOutcome } from '@qwen-code/qwen-code-core';
9-
import { toPermissionOptions } from './permissionUtils.js';
9+
import {
10+
buildPermissionRequestContent,
11+
toPermissionOptions,
12+
} from './permissionUtils.js';
1013

1114
describe('permissionUtils', () => {
1215
describe('toPermissionOptions', () => {
@@ -94,4 +97,46 @@ describe('permissionUtils', () => {
9497
);
9598
});
9699
});
100+
101+
// Regression coverage for #4386 review: shell commands flagged with a
102+
// command-substitution warning should propagate that warning into the
103+
// ACP content channel so IDE clients can surface it. Without this,
104+
// IDE users approving substitution commands would see no hint.
105+
describe('buildPermissionRequestContent', () => {
106+
it('emits ⚠ content entries for each exec warning', () => {
107+
const content = buildPermissionRequestContent({
108+
type: 'exec',
109+
title: 'Confirm Shell Command',
110+
command: 'python3 -c "print($(echo hello))"',
111+
rootCommand: 'python3',
112+
warnings: [
113+
'Contains command substitution ($(...), backticks, <(...), or >(...)).',
114+
],
115+
onConfirm: async () => undefined,
116+
});
117+
118+
expect(content).toHaveLength(1);
119+
expect(content[0]).toMatchObject({
120+
type: 'content',
121+
content: { type: 'text' },
122+
});
123+
const node = content[0] as {
124+
type: 'content';
125+
content: { type: 'text'; text: string };
126+
};
127+
expect(node.content.text).toMatch(/^⚠ .*command substitution/);
128+
});
129+
130+
it('emits no content for exec confirmations without warnings', () => {
131+
const content = buildPermissionRequestContent({
132+
type: 'exec',
133+
title: 'Confirm Shell Command',
134+
command: 'npm install',
135+
rootCommand: 'npm',
136+
onConfirm: async () => undefined,
137+
});
138+
139+
expect(content).toEqual([]);
140+
});
141+
});
97142
});

‎packages/cli/src/acp-integration/session/permissionUtils.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,22 @@ export function buildPermissionRequestContent(
9292
});
9393
}
9494

95+
// Propagate informational warnings (e.g. command substitution detected
96+
// in a shell command — see #4093) so ACP clients can surface them when
97+
// prompting the user for approval. The tool input itself is already
98+
// visible to the host, so we only emit the new warning channel here.
99+
if (confirmation.type === 'exec' && confirmation.warnings?.length) {
100+
for (const warning of confirmation.warnings) {
101+
content.push({
102+
type: 'content',
103+
content: {
104+
type: 'text',
105+
text: `⚠ ${warning}`,
106+
},
107+
});
108+
}
109+
}
110+
95111
return content;
96112
}
97113

‎packages/cli/src/nonInteractive/control/controllers/permissionController.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -263,19 +263,36 @@ export class PermissionController extends BaseController {
263263
const confirmationType = type as ToolConfirmationType;
264264

265265
switch (confirmationType) {
266-
case 'exec': // ToolExecuteConfirmationDetails
266+
case 'exec': {
267+
// ToolExecuteConfirmationDetails. Propagate any informational
268+
// warnings (e.g. command substitution flagged by the shell tool
269+
// — see #4093) into the suggestion description so daemon/API
270+
// consumers can surface them to the end-user when prompting
271+
// for approval. Mitigated by auto-deny in non-interactive
272+
// default mode, but matters for hosts that delegate the choice
273+
// back to a user.
274+
const warnings = Array.isArray(details['warnings'])
275+
? (details['warnings'] as unknown[]).filter(
276+
(w): w is string => typeof w === 'string',
277+
)
278+
: [];
279+
const warningSuffix =
280+
warnings.length > 0
281+
? ` (${warnings.map((w) => `⚠ ${w}`).join('; ')})`
282+
: '';
267283
return [
268284
{
269285
type: 'allow',
270286
label: 'Allow Command',
271-
description: `Execute: ${details['command']}`,
287+
description: `Execute: ${details['command']}${warningSuffix}`,
272288
},
273289
{
274290
type: 'deny',
275291
label: 'Deny',
276292
description: 'Block this command execution',
277293
},
278294
];
295+
}
279296

280297
case 'edit': // ToolEditConfirmationDetails
281298
return [

‎packages/core/src/permissions/permission-manager.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,6 +1058,15 @@ describe('PermissionManager', () => {
10581058
).toBe('ask');
10591059
});
10601060

1061+
it('returns ask for >() output process substitution', async () => {
1062+
expect(
1063+
await pm.evaluate({
1064+
toolName: 'run_shell_command',
1065+
command: 'echo data > >(tee log.txt)',
1066+
}),
1067+
).toBe('ask');
1068+
});
1069+
10611070
it('still honors explicit deny rules over substitution-bearing commands', async () => {
10621071
// The 'ask' from substitution must never downgrade a real deny rule.
10631072
expect(

‎packages/core/src/tools/monitor.test.ts‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -416,44 +416,49 @@ describe('MonitorTool', () => {
416416
});
417417

418418
describe('getDefaultPermission', () => {
419-
it('denies command substitution before confirmation', async () => {
419+
// Command substitution previously returned 'deny' here. Per #4093 it
420+
// now falls through to 'ask' (matching ShellToolInvocation and
421+
// PermissionManager.resolveDefaultPermission); the substitution
422+
// warning is surfaced via getConfirmationDetails. YOLO mode can now
423+
// override the prompt; before this change it could not.
424+
it('asks for command substitution before confirmation', async () => {
420425
const invocation = createInvocation({
421426
command: 'echo $(cat secret.txt)',
422427
});
423428

424-
await expect(invocation.getDefaultPermission()).resolves.toBe('deny');
429+
await expect(invocation.getDefaultPermission()).resolves.toBe('ask');
425430
});
426431

427-
it('denies command substitution inside explicit shell wrappers', async () => {
432+
it('asks for command substitution inside explicit shell wrappers', async () => {
428433
const invocation = createInvocation({
429434
command: `/bin/bash -c 'echo $(cat secret.txt)'`,
430435
});
431436

432-
await expect(invocation.getDefaultPermission()).resolves.toBe('deny');
437+
await expect(invocation.getDefaultPermission()).resolves.toBe('ask');
433438
});
434439

435-
it('denies command substitution inside wrapped scripts with argv suffixes', async () => {
440+
it('asks for command substitution inside wrapped scripts with argv suffixes', async () => {
436441
const invocation = createInvocation({
437442
command: `/bin/bash -c 'echo $(cat secret.txt)' ignored`,
438443
});
439444

440-
await expect(invocation.getDefaultPermission()).resolves.toBe('deny');
445+
await expect(invocation.getDefaultPermission()).resolves.toBe('ask');
441446
});
442447

443-
it('denies command substitution inside quoted env-prefixed wrappers', async () => {
448+
it('asks for command substitution inside quoted env-prefixed wrappers', async () => {
444449
const invocation = createInvocation({
445450
command: `FOO="bar baz" /bin/bash -c 'echo $(cat secret.txt)'`,
446451
});
447452

448-
await expect(invocation.getDefaultPermission()).resolves.toBe('deny');
453+
await expect(invocation.getDefaultPermission()).resolves.toBe('ask');
449454
});
450455

451-
it('denies command substitution inside env-prefix assignments', async () => {
456+
it('asks for command substitution inside env-prefix assignments', async () => {
452457
const invocation = createInvocation({
453458
command: `FOO=$(cat secret.txt) /bin/bash -c 'echo ok'`,
454459
});
455460

456-
await expect(invocation.getDefaultPermission()).resolves.toBe('deny');
461+
await expect(invocation.getDefaultPermission()).resolves.toBe('ask');
457462
});
458463

459464
it('allows read-only monitor commands by default', async () => {
@@ -464,6 +469,17 @@ describe('MonitorTool', () => {
464469

465470
await expect(invocation.getDefaultPermission()).resolves.toBe('allow');
466471
});
472+
473+
it('surfaces a command-substitution warning via getConfirmationDetails (issue #4093)', async () => {
474+
const invocation = createInvocation({
475+
command: 'echo $(cat secret.txt)',
476+
});
477+
const details = (await invocation.getConfirmationDetails(
478+
new AbortController().signal,
479+
)) as { warnings?: string[] };
480+
481+
expect(details.warnings?.[0]).toMatch(/command substitution/i);
482+
});
467483
});
468484

469485
describe('validation', () => {

‎packages/core/src/tools/monitor.ts‎

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -171,10 +171,18 @@ class MonitorToolInvocation extends BaseToolInvocation<
171171
this.params.command,
172172
).safetyCommand;
173173

174-
if (detectCommandSubstitution(command)) {
175-
return 'deny';
176-
}
177-
174+
// Command substitution ($(), ``, <(), >()) is NOT a hard deny here —
175+
// it falls through to 'ask' along with every other non-read-only
176+
// command, so the user (or YOLO mode) can decide. The user-facing
177+
// warning is surfaced by getConfirmationDetails below so the
178+
// confirmation prompt still flags the substitution clearly. This
179+
// mirrors the same reasoning applied to ShellToolInvocation and
180+
// PermissionManager.resolveDefaultPermission in #4093: a hard deny
181+
// here (a) cannot be overridden by YOLO and (b) was inconsistent
182+
// with the shell-tool path. Monitor still maintains a separate
183+
// permission boundary (Monitor(...) rules don't share with
184+
// Bash(...) — see comment in getConfirmationDetails); only the
185+
// substitution-deny half is removed.
178186
try {
179187
const isReadOnly = await isShellCommandReadOnlyAST(command);
180188
if (isReadOnly) {
@@ -246,7 +254,24 @@ class MonitorToolInvocation extends BaseToolInvocation<
246254
permissionRules = [`Monitor(${normalized.safetyCommand})`];
247255
}
248256

249-
return {
257+
// Flag command substitution ($(), backticks, <(), >()) so the user
258+
// sees a visible warning in the confirmation dialog. Mirrors the
259+
// pattern in ShellToolInvocation.getConfirmationDetails — see #4093
260+
// for why we surface this as a warning rather than denying outright.
261+
// Checked against both the normalized safety command and the
262+
// original params.command so wrappers like `bash -c "..."` still
263+
// trigger the warning.
264+
const warnings: string[] = [];
265+
if (
266+
detectCommandSubstitution(normalized.safetyCommand) ||
267+
detectCommandSubstitution(this.params.command)
268+
) {
269+
warnings.push(
270+
'Contains command substitution ($(...), backticks, <(...), or >(...)).',
271+
);
272+
}
273+
274+
const confirmationDetails: ToolExecuteConfirmationDetails = {
250275
type: 'exec',
251276
title: 'Monitor',
252277
command: normalized.spawnCommand,
@@ -258,7 +283,11 @@ class MonitorToolInvocation extends BaseToolInvocation<
258283
_outcome: ToolConfirmationOutcome,
259284
_payload?: ToolConfirmationPayload,
260285
) => {},
261-
} satisfies ToolExecuteConfirmationDetails;
286+
};
287+
if (warnings.length > 0) {
288+
confirmationDetails.warnings = warnings;
289+
}
290+
return confirmationDetails;
262291
}
263292

264293
async execute(_signal: AbortSignal): Promise<ToolResult> {

‎packages/core/src/tools/shell.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5011,6 +5011,18 @@ describe('ShellTool', () => {
50115011
expect(details.warnings?.[0]).toMatch(/command substitution/i);
50125012
});
50135013

5014+
it('surfaces a warning for >() output process substitution', async () => {
5015+
const invocation = shellTool.build({
5016+
command: 'echo data > >(tee log.txt)',
5017+
is_background: false,
5018+
});
5019+
const details = (await invocation.getConfirmationDetails(
5020+
new AbortController().signal,
5021+
)) as { warnings?: string[] };
5022+
5023+
expect(details.warnings?.[0]).toMatch(/command substitution/i);
5024+
});
5025+
50145026
it('does not set warnings on commands without substitution', async () => {
50155027
const invocation = shellTool.build({
50165028
command: 'npm install',

0 commit comments

Comments
 (0)