From 08573459450bca078f64746d594f59abd0c25c21 Mon Sep 17 00:00:00 2001 From: CHAEWAN KIM <66085474+amsminn@users.noreply.github.com> Date: Wed, 3 Dec 2025 15:16:19 +0900 Subject: [PATCH] refactor(editor): use const assertion for editor types with single source of truth (#8604) --- packages/a2a-server/src/agent/task.ts | 3 +- .../core/src/tools/modifiable-tool.test.ts | 37 ++++++----- packages/core/src/utils/editor.ts | 62 ++++++++++--------- 3 files changed, 58 insertions(+), 44 deletions(-) diff --git a/packages/a2a-server/src/agent/task.ts b/packages/a2a-server/src/agent/task.ts index 0789810551..b90be4e811 100644 --- a/packages/a2a-server/src/agent/task.ts +++ b/packages/a2a-server/src/agent/task.ts @@ -15,6 +15,7 @@ import { isNodeError, parseAndFormatApiError, safeLiteralReplace, + DEFAULT_GUI_EDITOR, type AnyDeclarativeTool, type ToolCall, type ToolConfirmationPayload, @@ -435,7 +436,7 @@ export class Task { outputUpdateHandler: this._schedulerOutputUpdate.bind(this), onAllToolCallsComplete: this._schedulerAllToolCallsComplete.bind(this), onToolCallsUpdate: this._schedulerToolCallsUpdate.bind(this), - getPreferredEditor: () => 'vscode', + getPreferredEditor: () => DEFAULT_GUI_EDITOR, config: this.config, }); return scheduler; diff --git a/packages/core/src/tools/modifiable-tool.test.ts b/packages/core/src/tools/modifiable-tool.test.ts index 3061cb0557..3cecccf554 100644 --- a/packages/core/src/tools/modifiable-tool.test.ts +++ b/packages/core/src/tools/modifiable-tool.test.ts @@ -13,7 +13,7 @@ import { modifyWithEditor, isModifiableDeclarativeTool, } from './modifiable-tool.js'; -import type { EditorType } from '../utils/editor.js'; +import { DEFAULT_GUI_EDITOR } from '../utils/editor.js'; import fs from 'node:fs'; import fsp from 'node:fs/promises'; import os from 'node:os'; @@ -24,9 +24,13 @@ import { debugLogger } from '../utils/debugLogger.js'; const mockOpenDiff = vi.hoisted(() => vi.fn()); const mockCreatePatch = vi.hoisted(() => vi.fn()); -vi.mock('../utils/editor.js', () => ({ - openDiff: mockOpenDiff, -})); +vi.mock('../utils/editor.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + openDiff: mockOpenDiff, + }; +}); vi.mock('diff', () => ({ createPatch: mockCreatePatch, @@ -103,7 +107,7 @@ describe('modifyWithEditor', () => { const result = await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ); @@ -169,9 +173,14 @@ describe('modifyWithEditor', () => { await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ); + + const [oldFilePath] = mockOpenDiff.mock.calls[0]; + const diffDir = path.dirname(oldFilePath); + // Temp directory should be cleaned up after modification + await expect(fsp.stat(diffDir)).rejects.toThrow(); }); }); @@ -184,7 +193,7 @@ describe('modifyWithEditor', () => { const result = await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ); @@ -212,7 +221,7 @@ describe('modifyWithEditor', () => { const result = await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ); @@ -241,7 +250,7 @@ describe('modifyWithEditor', () => { await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, { currentContent: overrideCurrent, @@ -268,7 +277,7 @@ describe('modifyWithEditor', () => { await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, { currentContent: null, @@ -298,7 +307,7 @@ describe('modifyWithEditor', () => { modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ), ).rejects.toThrow('Editor failed to open'); @@ -327,7 +336,7 @@ describe('modifyWithEditor', () => { await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ); @@ -353,7 +362,7 @@ describe('modifyWithEditor', () => { await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ); @@ -374,7 +383,7 @@ describe('modifyWithEditor', () => { await modifyWithEditor( mockParams, mockModifyContext, - 'vscode' as EditorType, + DEFAULT_GUI_EDITOR, abortSignal, ); diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index 0a7cfbd5df..a4ca04b292 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -8,16 +8,36 @@ import { execSync, spawn, spawnSync } from 'node:child_process'; import { debugLogger } from './debugLogger.js'; import { coreEvents, CoreEvent } from './events.js'; -export type EditorType = - | 'vscode' - | 'vscodium' - | 'windsurf' - | 'cursor' - | 'vim' - | 'neovim' - | 'zed' - | 'emacs' - | 'antigravity'; +const GUI_EDITORS = [ + 'vscode', + 'vscodium', + 'windsurf', + 'cursor', + 'zed', + 'antigravity', +] as const; +const TERMINAL_EDITORS = ['vim', 'neovim', 'emacs'] as const; +const EDITORS = [...GUI_EDITORS, ...TERMINAL_EDITORS] as const; + +const GUI_EDITORS_SET = new Set(GUI_EDITORS); +const TERMINAL_EDITORS_SET = new Set(TERMINAL_EDITORS); +const EDITORS_SET = new Set(EDITORS); + +export const DEFAULT_GUI_EDITOR: GuiEditorType = 'vscode'; + +export type GuiEditorType = (typeof GUI_EDITORS)[number]; +export type TerminalEditorType = (typeof TERMINAL_EDITORS)[number]; +export type EditorType = (typeof EDITORS)[number]; + +export function isGuiEditor(editor: EditorType): editor is GuiEditorType { + return GUI_EDITORS_SET.has(editor); +} + +export function isTerminalEditor( + editor: EditorType, +): editor is TerminalEditorType { + return TERMINAL_EDITORS_SET.has(editor); +} export const EDITOR_DISPLAY_NAMES: Record = { vscode: 'VS Code', @@ -36,17 +56,7 @@ export function getEditorDisplayName(editor: EditorType): string { } function isValidEditorType(editor: string): editor is EditorType { - return [ - 'vscode', - 'vscodium', - 'windsurf', - 'cursor', - 'vim', - 'neovim', - 'zed', - 'emacs', - 'antigravity', - ].includes(editor); + return EDITORS_SET.has(editor); } interface DiffCommand { @@ -94,11 +104,7 @@ export function checkHasEditorType(editor: EditorType): boolean { export function allowEditorTypeInSandbox(editor: EditorType): boolean { const notUsingSandbox = !process.env['SANDBOX']; - if ( - ['vscode', 'vscodium', 'windsurf', 'cursor', 'zed', 'antigravity'].includes( - editor, - ) - ) { + if (isGuiEditor(editor)) { return notUsingSandbox; } // For terminal-based editors like vim and emacs, allow in sandbox. @@ -197,9 +203,7 @@ export async function openDiff( return; } - const isTerminalEditor = ['vim', 'emacs', 'neovim'].includes(editor); - - if (isTerminalEditor) { + if (isTerminalEditor(editor)) { try { const result = spawnSync(diffCommand.command, diffCommand.args, { stdio: 'inherit',