fix: convert dragged file paths to @mentions for attachment#382
fix: convert dragged file paths to @mentions for attachment#382kevincodex1 merged 9 commits intoGitlawb:mainfrom
Conversation
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.
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.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head 2b3dd004d411e803f485ededc197139cb4a3e3ef.
I still can't approve this because there is one real Windows-path regression in the new @mention conversion:
-
src/components/PromptInput/PromptInput.tsxemits unquoted Windows absolute paths like@C:\Users\me\file.txtwhenever the dragged path has no spaces.
That overlaps with the existing MCP mention syntax, becauseattachments.tstreats any unquoted@name:resttoken as an MCP resource mention too.Direct repro on this head:
extractAtMentionedFiles('@C:\Users\me\file.txt')->['C:\Users\me\file.txt']extractMcpResourceMentions('@C:\Users\me\file.txt')->['C:\Users\me\file.txt']
So the new Windows drag/drop path format is currently ambiguous: it is treated as both a file mention and an MCP resource mention. The fix is to emit Windows absolute paths in quoted form too (for example whenever the path contains
:), not only when the path contains spaces.
Fresh verification on this head:
bun test src/utils/dragDropPaths.test.ts-> 12 passbun run build-> successbun run smoke-> success
The Unix-style drag/drop parsing and the build/smoke checks look fine, but I wouldn't merge this until the Windows mention format no longer collides with MCP resource parsing.
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) <noreply@anthropic.com>
|
@paulovitin ready for review ? |
Not yet! |
- 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.
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.
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.
|
ping me when you need review @paulovitin |
|
@Vasanthdev2004, it's good to me now. Your review is welcome |
| const draggedPaths = extractDraggedFilePaths(text); | ||
| if (draggedPaths.length > 0) { | ||
| const mentions = draggedPaths | ||
| .map(p => (p.includes(' ') || p.includes(':') ? `@"${p}"` : `@${p}`)) |
There was a problem hiding this comment.
This still does not avoid the MCP collision on Windows. On the current head, @\C:\Users\me\file.txt\ is still parsed by both extractors: extractAtMentionedFiles() returns C:\Users\me\file.txt, and extractMcpResourceMentions() returns the same string because its regex (/@([^\\s]+:[^\\s]+)\b/) still matches inside the quoted form. So quoting the Windows path here does not actually fix the ambiguity the previous review called out.
There was a problem hiding this comment.
You're right — I verified the collision empirically. The \b at the end of the MCP regex backtracks past the trailing " and produces a ghost match like "C:\Users\me\file.txt (leading quote included, trailing quote dropped), so the quoting in 55276b1 only hides the symptom.
I pushed the failing contract tests in 04998d5 so the bug is reproducible in the suite — 3 of the 11 cases fail on current HEAD, exactly at the boundary you described:
@"C:\Users\me\file.txt"— quoted Windows drive-letter path@C:\Users\me\file.txt— unquoted equivalent@"/tmp/weird:name.txt"— POSIX path with a:in the filename (same family)
My plan for the fix, before I push it — want your take:
- In
extractMcpResourceMentions, tighten the regex to/(^|\s)@(?!")([^\s"]+:[^\s"]+)\b/g. The negative lookahead(?!")drops quoted tokens entirely, and adding"to the character classes prevents any partial match via backtracking even if the lookahead were ever removed. - On top of that, a post-match filter that discards anything matching
/^[A-Za-z]:[\\/]/— a single-letter "server" followed by:\or:/is always a Windows drive-letter prefix, never a real MCP resource. This covers the unquoted case that regex alone can't disambiguate.
The legitimate MCP cases (@server:resource/path, plugin-scoped like @asana-plugin:project-status/123, inline prose mentions) stay green — the test file pins them explicitly so the fix can't over-correct.
Does this approach land where you'd want it, or would you rather see the disambiguation somewhere else (e.g. at the call site in attachments.ts by subtracting file mentions from MCP matches)? Happy to adjust.
There was a problem hiding this comment.
@Vasanthdev2004 pushed the fix in 2951eb0. Both guards from the plan landed:
extractMcpResourceMentionsregex is now/(^|\s)@(?!")([^\s"]+:[^\s"]+)\b/g— the(?!")drops quoted tokens outright, and"in the character classes blocks any residual backtracking across the closing quote.- Post-match filter strips anything matching
^[A-Za-z]:[\\/]— covers the unquoted@C:\Users\...case the regex alone can't disambiguate.
All 11 contract tests in attachments.extractors.test.ts are green, including the 3 that were failing on the previous head (@"C:\Users\me\file.txt", @C:\Users\me\file.txt, @"/tmp/weird:name.txt"). Full suite: 388/388. Legitimate MCP mentions (@server:resource/path, @asana-plugin:project-status/123, inline prose) stay matched — pinned explicitly so the fix can't over-correct.
Mind taking another look when you have a minute?
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head c2e32b6f37392786bfbaaa2664d468af74f830fa.
The newer cleanup in this branch does improve a few real edge cases:
- quoted and escaped image paths are now correctly excluded from file-mention conversion
- the drag/drop parser tests are broader and all pass on this head
- build and smoke still pass
But the previous Windows-path blocker is still present on the latest head, so I still can't approve it.
Current blocker:
-
Quoting Windows absolute paths in
PromptInputdoes NOT actually avoid the MCP resource collision.
Direct repro on this head:extractAtMentionedFiles('@C:\Users\me\file.txt')->['C:\Users\me\file.txt']extractMcpResourceMentions('@C:\Users\me\file.txt')->['C:\Users\me\file.txt']
So
@C:\...is still ambiguous: it is parsed as both a file mention and an MCP resource mention. The quoted form does not fix the underlying overlap becauseextractMcpResourceMentions()still matches the quoted token.
Fresh verification on this head:
- direct repro: quoted Windows path still matches both extractors
- direct repro: quoted image paths and escaped image paths now correctly return
[]fromextractDraggedFilePaths() bun test src/utils/dragDropPaths.test.ts-> 19 passbun run build-> successbun run smoke-> success
So the new parser hardening is good, but I still wouldn't merge it until Windows absolute-path mentions no longer collide with MCP mention parsing.
|
Clarification on my latest review: the exact current-head repro string is:
On
So the new quoting change does not yet eliminate the MCP overlap. |
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 Gitlawb#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.
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.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head 2951eb0487a65450ec8f886825df49197bb1cced.
The previous blocker is fixed on this revision.
What I verified on this head:
- the drag/drop mention formatter in
PromptInputnow quotes paths containing:as well as spaces, so Windows absolute paths and POSIX filenames containing:are routed through the quoted form - the MCP extractor now has two guards against the old collision:
- it skips quoted tokens entirely via
(?!") - it filters out
^[A-Za-z]:[\\/]so bare Windows drive-letter paths are not treated as MCP resources
- it skips quoted tokens entirely via
- direct repro on this head:
extractAtMentionedFiles('@C:\Users\me\file.txt')->['C:\Users\me\file.txt']extractMcpResourceMentions('@C:\Users\me\file.txt')->[]
- the new extractor contract tests pass:
bun test src/utils/attachments.extractors.test.ts --filter extractor-> 11 pass
- broader attachment/drag-drop checks also pass:
bun test src/utils/attachments.extractors.test.ts src/utils/dragDropPaths.test.ts-> 30 pass
bun run build-> successbun run smoke-> success
I didn't find a remaining blocker on the current head.
Residual risk is small and pre-existing rather than introduced here: legitimate MCP mentions like @server:resource/path are still also visible to the generic file-mention extractor, but the branch-specific Windows/quoted-path collision that blocked this PR is addressed.
) * 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) <noreply@anthropic.com> * 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 Gitlawb#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) <noreply@anthropic.com>
3 participants
Summary
@mentions, so the existing attachment system picks them up on submitImpact
.ts,.json,.txt, etc.) into the terminal now attaches them as@mentionsinstead of inserting raw paths as inert text. Image files are unaffected (already handled upstream by the image paste handler).src/utils/dragDropPaths.tswithextractDraggedFilePaths()— splits pasted text using the same logic asusePasteHandler, validates each segment is an absolute path to an existing non-image file, and returns cleaned paths. Called fromonTextPasteinPromptInput.tsx.Testing
bun run buildbun run smokebun test src/utils/dragDropPaths.test.ts(12 tests — single path, newline-separated, space-separated, quoted paths, shell-escaped paths, image exclusion, nonexistent files, empty input)bun testfull suite (368 pass, 0 fail).tsand.jsonfiles from Finder into the terminal — paths converted to@mentionscorrectlyNotes
@prefix)@"path"format. Files must exist on disk to be converted (prevents false positives on pasted text that happens to start with/).