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

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

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

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

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

[Suggestion] No test proves opts.editor overrides preferredEditor

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

Consider adding a test like:

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

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

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

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

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

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

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

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

[Suggestion] Sandbox-aware /editor policy is silently bypassed by this env-var fallback

usePreferredEditor correctly returns undefined for GUI editors under sandbox (via allowEditorTypeInSandbox). But here, an undefined preferredEditor makes resolved null and execution falls through to $VISUAL / $EDITOR / vi|notepad with no sandbox re-check. The sandbox layer (packages/cli/src/utils/sandbox.ts:294) inherits the host process.env wholesale — never strips VISUAL/EDITOR. So a user with /editor zed configured and VISUAL=/Applications/Zed.app/Contents/MacOS/cli set in their shell, running under sandbox-exec: the hook politely refuses to honor /editor zed, then this fallback launches the same Zed binary anyway. The layered defense (/editor allowlist + sandbox check) is hollow whenever the host shell has $VISUAL/$EDITOR set.

Fix: plumb a "blocked by sandbox" signal from usePreferredEditor into openInExternalEditor (e.g., a sentinel like 'blocked' distinct from undefined) and refuse the env-var fallback in that case; or validate the env-var basename against isValidEditorType + allowEditorTypeInSandbox and reject GUI editors when process.env.SANDBOX is set.

— claude-opus-4-7 via Claude Code /qreview

[Suggestion] setRawMode refcount leak when writeFileSync throws — corrupts terminal at exit

Pre-PR, writeFileSync lived outside the try, so a throw bypassed both setRawMode(false) and setRawMode(true) — terminal state preserved. The new code moves writeFileSync inside try (good — needed to keep tmpDir cleanup paired). But the ordering is now:

const wasRaw = stdin?.isRaw ?? false;
try {
  fs.writeFileSync(filePath, text, ...);  // may throw
  setRawMode?.(false);                     // skipped if writeFileSync threw
  ...
} catch (err) { ... }
finally {
  try { if (wasRaw) setRawMode?.(true); } ...   // ALWAYS runs
  ...
}

If writeFileSync throws (disk full, EACCES on /tmp, ENOSPC), (false) is skipped but (true) still runs. Ink's raw mode is refcounted — each (true) increments, (false) decrements, and the terminal stays raw while count > 0. One failure → +1 leaked. On app exit, KeypressContext's cleanup decrements by 1 but the count stays > 0, so Ink never disables raw mode — the user's shell prompt is broken until they run stty sane / reset.

The existing test should clean up temp file when writeFileSync throws (line 133) doesn't pass stdin / setRawMode mocks, so this regression is undetected.

Fix: move setRawMode?.(false) before writeFileSync, or gate the restore on a rawDisabled flag set right after the disable call:

let rawDisabled = false;
try {
  fs.writeFileSync(filePath, text, { encoding: 'utf8', mode: 0o600 });
  setRawMode?.(false);
  rawDisabled = true;
  // ... spawnSync, readFileSync ...
} catch (err) { ... }
finally {
  try { if (rawDisabled && wasRaw) setRawMode?.(true); } catch ...
  ...
}

Also add a regression test: stdin: { isRaw: true } + setRawMode mock + writeFileSync forced to throw, assert setRawMode(false) / setRawMode(true) call counts are balanced.

— claude-opus-4-7 via Claude Code /qreview

[Suggestion] Duplicated Windows .cmd/.bat unsafe-character guard across the two branches

