From f3d4e40cac8557b1bb5d199e05828dff4cba5317 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 15 Jul 2026 16:55:28 -0400 Subject: [PATCH 1/7] 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 d69ffe619..a302f5610 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 be7f9d6c6..d3fac188c 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 d549781e2..91632af8c 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 54fb692ed..0ecd4696a 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 716b38b0f..d73d43ec0 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 3b6215b87..9c49dab3c 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 bf7e899da..752aa3a9b 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 e8555fff9..de0e552f8 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 6e5e7b359..9cb9081c5 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 5be19165a..a458c0448 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 6c89d8df3..542b9f543 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 4dcdd9dc0..eb69afdaa 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/7] 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 a302f5610..38c51992f 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 d3fac188c..3dd2b47a9 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 91632af8c..89b7223f9 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 542b9f543..027c28d76 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 eb69afdaa..a0485c774 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/7] 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 b9ca2700d..323ccb645 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/7] 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 38c51992f..0da05c2c1 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 3dd2b47a9..d0da8616b 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 89b7223f9..81254f5a4 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 9c49dab3c..af2037f5d 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 000000000..ea0eaa9d1 --- /dev/null +++ b/python/packages/kagent-adk/tests/unittests/test_file_tools.py @@ -0,0 +1,62 @@ +"""Tests for GrepFileTool's timeout protection against slow/catastrophic regex matching.""" + +from unittest.mock import patch + +import pytest +from kagent.skills import initialize_session_path + +from kagent.adk.tools.file_tools import GrepFileTool + + +class MockSession: + def __init__(self, session_id: str = "test-session-grep-timeout"): + self.id = session_id + + +class MockToolContext: + def __init__(self, session_id: str = "test-session-grep-timeout"): + self.session = MockSession(session_id) + + +@pytest.mark.asyncio +async def test_grep_file_tool_times_out_on_slow_match(tmp_path): + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + session_id = "test-session-grep-timeout" + initialize_session_path(session_id, str(skills_dir)) + + tool = GrepFileTool(skills_directory=str(skills_dir)) + tool._TIMEOUT_SECONDS = 0.05 + + def slow_grep(*args, **kwargs): + import time + + time.sleep(1) + return "no matches found" + + with patch("kagent.adk.tools.file_tools.grep_content", side_effect=slow_grep): + result = await tool.run_async( + args={"pattern": "foo", "path": "."}, + tool_context=MockToolContext(session_id), + ) + + assert "took too long" in result + + +@pytest.mark.asyncio +async def test_grep_file_tool_returns_normally_when_fast(tmp_path): + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + session_id = "test-session-grep-timeout-fast" + initialize_session_path(session_id, str(skills_dir)) + + tool = GrepFileTool(skills_directory=str(skills_dir)) + + with patch("kagent.adk.tools.file_tools.grep_content", return_value="match found") as mocked: + result = await tool.run_async( + args={"pattern": "foo", "path": "."}, + tool_context=MockToolContext(session_id), + ) + + assert result == "match found" + assert mocked.called diff --git a/python/packages/kagent-skills/src/kagent/skills/prompts.py b/python/packages/kagent-skills/src/kagent/skills/prompts.py index a458c0448..edf5d8a83 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/7] 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 0da05c2c1..48d16cd50 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 d0da8616b..dd19735ce 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 81254f5a4..1779af23e 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 323ccb645..238838c18 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 af2037f5d..189bd25a6 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py @@ -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 edf5d8a83..96a4dab11 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 027c28d76..b6fe80b74 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 a0485c774..034f30368 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. From a56d813cc0898f87f90ac685ad78711a16d48c89 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Thu, 23 Jul 2026 16:49:00 -0400 Subject: [PATCH 6/7] feat: gate list_files/grep_file behind KAGENT_ENABLE_FILE_SEARCH_TOOLS A maintainer asked that list_files/grep_file default to disabled rather than being registered unconditionally, since they give an agent broader filesystem visibility than read_file/write_file/edit_file. Both runtimes now check a single env var, KAGENT_ENABLE_FILE_SEARCH_TOOLS (off by default, same true-ish values "1"/"t"/"true" case-insensitive in both languages), before registering the two tools. read_file/write_file/ edit_file/skills/bash are unaffected. Verified end-to-end on a live kind cluster: dedicated Go- and Python-runtime test agents with and without the env var set, confirming the tools are absent/present in the registered tool list and functional when enabled. Signed-off-by: brandonkeung --- go/adk/pkg/tools/skills.go | 132 ++++++++++++------ go/adk/pkg/tools/skills_test.go | 71 ++++++++++ go/core/pkg/env/kagent.go | 13 ++ .../kagent-adk/src/kagent/adk/tools/README.md | 1 + .../src/kagent/adk/tools/skills_plugin.py | 21 ++- .../src/kagent/adk/tools/skills_toolset.py | 16 ++- .../tests/unittests/test_skills_plugin.py | 33 +++++ .../src/kagent/skills/__init__.py | 2 + .../src/kagent/skills/prompts.py | 14 +- .../kagent-skills/src/kagent/skills/shell.py | 12 ++ .../tests/unittests/test_skill_execution.py | 38 +++++ 11 files changed, 297 insertions(+), 56 deletions(-) create mode 100644 python/packages/kagent-adk/tests/unittests/test_skills_plugin.py diff --git a/go/adk/pkg/tools/skills.go b/go/adk/pkg/tools/skills.go index 1779af23e..767e932be 100644 --- a/go/adk/pkg/tools/skills.go +++ b/go/adk/pkg/tools/skills.go @@ -13,6 +13,31 @@ import ( "google.golang.org/adk/v2/tool/functiontool" ) +// enableFileSearchToolsEnv gates the list_files and grep_file tools, which +// are opt-in (disabled by default): they let an agent walk the filesystem +// under its session/skills roots without a shell, which some deployments +// want to keep off by default alongside bash rather than enable implicitly. +// +// Also registered (separately, for `kagent env` CLI discoverability only, +// not read here) as KagentEnableFileSearchTools in go/core/pkg/env/kagent.go +// -- keep both string literals in sync if this name ever changes. +const enableFileSearchToolsEnv = "KAGENT_ENABLE_FILE_SEARCH_TOOLS" + +// fileSearchToolsEnabled accepts the same case-insensitive true-values as +// Python's file_search_tools_enabled() (kagent-skills/shell.py), so the +// same literal env var value behaves identically in either runtime rather +// than relying on Go's strconv.ParseBool grammar, which Python doesn't +// replicate exactly (e.g. ParseBool requires the exact casing "True", not +// "tRue"). +func fileSearchToolsEnabled() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv(enableFileSearchToolsEnv))) { + case "1", "t", "true": + return true + default: + return false + } +} + const ( readFileDescription = `Reads a file from the filesystem with line numbers. @@ -82,11 +107,18 @@ 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 - other commands: 30s` + + // fileSearchToolsBashHint is appended to bashDescription only when + // list_files/grep_file are enabled, so bash's own description doesn't + // point the model at tools that aren't registered. Appended as a + // trailing paragraph rather than interpolated into bashDescription, so + // the long, free-form prose above stays a plain string -- not a format + // template where a stray '%' added later could silently corrupt output. + fileSearchToolsBashHint = "\nAlso available: list_files and grep_file, for exploring the filesystem without a full shell command." ) type skillsInput struct { @@ -226,65 +258,79 @@ func NewSkillsTools(skillsDirectory string) ([]tool.Tool, error) { return nil, fmt.Errorf("failed to create edit_file tool: %w", err) } - 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 = "." - } + tools := []tool.Tool{skillsTool, readFileTool, writeFileTool, editFileTool} + + // list_files/grep_file are opt-in: they give an agent broad filesystem + // visibility, so some deployments want them off unless explicitly + // enabled, same as bash below. + fileSearchEnabled := fileSearchToolsEnabled() + if fileSearchEnabled { + listFilesTool, err := functiontool.New(functiontool.Config{ + Name: "list_files", + Description: listFilesDescription, + }, func(ctx adkagent.Context, in listFilesInput) (string, error) { + requestedPath := in.Path + if strings.TrimSpace(requestedPath) == "" { + requestedPath = "." + } - path, err := resolveReadPath(ctx.SessionID(), absSkillsDir, requestedPath) - if err != nil { - return fmt.Sprintf("Error listing %s: %v", requestedPath, err), nil - } + 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) + 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 fmt.Sprintf("Error listing %s: %v", requestedPath, err), nil + return nil, fmt.Errorf("failed to create list_files tool: %w", err) } - return content, nil - }) - if err != nil { - return nil, fmt.Errorf("failed to create list_files tool: %w", err) - } - grepFileTool, err := functiontool.New(functiontool.Config{ - Name: "grep_file", - Description: grepFileDescription, - }, func(ctx adkagent.Context, in grepFileInput) (string, error) { - if strings.TrimSpace(in.Pattern) == "" { - return "Error: No pattern provided", nil - } - if strings.TrimSpace(in.Path) == "" { - return "Error: No file path provided", nil - } + grepFileTool, err := functiontool.New(functiontool.Config{ + Name: "grep_file", + Description: grepFileDescription, + }, func(ctx adkagent.Context, in grepFileInput) (string, error) { + if strings.TrimSpace(in.Pattern) == "" { + return "Error: No pattern provided", nil + } + if strings.TrimSpace(in.Path) == "" { + return "Error: No file path provided", nil + } - path, err := resolveReadPath(ctx.SessionID(), absSkillsDir, in.Path) - if err != nil { - return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil - } + path, err := resolveReadPath(ctx.SessionID(), absSkillsDir, in.Path) + if err != nil { + return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil + } - content, err := skillruntime.GrepContent(path, in.Pattern, in.Recursive, in.IgnoreCase) + content, err := skillruntime.GrepContent(path, in.Pattern, in.Recursive, in.IgnoreCase) + if err != nil { + return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil + } + return content, nil + }) if err != nil { - return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil + return nil, fmt.Errorf("failed to create grep_file tool: %w", err) } - return content, nil - }) - if err != nil { - return nil, fmt.Errorf("failed to create grep_file tool: %w", err) - } - tools := []tool.Tool{skillsTool, readFileTool, writeFileTool, editFileTool, listFilesTool, grepFileTool} + tools = append(tools, listFilesTool, grepFileTool) + } else { + slog.Debug("omitting list_files/grep_file tools: " + enableFileSearchToolsEnv + " not enabled") + } // 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 { + desc := bashDescription + if fileSearchEnabled { + desc += fileSearchToolsBashHint + } bashTool, err := functiontool.New(functiontool.Config{ Name: "bash", - Description: bashDescription, + Description: desc, }, func(ctx adkagent.Context, in bashInput) (string, error) { command := strings.TrimSpace(in.Command) if command == "" { diff --git a/go/adk/pkg/tools/skills_test.go b/go/adk/pkg/tools/skills_test.go index 0ecd4696a..aa1c648dc 100644 --- a/go/adk/pkg/tools/skills_test.go +++ b/go/adk/pkg/tools/skills_test.go @@ -88,6 +88,7 @@ func TestResolveWritePath_BlocksSkillsSymlink(t *testing.T) { func TestNewSkillsTools_ReturnsExpectedToolSet(t *testing.T) { skillsDir := t.TempDir() t.Setenv("KAGENT_SRT_SETTINGS_PATH", filepath.Join(t.TempDir(), "srt-settings.json")) + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "true") skillDir := filepath.Join(skillsDir, "demo") if err := os.MkdirAll(skillDir, 0755); err != nil { t.Fatalf("failed to create skill dir: %v", err) @@ -120,6 +121,7 @@ description: Demo skill. func TestNewSkillsTools_OmitsBashWithoutSRTSettings(t *testing.T) { skillsDir := t.TempDir() t.Setenv("KAGENT_SRT_SETTINGS_PATH", "") + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "true") tools, err := NewSkillsTools(skillsDir) if err != nil { @@ -141,6 +143,74 @@ func TestNewSkillsTools_OmitsBashWithoutSRTSettings(t *testing.T) { } } +func TestNewSkillsTools_OmitsListFilesAndGrepFileByDefault(t *testing.T) { + skillsDir := t.TempDir() + t.Setenv("KAGENT_SRT_SETTINGS_PATH", filepath.Join(t.TempDir(), "srt-settings.json")) + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "") + + tools, err := NewSkillsTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillsTools() error = %v, want nil (list_files/grep_file should be omitted, not fatal)", err) + } + + got := map[string]bool{} + for _, tool := range tools { + got[tool.Name()] = true + } + + for _, name := range []string{"skills", "read_file", "write_file", "edit_file", "bash"} { + if !got[name] { + t.Errorf("expected tool %q to be present even without KAGENT_ENABLE_FILE_SEARCH_TOOLS", name) + } + } + if got["list_files"] { + t.Error("expected list_files tool to be omitted by default") + } + if got["grep_file"] { + t.Error("expected grep_file tool to be omitted by default") + } +} + +func TestNewSkillsTools_BashDescriptionMentionsFileSearchToolsOnlyWhenEnabled(t *testing.T) { + skillsDir := t.TempDir() + t.Setenv("KAGENT_SRT_SETTINGS_PATH", filepath.Join(t.TempDir(), "srt-settings.json")) + + findBash := func(t *testing.T, tools []tool.Tool) tool.Tool { + t.Helper() + for _, tl := range tools { + if tl.Name() == "bash" { + return tl + } + } + t.Fatal("expected bash tool to be present") + return nil + } + + t.Run("disabled by default", func(t *testing.T) { + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "") + tools, err := NewSkillsTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillsTools() error = %v", err) + } + desc := findBash(t, tools).Description() + if strings.Contains(desc, "list_files") || strings.Contains(desc, "grep_file") { + t.Errorf("bash description should not mention list_files/grep_file when disabled, got %q", desc) + } + }) + + t.Run("mentioned when enabled", func(t *testing.T) { + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "true") + tools, err := NewSkillsTools(skillsDir) + if err != nil { + t.Fatalf("NewSkillsTools() error = %v", err) + } + desc := findBash(t, tools).Description() + if !strings.Contains(desc, "list_files and grep_file") { + t.Errorf("bash description should mention list_files and grep_file when enabled, got %q", desc) + } + }) +} + // TestListFilesAndGrepFileTools_RunThroughADK invokes the real functiontool.Run() // path (the same one the ADK flow engine uses to execute a model's tool call), // rather than calling ListDirContent/GrepContent directly, to verify the @@ -150,6 +220,7 @@ func TestListFilesAndGrepFileTools_RunThroughADK(t *testing.T) { t.Setenv("TMPDIR", t.TempDir()) skillsDir := t.TempDir() t.Setenv("KAGENT_SRT_SETTINGS_PATH", "") + t.Setenv("KAGENT_ENABLE_FILE_SEARCH_TOOLS", "true") tools, err := NewSkillsTools(skillsDir) if err != nil { diff --git a/go/core/pkg/env/kagent.go b/go/core/pkg/env/kagent.go index a72b6f002..e5ce0108c 100644 --- a/go/core/pkg/env/kagent.go +++ b/go/core/pkg/env/kagent.go @@ -88,6 +88,19 @@ var ( ComponentAgentRuntime, ) + // Registered here for `kagent env` CLI discoverability only -- the + // actual gate is read independently (raw os.Getenv, not via this var) + // in go/adk/pkg/tools/skills.go's enableFileSearchToolsEnv. Keep both + // string literals in sync if this name ever changes. + KagentEnableFileSearchTools = RegisterBoolVar( + "KAGENT_ENABLE_FILE_SEARCH_TOOLS", + false, + "When true, enables the list_files and grep_file skills tools, which let an agent "+ + "walk the filesystem under its session/skills roots without a shell. Disabled by "+ + "default alongside bash; set on the Agent's env to opt in.", + ComponentAgentRuntime, + ) + StsWellKnownURI = RegisterStringVar( "STS_WELL_KNOWN_URI", "", diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/README.md b/python/packages/kagent-adk/src/kagent/adk/tools/README.md index 238838c18..78300dcd2 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/README.md +++ b/python/packages/kagent-adk/src/kagent/adk/tools/README.md @@ -185,6 +185,7 @@ return_artifacts(file_paths=["outputs/report.pdf"]) - File size limits (100 MB max) - `grep_file` skips symlinked entries that resolve outside the directory being searched, so a symlink can't be used to read files outside the session's working directory - `recursive=True` does not descend into symlinked subdirectories (this is standard Go/Python stdlib walk behavior, not something this tool adds) — since `skills/` is itself a symlink, a recursive search from the working directory root will not find matches inside it; pass `skills/...` as the path directly to search skill contents +- `list_files`/`grep_file` are disabled by default — they give an agent broad filesystem visibility, so they're opt-in. Set `KAGENT_ENABLE_FILE_SEARCH_TOOLS=true` on the agent's env to enable them (`read_file`/`write_file`/`edit_file`/`bash` are unaffected and always available in this Python runtime) **Bash tool:** diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py b/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py index 752aa3a9b..aea3b70a2 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py @@ -5,6 +5,7 @@ from typing import Optional from google.adk.agents import BaseAgent, LlmAgent +from kagent.skills import file_search_tools_enabled from ..tools import BashTool, EditFileTool, GrepFileTool, ListFilesTool, ReadFileTool, WriteFileTool from .skill_tool import SkillsTool @@ -51,10 +52,16 @@ def add_skills_tool_to_agent( 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}") + # list_files/grep_file are opt-in: they give an agent broad filesystem + # visibility, so some deployments want them off unless explicitly + # enabled, same as bash. + if file_search_tools_enabled(): + if "list_files" not in existing_tool_names: + agent.tools.append(ListFilesTool(skills_directory)) + logger.debug(f"Added list files tool to agent: {agent.name}") + + if "grep_file" not in existing_tool_names: + agent.tools.append(GrepFileTool(skills_directory)) + logger.debug(f"Added grep file tool to agent: {agent.name}") + else: + logger.debug(f"Omitting list_files/grep_file tools for agent: {agent.name} (KAGENT_ENABLE_FILE_SEARCH_TOOLS not enabled)") diff --git a/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py b/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py index de0e552f8..579d588e1 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py @@ -12,6 +12,7 @@ from google.adk.agents.readonly_context import ReadonlyContext from google.adk.tools import BaseTool from google.adk.tools.base_toolset import BaseToolset +from kagent.skills import file_search_tools_enabled from ..tools import BashTool, EditFileTool, GrepFileTool, ListFilesTool, ReadFileTool, WriteFileTool from .skill_tool import SkillsTool @@ -52,8 +53,15 @@ def __init__(self, skills_directory: str | Path): self.read_file_tool = ReadFileTool(skills_directory) self.write_file_tool = WriteFileTool() self.edit_file_tool = EditFileTool() - self.list_files_tool = ListFilesTool(skills_directory) - self.grep_file_tool = GrepFileTool(skills_directory) + # list_files/grep_file are opt-in: they give an agent broad + # filesystem visibility, so some deployments want them off unless + # explicitly enabled, same as bash. A single list (rather than two + # separately-nullable attributes) keeps "both present or both + # absent" a structural guarantee instead of a convention the two + # attributes have to be kept in sync by hand. + self._file_search_tools: list[BaseTool] = ( + [ListFilesTool(skills_directory), GrepFileTool(skills_directory)] if file_search_tools_enabled() else [] + ) self.bash_tool = BashTool(skills_directory) @override @@ -62,13 +70,13 @@ async def get_tools(self, readonly_context: Optional[ReadonlyContext] = None) -> Returns: List containing all skills tools: skills, read, write, edit, list, grep, and bash. + list/grep are omitted unless KAGENT_ENABLE_FILE_SEARCH_TOOLS is enabled. """ return [ self.skills_tool, self.read_file_tool, self.write_file_tool, self.edit_file_tool, - self.list_files_tool, - self.grep_file_tool, + *self._file_search_tools, self.bash_tool, ] diff --git a/python/packages/kagent-adk/tests/unittests/test_skills_plugin.py b/python/packages/kagent-adk/tests/unittests/test_skills_plugin.py new file mode 100644 index 000000000..d716a50e0 --- /dev/null +++ b/python/packages/kagent-adk/tests/unittests/test_skills_plugin.py @@ -0,0 +1,33 @@ +"""Tests for add_skills_tool_to_agent's list_files/grep_file feature-flag gating.""" + +from unittest.mock import patch + +from google.adk.agents import LlmAgent + +from kagent.adk.tools.skills_plugin import add_skills_tool_to_agent + + +def _tool_names(agent: LlmAgent) -> set[str]: + return {getattr(t, "name", None) for t in agent.tools} + + +def test_add_skills_tool_to_agent_omits_list_files_and_grep_file_by_default(tmp_path): + agent = LlmAgent(name="test_agent", model="gemini-2.0-flash", tools=[]) + + with patch.dict("os.environ", {}, clear=True): + add_skills_tool_to_agent(str(tmp_path), agent) + + names = _tool_names(agent) + assert {"skills", "read_file", "write_file", "edit_file", "bash"} <= names + assert "list_files" not in names + assert "grep_file" not in names + + +def test_add_skills_tool_to_agent_adds_list_files_and_grep_file_when_enabled(tmp_path): + agent = LlmAgent(name="test_agent", model="gemini-2.0-flash", tools=[]) + + with patch.dict("os.environ", {"KAGENT_ENABLE_FILE_SEARCH_TOOLS": "true"}, clear=True): + add_skills_tool_to_agent(str(tmp_path), agent) + + names = _tool_names(agent) + assert {"skills", "read_file", "write_file", "edit_file", "bash", "list_files", "grep_file"} <= names diff --git a/python/packages/kagent-skills/src/kagent/skills/__init__.py b/python/packages/kagent-skills/src/kagent/skills/__init__.py index 9cb9081c5..160778c66 100644 --- a/python/packages/kagent-skills/src/kagent/skills/__init__.py +++ b/python/packages/kagent-skills/src/kagent/skills/__init__.py @@ -17,6 +17,7 @@ from .shell import ( edit_file_content, execute_command, + file_search_tools_enabled, grep_content, list_dir_content, read_file_content, @@ -33,6 +34,7 @@ "list_dir_content", "grep_content", "execute_command", + "file_search_tools_enabled", "generate_skills_tool_description", "get_read_file_description", "get_write_file_description", diff --git a/python/packages/kagent-skills/src/kagent/skills/prompts.py b/python/packages/kagent-skills/src/kagent/skills/prompts.py index 96a4dab11..1f47e633d 100644 --- a/python/packages/kagent-skills/src/kagent/skills/prompts.py +++ b/python/packages/kagent-skills/src/kagent/skills/prompts.py @@ -1,4 +1,5 @@ from .models import Skill +from .shell import file_search_tools_enabled def generate_skills_xml(skills: list[Skill]) -> str: @@ -118,7 +119,7 @@ def get_grep_file_description() -> str: def get_bash_description() -> str: """Returns the standardized description for the bash tool.""" # This combines the useful parts from both ADK and OpenAI descriptions - return """Execute bash commands in the skills environment with sandbox protection. + description = """Execute bash commands in the skills environment with sandbox protection. Working Directory & Structure: - Commands run in a temporary session directory: /tmp/kagent/{session_id}/ @@ -135,9 +136,18 @@ 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 - other commands: 30s """ + # Appended as a trailing paragraph rather than interpolated into the + # description above, so that long, free-form prose block stays a plain + # string -- not an f-string where a stray '{'/'}' added later could + # raise or silently corrupt output. + if file_search_tools_enabled(): + description += ( + "\nAlso available: list_files and grep_file, " + "for exploring the filesystem without a full shell command.\n" + ) + return description diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index b6fe80b74..17b09f449 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -293,6 +293,18 @@ def _sanitize_env(env: dict[str, str] | None = None) -> dict[str, str]: return {k: v for k, v in source.items() if k not in _SECRET_ENV_NAMES and not _SECRET_PATTERNS.search(k)} +_ENABLE_FILE_SEARCH_TOOLS_ENV = "KAGENT_ENABLE_FILE_SEARCH_TOOLS" + + +def file_search_tools_enabled() -> bool: + """Whether the list_files/grep_file tools are enabled. + + Disabled by default, same as bash: both give an agent broad filesystem + visibility, so some deployments want them off unless explicitly enabled. + """ + return os.environ.get(_ENABLE_FILE_SEARCH_TOOLS_ENV, "").strip().lower() in ("1", "t", "true") + + def _get_srt_settings_args() -> list[str]: """Return srt settings args using the mounted config path.""" settings_path_env = os.environ.get("KAGENT_SRT_SETTINGS_PATH", "").strip() 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 034f30368..bd44048f6 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 @@ -13,12 +13,14 @@ discover_skills, edit_file_content, execute_command, + file_search_tools_enabled, grep_content, list_dir_content, load_skill_content, read_file_content, write_file_content, ) +from kagent.skills.prompts import get_bash_description from kagent.skills.shell import _get_srt_settings_args, _sanitize_env @@ -201,6 +203,42 @@ def test_get_srt_settings_args_requires_mounted_path(): _get_srt_settings_args() +@pytest.mark.parametrize( + "value,expected", + [ + (None, False), + ("", False), + ("false", False), + ("0", False), + ("no", False), + ("true", True), + ("TRUE", True), + ("True", True), + ("1", True), + ("t", True), + ("T", True), + ], +) +def test_file_search_tools_enabled(value, expected): + """list_files/grep_file are disabled by default and opt-in via the env var.""" + env = {} if value is None else {"KAGENT_ENABLE_FILE_SEARCH_TOOLS": value} + with patch.dict("os.environ", env, clear=True): + assert file_search_tools_enabled() is expected + + +def test_get_bash_description_omits_file_search_tools_by_default(): + with patch.dict("os.environ", {}, clear=True): + desc = get_bash_description() + assert "list_files" not in desc + assert "grep_file" not in desc + + +def test_get_bash_description_mentions_file_search_tools_when_enabled(): + with patch.dict("os.environ", {"KAGENT_ENABLE_FILE_SEARCH_TOOLS": "true"}, clear=True): + desc = get_bash_description() + assert "list_files and grep_file" in desc + + # --- Path traversal tests --- From e3f9c76d181ce4571a01d16d07ca693a869529e0 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Fri, 24 Jul 2026 12:04:10 -0400 Subject: [PATCH 7/7] fix: close symlink-listing, TOCTOU, and skip-count gaps in grep_file/list_files Addresses 4 issues mesutoezdil found in review on PR #2267: - ListDirContent listed a directory symlink (e.g. every session's "skills" entry) as a file instead of a directory, since entry.IsDir() doesn't follow symlinks. Now stats symlink entries to classify them correctly, matching Python's existing symlink-following behavior. - classifyWalkEntry verified a walked entry's resolved, in-bounds target, but grepFile then reopened the original unresolved path -- a verify-then-use gap where the symlink's target could differ between the check and the read. grepFile now reads the resolved path that was actually verified. This narrows the race but doesn't fully eliminate it (documented in a comment on classifyWalkEntry); closing it completely would need platform-specific work disproportionate to this file's existing security bar. - Go and Python both silently dropped the "N entries could not be read" note whenever there were also real matches, only surfacing it when the result was otherwise empty -- masking partial failures. Both now append it alongside real matches too. Verified end-to-end on a live kind cluster via A2A against redeployed Go- and Python-runtime test agents, extracting raw tool function_response payloads (not model-summarized text) to confirm each fix's actual behavior, plus a targeted regression check confirming symlink-escape protection still holds after the refactor. Signed-off-by: brandonkeung --- go/adk/pkg/skills/shell.go | 61 +++++++++---- go/adk/pkg/skills/shell_test.go | 89 +++++++++++++++++++ .../kagent-skills/src/kagent/skills/shell.py | 5 +- .../tests/unittests/test_skill_execution.py | 23 +++++ 4 files changed, 161 insertions(+), 17 deletions(-) diff --git a/go/adk/pkg/skills/shell.go b/go/adk/pkg/skills/shell.go index 48d16cd50..5d674db34 100644 --- a/go/adk/pkg/skills/shell.go +++ b/go/adk/pkg/skills/shell.go @@ -129,6 +129,18 @@ func ListDirContent(path string) (string, error) { continue } + // entry.IsDir() reflects the entry's own type (Lstat-like) and is + // false for a symlink even when its target is a directory -- e.g. + // the "skills" symlink present in every session dir. Stat (which + // follows symlinks) to classify those correctly, matching Python's + // pathlib Path.is_dir(), which follows symlinks by default. + if entry.Type()&fs.ModeSymlink != 0 { + if target, statErr := os.Stat(filepath.Join(path, entry.Name())); statErr == nil && target.IsDir() { + fmt.Fprintf(&result, "%s/\n", entry.Name()) + continue + } + } + info, err := entry.Info() if err != nil { fmt.Fprintf(&result, "%s\n", entry.Name()) @@ -164,38 +176,51 @@ const ( // 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 { +// caller is responsible for that once this returns walkEntryGrep, and must +// read the returned resolved path rather than p itself: p is the symlink +// as walked, and re-resolving or reopening it separately from this check +// would reintroduce a TOCTOU window where the symlink's target could change +// between the check and the read. +// +// This narrows that window but doesn't eliminate it: resolved is still a +// path string, so the later os.Open on it is a fresh lookup, not an +// operation on a captured file handle. A path component of resolved could +// still be swapped out between this check and that open. Closing that +// residual race fully would need platform-specific work (e.g. Linux's +// openat2 with RESOLVE_NO_SYMLINKS), which isn't done anywhere else in this +// file either -- this function only guarantees "reads what was just +// verified", not "reads atomically with no concurrent tampering". +func classifyWalkEntry(root, p string, d fs.DirEntry) (walkEntryAction, string) { if d.IsDir() { - return walkEntrySkip + return walkEntrySkip, "" } resolved, err := filepath.EvalSymlinks(p) if err != nil { - return walkEntryUnreadable + return walkEntryUnreadable, "" } fi, statErr := os.Stat(resolved) if statErr != nil { - return walkEntryUnreadable + 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 + 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 + 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 walkEntrySkip, "" } - return walkEntryGrep + return walkEntryGrep, resolved } // GrepContent searches path for lines matching a regular expression pattern. @@ -216,6 +241,7 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro } var result strings.Builder + var skipped int grepFile := func(filePath string) error { file, err := os.Open(filePath) if err != nil { @@ -254,7 +280,6 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro // 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 { @@ -272,22 +297,19 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro skipped++ return nil } - switch classifyWalkEntry(root, p, d) { + switch action, resolved := classifyWalkEntry(root, p, d); action { case walkEntryUnreadable: skipped++ case walkEntryGrep: // A read error on one file (permission denied, a line // exceeding the scan buffer, etc.) shouldn't abort matches // already found elsewhere in the tree. - if grepErr := grepFile(p); grepErr != nil { + if grepErr := grepFile(resolved); grepErr != nil { skipped++ } } 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) @@ -299,10 +321,17 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro } if result.Len() == 0 { + if skipped > 0 { + return fmt.Sprintf("no matches found (%d entries could not be read)", skipped), nil + } return "no matches found", nil } - return strings.TrimSuffix(result.String(), "\n"), nil + matches := strings.TrimSuffix(result.String(), "\n") + if skipped > 0 { + matches += fmt.Sprintf("\n\n(%d entries could not be read)", skipped) + } + return matches, nil } func resolveSRTSettingsArgs() ([]string, error) { diff --git a/go/adk/pkg/skills/shell_test.go b/go/adk/pkg/skills/shell_test.go index dd19735ce..e416f2d2f 100644 --- a/go/adk/pkg/skills/shell_test.go +++ b/go/adk/pkg/skills/shell_test.go @@ -345,6 +345,26 @@ func TestListDirContent(t *testing.T) { t.Fatal("expected error for nonexistent path") } }) + + t.Run("symlink to a directory is listed as a directory", func(t *testing.T) { + realDir := filepath.Join(tmpDir, "a-subdir") + linkPath := filepath.Join(tmpDir, "dir-link") + if err := os.Symlink(realDir, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + result, err := ListDirContent(tmpDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + if !strings.Contains(result, "dir-link/") { + t.Errorf("expected symlinked directory to be listed with a trailing slash, got %q", result) + } + if strings.Contains(result, "dir-link\t") { + t.Errorf("expected symlinked directory not to be listed as a file, got %q", result) + } + }) } func TestGrepContent(t *testing.T) { @@ -437,6 +457,35 @@ func TestGrepContent(t *testing.T) { } }) + t.Run("recursive search greps the resolved target of an in-bounds file symlink", func(t *testing.T) { + realFile := filepath.Join(subDir, "real_target.txt") + if err := os.WriteFile(realFile, []byte("foo via symlink\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + linkPath := filepath.Join(subDir, "file_link.txt") + if err := os.Symlink(realFile, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + result, err := GrepContent(subDir, "foo via symlink", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + // The match must be reported against the resolved target path + // (real_target.txt), not the walked symlink path (file_link.txt): + // classifyWalkEntry verifies the resolved target is in-bounds, and + // the actual read must use that same resolved value rather than + // re-deriving/reopening the raw symlink path, or the verified-safe + // check and the actual read could diverge (TOCTOU). + if !strings.Contains(result, "real_target.txt:1:foo via symlink") { + t.Errorf("expected match to be reported against the resolved target path, got %q", result) + } + if strings.Contains(result, "file_link.txt:") { + t.Errorf("expected match not to be reported against the unresolved symlink path, got %q", result) + } + }) + t.Run("recursive search does not abort on an in-bounds directory symlink", func(t *testing.T) { walkDir := createTempDir(t) defer os.RemoveAll(walkDir) @@ -635,6 +684,46 @@ func TestGrepContent(t *testing.T) { } }) + t.Run("matches are annotated with the skip count when some entries could not be read", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + okSub := filepath.Join(walkDir, "aaa_ok") + if err := os.Mkdir(okSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(okSub, "match.txt"), []byte("foo readable\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + noPermSub := filepath.Join(walkDir, "mmm_noperm") + if err := os.Mkdir(noPermSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(noPermSub, 0000); err != nil { + t.Fatalf("Failed to chmod subdir: %v", err) + } + defer os.Chmod(noPermSub, 0755) + + result, err := GrepContent(walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "match.txt:1:foo readable") { + t.Errorf("expected the real match to still be reported, got %q", result) + } + if !strings.Contains(result, "could not be read") { + t.Errorf("expected the skip count to be reported alongside real matches, not silently dropped, got %q", result) + } + }) + t.Run("recursive search on a fully unreadable root returns an error, not a confident empty result", func(t *testing.T) { if os.Geteuid() == 0 { t.Skip("root bypasses directory permissions; cannot exercise this case") diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index 17b09f449..1a542628a 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -257,7 +257,10 @@ def grep_file(file_path: Path) -> list[str]: return f"no matches found ({skipped} entries could not be read)" return "no matches found" - return "\n".join(results) + output = "\n".join(results) + if skipped: + output += f"\n\n({skipped} entries could not be read)" + return output # --- Shell Operation Tools --- 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 bd44048f6..b23a3c5b9 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 @@ -519,6 +519,29 @@ def test_grep_content_recursive_does_not_abort_or_discard_matches_on_an_unreadab assert "match.txt:1:foo readable" in result +def test_grep_content_annotates_skip_count_alongside_real_matches(tmp_path): + """A skip count found alongside real matches must not be silently dropped.""" + if os.geteuid() == 0: + pytest.skip("root bypasses directory permissions; cannot exercise this case") + + ok_sub = tmp_path / "aaa_ok" + ok_sub.mkdir() + (ok_sub / "match.txt").write_text("foo readable\n") + + noperm_sub = tmp_path / "mmm_noperm" + noperm_sub.mkdir() + (noperm_sub / "hidden.txt").write_text("foo hidden\n") + noperm_sub.chmod(0o000) + + try: + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + finally: + noperm_sub.chmod(0o755) + + assert "match.txt:1:foo readable" in result + assert "could not be read" in result + + def test_grep_content_no_matches_found_is_annotated_when_a_subdirectory_could_not_be_read(tmp_path): if os.geteuid() == 0: pytest.skip("root bypasses directory permissions; cannot exercise this case")