feat(cli): respect /editor preference in Ctrl+X external editor

[dreamWB]

Summary

Closes #4165

Ctrl+X in the prompt input now opens the external editor configured via /editor (the general.preferredEditor setting) instead of always falling back to $VISUAL / $EDITOR / platform default.

Changes

• packages/core/src/utils/editor.ts — Added getExternalEditorCommand() returning { command, args, needsShell } for any EditorType. GUI editors (vscode, vscodium, windsurf, cursor, trae, zed) get --wait; terminal editors (vim, neovim, emacs) get raw [filePath]. Also exported isValidEditorType() type guard.
• packages/core/src/utils/editor.test.ts — Unit tests for getExternalEditorCommand.
• packages/cli/src/ui/hooks/usePreferredEditor.ts — New hook extracting /editor preference with isValidEditorType validation and sandbox availability check.
• packages/cli/src/ui/components/shared/text-buffer.ts — openInExternalEditor reads preferredEditor prop, resolves via getExternalEditorCommand(), falls back to $VISUAL/$EDITOR/platform default when unset or unavailable. Temp file uses mkdtempSync for isolation + buffer.txt with 0o600 permissions. Includes signal detection, 30-minute timeout, and conditional undo snapshot (only after successful edit with changed content).
• packages/cli/src/ui/components/shared/TextInput.tsx — Passes preferredEditor, stdin, and setRawMode to useTextBuffer for terminal editor support.
• packages/cli/src/ui/AppContainer.tsx / AgentComposer.tsx — Use usePreferredEditor() hook (no duplicated validation logic).

Evidence

Demo Case 1 — Default editor (no /editor configured)

Press Ctrl+X without configuring /editor. The default editor opens: vi on Unix/macOS, notepad on Windows (matching $VISUAL → $EDITOR → platform default fallback chain). Edit, save, and close — content appears in the prompt input.

case.1.mp4

Demo Case 2 — Preferred editor (VS Code via /editor)

Run /editor and select vscode. Press Ctrl+X — VS Code opens with --wait flag. Edit, save, and close the VS Code tab — content appears in the prompt input.

case.2.mp4

Linked Issues

Closes #4165

Test Plan

• Unit tests for getExternalEditorCommand
• 24 external editor tests in text-buffer-external-editor.test.ts
• Full test suite passes
• Manual test: default editor (vi) opens and returns content
• Manual test: VS Code opens with --wait and returns content

[github-actions]

📋 Review Summary

This PR implements editor preference respect for the Ctrl+X external editor feature, allowing users to configure their preferred editor via /editor command instead of relying solely on environment variables. The implementation is well-structured with good test coverage and proper fallback behavior. Overall, this is a solid enhancement that improves user experience while maintaining backward compatibility.

🔍 General Feedback

• Good separation of concerns: Core editor logic lives in packages/core/src/utils/editor.ts, while UI integration is in packages/cli/src/ui/components/
• Comprehensive test coverage: 14 new test cases covering isTerminalEditor and getExternalEditorCommand functions
• Proper fallback chain: Maintains the $VISUAL → $EDITOR → platform default fallback when no preferred editor is configured
• Type safety: Uses isValidEditorType() type guard consistently to avoid unsafe casts
• Cross-platform awareness: Handles Windows-specific cases (.cmd files, shell requirements)

🎯 Specific Feedback

🟡 High

• packages/cli/src/ui/components/agent-view/AgentComposer.tsx:27 - Import statement has incorrect formatting. The import line shows:

  } from '@qwen-code/qwen-code-core';
   isValidEditorType } from '@qwen-code/qwen-code-core';

  This appears to be a formatting issue that will cause a syntax error. Should be:

  isValidEditorType,
  } from '@qwen-code/qwen-code-core';

🟢 Medium

• packages/cli/src/ui/components/shared/text-buffer.ts:2232-2238 - The fallback logic for when preferredEditor is set but getExternalEditorCommand returns null could be improved. Currently it logs a warning but doesn't explain why the editor wasn't found (executable missing vs invalid type). Consider adding more specific error messaging or checking if the executable exists before attempting to use it.

• packages/core/src/utils/editor.ts:160 - The getExternalEditorCommand function returns null for invalid editor types, but this silent failure could be confusing. Consider whether callers should be prevented from passing invalid types at compile time, or if there should be better error handling when an editor type becomes unsupported.

