From f3d4e40cac8557b1bb5d199e05828dff4cba5317 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 15 Jul 2026 16:55:28 -0400 Subject: [PATCH 1/5] feat: add list_files and grep_file skills tools Adds native, in-process list_files and grep_file tools to both the Go and Python agent runtimes, alongside the existing read_file/write_file/ edit_file/bash skills tools. Gives agents safe, non-privileged file visibility without depending on bash, which some deployments disable for privilege/security reasons. Also fixes two related bugs found while implementing and testing this: - Go: a symlink-resolution inconsistency in resolveReadPath/ resolveWritePath/resolveEditPath could reject valid paths under a symlinked session root. - Go: NewSkillsTools failed entirely (dropping all tools) when the bash command executor couldn't be constructed, instead of omitting bash. Signed-off-by: brandonkeung --- go/adk/pkg/skills/shell.go | 95 ++++++++++++ go/adk/pkg/skills/shell_test.go | 110 +++++++++++++ go/adk/pkg/tools/skills.go | 120 ++++++++++++--- go/adk/pkg/tools/skills_test.go | 145 +++++++++++++++++- .../src/kagent/adk/tools/__init__.py | 4 +- .../src/kagent/adk/tools/file_tools.py | 117 ++++++++++++++ .../src/kagent/adk/tools/skills_plugin.py | 10 +- .../src/kagent/adk/tools/skills_toolset.py | 12 +- .../src/kagent/skills/__init__.py | 8 + .../src/kagent/skills/prompts.py | 24 +++ .../kagent-skills/src/kagent/skills/shell.py | 78 ++++++++++ .../tests/unittests/test_skill_execution.py | 92 +++++++++++ 12 files changed, 788 insertions(+), 27 deletions(-) diff --git a/go/adk/pkg/skills/shell.go b/go/adk/pkg/skills/shell.go index d69ffe619b..a302f5610d 100644 --- a/go/adk/pkg/skills/shell.go +++ b/go/adk/pkg/skills/shell.go @@ -5,9 +5,11 @@ import ( "bytes" "context" "fmt" + "io/fs" "os" "os/exec" "path/filepath" + "regexp" "strings" "time" ) @@ -108,6 +110,99 @@ 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 "", 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 + } + + 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 +} + +// GrepContent searches path for lines matching a regular expression pattern. +// If path is a directory, recursive must be true to search its files. +func GrepContent(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 "", err + } + + var result strings.Builder + grepFile := func(filePath string) error { + file, err := os.Open(filePath) + if err != nil { + return err + } + defer file.Close() + + scanner := bufio.NewScanner(file) + lineNum := 1 + for scanner.Scan() { + if line := scanner.Text(); re.MatchString(line) { + fmt.Fprintf(&result, "%s:%d:%s\n", filePath, lineNum, line) + } + lineNum++ + } + return scanner.Err() + } + + if info.IsDir() { + if !recursive { + return "", fmt.Errorf("%q is a directory; set recursive=true to search directories", path) + } + err = filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + return grepFile(p) + }) + } else { + err = grepFile(path) + } + if err != nil { + return "", err + } + + if result.Len() == 0 { + return "no matches found", nil + } + + return strings.TrimSuffix(result.String(), "\n"), nil +} + func resolveSRTSettingsArgs() ([]string, error) { settingsPath := strings.TrimSpace(os.Getenv(srtSettingsPathEnv)) if settingsPath == "" { diff --git a/go/adk/pkg/skills/shell_test.go b/go/adk/pkg/skills/shell_test.go index be7f9d6c68..d3fac188ca 100644 --- a/go/adk/pkg/skills/shell_test.go +++ b/go/adk/pkg/skills/shell_test.go @@ -304,6 +304,116 @@ 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") + } + }) +} + +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(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(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(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(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(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(filepath.Join(tmpDir, "a.txt"), "(", false, false); err == nil { + t.Fatal("expected error for invalid regex pattern") + } + }) +} + 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 d549781e28..91632af8cc 100644 --- a/go/adk/pkg/tools/skills.go +++ b/go/adk/pkg/tools/skills.go @@ -46,6 +46,22 @@ 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 +- 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: @@ -96,6 +112,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"` +} + func NewSkillsTools(skillsDirectory string) ([]tool.Tool, error) { skillsDirectory = strings.TrimSpace(skillsDirectory) if skillsDirectory == "" { @@ -114,10 +141,6 @@ func NewSkillsTools(skillsDirectory string) ([]tool.Tool, error) { if err != nil { return nil, fmt.Errorf("failed to discover skills: %w", err) } - commandExecutor, err := skillruntime.NewCommandExecutorFromEnv() - if err != nil { - return nil, fmt.Errorf("failed to configure bash sandbox: %w", err) - } skillsTool, err := functiontool.New(functiontool.Config{ Name: "skills", @@ -199,31 +222,86 @@ func NewSkillsTools(skillsDirectory string) ([]tool.Tool, error) { return nil, fmt.Errorf("failed to create edit_file tool: %w", err) } - bashTool, err := functiontool.New(functiontool.Config{ - Name: "bash", - Description: bashDescription, - }, func(ctx adkagent.Context, in bashInput) (string, error) { - command := strings.TrimSpace(in.Command) - if command == "" { - return "Error: No command provided", nil + 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 := skillruntime.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 } - sessionPath, err := skillruntime.GetSessionPath(ctx.SessionID(), absSkillsDir) + path, err := resolveReadPath(ctx.SessionID(), absSkillsDir, in.Path) if err != nil { - return fmt.Sprintf("Error executing command %q: %v", command, err), nil + return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil } - result, err := commandExecutor.ExecuteCommand(ctx, command, sessionPath) + content, err := skillruntime.GrepContent(path, in.Pattern, in.Recursive, in.IgnoreCase) if err != nil { - return fmt.Sprintf("Error executing command %q: %v", command, err), nil + return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil } - return result, nil + return content, nil }) if err != nil { - return nil, fmt.Errorf("failed to create bash tool: %w", err) + return nil, fmt.Errorf("failed to create grep_file tool: %w", err) + } + + tools := []tool.Tool{skillsTool, readFileTool, writeFileTool, editFileTool, listFilesTool, grepFileTool} + + // bash requires the sandbox-runtime (KAGENT_SRT_SETTINGS_PATH); when that's not + // configured (e.g. bash is intentionally disabled), skip only this tool rather + // than failing the whole toolset. + if commandExecutor, err := skillruntime.NewCommandExecutorFromEnv(); err == nil { + bashTool, err := functiontool.New(functiontool.Config{ + Name: "bash", + Description: bashDescription, + }, func(ctx adkagent.Context, in bashInput) (string, error) { + command := strings.TrimSpace(in.Command) + if command == "" { + return "Error: No command provided", nil + } + + sessionPath, err := skillruntime.GetSessionPath(ctx.SessionID(), absSkillsDir) + if err != nil { + return fmt.Sprintf("Error executing command %q: %v", command, err), nil + } + + result, err := commandExecutor.ExecuteCommand(ctx, command, sessionPath) + if err != nil { + return fmt.Sprintf("Error executing command %q: %v", command, err), nil + } + return result, nil + }) + if err != nil { + return nil, fmt.Errorf("failed to create bash tool: %w", err) + } + tools = append(tools, bashTool) } - return []tool.Tool{skillsTool, readFileTool, writeFileTool, editFileTool, bashTool}, nil + return tools, nil } func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, error) { @@ -242,7 +320,7 @@ func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, return "", err } - sessionRoot, err := filepath.Abs(sessionPath) + sessionRoot, err := filepath.EvalSymlinks(sessionPath) if err != nil { return "", err } @@ -274,7 +352,7 @@ func resolveEditPath(sessionID, skillsDirectory, requestedPath string) (string, return "", err } - sessionRoot, err := filepath.Abs(sessionPath) + sessionRoot, err := filepath.EvalSymlinks(sessionPath) if err != nil { return "", err } @@ -301,7 +379,7 @@ func resolveWritePath(sessionID, skillsDirectory, requestedPath string) (string, return "", err } - sessionRoot, err := filepath.Abs(sessionPath) + sessionRoot, err := filepath.EvalSymlinks(sessionPath) if err != nil { return "", err } diff --git a/go/adk/pkg/tools/skills_test.go b/go/adk/pkg/tools/skills_test.go index 54fb692ed9..0ecd4696a5 100644 --- a/go/adk/pkg/tools/skills_test.go +++ b/go/adk/pkg/tools/skills_test.go @@ -6,8 +6,50 @@ import ( "path/filepath" "strings" "testing" + + skillruntime "github.com/kagent-dev/kagent/go/adk/pkg/skills" + adkagent "google.golang.org/adk/v2/agent" + "google.golang.org/adk/v2/tool" + "google.golang.org/adk/v2/tool/toolconfirmation" ) +// 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()) + } + result, err := runner.Run(ctx, args) + if err != nil { + t.Fatalf("%s.Run() error = %v", tl.Name(), err) + } + text, ok := result["result"].(string) + if !ok { + t.Fatalf("%s.Run() result = %#v, want map with string \"result\"", tl.Name(), result) + } + return text +} + func TestResolveReadPath_AllowsSymlinkedSkillsDirectory(t *testing.T) { t.Setenv("TMPDIR", t.TempDir()) skillsDir := t.TempDir() @@ -68,9 +110,110 @@ description: Demo skill. got[tool.Name()] = true } - for _, name := range []string{"skills", "read_file", "write_file", "edit_file", "bash"} { + for _, name := range []string{"skills", "read_file", "write_file", "edit_file", "list_files", "grep_file", "bash"} { if !got[name] { t.Errorf("expected tool %q to be present", name) } } } + +func TestNewSkillsTools_OmitsBashWithoutSRTSettings(t *testing.T) { + skillsDir := t.TempDir() + t.Setenv("KAGENT_SRT_SETTINGS_PATH", "") + + tools, err := NewSkillsTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillsTools() error = %v, want nil (bash should be omitted, not fatal)", err) + } + + got := map[string]bool{} + for _, tool := range tools { + got[tool.Name()] = true + } + + for _, name := range []string{"skills", "read_file", "write_file", "edit_file", "list_files", "grep_file"} { + if !got[name] { + t.Errorf("expected tool %q to be present even without SRT settings", name) + } + } + if got["bash"] { + t.Error("expected bash tool to be omitted without SRT settings") + } +} + +// 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 NewSkillsTools 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_SRT_SETTINGS_PATH", "") + + tools, err := NewSkillsTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillsTools() error = %v", err) + } + + var listFilesTool, grepFileTool tool.Tool + for _, tl := range tools { + switch tl.Name() { + case "list_files": + listFilesTool = tl + case "grep_file": + grepFileTool = tl + } + } + if listFilesTool == nil || grepFileTool == nil { + t.Fatal("expected list_files and grep_file tools to be present") + } + + sessionID := fmt.Sprintf("%s-session", t.Name()) + sessionPath, err := skillruntime.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) + } + }) +} 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 716b38b0fd..d73d43ec0d 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 .share_tools import CreateShareLinkTool, DeleteShareLinkTool, ListShareLinksTool from .skill_tool import SkillsTool from .skills_plugin import add_skills_tool_to_agent @@ -12,6 +12,8 @@ "EditFileTool", "ReadFileTool", "WriteFileTool", + "ListFilesTool", + "GrepFileTool", "add_skills_tool_to_agent", "CreateShareLinkTool", "ListShareLinksTool", 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 3b6215b876..9c49dab3cf 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 @@ -15,9 +15,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, ) @@ -133,6 +137,119 @@ 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: + working_dir = get_session_path(session_id=tool_context.session.id) + path = Path(path_str) + if not path.is_absolute(): + path = working_dir / path + path = path.resolve() + + 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.""" + + 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: + working_dir = get_session_path(session_id=tool_context.session.id) + path = Path(path_str) + if not path.is_absolute(): + path = working_dir / path + path = path.resolve() + + return grep_content( + path, + pattern, + recursive=recursive, + ignore_case=ignore_case, + allowed_root=[working_dir, Path(self.skills_directory)], + ) + 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.""" 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 bf7e899da7..752aa3a9ba 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 @@ -6,7 +6,7 @@ from google.adk.agents import BaseAgent, LlmAgent -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 +50,11 @@ 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}") + + 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}") 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 e8555fff9e..de0e552f83 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 @@ -13,7 +13,7 @@ from google.adk.tools import BaseTool from google.adk.tools.base_toolset import BaseToolset -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 +27,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 +52,8 @@ def __init__(self, skills_directory: str | Path): self.read_file_tool = ReadFileTool(skills_directory) self.write_file_tool = WriteFileTool() self.edit_file_tool = EditFileTool() + self.list_files_tool = ListFilesTool(skills_directory) + self.grep_file_tool = GrepFileTool(skills_directory) self.bash_tool = BashTool(skills_directory) @override @@ -57,12 +61,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. """ return [ self.skills_tool, self.read_file_tool, self.write_file_tool, self.edit_file_tool, + self.list_files_tool, + self.grep_file_tool, self.bash_tool, ] diff --git a/python/packages/kagent-skills/src/kagent/skills/__init__.py b/python/packages/kagent-skills/src/kagent/skills/__init__.py index 6e5e7b359b..9cb9081c5b 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,8 @@ from .shell import ( edit_file_content, execute_command, + grep_content, + list_dir_content, read_file_content, write_file_content, ) @@ -26,11 +30,15 @@ "read_file_content", "write_file_content", "edit_file_content", + "list_dir_content", + "grep_content", "execute_command", "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 5be19165a4..a458c0448c 100644 --- a/python/packages/kagent-skills/src/kagent/skills/prompts.py +++ b/python/packages/kagent-skills/src/kagent/skills/prompts.py @@ -89,6 +89,30 @@ 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 +- 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 diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index 6c89d8df3b..542b9f543f 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -126,6 +126,84 @@ 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. + """ + 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}:{line}") + return matches + + results: list[str] = [] + 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") + for entry in sorted(file_or_dir_path.rglob("*")): + if entry.is_file(): + results.extend(grep_file(entry)) + else: + results.extend(grep_file(file_or_dir_path)) + + if not results: + return "no matches found" + + return "\n".join(results) + + # --- Shell Operation Tools --- # Matches env-var names containing secret-related segments as whole 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 4dcdd9dc07..eb69afdaa9 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 @@ -12,6 +12,8 @@ discover_skills, edit_file_content, execute_command, + grep_content, + list_dir_content, load_skill_content, read_file_content, write_file_content, @@ -280,6 +282,96 @@ 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_skill_discovery_and_loading(skill_test_env: Path): """ Tests the core logic of discovering a skill and loading its instructions. From 27fb17cb31e2c36f5ce6e5e9977c194f94fb470d Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 15 Jul 2026 17:09:20 -0400 Subject: [PATCH 2/5] fix: address Copilot review findings on grep_file/list_files - Skip symlinked entries that resolve outside the searched root during recursive grep_file, in both the Go and Python implementations. A symlink inside an otherwise-jailed directory (e.g. from an untrusted skill package) could previously be followed to read file contents outside the intended sandbox. - Bound the Go grep scanner's line buffer (was capped at the default 64KiB bufio.Scanner token size, which errored on long lines such as minified JSON). - Reject an empty path explicitly in the Go grep_file tool instead of surfacing a confusing "no file path provided" error from deeper in the call stack. Signed-off-by: brandonkeung --- go/adk/pkg/skills/shell.go | 16 +++++++++++++ go/adk/pkg/skills/shell_test.go | 23 ++++++++++++++++++ go/adk/pkg/tools/skills.go | 3 +++ .../kagent-skills/src/kagent/skills/shell.py | 12 ++++++++-- .../tests/unittests/test_skill_execution.py | 24 +++++++++++++++++++ 5 files changed, 76 insertions(+), 2 deletions(-) diff --git a/go/adk/pkg/skills/shell.go b/go/adk/pkg/skills/shell.go index a302f5610d..38c51992f5 100644 --- a/go/adk/pkg/skills/shell.go +++ b/go/adk/pkg/skills/shell.go @@ -166,6 +166,7 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro defer file.Close() scanner := bufio.NewScanner(file) + scanner.Buffer(make([]byte, 64*1024), 1024*1024) lineNum := 1 for scanner.Scan() { if line := scanner.Text(); re.MatchString(line) { @@ -180,6 +181,10 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro if !recursive { return "", fmt.Errorf("%q is a directory; set recursive=true to search directories", path) } + root, err := filepath.EvalSymlinks(path) + if err != nil { + return "", err + } err = filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { if err != nil { return err @@ -187,6 +192,17 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro if d.IsDir() { return nil } + // Skip entries whose symlink-resolved target escapes the root + // being searched, so a symlink can't be used to read files + // outside the requested directory. + resolved, err := filepath.EvalSymlinks(p) + if err != nil { + return nil + } + rel, err := filepath.Rel(root, resolved) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return nil + } return grepFile(p) }) } else { diff --git a/go/adk/pkg/skills/shell_test.go b/go/adk/pkg/skills/shell_test.go index d3fac188ca..3dd2b47a9d 100644 --- a/go/adk/pkg/skills/shell_test.go +++ b/go/adk/pkg/skills/shell_test.go @@ -412,6 +412,29 @@ func TestGrepContent(t *testing.T) { 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(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) + } + }) } func TestExecuteCommand(t *testing.T) { diff --git a/go/adk/pkg/tools/skills.go b/go/adk/pkg/tools/skills.go index 91632af8cc..89b7223f9d 100644 --- a/go/adk/pkg/tools/skills.go +++ b/go/adk/pkg/tools/skills.go @@ -253,6 +253,9 @@ func NewSkillsTools(skillsDirectory string) ([]tool.Tool, 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 { diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index 542b9f543f..027c28d761 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -193,8 +193,16 @@ def grep_file(file_path: Path) -> list[str]: if not recursive: raise IsADirectoryError(f"{file_or_dir_path} is a directory; set recursive=true to search directories") for entry in sorted(file_or_dir_path.rglob("*")): - if entry.is_file(): - results.extend(grep_file(entry)) + if not entry.is_file(): + continue + try: + # Skip entries whose symlink-resolved target escapes the + # root being searched, so a symlink can't be used to read + # files outside the requested directory. + safe_entry = _validate_path(entry, allowed_root) + except PermissionError: + continue + results.extend(grep_file(safe_entry)) else: results.extend(grep_file(file_or_dir_path)) 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 eb69afdaa9..a0485c7745 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 @@ -372,6 +372,30 @@ def test_grep_content_blocks_path_traversal(tmp_path): 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_skill_discovery_and_loading(skill_test_env: Path): """ Tests the core logic of discovering a skill and loading its instructions. From 8927e931a1c79464afe4411084d8f0fd566fd75e Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 15 Jul 2026 17:47:23 -0400 Subject: [PATCH 3/5] docs: document list_files/grep_file in the ADK skills tools README Adds the two new tools to the quick-start import example and the Tool Workflow table, and notes the symlink-escape protection in the Security section, matching how the existing read_file/write_file/ edit_file/bash tools are already documented there. Signed-off-by: brandonkeung --- python/packages/kagent-adk/src/kagent/adk/tools/README.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 b9ca2700df..323ccb645b 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,7 @@ 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 **Bash tool:** From 69b5d5fe07d6cfa58a04ba4e09086a4729375358 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Thu, 16 Jul 2026 10:24:01 -0400 Subject: [PATCH 4/5] fix: correct err-shadowing bug and add timeout to grep_file Thermos branch review turned up a real bug introduced by the previous symlink-escape fix, plus a consolidation opportunity and a missing timeout: - Go: `root, err := filepath.EvalSymlinks(path)` inside GrepContent's `if info.IsDir()` block shadowed the outer `err`, so a WalkDir failure was never observed by the `err != nil` check afterward. Combined with an in-bounds directory symlink (which WalkDir doesn't recurse into, and which grepFile can't read as a file), this caused the walk to abort silently partway through, returning a truncated "success" result with no error. Fixed by not shadowing err, and by explicitly skipping symlinked directories instead of letting grepFile fail on them. - Go: consolidated the symlink-escape containment check into a single shared `skillruntime.WithinRoot` helper (previously GrepContent had its own filepath.Rel-based check, duplicating the pre-existing isWithinRoot used by resolveReadPath/resolveEditPath/resolveWritePath) so there's one implementation of this security-relevant property instead of two that could drift. - Python: grep_file's regex match now runs via asyncio.to_thread with a 30s asyncio.wait_for timeout, mirroring the timeout bash already enforces. Python's re engine backtracks and a pathological, agent-controlled pattern run synchronously inside an async def could otherwise block the whole event loop indefinitely (Go is unaffected; its regexp package is RE2-based and linear-time). - Mention list_files/grep_file in the bash tool's own description in both languages, and log (debug level) when bash is omitted because the sandbox-runtime isn't configured, so its absence isn't silent. Verified live: both Go and Python runtime pods rebuilt and redeployed, confirmed via the UI that a recursive grep_file across a working directory containing the skills/ symlink (the exact scenario the shadowing bug silently broke) now correctly finds matches on both sides of the symlink. Signed-off-by: brandonkeung --- go/adk/pkg/skills/shell.go | 30 +++++++-- go/adk/pkg/skills/shell_test.go | 41 ++++++++++++ go/adk/pkg/tools/skills.go | 16 +++-- .../src/kagent/adk/tools/file_tools.py | 24 +++++-- .../tests/unittests/test_file_tools.py | 62 +++++++++++++++++++ .../src/kagent/skills/prompts.py | 1 + 6 files changed, 153 insertions(+), 21 deletions(-) create mode 100644 python/packages/kagent-adk/tests/unittests/test_file_tools.py diff --git a/go/adk/pkg/skills/shell.go b/go/adk/pkg/skills/shell.go index 38c51992f5..0da05c2c1e 100644 --- a/go/adk/pkg/skills/shell.go +++ b/go/adk/pkg/skills/shell.go @@ -140,6 +140,15 @@ func ListDirContent(path string) (string, error) { return strings.TrimSuffix(result.String(), "\n"), nil } +// 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. +func WithinRoot(resolved, root string) bool { + resolved = filepath.Clean(resolved) + root = filepath.Clean(root) + return resolved == root || strings.HasPrefix(resolved, root+string(filepath.Separator)) +} + // GrepContent searches path for lines matching a regular expression pattern. // If path is a directory, recursive must be true to search its files. func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, error) { @@ -181,7 +190,11 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro if !recursive { return "", fmt.Errorf("%q is a directory; set recursive=true to search directories", path) } - root, err := filepath.EvalSymlinks(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 "", err } @@ -192,15 +205,20 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro if d.IsDir() { return nil } - // Skip entries whose symlink-resolved target escapes the root - // being searched, so a symlink can't be used to read files - // outside the requested directory. resolved, err := filepath.EvalSymlinks(p) if err != nil { return nil } - rel, err := filepath.Rel(root, resolved) - if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + if fi, statErr := os.Stat(resolved); statErr == nil && 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 aborting the walk. + return nil + } + // Skip entries whose symlink-resolved target escapes the root + // being searched, so a symlink can't be used to read files + // outside the requested directory. + if !WithinRoot(resolved, root) { return nil } return grepFile(p) diff --git a/go/adk/pkg/skills/shell_test.go b/go/adk/pkg/skills/shell_test.go index 3dd2b47a9d..d0da8616ba 100644 --- a/go/adk/pkg/skills/shell_test.go +++ b/go/adk/pkg/skills/shell_test.go @@ -435,6 +435,47 @@ func TestGrepContent(t *testing.T) { t.Errorf("expected symlinked file outside root to be skipped, 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(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) + } + }) } func TestExecuteCommand(t *testing.T) { diff --git a/go/adk/pkg/tools/skills.go b/go/adk/pkg/tools/skills.go index 89b7223f9d..81254f5a43 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" @@ -79,6 +80,7 @@ Python Imports (CRITICAL): For file operations: - Use read_file, write_file, and edit_file for interacting with the filesystem. +- Use list_files and grep_file to explore the filesystem without a full shell command. Timeouts: - python scripts: 60s @@ -302,6 +304,8 @@ func NewSkillsTools(skillsDirectory string) ([]tool.Tool, error) { return nil, fmt.Errorf("failed to create bash tool: %w", err) } tools = append(tools, bashTool) + } else { + slog.Debug("omitting bash tool: sandbox-runtime not configured", "error", err) } return tools, nil @@ -332,7 +336,7 @@ func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, return "", err } - if !isWithinRoot(resolvedCandidate, sessionRoot) && !isWithinRoot(resolvedCandidate, skillsRoot) { + if !skillruntime.WithinRoot(resolvedCandidate, sessionRoot) && !skillruntime.WithinRoot(resolvedCandidate, skillsRoot) { return "", fmt.Errorf("path %q is outside the allowed roots", requestedPath) } @@ -359,7 +363,7 @@ func resolveEditPath(sessionID, skillsDirectory, requestedPath string) (string, if err != nil { return "", err } - if !isWithinRoot(resolvedCandidate, sessionRoot) { + if !skillruntime.WithinRoot(resolvedCandidate, sessionRoot) { return "", fmt.Errorf("path %q is outside the writable session directory", requestedPath) } @@ -386,7 +390,7 @@ func resolveWritePath(sessionID, skillsDirectory, requestedPath string) (string, if err != nil { return "", err } - if !isWithinRoot(resolvedCandidate, sessionRoot) { + if !skillruntime.WithinRoot(resolvedCandidate, sessionRoot) { return "", fmt.Errorf("path %q is outside the writable session directory", requestedPath) } @@ -439,9 +443,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/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py b/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py index 9c49dab3cf..af2037f5d9 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,7 @@ from __future__ import annotations +import asyncio import logging from pathlib import Path from typing import Any, Dict @@ -183,6 +184,11 @@ async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> 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). + _TIMEOUT_SECONDS = 30 + def __init__(self, skills_directory: str | Path): super().__init__( name="grep_file", @@ -239,13 +245,19 @@ async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> path = working_dir / path path = path.resolve() - return grep_content( - path, - pattern, - recursive=recursive, - ignore_case=ignore_case, - allowed_root=[working_dir, Path(self.skills_directory)], + return await asyncio.wait_for( + asyncio.to_thread( + grep_content, + path, + pattern, + recursive=recursive, + ignore_case=ignore_case, + allowed_root=[working_dir, Path(self.skills_directory)], + ), + timeout=self._TIMEOUT_SECONDS, ) + except TimeoutError: + 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}" 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 0000000000..ea0eaa9d19 --- /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-skills/src/kagent/skills/prompts.py b/python/packages/kagent-skills/src/kagent/skills/prompts.py index a458c0448c..edf5d8a83b 100644 --- a/python/packages/kagent-skills/src/kagent/skills/prompts.py +++ b/python/packages/kagent-skills/src/kagent/skills/prompts.py @@ -133,6 +133,7 @@ def get_bash_description() -> str: For file operations: - Use read_file, write_file, and edit_file for interacting with the filesystem. +- Use list_files and grep_file to explore the filesystem without a full shell command. Timeouts: - python scripts: 60s From 041e74783f08a8fee9ae8ad8739690fafe4f7d39 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Thu, 16 Jul 2026 15:02:05 -0400 Subject: [PATCH 5/5] fix: harden grep_file against hangs, unbounded output, and silent read failures Across several review passes on grep_file/list_files, close out the remaining correctness gaps in the recursive-search path (Go and Python): - Fix filepath.WalkDir root resolution so an unresolved symlink root is actually recursed into, and fix an err-shadowing bug that silently truncated results on a WalkDir failure. - Skip non-regular files (FIFOs, sockets, devices) before opening them -- previously a FIFO with no writer connected would hang the search indefinitely, in both the recursive walk and single-target paths. - Cap matched lines to 2000 chars (matching read_file's existing convention); previously unbounded in Python and could fail the whole search past 1MB in Go. - Give GrepFileTool a dedicated thread pool in Python instead of the shared default pool, since a hung regex match can't be forcibly killed and would otherwise starve unrelated work. - Treat a single unreadable file or subdirectory as a skip rather than aborting the whole search, so one bad entry doesn't discard matches already found elsewhere in the tree. Annotate "no matches found (N entries could not be read)" when skips occurred, so a systemic failure isn't indistinguishable from a genuinely empty search. - Surface a real error, instead of a misleadingly confident empty result, when the search root itself is unreadable. - Extract Go's classifyWalkEntry and Python's _resolve_working_path helpers to keep the now-more-involved walk logic readable. Regression tests added for each fix above, verified to fail against the prior code. Signed-off-by: brandonkeung --- go/adk/pkg/skills/shell.go | 110 ++++++++-- go/adk/pkg/skills/shell_test.go | 200 ++++++++++++++++++ go/adk/pkg/tools/skills.go | 2 + .../kagent-adk/src/kagent/adk/tools/README.md | 1 + .../src/kagent/adk/tools/file_tools.py | 83 +++++--- .../src/kagent/skills/prompts.py | 2 + .../kagent-skills/src/kagent/skills/shell.py | 52 ++++- .../tests/unittests/test_skill_execution.py | 135 ++++++++++++ 8 files changed, 528 insertions(+), 57 deletions(-) diff --git a/go/adk/pkg/skills/shell.go b/go/adk/pkg/skills/shell.go index 0da05c2c1e..48d16cd50b 100644 --- a/go/adk/pkg/skills/shell.go +++ b/go/adk/pkg/skills/shell.go @@ -149,6 +149,55 @@ func WithinRoot(resolved, root string) bool { 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. +func classifyWalkEntry(root, p string, d fs.DirEntry) walkEntryAction { + 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 +} + // GrepContent searches path for lines matching a regular expression pattern. // If path is a directory, recursive must be true to search its files. func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, error) { @@ -179,6 +228,9 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro lineNum := 1 for scanner.Scan() { if line := scanner.Text(); re.MatchString(line) { + if len(line) > 2000 { + line = line[:2000] + "..." + } fmt.Fprintf(&result, "%s:%d:%s\n", filePath, lineNum, line) } lineNum++ @@ -198,32 +250,48 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro if err != nil { return "", err } - err = filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() { + // 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. + var skipped int + err = filepath.WalkDir(root, func(p string, d fs.DirEntry, walkErr error) error { + 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 } - resolved, err := filepath.EvalSymlinks(p) - if err != nil { - return nil - } - if fi, statErr := os.Stat(resolved); statErr == nil && 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 aborting the walk. - return nil + switch classifyWalkEntry(root, p, d) { + 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(p); grepErr != nil { + skipped++ + } } - // Skip entries whose symlink-resolved target escapes the root - // being searched, so a symlink can't be used to read files - // outside the requested directory. - if !WithinRoot(resolved, root) { - return nil - } - return grepFile(p) + return nil }) + if err == nil && skipped > 0 && result.Len() == 0 { + return fmt.Sprintf("no matches found (%d entries could not be read)", skipped), nil + } } else { + if !info.Mode().IsRegular() { + return "", fmt.Errorf("%q is not a regular file", path) + } err = grepFile(path) } if err != nil { diff --git a/go/adk/pkg/skills/shell_test.go b/go/adk/pkg/skills/shell_test.go index d0da8616ba..dd19735ce2 100644 --- a/go/adk/pkg/skills/shell_test.go +++ b/go/adk/pkg/skills/shell_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" "time" ) @@ -476,6 +477,205 @@ func TestGrepContent(t *testing.T) { 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(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(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(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(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(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("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(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(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)) + } + }) } func TestExecuteCommand(t *testing.T) { diff --git a/go/adk/pkg/tools/skills.go b/go/adk/pkg/tools/skills.go index 81254f5a43..1779af23ec 100644 --- a/go/adk/pkg/tools/skills.go +++ b/go/adk/pkg/tools/skills.go @@ -59,6 +59,8 @@ Usage: 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` 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 323ccb645b..238838c18f 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/README.md +++ b/python/packages/kagent-adk/src/kagent/adk/tools/README.md @@ -184,6 +184,7 @@ return_artifacts(file_paths=["outputs/report.pdf"]) - 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 **Bash tool:** 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 af2037f5d9..189bd25a63 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 @@ -7,6 +7,8 @@ from __future__ import annotations import asyncio +import concurrent.futures +import functools import logging from pathlib import Path from typing import Any, Dict @@ -30,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.""" @@ -76,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: @@ -125,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: @@ -170,11 +177,7 @@ async def run_async(self, *, args: Dict[str, Any], tool_context: ToolContext) -> path_str = args.get("path", "").strip() or "." try: - working_dir = get_session_path(session_id=tool_context.session.id) - path = Path(path_str) - if not path.is_absolute(): - path = working_dir / path - path = path.resolve() + 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: @@ -186,9 +189,23 @@ class GrepFileTool(BaseTool): # 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). + # 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", @@ -239,24 +256,26 @@ 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(path_str) - if not path.is_absolute(): - path = working_dir / path - path = path.resolve() + path, working_dir = _resolve_working_path(tool_context, path_str) + loop = asyncio.get_running_loop() return await asyncio.wait_for( - asyncio.to_thread( - grep_content, - path, - pattern, - recursive=recursive, - ignore_case=ignore_case, - allowed_root=[working_dir, Path(self.skills_directory)], + 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: + 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}" @@ -310,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-skills/src/kagent/skills/prompts.py b/python/packages/kagent-skills/src/kagent/skills/prompts.py index edf5d8a83b..96a4dab118 100644 --- a/python/packages/kagent-skills/src/kagent/skills/prompts.py +++ b/python/packages/kagent-skills/src/kagent/skills/prompts.py @@ -107,6 +107,8 @@ def get_grep_file_description() -> str: 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 diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index 027c28d761..b6fe80b741 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -168,6 +168,12 @@ def grep_content( """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) @@ -185,14 +191,45 @@ def grep_file(file_path: Path) -> list[str]: for line_num, line in enumerate(f, start=1): line = line.rstrip("\n") if compiled.search(line): + if len(line) > 2000: + line = line[:2000] + "..." matches.append(f"{file_path}:{line_num}:{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") - for entry in sorted(file_or_dir_path.rglob("*")): + + # 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): + # is_file() follows symlinks and checks S_ISREG, so this also + # excludes FIFOs/sockets/devices -- opening one for reading can + # block indefinitely (e.g. a FIFO with no writer connected). if not entry.is_file(): continue try: @@ -202,11 +239,22 @@ def grep_file(file_path: Path) -> list[str]: safe_entry = _validate_path(entry, allowed_root) except PermissionError: continue - results.extend(grep_file(safe_entry)) + 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 -- see the note below. + skipped += 1 + continue 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" return "\n".join(results) 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 a0485c7745..034f30368e 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,3 +1,4 @@ +import concurrent.futures import json import os import shutil @@ -396,6 +397,140 @@ def test_grep_content_recursive_skips_symlinks_that_escape_root(tmp_path): outside_dir.rmdir() +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_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.