Skip to content

Conversation

@roomote
Copy link
Contributor

@roomote roomote bot commented Sep 11, 2025

Summary

This PR fixes issue #7875 by restricting @-mention parsing to only work at line-start or after whitespace, preventing accidental loading of URLs/files from pasted logs.

Problem

When users paste logs containing patterns like @https://... or @/path mid-line (e.g., Error loading resource@https://api.example.com), the chat incorrectly treats them as mentions and attempts to load the content.

Solution

  • Updated the mentionRegex pattern to require the @ symbol to be either:
    • At the start of a line (^)
    • Preceded by whitespace ((?<=\s))
  • This prevents mentions from being recognized when they appear mid-word or after non-whitespace characters

Changes

  • Modified src/shared/context-mentions.ts to update the regex pattern
  • Added comprehensive test cases in src/shared/__tests__/context-mentions.spec.ts to verify:
    • Valid mentions still work (at line-start and after whitespace)
    • Invalid mentions are rejected (mid-word, after punctuation without space)
    • Pasted log scenarios are handled correctly

Screenshots:

Before:
image

After:
image

Testing

  • ✅ All existing tests pass (71 tests total)
  • ✅ Added new test cases specifically for boundary restrictions
  • ✅ Verified backward compatibility - all valid use cases continue to work
  • ✅ Linting and type checks pass

Backward Compatibility

This change maintains full backward compatibility:

  • All valid mentions (at line-start or after whitespace) continue to work
  • The escape mechanism (\@) is preserved
  • No breaking changes to existing functionality

Fixes #7875


Important

Restricts @-mention parsing to line-start or after whitespace to prevent incorrect parsing in pasted logs.

  • Behavior:
    • Restricts @-mention parsing to line-start or after whitespace in mentionRegex in context-mentions.ts.
    • Prevents incorrect parsing of URLs/files in pasted logs.
  • Testing:
    • Adds test cases in context-mentions.spec.ts for valid mentions (line-start, after whitespace) and invalid mentions (mid-word, after punctuation without space).
    • Verifies handling of pasted log scenarios.
  • Backward Compatibility:
    • Maintains existing functionality for valid mentions.
    • No breaking changes introduced.

This description was created by Ellipsis for 2f64e13. You can customize this summary. It will automatically update as commits are pushed.

- Updated mentionRegex to require @ symbol at start of line or after whitespace
- Prevents accidental mention loading from pasted logs containing @https:// or @/path mid-line
- Added comprehensive test cases for boundary restrictions
- Maintains backward compatibility for valid mentions while fixing the issue

Fixes #7875
Copy link
Contributor Author

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing my own code feels like debugging in a mirror maze - everything looks backward but somehow still broken.

*/
export const mentionRegex =
/(?<!\\)@((?:\/|\w+:\/\/)(?:[^\s\\]|\\ )+?|[a-f0-9]{7,40}\b|problems\b|git-changes\b|terminal\b)(?=[.,;:!?]?(?=[\s\r\n]|$))/
/(?:^|(?<=\s))(?<!\\)@((?:\/|\w+:\/\/)(?:[^\s\\]|\\ )+?|[a-f0-9]{7,40}\b|problems\b|git-changes\b|terminal\b)(?=[.,;:!?]?(?=[\s\r\n]|$))/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern has become quite complex with the addition of (?:^|(?<=\s)). Have you considered adding a helper function that validates mention boundaries separately? This could improve readability and make the logic easier to maintain.

export const mentionRegex =
/(?<!\\)@((?:\/|\w+:\/\/)(?:[^\s\\]|\\ )+?|[a-f0-9]{7,40}\b|problems\b|git-changes\b|terminal\b)(?=[.,;:!?]?(?=[\s\r\n]|$))/
/(?:^|(?<=\s))(?<!\\)@((?:\/|\w+:\/\/)(?:[^\s\\]|\\ )+?|[a-f0-9]{7,40}\b|problems\b|git-changes\b|terminal\b)(?=[.,;:!?]?(?=[\s\r\n]|$))/
export const mentionRegexGlobal = new RegExp(mentionRegex.source, "g")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that the regex doesn't use the multiline flag? Without it, ^ only matches the absolute start of the string. Mentions at the beginning of lines after newlines currently work because \n counts as whitespace in (?<=\s), but this might be worth clarifying in the documentation.

{ input: "URL: @https://example.com.", expected: ["@https://example.com"] }, // After colon and space
{ input: "Commit @a1b2c3d, then check @/file.txt", expected: ["@a1b2c3d", "@/file.txt"] },

// NEW: Test cases for mentions mid-line without whitespace (should NOT match)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a test case for mentions after punctuation with a space? For example, 'Hello! @problems' should work but 'Hello!@problems' shouldn't. This would explicitly document the expected behavior for this edge case.

- The exact word 'git-changes'.
- The exact word 'terminal'.
- It ensures that any trailing punctuation marks (such as ',', '.', '!', etc.) are not included in the matched mention, allowing the punctuation to follow the mention naturally in the text.
- **NEW**: The @ symbol must be at the start of a line or preceded by whitespace to prevent accidental matches in pasted logs.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding examples of valid vs invalid patterns in the comments to make it immediately clear for future developers. For instance, showing that '@/path/to/file.txt' at line start is valid, 'Check @problems here' after space is valid, but 'error@https://api.com' mid-word is invalid.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 11, 2025
@hannesrudolph hannesrudolph moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Sep 11, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Sep 11, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 11, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Sep 11, 2025
@mrubens mrubens merged commit 03709fd into main Sep 11, 2025
29 checks passed
@mrubens mrubens deleted the fix/restrict-mention-parsing-boundaries branch September 11, 2025 23:37
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Sep 11, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:L This PR changes 100-499 lines, ignoring generated files.