• packages/cli/src/ui/components/shared/text-buffer.ts:2217 - The temp file path changed from a temp directory (qwen-edit-XXXXX/buffer.txt) to a flat file (qwen-edit-UUID.md). While simpler, using .md extension assumes markdown content. Consider whether this extension is appropriate for all use cases, or if no extension would be more flexible.

🔵 Low

• packages/core/src/utils/editor.ts:142 - The isTerminalEditor function uses an inline array with .includes(). For consistency with the existing codebase pattern (seen in isValidEditorType), consider using a similar structure or extracting the terminal editor list to a constant for easier maintenance.

• packages/core/src/utils/editor.ts:148-154 - The ExternalEditorCommand interface documentation could benefit from a brief example showing typical values for needsShell and when it's relevant.

• packages/cli/src/ui/AppContainer.tsx:719-722 - The prefEditorRaw validation pattern is duplicated in both AppContainer.tsx and AgentComposer.tsx. Consider extracting this into a utility function or custom hook to reduce duplication.

✅ Highlights

• Excellent test coverage: The 14 new tests thoroughly cover both terminal and GUI editor behaviors, including Windows-specific edge cases for .cmd executables
• Smart fallback design: The implementation gracefully degrades when preferred editor is unavailable, maintaining the existing environment variable chain
• Proper --wait flag handling: GUI editors correctly receive the --wait flag to block execution until the editor closes, which is critical for the feature to work correctly
• Type-safe integration: Both AppContainer.tsx and AgentComposer.tsx use isValidEditorType() guard before passing the editor value, preventing runtime type errors
• Clean temp file management: Using crypto.randomUUID() for unique temp filenames is cleaner than the previous directory-based approach

[wenshao]

[Suggestion] TextInput component does not propagate preferredEditor — Ctrl+X ignores /editor in secondary inputs

TextInput.tsx:79 creates its own useTextBuffer without calling usePreferredEditor(), yet handles OPEN_EXTERNAL_EDITOR (Ctrl+X) at line 155. This means Ctrl+X in 8+ consumers (AskUserQuestionDialog, PermissionsDialog, ManageModelsDialog, ApiKeyInput, SettingInputPrompt, etc.) silently ignores the user's /editor preference and falls back to $VISUAL/$EDITOR/default.

import { usePreferredEditor } from '../../hooks/usePreferredEditor.js';

const preferredEditor = usePreferredEditor();
const buffer = useTextBuffer({
  initialText: value || '',
  // ...existing props...
  preferredEditor,
});

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

[dreamWB]

[Suggestion] TextInput component does not propagate preferredEditor — Ctrl+X ignores /editor in secondary inputs

已修复。TextInput 组件现在调用 usePreferredEditor() 并将结果传递给 useTextBuffer，确保所有二级输入场景（对话框、设置页等）中的 Ctrl+X 都会尊重 /editor 偏好设置。同步更新了 TextInput.test.tsx 的 mock。

commit: a793e9f

[dreamWB]

Re: @wenshao's review comment about TextInput not propagating preferredEditor — already fixed in commit a793e9f7f. TextInput.tsx line 80 calls usePreferredEditor() and passes it to useTextBuffer at line 88. All TextInput consumers (dialogs, settings prompts, etc.) now respect the /editor preference via Ctrl+X.

[wenshao]

[Suggestion] Stale JSDoc on TextBuffer.openInExternalEditor

The JSDoc on the TextBuffer interface for openInExternalEditor describes two behaviors that this PR changed:

1. "snapshot the previous state once before launching the editor" — create_undo_snapshot was moved to after the editor exits (after readFileSync, before set_text).
2. "preferred terminal text editor ($VISUAL or $EDITOR, falling back to 'vi')" — the primary resolution path is now preferredEditor via /editor setting, with $VISUAL/$EDITOR only as fallback.

Recommend updating the JSDoc to match the new behavior.

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

[wenshao]

[Suggestion] useLaunchEditor has divergent editor resolution