A ~14-line block — platform check + /\.(cmd|bat)$/i regex + /["|%!]/ unsafe-char check + fs.rmSync({ recursive, force }) cleanup + return + useShell = true — appears nearly verbatim here (opts.editor arm, lines 2243-2256) and in the env-var arm (lines 2278-2291). The two blocks differ only in the error-message phrase ("opts.editor command contains" vs "Editor command from environment contains").

This is real lockstep coupling, not speculative DRY — the PR history shows multiple regex updates (/["|]/ → /["|%!]/, unlinkSync/rmdirSync → fs.rmSync) had to touch both arms in sync.

Fix: hoist the guard out. The preferred path already returns needsShell from getExternalEditorCommand. After the if/else sets editorCmd/editorArgs/editorSource, run a single guard for the non-preferred cases:

if (editorSource !== 'preferred' &&
    process.platform === 'win32' &&
    /\.(cmd|bat)$/i.test(editorCmd)) {
  if (/["|%!]/.test(editorCmd)) {
    debugLogger.error(
      `[useTextBuffer] ${editorSource} editor command contains unsafe characters: ${editorCmd}`,
    );
    try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch { /* ignore */ }
    return;
  }
  useShell = true;
}

— claude-opus-4-7 via Claude Code /qreview

[Suggestion] opts.editor and preferredEditor branches have asymmetric semantics — API trapdoor

preferredEditor flows through getExternalEditorCommand, which (a) resolves 'vscode' → 'code' / 'code.cmd' via getEditorExecutable, (b) appends --wait for GUI editors, (c) derives needsShell. The opts.editor branch does none of this — editorArgs = [filePath], editorCmd = opts.editor literal, no PATH resolution, no --wait, useShell conditional only on .cmd/.bat suffix of the raw string.

A caller doing openInExternalEditor({ editor: 'vscode' }) (a natural-looking call given the type { editor?: string }) would spawn the literal 'vscode' — not a binary on any platform — yielding ENOENT. If they pass 'code', the GUI editor would fork-detach instantly because --wait is missing, and the function would read back the unchanged temp file and silently no-op. The test at line 647 hard-codes this asymmetry by using 'nano' (a raw binary, not an EditorType), so a future refactor trying to unify the two branches would see the test fail and back off.

Fix: either (a) route opts.editor through getExternalEditorCommand when isValidEditorType(opts.editor), falling through to the raw-string spawn only for non-EditorType strings; or (b) narrow the type to opts: { editor?: string /* raw binary path, NOT EditorType */ } and add a comment here explaining the asymmetry, so a future "fix" doesn't regress the test at line 647.

— claude-opus-4-7 via Claude Code /qreview

[Suggestion] JSDoc omits opts.editor from the resolution order

This JSDoc lists the resolution order as /editor preference → $VISUAL → $EDITOR → platform default. But the implementation at line 2238 places opts.editor at highest priority ("Explicit programmatic override takes highest priority"). The function's only documented parameter (opts?: { editor?: string } at line 2707) is unmentioned in the doc.

A reader consulting only the JSDoc would not know that the override exists or that it bypasses the entire /editor / sandbox-gate machinery. Combined with the asymmetric semantics flagged at line 2239, this misleads anyone trying to call the function programmatically.

— claude-opus-4-7 via Claude Code /qreview

[Suggestion] .bat extension never actually tested on the CLI side, only .cmd

The test name claims .cmd/.bat coverage, but the test input is only code.cmd. No test in this file ever passes a .bat editor — the opts.editor shell-mode test at line ~668, the env-var unsafe-char it.each at line 695, and the opts.editor unsafe-char it.each at line ~852 all use exclusively .cmd strings.

The two /\.(cmd|bat)$/i regex sites in text-buffer.ts (lines 2243 opts.editor arm and 2278 env-var arm) have zero .bat coverage on the CLI side. The matching editor.ts:171 regex IS guarded by editor.test.ts:893, but that only exercises the preferredEditor path through getExternalEditorCommand; the CLI-side env-var and opts.editor branches remain unguarded.

Impact: a typo like /\.cmd$/i or a regex consolidation that drops bat would silently pass the entire test suite while breaking Windows .bat editor handling.

Fix: convert the existing .cmd tests (env-var line 350, opts.editor line ~668) into it.each([{ ext: 'cmd' }, { ext: 'bat' }]) parametrised tests, or add two dedicated .bat smoke tests covering both arms. Bonus: add a .bat case to the unsafe-char it.each at lines 695 and ~852 (e.g., expand%PATH%.bat).

— claude-opus-4-7 via Claude Code /qreview

[Suggestion] Missing test for setRawMode(true) throw path inside the finally block

The inner try/catch wrapping setRawMode?.(true) at text-buffer.ts:2335-2342 was added specifically so a stdin-destroyed scenario (remote SSH session dies while editor is open, terminal multiplexer detached, etc.) doesn't prevent the subsequent fs.rmSync cleanup from running.

Both existing raw-mode tests (should disable and restore raw mode around editor session at line 730, this test at line 755) use mockSetRawMode = vi.fn() which never throws. The spawnSync-failure variant only verifies that setRawMode(true) is called, not that rmSync still runs when setRawMode(true) itself throws. A regression removing the inner try/catch (or moving rmSync above it) would silently leak temp directories AND produce an unhandled rejection from the finally — every existing test would still pass.

Fix: add a test that throws from the second setRawMode call and asserts fs.rmSync was still invoked and the promise resolves:

it('should still clean up tmpDir when setRawMode(true) throws', async () => {
  mockSetRawMode
    .mockImplementationOnce(() => {})           // setRawMode(false) succeeds
    .mockImplementationOnce(() => {              // setRawMode(true) throws
      throw new Error('ERR_STREAM_DESTROYED');
    });
  // ... run editor ...
  expect(fs.rmSync).toHaveBeenCalledWith(
    expect.stringContaining('qwen-edit-'),
    expect.objectContaining({ recursive: true, force: true }),
  );
  // and the promise resolved — no unhandled rejection
});

— claude-opus-4-7 via Claude Code /qreview