Skip to content

fix(tools): list_files returns paths relative to queried directory — closes #24 #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jpleva91 wants to merge 1 commit into
base: main
Choose a base branch
from
+107 −1
Open

fix(tools): list_files returns paths relative to queried directory — closes #24 #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion internal/tools/tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,10 @@ return fmt.Errorf("limit reached")
if ext != "" && filepath.Ext(name) != ext {
return nil
}
rel, _ := filepath.Rel(".", path)
rel, _ := filepath.Rel(dir, path)
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

filepath.Rel(dir, path) errors are currently ignored. If Rel fails (e.g., different volume on Windows, malformed input), rel becomes an empty string and the code may append "" or "/" to the output, producing incorrect paths. Handle the error explicitly (e.g., return it to fail the tool call, or skip the entry) before using rel.

Suggested change
rel, _ := filepath.Rel(dir, path)
rel, errRel := filepath.Rel(dir, path)
if errRel != nil {
return errRel
}

Copilot uses AI. Check for mistakes.
if rel == "." {
return nil // skip the root directory entry itself
}
if d.IsDir() {
files = append(files, rel+"/")
} else {
Expand Down
103 changes: 103 additions & 0 deletions internal/tools/tools_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,109 @@ func TestGrep_RecursiveSearch(t *testing.T) {
}
}

// ── listFiles tests ──

func TestListFiles_PathsRelativeToDirectory(t *testing.T) {
dir := t.TempDir()
sub := filepath.Join(dir, "sub")
os.MkdirAll(sub, 0o755)
os.WriteFile(filepath.Join(dir, "a.go"), []byte(""), 0o644)
os.WriteFile(filepath.Join(sub, "b.go"), []byte(""), 0o644)

r := listFiles(map[string]string{"directory": dir}, 0)

if !r.Success {
t.Fatalf("expected success, got error: %s", r.Error)
}
// Paths must be relative to dir, not cwd
for _, line := range strings.Split(r.Output, "\n") {
if strings.HasPrefix(line, dir) {
t.Fatalf("got absolute path %q — expected relative to listed dir", line)
}
Comment on lines +432 to +436
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

TestListFiles_PathsRelativeToDirectory only checks that output lines aren’t absolute and don’t start with the temp dir path. With the original bug, entries like "../../tmp/.../a.go" are still relative and would pass these assertions. To actually verify paths are relative to the queried directory, assert that returned paths don’t start with ".." (after filepath.Clean) / don’t contain parent traversals, or add a direct expectation like "sub/b.go" vs any prefixed path.

Suggested change
// Paths must be relative to dir, not cwd
for _, line := range strings.Split(r.Output, "\n") {
if strings.HasPrefix(line, dir) {
t.Fatalf("got absolute path %q — expected relative to listed dir", line)
}
// Paths must be relative to dir, not cwd, and must not contain parent traversals.
for _, raw := range strings.Split(r.Output, "\n") {
line := strings.TrimSpace(raw)
if line == "" {
continue
}
if strings.HasPrefix(line, dir) {
t.Fatalf("got absolute path %q — expected relative to listed dir", line)
}
clean := filepath.Clean(line)
if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) {
t.Fatalf("got path with parent traversal %q (clean: %q) — expected path relative to listed dir", line, clean)
}

Copilot uses AI. Check for mistakes.
}
if !strings.Contains(r.Output, "a.go") {
t.Fatalf("expected a.go in output, got: %s", r.Output)
}
if !strings.Contains(r.Output, "sub/") || !strings.Contains(r.Output, "b.go") {
t.Fatalf("expected sub/ and b.go in output, got: %s", r.Output)
}
}

func TestListFiles_NoDoublePrefix(t *testing.T) {
// Regression test: when dir is a subdirectory, paths must not include the
// directory prefix (e.g. listing "internal/tools" must return "tools.go"
// not "internal/tools/tools.go").
dir := t.TempDir()
sub := filepath.Join(dir, "internal", "tools")
os.MkdirAll(sub, 0o755)
os.WriteFile(filepath.Join(sub, "tools.go"), []byte(""), 0o644)

r := listFiles(map[string]string{"directory": sub}, 0)

if !r.Success {
t.Fatalf("expected success, got error: %s", r.Error)
}
if !strings.Contains(r.Output, "tools.go") {
t.Fatalf("expected tools.go, got: %s", r.Output)
}
// Must NOT contain the sub-path prefix
if strings.Contains(r.Output, "internal") {
t.Fatalf("path should be relative to queried dir, not contain parent: %s", r.Output)
}
}

func TestListFiles_ExtensionFilter(t *testing.T) {
dir := t.TempDir()
os.WriteFile(filepath.Join(dir, "a.go"), []byte(""), 0o644)
os.WriteFile(filepath.Join(dir, "b.txt"), []byte(""), 0o644)

r := listFiles(map[string]string{"directory": dir, "extension": ".go"}, 0)

if !r.Success {
t.Fatalf("expected success, got error: %s", r.Error)
}
if !strings.Contains(r.Output, "a.go") {
t.Fatalf("expected a.go, got: %s", r.Output)
}
if strings.Contains(r.Output, "b.txt") {
t.Fatalf("should not include b.txt, got: %s", r.Output)
}
}

func TestListFiles_EmptyDir(t *testing.T) {
dir := t.TempDir()

r := listFiles(map[string]string{"directory": dir}, 0)

if !r.Success {
t.Fatalf("expected success, got error: %s", r.Error)
}
if r.Output != "(empty directory)" {
t.Fatalf("expected '(empty directory)', got: %s", r.Output)
}
}

func TestListFiles_SkipsHiddenAndNodeModules(t *testing.T) {
dir := t.TempDir()
os.MkdirAll(filepath.Join(dir, ".git"), 0o755)
os.MkdirAll(filepath.Join(dir, "node_modules"), 0o755)
os.WriteFile(filepath.Join(dir, ".git", "config"), []byte(""), 0o644)
os.WriteFile(filepath.Join(dir, "node_modules", "pkg.js"), []byte(""), 0o644)
os.WriteFile(filepath.Join(dir, "main.go"), []byte(""), 0o644)

r := listFiles(map[string]string{"directory": dir}, 0)

if !r.Success {
t.Fatalf("expected success, got error: %s", r.Error)
}
if strings.Contains(r.Output, ".git") || strings.Contains(r.Output, "node_modules") {
t.Fatalf("should skip hidden dirs, got: %s", r.Output)
}
if !strings.Contains(r.Output, "main.go") {
t.Fatalf("expected main.go, got: %s", r.Output)
}
}

// ── ExecuteDirect dispatch tests ──

func TestExecuteDirect_UnknownTool(t *testing.T) {
Expand Down
Loading