useLaunchEditor.ts (consumed by CreationSummary, AgentEditStep, MemoryDialog) has its own editor resolution that diverges from openInExternalEditor on four axes: (1) no isValidEditorType validation (line 44 uses as EditorType | undefined), (2) no --wait for GUI editors, (3) no timeout on spawnSync, (4) no signal handling. Consider refactoring to use getExternalEditorCommand() or at minimum sharing the usePreferredEditor hook.

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

[dreamWB]

Re: review comment about TextInput not propagating preferredEditor —

This is already addressed in the current PR. TextInput.tsx imports usePreferredEditor at line 12 and passes it to useTextBuffer at line 88:

import { usePreferredEditor } from '../../hooks/usePreferredEditor.js';
// ...
const preferredEditor = usePreferredEditor();
const buffer = useTextBuffer({
  // ...existing props...
  preferredEditor,
});

So all consumers of TextInput (including AskUserQuestionDialog, PermissionsDialog, etc.) get the /editor preference automatically.

[dreamWB]

Re: [Suggestion] Stale JSDoc on TextBuffer.openInExternalEditor

Good catch — fixed in 930120e. Updated the JSDoc to reflect:

• Resolution order: /editor preference → $VISUAL → $EDITOR → vi
• Undo snapshot is now taken after the editor exits and only when content changed

Re: [Suggestion] useLaunchEditor has divergent editor resolution

useLaunchEditor and openInExternalEditor serve different purposes — useLaunchEditor opens a diff view (via openDiff / getDiffCommand), while openInExternalEditor opens a single file for editing (via getExternalEditorCommand). The four "divergences" are actually correct behavior for each use case:

1. as EditorType — useLaunchEditor gets its editor type from useSettings which already validates; adding isValidEditorType is redundant there
2. No --wait — openDiff handles --wait internally in getDiffCommand
3. No timeout — diff review has no natural timeout (user may spend significant time reviewing)
4. No signal handling — openDiff uses its own error handling via the Promise rejection path

That said, sharing usePreferredEditor hook in useLaunchEditor is a good idea to centralize the settings → validated EditorType logic. Worth a follow-up PR.

[dreamWB]

This finding flags filePath flowing into a shell command. The filePath here is a process-generated temp path (os.tmpdir() + "qwen-edit-" + UUID + ".txt"), not arbitrary user input.

At the spawnSync call site in text-buffer.ts, args are wrapped in double quotes when shell: true is needed for .cmd/.bat files on Windows. This handles spaces and common separators in the temp path. Full cmd.exe metacharacter escaping is not warranted — if an attacker controls TEMP/TMP env vars, they already have broader process control.

commit: 9ced043

[dreamWB]

Same analysis as above — this is the GUI editor branch returning [filePath, "--wait"]. Both values are program-controlled: filePath is a temp path generated by this process, and "--wait" is a hardcoded constant. Double-quoting at the shell execution boundary is sufficient for this threat model.

commit: 9ced043

[LaZzyMan]

Review

Wires the /editor preference into the Ctrl+X external-editor flow with a clean factoring (getExternalEditorCommand + usePreferredEditor), and meaningfully hardens tempfile handling (private 0700 directory, 0o600 file mode, 30-minute spawn timeout, signal detection, conditional undo snapshot). Test coverage is thorough across the controlled command/args paths.

Two concerns worth tightening before this lands. First, in SANDBOX mode, usePreferredEditor still returns a configured GUI editor (vscode, cursor, zed, etc.) without checking sandbox availability — Ctrl+X then tries to launch a GUI binary the sandbox can't run, causing a fail or hang. Second, on Windows, the $VISUAL/$EDITOR fallback branch promotes user-controlled values to shell: true when the extension matches .cmd|.bat, and then wraps the editor command in naive double quotes without escaping embedded " — exploitability requires a malicious env var in the user's own shell config (so very low real-world risk), but a one-line check that the env value contains no " before opting into shell mode would close it. A small description nit: the PR brief says "tempfile simplified to flat .md", but the code uses mkdtempSync + buffer.txt — the actual implementation is the better design, just update the description.

Verdict

COMMENT — implementation is correct on the controlled plumbing and the security-critical paths; the sandbox gap and the Windows env-var fallback are worth tightening before this lands.

[dreamWB]

@LaZzyMan Thanks for the thorough review! All three points have been addressed in 0adb50d:

