Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@

import { describe, it, expect, vi } from 'vitest';
import { EOL } from 'node:os';
import { promises as fsp } from 'node:fs';

// Capture launches of the external editor so the full-plan viewer (#7001)
// can be asserted without spawning a real editor process.
const { launchEditorMock } = vi.hoisted(() => ({
launchEditorMock: vi.fn((_filePath: string) => Promise.resolve()),
}));
vi.mock('../../hooks/useLaunchEditor.js', () => ({
useLaunchEditor: () => launchEditorMock,
}));

import { ToolConfirmationMessage } from './ToolConfirmationMessage.js';
import type {
ToolCallConfirmationDetails,
Expand Down Expand Up @@ -202,6 +213,93 @@ describe('ToolConfirmationMessage', () => {
expect(lastFrame()).toContain('Step one');
});

describe('full-plan viewer (#7001)', () => {
const plan = [
'# Big Plan',
...Array.from({ length: 60 }, (_, i) => `- Step ${i + 1}`),
].join('\n');

const planDetails = (onConfirm = vi.fn()): ToolCallConfirmationDetails => ({
type: 'plan',
title: 'Would you like to proceed?',
plan,
onConfirm,
});

it('shows the open-in-editor hint on plan confirmations', () => {
launchEditorMock.mockClear();
const { lastFrame } = renderWithProviders(
<ToolConfirmationMessage
confirmationDetails={planDetails()}
config={mockConfig}
availableTerminalHeight={30}
contentWidth={80}
/>,
);
expect(lastFrame()).toContain('o open full plan in editor');
});

it('`o` writes the FULL plan to a temp file and opens the editor without confirming', async () => {
launchEditorMock.mockClear();
const onConfirm = vi.fn();
const { stdin } = renderWithProviders(
<ToolConfirmationMessage
confirmationDetails={planDetails(onConfirm)}
config={mockConfig}
availableTerminalHeight={30}
contentWidth={80}
/>,
);

stdin.write('o');
await vi.waitFor(() => expect(launchEditorMock).toHaveBeenCalledTimes(1));

const openedPath = launchEditorMock.mock.calls[0]![0];
// The staged file must contain the COMPLETE plan, not the truncated view.
expect(await fsp.readFile(openedPath, 'utf-8')).toBe(plan);
// Viewing the plan must not resolve the confirmation either way.
expect(onConfirm).not.toHaveBeenCalled();
});

it('Ctrl+O does not open the editor', async () => {
launchEditorMock.mockClear();
const { stdin } = renderWithProviders(
<ToolConfirmationMessage
confirmationDetails={planDetails()}
config={mockConfig}
availableTerminalHeight={30}
contentWidth={80}
/>,
);
stdin.write('\x0f'); // Ctrl+O
await new Promise((r) => setTimeout(r, 50));
expect(launchEditorMock).not.toHaveBeenCalled();
});

it('`o` is inert for non-plan confirmations', async () => {
launchEditorMock.mockClear();
const confirmationDetails: ToolCallConfirmationDetails = {
type: 'info',
title: 'Confirm Web Fetch',
prompt: 'https://example.com',
urls: ['https://example.com'],
onConfirm: vi.fn(),
};
const { stdin, lastFrame } = renderWithProviders(
<ToolConfirmationMessage
confirmationDetails={confirmationDetails}
config={mockConfig}
availableTerminalHeight={30}
contentWidth={80}
/>,
);
expect(lastFrame()).not.toContain('o open full plan in editor');
stdin.write('o');
await new Promise((r) => setTimeout(r, 50));
expect(launchEditorMock).not.toHaveBeenCalled();
});
});

describe('with folder trust', () => {
const editConfirmationDetails: ToolCallConfirmationDetails = {
type: 'edit',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

import type React from 'react';
import { useEffect, useState } from 'react';
import { promises as fs } from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { Box, Text } from 'ink';
import { DiffRenderer } from './DiffRenderer.js';
import { RenderInline } from '../../utils/InlineMarkdownRenderer.js';
Expand All @@ -26,6 +29,7 @@ import type { RadioSelectItem } from '../shared/RadioButtonSelect.js';
import { RadioButtonSelect } from '../shared/RadioButtonSelect.js';
import { MaxSizedBox } from '../shared/MaxSizedBox.js';
import { useKeypress } from '../../hooks/useKeypress.js';
import { useLaunchEditor } from '../../hooks/useLaunchEditor.js';
import { useSettings } from '../../contexts/SettingsContext.js';
import { theme } from '../../semantic-colors.js';
import { t } from '../../../i18n/index.js';
Expand Down Expand Up @@ -103,11 +107,42 @@ export const ToolConfirmationMessage: React.FC<

const isTrustedFolder = config.isTrustedFolder();

const launchEditor = useLaunchEditor();
const [planViewError, setPlanViewError] = useState<string | null>(null);

// #7001: a long plan can exceed the confirmation dialog's height budget and
// get truncated (with a "... N more lines not shown ..." cue since #6882),
// yet the user is asked to approve it. `o` writes the FULL plan to a temp
// file and opens it in the configured editor so the decision is informed;
// the dialog stays open (nothing is confirmed) while the user reads.
const openFullPlanInEditor = () => {
if (confirmationDetails.type !== 'plan') return;
setPlanViewError(null);
const openPlan = async () => {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-plan-'));
const planPath = path.join(dir, 'plan.md');
await fs.writeFile(planPath, confirmationDetails.plan);
await launchEditor(planPath);
Comment on lines +122 to +125

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] openFullPlanInEditor creates a temp directory via fs.mkdtemp on every o press but never removes it — not after the editor exits, not on error, not on dialog dismissal. The established codebase pattern in text-buffer.ts (lines 2524, 2559, 2626) wraps identical mkdtemp usage in try/finally with fs.rmSync(tmpDir, { recursive: true, force: true }). — Failure scenario: each o press orphans a qwen-plan-XXXXXX/plan.md directory containing the full plan text (potentially sensitive conversation content) under os.tmpdir(). On shared systems other processes can discover and read these files; on tmpfs-backed /tmp they consume RAM until reboot.

Suggested change
const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-plan-'));
const planPath = path.join(dir, 'plan.md');
await fs.writeFile(planPath, confirmationDetails.plan);
await launchEditor(planPath);
const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-plan-'));
try {
const planPath = path.join(dir, 'plan.md');
await fs.writeFile(planPath, confirmationDetails.plan, { mode: 0o600 });
await launchEditor(planPath);
} finally {
await fs.rm(dir, { recursive: true, force: true }).catch(() => {});
}

— qwen3.7-max via Qwen Code /review

};
void openPlan().catch((err: unknown) => {
setPlanViewError(err instanceof Error ? err.message : String(err));
});
Comment on lines +127 to +129

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The .catch handler that sets planViewError when the editor fails to launch has no test coverage — no test uses launchEditorMock.mockRejectedValueOnce(...) to exercise the error-display path. — Concrete cost: a regression that silently swallows the error (e.g., someone "simplifies" the catch to a no-op) would ship undetected, leaving the user staring at a frozen dialog with no feedback on why the editor never opened.

Suggested change
void openPlan().catch((err: unknown) => {
setPlanViewError(err instanceof Error ? err.message : String(err));
});
void openPlan().catch((err: unknown) => {
setPlanViewError(err instanceof Error ? err.message : String(err));
});
// Add test: launchEditorMock.mockRejectedValueOnce(new Error('No editor found'))
// Assert: lastFrame() contains 'No editor found'

— qwen3.7-max via Qwen Code /review

};

useKeypress(
(key) => {
if (!isFocused) return;
if (key.name === 'escape' || (key.ctrl && key.name === 'c')) {
handleConfirm(ToolConfirmationOutcome.Cancel);
return;
}
if (
key.name === 'o' &&
!key.ctrl &&
!key.meta &&
confirmationDetails.type === 'plan'
) {
openFullPlanInEditor();
}
},
{ isActive: isFocused },
Expand Down Expand Up @@ -347,12 +382,15 @@ export const ToolConfirmationMessage: React.FC<
value: ToolConfirmationOutcome.Cancel,
});

const planHeight = compactMode
// Reserve one row for the "o open full plan" hint below the plan body.
const rawPlanHeight = compactMode
? Math.min(
availableBodyContentHeight() ?? COMPACT_BODY_MAX_LINES,
COMPACT_BODY_MAX_LINES,
)
: availableBodyContentHeight();
const planHeight =
rawPlanHeight === undefined ? undefined : Math.max(rawPlanHeight - 1, 1);
bodyContent = (
<Box flexDirection="column" paddingX={1} marginLeft={1}>
<MarkdownDisplay
Expand All @@ -368,6 +406,17 @@ export const ToolConfirmationMessage: React.FC<
// See #6867.
enforceHeightBudget
/>
{/* The plan can be height-truncated above, and the user is about to
approve it — always offer a way to read the WHOLE thing. See #7001. */}
{planViewError ? (
<Text color={theme.status.error} wrap="truncate">
{planViewError}
</Text>
) : (
<Text color={theme.text.secondary} wrap="truncate">
{t('o open full plan in editor')}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The i18n key 'o open full plan in editor' is not registered in any translation file under packages/cli/src/i18n/. The t() function falls back to the key string, so English works, but non-English locales will show untranslated English while all surrounding UI is translated. — Concrete cost: breaks the established convention where all other t() keys in this component (e.g., 'Apply this change?', 'Yes, allow once') are registered in all 9 locale files.

Suggested change
{t('o open full plan in editor')}
{t('o open full plan in editor')}

Register the key in en.js and other locale files under packages/cli/src/i18n/.

— qwen3.7-max via Qwen Code /review

</Text>
)}
</Box>
);
} else if (confirmationDetails.type === 'info') {
Expand Down
Loading