From 560e6103a9b242ffe13628b7222ecd42cace7495 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 8 Jul 2026 09:36:11 +0800 Subject: [PATCH] feat(cli): review auto-generated skills with an inline preview, editor handoff, and an in-dialog off switch (#6393) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(cli): skill review dialog — inline preview, open-in-editor, turn-off option The auto-skill review dialog now shows the staged SKILL.md inline (sanitized, bounded reads, wrap-aware height cap), opens it in the configured editor without advancing (with watcher-based preview refresh so non-blocking GUI editors work), and offers a visible last option to turn the feature off — effective immediately in-session, persisted at workspace scope, non-destructive to the pending batch. Bulk options render only while at least two skills remain. Re-enabling auto-skill from /memory can resurface a batch put aside by turn-off. * test(integration): render harness + capture scenarios for the skill review dialog Browser-free harness that renders the production dialog from source via an ESM loader hook; its before mode renders the globally installed qwen (no local fixture — the baseline is what actually shipped, or a loud failure). Terminal-capture scenarios produce the PR's before/after screenshots. * fix(cli): address review findings on the skill review dialog - Sanitize the model-generated name and description in the dialog header, same as the preview body — an escape sequence in the frontmatter must not reach the terminal through the header fields. - Clamp the preview width to the dialog container cap (min(columns-4, 100), the same clamp DiffDialog uses) instead of the raw terminal width, which broke the wrapped-row accounting on terminals wider than ~106 columns. - Catch settings persistence failures in the turn-off option: surface the error in the dialog and leave the feature untouched instead of letting the throw escape the keypress handler. - Extract the auto-open gate into shouldAutoOpenSkillReview and cover it with a truth table (turn-off, /memory overlap, re-enable, Esc-dismiss). - Cover the MemoryDialog auto-skill ON->OFF toggle direction. - Release the capture harness temp dir with try/finally. * fix(cli): guard the preview watcher against async errors and event bursts An FSWatcher 'error' event after attach had no listener, so Node raised it as an uncaught exception and the global handler exited the CLI. Consume it and drop the watcher; the blocking-editor reload still works. Also debounce the watch callback (300ms, same as SettingsWatcher): a single editor save fires several raw events, and each one re-read the file and re-attached the watcher. * test(cli): drop white-box watcher tests, keep the end-to-end refresh test The prototype-spy scaffolding tested implementation details (listener registration, synthetic event bursts) and leaned on vite-node interop quirks. The existing on-disk refresh test already exercises the watcher path, debounce included. * fix(cli): sanitize action errors, log preview read failures, cover key guards - Render actionError through sanitizeMultilineForDisplay: error messages can embed the staged path, whose basename derives from the model-generated skill name. - Log the underlying cause when the preview read fails; all failures render the same 'Preview unavailable' otherwise. - Cover Ctrl+O/Cmd+O inertness and Esc dismissal with tests. - Document that getAutoSkillEnabled() also gates on bare/safe mode. --------- Co-authored-by: qwen-code-dev-bot Co-authored-by: Qwen-Coder --- .../scenarios/skill-review-dialog.ts | 69 ++ .../skill-review-harness/text-capture.tsx | 407 +++++++++++ packages/cli/src/ui/AppContainer.test.tsx | 78 +++ packages/cli/src/ui/AppContainer.tsx | 50 +- .../src/ui/components/MemoryDialog.test.tsx | 46 ++ .../cli/src/ui/components/MemoryDialog.tsx | 8 +- .../ui/components/SkillReviewDialog.test.tsx | 631 ++++++++++++++++-- .../src/ui/components/SkillReviewDialog.tsx | 342 +++++++++- .../cli/src/ui/contexts/UIStateContext.tsx | 2 + packages/cli/src/ui/utils/textUtils.test.ts | 18 + packages/cli/src/ui/utils/textUtils.ts | 23 +- packages/core/src/config/config.test.ts | 11 + packages/core/src/config/config.ts | 15 +- 13 files changed, 1609 insertions(+), 91 deletions(-) create mode 100644 integration-tests/terminal-capture/scenarios/skill-review-dialog.ts create mode 100644 integration-tests/terminal-capture/skill-review-harness/text-capture.tsx 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(); }