Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions packages/cli/src/acp-integration/session/permissionUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,64 @@ describe('permissionUtils', () => {
});
});

it('renders the hook ask reason on edit, exec, and info permission requests', () => {
// A PreToolUse hook escalated these calls (#9434): off-TUI surfaces
// must carry the reason so the prompt is distinguishable from an
// ordinary permission request. The info class is the ask bounce's
// synthetic fallback for tools without a structured view (#9434
// review R5-2).
const editContent = buildPermissionRequestContent({
type: 'edit',
title: 'Confirm edit',
fileName: 'a.txt',
filePath: '/tmp/a.txt',
fileDiff: 'diff',
originalContent: 'a',
newContent: 'b',
hookAskReason: 'path requires human review',
onConfirm: async () => undefined,
});
expect(editContent).toContainEqual({
type: 'content',
content: {
type: 'text',
text: 'Hook requested confirmation: path requires human review',
},
});

const execContent = buildPermissionRequestContent({
type: 'exec',
title: 'Confirm shell',
command: 'git status',
rootCommand: 'git',
hookAskReason: 'path requires human review',
onConfirm: async () => undefined,
});
expect(execContent).toContainEqual({
type: 'content',
content: {
type: 'text',
text: 'Hook requested confirmation: path requires human review',
},
});

const infoContent = buildPermissionRequestContent({
type: 'info',
title: 'Hook requested confirmation to run web_fetch',
prompt: 'network egress requires human review',
renderPromptAsPlainText: true,
hookAskReason: 'network egress requires human review',
onConfirm: async () => undefined,
});
expect(infoContent).toContainEqual({
type: 'content',
content: {
type: 'text',
text: 'Hook requested confirmation: network egress requires human review',
},
});
});

