From 112df5911791ea71ee9efbb98ea59c5ded1ea161 Mon Sep 17 00:00:00 2001 From: Paulo Reis Date: Mon, 6 Apr 2026 06:49:38 -0300 Subject: [PATCH] fix: convert dragged file paths to @mentions for attachment (#382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: convert dragged file paths to @mentions for attachment When non-image files are dragged into the terminal, the file path was inserted as plain text and never attached. Now detected absolute paths are converted to @mentions so they get picked up by the attachment system. * test: add tests for drag-and-drop file path detection * fix: multi-image drag-and-drop only showing last image insertTextAtCursor read input and cursorOffset from the React closure, which is stale when called in a synchronous loop (e.g. onImagePaste for multiple dragged images). Now uses refs so each insertion chains on the previous one. * fix: quote Windows absolute paths to avoid MCP mention collision Paths containing ':' (e.g. Windows drive letters) are now emitted in quoted @"..." form so they don't match the MCP resource mention regex. Co-Authored-By: Claude Opus 4.6 (1M context) * refactor: decouple dragDropPaths from imagePaste and harden image checks - Check image extension against the cleaned path (post quote/escape stripping) so quoted or backslash-escaped image drops are reliably routed to the image paste handler. - Inline the image extension regex and drop the imagePaste/fsOperations imports so the module (and its tests) no longer pull in `bun:bundle` and the heavier fs wrapper chain. Use plain `fs.existsSync` for the on-disk check. - Add tests covering quoted image paths, uppercase extensions, backslash-escaped image paths, escaped real files with spaces, mixed segments containing an image, quoted-nonexistent paths, and leading or trailing whitespace. * test: verify dragged paths with an `@` segment are preserved Adds a fixture under a scoped-package-style subdir (`@types/index.d.ts`) so we exercise the realistic `node_modules/@types/...` drag case and lock in that `extractDraggedFilePaths` returns the raw path unchanged — the `@` inside the path must not collide with the mention prefix the caller prepends downstream. * test: parametrize dragDropPaths cases with test.each Groups the 21 scenarios into four table-driven describes (empty-result, single-path, multi-path, backslash-escaped) so that adding a new case is a one-line row instead of a new `test()` block. Fixture directories are now created synchronously at describe-load time so their paths are available to the test.each tables, which are built before any hook runs. * test: add contract tests for @-mention extractor boundary Pins the contract between `extractAtMentionedFiles` and `extractMcpResourceMentions` so the MCP regex can't silently swallow quoted file-path mentions. These tests fail on current HEAD — 3 of 11 cases expose the regression pointed out in the review on #382: `extractMcpResourceMentions`'s trailing `\b` backtracks past the closing `"` of a quoted mention and produces a ghost match for `@"C:\Users\..."`, `@C:\Users\...`, and `@"/tmp/weird:name.txt"`. The remaining 8 cases lock in the behaviour that must not change (legitimate `server:resource` mentions and plain file-path mentions). Committed failing on purpose as the first half of a test-then-fix pair; the regex fix follows in a subsequent commit. * fix: prevent MCP extractor from ghost-matching quoted/Windows paths The MCP resource regex used `\b` as a trailing anchor with `[^\s]+` character classes. On any quoted file mention containing a colon (`@"C:\Users\me\file.txt"`, `@"/tmp/weird:name.txt"`), the engine backtracked past the closing `"` to satisfy `\b`, producing a ghost match that collided with `extractAtMentionedFiles`. Unquoted Windows drive-letter paths (`@C:\Users\me\file.txt`) also matched because a drive letter is structurally identical to an MCP `server:resource` token. Two guards: 1. `(?!")` right after `@` drops quoted tokens entirely, and adding `"` to the character classes blocks any mid-match backtracking. 2. A post-match filter discards `^[A-Za-z]:[\\/]` — a single-letter server followed by a path separator is always a Windows drive prefix, never a real MCP resource. Legitimate MCP forms (`@server:resource/path`, plugin-scoped like `@asana-plugin:project-status/123`, inline prose mentions) remain matched and are pinned by the contract tests added in 04998d5. --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/components/PromptInput/PromptInput.tsx | 36 +++++++- src/utils/attachments.extractors.test.ts | 85 ++++++++++++++++++ src/utils/attachments.ts | 25 +++++- src/utils/dragDropPaths.test.ts | 100 +++++++++++++++++++++ src/utils/dragDropPaths.ts | 55 ++++++++++++ 5 files changed, 294 insertions(+), 7 deletions(-) create mode 100644 src/utils/attachments.extractors.test.ts create mode 100644 src/utils/dragDropPaths.test.ts create mode 100644 src/utils/dragDropPaths.ts diff --git a/src/components/PromptInput/PromptInput.tsx b/src/components/PromptInput/PromptInput.tsx index 27e6ff4d..45c16233 100644 --- a/src/components/PromptInput/PromptInput.tsx +++ b/src/components/PromptInput/PromptInput.tsx @@ -67,6 +67,7 @@ import { isBilledAsExtraUsage } from '../../utils/extraUsage.js'; import { getFastModeUnavailableReason, isFastModeAvailable, isFastModeCooldown, isFastModeEnabled, isFastModeSupportedByModel } from '../../utils/fastMode.js'; import { isFullscreenEnvEnabled } from '../../utils/fullscreen.js'; import type { PromptInputHelpers } from '../../utils/handlePromptSubmit.js'; +import { extractDraggedFilePaths } from '../../utils/dragDropPaths.js'; import { getImageFromClipboard, PASTE_THRESHOLD } from '../../utils/imagePaste.js'; import type { ImageDimensions } from '../../utils/imageResizer.js'; import { cacheImagePath, storeImage } from '../../utils/imageStore.js'; @@ -1204,6 +1205,22 @@ function PromptInput({ // Clean up pasted text - strip ANSI escape codes and normalize line endings and tabs let text = stripAnsi(rawText).replace(/\r/g, '\n').replaceAll('\t', ' '); + // Detect file paths from drag-and-drop and convert to @mentions. + // When files are dragged into the terminal, the terminal sends their + // absolute paths via bracketed paste. Image files are handled by the + // image paste handler upstream; here we handle non-image files by + // converting them to @mentions so they get attached on submit. + const draggedPaths = extractDraggedFilePaths(text); + if (draggedPaths.length > 0) { + const mentions = draggedPaths + .map(p => (p.includes(' ') || p.includes(':') ? `@"${p}"` : `@${p}`)) + .join(' '); + // Ensure spacing around the mention(s) relative to existing input + const charBefore = input[cursorOffset - 1]; + const prefix = charBefore && !/\s/.test(charBefore) ? ' ' : ''; + text = prefix + mentions + ' '; + } + // Match typed/auto-suggest: `!cmd` pasted into empty input enters bash mode. if (input.length === 0) { const pastedMode = getModeFromInput(text); @@ -1245,12 +1262,23 @@ function PromptInput({ if (isNonSpacePrintable(input, key)) return ' ' + input; return input; }, []); + // Ref mirrors cursorOffset for use in synchronous loops (e.g. multi-image + // paste) where React batches state updates and the closure value is stale. + const cursorOffsetRef = useRef(cursorOffset); + cursorOffsetRef.current = cursorOffset; + function insertTextAtCursor(text: string) { - // Push current state to buffer before inserting - pushToBuffer(input, cursorOffset, pastedContents); - const newInput = input.slice(0, cursorOffset) + text + input.slice(cursorOffset); + // Use refs for input/cursor so back-to-back calls in the same event + // (e.g. onImagePaste loop for multiple dragged images) chain correctly + // instead of each reading the same stale closure values. + const currentInput = lastInternalInputRef.current; + const currentOffset = cursorOffsetRef.current; + pushToBuffer(currentInput, currentOffset, pastedContents); + const newInput = currentInput.slice(0, currentOffset) + text + currentInput.slice(currentOffset); trackAndSetInput(newInput); - setCursorOffset(cursorOffset + text.length); + const newOffset = currentOffset + text.length; + cursorOffsetRef.current = newOffset; + setCursorOffset(newOffset); } const doublePressEscFromEmpty = useDoublePress(() => {}, () => onShowMessageSelector()); diff --git a/src/utils/attachments.extractors.test.ts b/src/utils/attachments.extractors.test.ts new file mode 100644 index 00000000..4011bb88 --- /dev/null +++ b/src/utils/attachments.extractors.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, test } from 'bun:test' +import { + extractAtMentionedFiles, + extractMcpResourceMentions, +} from './attachments.js' + +// Contract tests for the two @-mention extractors. +// +// Scope: the narrow contract between `extractAtMentionedFiles` and +// `extractMcpResourceMentions` where both are called on the same input +// and must not both claim the same token. The motivating bug is that +// `extractMcpResourceMentions`'s `\b` anchor lets it backtrack over the +// closing quote of a quoted file mention, producing a ghost match for +// `@"C:\Users\..."`. These tests pin the boundary so any regression in +// the MCP regex is caught immediately. +describe('extractor contract', () => { + describe('extractMcpResourceMentions must return empty for', () => { + const cases: Array<[string, string]> = [ + // Primary bug: the quoted form that PromptInput emits for Windows + // paths today. `\b` backtracks past the trailing `"` and produces + // a ghost MCP match on current HEAD. + ['a quoted Windows drive-letter path', '@"C:\\Users\\me\\file.txt"'], + // Even if the quote layer were stripped, a bare drive letter + // followed by a path separator is never an MCP resource. + ['an unquoted Windows drive-letter path', '@C:\\Users\\me\\file.txt'], + // Sanity: quoted POSIX paths with no `:` at all never matched the + // MCP regex and must keep not matching after the fix. + ['a quoted POSIX path with a space', '@"/Users/foo/my file.ts"'], + ['an unquoted POSIX path', '@/Users/foo/bar.ts'], + // Quoted POSIX path that embeds a `:` in the filename — the quote + // layer must shield it from MCP matching, same as the Windows case. + ['a quoted POSIX path with a colon in the name', '@"/tmp/weird:name.txt"'], + ] + test.each(cases)('%s', (_label, input) => { + expect(extractMcpResourceMentions(input)).toEqual([]) + }) + }) + + describe('extractMcpResourceMentions still matches legitimate MCP mentions', () => { + // Regression guard for the fix. If someone tightens the MCP regex + // too aggressively, these break and the intent is clear. + const cases: Array<[string, string, string[]]> = [ + [ + 'a simple server:resource token', + '@server:resource/path', + ['server:resource/path'], + ], + [ + 'a plugin-scoped server name with a dash', + '@asana-plugin:project-status/123', + ['asana-plugin:project-status/123'], + ], + [ + 'an MCP mention inline in prose', + 'please check @server:res here', + ['server:res'], + ], + ] + test.each(cases)('%s', (_label, input, expected) => { + expect(extractMcpResourceMentions(input)).toEqual(expected) + }) + }) + + describe('extractAtMentionedFiles extracts the file paths it should', () => { + // Asserted separately from the MCP side: the bug is purely in the + // MCP extractor over-matching, so these assertions are the + // "baseline still works" half of the contract. + const cases: Array<[string, string, string[]]> = [ + [ + 'a quoted Windows drive-letter path', + '@"C:\\Users\\me\\file.txt"', + ['C:\\Users\\me\\file.txt'], + ], + [ + 'a quoted POSIX path with a space', + '@"/Users/foo/my file.ts"', + ['/Users/foo/my file.ts'], + ], + ['an unquoted POSIX path', '@/Users/foo/bar.ts', ['/Users/foo/bar.ts']], + ] + test.each(cases)('%s', (_label, input, expected) => { + expect(extractAtMentionedFiles(input)).toEqual(expected) + }) + }) +}) diff --git a/src/utils/attachments.ts b/src/utils/attachments.ts index 36f5e57c..37b5eb65 100644 --- a/src/utils/attachments.ts +++ b/src/utils/attachments.ts @@ -2793,11 +2793,30 @@ export function extractAtMentionedFiles(content: string): string[] { export function extractMcpResourceMentions(content: string): string[] { // Extract MCP resources mentioned with @ symbol in format @server:uri // Example: "@server1:resource/path" would extract "server1:resource/path" - const atMentionRegex = /(^|\s)@([^\s]+:[^\s]+)\b/g + // + // Two guards against Windows-path / quoted-file collisions (see + // `attachments.extractors.test.ts`): + // + // 1. `(?!")` right after `@` drops quoted tokens entirely. The earlier + // form (without the lookahead and with `[^\s]` character classes) + // backtracked past the closing `"` at the `\b` anchor and produced + // ghost matches like `"C:\Users\...\file.txt` for any quoted file + // mention containing a colon. + // 2. The `"` added to the character classes is belt-and-braces: even + // if the lookahead were later removed or bypassed, the engine can + // no longer consume a quote character mid-match. + const atMentionRegex = /(^|\s)@(?!")([^\s"]+:[^\s"]+)\b/g const matches = content.match(atMentionRegex) || [] - // Remove the prefix (everything before @) from each match - return uniq(matches.map(match => match.slice(match.indexOf('@') + 1))) + return uniq( + matches + .map(match => match.slice(match.indexOf('@') + 1)) + // Post-match filter: a single-letter "server" followed by `:\` or + // `:/` is always a Windows drive-letter prefix, never a real MCP + // resource. This covers the unquoted `@C:\Users\...` case that + // the regex alone cannot disambiguate from `@server:resource`. + .filter(m => !/^[A-Za-z]:[\\/]/.test(m)), + ) } export function extractAgentMentions(content: string): string[] { diff --git a/src/utils/dragDropPaths.test.ts b/src/utils/dragDropPaths.test.ts new file mode 100644 index 00000000..f198f211 --- /dev/null +++ b/src/utils/dragDropPaths.test.ts @@ -0,0 +1,100 @@ +import { afterAll, describe, expect, test } from 'bun:test' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' +import { extractDraggedFilePaths } from './dragDropPaths.js' + +describe('extractDraggedFilePaths', () => { + // Paths that exist on any system. + const thisFile = import.meta.path + const packageJson = `${process.cwd()}/package.json` + + // Fixtures created synchronously at describe-load time (not in + // `beforeAll`) so their paths are available to `test.each` tables, + // which are built before any hook runs. + const tmpDir = mkdtempSync(join(tmpdir(), 'dragdrop-test-')) + const spacedFile = join(tmpDir, 'my file.txt') + writeFileSync(spacedFile, 'test') + const scopedDir = join(tmpDir, '@types') + mkdirSync(scopedDir) + const atSignFile = join(scopedDir, 'index.d.ts') + writeFileSync(atSignFile, 'test') + + afterAll(() => { + rmSync(tmpDir, { recursive: true, force: true }) + }) + + describe('returns an empty array', () => { + const emptyCases: Array<[string, string]> = [ + ['a non-absolute path', 'relative/path/file.ts'], + ['a plain image path', '/Users/foo/image.png'], + ['an uppercase image extension', '/Users/foo/SHOT.PNG'], + ['a double-quoted image path', '"/Users/foo/shot.png"'], + ['a single-quoted image path', "'/Users/foo/shot.jpg'"], + ['regular prose text', 'hello world this is text'], + ['a nonexistent absolute path', '/definitely/nonexistent/file.ts'], + ['a single-quoted nonexistent path', "'/definitely/nonexistent.ts'"], + ['an empty string', ''], + ['whitespace only', ' \n '], + // Mixed-segment cases: all-or-nothing policy means a single bad + // entry disqualifies the whole paste. + ['a mix where one path does not exist', `${thisFile}\n/nonexistent/file.ts`], + ['a mix where one segment is an image', `${thisFile}\n/Users/foo/shot.png`], + ] + test.each(emptyCases)('for %s', (_label, input) => { + expect(extractDraggedFilePaths(input)).toEqual([]) + }) + }) + + describe('resolves a single path', () => { + const singleCases: Array<[string, string, string]> = [ + ['a plain absolute path', thisFile, thisFile], + ['a double-quoted path', `"${thisFile}"`, thisFile], + ['a single-quoted path', `'${thisFile}'`, thisFile], + ['a path with leading/trailing whitespace', ` ${thisFile} `, thisFile], + // Realistic: dragging something under `node_modules/@types/...`. + // `@` inside the path must not collide with the mention prefix + // that the caller prepends downstream. + ['a path containing an `@` segment', atSignFile, atSignFile], + ] + test.each(singleCases)('from %s', (_label, input, expected) => { + expect(extractDraggedFilePaths(input)).toEqual([expected]) + }) + }) + + describe('resolves multiple paths', () => { + const multiCases: Array<[string, string, string[]]> = [ + [ + 'newline-separated', + `${thisFile}\n${packageJson}`, + [thisFile, packageJson], + ], + [ + 'space-separated (Finder drag)', + `${thisFile} ${packageJson}`, + [thisFile, packageJson], + ], + ] + test.each(multiCases)('when input is %s', (_label, input, expected) => { + expect(extractDraggedFilePaths(input)).toEqual(expected) + }) + }) + + // Backslash-escaped paths are a Finder/macOS + Linux convention — on + // Windows the shell-escape step is skipped, so these cases do not apply. + if (process.platform !== 'win32') { + describe('handles backslash-escaped paths', () => { + test('returns empty for an escaped image path', () => { + // The image check must apply after escape stripping so Finder + // image drags still route to the image paste handler. + expect(extractDraggedFilePaths('/Users/foo/my\\ shot.png')).toEqual([]) + }) + + test('resolves an escaped real file with a space in its name', () => { + // Raw form matches what a terminal delivers on Finder drag. + const escaped = spacedFile.replace(/ /g, '\\ ') + expect(extractDraggedFilePaths(escaped)).toEqual([spacedFile]) + }) + }) + } +}) diff --git a/src/utils/dragDropPaths.ts b/src/utils/dragDropPaths.ts new file mode 100644 index 00000000..a940efb7 --- /dev/null +++ b/src/utils/dragDropPaths.ts @@ -0,0 +1,55 @@ +import { existsSync } from 'fs' +import { isAbsolute } from 'path' + +// Inlined to avoid pulling the full `imagePaste.ts` module (which imports +// `bun:bundle`) into this file's dependency graph. Must stay in sync with +// `IMAGE_EXTENSION_REGEX` in `./imagePaste.ts`. +const IMAGE_EXTENSION_REGEX = /\.(png|jpe?g|gif|webp)$/i + +/** + * Detect absolute file paths in pasted text (typically from drag-and-drop). + * Returns the cleaned paths if ALL segments are existing non-image files, + * or an empty array otherwise. + * + * Splitting logic mirrors usePasteHandler: space preceding `/` or a Windows + * drive letter, plus newline separators. + */ +export function extractDraggedFilePaths(text: string): string[] { + const segments = text + .split(/ (?=\/|[A-Za-z]:\\)/) + .flatMap(part => part.split('\n')) + .map(s => s.trim()) + .filter(Boolean) + + if (segments.length === 0) return [] + + const cleaned: string[] = [] + + for (const raw of segments) { + // Strip outer quotes and shell-escape backslashes + let p = raw + if ( + (p.startsWith('"') && p.endsWith('"')) || + (p.startsWith("'") && p.endsWith("'")) + ) { + p = p.slice(1, -1) + } + if (process.platform !== 'win32') { + p = p.replace(/\\(.)/g, '$1') + } + + // Image files are handled by the upstream image paste handler. + // Check against the cleaned path so quoted/escaped image paths like + // `"/foo/shot.png"` or `/foo/my\ shot.png` are reliably excluded. + if (IMAGE_EXTENSION_REGEX.test(p)) return [] + if (!isAbsolute(p)) return [] + // Verify the path actually exists on disk. Plain `fs.existsSync` is + // used intentionally here instead of the wrapped `getFsImplementation` + // to keep this module free of the heavy `fsOperations` dependency + // chain — this is a pure existence check with no permission semantics. + if (!existsSync(p)) return [] + cleaned.push(p) + } + + return cleaned +}