1. Sandbox gap — usePreferredEditor now calls allowEditorTypeInSandbox() and returns undefined for GUI editors (vscode, cursor, zed, etc.) when SANDBOX env is set. The Ctrl+X path then falls through to the env/default editor instead of attempting a GUI launch.

2. Windows env-var safety — The $VISUAL/$EDITOR fallback now rejects commands containing " or | before opting into shell: true, closing the cmd.exe metacharacter injection vector.

3. PR description — Already updated in the previous push to accurately state mkdtempSync + buffer.txt (not flat .md).

[wenshao]

Also: the finally block cleanup (line ~2349) uses separate unlinkSync + rmdirSync. When a terminal editor (vim/neovim) is killed by the 30-min timeout or signal, swap files (.buffer.txt.swp) are left behind and rmdirSync silently fails on the non-empty directory, leaking the temp dir. Consider replacing both cleanup steps with fs.rmSync(tmpDir, { recursive: true, force: true }) — consistent with the pattern used elsewhere in the codebase (gemini-converter.ts, claude-converter.ts). — qwen-latest-series-invite-beta-v34 via Qwen Code /review

[dreamWB]

Good catch — fixed in 1e2ae54. Replaced all three unlinkSync + rmdirSync call sites (two early-return unsafe-char paths + the main finally block) with a single fs.rmSync(tmpDir, { recursive: true, force: true }), consistent with the pattern in gemini-converter.ts / claude-converter.ts.

Also added a comment in the finally block noting the Windows residual: EPERM/EBUSY from locked swap files may cause a partial delete — the outer catch keeps it non-fatal.

Tests updated accordingly: removed dead unlinkSync mock, added cleanup-failure-doesn't-propagate test, strengthened unsafe-char assertions to verify full { recursive: true, force: true } args.

[wenshao]

[Suggestion] debugLogger.warn used for normal operation — should be debugLogger.info

The "launching external editor" message describes a normal, expected code path, not a degraded or anomalous condition. The convention in this file reserves warn for fallback/degraded paths:

• debugLogger.info — normal success (line 104: Segmenter init)
• debugLogger.warn — degraded/fallback (line 106: Segmenter failure, line 270: word boundary fallback, line 2269: preferred editor not found)

Using warn here creates noise in debug logs and may trigger false alerting in log-monitoring setups — every successful Ctrl+X press emits a [WARN] line.

Suggested change

	debugLogger.warn(
	debugLogger.info(
	`[useTextBuffer] launching external editor (cmd=${editorCmd}, shell=${useShell}, source=${editorSource}, file=${filePath})`,
	);

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

[wenshao]

[Suggestion] No test proves opts.editor overrides preferredEditor

The priority chain opts.editor > preferredEditor > env/default (implemented at text-buffer.ts:2238) is the core design contract of this method, but no test exercises the case where both opts.editor and preferredEditor are present. If someone accidentally swaps the if/else order, no test would catch it.

Consider adding a test like:

it('opts.editor takes priority over preferredEditor', async () => {
  mockGetExternalEditorCommand.mockReturnValue({
    command: 'code', args: [expectedTmpFile, '--wait'], needsShell: false,
  });
  const { result } = renderHook(() =>
    useTextBuffer({
      initialText: 'hello', viewport, isValidPath: () => false,
      preferredEditor: 'vscode',
    }),
  );
  await act(async () => {
    await result.current.openInExternalEditor({ editor: 'nano' });
  });
  expect(mockGetExternalEditorCommand).not.toHaveBeenCalled();
  expect(mockSpawnSync).toHaveBeenCalledWith('nano', ...);
});

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

[wenshao]

[Suggestion] Pipe | not independently tested in unsafe-char parametrized tests

The regex /["|%!]/ matches 4 characters, but the it.each covers only 3: ", %, !. The " test string (evil"|cmd.cmd) contains |, but it's masked by " — if the regex regressed to /["%!]/, that test would still pass on ".

Add a dedicated entry for | to both it.each arrays (opts.editor and env-var):

{ char: '|', editor: 'pipe|cmd.cmd' },

Pipe is the highest-risk character in the set (command chaining in cmd.exe), so independent coverage matters.

— qwen-latest-series-invite-beta-v34 via Qwen Code /review