diff --git a/go/adk/pkg/tools/grep.go b/go/adk/pkg/tools/grep.go new file mode 100644 index 000000000..10f864559 --- /dev/null +++ b/go/adk/pkg/tools/grep.go @@ -0,0 +1,197 @@ +package tools + +import ( + "context" + "fmt" + "io/fs" + "os" + "path/filepath" + "regexp" + "strings" +) + +// WithinRoot reports whether resolved (an already symlink-resolved path) is +// root itself or nested under it. Callers are responsible for resolving +// symlinks on both arguments first; this is a pure path-containment check. +// +// It lives here with classifyWalkEntry, its main caller, but is also the +// containment test behind resolveSandboxedPath in skills.go. +func WithinRoot(resolved, root string) bool { + resolved = filepath.Clean(resolved) + root = filepath.Clean(root) + return resolved == root || strings.HasPrefix(resolved, root+string(filepath.Separator)) +} + +type walkEntryAction int + +const ( + // walkEntryGrep: a regular, in-bounds file that should be grepped. + walkEntryGrep walkEntryAction = iota + // walkEntrySkip: silently excluded by policy, not a read failure -- a + // directory, a symlinked directory, a non-regular file (FIFO/socket/ + // device), or a symlink whose target escapes the search root. + walkEntrySkip + // walkEntryUnreadable: a genuine read/stat failure on this entry. + walkEntryUnreadable +) + +// classifyWalkEntry decides how GrepContent's WalkDir callback should treat +// p, given the resolved search root. It never opens p for reading -- the +// caller is responsible for that once this returns walkEntryGrep, and must +// read the returned resolved path rather than p itself: p is the symlink +// as walked, and re-resolving or reopening it separately from this check +// would reintroduce a TOCTOU window where the symlink's target could change +// between the check and the read. +// +// This narrows that window but doesn't eliminate it: resolved is still a +// path string, so the later os.Open on it is a fresh lookup, not an +// operation on a captured file handle. A path component of resolved could +// still be swapped out between this check and that open. Closing that +// residual race fully would need platform-specific work (e.g. Linux's +// openat2 with RESOLVE_NO_SYMLINKS), which isn't done anywhere else in this +// file either -- this function only guarantees "reads what was just +// verified", not "reads atomically with no concurrent tampering". +func classifyWalkEntry(root, p string, d fs.DirEntry) (walkEntryAction, string) { + if d.IsDir() { + return walkEntrySkip, "" + } + resolved, err := filepath.EvalSymlinks(p) + if err != nil { + return walkEntryUnreadable, "" + } + fi, statErr := os.Stat(resolved) + if statErr != nil { + return walkEntryUnreadable, "" + } + if fi.IsDir() { + // p is a symlink to a directory: WalkDir doesn't recurse into + // symlinked directories, and grepFile would fail trying to read + // one as a file, so skip it rather than treating it as an error. + return walkEntrySkip, "" + } + if !fi.Mode().IsRegular() { + // Skip non-regular files (FIFOs, sockets, devices): opening one + // for reading can block indefinitely (e.g. a FIFO with no writer + // connected), and grep has no business reading them. + return walkEntrySkip, "" + } + if !WithinRoot(resolved, root) { + // The symlink-resolved target escapes the root being searched, so + // a symlink can't be used to read files outside the requested + // directory. + return walkEntrySkip, "" + } + return walkEntryGrep, resolved +} + +// GrepContent searches path for lines matching a regular expression pattern. +// If path is a directory, recursive must be true to search its files. +// +// A recursive search walks the whole tree under path, which is unbounded from +// this function's point of view, so ctx is honored between entries and lets a +// caller abort one. Note Go needs no equivalent of the Python runtime's regex +// timeout: this uses RE2, which is linear-time and cannot backtrack +// catastrophically, so it is the walk rather than the match that can run long. +func GrepContent(ctx context.Context, path, pattern string, recursive, ignoreCase bool) (string, error) { + expr := pattern + if ignoreCase { + expr = "(?i)" + expr + } + re, err := regexp.Compile(expr) + if err != nil { + return "", fmt.Errorf("invalid pattern: %w", err) + } + + info, err := os.Stat(path) + if err != nil { + return "", fmt.Errorf("failed to stat %q: %w", path, err) + } + + var result strings.Builder + var skipped int + grepFile := func(filePath string) error { + return scanFileLines(filePath, func(lineNum int, line string) bool { + if re.MatchString(line) { + fmt.Fprintf(&result, "%s:%d:%s\n", filePath, lineNum, truncateRunes(line, maxLineRunes)) + } + return true + }) + } + + if info.IsDir() { + if !recursive { + return "", fmt.Errorf("%q is a directory; set recursive=true to search directories", path) + } + // Reuse the outer err (rather than := , which would shadow it in this + // block) so a WalkDir failure below is actually observed by the + // err != nil check after this if/else. + var root string + root, err = filepath.EvalSymlinks(path) + if err != nil { + return "", fmt.Errorf("failed to resolve %q: %w", path, err) + } + // Walk the resolved root, not path: filepath.WalkDir uses Lstat on + // its root argument, so if path itself were an unresolved directory + // symlink, WalkDir would see a non-directory at the root and never + // descend into it at all. + err = filepath.WalkDir(root, func(p string, d fs.DirEntry, walkErr error) error { + // Checked per entry rather than per line: a single file's scan is + // bounded by its size, but the number of entries is not. + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } + if walkErr != nil { + if p == root { + // The search root itself couldn't be read (e.g. + // permission denied): the search never actually ran, so + // surface a real error instead of a misleadingly + // confident "no matches found". + return walkErr + } + // WalkDir surfaces ReadDir/Lstat failures (e.g. a + // permission-denied subdirectory) through this err rather + // than via grepFile, but it deserves the same treatment: one + // unreadable subtree shouldn't discard matches already found + // in its siblings. + skipped++ + return nil + } + switch action, resolved := classifyWalkEntry(root, p, d); action { + case walkEntryUnreadable: + skipped++ + case walkEntryGrep: + // A read error on one file (permission denied, a line + // exceeding the scan buffer, etc.) shouldn't abort matches + // already found elsewhere in the tree. + if grepErr := grepFile(resolved); grepErr != nil { + skipped++ + } + } + return nil + }) + } else { + // scanFileLines rejects non-regular files too, but reusing the stat + // already taken above lets an explicitly-targeted FIFO report the + // plain reason rather than one wrapped in "failed to search". + if !info.Mode().IsRegular() { + return "", fmt.Errorf("%q is not a regular file", path) + } + err = grepFile(path) + } + if err != nil { + return "", fmt.Errorf("failed to search %q: %w", path, err) + } + + if result.Len() == 0 { + if skipped > 0 { + return fmt.Sprintf("no matches found (%d entries could not be read)", skipped), nil + } + return "no matches found", nil + } + + matches := strings.TrimSuffix(result.String(), "\n") + if skipped > 0 { + matches += fmt.Sprintf("\n\n(%d entries could not be read)", skipped) + } + return matches, nil +} diff --git a/go/adk/pkg/tools/grep_test.go b/go/adk/pkg/tools/grep_test.go new file mode 100644 index 000000000..e42c7dcf0 --- /dev/null +++ b/go/adk/pkg/tools/grep_test.go @@ -0,0 +1,571 @@ +package tools + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" +) + +// TestClassifyWalkEntry pins the three-way split directly rather than only +// through TestGrepContent's integration assertions, so a change to the +// classifier fails here with the specific case that broke. +// +// It is the deliberate mirror of Python's +// test_classify_walk_entry_covers_each_outcome (kagent-skills, shell.py): +// same cases, same order, same expected outcomes. The two runtimes must sort +// a tree identically, and reading these two tests side by side is how that +// stays true. Prefer changing both, or neither. +// +// The directory case has no Python counterpart on purpose -- filepath.WalkDir +// hands this function directories, while Python's os.walk yields only +// filenames, so only Go can reach it. +func TestClassifyWalkEntry(t *testing.T) { + root := createTempDir(t) + defer os.RemoveAll(root) + + plain := filepath.Join(root, "plain.txt") + if err := os.WriteFile(plain, []byte("hello\n"), 0644); err != nil { + t.Fatalf("failed to write test file: %v", err) + } + if err := os.Mkdir(filepath.Join(root, "subdir"), 0755); err != nil { + t.Fatalf("failed to create subdir: %v", err) + } + // A sibling temp dir, not filepath.Dir(root): that would be the shared + // system temp directory, where a fixed filename races other runs of this + // package. + outsideDir := createTempDir(t) + defer os.RemoveAll(outsideDir) + outside := filepath.Join(outsideDir, "outside.txt") + if err := os.WriteFile(outside, []byte("secret\n"), 0644); err != nil { + t.Fatalf("failed to write out-of-root file: %v", err) + } + + for name, target := range map[string]string{ + "inside-link": "plain.txt", + "escaping-link": outside, + "broken-link": filepath.Join(root, "does-not-exist"), + } { + if err := os.Symlink(target, filepath.Join(root, name)); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + } + loop := filepath.Join(root, "loop-link") + if err := os.Symlink(loop, loop); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + fifoErr := syscall.Mkfifo(filepath.Join(root, "pipe"), 0644) + + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatalf("EvalSymlinks(root) error = %v", err) + } + resolvedPlain, err := filepath.EvalSymlinks(plain) + if err != nil { + t.Fatalf("EvalSymlinks(plain) error = %v", err) + } + + entries := map[string]fs.DirEntry{} + dirEntries, err := os.ReadDir(resolvedRoot) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + for _, d := range dirEntries { + entries[d.Name()] = d + } + + tests := []struct { + name string + entry string + wantAction walkEntryAction + wantResolved string + why string + }{ + {"regular file", "plain.txt", walkEntryGrep, resolvedPlain, + "a normal in-bounds file is the whole point"}, + {"symlink inside the root", "inside-link", walkEntryGrep, resolvedPlain, + "resolves through to its target so the caller reads what was just checked"}, + {"symlink escaping the root", "escaping-link", walkEntrySkip, "", + "a symlink must not be usable to read outside the searched directory"}, + {"broken symlink", "broken-link", walkEntryUnreadable, "", + "a dangling link is a real read failure, not a policy exclusion"}, + {"symlink loop", "loop-link", walkEntryUnreadable, "", + "same class of failure as a dangling link"}, + {"directory", "subdir", walkEntrySkip, "", + "WalkDir descends on its own; Python never reaches this case"}, + {"fifo", "pipe", walkEntrySkip, "", + "excluded by policy, not failure -- opening one can block forever"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.entry == "pipe" && fifoErr != nil { + t.Skipf("FIFOs not supported: %v", fifoErr) + } + d, ok := entries[tt.entry] + if !ok { + t.Fatalf("test setup: no dir entry named %q", tt.entry) + } + action, resolved := classifyWalkEntry(resolvedRoot, filepath.Join(resolvedRoot, tt.entry), d) + if action != tt.wantAction { + t.Errorf("classifyWalkEntry() action = %v, want %v (%s)", action, tt.wantAction, tt.why) + } + if resolved != tt.wantResolved { + t.Errorf("classifyWalkEntry() resolved = %q, want %q", resolved, tt.wantResolved) + } + }) + } +} + +func TestGrepContent(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + if err := os.WriteFile(filepath.Join(tmpDir, "a.txt"), []byte("hello world\nFOO bar\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + subDir := filepath.Join(tmpDir, "sub") + if err := os.Mkdir(subDir, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(subDir, "b.txt"), []byte("another foo line\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + t.Run("matches within a single file", func(t *testing.T) { + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "hello", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "a.txt:1:hello world") { + t.Errorf("expected match with path:line:content, got %q", result) + } + }) + + t.Run("no matches", func(t *testing.T) { + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "nope", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if result != "no matches found" { + t.Errorf("expected no matches message, got %q", result) + } + }) + + t.Run("ignore case", func(t *testing.T) { + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "foo", false, true) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "FOO bar") { + t.Errorf("expected case-insensitive match, got %q", result) + } + }) + + t.Run("directory requires recursive", func(t *testing.T) { + if _, err := GrepContent(context.Background(), tmpDir, "foo", false, false); err == nil { + t.Fatal("expected error when searching a directory without recursive=true") + } + }) + + t.Run("recursive searches subdirectories", func(t *testing.T) { + result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "b.txt:1:another foo line") { + t.Errorf("expected match from subdirectory, got %q", result) + } + }) + + t.Run("invalid pattern", func(t *testing.T) { + if _, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "(", false, false); err == nil { + t.Fatal("expected error for invalid regex pattern") + } + }) + + t.Run("recursive search skips symlinks that escape the root", func(t *testing.T) { + outsideDir := createTempDir(t) + defer os.RemoveAll(outsideDir) + secretPath := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(secretPath, []byte("top secret foo\n"), 0644); err != nil { + t.Fatalf("Failed to write outside file: %v", err) + } + + linkPath := filepath.Join(subDir, "escape.txt") + if err := os.Symlink(secretPath, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if strings.Contains(result, "top secret") { + t.Errorf("expected symlinked file outside root to be skipped, got %q", result) + } + }) + + t.Run("recursive search greps the resolved target of an in-bounds file symlink", func(t *testing.T) { + realFile := filepath.Join(subDir, "real_target.txt") + if err := os.WriteFile(realFile, []byte("foo via symlink\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + linkPath := filepath.Join(subDir, "file_link.txt") + if err := os.Symlink(realFile, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + result, err := GrepContent(context.Background(), subDir, "foo via symlink", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + // The match must be reported against the resolved target path + // (real_target.txt), not the walked symlink path (file_link.txt): + // classifyWalkEntry verifies the resolved target is in-bounds, and + // the actual read must use that same resolved value rather than + // re-deriving/reopening the raw symlink path, or the verified-safe + // check and the actual read could diverge (TOCTOU). + if !strings.Contains(result, "real_target.txt:1:foo via symlink") { + t.Errorf("expected match to be reported against the resolved target path, got %q", result) + } + if strings.Contains(result, "file_link.txt:") { + t.Errorf("expected match not to be reported against the unresolved symlink path, got %q", result) + } + }) + + t.Run("recursive search does not abort on an in-bounds directory symlink", func(t *testing.T) { + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + if err := os.WriteFile(filepath.Join(walkDir, "aaa_first.txt"), []byte("foo one\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + realSub := filepath.Join(walkDir, "real_sub") + if err := os.Mkdir(realSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + + // A symlink to an in-bounds directory, lexically sorted between the + // two files below, so an incorrect abort partway through the walk + // would silently drop "zzz_sub"'s match. + linkPath := filepath.Join(walkDir, "mmm_link") + if err := os.Symlink(realSub, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + zzzSub := filepath.Join(walkDir, "zzz_sub") + if err := os.Mkdir(zzzSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(zzzSub, "zzz_last.txt"), []byte("foo two\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "aaa_first.txt:1:foo one") { + t.Errorf("expected match before the symlink, got %q", result) + } + if !strings.Contains(result, "zzz_last.txt:1:foo two") { + t.Errorf("expected match after the symlink (walk must not abort on it), got %q", result) + } + }) + + t.Run("recursive search resolves the root itself when it is an unresolved symlink", func(t *testing.T) { + realDir := createTempDir(t) + defer os.RemoveAll(realDir) + if err := os.WriteFile(filepath.Join(realDir, "match.txt"), []byte("foo inside\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + parentDir := createTempDir(t) + defer os.RemoveAll(parentDir) + linkRoot := filepath.Join(parentDir, "link-root") + if err := os.Symlink(realDir, linkRoot); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + // Pass the unresolved symlink directly, as a caller that doesn't + // pre-resolve its path would. + result, err := GrepContent(context.Background(), linkRoot, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "match.txt:1:foo inside") { + t.Errorf("expected GrepContent to resolve a symlinked root and recurse into it, got %q", result) + } + }) + + t.Run("recursive search does not hang on a FIFO and finds matches around it", func(t *testing.T) { + fifoDir := createTempDir(t) + defer os.RemoveAll(fifoDir) + + if err := os.WriteFile(filepath.Join(fifoDir, "aaa_before.txt"), []byte("foo before\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + fifoPath := filepath.Join(fifoDir, "mmm_pipe") + if err := syscall.Mkfifo(fifoPath, 0644); err != nil { + t.Skipf("FIFOs not supported: %v", err) + } + if err := os.WriteFile(filepath.Join(fifoDir, "zzz_after.txt"), []byte("foo after\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + done := make(chan struct{}) + var result string + var err error + go func() { + result, err = GrepContent(context.Background(), fifoDir, "foo", true, false) + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("GrepContent hung on a FIFO instead of skipping it") + } + + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "aaa_before.txt:1:foo before") { + t.Errorf("expected match before the FIFO, got %q", result) + } + if !strings.Contains(result, "zzz_after.txt:1:foo after") { + t.Errorf("expected match after the FIFO (walk must not hang or abort on it), got %q", result) + } + }) + + t.Run("a single target FIFO returns an error instead of hanging", func(t *testing.T) { + fifoDir := createTempDir(t) + defer os.RemoveAll(fifoDir) + fifoPath := filepath.Join(fifoDir, "pipe") + if err := syscall.Mkfifo(fifoPath, 0644); err != nil { + t.Skipf("FIFOs not supported: %v", err) + } + + done := make(chan struct{}) + var err error + go func() { + _, err = GrepContent(context.Background(), fifoPath, "foo", false, false) + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("GrepContent hung opening a FIFO directly instead of erroring") + } + + if err == nil { + t.Fatal("expected an error for a non-regular file target, got nil") + } + }) + + t.Run("recursive search does not abort or discard matches on an unreadable subdirectory", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + okSub := filepath.Join(walkDir, "aaa_ok") + if err := os.Mkdir(okSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(okSub, "match.txt"), []byte("foo readable\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + noPermSub := filepath.Join(walkDir, "mmm_noperm") + if err := os.Mkdir(noPermSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(noPermSub, 0000); err != nil { + t.Fatalf("Failed to chmod subdir: %v", err) + } + defer os.Chmod(noPermSub, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v, expected the unreadable subdirectory to be skipped rather than aborting the whole search", err) + } + if !strings.Contains(result, "match.txt:1:foo readable") { + t.Errorf("expected match from the readable sibling directory to survive an unreadable subdirectory elsewhere in the tree, got %q", result) + } + }) + + t.Run("no matches found is annotated when entries could not be read", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + noPermSub := filepath.Join(walkDir, "noperm") + if err := os.Mkdir(noPermSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(noPermSub, 0000); err != nil { + t.Fatalf("Failed to chmod subdir: %v", err) + } + defer os.Chmod(noPermSub, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "no matches found") || !strings.Contains(result, "could not be read") { + t.Errorf("expected an annotated no-matches message noting unreadable entries, got %q", result) + } + }) + + t.Run("matches are annotated with the skip count when some entries could not be read", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + okSub := filepath.Join(walkDir, "aaa_ok") + if err := os.Mkdir(okSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(okSub, "match.txt"), []byte("foo readable\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + noPermSub := filepath.Join(walkDir, "mmm_noperm") + if err := os.Mkdir(noPermSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(noPermSub, 0000); err != nil { + t.Fatalf("Failed to chmod subdir: %v", err) + } + defer os.Chmod(noPermSub, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "match.txt:1:foo readable") { + t.Errorf("expected the real match to still be reported, got %q", result) + } + if !strings.Contains(result, "could not be read") { + t.Errorf("expected the skip count to be reported alongside real matches, not silently dropped, got %q", result) + } + }) + + t.Run("recursive search on a fully unreadable root returns an error, not a confident empty result", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + if err := os.WriteFile(filepath.Join(walkDir, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(walkDir, 0000); err != nil { + t.Fatalf("Failed to chmod root: %v", err) + } + defer os.Chmod(walkDir, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err == nil { + t.Fatalf("expected an error when the search root itself is unreadable, got a result instead: %q", result) + } + }) + + t.Run("matched lines are truncated to 2000 characters", func(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + longLine := "foo " + strings.Repeat("x", 3000) + if err := os.WriteFile(filepath.Join(tmpDir, "long.txt"), []byte(longLine+"\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "long.txt"), "foo", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.HasSuffix(result, "...") { + t.Errorf("expected truncated line to end with '...', got %q", result) + } + if len(result) > 2100 { + t.Errorf("expected result to be truncated to roughly 2000 chars, got length %d", len(result)) + } + }) + + t.Run("recursive search honors a cancelled context", func(t *testing.T) { + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + // Enough entries that the walk is guaranteed to consult ctx at least + // once after cancellation rather than finishing outright. + for i := range 50 { + name := filepath.Join(walkDir, fmt.Sprintf("file_%02d.txt", i)) + if err := os.WriteFile(name, []byte("foo\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // already cancelled before the walk starts + + _, err := GrepContent(ctx, walkDir, "foo", true, false) + if err == nil { + t.Fatal("expected a cancelled context to abort the search, got no error") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("expected the error to wrap context.Canceled, got %v", err) + } + }) + + t.Run("a non-recursive search is unaffected by context", func(t *testing.T) { + // Single-file reads are bounded by file size, so they intentionally + // don't consult ctx -- pinning that so the cancellation check isn't + // later "helpfully" moved somewhere it would break normal reads. + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + if err := os.WriteFile(filepath.Join(tmpDir, "a.txt"), []byte("foo\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + result, err := GrepContent(ctx, filepath.Join(tmpDir, "a.txt"), "foo", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "a.txt:1:foo") { + t.Errorf("expected the single-file match, got %q", result) + } + }) +} diff --git a/go/adk/pkg/tools/shell.go b/go/adk/pkg/tools/shell.go index e8ee3932d..4456ee7b4 100644 --- a/go/adk/pkg/tools/shell.go +++ b/go/adk/pkg/tools/shell.go @@ -5,6 +5,7 @@ import ( "bytes" "context" "fmt" + "io/fs" "os" "os/exec" "path/filepath" @@ -14,6 +15,84 @@ import ( type CommandExecutor struct{} +// maxLineRunes is the longest line read_file and grep_file will emit before +// truncating. Counted in runes, not bytes -- see truncateRunes. +const maxLineRunes = 2000 + +// maxLineBytes caps how much of a single line scanFileLines will buffer. +// Lines are emitted truncated to maxLineRunes, but grep has to see the whole +// line to match against it, so the read limit is far larger than the emit +// limit. A line longer than this makes its file unreadable, which callers +// surface as an error (read_file) or as a skip count (grep_file). +const maxLineBytes = 1024 * 1024 + +// scanFileLines opens path and hands each line to visit, stopping early if +// visit returns false. Line numbers are 1-based. +// +// This is the single reader behind read_file and grep_file. Both need the +// same three guarantees -- reject non-regular files, cap how much of a line +// is buffered, and surface failures wrapped -- and keeping them in one place +// is what stops the two from drifting apart: an earlier revision set the +// scanner buffer only in the grep path, which left read_file failing outright +// on any file with a line over bufio's 64KB default. +func scanFileLines(path string, visit func(lineNum int, line string) bool) error { + // Stat before opening: os.Open on a FIFO with no writer connected blocks + // indefinitely, and nothing on these paths imposes a timeout. Doing it + // here rather than at each call site means no future caller can omit it. + info, err := os.Stat(path) + if err != nil { + return fmt.Errorf("failed to stat %q: %w", path, err) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("%q is not a regular file", path) + } + + file, err := os.Open(path) + if err != nil { + return fmt.Errorf("failed to open %q: %w", path, err) + } + defer file.Close() + + scanner := bufio.NewScanner(file) + // nil initial buffer, not a fixed 64KB one: bufio grows from 4KB by + // doubling, so a long line still reaches maxLineBytes, but the common + // case -- a recursive grep over a tree of small files -- stops paying + // 64KB of allocation per file. Measured at ~15x less allocated per file. + scanner.Buffer(nil, maxLineBytes) + for lineNum := 1; scanner.Scan(); lineNum++ { + if !visit(lineNum, scanner.Text()) { + return nil + } + } + if err := scanner.Err(); err != nil { + return fmt.Errorf("failed to read %q: %w", path, err) + } + return nil +} + +// truncateRunes shortens s to at most maxRunes code points, appending "..." +// when it truncates. +// +// Slicing a Go string directly (s[:n]) cuts on a byte boundary, which can +// split a multi-byte UTF-8 sequence and leave a partial code point that +// renders as U+FFFD. It also makes the limit mean bytes, so the same line +// would truncate at a different point than in the Python runtime (whose str +// slicing is per-code-point) and than the tool descriptions promise, which +// say "characters". +func truncateRunes(s string, maxRunes int) string { + count := 0 + // Ranging a string yields one iteration per rune, with i the byte index + // where that rune starts -- so at the maxRunes'th rune, s[:i] holds + // exactly maxRunes complete runes. + for i := range s { + if count == maxRunes { + return s[:i] + "..." + } + count++ + } + return s +} + // GetSessionPath creates the working directory used by skill execution tools. func GetSessionPath(sessionID, skillsDirectory string) (string, error) { if sessionID == "" { @@ -53,34 +132,19 @@ func GetSessionPath(sessionID, skillsDirectory string) (string, error) { // ReadFileContent reads a file with line numbers. func ReadFileContent(path string, offset, limit int) (string, error) { - file, err := os.Open(path) - if err != nil { - return "", err - } - defer file.Close() - var result strings.Builder - scanner := bufio.NewScanner(file) - lineNum := 1 start := max(offset, 1) count := 0 - for scanner.Scan() { - if lineNum >= start { - line := scanner.Text() - if len(line) > 2000 { - line = line[:2000] + "..." - } - fmt.Fprintf(&result, "%6d|%s\n", lineNum, line) - count++ - if limit > 0 && count >= limit { - break - } + err := scanFileLines(path, func(lineNum int, line string) bool { + if lineNum < start { + return true } - lineNum++ - } - - if err := scanner.Err(); err != nil { + fmt.Fprintf(&result, "%6d|%s\n", lineNum, truncateRunes(line, maxLineRunes)) + count++ + return limit <= 0 || count < limit + }) + if err != nil { return "", err } @@ -141,6 +205,57 @@ func EditFileContent(path string, oldString, newString string, replaceAll bool) return os.WriteFile(path, []byte(newContent), 0644) } +// ListDirContent lists the entries of a directory, one per line. Directories +// are suffixed with "/"; files are followed by their size in bytes. +func ListDirContent(path string) (string, error) { + entries, err := os.ReadDir(path) + if err != nil { + return "", fmt.Errorf("failed to read directory %q: %w", path, err) + } + + if len(entries) == 0 { + return "Directory is empty.", nil + } + + var result strings.Builder + for _, entry := range entries { + if entry.IsDir() { + fmt.Fprintf(&result, "%s/\n", entry.Name()) + continue + } + + // entry.IsDir() and entry.Info() both describe the entry itself + // (Lstat-like), so for a symlink they report the link rather than + // its target -- the target's type is lost, and Info().Size() is the + // length of the stored target path, not the file's size. os.Stat + // follows the link, so use it for both, matching Python's pathlib + // Path.is_dir()/Path.stat(), which follow symlinks by default. + if entry.Type()&fs.ModeSymlink != 0 { + target, statErr := os.Stat(filepath.Join(path, entry.Name())) + switch { + case statErr != nil: + // Broken link: there is no target to size. Print the bare + // name, as Python does when its stat() raises here. + fmt.Fprintf(&result, "%s\n", entry.Name()) + case target.IsDir(): + fmt.Fprintf(&result, "%s/\n", entry.Name()) + default: + fmt.Fprintf(&result, "%s\t%d\n", entry.Name(), target.Size()) + } + continue + } + + info, err := entry.Info() + if err != nil { + fmt.Fprintf(&result, "%s\n", entry.Name()) + continue + } + fmt.Fprintf(&result, "%s\t%d\n", entry.Name(), info.Size()) + } + + return strings.TrimSuffix(result.String(), "\n"), nil +} + func NewCommandExecutor() *CommandExecutor { return &CommandExecutor{} } diff --git a/go/adk/pkg/tools/shell_test.go b/go/adk/pkg/tools/shell_test.go index 076faa1c2..2c42ae482 100644 --- a/go/adk/pkg/tools/shell_test.go +++ b/go/adk/pkg/tools/shell_test.go @@ -2,11 +2,14 @@ package tools import ( "context" + "fmt" "os" "path/filepath" "strings" + "syscall" "testing" "time" + "unicode/utf8" ) func createTempDir(t *testing.T) string { @@ -173,6 +176,135 @@ func TestReadFileContent(t *testing.T) { } } +// TestReadFileContent_LongLineIsTruncatedNotFatal pins the buffer that +// scanFileLines sets. bufio.Scanner's default cap is 64KB, and exceeding it +// fails the *whole* scan -- so before the shared reader existed, one long line +// (a minified bundle, a single-line JSON blob) made every other line in the +// file unreadable too, even though grep_file handled the same file fine and +// read_file's own tool description promises such lines are truncated. +func TestReadFileContent_LongLineIsTruncatedNotFatal(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + // Well past bufio's 64KB default, well under scanFileLines' maxLineBytes. + longLine := strings.Repeat("a", 100_000) + path := filepath.Join(tmpDir, "bundle.min.js") + if err := os.WriteFile(path, []byte(longLine+"\nsecond line\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + result, err := ReadFileContent(path, 0, 0) + if err != nil { + t.Fatalf("ReadFileContent() error = %v, want the long line truncated instead", err) + } + + lines := strings.Split(result, "\n") + if len(lines) != 2 { + t.Fatalf("expected 2 lines, got %d: %q", len(lines), result) + } + // Six-column line number, "|", then the truncated body and the ellipsis. + body := strings.TrimSuffix(strings.SplitN(lines[0], "|", 2)[1], "...") + if n := utf8.RuneCountInString(body); n != maxLineRunes { + t.Errorf("expected the long line truncated to %d runes, got %d", maxLineRunes, n) + } + if !strings.HasSuffix(lines[0], "...") { + t.Error("expected the truncated line to end with '...'") + } + // The whole point: the rest of the file survives. + if !strings.Contains(lines[1], "second line") { + t.Errorf("expected the line after the long one to still be read, got %q", lines[1]) + } +} + +// TestReadFileContent_LineOverMaxBytesErrors documents the deliberate residual +// limit: maxLineBytes exists so a sandboxed runtime can't be made to buffer an +// arbitrarily long line, so a line past it still fails rather than truncating. +func TestReadFileContent_LineOverMaxBytesErrors(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + path := filepath.Join(tmpDir, "huge.txt") + if err := os.WriteFile(path, []byte(strings.Repeat("a", maxLineBytes+1)+"\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + if _, err := ReadFileContent(path, 0, 0); err == nil { + t.Fatal("expected a line over maxLineBytes to error, got nil") + } +} + +func TestReadFileContent_FIFOReturnsErrorInsteadOfHanging(t *testing.T) { + fifoDir := createTempDir(t) + defer os.RemoveAll(fifoDir) + + fifoPath := filepath.Join(fifoDir, "pipe") + if err := syscall.Mkfifo(fifoPath, 0644); err != nil { + t.Skipf("FIFOs not supported: %v", err) + } + + // Opening a FIFO with no writer connected blocks forever, and nothing on + // this path has a timeout -- so run it off-goroutine and fail on the + // timeout rather than hanging the whole test binary. + done := make(chan struct{}) + var err error + go func() { + _, err = ReadFileContent(fifoPath, 0, 0) + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("ReadFileContent hung opening a FIFO instead of erroring") + } + + if err == nil { + t.Fatal("expected an error for a non-regular file target, got nil") + } + if !strings.Contains(err.Error(), "not a regular file") { + t.Errorf("expected a not-a-regular-file error, got %v", err) + } +} + +func TestTruncateRunes(t *testing.T) { + t.Run("truncates on a rune boundary and stays valid UTF-8", func(t *testing.T) { + // 3000 three-byte runes: byte-slicing at 2000 would land mid-rune. + long := strings.Repeat("世", 3000) + + got := truncateRunes(long, maxLineRunes) + + if !utf8.ValidString(got) { + t.Error("expected truncated output to be valid UTF-8") + } + if strings.ContainsRune(got, utf8.RuneError) { + t.Error("expected no U+FFFD replacement char from a split rune") + } + body := strings.TrimSuffix(got, "...") + if n := utf8.RuneCountInString(body); n != maxLineRunes { + t.Errorf("expected exactly %d runes before the ellipsis, got %d", maxLineRunes, n) + } + if !strings.HasSuffix(got, "...") { + t.Errorf("expected truncated output to end with '...', got %q", got) + } + }) + + t.Run("leaves a short string untouched", func(t *testing.T) { + if got := truncateRunes("héllo", maxLineRunes); got != "héllo" { + t.Errorf("expected input returned unchanged, got %q", got) + } + }) + + t.Run("counts characters not bytes, matching the Python runtime", func(t *testing.T) { + // 2000 multi-byte runes is 6000 bytes -- well over a byte-based + // limit, but exactly at the rune limit, so it must NOT truncate. + exact := strings.Repeat("世", maxLineRunes) + if got := truncateRunes(exact, maxLineRunes); got != exact { + t.Errorf("expected a string of exactly %d runes to be left alone, got %d runes", + maxLineRunes, utf8.RuneCountInString(got)) + } + }) +} + func TestWriteFileContent(t *testing.T) { tmpDir := createTempDir(t) defer os.RemoveAll(tmpDir) @@ -301,6 +433,120 @@ func TestEditFileContent(t *testing.T) { } } +func TestListDirContent(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + if err := os.WriteFile(filepath.Join(tmpDir, "b.txt"), []byte("hello"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Mkdir(filepath.Join(tmpDir, "a-subdir"), 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + + t.Run("lists files and directories", func(t *testing.T) { + result, err := ListDirContent(tmpDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + if !strings.Contains(result, "a-subdir/") { + t.Errorf("expected directory entry with trailing slash, got %q", result) + } + if !strings.Contains(result, "b.txt\t5") { + t.Errorf("expected file entry with size, got %q", result) + } + }) + + t.Run("empty directory", func(t *testing.T) { + emptyDir := filepath.Join(tmpDir, "a-subdir") + result, err := ListDirContent(emptyDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + if result != "Directory is empty." { + t.Errorf("expected empty directory message, got %q", result) + } + }) + + t.Run("nonexistent path", func(t *testing.T) { + if _, err := ListDirContent(filepath.Join(tmpDir, "does-not-exist")); err == nil { + t.Fatal("expected error for nonexistent path") + } + }) + + t.Run("symlink to a directory is listed as a directory", func(t *testing.T) { + realDir := filepath.Join(tmpDir, "a-subdir") + linkPath := filepath.Join(tmpDir, "dir-link") + if err := os.Symlink(realDir, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + result, err := ListDirContent(tmpDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + if !strings.Contains(result, "dir-link/") { + t.Errorf("expected symlinked directory to be listed with a trailing slash, got %q", result) + } + if strings.Contains(result, "dir-link\t") { + t.Errorf("expected symlinked directory not to be listed as a file, got %q", result) + } + }) + + t.Run("symlink to a file reports the target's size, not the link's", func(t *testing.T) { + linkDir := createTempDir(t) + defer os.RemoveAll(linkDir) + + // The target's contents must be longer than the symlink's own + // "size" (the byte length of the stored target path) so the two are + // unambiguous: Lstat would report len(target path), Stat reports 20. + target := filepath.Join(linkDir, "target.txt") + contents := strings.Repeat("x", 20) + if err := os.WriteFile(target, []byte(contents), 0644); err != nil { + t.Fatalf("Failed to write target file: %v", err) + } + linkPath := filepath.Join(linkDir, "file-link") + if err := os.Symlink(target, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + result, err := ListDirContent(linkDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + if !strings.Contains(result, fmt.Sprintf("file-link\t%d", len(contents))) { + t.Errorf("expected symlinked file to report the target's size (%d), got %q", len(contents), result) + } + if strings.Contains(result, fmt.Sprintf("file-link\t%d", len(target))) { + t.Errorf("expected not to report the symlink's own size (%d), got %q", len(target), result) + } + }) + + t.Run("broken symlink is listed without a size", func(t *testing.T) { + linkDir := createTempDir(t) + defer os.RemoveAll(linkDir) + + linkPath := filepath.Join(linkDir, "broken-link") + if err := os.Symlink(filepath.Join(linkDir, "does-not-exist"), linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + result, err := ListDirContent(linkDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + // Bare name, no size and no trailing slash -- matching Python's + // list_dir_content, whose stat() raises on a dangling link. + if !strings.Contains(result, "broken-link") { + t.Errorf("expected broken symlink to be listed, got %q", result) + } + if strings.Contains(result, "broken-link\t") || strings.Contains(result, "broken-link/") { + t.Errorf("expected broken symlink to be listed with no size and no trailing slash, got %q", result) + } + }) +} + func TestExecuteCommand(t *testing.T) { tmpDir := createTempDir(t) defer os.RemoveAll(tmpDir) diff --git a/go/adk/pkg/tools/skills.go b/go/adk/pkg/tools/skills.go index 8786555ff..35a235189 100644 --- a/go/adk/pkg/tools/skills.go +++ b/go/adk/pkg/tools/skills.go @@ -2,6 +2,7 @@ package tools import ( "fmt" + "log/slog" "os" "path/filepath" "strings" @@ -11,6 +12,33 @@ import ( "google.golang.org/adk/v2/tool/functiontool" ) +// enableFileSearchToolsEnv gates the list_files and grep_file tools, which +// are opt-in (disabled by default): they let an agent enumerate and search +// the filesystem under its session/skills roots without invoking a shell, so +// deployments that want to grant that visibility deliberately can, rather +// than having it enabled implicitly. +// +// Also registered (separately, for `kagent env` CLI discoverability only, +// not read here) as KagentEnableFileSearchTools in go/core/pkg/env/kagent.go. +// TestEnableFileSearchToolsEnvMatchesRegistry pins the two literals together +// so they cannot drift. +const enableFileSearchToolsEnv = "KAGENT_ENABLE_FILE_SEARCH_TOOLS" + +// fileSearchToolsEnabled accepts the same case-insensitive true-values as +// Python's file_search_tools_enabled() (kagent-skills/shell.py), so the +// same literal env var value behaves identically in either runtime rather +// than relying on Go's strconv.ParseBool grammar, which Python doesn't +// replicate exactly (e.g. ParseBool requires the exact casing "True", not +// "tRue"). +func fileSearchToolsEnabled() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv(enableFileSearchToolsEnv))) { + case "1", "t", "true": + return true + default: + return false + } +} + const ( readFileDescription = `Reads a file from the filesystem with line numbers. @@ -45,6 +73,24 @@ Usage: - old_string and new_string must be different - Note: skills/ directory is read-only` + listFilesDescription = `Lists files and directories at a given path. + +Usage: +- Provide a path (absolute or relative to your working directory); defaults to the working directory +- Directories are listed with a trailing "/"; files are followed by their size in bytes +- You can list skills/ directory, uploads/, outputs/, or any directory in your session` + + grepFileDescription = `Searches for a regular expression pattern in a file or directory. + +Usage: +- Provide a pattern and a path (absolute or relative to your working directory) +- Set recursive=true to search all files under a directory path +- Recursion does not follow symlinked subdirectories (e.g. skills/ is a symlink) - + point path directly at skills/ to search inside it +- Set ignore_case=true for case-insensitive matching +- Returns matching lines as path:line_number:content +- You can search the skills/ directory, uploads/, outputs/, or any file/directory in your session` + bashDescription = `Execute bash commands in the skills environment with sandbox protection. Working Directory & Structure: @@ -66,6 +112,14 @@ For file operations: Timeouts: - python scripts: 60s - other commands: 30s` + + // fileSearchToolsBashHint is appended to bashDescription only when + // list_files/grep_file are enabled, so bash's own description doesn't + // point the model at tools that aren't registered. Appended as a + // trailing paragraph rather than interpolated into bashDescription, so + // the long, free-form prose above stays a plain string -- not a format + // template where a stray '%' added later could silently corrupt output. + fileSearchToolsBashHint = "\nAlso available: list_files and grep_file, for exploring the filesystem without a full shell command." ) type bashInput struct { @@ -91,6 +145,17 @@ type editFileInput struct { ReplaceAll bool `json:"replace_all,omitempty"` } +type listFilesInput struct { + Path string `json:"path,omitempty"` +} + +type grepFileInput struct { + Pattern string `json:"pattern"` + Path string `json:"path"` + Recursive bool `json:"recursive,omitempty"` + IgnoreCase bool `json:"ignore_case,omitempty"` +} + // NewSkillExecutionTools creates the filesystem and shell tools used to execute // skills. Skill discovery and loading are provided by Go ADK's skilltoolset. func NewSkillExecutionTools(skillsDirectory string) ([]tool.Tool, error) { @@ -164,9 +229,87 @@ func NewSkillExecutionTools(skillsDirectory string) ([]tool.Tool, error) { return nil, fmt.Errorf("failed to create edit_file tool: %w", err) } + tools := []tool.Tool{readFileTool, writeFileTool, editFileTool} + + // list_files/grep_file are opt-in: they give an agent broad filesystem + // visibility, so deployments enable them deliberately. Note this gate is + // theirs alone -- bash below is always registered. + fileSearchEnabled := fileSearchToolsEnabled() + if fileSearchEnabled { + listFilesTool, err := functiontool.New(functiontool.Config{ + Name: "list_files", + Description: listFilesDescription, + }, func(ctx adkagent.Context, in listFilesInput) (string, error) { + requestedPath := in.Path + if strings.TrimSpace(requestedPath) == "" { + requestedPath = "." + } + + path, err := resolveReadPath(ctx.SessionID(), absSkillsDir, requestedPath) + if err != nil { + return fmt.Sprintf("Error listing %s: %v", requestedPath, err), nil + } + + content, err := ListDirContent(path) + if err != nil { + return fmt.Sprintf("Error listing %s: %v", requestedPath, err), nil + } + return content, nil + }) + if err != nil { + return nil, fmt.Errorf("failed to create list_files tool: %w", err) + } + + grepFileTool, err := functiontool.New(functiontool.Config{ + Name: "grep_file", + Description: grepFileDescription, + }, func(ctx adkagent.Context, in grepFileInput) (string, error) { + if strings.TrimSpace(in.Pattern) == "" { + return "Error: No pattern provided", nil + } + if strings.TrimSpace(in.Path) == "" { + return "Error: No file path provided", nil + } + + path, err := resolveReadPath(ctx.SessionID(), absSkillsDir, in.Path) + if err != nil { + return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil + } + + // ctx is the ADK tool context, which embeds context.Context, so a + // recursive search is abortable by whatever deadline or + // cancellation the caller already set -- and the walk, not the + // match, is the part that can run long here. + // + // No fixed deadline is added on top of that. The Python runtime + // caps grep at 30s, but that bound exists for a hazard Go doesn't + // have: Python's `re` backtracks, so an adversarial pattern can + // hang on a single line, while RE2 is linear-time. Since only the + // walk is unbounded, and its cost scales with the tree the + // deployment itself provisioned, a fixed cap here would break + // large legitimate searches without closing a distinct risk. + content, err := GrepContent(ctx, path, in.Pattern, in.Recursive, in.IgnoreCase) + if err != nil { + return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil + } + return content, nil + }) + if err != nil { + return nil, fmt.Errorf("failed to create grep_file tool: %w", err) + } + + tools = append(tools, listFilesTool, grepFileTool) + } else { + slog.Debug("omitting list_files/grep_file tools: " + enableFileSearchToolsEnv + " not enabled") + } + + desc := bashDescription + if fileSearchEnabled { + desc += fileSearchToolsBashHint + } bashTool, err := functiontool.New(functiontool.Config{ Name: "bash", - Description: bashDescription, + Description: desc, }, func(ctx adkagent.Context, in bashInput) (string, error) { command := strings.TrimSpace(in.Command) if command == "" { @@ -187,11 +330,43 @@ func NewSkillExecutionTools(skillsDirectory string) ([]tool.Tool, error) { if err != nil { return nil, fmt.Errorf("failed to create bash tool: %w", err) } + tools = append(tools, bashTool) - return []tool.Tool{readFileTool, writeFileTool, editFileTool, bashTool}, nil + return tools, nil } -func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, error) { +// pathPolicy is the sandbox contract for one class of filesystem access. +// The three resolvers below are the same code differing only in these +// fields, so they are expressed as data rather than as flags threaded +// through a shared function. +type pathPolicy struct { + // resolve maps the requested leaf to a real path. EvalSymlinks requires + // it to already exist; resolvePathWithExistingParents tolerates a + // not-yet-created leaf, which only writes need. + resolve func(string) (string, error) + // allowSkillsRoot lets the read-only skills directory count as an allowed + // root. Reads may reach it; edits and writes must not, or an agent could + // modify the skills it was given. + allowSkillsRoot bool + // denied names the boundary in the error an out-of-bounds path gets. It + // is its own field rather than being derived from allowSkillsRoot: the + // two happen to correlate today, but a resolver that denied the skills + // root for a reason other than writability would otherwise be described + // wrongly. + denied string +} + +// TestResolvePathContainment pins this matrix for all three policies. +var ( + readPolicy = pathPolicy{filepath.EvalSymlinks, true, "the allowed roots"} + editPolicy = pathPolicy{filepath.EvalSymlinks, false, "the writable session directory"} + writePolicy = pathPolicy{resolvePathWithExistingParents, false, "the writable session directory"} +) + +// resolveSandboxedPath maps a requested path onto the session directory, +// resolves it under policy, and then requires the result to land inside an +// allowed root -- the check that keeps an agent inside its sandbox. +func resolveSandboxedPath(sessionID, skillsDirectory, requestedPath string, policy pathPolicy) (string, error) { sessionPath, err := GetSessionPath(sessionID, skillsDirectory) if err != nil { return "", err @@ -202,79 +377,47 @@ func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, return "", err } - resolvedCandidate, err := filepath.EvalSymlinks(candidate) + resolvedCandidate, err := policy.resolve(candidate) if err != nil { return "", err } - sessionRoot, err := filepath.Abs(sessionPath) - if err != nil { - return "", err - } - skillsRoot, err := filepath.EvalSymlinks(skillsDirectory) + sessionRoot, err := filepath.EvalSymlinks(sessionPath) if err != nil { return "", err } + roots := []string{sessionRoot} - if !isWithinRoot(resolvedCandidate, sessionRoot) && !isWithinRoot(resolvedCandidate, skillsRoot) { - return "", fmt.Errorf("path %q is outside the allowed roots", requestedPath) - } - - return resolvedCandidate, nil -} - -func resolveEditPath(sessionID, skillsDirectory, requestedPath string) (string, error) { - sessionPath, err := GetSessionPath(sessionID, skillsDirectory) - if err != nil { - return "", err + if policy.allowSkillsRoot { + // Resolved eagerly rather than only when the session root misses, so + // an unresolvable skills directory still surfaces as an error the way + // it did before these three were merged into one function. + skillsRoot, err := filepath.EvalSymlinks(skillsDirectory) + if err != nil { + return "", err + } + roots = append(roots, skillsRoot) } - candidate, err := resolveRequestedPath(sessionPath, requestedPath) - if err != nil { - return "", err + for _, root := range roots { + if WithinRoot(resolvedCandidate, root) { + return resolvedCandidate, nil + } } - resolvedCandidate, err := filepath.EvalSymlinks(candidate) - if err != nil { - return "", err - } + return "", fmt.Errorf("path %q is outside %s", requestedPath, policy.denied) +} - sessionRoot, err := filepath.Abs(sessionPath) - if err != nil { - return "", err - } - if !isWithinRoot(resolvedCandidate, sessionRoot) { - return "", fmt.Errorf("path %q is outside the writable session directory", requestedPath) - } +func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, error) { + return resolveSandboxedPath(sessionID, skillsDirectory, requestedPath, readPolicy) +} - return resolvedCandidate, nil +func resolveEditPath(sessionID, skillsDirectory, requestedPath string) (string, error) { + return resolveSandboxedPath(sessionID, skillsDirectory, requestedPath, editPolicy) } func resolveWritePath(sessionID, skillsDirectory, requestedPath string) (string, error) { - sessionPath, err := GetSessionPath(sessionID, skillsDirectory) - if err != nil { - return "", err - } - - candidate, err := resolveRequestedPath(sessionPath, requestedPath) - if err != nil { - return "", err - } - - resolvedCandidate, err := resolvePathWithExistingParents(candidate) - if err != nil { - return "", err - } - - sessionRoot, err := filepath.Abs(sessionPath) - if err != nil { - return "", err - } - if !isWithinRoot(resolvedCandidate, sessionRoot) { - return "", fmt.Errorf("path %q is outside the writable session directory", requestedPath) - } - - return resolvedCandidate, nil + return resolveSandboxedPath(sessionID, skillsDirectory, requestedPath, writePolicy) } func resolveRequestedPath(basePath, requestedPath string) (string, error) { @@ -323,9 +466,3 @@ func resolvePathWithExistingParents(path string) (string, error) { current = parent } } - -func isWithinRoot(path, root string) bool { - path = filepath.Clean(path) - root = filepath.Clean(root) - return path == root || strings.HasPrefix(path, root+string(filepath.Separator)) -} diff --git a/go/adk/pkg/tools/skills_test.go b/go/adk/pkg/tools/skills_test.go index 6998f7de9..a0b20442b 100644 --- a/go/adk/pkg/tools/skills_test.go +++ b/go/adk/pkg/tools/skills_test.go @@ -6,45 +6,178 @@ import ( "path/filepath" "strings" "testing" + "unicode/utf8" + + "github.com/kagent-dev/kagent/go/core/pkg/env" + adkagent "google.golang.org/adk/v2/agent" + "google.golang.org/adk/v2/tool" + "google.golang.org/adk/v2/tool/toolconfirmation" ) -func TestResolveReadPath_AllowsSymlinkedSkillsDirectory(t *testing.T) { - t.Setenv("TMPDIR", t.TempDir()) - skillsDir := t.TempDir() - skillFile := filepath.Join(skillsDir, "script.py") - if err := os.WriteFile(skillFile, []byte("print('ok')\n"), 0644); err != nil { - t.Fatalf("failed to write skill file: %v", err) +// TestEnableFileSearchToolsEnvMatchesRegistry pins this package's env var +// literal to the `kagent env` registry entry in go/core/pkg/env. The name is +// declared twice on purpose -- the runtime reads the raw variable here, while +// the registry exists solely so `kagent env` can document it -- so without +// this assertion nothing would catch the two drifting apart. +// +// This is a test-only import: it does not put a dependency on the +// control-plane module into the shipped agent runtime. +func TestEnableFileSearchToolsEnvMatchesRegistry(t *testing.T) { + if got, want := enableFileSearchToolsEnv, env.KagentEnableFileSearchTools.Name(); got != want { + t.Fatalf("env var name drift: %s has %q, go/core/pkg/env registry has %q", + "go/adk/pkg/tools/skills.go", got, want) } +} - sessionID := fmt.Sprintf("%s-read", t.Name()) - resolved, err := resolveReadPath(sessionID, skillsDir, "skills/script.py") - if err != nil { - t.Fatalf("resolveReadPath() error = %v", err) +// fakeToolContext is a minimal agent.Context for directly invoking tools in +// tests, bypassing the full ADK flow engine. Embeds StrictContextMock (an +// ADK test double) and overrides only the methods functiontool.Run() calls. +type fakeToolContext struct { + adkagent.StrictContextMock + sessionID string +} + +func (f *fakeToolContext) SessionID() string { return f.sessionID } +func (f *fakeToolContext) ToolConfirmation() *toolconfirmation.ToolConfirmation { + return nil +} + +// runnableTool mirrors the unexported Run method that functiontool.New's +// concrete type implements; declaring it locally lets us type-assert without +// depending on functiontool internals. +type runnableTool interface { + Run(ctx adkagent.Context, args any) (map[string]any, error) +} + +func runTool(t *testing.T, tl tool.Tool, ctx adkagent.Context, args map[string]any) string { + t.Helper() + runner, ok := tl.(runnableTool) + if !ok { + t.Fatalf("tool %q does not support direct invocation", tl.Name()) } - want, err := filepath.EvalSymlinks(skillFile) + result, err := runner.Run(ctx, args) if err != nil { - t.Fatalf("EvalSymlinks(skillFile) error = %v", err) + t.Fatalf("%s.Run() error = %v", tl.Name(), err) } - if resolved != want { - t.Fatalf("resolveReadPath() = %q, want %q", resolved, want) + text, ok := result["result"].(string) + if !ok { + t.Fatalf("%s.Run() result = %#v, want map with string \"result\"", tl.Name(), result) } + return text } -func TestResolveWritePath_BlocksSkillsSymlink(t *testing.T) { - t.Setenv("TMPDIR", t.TempDir()) - skillsDir := t.TempDir() - sessionID := fmt.Sprintf("%s-write", t.Name()) - _, err := resolveWritePath(sessionID, skillsDir, "skills/new-file.txt") - if err == nil { - t.Fatal("expected write through skills symlink to be rejected") +// TestResolvePathContainment pins the sandbox boundary each resolver +// enforces. The three differ along exactly two axes -- whether the skills +// directory is an allowed root, and whether a not-yet-existing leaf is +// tolerated -- and those differences are the whole security contract: +// +// session dir skills dir missing leaf +// resolveReadPath allow allow reject +// resolveEditPath allow REJECT reject +// resolveWritePath allow REJECT allow +// +// resolveEditPath in particular had no direct coverage before this, so a +// refactor could have silently granted it the skills root -- letting an +// agent edit read-only skill files -- with nothing failing. +func TestResolvePathContainment(t *testing.T) { + resolvers := map[string]struct { + fn func(sessionID, skillsDirectory, requestedPath string) (string, error) + allowsSkillsDir bool + allowsNewLeaf bool + // deniedContains is the boundary each policy names when it rejects a + // path, kept here so the wording stays tied to the resolver it + // describes rather than drifting into a generic message. + deniedContains string + }{ + "read": {resolveReadPath, true, false, "outside the allowed roots"}, + "edit": {resolveEditPath, false, false, "outside the writable session directory"}, + "write": {resolveWritePath, false, true, "outside the writable session directory"}, } - if !strings.Contains(err.Error(), "outside the writable session directory") { - t.Fatalf("unexpected error: %v", err) + + for name, r := range resolvers { + t.Run(name, func(t *testing.T) { + t.Setenv("TMPDIR", t.TempDir()) + skillsDir := t.TempDir() + if err := os.WriteFile(filepath.Join(skillsDir, "script.py"), []byte("print('ok')\n"), 0644); err != nil { + t.Fatalf("failed to write skill file: %v", err) + } + sessionID := fmt.Sprintf("%s-%s", t.Name(), name) + + // Seed a real file in the session dir so the "existing leaf" + // cases have something to resolve to. + sessionPath, err := GetSessionPath(sessionID, skillsDir) + if err != nil { + t.Fatalf("GetSessionPath() error = %v", err) + } + if err := os.WriteFile(filepath.Join(sessionPath, "notes.txt"), []byte("hi\n"), 0644); err != nil { + t.Fatalf("failed to seed session file: %v", err) + } + + t.Run("allows a path inside the session directory", func(t *testing.T) { + if _, err := r.fn(sessionID, skillsDir, "notes.txt"); err != nil { + t.Errorf("expected session-dir path to be allowed, got %v", err) + } + }) + + t.Run("rejects a path outside every root", func(t *testing.T) { + _, err := r.fn(sessionID, skillsDir, "/etc/passwd") + if err == nil { + t.Fatal("expected a path outside the allowed roots to be rejected") + } + if !strings.Contains(err.Error(), r.deniedContains) { + t.Errorf("expected the error to name %q, got %v", r.deniedContains, err) + } + }) + + t.Run("skills directory", func(t *testing.T) { + resolved, err := r.fn(sessionID, skillsDir, "skills/script.py") + if r.allowsSkillsDir { + if err != nil { + t.Fatalf("expected the skills dir to be readable, got %v", err) + } + // The skills dir is reached through a symlink in the + // session dir, so a resolver that returned the unresolved + // path would still look "allowed" -- assert it resolved + // through to the real file. + want, evalErr := filepath.EvalSymlinks(filepath.Join(skillsDir, "script.py")) + if evalErr != nil { + t.Fatalf("EvalSymlinks() error = %v", evalErr) + } + if resolved != want { + t.Errorf("resolved to %q, want %q", resolved, want) + } + return + } + if err == nil { + t.Fatal("expected the read-only skills dir to be rejected for mutation") + } + if !strings.Contains(err.Error(), r.deniedContains) { + t.Errorf("expected the error to name %q, got %v", r.deniedContains, err) + } + }) + + t.Run("not-yet-existing leaf", func(t *testing.T) { + _, err := r.fn(sessionID, skillsDir, "brand-new-file.txt") + if r.allowsNewLeaf && err != nil { + t.Errorf("expected a new file in the session dir to be allowed, got %v", err) + } + if !r.allowsNewLeaf && err == nil { + t.Error("expected a nonexistent path to be rejected") + } + }) + + t.Run("rejects traversal escaping the session directory", func(t *testing.T) { + if _, err := r.fn(sessionID, skillsDir, "../../../etc/passwd"); err == nil { + t.Error("expected ../ traversal out of the session dir to be rejected") + } + }) + }) } } func TestNewSkillExecutionTools_ReturnsExpectedToolSet(t *testing.T) { skillsDir := t.TempDir() + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "true") skillDir := filepath.Join(skillsDir, "demo") if err := os.MkdirAll(skillDir, 0755); err != nil { t.Fatalf("failed to create skill dir: %v", err) @@ -67,9 +200,217 @@ description: Demo skill. got[tool.Name()] = true } - for _, name := range []string{"read_file", "write_file", "edit_file", "bash"} { + for _, name := range []string{"read_file", "write_file", "edit_file", "list_files", "grep_file", "bash"} { if !got[name] { t.Errorf("expected tool %q to be present", name) } } } + +func TestNewSkillExecutionTools_OmitsListFilesAndGrepFileByDefault(t *testing.T) { + skillsDir := t.TempDir() + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "") + + tools, err := NewSkillExecutionTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillExecutionTools() error = %v, want nil (list_files/grep_file should be omitted, not fatal)", err) + } + + got := map[string]bool{} + for _, tool := range tools { + got[tool.Name()] = true + } + + for _, name := range []string{"read_file", "write_file", "edit_file", "bash"} { + if !got[name] { + t.Errorf("expected tool %q to be present even without KAGENT_ENABLE_FILE_SEARCH_TOOLS", name) + } + } + if got["list_files"] { + t.Error("expected list_files tool to be omitted by default") + } + if got["grep_file"] { + t.Error("expected grep_file tool to be omitted by default") + } +} + +func TestNewSkillExecutionTools_BashDescriptionMentionsFileSearchToolsOnlyWhenEnabled(t *testing.T) { + skillsDir := t.TempDir() + + findBash := func(t *testing.T, tools []tool.Tool) tool.Tool { + t.Helper() + for _, tl := range tools { + if tl.Name() == "bash" { + return tl + } + } + t.Fatal("expected bash tool to be present") + return nil + } + + t.Run("disabled by default", func(t *testing.T) { + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "") + tools, err := NewSkillExecutionTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillExecutionTools() error = %v", err) + } + desc := findBash(t, tools).Description() + if strings.Contains(desc, "list_files") || strings.Contains(desc, "grep_file") { + t.Errorf("bash description should not mention list_files/grep_file when disabled, got %q", desc) + } + }) + + t.Run("mentioned when enabled", func(t *testing.T) { + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "true") + tools, err := NewSkillExecutionTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillExecutionTools() error = %v", err) + } + desc := findBash(t, tools).Description() + if !strings.Contains(desc, "list_files and grep_file") { + t.Errorf("bash description should mention list_files and grep_file when enabled, got %q", desc) + } + }) +} + +// TestListFilesAndGrepFileTools_RunThroughADK invokes the real functiontool.Run() +// path (the same one the ADK flow engine uses to execute a model's tool call), +// rather than calling ListDirContent/GrepContent directly, to verify the +// closures in NewSkillExecutionTools correctly wire path resolution and +// argument parsing end-to-end. +func TestListFilesAndGrepFileTools_RunThroughADK(t *testing.T) { + t.Setenv("TMPDIR", t.TempDir()) + skillsDir := t.TempDir() + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "true") + + tools, err := NewSkillExecutionTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillExecutionTools() error = %v", err) + } + + var listFilesTool, grepFileTool, readFileTool, bashTool tool.Tool + for _, tl := range tools { + switch tl.Name() { + case "list_files": + listFilesTool = tl + case "grep_file": + grepFileTool = tl + case "read_file": + readFileTool = tl + case "bash": + bashTool = tl + } + } + if listFilesTool == nil || grepFileTool == nil || readFileTool == nil || bashTool == nil { + t.Fatal("expected list_files, grep_file, read_file and bash tools to be present") + } + + sessionID := fmt.Sprintf("%s-session", t.Name()) + sessionPath, err := GetSessionPath(sessionID, skillsDir) + if err != nil { + t.Fatalf("GetSessionPath() error = %v", err) + } + if err := os.WriteFile(filepath.Join(sessionPath, "notes.txt"), []byte("hello kagent\nsecond line\n"), 0644); err != nil { + t.Fatalf("failed to seed session file: %v", err) + } + if err := os.Mkdir(filepath.Join(sessionPath, "logs"), 0755); err != nil { + t.Fatalf("failed to create session subdir: %v", err) + } + + ctx := &fakeToolContext{sessionID: sessionID} + + t.Run("list_files defaults to the working directory", func(t *testing.T) { + result := runTool(t, listFilesTool, ctx, map[string]any{}) + if !strings.Contains(result, "notes.txt") || !strings.Contains(result, "logs/") { + t.Errorf("list_files result = %q, want entries for notes.txt and logs/", result) + } + }) + + t.Run("grep_file finds a match by relative path", func(t *testing.T) { + result := runTool(t, grepFileTool, ctx, map[string]any{ + "pattern": "kagent", + "path": "notes.txt", + }) + if !strings.Contains(result, "notes.txt:1:hello kagent") { + t.Errorf("grep_file result = %q, want a match on line 1", result) + } + }) + + t.Run("grep_file reports no matches without erroring", func(t *testing.T) { + result := runTool(t, grepFileTool, ctx, map[string]any{ + "pattern": "nope", + "path": "notes.txt", + }) + if result != "no matches found" { + t.Errorf("grep_file result = %q, want %q", result, "no matches found") + } + }) + + t.Run("list_files rejects paths outside the session/skills roots", func(t *testing.T) { + result := runTool(t, listFilesTool, ctx, map[string]any{"path": "/etc"}) + if !strings.Contains(result, "outside the allowed roots") { + t.Errorf("list_files result = %q, want an outside-allowed-roots error", result) + } + }) + + // Exercised here, through the registered tool rather than against + // ReadFileContent directly, because the failure this guards was invisible + // at the function level: read_file and grep_file scanned with different + // buffer limits, so a file grep_file handled fine made read_file fail + // outright. Driving the same tree through both tools is what makes that + // asymmetry observable. + t.Run("read_file and grep_file agree on a line past bufio's default", func(t *testing.T) { + longLine := strings.Repeat("a", 100_000) + bundle := filepath.Join(sessionPath, "bundle.min.js") + if err := os.WriteFile(bundle, []byte(longLine+"\nneedle here\n"), 0644); err != nil { + t.Fatalf("failed to seed long-line file: %v", err) + } + + read := runTool(t, readFileTool, ctx, map[string]any{"file_path": "bundle.min.js"}) + if strings.Contains(read, "Error reading file") { + t.Fatalf("read_file failed on a long line instead of truncating: %q", read) + } + readLines := strings.Split(read, "\n") + if len(readLines) != 2 { + t.Fatalf("read_file returned %d lines, want 2: %.200q", len(readLines), read) + } + body := strings.TrimSuffix(strings.SplitN(readLines[0], "|", 2)[1], "...") + if n := utf8.RuneCountInString(body); n != maxLineRunes { + t.Errorf("read_file truncated line 1 to %d runes, want %d", n, maxLineRunes) + } + if !strings.Contains(readLines[1], "needle here") { + t.Errorf("read_file lost the line after the long one: %q", readLines[1]) + } + + // The same file must remain greppable -- this is the side that always + // worked, and it is what read_file is now consistent with. + grep := runTool(t, grepFileTool, ctx, map[string]any{ + "pattern": "needle here", + "path": "bundle.min.js", + }) + if !strings.Contains(grep, "bundle.min.js:2:needle here") { + t.Errorf("grep_file result = %q, want the match on line 2", grep) + } + }) + + t.Run("list_files reports a symlink's target size, not the link's", func(t *testing.T) { + linkDir := filepath.Join(sessionPath, "linkdir") + if err := os.Mkdir(linkDir, 0755); err != nil { + t.Fatalf("failed to create linkdir: %v", err) + } + target := filepath.Join(linkDir, "target.txt") + if err := os.WriteFile(target, []byte(strings.Repeat("x", 5000)), 0644); err != nil { + t.Fatalf("failed to write symlink target: %v", err) + } + // A relative target, so the stored path string is far shorter than the + // file -- an Lstat-based size would report 10, not 5000. + if err := os.Symlink("target.txt", filepath.Join(linkDir, "file-link")); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + result := runTool(t, listFilesTool, ctx, map[string]any{"path": "linkdir"}) + if !strings.Contains(result, "file-link\t5000") { + t.Errorf("list_files result = %q, want file-link sized by its target (5000)", result) + } + }) +} diff --git a/go/core/pkg/env/kagent.go b/go/core/pkg/env/kagent.go index 35464cc4e..c189fcacb 100644 --- a/go/core/pkg/env/kagent.go +++ b/go/core/pkg/env/kagent.go @@ -78,6 +78,20 @@ var ( ComponentAgentRuntime, ) + // Registered here for `kagent env` CLI discoverability only -- the + // actual gate is read independently (raw os.Getenv, not via this var) + // in go/adk/pkg/tools/skills.go's enableFileSearchToolsEnv. The two + // literals are pinned together by that package's + // TestEnableFileSearchToolsEnvMatchesRegistry. + KagentEnableFileSearchTools = RegisterBoolVar( + "KAGENT_ENABLE_FILE_SEARCH_TOOLS", + false, + "When true, enables the list_files and grep_file skills tools, which let an agent "+ + "enumerate and search the filesystem under its session/skills roots without a "+ + "shell. Disabled by default; set on the Agent's env to opt in.", + ComponentAgentRuntime, + ) + StsWellKnownURI = RegisterStringVar( "STS_WELL_KNOWN_URI", "", diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/README.md b/python/packages/kagent-adk/src/kagent/adk/tools/README.md index b9ca2700d..78300dcd2 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/README.md +++ b/python/packages/kagent-adk/src/kagent/adk/tools/README.md @@ -29,7 +29,7 @@ app = App( ```python from kagent.adk.skills import SkillsTool -from kagent.adk.tools import BashTool, ReadFileTool, WriteFileTool, EditFileTool +from kagent.adk.tools import BashTool, ReadFileTool, WriteFileTool, EditFileTool, ListFilesTool, GrepFileTool agent = Agent( tools=[ @@ -38,6 +38,8 @@ agent = Agent( ReadFileTool(skills_directory="./skills"), WriteFileTool(), EditFileTool(), + ListFilesTool(skills_directory="./skills"), + GrepFileTool(skills_directory="./skills"), ] ) ``` @@ -123,6 +125,8 @@ description: Analyze CSV/Excel files | **ReadFile** | Read files with line numbers | `read_file("skills/data-analysis/config.json")` | | **WriteFile** | Create/overwrite files | `write_file("outputs/report.pdf", data)` | | **EditFile** | Precise string replacements | `edit_file("script.py", old="x", new="y")` | +| **ListFiles** | List a directory's contents | `list_files("skills/data-analysis")` | +| **GrepFile** | Search files by pattern | `grep_file("skills", pattern="analyze", recursive=True)` | ### Working Directory Structure @@ -179,6 +183,9 @@ return_artifacts(file_paths=["outputs/report.pdf"]) - Path traversal protection (no `..`) - Session isolation (each session has separate working directory) - File size limits (100 MB max) +- `grep_file` skips symlinked entries that resolve outside the directory being searched, so a symlink can't be used to read files outside the session's working directory +- `recursive=True` does not descend into symlinked subdirectories (this is standard Go/Python stdlib walk behavior, not something this tool adds) — since `skills/` is itself a symlink, a recursive search from the working directory root will not find matches inside it; pass `skills/...` as the path directly to search skill contents +- `list_files`/`grep_file` are disabled by default — they give an agent broad filesystem visibility, so they're opt-in. Set `KAGENT_ENABLE_FILE_SEARCH_TOOLS=true` on the agent's env to enable them (`read_file`/`write_file`/`edit_file`/`bash` are unaffected and always available in this Python runtime) **Bash tool:** diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/__init__.py b/python/packages/kagent-adk/src/kagent/adk/tools/__init__.py index 062f17e8e..cee479c00 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/__init__.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/__init__.py @@ -1,5 +1,5 @@ from .bash_tool import BashTool -from .file_tools import EditFileTool, ReadFileTool, WriteFileTool +from .file_tools import EditFileTool, GrepFileTool, ListFilesTool, ReadFileTool, WriteFileTool from .skill_tool import SkillsTool from .skills_plugin import add_skills_tool_to_agent from .skills_toolset import SkillsToolset @@ -11,5 +11,7 @@ "EditFileTool", "ReadFileTool", "WriteFileTool", + "ListFilesTool", + "GrepFileTool", "add_skills_tool_to_agent", ] diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py b/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py index 3b6215b87..189bd25a6 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py @@ -6,6 +6,9 @@ from __future__ import annotations +import asyncio +import concurrent.futures +import functools import logging from pathlib import Path from typing import Any, Dict @@ -15,9 +18,13 @@ from kagent.skills import ( edit_file_content, get_edit_file_description, + get_grep_file_description, + get_list_files_description, get_read_file_description, get_session_path, get_write_file_description, + grep_content, + list_dir_content, read_file_content, write_file_content, ) @@ -25,6 +32,19 @@ logger = logging.getLogger("kagent_adk." + __name__) +def _resolve_working_path(tool_context: ToolContext, path_str: str) -> tuple[Path, Path]: + """Resolve path_str relative to the session's working directory. + + Returns (resolved_path, working_dir); callers use working_dir to build + their allowed_root argument. + """ + working_dir = get_session_path(session_id=tool_context.session.id) + path = Path(path_str) + if not path.is_absolute(): + path = working_dir / path + return path.resolve(), working_dir + + class ReadFileTool(BaseTool): """Read files with line numbers for precise editing.""" @@ -71,11 +91,7 @@ async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> return "Error: No file path provided" try: - working_dir = get_session_path(session_id=tool_context.session.id) - path = Path(file_path_str) - if not path.is_absolute(): - path = working_dir / path - path = path.resolve() + path, working_dir = _resolve_working_path(tool_context, file_path_str) return read_file_content(path, offset, limit, allowed_root=[working_dir, Path(self.skills_directory)]) except (FileNotFoundError, IsADirectoryError, PermissionError, IOError) as e: @@ -120,11 +136,7 @@ async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> return "Error: No file path provided" try: - working_dir = get_session_path(session_id=tool_context.session.id) - path = Path(file_path_str) - if not path.is_absolute(): - path = working_dir / path - path = path.resolve() + path, working_dir = _resolve_working_path(tool_context, file_path_str) return write_file_content(path, content, allowed_root=working_dir) except (PermissionError, IOError) as e: @@ -133,6 +145,142 @@ async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> return error_msg +class ListFilesTool(BaseTool): + """List files and directories at a given path.""" + + def __init__(self, skills_directory: str | Path): + super().__init__( + name="list_files", + description=get_list_files_description(), + ) + self.skills_directory = Path(skills_directory).resolve() + if not self.skills_directory.exists(): + raise ValueError(f"Skills directory does not exist: {self.skills_directory}") + + def _get_declaration(self) -> types.FunctionDeclaration: + return types.FunctionDeclaration( + name=self.name, + description=self.description, + parameters=types.Schema( + type=types.Type.OBJECT, + properties={ + "path": types.Schema( + type=types.Type.STRING, + description="Directory path to list (absolute or relative to working directory); defaults to the working directory", + ), + }, + ), + ) + + async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> str: + """List the contents of a directory.""" + path_str = args.get("path", "").strip() or "." + + try: + path, working_dir = _resolve_working_path(tool_context, path_str) + + return list_dir_content(path, allowed_root=[working_dir, Path(self.skills_directory)]) + except (FileNotFoundError, NotADirectoryError, PermissionError, IOError) as e: + return f"Error listing {path_str}: {e}" + + +class GrepFileTool(BaseTool): + """Search for a regular expression pattern in a file or directory.""" + + # Bounds regex execution time: the pattern is agent-controlled, and Python's + # backtracking `re` engine can take catastrophically long on adversarial + # patterns (unlike Go's RE2-based regexp, which is linear-time). Note this + # only bounds the *caller's* wait -- CPython can't forcibly stop a running + # thread, so a pathological match keeps running in the background after + # the timeout fires. + _TIMEOUT_SECONDS = 30 + + # A small dedicated pool, rather than asyncio's shared default executor, + # so a hung or catastrophically slow match can only ever starve other + # grep_file calls -- not unrelated to_thread-based work elsewhere in the + # process (token counting, embeddings, other provider clients). Note + # ThreadPoolExecutor workers are non-daemon threads that CPython's atexit + # hook joins before the interpreter exits, so a permanently-stuck worker + # (e.g. from a pathological pattern) also blocks a clean process shutdown, + # not just steady-state grep_file availability -- bounded in practice by + # Kubernetes' terminationGracePeriodSeconds before SIGKILL. + _EXECUTOR = concurrent.futures.ThreadPoolExecutor(max_workers=4, thread_name_prefix="grep-file") + + def __init__(self, skills_directory: str | Path): + super().__init__( + name="grep_file", + description=get_grep_file_description(), + ) + self.skills_directory = Path(skills_directory).resolve() + if not self.skills_directory.exists(): + raise ValueError(f"Skills directory does not exist: {self.skills_directory}") + + def _get_declaration(self) -> types.FunctionDeclaration: + return types.FunctionDeclaration( + name=self.name, + description=self.description, + parameters=types.Schema( + type=types.Type.OBJECT, + properties={ + "pattern": types.Schema( + type=types.Type.STRING, + description="The regular expression pattern to search for", + ), + "path": types.Schema( + type=types.Type.STRING, + description="The file or directory path to search (absolute or relative to working directory)", + ), + "recursive": types.Schema( + type=types.Type.BOOLEAN, + description="Search directories recursively (default: false)", + ), + "ignore_case": types.Schema( + type=types.Type.BOOLEAN, + description="Ignore case when matching (default: false)", + ), + }, + required=["pattern", "path"], + ), + ) + + async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> str: + """Search a file or directory for a pattern.""" + pattern = args.get("pattern", "").strip() + path_str = args.get("path", "").strip() + recursive = args.get("recursive", False) + ignore_case = args.get("ignore_case", False) + + if not pattern: + return "Error: No pattern provided" + if not path_str: + return "Error: No file path provided" + + try: + path, working_dir = _resolve_working_path(tool_context, path_str) + + loop = asyncio.get_running_loop() + return await asyncio.wait_for( + loop.run_in_executor( + self._EXECUTOR, + functools.partial( + grep_content, + path, + pattern, + recursive=recursive, + ignore_case=ignore_case, + allowed_root=[working_dir, Path(self.skills_directory)], + ), + ), + timeout=self._TIMEOUT_SECONDS, + ) + except (TimeoutError, asyncio.TimeoutError): + # asyncio.TimeoutError is TimeoutError on Python >=3.11, but this + # package supports >=3.10 where they're distinct classes. + return f"Error searching {path_str}: pattern took too long to match (possible catastrophic backtracking); try a simpler pattern" + except (FileNotFoundError, IsADirectoryError, ValueError, PermissionError, IOError) as e: + return f"Error searching {path_str}: {e}" + + class EditFileTool(BaseTool): """Edit files by replacing exact string matches.""" @@ -181,11 +329,7 @@ async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> return "Error: No file path provided" try: - working_dir = get_session_path(session_id=tool_context.session.id) - path = Path(file_path_str) - if not path.is_absolute(): - path = working_dir / path - path = path.resolve() + path, working_dir = _resolve_working_path(tool_context, file_path_str) return edit_file_content(path, old_string, new_string, replace_all, allowed_root=working_dir) except (FileNotFoundError, IsADirectoryError, ValueError, PermissionError, IOError) as e: diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py b/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py index bf7e899da..45ba4f83b 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py @@ -5,8 +5,9 @@ from typing import Optional from google.adk.agents import BaseAgent, LlmAgent +from kagent.skills import file_search_tools_enabled -from ..tools import BashTool, EditFileTool, ReadFileTool, WriteFileTool +from ..tools import BashTool, EditFileTool, GrepFileTool, ListFilesTool, ReadFileTool, WriteFileTool from .skill_tool import SkillsTool logger = logging.getLogger("kagent_adk." + __name__) @@ -50,3 +51,19 @@ def add_skills_tool_to_agent( if "edit_file" not in existing_tool_names: agent.tools.append(EditFileTool()) logger.debug(f"Added edit file tool to agent: {agent.name}") + + # list_files/grep_file are opt-in: they give an agent broad filesystem + # visibility, so deployments enable them deliberately. Note this gate is + # theirs alone -- bash above is always registered. + if file_search_tools_enabled(): + if "list_files" not in existing_tool_names: + agent.tools.append(ListFilesTool(skills_directory)) + logger.debug(f"Added list files tool to agent: {agent.name}") + + if "grep_file" not in existing_tool_names: + agent.tools.append(GrepFileTool(skills_directory)) + logger.debug(f"Added grep file tool to agent: {agent.name}") + else: + logger.debug( + f"Omitting list_files/grep_file tools for agent: {agent.name} (KAGENT_ENABLE_FILE_SEARCH_TOOLS not enabled)" + ) diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py b/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py index e8555fff9..b6f60e140 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py @@ -12,8 +12,9 @@ from google.adk.agents.readonly_context import ReadonlyContext from google.adk.tools import BaseTool from google.adk.tools.base_toolset import BaseToolset +from kagent.skills import file_search_tools_enabled -from ..tools import BashTool, EditFileTool, ReadFileTool, WriteFileTool +from ..tools import BashTool, EditFileTool, GrepFileTool, ListFilesTool, ReadFileTool, WriteFileTool from .skill_tool import SkillsTool logger = logging.getLogger("kagent_adk." + __name__) @@ -27,7 +28,9 @@ class SkillsToolset(BaseToolset): 2. ReadFileTool - Read files with line numbers 3. WriteFileTool - Write/create files 4. EditFileTool - Edit files with precise replacements - 5. BashTool - Execute shell commands + 5. ListFilesTool - List files and directories + 6. GrepFileTool - Search file contents with a regular expression + 7. BashTool - Execute shell commands Skills provide specialized domain knowledge and scripts that the agent can use to solve complex tasks. The toolset enables discovery of available skills, @@ -50,6 +53,15 @@ def __init__(self, skills_directory: str | Path): self.read_file_tool = ReadFileTool(skills_directory) self.write_file_tool = WriteFileTool() self.edit_file_tool = EditFileTool() + # list_files/grep_file are opt-in: they give an agent broad + # filesystem visibility, so deployments enable them deliberately. + # Note this gate is theirs alone -- bash below is always registered. + # A single list (rather than two separately-nullable attributes) + # keeps "both present or both absent" a structural guarantee instead + # of a convention the two attributes have to be kept in sync by hand. + self._file_search_tools: list[BaseTool] = ( + [ListFilesTool(skills_directory), GrepFileTool(skills_directory)] if file_search_tools_enabled() else [] + ) self.bash_tool = BashTool(skills_directory) @override @@ -57,12 +69,14 @@ async def get_tools(self, readonly_context: Optional[ReadonlyContext] = None) -> """Get all skills tools. Returns: - List containing all skills tools: skills, read, write, edit, and bash. + List containing all skills tools: skills, read, write, edit, list, grep, and bash. + list/grep are omitted unless KAGENT_ENABLE_FILE_SEARCH_TOOLS is enabled. """ return [ self.skills_tool, self.read_file_tool, self.write_file_tool, self.edit_file_tool, + *self._file_search_tools, self.bash_tool, ] diff --git a/python/packages/kagent-adk/tests/unittests/test_file_tools.py b/python/packages/kagent-adk/tests/unittests/test_file_tools.py new file mode 100644 index 000000000..ea0eaa9d1 --- /dev/null +++ b/python/packages/kagent-adk/tests/unittests/test_file_tools.py @@ -0,0 +1,62 @@ +"""Tests for GrepFileTool's timeout protection against slow/catastrophic regex matching.""" + +from unittest.mock import patch + +import pytest +from kagent.skills import initialize_session_path + +from kagent.adk.tools.file_tools import GrepFileTool + + +class MockSession: + def __init__(self, session_id: str = "test-session-grep-timeout"): + self.id = session_id + + +class MockToolContext: + def __init__(self, session_id: str = "test-session-grep-timeout"): + self.session = MockSession(session_id) + + +@pytest.mark.asyncio +async def test_grep_file_tool_times_out_on_slow_match(tmp_path): + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + session_id = "test-session-grep-timeout" + initialize_session_path(session_id, str(skills_dir)) + + tool = GrepFileTool(skills_directory=str(skills_dir)) + tool._TIMEOUT_SECONDS = 0.05 + + def slow_grep(*args, **kwargs): + import time + + time.sleep(1) + return "no matches found" + + with patch("kagent.adk.tools.file_tools.grep_content", side_effect=slow_grep): + result = await tool.run_async( + args={"pattern": "foo", "path": "."}, + tool_context=MockToolContext(session_id), + ) + + assert "took too long" in result + + +@pytest.mark.asyncio +async def test_grep_file_tool_returns_normally_when_fast(tmp_path): + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + session_id = "test-session-grep-timeout-fast" + initialize_session_path(session_id, str(skills_dir)) + + tool = GrepFileTool(skills_directory=str(skills_dir)) + + with patch("kagent.adk.tools.file_tools.grep_content", return_value="match found") as mocked: + result = await tool.run_async( + args={"pattern": "foo", "path": "."}, + tool_context=MockToolContext(session_id), + ) + + assert result == "match found" + assert mocked.called diff --git a/python/packages/kagent-adk/tests/unittests/test_skills_plugin.py b/python/packages/kagent-adk/tests/unittests/test_skills_plugin.py new file mode 100644 index 000000000..d716a50e0 --- /dev/null +++ b/python/packages/kagent-adk/tests/unittests/test_skills_plugin.py @@ -0,0 +1,33 @@ +"""Tests for add_skills_tool_to_agent's list_files/grep_file feature-flag gating.""" + +from unittest.mock import patch + +from google.adk.agents import LlmAgent + +from kagent.adk.tools.skills_plugin import add_skills_tool_to_agent + + +def _tool_names(agent: LlmAgent) -> set[str]: + return {getattr(t, "name", None) for t in agent.tools} + + +def test_add_skills_tool_to_agent_omits_list_files_and_grep_file_by_default(tmp_path): + agent = LlmAgent(name="test_agent", model="gemini-2.0-flash", tools=[]) + + with patch.dict("os.environ", {}, clear=True): + add_skills_tool_to_agent(str(tmp_path), agent) + + names = _tool_names(agent) + assert {"skills", "read_file", "write_file", "edit_file", "bash"} <= names + assert "list_files" not in names + assert "grep_file" not in names + + +def test_add_skills_tool_to_agent_adds_list_files_and_grep_file_when_enabled(tmp_path): + agent = LlmAgent(name="test_agent", model="gemini-2.0-flash", tools=[]) + + with patch.dict("os.environ", {"KAGENT_ENABLE_FILE_SEARCH_TOOLS": "true"}, clear=True): + add_skills_tool_to_agent(str(tmp_path), agent) + + names = _tool_names(agent) + assert {"skills", "read_file", "write_file", "edit_file", "bash", "list_files", "grep_file"} <= names diff --git a/python/packages/kagent-skills/src/kagent/skills/__init__.py b/python/packages/kagent-skills/src/kagent/skills/__init__.py index 6e5e7b359..160778c66 100644 --- a/python/packages/kagent-skills/src/kagent/skills/__init__.py +++ b/python/packages/kagent-skills/src/kagent/skills/__init__.py @@ -4,6 +4,8 @@ generate_skills_tool_description, get_bash_description, get_edit_file_description, + get_grep_file_description, + get_list_files_description, get_read_file_description, get_write_file_description, ) @@ -15,6 +17,9 @@ from .shell import ( edit_file_content, execute_command, + file_search_tools_enabled, + grep_content, + list_dir_content, read_file_content, write_file_content, ) @@ -26,11 +31,16 @@ "read_file_content", "write_file_content", "edit_file_content", + "list_dir_content", + "grep_content", "execute_command", + "file_search_tools_enabled", "generate_skills_tool_description", "get_read_file_description", "get_write_file_description", "get_edit_file_description", + "get_list_files_description", + "get_grep_file_description", "get_bash_description", "initialize_session_path", "get_session_path", diff --git a/python/packages/kagent-skills/src/kagent/skills/prompts.py b/python/packages/kagent-skills/src/kagent/skills/prompts.py index 5be19165a..192feeca0 100644 --- a/python/packages/kagent-skills/src/kagent/skills/prompts.py +++ b/python/packages/kagent-skills/src/kagent/skills/prompts.py @@ -1,4 +1,5 @@ from .models import Skill +from .shell import file_search_tools_enabled def generate_skills_xml(skills: list[Skill]) -> str: @@ -89,10 +90,36 @@ def get_edit_file_description() -> str: """ +def get_list_files_description() -> str: + """Returns the standardized description for the list_files tool.""" + return """Lists files and directories at a given path. + +Usage: +- Provide a path (absolute or relative to your working directory); defaults to the working directory +- Directories are listed with a trailing "/"; files are followed by their size in bytes +- You can list skills/ directory, uploads/, outputs/, or any directory in your session +""" + + +def get_grep_file_description() -> str: + """Returns the standardized description for the grep_file tool.""" + return """Searches for a regular expression pattern in a file or directory. + +Usage: +- Provide a pattern and a path (absolute or relative to your working directory) +- Set recursive=true to search all files under a directory path +- Recursion does not follow symlinked subdirectories (e.g. skills/ is a symlink) - + point path directly at skills/ to search inside it +- Set ignore_case=true for case-insensitive matching +- Returns matching lines as path:line_number:content +- You can search the skills/ directory, uploads/, outputs/, or any file/directory in your session +""" + + def get_bash_description() -> str: """Returns the standardized description for the bash tool.""" # This combines the useful parts from both ADK and OpenAI descriptions - return """Execute bash commands in the skills environment with sandbox protection. + description = """Execute bash commands in the skills environment with sandbox protection. Working Directory & Structure: - Commands run in a temporary session directory: /tmp/kagent/{session_id}/ @@ -114,3 +141,12 @@ def get_bash_description() -> str: - python scripts: 60s - other commands: 30s """ + # Appended as a trailing paragraph rather than interpolated into the + # description above, so that long, free-form prose block stays a plain + # string -- not an f-string where a stray '{'/'}' added later could + # raise or silently corrupt output. + if file_search_tools_enabled(): + description += ( + "\nAlso available: list_files and grep_file, for exploring the filesystem without a full shell command.\n" + ) + return description diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index ceda59e9b..330d6c9dc 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import enum import logging import os import re @@ -13,6 +14,81 @@ # --- File Operation Tools --- +# Longest line read_file and grep_file will emit before truncating, counted in +# characters. Mirrors maxLineRunes in go/adk/pkg/tools/shell.go, and the +# "Lines longer than 2000 characters are truncated" both runtimes' read_file +# descriptions promise (see prompts.py). +_MAX_LINE_CHARS = 2000 + + +def _truncate_line(line: str) -> str: + """Shorten line to at most _MAX_LINE_CHARS characters, marking a cut with "...".""" + if len(line) > _MAX_LINE_CHARS: + return line[:_MAX_LINE_CHARS] + "..." + return line + + +class _WalkEntryAction(enum.Enum): + """How a recursive grep should treat one entry it walked.""" + + #: A regular, in-bounds file that should be grepped. + GREP = enum.auto() + #: Silently excluded by policy, not a read failure -- a non-regular file + #: (FIFO/socket/device), or a symlink whose target escapes the search root. + SKIP = enum.auto() + #: A genuine read/stat failure on this entry. + UNREADABLE = enum.auto() + + +def _classify_walk_entry(root: Path, entry: Path) -> tuple[_WalkEntryAction, Path | None]: + """Decide how grep_content should treat entry, given the resolved search root. + + This is the Python half of a contract the Go runtime implements as + classifyWalkEntry in go/adk/pkg/tools/grep.go. Both must sort a tree into + the same three outcomes, and reading them side by side is the only + practical way to confirm they still do -- so they share these names, this + argument order, and this return shape. Prefer changing both, or neither. + + They are not branch-for-branch identical, and should not be forced to be. + Go tests IsDir, EvalSymlinks, Stat and IsRegular separately because + filepath.WalkDir hands it directories and unresolvable links; here + os.walk yields only filenames, and Path.is_file() already collapses + "exists, resolves, and is a regular file" into one call. The outcomes + agree; the number of branches reaching them does not. + + One known divergence, not reachable through any case we could construct: + Go counts a failed Stat on a non-symlink as UNREADABLE, while + Path.is_file() swallows the OSError and lands here on SKIP. Reaching it + needs a stat that fails on an entry os.walk just listed -- EIO, a stale + NFS handle, a misbehaving FUSE mount. Symlink loops and broken links are + unaffected; both are UNREADABLE in either runtime. + + Returns the resolved path alongside GREP so the caller reads what was + just checked rather than re-resolving the symlink separately. + """ + # is_file() follows symlinks and checks S_ISREG, so a False covers two + # cases that deserve different treatment. + if not entry.is_file(): + # A broken symlink (or a symlink loop) is a genuine read failure. + # Counting it is what stops a tree of dangling links from reporting a + # confidently empty "no matches found". + if entry.is_symlink() and not entry.exists(): + return _WalkEntryAction.UNREADABLE, None + # A FIFO/socket/device is excluded by policy, not failure: opening one + # can block indefinitely, and grep has no business reading it. + return _WalkEntryAction.SKIP, None + + resolved = entry.resolve() + # Bound each entry by the directory actually being searched. The caller's + # allowed_root is the whole session plus the skills dir, so it alone would + # let a symlink here pull in a file from a sibling directory nobody asked + # to search. Containment against root subsumes it: root was itself + # validated against allowed_root, so anything under root is inside a root. + if not resolved.is_relative_to(root): + return _WalkEntryAction.SKIP, None + + return _WalkEntryAction.GREP, resolved + def _validate_path( file_path: Path, @@ -57,9 +133,7 @@ def read_file_content( result_lines = [] for i, line in enumerate(lines[start:end], start=start + 1): - if len(line) > 2000: - line = line[:2000] + "..." - result_lines.append(f"{i:6d}|{line}") + result_lines.append(f"{i:6d}|{_truncate_line(line)}") if not result_lines: return "File is empty." @@ -126,6 +200,134 @@ def edit_file_content( raise OSError(f"Error writing file {file_path}: {e}") from e +def list_dir_content(dir_path: Path, allowed_root: Path | list[Path] | None = None) -> str: + """Lists the entries of a directory, one per line. + + Directories are suffixed with "/"; files are followed by their size in bytes. + """ + dir_path = _validate_path(dir_path, allowed_root) + + if not dir_path.exists(): + raise FileNotFoundError(f"Directory not found: {dir_path}") + + if not dir_path.is_dir(): + raise NotADirectoryError(f"Path is not a directory: {dir_path}") + + entries = sorted(dir_path.iterdir(), key=lambda p: p.name) + if not entries: + return "Directory is empty." + + lines = [] + for entry in entries: + if entry.is_dir(): + lines.append(f"{entry.name}/") + continue + try: + size = entry.stat().st_size + except OSError: + lines.append(entry.name) + continue + lines.append(f"{entry.name}\t{size}") + + return "\n".join(lines) + + +def grep_content( + file_or_dir_path: Path, + pattern: str, + recursive: bool = False, + ignore_case: bool = False, + allowed_root: Path | list[Path] | None = None, +) -> str: + """Searches path for lines matching a regular expression pattern. + + If path is a directory, recursive must be true to search its files. + + pattern is untrusted, agent-controlled input: Python's backtracking `re` + engine can take catastrophically long on an adversarial pattern. This + function does not bound its own execution time -- callers must do so + (e.g. via a timeout around a thread/process offload) if the caller is + exposed to untrusted patterns. + """ + file_or_dir_path = _validate_path(file_or_dir_path, allowed_root) + + try: + compiled = re.compile(pattern, re.IGNORECASE if ignore_case else 0) + except re.error as e: + raise ValueError(f"invalid pattern: {e}") from e + + if not file_or_dir_path.exists(): + raise FileNotFoundError(f"Path not found: {file_or_dir_path}") + + def grep_file(file_path: Path) -> list[str]: + matches = [] + with file_path.open("r", encoding="utf-8", errors="replace") as f: + for line_num, line in enumerate(f, start=1): + line = line.rstrip("\n") + if compiled.search(line): + matches.append(f"{file_path}:{line_num}:{_truncate_line(line)}") + return matches + + results: list[str] = [] + skipped = 0 + if file_or_dir_path.is_dir(): + if not recursive: + raise IsADirectoryError(f"{file_or_dir_path} is a directory; set recursive=true to search directories") + + # os.walk (not rglob) so that a directory-level failure -- root or + # nested -- is observable via onerror. rglob() silently omits any + # directory it can't list, at any depth, with no hook to detect it; + # that let a nested unreadable subdirectory disappear from a + # recursive search with no signal at all, exactly the "confidently + # wrong empty result" failure mode skipped/the annotation below + # exists to prevent. followlinks defaults to False, so this doesn't + # descend into symlinked directories, matching grepFile's Go twin. + root_str = str(file_or_dir_path) + walk_errors: list[OSError] = [] + entries: list[Path] = [] + for dirpath, _dirnames, filenames in os.walk(file_or_dir_path, onerror=walk_errors.append): + entries.extend(Path(dirpath) / name for name in filenames) + + for walk_err in walk_errors: + if walk_err.filename == root_str: + # The search root itself couldn't be read: the search never + # actually ran, so surface a real error instead of a + # misleadingly confident "no matches found". + raise OSError(f"{file_or_dir_path} could not be read: {walk_err}") from walk_err + # A nested subdirectory that couldn't be read shouldn't abort + # matches already found in sibling directories -- just count it. + skipped += len(walk_errors) + + for entry in sorted(entries): + action, safe_entry = _classify_walk_entry(file_or_dir_path, entry) + if action is _WalkEntryAction.SKIP: + continue + if action is _WalkEntryAction.UNREADABLE: + skipped += 1 + continue + try: + results.extend(grep_file(safe_entry)) + except OSError: + # A read error on one file shouldn't abort matches already + # found elsewhere in the tree, but it also shouldn't look + # identical to a genuinely empty search -- hence the count. + skipped += 1 + else: + if not file_or_dir_path.is_file(): + raise OSError(f"{file_or_dir_path} is not a regular file") + results.extend(grep_file(file_or_dir_path)) + + if not results: + if skipped: + return f"no matches found ({skipped} entries could not be read)" + return "no matches found" + + output = "\n".join(results) + if skipped: + output += f"\n\n({skipped} entries could not be read)" + return output + + # --- Shell Operation Tools --- # Matches env-var names containing secret-related segments as whole @@ -159,6 +361,21 @@ def _sanitize_env(env: dict[str, str] | None = None) -> dict[str, str]: return {k: v for k, v in source.items() if k not in _SECRET_ENV_NAMES and not _SECRET_PATTERNS.search(k)} +_ENABLE_FILE_SEARCH_TOOLS_ENV = "KAGENT_ENABLE_FILE_SEARCH_TOOLS" + + +def file_search_tools_enabled() -> bool: + """Whether the list_files/grep_file tools are enabled. + + Opt-in (disabled by default): they let an agent enumerate and search the + filesystem under its session/skills roots without invoking a shell, so + deployments that want to grant that visibility do so deliberately rather + than having it enabled implicitly. Note this gate is theirs alone -- bash + is always registered. + """ + return os.environ.get(_ENABLE_FILE_SEARCH_TOOLS_ENV, "").strip().lower() in ("1", "t", "true") + + def _get_command_timeout_seconds(command: str) -> float: """Determine appropriate timeout for a command.""" if "python " in command or "python3 " in command: diff --git a/python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py b/python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py index 0ba9fad66..5206f5a72 100644 --- a/python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py +++ b/python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py @@ -1,4 +1,6 @@ +import concurrent.futures import json +import os import shutil import tempfile import textwrap @@ -11,11 +13,15 @@ discover_skills, edit_file_content, execute_command, + file_search_tools_enabled, + grep_content, + list_dir_content, load_skill_content, read_file_content, write_file_content, ) -from kagent.skills.shell import _sanitize_env +from kagent.skills.prompts import get_bash_description +from kagent.skills.shell import _classify_walk_entry, _sanitize_env, _WalkEntryAction @pytest.fixture @@ -156,6 +162,42 @@ async def mock_exec(*args, **kwargs): assert args == ("bash", "-c", injection_payload) +@pytest.mark.parametrize( + "value,expected", + [ + (None, False), + ("", False), + ("false", False), + ("0", False), + ("no", False), + ("true", True), + ("TRUE", True), + ("True", True), + ("1", True), + ("t", True), + ("T", True), + ], +) +def test_file_search_tools_enabled(value, expected): + """list_files/grep_file are disabled by default and opt-in via the env var.""" + env = {} if value is None else {"KAGENT_ENABLE_FILE_SEARCH_TOOLS": value} + with patch.dict("os.environ", env, clear=True): + assert file_search_tools_enabled() is expected + + +def test_get_bash_description_omits_file_search_tools_by_default(): + with patch.dict("os.environ", {}, clear=True): + desc = get_bash_description() + assert "list_files" not in desc + assert "grep_file" not in desc + + +def test_get_bash_description_mentions_file_search_tools_when_enabled(): + with patch.dict("os.environ", {"KAGENT_ENABLE_FILE_SEARCH_TOOLS": "true"}, clear=True): + desc = get_bash_description() + assert "list_files and grep_file" in desc + + # --- Path traversal tests --- @@ -195,6 +237,29 @@ def test_read_file_allows_path_inside_root(tmp_path): assert "hello world" in result +def test_read_file_truncates_a_long_line_without_failing_the_file(tmp_path): + """A line past the truncation limit is shortened, not fatal. + + The Go twin of this (TestReadFileContent_LongLineIsTruncatedNotFatal) is + the one that actually caught a bug: Go's scanner had a hard 64KB cap that + failed the entire read. Keeping both pins the promise each runtime's + read_file description makes -- "Lines longer than 2000 characters are + truncated" -- to the same observable behavior. + """ + f = tmp_path / "bundle.min.js" + f.write_text("a" * 100_000 + "\nsecond line\n") + + result = read_file_content(f, allowed_root=tmp_path) + + lines = result.split("\n") + assert len(lines) == 2 + body = lines[0].split("|", 1)[1] + assert body.endswith("...") + assert len(body.removesuffix("...")) == 2000 + # The whole point: the rest of the file survives. + assert "second line" in lines[1] + + def test_read_file_allows_multiple_roots(tmp_path): """Read should succeed when the file is inside any of the allowed roots.""" skills_dir = tmp_path / "skills" @@ -238,6 +303,394 @@ def test_edit_file_blocks_path_traversal(tmp_path): outside_file.unlink(missing_ok=True) +# --- list_dir_content / grep_content tests --- + + +def test_list_dir_content_lists_entries(tmp_path): + (tmp_path / "b.txt").write_text("hello") + (tmp_path / "a-subdir").mkdir() + + result = list_dir_content(tmp_path) + assert "a-subdir/" in result + assert "b.txt\t5" in result + + +def test_list_dir_content_empty_directory(tmp_path): + assert list_dir_content(tmp_path) == "Directory is empty." + + +def test_list_dir_content_nonexistent_path(tmp_path): + with pytest.raises(FileNotFoundError): + list_dir_content(tmp_path / "does-not-exist") + + +def test_list_dir_content_blocks_path_traversal(tmp_path): + outside = tmp_path.parent / "outside-dir" + outside.mkdir(exist_ok=True) + try: + with pytest.raises(PermissionError, match="outside the allowed director"): + list_dir_content(outside, allowed_root=tmp_path) + finally: + shutil.rmtree(outside, ignore_errors=True) + + +def test_grep_content_finds_match(tmp_path): + f = tmp_path / "a.txt" + f.write_text("hello world\nFOO bar\n") + + result = grep_content(f, "hello") + assert "a.txt:1:hello world" in result + + +def test_grep_content_no_matches(tmp_path): + f = tmp_path / "a.txt" + f.write_text("hello world\n") + + assert grep_content(f, "nope") == "no matches found" + + +def test_grep_content_ignore_case(tmp_path): + f = tmp_path / "a.txt" + f.write_text("FOO bar\n") + + result = grep_content(f, "foo", ignore_case=True) + assert "FOO bar" in result + + +def test_grep_content_directory_requires_recursive(tmp_path): + (tmp_path / "a.txt").write_text("foo\n") + + with pytest.raises(IsADirectoryError, match="set recursive=true"): + grep_content(tmp_path, "foo") + + +def test_grep_content_recursive_searches_subdirectories(tmp_path): + (tmp_path / "a.txt").write_text("hello\n") + sub = tmp_path / "sub" + sub.mkdir() + (sub / "b.txt").write_text("another foo line\n") + + result = grep_content(tmp_path, "foo", recursive=True) + assert "b.txt:1:another foo line" in result + + +def test_grep_content_invalid_pattern(tmp_path): + f = tmp_path / "a.txt" + f.write_text("hello\n") + + with pytest.raises(ValueError, match="invalid pattern"): + grep_content(f, "(") + + +def test_grep_content_blocks_path_traversal(tmp_path): + outside = tmp_path.parent / "outside.txt" + outside.write_text("secret") + + try: + with pytest.raises(PermissionError, match="outside the allowed director"): + grep_content(outside, "secret", allowed_root=tmp_path) + finally: + outside.unlink(missing_ok=True) + + +def test_grep_content_recursive_skips_symlinks_that_escape_root(tmp_path): + outside_dir = tmp_path.parent / "grep_symlink_outside" + outside_dir.mkdir(exist_ok=True) + secret = outside_dir / "secret.txt" + secret.write_text("top secret foo\n") + + sub = tmp_path / "sub" + sub.mkdir() + link = sub / "escape.txt" + + try: + link.symlink_to(secret) + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported") + + try: + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + assert "top secret" not in result + finally: + link.unlink(missing_ok=True) + secret.unlink(missing_ok=True) + outside_dir.rmdir() + + +def test_grep_content_recursive_skips_symlinks_that_escape_the_searched_directory(tmp_path): + """A symlink may not pull in a file from outside the directory being searched. + + allowed_root in production is the whole session dir plus the skills dir -- + wider than the search root -- so validating against it alone would let a + link in the searched subdirectory read a sibling the caller never asked + about. Go bounds each entry by the search root; this pins the same rule. + """ + sibling = tmp_path / "sibling" + sibling.mkdir() + (sibling / "secret.txt").write_text("sibling foo\n") + + searched = tmp_path / "searched" + searched.mkdir() + (searched / "own.txt").write_text("own foo\n") + link = searched / "escape.txt" + + try: + link.symlink_to(sibling / "secret.txt") + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported") + + # allowed_root is tmp_path (the shared parent), so the link's target IS + # inside the sandbox -- only the search-root check can exclude it. + result = grep_content(searched, "foo", recursive=True, allowed_root=tmp_path) + + assert "own foo" in result + assert "sibling foo" not in result + + +def test_grep_content_counts_broken_symlink_as_unreadable(tmp_path): + """A dangling link is a genuine read failure and must not vanish silently.""" + (tmp_path / "match.txt").write_text("foo readable\n") + link = tmp_path / "broken.txt" + + try: + link.symlink_to(tmp_path / "does-not-exist.txt") + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported") + + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + + assert "match.txt:1:foo readable" in result + assert "could not be read" in result + + +def test_classify_walk_entry_covers_each_outcome(tmp_path): + """Pin the three-way split directly, not just through grep_content. + + This is the Python half of the contract go/adk/pkg/tools/grep.go states as + classifyWalkEntry. Asserting it at the unit level is what makes the two + runtimes comparable side by side: same three outcomes, same inputs, same + answers. A divergence should fail here rather than surface later as a + grep that quietly returns different results in one runtime. + """ + root = tmp_path / "root" + root.mkdir() + + plain = root / "plain.txt" + plain.write_text("hello\n") + + inside_link = root / "inside-link" + inside_link.symlink_to(plain) + + outside = tmp_path / "outside.txt" + outside.write_text("secret\n") + escaping_link = root / "escaping-link" + escaping_link.symlink_to(outside) + + broken_link = root / "broken-link" + broken_link.symlink_to(tmp_path / "does-not-exist") + + assert _classify_walk_entry(root, plain) == (_WalkEntryAction.GREP, plain.resolve()) + # A symlink that stays inside the root is greppable, and resolves through + # to its target so the caller reads what was just checked. + assert _classify_walk_entry(root, inside_link) == (_WalkEntryAction.GREP, plain.resolve()) + # Escaping the searched directory is a policy exclusion, so it is silent. + assert _classify_walk_entry(root, escaping_link) == (_WalkEntryAction.SKIP, None) + # A dangling link is a real read failure and must be counted. + assert _classify_walk_entry(root, broken_link) == (_WalkEntryAction.UNREADABLE, None) + # A symlink loop is the same class of failure as a dangling link. + loop = root / "loop-link" + loop.symlink_to(loop) + assert _classify_walk_entry(root, loop) == (_WalkEntryAction.UNREADABLE, None) + + +def test_classify_walk_entry_treats_a_fifo_as_a_silent_skip(tmp_path): + """A FIFO is excluded by policy, not failure, so it must not be counted.""" + fifo_path = tmp_path / "pipe" + try: + os.mkfifo(fifo_path) + except (AttributeError, NotImplementedError, OSError): + pytest.skip("FIFOs not supported") + + assert _classify_walk_entry(tmp_path, fifo_path) == (_WalkEntryAction.SKIP, None) + + +def test_grep_content_does_not_count_fifo_as_unreadable(tmp_path): + """A FIFO is excluded by policy, not failure -- it must stay silent. + + Counting it would make every session dir containing a pipe report a + spurious "N entries could not be read", which is exactly the confusing + signal the annotation exists to avoid. + """ + (tmp_path / "match.txt").write_text("foo readable\n") + fifo_path = tmp_path / "pipe" + try: + os.mkfifo(fifo_path) + except (AttributeError, NotImplementedError, OSError): + pytest.skip("FIFOs not supported") + + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + + assert "match.txt:1:foo readable" in result + assert "could not be read" not in result + + +def _call_with_timeout(fn, *args, timeout=5, **kwargs): + """Run fn in a worker thread and fail loudly if it doesn't return in time. + + The whole point of a regression test for a hang bug is that the test + itself must not be hangable: calling grep_content directly on the test + thread would, on a regression, block the entire pytest run with no + diagnostic (no pytest-timeout plugin is configured for this package). + """ + with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor: + future = executor.submit(fn, *args, **kwargs) + return future.result(timeout=timeout) + + +def test_grep_content_recursive_skips_fifo_and_finds_matches_around_it(tmp_path): + (tmp_path / "aaa_before.txt").write_text("foo before\n") + fifo_path = tmp_path / "mmm_pipe" + try: + os.mkfifo(fifo_path) + except (AttributeError, OSError): + pytest.skip("FIFOs not supported") + (tmp_path / "zzz_after.txt").write_text("foo after\n") + + try: + result = _call_with_timeout(grep_content, tmp_path, "foo", recursive=True, allowed_root=tmp_path) + except concurrent.futures.TimeoutError: + pytest.fail("grep_content hung on a FIFO instead of skipping it") + assert "aaa_before.txt:1:foo before" in result + assert "zzz_after.txt:1:foo after" in result + + +def test_grep_content_single_target_fifo_raises_instead_of_hanging(tmp_path): + fifo_path = tmp_path / "pipe" + try: + os.mkfifo(fifo_path) + except (AttributeError, OSError): + pytest.skip("FIFOs not supported") + + try: + with pytest.raises(OSError, match="not a regular file"): + _call_with_timeout(grep_content, fifo_path, "foo", allowed_root=tmp_path) + except concurrent.futures.TimeoutError: + pytest.fail("grep_content hung opening a FIFO directly instead of erroring") + + +def test_grep_content_no_matches_found_is_annotated_when_a_file_could_not_be_read(tmp_path): + if os.geteuid() == 0: + pytest.skip("root bypasses file permissions; cannot exercise this case") + + # An unreadable *file* (listed fine, but fails to open) exercises the + # skip-counting path via grep_file's own except OSError handler. + unreadable = tmp_path / "hidden.txt" + unreadable.write_text("foo hidden\n") + unreadable.chmod(0o000) + + try: + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + finally: + unreadable.chmod(0o644) + + assert "no matches found" in result + assert "could not be read" in result + + +def test_grep_content_recursive_does_not_abort_or_discard_matches_on_an_unreadable_subdirectory(tmp_path): + if os.geteuid() == 0: + pytest.skip("root bypasses directory permissions; cannot exercise this case") + + ok_sub = tmp_path / "aaa_ok" + ok_sub.mkdir() + (ok_sub / "match.txt").write_text("foo readable\n") + + noperm_sub = tmp_path / "mmm_noperm" + noperm_sub.mkdir() + (noperm_sub / "hidden.txt").write_text("foo hidden\n") + noperm_sub.chmod(0o000) + + try: + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + finally: + noperm_sub.chmod(0o755) + + assert "match.txt:1:foo readable" in result + + +def test_grep_content_annotates_skip_count_alongside_real_matches(tmp_path): + """A skip count found alongside real matches must not be silently dropped.""" + if os.geteuid() == 0: + pytest.skip("root bypasses directory permissions; cannot exercise this case") + + ok_sub = tmp_path / "aaa_ok" + ok_sub.mkdir() + (ok_sub / "match.txt").write_text("foo readable\n") + + noperm_sub = tmp_path / "mmm_noperm" + noperm_sub.mkdir() + (noperm_sub / "hidden.txt").write_text("foo hidden\n") + noperm_sub.chmod(0o000) + + try: + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + finally: + noperm_sub.chmod(0o755) + + assert "match.txt:1:foo readable" in result + assert "could not be read" in result + + +def test_grep_content_no_matches_found_is_annotated_when_a_subdirectory_could_not_be_read(tmp_path): + if os.geteuid() == 0: + pytest.skip("root bypasses directory permissions; cannot exercise this case") + + # The only match lives inside a subdirectory that can't be listed. Before + # switching from rglob() (which silently omits directories it can't + # list, at any depth, with no hook to observe the failure) to os.walk's + # onerror callback, this returned a bare "no matches found" -- a + # confidently wrong empty result -- with no indication anything was + # skipped. + noperm_sub = tmp_path / "noperm" + noperm_sub.mkdir() + (noperm_sub / "hidden.txt").write_text("foo hidden\n") + noperm_sub.chmod(0o000) + + try: + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + finally: + noperm_sub.chmod(0o755) + + assert "no matches found" in result + assert "could not be read" in result + + +def test_grep_content_recursive_on_fully_unreadable_root_raises_instead_of_empty_result(tmp_path): + if os.geteuid() == 0: + pytest.skip("root bypasses directory permissions; cannot exercise this case") + + (tmp_path / "hidden.txt").write_text("foo hidden\n") + tmp_path.chmod(0o000) + + try: + with pytest.raises(OSError, match="could not be read"): + grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + finally: + tmp_path.chmod(0o755) + + +def test_grep_content_truncates_long_matched_lines(tmp_path): + long_line = "foo " + "x" * 3000 + f = tmp_path / "long.txt" + f.write_text(long_line + "\n") + + result = grep_content(f, "foo", allowed_root=tmp_path) + assert result.endswith("...") + assert long_line not in result + matched_line = result.split(":", 2)[-1] + assert len(matched_line) < 2100 + + def test_skill_discovery_and_loading(skill_test_env: Path): """ Tests the core logic of discovering a skill and loading its instructions.