[Critical] Silent data loss regression: tryExtractFilePaths extracts paths from within arbitrary text and replaces the entire paste with only @-prefixed paths. Non-path text is silently discarded.

The old code was all-or-nothing: it only prepended @ if the entire pasted string was a single valid path, otherwise it kept the text unchanged. The new code breaks this — pasting "Check out /src/index.ts and /src/utils.ts" produces "@/src/index.ts @/src/utils.ts " (words "Check out" and "and" are lost).

This affects both the single-line branch (here) and the multi-line branch above. The existing test "should only prepend @ to valid paths among mixed content" actually demonstrates this data loss by treating /path/to/invalid.txt being dropped as correct behavior — but prose text between valid paths is equally dropped.

Suggested fix: Only transform the paste when the content looks like a pure list of paths (every token is either a valid path or whitespace/newline). If any non-path, non-separator token exists, fall back to inserting the raw paste unchanged. This preserves the original all-or-nothing semantics while still supporting the multi-file use case.

— qwen3.7-max via Qwen Code /review

[Critical] O(n²) synchronous filesystem calls on the UI thread. The inner loop tries every j from i+1 to tokens.length, calling isValidPath (which does fs.existsSync + fs.statSync) on each candidate. For N tokens of non-path text, this is N*(N+1)/2 iterations with ~2 sync syscalls each.

Pasting a 40-word paragraph of non-path text produces ~820 isValidPath calls (~1640 sync fs operations), all blocking the React/Ink render thread. This can cause a visible UI freeze.

Suggested fix: Pre-filter tokens that look like path prefixes (start with /, ./, ~/, or a drive letter) before entering the inner loop. For tokens that can't plausibly be paths, skip directly to i++. This reduces the worst case dramatically for typical prose pastes.

— qwen3.7-max via Qwen Code /review

[Critical] No test exercises the greedy multi-token matching or the escapePath integration. All 6 new tests use paths without spaces (/path/to/file1.txt), yet the greedy for (let j = i + 1; ...) loop and escapePath (which backslash-escapes spaces) exist specifically to handle paths with spaces.

If the greedy matching or escaping has a bug, it would go completely undetected. Users dragging files like My Document.txt from Finder would get broken @ references.

Suggested fix: Add tests like:

it('should handle unquoted paths with spaces via greedy matching', () => {
  const { result } = renderHook(() =>
    useTextBuffer({
      viewport,
      isValidPath: (p: string) => p === '/path/to/my file.txt',
    }),
  );
  act(() => result.current.insert('/path/to/my file.txt', { paste: true }));
  expect(getBufferState(result).text).toBe('@/path/to/my\\ file.txt ');
});

— qwen3.7-max via Qwen Code /review

[Suggestion] The regex /'([^']*)'/g matches text between any two apostrophes, including English contractions. Input like don't /src/file.ts it's done produces a false match 't /src/file.ts it' (capturing everything between the apostrophe in "don't" and the one in "it's"). This sets hasQuotedPaths = true, which changes how the line is processed — the actual path /src/file.ts gets trapped inside the false quoted match and fails isValidPath, then is silently dropped.

Suggested fix: Require the quoted content to look path-like before treating it as a quoted path:

const quotedPathRegex = /'([\/~.][^']*)'/g;

Or only set hasQuotedPaths = true when at least one quoted match is actually a valid path.

— qwen3.7-max via Qwen Code /review

[Suggestion] Both the if (validPaths) branch and the else branch below dispatch the identical action dispatch({ type: 'insert', payload: ch }) followed by return. This can be flattened:

if (paste && /[\n\r]/.test(ch) && !shellModeActive) {
  const validPaths = tryExtractFilePaths(ch, isValidPath);
  if (validPaths) {
    ch = `${validPaths.join(' ')} `;
  }
  dispatch({ type: 'insert', payload: ch });
  return;
}

— qwen3.7-max via Qwen Code /review

[Suggestion] Several branches in the new code lack test coverage:

1. CRLF normalization (this line): content.replace(/\r\n/g, '\n').replace(/\r/g, '\n') is never tested with \r\n or bare \r input.
2. Shell mode + newline paste: the !shellModeActive guard above means newline pastes in shell mode skip path extraction — no test verifies this fallthrough.
3. Null return from tryExtractFilePaths: when no valid paths are found in a newline paste, the raw content is dispatched — no test covers this branch.

Suggested fix: Add tests for each branch:

// CRLF
act(() => result.current.insert('/a.txt\r\n/b.txt', { paste: true }));
// Shell mode + newline
useTextBuffer({ viewport, isValidPath: () => true, shellModeActive: true });
act(() => result.current.insert('/a.txt\n/b.txt', { paste: true }));
// No valid paths
useTextBuffer({ viewport, isValidPath: () => false });
act(() => result.current.insert('line one\nline two', { paste: true }));

— qwen3.7-max via Qwen Code /review

[Suggestion] The JSDoc says escapePath escapes for parseAllAtCommands, but the two functions have a mismatched character set. The parser terminates at /[,\s;!?()[\]{}]/ (line 94 of atCommandProcessor.ts) — note the comma. But SHELL_SPECIAL_CHARS used by escapePath is /[ \t()[\]{};|*?$'"#&<>!~]/` — no comma.

A file like /path/to/report,v2.txt will be output as @/path/to/report,v2.txt (comma unescaped), and the parser will truncate at the comma, reading only @/path/to/report.

Suggested fix: Add , to SHELL_SPECIAL_CHARS in packages/core/src/utils/paths.ts, or additionally escape commas in the output after escapePath.

— qwen3.7-max via Qwen Code /review

[Suggestion] looksLikePath is too restrictive compared to the old single-path paste logic it replaces. The old code (on main) passed any token directly to isValidPath with no prefix check. This new pre-filter rejects bare relative paths like package.json, .gitignore, and src/index.ts — none of which start with /, ./, ../, ~/, or a drive letter.

This is a silent regression: pasting package.json (which exists on disk) used to produce @package.json but now preserves the original text unchanged.

Adding unquoted.startsWith('.') covers dotfiles (.gitignore, .env). Adding unquoted.includes('/') covers bare relative paths (src/index.ts, packages/core/src/utils/paths.ts) while still rejecting plain words.

— qwen3.7-max via Qwen Code /review

[Suggestion] Shortest-match-first greedy matching can miss transformations for unquoted paths with spaces. If /tmp/a exists as a file and the user pastes /tmp/a b.txt, the algorithm matches /tmp/a at j = i + 1 (shortest), then b.txt fails looksLikePath, setting hadNonPathToken = true. The entire paste falls back to original content — the valid longer path /tmp/a b.txt is never tried.

Consider reversing the loop to longest-match-first:

This tries the longest candidate first and falls back to shorter ones, matching user intent more accurately for unquoted paths with spaces. Trade-off: more isValidPath calls in the worst case, but looksLikePath pre-filtering limits this to path-like tokens only.

— qwen3.7-max via Qwen Code /review

[Suggestion] Adding , to SHELL_SPECIAL_CHARS has global side effects on all 15+ callers of escapePath()/unescapePath() across every tool (edit, read-file, write-file, glob, grep, etc.). Comma is not a POSIX shell metacharacter — it's included here because parseAllAtCommands treats it as a path terminator, but the JSDoc still says "shell metacharacters."

Two follow-up items:

1. The vscode-ide-companion package has its own copy of SHELL_SPECIAL_CHARS at packages/vscode-ide-companion/src/utils/imageSupport.ts:31 that does NOT include comma. If a comma-escaped path from core flows into the extension's unescapePath, the \, won't be unescaped, leaving a stray backslash.
2. Consider adding a test in paths.test.ts for comma-escaping to prevent silent regression.

— qwen3.7-max via Qwen Code /review

[Suggestion] Several code paths in the new helpers lack test coverage:

1. Relative and home-dir paths: looksLikePath has explicit branches for ./, ../, and ~/ but all 11 tests use absolute paths starting with /. A test like './src/index.ts ../lib/utils.ts ~/notes.md' would cover these.
2. Unquoted path that looks like a path but is invalid: The !found branch in extractPathsFromSegment (line 2048) is never exercised. Existing non-path tests use quoted invalid paths which are handled by quotedPathRegex, not this branch. A test like '/valid/file.txt /nonexistent/path' (where only the first is valid) would cover it.
3. paths.test.ts comma test: SHELL_SPECIAL_CHARS was modified to include comma but the exhaustive special-characters test in paths.test.ts was not updated.

— qwen3.7-max via Qwen Code /review

[Suggestion] Stale JSDoc: says "Tries shortest possible path first" but the implementation at line 2037 iterates for (let j = tokens.length; j >= i + 1; j--) — longest-match-first. The inline comment three lines below correctly says "Try longest-match-first". A future reader relying on this JSDoc will misunderstand the algorithm and may "fix" the code to match the docstring, silently breaking paths with spaces.

— qwen3.7-max via Qwen Code /review

[Suggestion] unquoted.includes('/') is overly broad as a path pre-filter — it matches URLs (https://example.com/foo), dates (2026/05/27), MIME types (text/plain), and git diff lines (+++ b/src/foo.ts). These all pass through to the greedy matching loop where each candidate triggers synchronous fs.existsSync + fs.statSync calls.

The preceding checks (startsWith('/'), ./, ../, ~/, drive letter) already cover all standard path formats. The new test for relative paths only covers ./, ../, and ~/ prefixed paths — no test exercises bare src/foo.ts paths (without prefix), which was presumably the reason for adding this catch-all.

Consider removing unquoted.includes('/'). If bare relative paths like src/foo.ts need support, add a test for them and use a more targeted check (e.g., require a known file extension).

— qwen3.7-max via Qwen Code /review

[Suggestion] vscode-ide-companion has its own copy of SHELL_SPECIAL_CHARS at packages/vscode-ide-companion/src/utils/imageSupport.ts:31 that does NOT include ,:

// imageSupport.ts:31 (unchanged by this PR):
export const SHELL_SPECIAL_CHARS = /[ \t()[\]{};|*?$`'"#&<>!~]/;

// paths.ts:47 (this PR):
export const SHELL_SPECIAL_CHARS = /[ \t()[\]{};|*?$`'"#&<>!~,]/;

The two packages' escape/unescape behavior now diverges — core's escapePath escapes commas, vscode's does not. If the vscode extension generates @-paths consumed by core's parseAllAtCommands, paths with commas will be truncated.

Consider syncing the vscode copy, or better: eliminating the duplication by importing from core.

— qwen3.7-max via Qwen Code /review

[Suggestion] Test coverage gap for shell metacharacters: only spaces and commas are tested. Real-world filenames commonly contain parentheses (file(1).txt, macOS downloads), semicolons (report;v2.txt), or brackets (data[2024].csv). If escapePath fails to escape any of these, parseAllAtCommands (which terminates at /[,\s;!?()[\]{}]/) would silently truncate the @-path with no user-visible error.

Also: the single-file paste path (line 2185) now goes through escapePath — the old code did NOT escape, so this silently fixes a pre-existing truncation bug for paths like /Downloads/report(v2).txt. No test validates this fix.

Suggest adding at least one test:

it('should escape shell metacharacters in paths', () => {
  const { result } = renderHook(() =>
    useTextBuffer({
      viewport,
      isValidPath: (p: string) => p === '/Downloads/report(v2).txt',
    }),
  );
  act(() => result.current.insert("'/Downloads/report(v2).txt'", { paste: true }));
  expect(getBufferState(result).text).toBe('@/Downloads/report\\(v2\\).txt ');
});

— qwen3.7-max via Qwen Code /review

[Suggestion] Missing short-circuit when hadNonPathToken is already set. Once any token is flagged as a non-path, the all-or-nothing check at the end of tryExtractFilePaths guarantees the result will be null — but both loops continue processing all remaining tokens, making unnecessary isValidPath (fs) calls.

And in the outer tryExtractFilePaths line loop:

for (const line of lines) {
  if (hadNonPathToken.value) break;
  // ... existing logic
}

This eliminates wasted fs calls when the paste contains mixed path/non-path content.

— qwen3.7-max via Qwen Code /review

[Suggestion] escapePathAndCommas is a trivial pass-through to escapePath — the comma escaping only works because , was added to the global SHELL_SPECIAL_CHARS in this PR. Two follow-ups:

1. Missing test in paths.test.ts: the escapePath describe block (line 68) has no test for comma escaping. The special-char test string at line 189 omits ,. A future change could revert the comma from SHELL_SPECIAL_CHARS and no core-level test would catch it. Add: expect(escapePath('file,v2.txt')).toBe('file\\,v2.txt');

2. Consider either inlining escapePath(path) at call sites (removing the misleading wrapper name) or making the comma logic explicit here: return escapePath(path).replace(/,/g, '\\,') — this decouples from the global regex change.

— qwen3.7-max via Qwen Code /review

[Suggestion] quotedPathRegex = /'([~/.][^']*)'/g does not match Windows drive-letter paths. The character class [~/.] requires the quoted content to start with ~, /, or ., so paths like 'C:/Users/My Documents/file.txt' are never matched by this regex. They fall through to extractPathsFromSegment, which splits by whitespace — 'C:/Users/My fails looksLikePath (quote-strip requires both quotes, drive-letter test sees ' not C), setting hadNonPathToken = true. The paste silently preserves the original text unchanged — a regression from the old code which handled 'C:/Users/My Documents/file.txt' via potentialPath.match(/^'(.*)'$/).

Also add a test for quoted Windows paths with spaces, and for the drive-letter branch of looksLikePath (/^[A-Za-z]:/) which currently has zero test coverage.

— qwen3.7-max via Qwen Code /review

[Suggestion] looksLikePath drops bare filenames — behavioral regression for single-file paste. The old code on main called isValidPath(unescapePath(potentialPath)) directly on any pasted text ≥ 3 chars with no prefix check. The new code routes through tryExtractFilePaths → extractPathsFromSegment → looksLikePath, which rejects tokens not starting with /, ./, ../, ~/, ., or a drive letter. A paste of README.md (when it exists as a file in CWD) previously produced @README.md ; it now produces README.md unchanged.

— qwen3.7-max via Qwen Code /review

[Critical] looksLikePath silently rejects quoted Windows drive-letter paths with spaces (e.g., 'C:\Users\my file.txt').

When extractPathsFromSegment splits by whitespace, the first token becomes 'C:\Users\my (with leading '). looksLikePath strips quotes via str.replace(/^'(.*)'$/, '$1') which requires both quotes — so the leading ' survives. Then ^[A-Za-z]: fails because the first char is ', not a letter. looksLikePath returns false → hadNonPathToken = true → entire paste preserved unchanged.

The greedy loop (line 2033) would correctly reconstruct the full quoted string and strip quotes — but it is unreachable because looksLikePath rejects the first token before the loop is entered.

Regression: the old code stripped quotes from the whole string before validating, so 'C:\Users\my file.txt' worked correctly.

— qwen3.7-max via Qwen Code /review

[Suggestion] Missing test for Windows drive-letter paths. looksLikePath has an explicit /^[A-Za-z]:/ branch (line 1919) but no test exercises it. All 16+ new tests use Unix-style paths (/, ./, ../, ~/). A regression in the drive-letter branch would go undetected.

it('should handle Windows drive-letter paths', () => {
  const { result } = renderHook(() =>
    useTextBuffer({
      viewport,
      isValidPath: (p: string) =>
        p === 'C:\\Users\\file.txt' || p === 'D:\\data\\report.csv',
    }),
  );
  act(() =>
    result.current.insert('C:\\Users\\file.txt D:\\data\\report.csv', {
      paste: true,
    }),
  );
  expect(getBufferState(result).text).toBe(
    '@C:\\Users\\file.txt @D:\\data\\report.csv ',
  );
});

— qwen3.7-max via Qwen Code /review

[Suggestion] The exhaustive escapePath test (paths.test.ts:188, "should handle paths with only special characters") does not include , in its input or expected output. Comma has zero unit-test coverage in escapePath/unescapePath despite being added to SHELL_SPECIAL_CHARS. The text-buffer integration test (comma escaping) covers it indirectly, but if escapePath itself regresses on comma, only that one integration test would catch it.

Update paths.test.ts:188 to include comma:

expect(escapePath(' ()[]{};&|*?$`\'"#!~<>,')).toBe(
  '\\ \\(\\)\\[\\]\\{\\}\\;\\&\\|\\*\\?\\$\\`\\\'\\"\\#\\!\\~\\<\\>\\,',
);

— qwen3.7-max via Qwen Code /review

[Suggestion] JSDoc summary is incomplete — lists only /, ./, ~/, and drive letter, but the implementation also checks ../ (line 1919) and bare ./.. (line 1921).

— qwen3.7-max via Qwen Code /review

[Suggestion] JSDoc says "Escapes spaces and commas" but escapePath actually escapes all SHELL_SPECIAL_CHARS (22 characters including parentheses, brackets, semicolons, pipes, etc.). The tests at lines 768-797 verify parentheses and brackets are escaped, contradicting this doc.

— qwen3.7-max via Qwen Code /review

[Suggestion] The tokens.length > 1 guard intentionally bypasses looksLikePath for single-token segments to preserve bare-filename behavior (e.g., pasting README.md). The comment documents this intent, but no test exercises it. A regression that removes this guard would silently break bare-filename paste with no test failure.

Consider adding:

it('should prepend @ to a bare filename when isValidPath returns true', () => {
  const { result } = renderHook(() =>
    useTextBuffer({ viewport, isValidPath: (p) => p === 'README.md' }),
  );
  act(() => result.current.insert('README.md', { paste: true }));
  expect(getBufferState(result).text).toBe('@README.md ');
});

— qwen3.7-max via Qwen Code /review