diff --git a/integration-tests/terminal-capture/scenarios/skill-review-dialog.ts b/integration-tests/terminal-capture/scenarios/skill-review-dialog.ts new file mode 100644 index 0000000000..d3ba66b0b4 --- /dev/null +++ b/integration-tests/terminal-capture/scenarios/skill-review-dialog.ts @@ -0,0 +1,69 @@ +import type { ScenarioConfig } from '../scenario-runner.js'; + +const base = { + terminal: { + cols: 112, + rows: 40, + theme: 'github-dark', + title: 'qwen-code skill review', + cwd: '../../..', + }, + gif: false, +} satisfies Pick; + +const harness = [ + 'npx', + 'tsx', + 'integration-tests/terminal-capture/skill-review-harness/text-capture.tsx', +]; + +export default [ + { + ...base, + name: 'skill-review-before-global-qwen', + spawn: [...harness, 'before'], + flow: [ + { + sleep: 7000, + capture: 'before-global-qwen.png', + captureFull: 'before-global-qwen-full.png', + }, + ], + }, + { + ...base, + name: 'skill-review-after-preview', + spawn: [...harness, 'after-preview'], + flow: [ + { + sleep: 7000, + capture: 'after-preview.png', + captureFull: 'after-preview-full.png', + }, + ], + }, + { + ...base, + name: 'skill-review-after-second', + spawn: [...harness, 'after-second'], + flow: [ + { + sleep: 7000, + capture: 'after-second.png', + captureFull: 'after-second-full.png', + }, + ], + }, + { + ...base, + name: 'skill-review-after-turn-off', + spawn: [...harness, 'after-turn-off'], + flow: [ + { + sleep: 7000, + capture: 'after-turn-off.png', + captureFull: 'after-turn-off-full.png', + }, + ], + }, +] satisfies ScenarioConfig[]; diff --git a/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx b/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx new file mode 100644 index 0000000000..bb4aa5a63c --- /dev/null +++ b/integration-tests/terminal-capture/skill-review-harness/text-capture.tsx @@ -0,0 +1,407 @@ +#!/usr/bin/env npx tsx +/** + * Browser-free capture of the real SkillReviewDialog render (before/after), + * using ink-testing-library so it works without Playwright/Chromium. Prints the + * literal rendered frames — the actual TUI output the component produces. + * + * Runs from SOURCE — no build needed. CLI source imports + * `@qwen-code/qwen-code-core`, which normally resolves through the package's + * built `dist` (absent on a fresh clone, and stale whenever core src moves + * ahead of the last build). To avoid both, this registers an ESM loader hook + * (same idea as scripts/dev.js) that redirects that specifier to + * `packages/core/index.ts`, then imports the core-dependent modules + * DYNAMICALLY so they resolve through the hook. Type-only imports below are + * erased at runtime and never trigger core resolution. + */ +import React from 'react'; +import { render } from 'ink-testing-library'; +import { Text } from 'ink'; +import { EventEmitter } from 'node:events'; +import { register } from 'node:module'; +import { promises as fs } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import type { Config } from '@qwen-code/qwen-code-core'; +import type { LoadedSettings } from '../../../packages/cli/src/config/settings.js'; +import type { PendingSkillView } from '../../../packages/cli/src/ui/contexts/UIStateContext.js'; + +const ALPHA = `--- +name: run-e2e-headless +description: Run the Qwen CLI headlessly against a mock model and inspect API traffic. +--- + +# Run E2E headless + +1. Build the bundle: npm run build && npm run bundle. +2. Start the fake OpenAI server on a free port. +3. Point OPENAI_BASE_URL at it and run node dist/cli.js -p "" --yolo. +4. Assert on the captured request/response JSON. +`; + +const BETA = `--- +name: vitest-mock-hoisting +description: Hoist vi.mock factories in CLI tests so mocks apply at load time. +--- + +# Vitest mock hoisting + +- Use vi.hoisted() for values referenced inside a vi.mock() factory. +- The factory runs before the test body, so plain const refs are undefined. +`; + +const delay = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +/** + * Poll the current frame until it contains `needle`, so captures are taken only + * once the (async) preview has actually rendered — a fixed delay races the + * fs.readFile and can capture a stale "Loading preview…" frame. + */ +async function waitForFrame( + getFrame: () => string | undefined, + needle: string, + timeoutMs = 5000, +) { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + if ((getFrame() ?? '').includes(needle)) return; + await delay(50); + } + throw new Error( + `Timed out (${timeoutMs}ms) waiting for frame to contain ${JSON.stringify(needle)}`, + ); +} + +function banner(title: string) { + const bar = '═'.repeat(74); + console.log(`\n${bar}\n ${title}\n${bar}`); +} + +const noop = () => {}; + +class CaptureStdout extends EventEmitter { + columns = 100; + rows = 30; + private last?: string; + + write = (frame: string | Buffer) => { + this.last = String(frame); + }; + + lastFrame = () => this.last; +} + +class CaptureStdin extends EventEmitter { + isTTY = true; + private data: string | Buffer | null = null; + + write = (data: string | Buffer) => { + this.data = data; + this.emit('readable'); + this.emit('data', data); + }; + + setEncoding() {} + setRawMode() {} + resume() {} + pause() {} + ref() {} + unref() {} + + read = () => { + const data = this.data; + this.data = null; + return data; + }; +} + +async function findGlobalInteractiveChunk(chunksDir: string) { + for (const entry of await fs.readdir(chunksDir)) { + if (!entry.endsWith('.js')) continue; + const filePath = path.join(chunksDir, entry); + const content = await fs.readFile(filePath, 'utf-8'); + if ( + content.includes('var SkillReviewDialog') && + content.includes('Esc to decide later') && + content.includes('startInteractiveUI') + ) { + return { filePath, content }; + } + } + throw new Error(`Could not find bundled SkillReviewDialog in ${chunksDir}`); +} + +async function renderGlobalBefore(skills: PendingSkillView[]) { + const npmRoot = execFileSync('npm', ['root', '-g'], { + encoding: 'utf-8', + }).trim(); + const packageRoot = + process.env['QWEN_GLOBAL_PACKAGE_ROOT'] ?? + path.join(npmRoot, '@qwen-code', 'qwen-code'); + const packageJson = JSON.parse( + await fs.readFile(path.join(packageRoot, 'package.json'), 'utf-8'), + ) as { version?: string }; + const chunksDir = path.join(packageRoot, 'chunks'); + const { content } = await findGlobalInteractiveChunk(chunksDir); + + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-before-bundle-')); + try { + for (const entry of await fs.readdir(chunksDir)) { + await fs.symlink(path.join(chunksDir, entry), path.join(tmp, entry)); + } + + const exportNeedle = 'export {\n startInteractiveUI\n};'; + const patched = content.replace( + exportNeedle, + [ + 'export {', + ' startInteractiveUI,', + ' SkillReviewDialog,', + ' KeypressProvider,', + ' render_default,', + ' require_jsx_runtime', + '};', + ].join('\n'), + ); + if (patched === content) { + throw new Error('Could not patch global qwen bundle exports'); + } + + const patchedPath = path.join(tmp, 'startInteractiveUI-before-export.js'); + await fs.writeFile(patchedPath, patched); + const mod = (await import(pathToFileURL(patchedPath).href)) as { + SkillReviewDialog: unknown; + KeypressProvider: unknown; + render_default: ( + tree: unknown, + options: Record, + ) => { unmount: () => void; cleanup?: () => void }; + require_jsx_runtime: () => { + jsx: (type: unknown, props: Record) => unknown; + }; + }; + + const jsx = mod.require_jsx_runtime(); + const stdout = new CaptureStdout(); + const stderr = new CaptureStdout(); + const stdin = new CaptureStdin(); + const element = jsx.jsx(mod.KeypressProvider, { + kittyProtocolEnabled: false, + children: jsx.jsx(mod.SkillReviewDialog, { + skills, + onAccept: noop, + onReject: noop, + onClose: noop, + onDismiss: noop, + }), + }); + + const instance = mod.render_default(element, { + stdout, + stderr, + stdin, + debug: true, + patchConsole: false, + exitOnCtrlC: false, + }); + await waitForFrame(() => stdout.lastFrame(), 'run-e2e-headless'); + const frame = stdout.lastFrame(); + instance.unmount(); + instance.cleanup?.(); + if (!frame) throw new Error('Global qwen before render produced no frame'); + return { frame, version: packageJson.version ?? 'unknown' }; + } finally { + await fs.rm(tmp, { recursive: true, force: true }); + } +} + +async function main() { + const mode = process.argv[2] ?? 'all'; + const shouldPrint = (name: string) => mode === 'all' || mode === name; + + // Redirect @qwen-code/qwen-code-core to its TypeScript source so the harness + // runs without a build and can never pick up a stale dist. Registered before + // the dynamic imports below, which is what routes them through the hook. + const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '../../..', + ); + const coreSrcUrl = pathToFileURL( + path.join(repoRoot, 'packages', 'core', 'index.ts'), + ).href; + const loader = ` + export function resolve(specifier, context, nextResolve) { + if (specifier === '@qwen-code/qwen-code-core') { + return { shortCircuit: true, url: '${coreSrcUrl}', format: 'module' }; + } + return nextResolve(specifier, context); + } + `; + register(`data:text/javascript,${encodeURIComponent(loader)}`); + + // Import core-dependent modules ONLY after the loader is registered. The + // dialog under review is deliberately NOT imported here — `before` mode must + // not depend on (or execute) the implementation being reviewed, so a + // regression in the new dialog can never break the baseline capture. + const [{ KeypressProvider }, { ConfigContext }, { SettingsContext }] = + await Promise.all([ + import('../../../packages/cli/src/ui/contexts/KeypressContext.js'), + import('../../../packages/cli/src/ui/contexts/ConfigContext.js'), + import('../../../packages/cli/src/ui/contexts/SettingsContext.js'), + ]); + + const fakeConfig = { + setAutoSkillEnabled: () => {}, + getBareMode: () => false, + isSafeMode: () => false, + } as unknown as Config; + + const fakeSettings = { + setValue: () => {}, + merged: { general: {}, memory: { enableAutoSkill: true } }, + } as unknown as LoadedSettings; + + const wrap = (node: React.ReactNode) => ( + + + + {node} + + + + ); + + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'skill-textcap-')); + // try/finally so a failed render or frame-wait doesn't leak the temp dir. + try { + const alphaPath = path.join(dir, 'run-e2e-headless', 'SKILL.md'); + const betaPath = path.join(dir, 'vitest-mock-hoisting', 'SKILL.md'); + await fs.mkdir(path.dirname(alphaPath), { recursive: true }); + await fs.mkdir(path.dirname(betaPath), { recursive: true }); + await fs.writeFile(alphaPath, ALPHA); + await fs.writeFile(betaPath, BETA); + + const skills: PendingSkillView[] = [ + { + name: 'run-e2e-headless', + description: + 'Run the Qwen CLI headlessly against a mock model and inspect API traffic.', + stagedManifestPath: alphaPath, + }, + { + name: 'vitest-mock-hoisting', + description: + 'Hoist vi.mock factories in CLI tests so mocks apply at load time.', + stagedManifestPath: betaPath, + }, + ]; + + // ── BEFORE ──────────────────────────────────────────────────────────────── + if (shouldPrint('before')) { + try { + const before = await renderGlobalBefore(skills); + banner( + `BEFORE — global qwen ${before.version} dialog (name + description only)`, + ); + console.log(before.frame); + } catch (err) { + // The baseline must come from the globally installed qwen or not at + // all — a hand-maintained pre-change fixture can silently drift from + // what actually shipped, so there is deliberately no local fallback. + const reason = err instanceof Error ? err.message : String(err); + banner('BEFORE — unavailable: could not render the global qwen dialog'); + console.log( + `${reason}\nInstall it first: npm install -g @qwen-code/qwen-code`, + ); + if (mode === 'before') throw err; + } + } + + // ── AFTER: skipped entirely in `before` mode so the baseline capture never + // executes the implementation under review. + if (mode === 'all' || mode.startsWith('after-')) { + const { SkillReviewDialog } = await import( + '../../../packages/cli/src/ui/components/SkillReviewDialog.js' + ); + + function AfterHarness({ + dialogSkills, + }: { + dialogSkills: PendingSkillView[]; + }) { + const [open, setOpen] = React.useState(true); + if (!open) { + return ( + + Auto-skill turned off. Re-enable it any time from /memory. + + ); + } + return ( + setOpen(false)} + onDismiss={() => setOpen(false)} + /> + ); + } + + // The preview and turn-off frames showcase the COMMON single-skill case + // (1/1, no bulk options); the advance frame needs a two-skill batch. They + // use separate dialog instances so `all` can show both — a single-skill + // dialog closes on its first decision and could never reach a second + // skill (this exact mismatch once made default-mode runs time out). + if (mode !== 'after-second') { + const single = render( + wrap(), + ); + // Wait for body-only text from the skill's preview (not its name or + // description, which show before the async read resolves). + await waitForFrame(() => single.lastFrame(), 'OPENAI_BASE_URL'); + if (shouldPrint('after-preview')) { + banner( + 'AFTER — common 1/1 review with inline preview and visible turn-off option', + ); + console.log(single.lastFrame()); + } + if (shouldPrint('after-turn-off')) { + // Select "Turn off auto-generated skills" — in the single-skill case + // the options are keep / discard / turn-off, so numeric quick-select + // "3" picks it. + single.stdin.write('3'); + await waitForFrame(() => single.lastFrame(), 'turned off'); + banner( + 'AFTER — after selecting "Turn off auto-generated skills": batch closed', + ); + console.log(single.lastFrame()); + } + single.unmount(); + } + + if (shouldPrint('after-second')) { + const batch = render(wrap()); + await waitForFrame(() => batch.lastFrame(), 'OPENAI_BASE_URL'); + // Drive Enter (keep skill 1 → advance to skill 2), then wait for body-only + // text from the SECOND skill's preview so we never capture "Loading preview…". + batch.stdin.write('\r'); + await waitForFrame(() => batch.lastFrame(), 'vi.hoisted'); + banner('AFTER — 2/2 final batch item hides bulk options'); + console.log(batch.lastFrame()); + batch.unmount(); + } + } + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } +} + +void main().then( + () => process.exit(0), + (e) => { + console.error(e); + process.exit(1); + }, +); diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 781da9ec4b..f9303ebc6d 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -40,6 +40,7 @@ import { isInputActiveForState, isRenderModeToggleKey, mergeStartupWarnings, + shouldAutoOpenSkillReview, shouldDrainMessageQueue, } from './AppContainer.js'; import { @@ -4113,6 +4114,83 @@ describe('AppContainer State Management', () => { expect(mockAddItemDisabled).not.toHaveBeenCalled(); }); }); + + describe('Skill review auto-open gating (shouldAutoOpenSkillReview)', () => { + const pending = { + taskId: 'skill-task-1', + skills: [ + { + name: 'auto-skill-alpha', + description: 'does alpha', + stagedManifestPath: '/tmp/staged/auto-skill-alpha/SKILL.md', + }, + ], + }; + + /** The baseline where every gate is satisfied and the dialog opens. */ + const openable = { + pending, + streamingState: StreamingState.Idle, + isMemoryDialogOpen: false, + autoSkillEnabled: true, + dismissedTaskIds: new Set(), + }; + + it('opens when idle with an undismissed pending batch and auto-skill on', () => { + expect(shouldAutoOpenSkillReview(openable)).toBe(true); + }); + + it('does NOT open while auto-skill is disabled (the turn-off flow)', () => { + // The state right after "Turn off auto-generated skills": the batch + // stays pending (turn-off closes without dismissing), so only the live + // flag keeps it from re-popping. + expect( + shouldAutoOpenSkillReview({ ...openable, autoSkillEnabled: false }), + ).toBe(false); + }); + + it('does NOT open over an open /memory dialog', () => { + expect( + shouldAutoOpenSkillReview({ ...openable, isMemoryDialogOpen: true }), + ).toBe(false); + }); + + it('opens again once /memory closes with auto-skill re-enabled', () => { + // The re-enable flow: same inputs as the case above except /memory has + // been closed (the effect re-runs on isMemoryDialogOpen for exactly + // this transition), so the pending batch resurfaces. + expect( + shouldAutoOpenSkillReview({ ...openable, isMemoryDialogOpen: false }), + ).toBe(true); + }); + + it('does NOT reopen a batch the user dismissed with Esc', () => { + expect( + shouldAutoOpenSkillReview({ + ...openable, + dismissedTaskIds: new Set([pending.taskId]), + }), + ).toBe(false); + }); + + it('does NOT open while streaming or with no pending skills', () => { + expect( + shouldAutoOpenSkillReview({ + ...openable, + streamingState: StreamingState.Responding, + }), + ).toBe(false); + expect(shouldAutoOpenSkillReview({ ...openable, pending: null })).toBe( + false, + ); + expect( + shouldAutoOpenSkillReview({ + ...openable, + pending: { taskId: 'skill-task-1', skills: [] }, + }), + ).toBe(false); + }); + }); }); describe('dedupeNewestFirst', () => { diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 0ca1907f1b..62168afe49 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -392,6 +392,32 @@ export function mergeStartupWarnings( return [...new Set([...currentWarnings, ...nextWarnings])]; } +/** + * Whether the skill-review dialog should auto-open. Exported for tests. + * + * Auto-open requires an undismissed pending batch while the app is idle, the + * auto-skill feature enabled (live flag — the dialog's turn-off must keep the + * batch from re-popping, while re-enabling from /memory lets it resurface), + * and /memory itself closed (the review dialog must not pop over the dialog + * where the flag is being toggled). + */ +export function shouldAutoOpenSkillReview(args: { + pending: UIState['skillReviewPending']; + streamingState: StreamingState; + isMemoryDialogOpen: boolean; + autoSkillEnabled: boolean; + dismissedTaskIds: ReadonlySet; +}): boolean { + return ( + args.pending !== null && + args.pending.skills.length > 0 && + args.streamingState === StreamingState.Idle && + !args.isMemoryDialogOpen && + args.autoSkillEnabled && + !args.dismissedTaskIds.has(args.pending.taskId) + ); +} + interface AppContainerProps { config: Config; settings: LoadedSettings; @@ -1351,6 +1377,7 @@ export const AppContainer = (props: AppContainerProps) => { const pendingSkills = withPending.metadata!['pendingSkills'] as Array<{ name: string; description: string; + stagedManifestPath: string; }>; const sig = `${withPending.id}|${pendingSkills .map((p) => p.name) @@ -1362,6 +1389,7 @@ export const AppContainer = (props: AppContainerProps) => { skills: pendingSkills.map((p) => ({ name: p.name, description: p.description, + stagedManifestPath: p.stagedManifestPath, })), }); }; @@ -1741,16 +1769,28 @@ export const AppContainer = (props: AppContainerProps) => { }, [streamingState]); // Auto-open the skill-review dialog when idle and there are pending skills. + // Gated on the live auto-skill flag: after the dialog's turn-off option + // (which disables the feature and closes WITHOUT dismissing), the batch must + // not re-pop — but re-enabling auto-skill from /memory flips the flag back, + // and the batch can then reopen. The flag lives on the stable `config` + // object (mutated imperatively), so no dependency changes when it flips; + // `isMemoryDialogOpen` is a dependency precisely so that closing /memory — + // the only in-session place the flag can be re-enabled — re-runs this check + // even when the app is already idle. It doubles as a gate so the review + // dialog never pops over the open /memory dialog. useEffect(() => { if ( - skillReviewPending && - skillReviewPending.skills.length > 0 && - streamingState === StreamingState.Idle && - !skillReviewDismissedTaskIdsRef.current.has(skillReviewPending.taskId) + shouldAutoOpenSkillReview({ + pending: skillReviewPending, + streamingState, + isMemoryDialogOpen, + autoSkillEnabled: config.getAutoSkillEnabled(), + dismissedTaskIds: skillReviewDismissedTaskIdsRef.current, + }) ) { setIsSkillReviewDialogOpen(true); } - }, [skillReviewPending, streamingState]); + }, [skillReviewPending, streamingState, isMemoryDialogOpen, config]); // Contextual tips — show tips based on context usage after model responses // Defer TipHistory loading when tips are disabled to avoid side effects diff --git a/packages/cli/src/ui/components/MemoryDialog.test.tsx b/packages/cli/src/ui/components/MemoryDialog.test.tsx index 07386482b4..8c6d93f84a 100644 --- a/packages/cli/src/ui/components/MemoryDialog.test.tsx +++ b/packages/cli/src/ui/components/MemoryDialog.test.tsx @@ -35,9 +35,12 @@ const mockedUseLaunchEditor = vi.mocked(useLaunchEditor); const mockedUseKeypress = vi.mocked(useKeypress); describe('MemoryDialog', () => { + let setAutoSkillEnabled: ReturnType; + beforeEach(() => { vi.clearAllMocks(); + setAutoSkillEnabled = vi.fn(); mockedUseConfig.mockReturnValue({ getWorkingDir: vi.fn(() => '/tmp/project'), getProjectRoot: vi.fn(() => '/tmp/project'), @@ -48,6 +51,7 @@ describe('MemoryDialog', () => { getManagedAutoMemoryEnabled: vi.fn(() => false), getManagedAutoDreamEnabled: vi.fn(() => false), getAutoSkillEnabled: vi.fn(() => false), + setAutoSkillEnabled, } as never); mockedUseSettings.mockReturnValue({ @@ -187,9 +191,51 @@ describe('MemoryDialog', () => { 'memory.enableAutoSkill', true, ); + // Also drives the live Config flag so it takes effect this session (and can + // re-enable auto-skill after the review dialog's `t` disabled it). + expect(setAutoSkillEnabled).toHaveBeenCalledWith(true); expect(lastFrame()).toContain('› Auto-skill: on'); }); + it('toggles Auto-skill back OFF on Enter (re-disable path)', () => { + const setValue = vi.fn(); + mockedUseSettings.mockReturnValue({ + setValue, + merged: { memory: { enableAutoSkill: true, autoSkillConfirm: true } }, + } as never); + + const { lastFrame } = render(); + + const pressKey = (key: { name: string }) => { + const keypressHandler = + mockedUseKeypress.mock.calls[ + mockedUseKeypress.mock.calls.length - 1 + ]![0]; + act(() => { + keypressHandler(key as never); + }); + }; + + expect(lastFrame()).toContain('Auto-skill: on'); + + // navigate to the autoSkillConfirm row first, then up to autoSkill + pressKey({ name: 'up' }); + pressKey({ name: 'up' }); + expect(lastFrame()).toContain('› Auto-skill: on'); + + pressKey({ name: 'return' }); + + expect(setValue).toHaveBeenCalledWith( + expect.anything(), + 'memory.enableAutoSkill', + false, + ); + // The live flag must go OFF too — this is the session-level disable, the + // same path the review dialog's turn-off option relies on. + expect(setAutoSkillEnabled).toHaveBeenCalledWith(false); + expect(lastFrame()).toContain('› Auto-skill: off'); + }); + it('toggles autoSkillConfirm on Enter and persists to workspace settings', () => { const setValue = vi.fn(); mockedUseSettings.mockReturnValue({ diff --git a/packages/cli/src/ui/components/MemoryDialog.tsx b/packages/cli/src/ui/components/MemoryDialog.tsx index a6eed85152..c28bfb0068 100644 --- a/packages/cli/src/ui/components/MemoryDialog.tsx +++ b/packages/cli/src/ui/components/MemoryDialog.tsx @@ -291,8 +291,14 @@ export function MemoryDialog({ onClose }: MemoryDialogProps) { 'memory.enableAutoSkill', newValue, ); + // Also drive the live Config flag: it is copied from settings at startup and + // read live by the skill-review scheduler. Without this, toggling here would + // not take effect until restart — and in particular could not re-enable + // auto-skill after the review dialog's turn-off option disabled it this + // session. + config.setAutoSkillEnabled(newValue); setAutoSkillOn(newValue); - }, [autoSkillOn, loadedSettings]); + }, [autoSkillOn, loadedSettings, config]); const handleToggleAutoSkillConfirm = useCallback(() => { const newValue = !autoSkillConfirmOn; diff --git a/packages/cli/src/ui/components/SkillReviewDialog.test.tsx b/packages/cli/src/ui/components/SkillReviewDialog.test.tsx index fc4b25ca8f..194d1df523 100644 --- a/packages/cli/src/ui/components/SkillReviewDialog.test.tsx +++ b/packages/cli/src/ui/components/SkillReviewDialog.test.tsx @@ -4,8 +4,15 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { render } from 'ink-testing-library'; -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render as inkRender } from 'ink-testing-library'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { useSettings } from '../contexts/SettingsContext.js'; +import { useConfig } from '../contexts/ConfigContext.js'; +import { useLaunchEditor } from '../hooks/useLaunchEditor.js'; +import type { Key } from '../hooks/useKeypress.js'; interface CapturedRadio { items?: Array<{ value: string; label: string }>; @@ -26,65 +33,184 @@ vi.mock('./shared/RadioButtonSelect.js', () => ({ }, })); -// Keep keypress handling inert for these tests. +// Capture the keypress handler so tests can drive `o` / `t` / Esc directly. +let keyHandler: ((key: Key) => void) | undefined; vi.mock('../hooks/useKeypress.js', () => ({ - useKeypress: vi.fn(), + useKeypress: (handler: (key: Key) => void) => { + keyHandler = handler; + }, })); +vi.mock('../contexts/SettingsContext.js', () => ({ + useSettings: vi.fn(), +})); + +vi.mock('../contexts/ConfigContext.js', () => ({ + useConfig: vi.fn(), +})); + +vi.mock('../hooks/useLaunchEditor.js', () => ({ + useLaunchEditor: vi.fn(), +})); + +const mockedUseSettings = vi.mocked(useSettings); +const mockedUseConfig = vi.mocked(useConfig); +const mockedUseLaunchEditor = vi.mocked(useLaunchEditor); + import { SkillReviewDialog } from './SkillReviewDialog.js'; +import type { SkillReviewDialogProps } from './SkillReviewDialog.js'; + +// Track every render so afterEach can unmount it. The dialog installs a +// process.stdout resize listener via useTerminalSize on mount; without +// unmounting, repeated renders leak listeners (MaxListenersExceededWarning). +const renderInstances: Array> = []; +function render(tree: Parameters[0]) { + const instance = inkRender(tree); + renderInstances.push(instance); + return instance; +} + +function pressKey(name: string, extra: Partial = {}) { + keyHandler?.({ + name, + ctrl: false, + meta: false, + shift: false, + paste: false, + sequence: name, + ...extra, + } as Key); +} describe('SkillReviewDialog', () => { - const skills = [ - { name: 'auto-skill-alpha', description: 'does alpha' }, - { name: 'auto-skill-beta', description: 'does beta' }, - ]; + let tempDir: string; + let setValue: ReturnType; + let setAutoSkillEnabled: ReturnType; + let launchEditor: ReturnType; + let skills: Array<{ + name: string; + description: string; + stagedManifestPath: string; + }>; - beforeEach(() => { + beforeEach(async () => { captured.items = undefined; captured.onSelect = undefined; + keyHandler = undefined; + + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'skill-review-')); + const alphaPath = await writeSkill( + 'auto-skill-alpha', + '---\nname: auto-skill-alpha\ndescription: does alpha\n---\nALPHA_BODY_MARKER steps here.\n', + ); + const betaPath = await writeSkill( + 'auto-skill-beta', + '---\nname: auto-skill-beta\ndescription: does beta\n---\nBETA_BODY_MARKER steps here.\n', + ); + skills = [ + { + name: 'auto-skill-alpha', + description: 'does alpha', + stagedManifestPath: alphaPath, + }, + { + name: 'auto-skill-beta', + description: 'does beta', + stagedManifestPath: betaPath, + }, + ]; + + setValue = vi.fn(); + mockedUseSettings.mockReturnValue({ + setValue, + merged: { memory: { enableAutoSkill: true } }, + } as never); + setAutoSkillEnabled = vi.fn(); + mockedUseConfig.mockReturnValue({ setAutoSkillEnabled } as never); + launchEditor = vi.fn().mockResolvedValue(undefined); + mockedUseLaunchEditor.mockReturnValue(launchEditor); }); - it('renders the first pending skill name and description with a counter', () => { - const { lastFrame } = render( + afterEach(async () => { + for (const instance of renderInstances) instance.unmount(); + renderInstances.length = 0; + vi.clearAllMocks(); + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + /** Write a staged SKILL.md under tempDir and return its absolute path. */ + async function writeSkill(dirName: string, content: string): Promise { + const skillPath = path.join(tempDir, dirName, 'SKILL.md'); + await fs.mkdir(path.dirname(skillPath), { recursive: true }); + await fs.writeFile(skillPath, content); + return skillPath; + } + + /** Render the dialog with every callback defaulted to a fresh spy. */ + function renderDialog( + skillsArg: SkillReviewDialogProps['skills'], + overrides: Partial = {}, + ) { + return render( , ); + } + + /** Write a single staged skill and render a one-skill dialog over it. */ + async function renderPreviewSkill(dirName: string, content: string) { + const stagedManifestPath = await writeSkill(dirName, content); + return renderDialog([ + { name: dirName, description: '', stagedManifestPath }, + ]); + } + + it('renders the first pending skill name and description with a counter', () => { + const { lastFrame } = renderDialog(skills); expect(lastFrame()).toContain('auto-skill-alpha'); expect(lastFrame()).toContain('does alpha'); expect(lastFrame()).toContain('1/2'); }); - it('offers keep / discard / keep-all / discard-all options', () => { - render( - , - ); + it('offers keep / discard / bulk / turn-off options while several remain', () => { + renderDialog(skills); const values = (captured.items ?? []).map((i) => i.value); - expect(values).toEqual(['keep', 'discard', 'keepAll', 'discardAll']); + expect(values).toEqual([ + 'keep', + 'discard', + 'keepAll', + 'discardAll', + 'turnOff', + ]); + }); + + it('hides the bulk options for a single-skill batch (they would duplicate keep/discard)', () => { + renderDialog([skills[0]!]); + const values = (captured.items ?? []).map((i) => i.value); + expect(values).toEqual(['keep', 'discard', 'turnOff']); + }); + + it('hides the bulk options once only the last skill of a batch remains', async () => { + renderDialog(skills); + // Advance from skill 1/2 to the final skill 2/2, then wait for the + // re-render to reach the mocked RadioButtonSelect. + captured.onSelect!('keep'); + await vi.waitFor(() => { + const values = (captured.items ?? []).map((i) => i.value); + expect(values).toEqual(['keep', 'discard', 'turnOff']); + }); }); it('keep accepts the current skill and does NOT close while more remain', () => { const onAccept = vi.fn(); const onClose = vi.fn(); - render( - , - ); + renderDialog(skills, { onAccept, onClose }); captured.onSelect!('keep'); expect(onAccept).toHaveBeenCalledTimes(1); expect(onAccept).toHaveBeenCalledWith('auto-skill-alpha'); @@ -94,15 +220,7 @@ describe('SkillReviewDialog', () => { it('keep on the last remaining skill closes the dialog', () => { const onAccept = vi.fn(); const onClose = vi.fn(); - render( - , - ); + renderDialog([skills[0]!], { onAccept, onClose }); captured.onSelect!('keep'); expect(onAccept).toHaveBeenCalledTimes(1); expect(onAccept).toHaveBeenCalledWith('auto-skill-alpha'); @@ -112,15 +230,7 @@ describe('SkillReviewDialog', () => { it('keepAll accepts every remaining skill then closes once', () => { const onAccept = vi.fn(); const onClose = vi.fn(); - render( - , - ); + renderDialog(skills, { onAccept, onClose }); captured.onSelect!('keepAll'); expect(onAccept).toHaveBeenCalledTimes(2); expect(onAccept).toHaveBeenCalledWith('auto-skill-alpha'); @@ -131,15 +241,7 @@ describe('SkillReviewDialog', () => { it('discardAll rejects every remaining skill then closes once', () => { const onReject = vi.fn(); const onClose = vi.fn(); - render( - , - ); + renderDialog(skills, { onReject, onClose }); captured.onSelect!('discardAll'); expect(onReject).toHaveBeenCalledTimes(2); expect(onReject).toHaveBeenCalledWith('auto-skill-alpha'); @@ -149,16 +251,409 @@ describe('SkillReviewDialog', () => { it('renders nothing and closes when there are no skills', async () => { const onClose = vi.fn(); - const { lastFrame } = render( - , - ); + const { lastFrame } = renderDialog([], { onClose }); expect(lastFrame()).toBe(''); await vi.waitFor(() => expect(onClose).toHaveBeenCalled()); }); + + // ─── New: inline preview ────────────────────────────────────────────────── + + it('renders the current staged SKILL.md content inline', async () => { + const { lastFrame } = renderDialog(skills); + await vi.waitFor(() => expect(lastFrame()).toContain('ALPHA_BODY_MARKER')); + }); + + it('shows a fallback when the staged file cannot be read', async () => { + const { lastFrame } = renderDialog([ + { + name: 'auto-skill-gone', + description: 'missing', + stagedManifestPath: path.join(tempDir, 'nope', 'SKILL.md'), + }, + ]); + await vi.waitFor(() => + expect(lastFrame()).toContain('Preview unavailable'), + ); + }); + + it('shows loading, not the previous skill body, while the next preview loads', async () => { + // Gate skill B's read behind a deferred promise so the window between + // advancing and B's read resolving is reliably observable. + let releaseBeta!: () => void; + const gate = new Promise((resolve) => { + releaseBeta = resolve; + }); + const realOpen = fs.open.bind(fs); + const openSpy = vi.spyOn(fs, 'open').mockImplementation((async ( + ...args: Parameters + ) => { + if (String(args[0]).includes('auto-skill-beta')) await gate; + return realOpen(...args); + }) as unknown as typeof fs.open); + + try { + const { lastFrame } = renderDialog(skills); + await vi.waitFor(() => + expect(lastFrame()).toContain('ALPHA_BODY_MARKER'), + ); + // Keep skill A → advance to skill B while B's read is still gated. + captured.onSelect!('keep'); + await vi.waitFor(() => expect(lastFrame()).toContain('2/2')); + // Skill B's header must not be rendered over skill A's stale body. + expect(lastFrame()).not.toContain('ALPHA_BODY_MARKER'); + expect(lastFrame()).toContain('Loading preview'); + releaseBeta(); + await vi.waitFor(() => expect(lastFrame()).toContain('BETA_BODY_MARKER')); + } finally { + openSpy.mockRestore(); + } + }); + + it('neutralizes ANSI/VT control sequences in the preview', async () => { + const escPath = path.join(tempDir, 'auto-skill-esc', 'SKILL.md'); + await fs.mkdir(path.dirname(escPath), { recursive: true }); + // Body contains a raw clear-screen (ESC [ 2 J) — a model-generated file + // could smuggle this in to wipe the terminal. + await fs.writeFile(escPath, '---\nname: x\n---\nCTRL_\u001b[2Jclear\n'); + const { lastFrame } = renderDialog([ + { name: 'auto-skill-esc', description: '', stagedManifestPath: escPath }, + ]); + await vi.waitFor(() => expect(lastFrame()).toContain('CTRL_')); + // The raw clear-screen escape must not reach the terminal... + expect(lastFrame()).not.toContain('\u001b[2J'); + // ...it is rendered as inert, escaped text instead. + expect(lastFrame()).toContain('u001b[2J'); + }); + + it('escapes bare control bytes (e.g. BEL) that are not ANSI sequences', async () => { + // BEL (0x07) is a bare C0 byte, not an ANSI escape sequence, so ansi-regex + // does not catch it — the preview sanitizer must handle it separately. + const { lastFrame } = await renderPreviewSkill( + 'auto-skill-bel', + `---\nname: x\n---\nBODY_${String.fromCharCode(7)}END\n`, + ); + await vi.waitFor(() => expect(lastFrame()).toContain('BODY_')); + // The raw BEL byte must not reach the terminal... + expect(lastFrame()).not.toContain(String.fromCharCode(7)); + // ...it is rendered as inert, escaped text instead. + expect(lastFrame()).toContain('u0007'); + }); + + it('escapes DEL and C1 control bytes (JSON.stringify leaves these raw)', async () => { + // 0x9B is the 8-bit CSI (behaves like ESC[) and 0x7F is DEL. JSON.stringify + // returns both unchanged, so they need an explicit code-point escape. + const { lastFrame } = await renderPreviewSkill( + 'auto-skill-c1', + `---\nname: x\n---\nC1_${String.fromCharCode(0x9b)}${String.fromCharCode(0x7f)}END\n`, + ); + await vi.waitFor(() => expect(lastFrame()).toContain('C1_')); + // Neither the 8-bit CSI nor DEL may reach the terminal raw... + expect(lastFrame()).not.toContain(String.fromCharCode(0x9b)); + expect(lastFrame()).not.toContain(String.fromCharCode(0x7f)); + // ...both render as inert, escaped text. + expect(lastFrame()).toContain('u009b'); + expect(lastFrame()).toContain('u007f'); + }); + + it('sanitizes the model-generated name and description in the header', async () => { + // The header fields come from the same model-generated source as the + // preview body (directory basename / frontmatter) — an escape smuggled + // there must not bypass the sanitizer just because it is not in the body. + const stagedManifestPath = await writeSkill( + 'auto-skill-header', + '---\nname: x\n---\nHEADER_BODY\n', + ); + const { lastFrame } = renderDialog([ + { + name: 'evil-\u001b[2Jname', + description: `desc_${String.fromCharCode(7)}end`, + stagedManifestPath, + }, + ]); + await vi.waitFor(() => expect(lastFrame()).toContain('HEADER_BODY')); + // Raw escape/control bytes must not reach the terminal... + expect(lastFrame()).not.toContain('\u001b[2J'); + expect(lastFrame()).not.toContain(String.fromCharCode(7)); + // ...they render as inert, escaped text, same as the preview body. + expect(lastFrame()).toContain('u001b[2J'); + expect(lastFrame()).toContain('u0007'); + }); + + it('renders CRLF line endings as ordinary line breaks, not CR escapes', async () => { + // Windows-authored / editor-saved file: every line ends with \r\n. + const { lastFrame } = await renderPreviewSkill( + 'auto-skill-crlf', + ['---', 'name: x', '---', 'CRLF_LINE_ONE', 'CRLF_LINE_TWO', ''].join( + '\r\n', + ), + ); + await vi.waitFor(() => expect(lastFrame()).toContain('CRLF_LINE_TWO')); + // Both lines render, with no visible CR escape anywhere. + expect(lastFrame()).toContain('CRLF_LINE_ONE'); + expect(lastFrame()).not.toContain('\\r'); + }); + + it('still escapes a lone CR that is not part of a CRLF pair', async () => { + // A bare CR mid-line can rewrite the current row — must stay escaped + // (rendered as the mnemonic `\r`). + const { lastFrame } = await renderPreviewSkill( + 'auto-skill-cr', + '---\nname: x\n---\nLONE_\rMID\n', + ); + await vi.waitFor(() => expect(lastFrame()).toContain('LONE_')); + expect(lastFrame()).toContain('\\r'); + }); + + it('reads only a bounded chunk of a huge preview file (no unbounded read)', async () => { + // > 64 KiB so an unbounded read/render would process the whole file. + const body = Array.from({ length: 20000 }, (_, i) => `LINE_${i}`).join( + '\n', + ); + const bigPath = await writeSkill( + 'auto-skill-big', + `---\nname: big\n---\n${body}\n`, + ); + + // Spy on the bounded read: the component must fs.open + read a capped chunk + // rather than fs.readFile the whole file. Without the cap, fs.open is never + // called (so openCalls stays 0 and this test fails). + let openCalls = 0; + let maxReadLength = 0; + const realOpen = fs.open.bind(fs); + const openSpy = vi.spyOn(fs, 'open').mockImplementation((async ( + ...args: Parameters + ) => { + openCalls++; + const handle = await realOpen(...args); + const realRead = handle.read.bind(handle); + vi.spyOn(handle, 'read').mockImplementation(((...rargs: unknown[]) => { + if (typeof rargs[2] === 'number') { + maxReadLength = Math.max(maxReadLength, rargs[2]); + } + return (realRead as (...a: unknown[]) => unknown)(...rargs); + }) as never); + return handle; + }) as unknown as typeof fs.open); + + try { + const { lastFrame } = renderDialog([ + { + name: 'auto-skill-big', + description: '', + stagedManifestPath: bigPath, + }, + ]); + await vi.waitFor(() => expect(lastFrame()).toContain('LINE_0')); + + // Bounded-read path was taken and never asked for more than the cap + // (+1 byte is read only to detect truncation). + expect(openCalls).toBeGreaterThan(0); + expect(maxReadLength).toBeGreaterThan(0); + expect(maxReadLength).toBeLessThanOrEqual(64 * 1024 + 1); + // Far-down lines are never rendered; the omission is surfaced. + expect(lastFrame()).not.toContain('LINE_400'); + expect(lastFrame()).toMatch(/lines hidden/); + } finally { + openSpy.mockRestore(); + } + }); + + it('caps the preview by WRAPPED rows, not logical lines', async () => { + // One huge logical line and one trailer. Without explicit wrap-aware + // layout, MaxSizedBox counts 2 rows ("fits") while Ink wraps the long line + // into dozens of rendered rows, bypassing PREVIEW_MAX_HEIGHT entirely and + // pushing the options/footer down on small terminals. + const { lastFrame } = await renderPreviewSkill( + 'auto-skill-wide', + `---\nname: wide\n---\n${'WIDE_ROW '.repeat(300)}\nTRAILER_LINE\n`, + ); + await vi.waitFor(() => expect(lastFrame()).toContain('WIDE_ROW')); + const frame = lastFrame()!; + // The wrapped rows shown never exceed the cap (one row is reserved for + // the hidden-lines marker)... + const wideRows = frame + .split('\n') + .filter((l) => l.includes('WIDE_ROW')).length; + expect(wideRows).toBeLessThanOrEqual(11); + // ...the overflow is surfaced, and the trailer (a hidden wrapped row) is + // not rendered. + expect(frame).toMatch(/lines hidden/); + expect(frame).not.toContain('TRAILER_LINE'); + // The footer below the capped preview stays rendered (RadioButtonSelect + // is mocked to null here, so assert on the real footer text instead). + expect(frame).toContain('open in editor'); + }); + + it('flags a byte-truncated preview even when no lines are hidden', async () => { + // A few short lines followed by a large trailing blob pushes the file past + // the 64 KiB read cap. The trailing newlines are stripped, so the visible + // lines fit with NO line-hidden marker — only the explicit truncation + // marker signals that content past the cap was dropped. + const content = `SHORT_1\nSHORT_2\nSHORT_3\n${'\n'.repeat(70 * 1024)}`; + const { lastFrame } = await renderPreviewSkill( + 'auto-skill-trunc', + `---\nname: trunc\n---\n${content}`, + ); + await vi.waitFor(() => expect(lastFrame()).toContain('SHORT_1')); + const frame = lastFrame()!; + // No line-hidden marker (the short lines fit)... + expect(frame).not.toMatch(/lines hidden/); + // ...but the byte-truncation is surfaced explicitly. + expect(frame).toMatch(/truncated/i); + }); + + // ─── New: open in editor (`o`) ──────────────────────────────────────────── + + it('`o` opens the current skill in the editor', async () => { + renderDialog(skills); + pressKey('o'); + await vi.waitFor(() => expect(launchEditor).toHaveBeenCalledTimes(1)); + expect(launchEditor).toHaveBeenCalledWith(skills[0]!.stagedManifestPath); + }); + + it('Ctrl+O and Cmd+O do not launch the editor', () => { + renderDialog(skills); + pressKey('o', { ctrl: true }); + pressKey('o', { meta: true }); + expect(launchEditor).not.toHaveBeenCalled(); + }); + + it('Esc dismisses the dialog (decide later)', () => { + const onDismiss = vi.fn(); + renderDialog(skills, { onDismiss }); + pressKey('escape'); + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it('refreshes the preview with the saved edits after the editor closes', async () => { + const editedPath = skills[0]!.stagedManifestPath; + // Simulate the user editing and saving the file inside the editor. + launchEditor.mockImplementationOnce(async (p: string) => { + await fs.writeFile( + p, + '---\nname: auto-skill-alpha\n---\nEDITED_BODY_MARKER after save.\n', + ); + }); + const { lastFrame } = renderDialog(skills); + await vi.waitFor(() => expect(lastFrame()).toContain('ALPHA_BODY_MARKER')); + pressKey('o'); + await vi.waitFor(() => + expect(launchEditor).toHaveBeenCalledWith(editedPath), + ); + // Preview reloads with the saved contents... + await vi.waitFor(() => expect(lastFrame()).toContain('EDITED_BODY_MARKER')); + // ...and the pre-edit content is gone. + expect(lastFrame()).not.toContain('ALPHA_BODY_MARKER'); + }); + + it('auto-refreshes the preview when the staged file changes on disk', async () => { + // A non-blocking GUI editor (macOS default `open -t`) resolves the launch + // before the user saves, so the preview must pick up later saves via the + // file watcher — no `o` keypress involved here at all. + const { lastFrame } = renderDialog(skills); + await vi.waitFor(() => expect(lastFrame()).toContain('ALPHA_BODY_MARKER')); + await fs.writeFile( + skills[0]!.stagedManifestPath, + '---\nname: auto-skill-alpha\n---\nWATCHED_BODY_MARKER saved later.\n', + ); + await vi.waitFor( + () => expect(lastFrame()).toContain('WATCHED_BODY_MARKER'), + { timeout: 5000 }, + ); + expect(lastFrame()).not.toContain('ALPHA_BODY_MARKER'); + }); + + it('`o` does not advance, accept, reject, or close', async () => { + const onAccept = vi.fn(); + const onReject = vi.fn(); + const onClose = vi.fn(); + const { lastFrame } = renderDialog(skills, { onAccept, onReject, onClose }); + pressKey('o'); + await vi.waitFor(() => expect(launchEditor).toHaveBeenCalled()); + expect(onAccept).not.toHaveBeenCalled(); + expect(onReject).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + // Still on the first skill. + expect(lastFrame()).toContain('1/2'); + }); + + it('clears a stale editor error when advancing to the next skill', async () => { + launchEditor.mockRejectedValueOnce(new Error('EDITOR_BOOM')); + const { lastFrame } = renderDialog(skills); + // Editor launch fails on the current skill → error is shown. + pressKey('o'); + await vi.waitFor(() => expect(lastFrame()).toContain('EDITOR_BOOM')); + // Keep advances to the next skill; the stale error must not carry over. + captured.onSelect!('keep'); + await vi.waitFor(() => expect(lastFrame()).not.toContain('EDITOR_BOOM')); + }); + + // ─── New: turn off (`t`) ────────────────────────────────────────────────── + + it('selecting turn-off disables auto-skill and closes without rejecting or dismissing', () => { + const onReject = vi.fn(); + const onClose = vi.fn(); + const onDismiss = vi.fn(); + renderDialog(skills, { onReject, onClose, onDismiss }); + captured.onSelect!('turnOff'); + expect(setValue).toHaveBeenCalledWith( + expect.anything(), + 'memory.enableAutoSkill', + false, + ); + // Also disabled for the live session, not just persisted for next launch. + expect(setAutoSkillEnabled).toHaveBeenCalledWith(false); + // Closes via onClose, NOT onDismiss: dismissing would blacklist the batch + // for the whole session, so re-enabling auto-skill from /memory could + // never reopen it. The parent's auto-open is gated on the live flag. + expect(onClose).toHaveBeenCalledTimes(1); + expect(onDismiss).not.toHaveBeenCalled(); + expect(onReject).not.toHaveBeenCalled(); + }); + + it('keeps the dialog open and the feature untouched when persisting turn-off fails', async () => { + // saveSettings re-throws write failures (read-only workspace, ENOSPC); + // the throw must not escape the keypress handler, and a half-applied + // turn-off (live flag off, setting still on) must not be left behind. + setValue.mockImplementation(() => { + throw new Error('EACCES: permission denied'); + }); + const onClose = vi.fn(); + const onDismiss = vi.fn(); + const { lastFrame } = renderDialog(skills, { onClose, onDismiss }); + captured.onSelect!('turnOff'); + // The failure is surfaced in the dialog so the user can retry or move on. + await vi.waitFor(() => + expect(lastFrame()).toContain('Failed to save setting'), + ); + expect(lastFrame()).toContain('EACCES'); + expect(setAutoSkillEnabled).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + expect(onDismiss).not.toHaveBeenCalled(); + }); + + it('the retired `t` hotkey is inert', () => { + const onClose = vi.fn(); + const onDismiss = vi.fn(); + renderDialog(skills, { onClose, onDismiss }); + pressKey('t'); + expect(setValue).not.toHaveBeenCalled(); + expect(setAutoSkillEnabled).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + expect(onDismiss).not.toHaveBeenCalled(); + }); + + // ─── New: footer hints ──────────────────────────────────────────────────── + + it('footer surfaces open-in-editor and Esc; turn-off lives in the option list', () => { + const { lastFrame } = renderDialog(skills); + const frame = lastFrame(); + expect(frame).toContain('open in editor'); + expect(frame).toContain('Esc decide later'); + // Turn-off moved from a footer hotkey hint into a visible selector option + // (always last). RadioButtonSelect is mocked, so assert on its items. + expect(frame).not.toContain('turn off'); + const labels = (captured.items ?? []).map((i) => i.label); + expect(labels[labels.length - 1]).toBe('Turn off auto-generated skills'); + }); }); diff --git a/packages/cli/src/ui/components/SkillReviewDialog.tsx b/packages/cli/src/ui/components/SkillReviewDialog.tsx index 8d3d368f76..4d7cde6583 100644 --- a/packages/cli/src/ui/components/SkillReviewDialog.tsx +++ b/packages/cli/src/ui/components/SkillReviewDialog.tsx @@ -5,15 +5,87 @@ */ import { useEffect, useState } from 'react'; +import { promises as fs, watch } from 'node:fs'; +import type { FSWatcher } from 'node:fs'; import { Box, Text } from 'ink'; import { RadioButtonSelect } from './shared/RadioButtonSelect.js'; import type { RadioSelectItem } from './shared/RadioButtonSelect.js'; +import { MaxSizedBox } from './shared/MaxSizedBox.js'; import { useKeypress } from '../hooks/useKeypress.js'; +import { useTerminalSize } from '../hooks/useTerminalSize.js'; +import { useLaunchEditor } from '../hooks/useLaunchEditor.js'; +import { useSettings } from '../contexts/SettingsContext.js'; +import { useConfig } from '../contexts/ConfigContext.js'; +import { SettingScope } from '../../config/settings.js'; +import { + sanitizeFilenameForDisplay, + sanitizeMultilineForDisplay, +} from '../utils/textUtils.js'; import { theme } from '../semantic-colors.js'; import { t } from '../../i18n/index.js'; import type { PendingSkillView } from '../contexts/UIStateContext.js'; +import { createDebugLogger } from '@qwen-code/qwen-code-core'; -type Choice = 'keep' | 'discard' | 'keepAll' | 'discardAll'; +type Choice = 'keep' | 'discard' | 'keepAll' | 'discardAll' | 'turnOff'; + +const debugLogger = createDebugLogger('SKILL_REVIEW_DIALOG'); + +/** How many lines of the staged SKILL.md to show before truncating. */ +const PREVIEW_MAX_HEIGHT = 12; + +/** + * A single editor save fires several raw watch events (inotify reports + * MODIFY/CLOSE_WRITE/ATTRIB separately; FSEvents can also fire more than once) + * and each reload re-reads the file and re-attaches the watcher — coalesce + * them so one save costs one reload. Same interval as SettingsWatcher. + */ +const WATCH_DEBOUNCE_MS = 300; + +/** + * Cap how much of the (model-generated, possibly huge) SKILL.md is read and + * processed. Bounds the read + sanitize + split cost regardless of file size; + * the rendered rows are separately capped to PREVIEW_MAX_HEIGHT. 64 KiB is far + * more than the preview shows but cheap to scan. + */ +const PREVIEW_MAX_BYTES = 64 * 1024; + +/** + * Read at most PREVIEW_MAX_BYTES from the head of the file so an enormous + * SKILL.md can't stall the dialog. A trailing partial UTF-8 char or line is + * harmless because the preview is line-capped anyway. `truncated` reports + * whether the file extends past the cap (one extra byte is requested so this is + * reliable even when the visible lines fit) so the caller can flag the omission + * — a line-hidden marker alone can miss it (e.g. a few short lines then a large + * trailing blob). + */ +async function readPreviewChunk( + filePath: string, +): Promise<{ text: string; truncated: boolean }> { + const handle = await fs.open(filePath, 'r'); + try { + const buf = Buffer.alloc(PREVIEW_MAX_BYTES + 1); + const { bytesRead } = await handle.read(buf, 0, PREVIEW_MAX_BYTES + 1, 0); + const truncated = bytesRead > PREVIEW_MAX_BYTES; + const end = Math.min(bytesRead, PREVIEW_MAX_BYTES); + return { text: buf.toString('utf-8', 0, end), truncated }; + } finally { + await handle.close(); + } +} + +type PreviewState = + | { status: 'loading' } + | { + status: 'ready'; + /** The staged file this preview was read from — a ready preview is only + * valid for that file; rendering guards on it so skill B never shows + * skill A's body while B's read is still in flight. */ + path: string; + lines: string[]; + hiddenLines: number; + truncated: boolean; + } + | { status: 'error' }; export interface SkillReviewDialogProps { skills: PendingSkillView[]; @@ -38,10 +110,168 @@ export const SkillReviewDialog = ({ // (otherwise resolving the current item shifts indices and skips skills). const [snapshot] = useState(() => skills); const [index, setIndex] = useState(0); + const [preview, setPreview] = useState({ status: 'loading' }); + const [actionError, setActionError] = useState(null); + // Bumped after the editor closes to re-read the (possibly edited) staged file. + const [reloadCounter, setReloadCounter] = useState(0); + + const launchEditor = useLaunchEditor(); + const settings = useSettings(); + const config = useConfig(); + const { columns } = useTerminalSize(); + // The dialog does not span the full terminal: the layout caps the dialog + // container at min(terminalWidth − 4, 100) (mainAreaWidth in AppContainer; + // DiffDialog applies the same clamp). MaxSizedBox wraps the preview at this + // width, and its height cap counts the WRAPPED rows — so this must never + // exceed the dialog's actual inner text width (container − marginLeft 1 − + // border 2 − paddingX 2 = container − 5), or Ink would re-wrap at render + // time and push rows past the cap. −6 keeps one column of slack. + const previewWidth = Math.max(20, Math.min(columns - 4, 100) - 6); + + const current = snapshot[index]; + const stagedPath = current?.stagedManifestPath; + + // Load a preview of the staged SKILL.md whenever the current skill changes. + // A cancelled flag guards against a stale read landing after the user has + // advanced (or the file was renamed away by resolving the previous skill). + useEffect(() => { + // A stale editor-launch error belongs to the previous skill; clear it when + // the current skill changes so it doesn't look like the new one failed. + setActionError(null); + if (!stagedPath) return; + let cancelled = false; + let watcher: FSWatcher | undefined; + let debounceTimer: ReturnType | undefined; + // Keep an already-visible preview on screen during a reload — flashing + // "Loading preview…" on every save would flicker. + setPreview((prev) => + prev.status === 'ready' ? prev : { status: 'loading' }, + ); + readPreviewChunk(stagedPath) + .then(({ text, truncated }) => { + if (cancelled) return; + // SKILL.md is model-generated; sanitize control characters before Ink + // writes them. Sanitize before the split so a sequence can't hide across + // a row boundary, then cap the rows we actually render: MaxSizedBox + // truncates the DISPLAY only, so without this cap every line would still + // be laid out and a giant file could freeze the dialog. + // CRLF is an ordinary line ending (Windows-authored or editor-saved + // files), not smuggled control bytes — normalize it before sanitizing + // so lines don't end in a visible CR escape. A lone CR + // (no LF) is still escaped: it can rewrite the current row. + const allLines = sanitizeMultilineForDisplay( + text.replace(/\r\n/g, '\n').replace(/\n+$/, ''), + ).split('\n'); + const lines = allLines.slice(0, PREVIEW_MAX_HEIGHT); + setPreview({ + status: 'ready', + path: stagedPath, + lines, + hiddenLines: allLines.length - lines.length, + truncated, + }); + // GUI editors need not block: the macOS default is `open -t`, which + // returns as soon as TextEdit is told to open the file, so the + // editor-resolve reload below fires before the user has saved anything. + // Watch the staged file and re-read on every change instead. An + // atomic-save rename can kill this watcher, but the bump re-runs the + // effect, which re-attaches a fresh one. + try { + watcher = watch(stagedPath, () => { + if (cancelled) return; + clearTimeout(debounceTimer); + debounceTimer = setTimeout(() => { + if (!cancelled) setReloadCounter((c) => c + 1); + }, WATCH_DEBOUNCE_MS); + }); + // Post-attach failures surface as async 'error' events, which are + // an uncaught exception (fatal via the global handler) unless + // consumed here. Same best-effort stance as the catch below: drop + // the watcher and rely on the blocking-editor reload. + watcher.on('error', () => { + watcher?.close(); + }); + } catch { + // Best-effort: without a watcher, the reload after a blocking + // editor exits (below) still works. + } + }) + .catch((err: unknown) => { + if (cancelled) return; + // ENOENT / EACCES / EIO all render the same "Preview unavailable" — + // keep the underlying cause reachable via the debug log. + debugLogger.warn(`skill preview read failed for ${stagedPath}:`, err); + setPreview({ status: 'error' }); + }); + return () => { + cancelled = true; + clearTimeout(debounceTimer); + watcher?.close(); + }; + // reloadCounter re-runs this after the editor closes or the watcher fires; + // the cancelled flag still guards against a stale read landing after the + // skill changed/unmount. + }, [stagedPath, reloadCounter]); + + const turnOff = () => { + // Persist for next launch... + try { + settings.setValue( + SettingScope.Workspace, + 'memory.enableAutoSkill', + false, + ); + } catch (err) { + // saveSettings re-throws write failures (read-only workspace, ENOSPC). + // Surface the error and keep the dialog open with the feature untouched + // so the choice can be retried — letting the throw escape the keypress + // handler would take down the render tree. + setActionError( + `Failed to save setting: ${err instanceof Error ? err.message : String(err)}`, + ); + return; + } + // ...and stop the scheduler for the rest of THIS session (Config copies the + // setting at startup, so persisting alone wouldn't take effect until relaunch + // and another review could still pop this dialog after the user asked to stop). + config.setAutoSkillEnabled(false); + // Non-destructive: close WITHOUT marking the batch dismissed (onClose, not + // onDismiss). The staged skills stay pending; the parent's auto-open is + // gated on the live auto-skill flag, so the dialog stays away while the + // feature is off but CAN reopen if the user re-enables it from /memory. + // Esc's dismissed-set would suppress this batch for the whole session. + onClose(); + }; + + const openInEditor = () => { + if (!stagedPath) return; + setActionError(null); + void launchEditor(stagedPath) + .then(() => { + // Re-read after a blocking (terminal) editor exits so the keep/discard + // decision reflects the saved contents. Non-blocking GUI editors (the + // macOS default `open -t` returns immediately) are covered by the + // file watcher in the preview effect instead. + setReloadCounter((c) => c + 1); + }) + .catch((err: unknown) => { + setActionError(err instanceof Error ? err.message : String(err)); + }); + }; useKeypress( (key) => { - if (key.name === 'escape') onDismiss(); + if (key.ctrl || key.meta) return; + if (key.name === 'escape') { + onDismiss(); + return; + } + // `o` opens the staged skill in the editor WITHOUT advancing, so the user + // can inspect (or edit) it before deciding keep/discard on return. + if (key.name === 'o') { + openInEditor(); + return; + } }, { isActive: true }, ); @@ -52,12 +282,10 @@ export const SkillReviewDialog = ({ if (snapshot.length === 0) onClose(); }, [snapshot.length, onClose]); - if (index >= snapshot.length) { + if (index >= snapshot.length || !current) { return null; } - const current = snapshot[index]!; - // Advance to the next snapshot entry; close once the last one is decided. const advance = () => { if (index + 1 >= snapshot.length) { @@ -89,6 +317,9 @@ export const SkillReviewDialog = ({ } onClose(); break; + case 'turnOff': + turnOff(); + break; default: break; } @@ -97,13 +328,40 @@ export const SkillReviewDialog = ({ const options: Array> = [ { label: t('Keep this skill'), value: 'keep', key: 'keep' }, { label: t('Discard this skill'), value: 'discard', key: 'discard' }, - { label: t('Keep all remaining'), value: 'keepAll', key: 'keepAll' }, - { - label: t('Discard all remaining'), - value: 'discardAll', - key: 'discardAll', - }, ]; + // With only one skill left, "…all remaining" would mean exactly the same as + // the per-skill options above — offering both is just misleading (and a + // single-skill batch is the common case). Only show the bulk options while + // they actually differ. + if (snapshot.length - index > 1) { + options.push( + { label: t('Keep all remaining'), value: 'keepAll', key: 'keepAll' }, + { + label: t('Discard all remaining'), + value: 'discardAll', + key: 'discardAll', + }, + ); + } + // Feature-level policy option, deliberately LAST — the same shape permission + // prompts use for "don't ask again". A visible option is how an annoyed user + // actually finds the off switch; a footer hotkey hint is not. The label names + // the feature ("auto-generated skills"), not this skill, so it can't be + // misread as a per-skill action. + options.push({ + label: t('Turn off auto-generated skills'), + value: 'turnOff', + key: 'turnOff', + }); + + // The keep-ready-during-reload logic above avoids flicker for SAME-file + // re-reads, but after advancing to the next skill the state still holds the + // previous skill's body until the new read lands. Guard by path at render so + // that window shows "Loading preview…" instead of the wrong skill's content. + const visiblePreview: PreviewState = + preview.status === 'ready' && preview.path !== stagedPath + ? { status: 'loading' } + : preview; return ( - {current.name} + {/* Name and description are model-generated too — sanitize them just + like the preview body, or an escape sequence in the frontmatter + would reach the terminal through the header. */} + + {sanitizeFilenameForDisplay(current.name)} + {current.description ? ( - {current.description} + + {sanitizeMultilineForDisplay(current.description)} + ) : null} + + + {visiblePreview.status === 'loading' ? ( + {t('Loading preview…')} + ) : visiblePreview.status === 'error' ? ( + {t('Preview unavailable')} + ) : ( + <> + + {visiblePreview.lines.map((line, i) => ( + + {/* wrap MUST be explicit: MaxSizedBox's layout treats a Text + with props but no wrap="wrap" as non-wrapping, while Ink + wraps it at render — long lines would then render more + rows than the height cap accounts for. */} + + {line === '' ? ' ' : line} + + + ))} + + {/* Byte-cap truncation is a distinct omission from the line-hidden + marker above, and can happen with no lines hidden at all. */} + {visiblePreview.truncated ? ( + + {t('… preview truncated (file too large) …')} + + ) : null} + + )} + + + {actionError ? ( + + {/* Error messages can embed the staged path, whose basename derives + from the model-generated skill name — sanitize like the rest. */} + + {sanitizeMultilineForDisplay(actionError)} + + + ) : null} + - {t('Esc to decide later')} + + {t('o open in editor · Esc decide later')} + ); diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index 133514a26c..91c73a1367 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -45,6 +45,8 @@ import type { StartupIdeConnectionStatus } from '../../utils/events.js'; export interface PendingSkillView { name: string; description: string; + /** Absolute path of the staged SKILL.md, for inline preview / open-in-editor. */ + stagedManifestPath: string; } export interface UIState { diff --git a/packages/cli/src/ui/utils/textUtils.test.ts b/packages/cli/src/ui/utils/textUtils.test.ts index 63167176f1..467014889c 100644 --- a/packages/cli/src/ui/utils/textUtils.test.ts +++ b/packages/cli/src/ui/utils/textUtils.test.ts @@ -12,6 +12,7 @@ import type { import { escapeAnsiCtrlCodes, sanitizeFilenameForDisplay, + sanitizeMultilineForDisplay, sanitizeSensitiveText, sliceTextByVisualHeight, } from './textUtils.js'; @@ -277,6 +278,23 @@ describe('textUtils', () => { }); }); + describe('sanitizeMultilineForDisplay', () => { + it('preserves line structure while escaping other control bytes', () => { + expect(sanitizeMultilineForDisplay('line one\n\tline two')).toBe( + 'line one\n\tline two', + ); + expect(sanitizeMultilineForDisplay('a\rb\x07c\x9bd')).toBe( + 'a\\rb\\u0007c\\u009bd', + ); + }); + + it('neutralizes ANSI sequences like the filename variant', () => { + const out = sanitizeMultilineForDisplay('x\x1b[2Jy\nz'); + expect(out.includes('\x1b')).toBe(false); + expect(out).toContain('\n'); + }); + }); + describe('sanitizeSensitiveText', () => { it('should return text unchanged if no sensitive patterns', () => { const text = 'Hello, this is a normal prompt'; diff --git a/packages/cli/src/ui/utils/textUtils.ts b/packages/cli/src/ui/utils/textUtils.ts index 4ca213e01f..6a02c68b22 100644 --- a/packages/cli/src/ui/utils/textUtils.ts +++ b/packages/cli/src/ui/utils/textUtils.ts @@ -437,7 +437,12 @@ export function sanitizeSensitiveText( // eslint-disable-next-line no-control-regex const FILENAME_CONTROL_CHARS_REGEX = /[\x00-\x1f\x7f-\x9f]/g; -function escapeFilenameControlChar(ch: string): string { +// Same as FILENAME_CONTROL_CHARS_REGEX minus `\n` (row separator) and `\t` +// (benign indentation), which multi-line display treats as layout. +// eslint-disable-next-line no-control-regex +const MULTILINE_CONTROL_CHARS_REGEX = /[\x00-\x08\x0b-\x1f\x7f-\x9f]/g; + +function escapeControlChar(ch: string): string { switch (ch) { case '\b': return '\\b'; @@ -473,6 +478,20 @@ function escapeFilenameControlChar(ch: string): string { export function sanitizeFilenameForDisplay(name: string): string { return escapeAnsiCtrlCodes(name).replace( FILENAME_CONTROL_CHARS_REGEX, - escapeFilenameControlChar, + escapeControlChar, + ); +} + +/** + * Make untrusted multi-line text (e.g. model-generated file contents) safe to + * render in the TUI while preserving its line structure: neutralizes + * multi-byte ANSI/VT sequences (via `escapeAnsiCtrlCodes`), then escapes the + * remaining bare control bytes — BEL, BS, CR, DEL, C1, the 8-bit CSI — as + * inert, visible text. `\n` and `\t` pass through untouched. + */ +export function sanitizeMultilineForDisplay(text: string): string { + return escapeAnsiCtrlCodes(text).replace( + MULTILINE_CONTROL_CHARS_REGEX, + escapeControlChar, ); } diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index b7fc061d31..a428aff436 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -590,6 +590,17 @@ describe('Server Config (config.ts)', () => { }); }); + describe('setAutoSkillEnabled', () => { + it('flips the live value read by getAutoSkillEnabled', () => { + const config = new Config({ ...baseParams, enableAutoSkill: true }); + expect(config.getAutoSkillEnabled()).toBe(true); + config.setAutoSkillEnabled(false); + expect(config.getAutoSkillEnabled()).toBe(false); + config.setAutoSkillEnabled(true); + expect(config.getAutoSkillEnabled()).toBe(true); + }); + }); + describe('agents.maxParallelAgents', () => { it('configures the background task registry concurrency cap', () => { const config = new Config({ diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 7dafb83853..4f860e0e59 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1710,7 +1710,7 @@ export class Config { // may re-run. Keyed rather than a single boolean so entering a new repo (/cd) // re-checks shareability instead of reusing the first repo's result. private readonly teamMemoryShareabilityChecked = new Set(); - private readonly enableAutoSkill: boolean; + private enableAutoSkill: boolean; private readonly autoSkillConfirm: boolean; private fastModel?: string; private visionModel?: string; @@ -5452,6 +5452,19 @@ export class Config { return this.enableAutoSkill && !this.getBareMode() && !this.isSafeMode(); } + /** + * Toggle auto-skill for the running session. The startup value is copied from + * settings, so persisting a settings change alone would not take effect until + * the next launch; the skill-review scheduler reads `getAutoSkillEnabled()` + * live, so flipping this stops (or resumes) reviews immediately. + * + * @remarks `getAutoSkillEnabled()` additionally gates on bare/safe mode, so + * it can still return false after `setAutoSkillEnabled(true)`. + */ + setAutoSkillEnabled(enabled: boolean): void { + this.enableAutoSkill = enabled; + } + getAutoSkillConfirmEnabled(): boolean { return this.autoSkillConfirm && !this.getBareMode(); }