fix: return not-found for missing directories#454
Open
Showiix wants to merge 4 commits into
Open
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 3/8 reviews remaining, refill in 30 minutes and 9 seconds.Comment |
codeant-ai
Bot
added
the
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test-list-directory-errors.js (1)
9-21: Use a unique temp directory per run to avoid cross-test collisions.Using a fixed
TEST_ROOTcan make parallel or retried runs interfere with each other.Suggested refactor
-const TEST_ROOT = path.join(os.tmpdir(), 'desktop-commander-list-directory-errors'); +const TEST_ROOT_PREFIX = path.join(os.tmpdir(), 'desktop-commander-list-directory-errors-'); async function setup() { const originalConfig = await configManager.getConfig(); - await fs.mkdir(TEST_ROOT, { recursive: true }); - await configManager.setValue('allowedDirectories', [TEST_ROOT]); - return originalConfig; + const testRoot = await fs.mkdtemp(TEST_ROOT_PREFIX); + await configManager.setValue('allowedDirectories', [testRoot]); + return { originalConfig, testRoot }; } -async function teardown(originalConfig) { +async function teardown(originalConfig, testRoot) { await configManager.updateConfig(originalConfig); - await fs.rm(TEST_ROOT, { recursive: true, force: true }); + await fs.rm(testRoot, { recursive: true, force: true }); } -async function testMissingDirectoryReturnsNotFound() { - const missingDir = path.join(TEST_ROOT, 'missing-dir'); +async function testMissingDirectoryReturnsNotFound(testRoot) { + const missingDir = path.join(testRoot, 'missing-dir'); @@ export default async function runTests() { - let originalConfig; + let originalConfig; + let testRoot; try { - originalConfig = await setup(); - await testMissingDirectoryReturnsNotFound(); + ({ originalConfig, testRoot } = await setup()); + await testMissingDirectoryReturnsNotFound(testRoot); @@ } finally { - if (originalConfig) { - await teardown(originalConfig); + if (originalConfig && testRoot) { + await teardown(originalConfig, testRoot); } } }Also applies to: 40-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-list-directory-errors.js` around lines 9 - 21, TEST_ROOT is a fixed path causing cross-test collisions; change the setup/teardown to create a unique temp directory per run (use a per-run temp name) instead of the constant TEST_ROOT: generate the temp dir inside setup (e.g., via fs.mkdtemp or appending a UUID/timestamp) and store that path in a variable accessible to teardown, update configManager.setValue('allowedDirectories', [thatTempPath]) in setup, and have teardown call configManager.updateConfig(originalConfig) and remove that same temp path; update references to TEST_ROOT in the file to use the per-run temp variable (affecting the setup(), teardown(), and any tests using TEST_ROOT).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/test-list-directory-errors.js`:
- Around line 9-21: TEST_ROOT is a fixed path causing cross-test collisions;
change the setup/teardown to create a unique temp directory per run (use a
per-run temp name) instead of the constant TEST_ROOT: generate the temp dir
inside setup (e.g., via fs.mkdtemp or appending a UUID/timestamp) and store that
path in a variable accessible to teardown, update
configManager.setValue('allowedDirectories', [thatTempPath]) in setup, and have
teardown call configManager.updateConfig(originalConfig) and remove that same
temp path; update references to TEST_ROOT in the file to use the per-run temp
variable (affecting the setup(), teardown(), and any tests using TEST_ROOT).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe7dbf73-b191-4908-8bc1-273e3bdf3c9f
📒 Files selected for processing (2)
src/tools/filesystem.tstest/test-list-directory-errors.js
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+680
to
+691
| try { | ||
| const stats = await fs.stat(validPath); | ||
| if (!stats.isDirectory()) { | ||
| throw new Error(`Path is not a directory: ${dirPath}`); | ||
| } | ||
| } catch (error) { | ||
| const err = error as NodeJS.ErrnoException; | ||
| if (err.code === 'ENOENT' || err.code === 'ENOTDIR') { | ||
| throw new Error(`Directory not found: ${dirPath}`); | ||
| } | ||
| throw error; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: The new top-level fs.stat pre-check changes error behavior for inaccessible directories: if stat fails with EACCES/EPERM/ETIMEDOUT, the function now throws immediately instead of returning the existing [DENIED] ... entry format. This breaks the established list-directory contract used by the UI/parser for access-denied cases. Handle these permission/timeout errors in this block the same way as the readdir catch (append [DENIED] and return), or remove this pre-check and rely on the existing readdir handling. [logic error]
Severity Level: Major ⚠️
- ⚠️ list_directory tool no longer returns [DENIED] for permissions.
- ⚠️ File preview UI can't mark denied root directories correctly.
- ⚠️ Error responses diverge from documented list_directory behavior.Steps of Reproduction ✅
1. Build the project so the runtime JS artifacts exist (as in the PR test instructions,
run `npm run build`) and note that the list_directory tool is wired up via
`handleListDirectory` in `src/handlers/filesystem-handlers.ts:325-331`, which calls
`listDirectory(parsed.path, parsed.depth)` and wraps any thrown error with
`createErrorResponse` from `src/error-handlers.ts:9-16`.
2. In a Node script or REPL, mirror the setup used in
`test/test-list-directory-errors.js:6-15`: import `configManager` from
`dist/config-manager.js` and `listDirectory` from `dist/tools/filesystem.js`, set
`allowedDirectories` to a temporary root (e.g.
`/tmp/desktop-commander-list-directory-errors`), create a subdirectory `denied-root` under
this root, then remove execute/search permissions on it (e.g. `await fs.mkdir(deniedDir);
await fs.chmod(deniedDir, 0);`) so that `fs.stat(deniedDir)` fails with `EACCES`/`EPERM`
on your platform.
3. Call `await listDirectory(deniedDir, 1)` (same function under test in
`test/test-list-directory-errors.js:26-31`): inside `src/tools/filesystem.ts:674-681`,
`listDirectory` validates the path then hits the new pre-check `fs.stat(validPath)` at
lines `680-683`; because the directory is not stat-able, the `catch` block at `684-691`
sees an `EACCES`/`EPERM` error, does not match `ENOENT`/`ENOTDIR`, and rethrows the raw
error instead of producing a `[DENIED]` entry.
4. Observe that the promise from `listDirectory` rejects with a generic `EACCES`/`EPERM`
error and no `[DENIED]`-prefixed line, so the recursive `fs.readdir` call and its error
handler at `src/tools/filesystem.ts:48-63` (which pushes `[DENIED] ${displayPath}` and is
explicitly documented with "Keep [DENIED] prefix so UI parser regex still matches.") is
never reached; when invoked through the tool path (`handleListDirectory` in
`src/handlers/filesystem-handlers.ts:325-331` and tool metadata in `src/server.ts:37-69`),
this means the directory tool returns an `Error: ... permission denied` response instead
of a listing containing `[DENIED]`, breaking the contract relied on by
`parseDirectoryEntries` in `src/ui/file-preview/src/directory-controller.ts:55-58` and the
documented behavior in `src/server.ts:63-64` ("If a directory cannot be accessed, it will
show [DENIED] instead.").Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/filesystem.ts
**Line:** 680:691
**Comment:**
*Logic Error: The new top-level `fs.stat` pre-check changes error behavior for inaccessible directories: if `stat` fails with `EACCES`/`EPERM`/`ETIMEDOUT`, the function now throws immediately instead of returning the existing `[DENIED] ...` entry format. This breaks the established list-directory contract used by the UI/parser for access-denied cases. Handle these permission/timeout errors in this block the same way as the `readdir` catch (append `[DENIED]` and return), or remove this pre-check and rely on the existing `readdir` handling.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
CodeAnt AI finished reviewing your PR. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/filesystem.ts`:
- Around line 694-700: The code throws "Path is not a directory: ${dirPath}"
when stats.isDirectory() is false, which leaks a different error shape; change
that throw to produce the standardized message "Directory not found: ${dirPath}"
(or create an Error with code 'ENOTDIR' and that message) so non-directory
targets are mapped to the same contract; locate the check using
stats.isDirectory() and the dirPath variable and replace the throw accordingly
(or assign err.code = 'ENOTDIR' before throwing) so the catch mapping remains
consistent.
- Around line 682-689: The change in addDeniedEntry() mutates the core `[DENIED]
<path>` payload and can break downstream parsers; restore the original
results.push(`[DENIED] ${displayPath}`) for both branches to keep the path-only
entry, and if isAccessDeniedError(err) is true, append a second, non-breaking
informational entry (e.g. results.push(`[DENIED_INFO] ${displayPath} — not
accessible (permission denied, cloud-only file, or Full Disk Access not
granted)`)) so you preserve compatibility while still surfacing the hint; update
only addDeniedEntry and reference results and isAccessDeniedError accordingly.
- Around line 703-704: The call to addDeniedEntry uses path.basename(validPath)
which can be empty for root paths, producing a blank denied label; change the
label passed to addDeniedEntry so if path.basename(validPath) is falsy/empty you
fall back to a non-empty representation (e.g., validPath or a normalized/root
indicator) — update the code around the addDeniedEntry invocation (referencing
addDeniedEntry, path.basename(validPath), and validPath) to compute label =
path.basename(validPath) || validPath (or an equivalent normalized fallback)
before calling addDeniedEntry, then return results as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7a0d9b7-8cd9-4a86-9e29-77a1869a8a85
📒 Files selected for processing (2)
src/tools/filesystem.tstest/test-list-directory-errors.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test-list-directory-errors.js
Comment on lines
+682
to
+689
| function addDeniedEntry(displayPath: string, err: NodeJS.ErrnoException): void { | ||
| // Keep [DENIED] prefix so UI parser regex still matches. | ||
| // Append a hint for permission/timeout errors so user gets context. | ||
| if (isAccessDeniedError(err)) { | ||
| results.push(`[DENIED] ${displayPath} — not accessible (permission denied, cloud-only file, or Full Disk Access not granted)`); | ||
| } else { | ||
| results.push(`[DENIED] ${displayPath}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep [DENIED] entries path-only to preserve downstream compatibility.
Line 686 changes denied entries from [DENIED] <path> to [DENIED] <path> — .... That mutates the path payload and can break consumers that parse the post-prefix content as a path (including UI tree parsing). It also conflicts with the stated goal of preserving existing denied output format.
Suggested fix
function addDeniedEntry(displayPath: string, err: NodeJS.ErrnoException): void {
- // Keep [DENIED] prefix so UI parser regex still matches.
- // Append a hint for permission/timeout errors so user gets context.
- if (isAccessDeniedError(err)) {
- results.push(`[DENIED] ${displayPath} — not accessible (permission denied, cloud-only file, or Full Disk Access not granted)`);
- } else {
- results.push(`[DENIED] ${displayPath}`);
- }
+ results.push(`[DENIED] ${displayPath}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/filesystem.ts` around lines 682 - 689, The change in
addDeniedEntry() mutates the core `[DENIED] <path>` payload and can break
downstream parsers; restore the original results.push(`[DENIED] ${displayPath}`)
for both branches to keep the path-only entry, and if isAccessDeniedError(err)
is true, append a second, non-breaking informational entry (e.g.
results.push(`[DENIED_INFO] ${displayPath} — not accessible (permission denied,
cloud-only file, or Full Disk Access not granted)`)) so you preserve
compatibility while still surfacing the hint; update only addDeniedEntry and
reference results and isAccessDeniedError accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Fixes #396
Test
CodeAnt-AI Description
Show a not-found error when a directory does not exist
What Changed
Directory not founderror instead of being treated like access denied.[DENIED].Impact
✅ Clearer missing-folder errors✅ Fewer false permission-denied messages✅ Safer directory checks🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Bug Fixes
Tests