feat: add support for symbolic links in .roo/commands directory #7284
Conversation
- Modified scanCommandDirectory to check for both regular files and symbolic links - Added comprehensive test coverage for symbolic link functionality - Ensures symbolic links to markdown files are properly loaded as commands Fixes #7282
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
src/services/command/commands.ts
Outdated
| for (const entry of entries) { | ||
| if (entry.isFile() && isMarkdownFile(entry.name)) { | ||
| // Check for both regular files and symbolic links | ||
| if ((entry.isFile() || entry.isSymbolicLink()) && isMarkdownFile(entry.name)) { |
There was a problem hiding this comment.
Is this approach intentional? I notice we're explicitly checking for symbolic links here in scanCommandDirectory, but the tryLoadCommand function (lines 54-112) doesn't have this explicit check. While fs.readFile follows symlinks automatically, this creates an inconsistency in how we handle symlinks across the codebase. Should we add a similar check in tryLoadCommand for consistency?
src/services/command/commands.ts
Outdated
| for (const entry of entries) { | ||
| if (entry.isFile() && isMarkdownFile(entry.name)) { | ||
| // Check for both regular files and symbolic links | ||
| if ((entry.isFile() || entry.isSymbolicLink()) && isMarkdownFile(entry.name)) { |
There was a problem hiding this comment.
Security consideration: Symbolic links can point to files outside the commands directory. While this flexibility might be useful, could it potentially allow access to sensitive files if a malicious symlink is placed in the commands directory? Should we consider validating that symlinks resolve to paths within expected boundaries?
src/services/command/commands.ts
Outdated
| for (const entry of entries) { | ||
| if (entry.isFile() && isMarkdownFile(entry.name)) { | ||
| // Check for both regular files and symbolic links | ||
| if ((entry.isFile() || entry.isSymbolicLink()) && isMarkdownFile(entry.name)) { |
There was a problem hiding this comment.
Consider adding a comment here explaining that symbolic links are now supported. Something like:
| if ((entry.isFile() || entry.isSymbolicLink()) && isMarkdownFile(entry.name)) { | |
| // Check for both regular files and symbolic links to support command aliasing | |
| if ((entry.isFile() || entry.isSymbolicLink()) && isMarkdownFile(entry.name)) { |
This would help future maintainers understand the design decision.
|
|
||
| // Mock fs and path modules | ||
| vi.mock("fs/promises") | ||
| vi.mock("../../roo-config", () => ({ |
There was a problem hiding this comment.
The mock path here uses ../../roo-config while the existing test file frontmatter-commands.spec.ts uses ../roo-config. Should we make this consistent?
| vi.mock("../../roo-config", () => ({ | |
| vi.mock("../roo-config", () => ({ |
| }) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Great test coverage! This test suite thoroughly covers the symbolic link functionality including edge cases like broken links. Consider also adding a test that explicitly verifies tryLoadCommand works with symbolic links (even though it should work via fs.readFile).
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
- Check if symbolic links point to files before processing - Prevent attempting to read directories as files - Add test coverage for symlinks pointing to directories
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
|
Closing due to security concerns - symbolic links could potentially allow reading files outside the workspace without proper permission grants. This needs security review before implementation. |
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
This PR attempts to address Issue #7282 by adding support for symbolic links in the
.roo/commandsdirectory.Problem
Users were unable to use symbolic links to markdown files in the
.roo/commandsdirectory. When a slash command markdown file was copied as a symbolic link, Roo Code would not recognize it as a valid command.Solution
Modified the
scanCommandDirectoryfunction insrc/services/command/commands.tsto check for both regular files and symbolic links usingentry.isSymbolicLink()in addition toentry.isFile().Changes
Testing
symlink-commands.spec.tswith 5 test casesFixes #7282
Feedback and guidance are welcome!
Important
Adds support for symbolic links in
.roo/commandsby updatingscanCommandDirectoryto recognize them as valid command files, with comprehensive test coverage.scanCommandDirectoryincommands.tsnow checks for symbolic links usingentry.isSymbolicLink()in addition toentry.isFile().symlink-commands.spec.tswith 5 test cases covering various scenarios of symbolic link handling.This description was created by
for 90e983e. You can customize this summary. It will automatically update as commits are pushed.