feat(cli): review auto-generated skills with an inline preview, editor handoff, and an in-dialog off switch (#6393)

* 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 <qwen-code-dev-bot@users.noreply.github.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

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<ScenarioConfig, 'terminal' | 'gif'>;
+
+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[];

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 "<prompt>" --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<string, unknown>,
+      ) => { unmount: () => void; cleanup?: () => void };
+      require_jsx_runtime: () => {
+        jsx: (type: unknown, props: Record<string, unknown>) => 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) => (
+    <KeypressProvider kittyProtocolEnabled={false}>
+      <ConfigContext.Provider value={fakeConfig}>
+        <SettingsContext.Provider value={fakeSettings}>
+          {node}
+        </SettingsContext.Provider>
+      </ConfigContext.Provider>
+    </KeypressProvider>
+  );
+
+  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 (
+            <Text>
+              Auto-skill turned off. Re-enable it any time from /memory.
+            </Text>
+          );
+        }
+        return (
+          <SkillReviewDialog
+            skills={dialogSkills}
+            onAccept={noop}
+            onReject={noop}
+            onClose={() => 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(<AfterHarness dialogSkills={[skills[0]!]} />),
+        );
+        // 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(<AfterHarness dialogSkills={skills} />));
+        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);
+  },
+);