Repository navigation
Filter blocked entries from list_directory tool output - #33
Conversation
Co-authored-by: kriserickson <325934+kriserickson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the list_directory tool implementation to avoid leaking blocked/sensitive filenames (e.g., .env, .ssh, *.key) in directory listings by switching from shelling out to native directory reads and applying per-entry filtering.
Changes:
- Replaced
execListDirectoryshell-out (ls -la/ PowerShell) withos.ReadDirand per-entryisBlockedfiltering. - Updated tests to remove command-stub failure coverage and add coverage asserting blocked entries are filtered from tool output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/tools/tools.go | Implements os.ReadDir-based listing and filters out blocked entries before formatting output. |
| internal/tools/tools_additional_test.go | Adds a blocked-entry filtering test and replaces the prior shell-command failure test with a non-existent directory case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| entryType = "l" | ||
| } | ||
|
|
||
| fmt.Fprintf(&b, "%s %10d %s %s\n", entryType+info.Mode().Perm().String(), size, info.ModTime().Format("Jan _2 15:04"), entry.Name()) |
There was a problem hiding this comment.
The permission/type string is being built as entryType + info.Mode().Perm().String(). Perm().String() already includes a leading type character (typically -), so this will produce malformed strings like --rw-r--r-- / d-rwx.... Consider using info.Mode().String() directly, or strip the leading char from the perm string before prefixing with entryType so the output format is consistent.
| fmt.Fprintf(&b, "%s %10d %s %s\n", entryType+info.Mode().Perm().String(), size, info.ModTime().Format("Jan _2 15:04"), entry.Name()) | |
| permStr := info.Mode().Perm().String() | |
| if len(permStr) > 0 { | |
| permStr = permStr[1:] | |
| } | |
| fmt.Fprintf(&b, "%s %10d %s %s\n", entryType+permStr, size, info.ModTime().Format("Jan _2 15:04"), entry.Name()) |
| // Entry may have been removed after ReadDir; skip it. | ||
| continue |
There was a problem hiding this comment.
entry.Info() failures are currently silently skipped with a comment implying the entry was removed. In practice Info() can also fail due to permission errors or filesystem issues, and skipping will make the listing incomplete without surfacing why. Consider either (a) returning an error when Info() fails for reasons other than os.IsNotExist, or (b) still emitting the entry with placeholder size/time/mode so users can see it exists.
| // Entry may have been removed after ReadDir; skip it. | |
| continue | |
| if os.IsNotExist(infoErr) { | |
| // Entry may have been removed after ReadDir; skip it. | |
| continue | |
| } | |
| return "", fmt.Errorf("list_directory failed to stat %q: %w", entry.Name(), infoErr) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bd9b68289
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| entries, err := os.ReadDir(absPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("list_directory failed: %s", string(out)) | ||
| return "", fmt.Errorf("list_directory failed: %w", err) |
There was a problem hiding this comment.
Accept regular-file paths in list_directory
Calling os.ReadDir(absPath) unconditionally now makes list_directory fail with not a directory whenever the provided path is a regular file, even though path validation allows file paths and the previous implementation returned a listing for them via ls -la/Get-ChildItem. This is a behavior regression that surfaces to users when they pass a file path (e.g., README.md) and now get a hard error instead of metadata output.
Useful? React with 👍 / 👎.
- Skip non-Windows tests in TestParentShellProcess_NonWindows and TestPreferredPowerShell_NonWindows. - Update TestValidatePath_Valid and TestValidatePath_OutsideCWD to use current working directory dynamically. - Enhance error messages in execCheckCommand for better clarity. - Set USERPROFILE environment variable in memory tests for consistency. - Adjust TestExecute_CheckCommand_NotFound to validate error handling for nonexistent commands.
…mprove test visibility
…ve error handling for outside paths
execListDirectoryvalidated the requested path but then shelled out tols -la/Get-ChildItem, which exposed blocked filenames (.env,.ssh/,*.key, etc.) in the output even when the directory itself was allowed.Changes
internal/tools/tools.go: Replace shell-out withos.ReadDir+ per-entryisBlockedfilter. Entries whose relative path cannot be computed are skipped as a fail-safe (avoid accidentally surfacing a file we can't verify).internal/tools/tools_additional_test.go: Replace thels-stub failure test with a non-existent directory test; addTestExecListDirectory_BlockedEntriesFilteredto assert.envis absent andsafe.txtis present in listing output.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.