it('accepts only an option that was actually offered', () => {
const options = toPermissionOptions({
type: 'exec',
Expand Down
20 changes: 20 additions & 0 deletions packages/cli/src/acp-integration/session/permissionUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,26 @@ export function buildPermissionRequestContent(
});
}

// A PreToolUse hook escalated this call (#9434): surface the hook's
// reason alongside the warnings so a hook-forced prompt is
// distinguishable from an ordinary one off-TUI. The info class is the
// ask bounce's synthetic fallback for tools without a structured view
// (#9434 review R5-2).
if (
(confirmation.type === 'exec' ||
confirmation.type === 'edit' ||
confirmation.type === 'info') &&
confirmation.hookAskReason
) {
Comment thread
yiliang114 marked this conversation as resolved.
content.push({
type: 'content',
content: {
type: 'text',
text: `Hook requested confirmation: ${confirmation.hookAskReason}`,
},
});
}

if (confirmation.type === 'edit') {
content.push({
type: 'diff',
Expand Down
85 changes: 85 additions & 0 deletions packages/cli/src/nonInteractive/permission-suggestions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**
* @license
* Copyright 2025 Qwen
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect } from 'vitest';
import { buildPermissionSuggestions } from './permission-suggestions.js';
Comment thread
yiliang114 marked this conversation as resolved.

describe('buildPermissionSuggestions', () => {
it('prepends the hook ask reason on exec and edit suggestions', () => {
// A PreToolUse hook escalated these calls (#9434): the stream-json
// can_use_tool suggestions must carry the reason so a hook-forced
// prompt is distinguishable from an ordinary one off-TUI.
const exec = buildPermissionSuggestions({
type: 'exec',
title: 'Confirm shell',
command: 'git status',
rootCommand: 'git',
hookAskReason: 'path requires human review',
onConfirm: async () => undefined,
});
expect(exec?.[0]?.description).toBe(
'Hook requested confirmation: path requires human review\nExecute: git status',
);

const edit = buildPermissionSuggestions({
type: 'edit',
title: 'Confirm edit',
fileName: 'a.txt',
filePath: '/tmp/a.txt',
fileDiff: 'diff',
originalContent: 'a',
newContent: 'b',
hookAskReason: 'path requires human review',
onConfirm: async () => undefined,
});
expect(edit?.[0]?.description).toBe(
'Hook requested confirmation: path requires human review\nEdit file: a.txt',
);
});

it('prepends the hook ask reason on info suggestions (ask-bounce fallback)', () => {
// Tools without a structured view land in the ask bounce's synthetic
// info prompt; the stream-json suggestions must carry the hook reason
// there too (#9434 review R5-2).
const info = buildPermissionSuggestions({
type: 'info',
title: 'Hook requested confirmation to run web_fetch',
prompt: 'network egress requires human review',
hookAskReason: 'network egress requires human review',
onConfirm: async () => undefined,
});
expect(info?.[0]?.description).toBe(
'Hook requested confirmation: network egress requires human review\n' +
'Hook requested confirmation to run web_fetch',
);
});

it('keeps warnings below the hook reason and above the description', () => {
const exec = buildPermissionSuggestions({
type: 'exec',
title: 'Confirm shell',
command: 'curl $(evil)',
rootCommand: 'curl',
warnings: ['Contains command substitution'],
hookAskReason: 'network egress',
onConfirm: async () => undefined,
});
expect(exec?.[0]?.description).toBe(
'Hook requested confirmation: network egress\nContains command substitution\nExecute: curl $(evil)',
);
});

it('leaves ordinary suggestions unchanged without a hook reason', () => {
const exec = buildPermissionSuggestions({
type: 'exec',
title: 'Confirm shell',
command: 'git status',
rootCommand: 'git',
onConfirm: async () => undefined,
});
expect(exec?.[0]?.description).toBe('Execute: git status');
});
});
23 changes: 20 additions & 3 deletions packages/cli/src/nonInteractive/permission-suggestions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,24 @@ function withWarnings(
description: string,
details: Record<string, unknown>,
): string {
const prefixes: string[] = [];
// A PreToolUse hook escalated this call (#9434): surface the hook's
// reason on the non-TUI permission surfaces too, so a hook-forced
// prompt is distinguishable from an ordinary one off-TUI.
const hookAskReason = details['hookAskReason'];
if (typeof hookAskReason === 'string' && hookAskReason.length > 0) {
prefixes.push(`Hook requested confirmation: ${hookAskReason}`);
}
const warnings = Array.isArray(details['warnings'])
? details['warnings'].filter(
(warning): warning is string => typeof warning === 'string',
)
: [];
return warnings.length > 0
? `${warnings.join('\n')}\n${description}`
if (warnings.length > 0) {
prefixes.push(warnings.join('\n'));
}
return prefixes.length > 0
? `${prefixes.join('\n')}\n${description}`
: description;
}

Expand Down Expand Up @@ -106,7 +117,13 @@ export function buildPermissionSuggestions(
{
type: 'allow',
label: 'Allow Info Request',
description: title || 'Allow information request',
// withWarnings surfaces a PreToolUse hook's ask reason on the
// synthetic info fallback the ask bounce uses for tools without
// a structured view (#9434 review R5-2).
description: withWarnings(
title || 'Allow information request',
details,
),
},
{
type: 'deny',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1094,4 +1094,109 @@ describe('ToolConfirmationMessage', () => {
expect(frame).toMatch(/\.{3} last \d+ lines hidden \.{3}/);
});
});

describe('PreToolUse hook ask reason (#9434)', () => {
it('shows the hook reason alongside the edit diff', () => {
const confirmationDetails: ToolCallConfirmationDetails = {
type: 'edit',
title: 'Confirm Edit: config.yaml',
fileName: 'config.yaml',
filePath: '/repo/config.yaml',
fileDiff: '-old\n+new',
originalContent: 'old',
newContent: 'new',
hideAlwaysAllow: true,
hookAskReason: 'path requires human review',
onConfirm: vi.fn(),
};

const { lastFrame } = renderWithProviders(
<ToolConfirmationMessage
confirmationDetails={confirmationDetails}
config={mockConfig}
availableTerminalHeight={30}
contentWidth={80}
/>,
);

const frame = lastFrame() ?? '';
// The hook reason is surfaced above the tool's diff view...
expect(frame).toContain(
'Hook requested confirmation: path requires human review',
);
// ...and the edit confirmation itself still renders.
expect(frame).toContain('Apply this change?');
});

it('does not show a hook reason line for ordinary confirmations', () => {
const confirmationDetails: ToolCallConfirmationDetails = {
type: 'edit',
title: 'Confirm Edit',
fileName: 'test.txt',
filePath: '/test.txt',
fileDiff: '...diff...',
originalContent: 'a',
newContent: 'b',
onConfirm: vi.fn(),
};

const { lastFrame } = renderWithProviders(
<ToolConfirmationMessage
confirmationDetails={confirmationDetails}
config={mockConfig}
availableTerminalHeight={30}
contentWidth={80}
/>,
);

expect(lastFrame()).not.toContain('Hook requested confirmation:');
});

// The 5-line reason cap and the matching HOOK_ASK_REASON_HEIGHT
// reservation have to be pinned together (#9434 review R3-7):
// a verbose hook reason must not push the question/options off a
// short terminal. Template: the warnings reservation test above.
it('caps a verbose hook reason at five lines and reserves its height on a short terminal', () => {
const reason = Array.from(
{ length: 8 },
(_, i) => `hook reason line ${i + 1}`,
).join('\n');
const confirmationDetails: ToolCallConfirmationDetails = {
type: 'edit',
title: 'Confirm Edit: config.yaml',
fileName: 'config.yaml',
filePath: '/repo/config.yaml',
fileDiff: ['-old-1', '-old-2', '+new-1', '+new-2'].join('\n'),
originalContent: 'old-1\nold-2',
newContent: 'new-1\nnew-2',
hideAlwaysAllow: true,
hookAskReason: reason,
onConfirm: vi.fn(),
};

const { lastFrame } = renderWithProviders(
<ToolConfirmationMessage
confirmationDetails={confirmationDetails}
config={mockConfig}
availableTerminalHeight={10}
contentWidth={80}
/>,
);

const frame = lastFrame() ?? '';
// Only the first five reason lines render, plus the ellipsis.
expect(frame).toContain('hook reason line 1');
expect(frame).toContain('hook reason line 5');
expect(frame).not.toContain('hook reason line 6');
expect(frame).toContain('…');
// The reason's height is reserved from the body cap: with the
// reservation the body collapses to the '... diff hidden ...'
// placeholder; without it the cap computes too loose and the diff
// renders instead.
expect(frame).toContain('... diff hidden ...');
// The question and options stay on-screen.
expect(frame).toContain('Apply this change?');
expect(frame).toContain('Yes, allow once');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,19 @@ export const ToolConfirmationMessage: React.FC<
}) => {
const { onConfirm } = confirmationDetails;
const autoModeFallback = confirmationDetails.autoModeFallback;
// Reason a PreToolUse hook gave when escalating this call to an
// interactive confirmation while the tool's own view (e.g. edit diff)
// is being shown (#9434). Undefined for ordinary confirmations.
const hookAskReason = confirmationDetails.hookAskReason;
// Cap the hook reason to a few lines so a verbose hook cannot push the
// question/options off a short terminal (#9434 follow-up).
const HOOK_ASK_REASON_MAX_LINES = 5;
const cappedHookAskReason = hookAskReason
Comment thread
yiliang114 marked this conversation as resolved.
? hookAskReason.split('\n').slice(0, HOOK_ASK_REASON_MAX_LINES).join('\n') +
Comment thread
yiliang114 marked this conversation as resolved.
(hookAskReason.split('\n').length > HOOK_ASK_REASON_MAX_LINES
? '\n…'
: '')
: undefined;

const settings = useSettings();
const preferredEditor = settings.merged.general?.preferredEditor as
Expand Down Expand Up @@ -192,14 +205,22 @@ export const ToolConfirmationMessage: React.FC<
hard: true,
}).split('\n').length + 1
: 0;
const HOOK_ASK_REASON_HEIGHT = cappedHookAskReason
? warningsHeight([
t('Hook requested confirmation: {{reason}}', {
reason: cappedHookAskReason,
}),
])
: 0;

const surroundingElementsHeight =
PADDING_OUTER_Y +
MARGIN_BODY_BOTTOM +
HEIGHT_QUESTION +
MARGIN_QUESTION_BOTTOM +
HEIGHT_OPTIONS +
AUTO_MODE_FALLBACK_HEIGHT;
AUTO_MODE_FALLBACK_HEIGHT +
HOOK_ASK_REASON_HEIGHT;
return Math.max(availableTerminalHeight - surroundingElementsHeight, 1);
}

Expand Down Expand Up @@ -696,6 +717,25 @@ export const ToolConfirmationMessage: React.FC<
);
}

// A PreToolUse hook escalated this call while the tool's own view (e.g.
// the edit diff) is shown — surface the hook's reason above the body so
// the user knows why the prompt appeared (#9434).
if (cappedHookAskReason) {
bodyContent = (
<Box flexDirection="column">
Comment thread
yiliang114 marked this conversation as resolved.
Comment thread
yiliang114 marked this conversation as resolved.
<Box paddingX={1} marginLeft={1} marginBottom={1}>
<Text color={theme.status.warning}>
⚠{' '}
{t('Hook requested confirmation: {{reason}}', {
Comment thread
yiliang114 marked this conversation as resolved.
Comment thread
yiliang114 marked this conversation as resolved.
reason: cappedHookAskReason,
})}
</Text>
</Box>
{bodyContent}
</Box>
);
}

// For exec/mcp confirmations the type-specific question text would
// restate what the body already shows (the full command, or the labeled
// server + tool). Use the generic prompt so the question line acts as a
Expand Down
Loading
Loading