From f3d4e40cac8557b1bb5d199e05828dff4cba5317 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 15 Jul 2026 16:55:28 -0400 Subject: [PATCH 01/15] 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 02/15] 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 03/15] 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 04/15] 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 05/15] 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 06/15] 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 07/15] 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") From 07e4d6275266c0feb9b9d2afa73227ce3d774538 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 26 Aug 2026 11:50:00 -0400 Subject: [PATCH 08/15] fix: correct symlink sizing, FIFO hang, UTF-8 truncation, and skip accounting Addresses mesutoezdil's review findings on PR #2267. Six were real bugs; all are regression-tested (each new test verified to fail against the pre-fix code). Go: - ListDirContent reported a symlink's own size -- the byte length of its stored target path -- because entry.Info() is Lstat-based. It now stats the target, so a symlinked file reports the file's size and a broken link is listed bare, matching Python's pathlib behavior. The needed os.Stat result was already being computed and discarded. - ReadFileContent had no regular-file guard, so read_file on a FIFO with no writer blocked forever with no timeout on the path. It now rejects non-regular files, as GrepContent already did. - Line truncation sliced bytes, not runes. Beyond emitting invalid UTF-8 from a split sequence, it cut CJK text at ~668 characters rather than the 2000 the tool descriptions promise. Both sites now share a truncateRunes helper that cuts on a rune boundary, matching Python's per-code-point slicing. - Wrap the bare errors in GrepContent and ReadFileContent with %w. Python: - grep_content dropped broken symlinks silently. A dangling link is a genuine read failure, so it now counts toward the "N entries could not be read" annotation, matching Go's walkEntryUnreadable. FIFOs and sockets stay silent, matching walkEntrySkip. - Entries were validated only against allowed_root, which in production is the whole session dir plus the skills dir -- wider than the directory being searched. A symlink could therefore pull in a sibling the caller never asked about, contradicting both the tool description and the README. Entries are now also bounded by the search root, as Go already does. Also corrects comments in five places that described list_files/grep_file as opt-in "alongside bash". Upstream removed bash's gating entirely in #2498, so bash is now unconditional in both runtimes and that comparison was false. Adds a test pinning the KAGENT_ENABLE_FILE_SEARCH_TOOLS literal in go/adk/pkg/tools to the `kagent env` registry entry in go/core/pkg/env so the two cannot drift. The import is test-only and does not add a dependency from the agent runtime onto the control-plane module. Signed-off-by: brandonkeung --- go/adk/pkg/tools/shell.go | 79 ++++++++--- go/adk/pkg/tools/shell_test.go | 126 ++++++++++++++++++ go/adk/pkg/tools/skills.go | 16 ++- go/adk/pkg/tools/skills_test.go | 16 +++ go/core/pkg/env/kagent.go | 9 +- .../src/kagent/adk/tools/skills_plugin.py | 4 +- .../src/kagent/adk/tools/skills_toolset.py | 10 +- .../kagent-skills/src/kagent/skills/shell.py | 28 +++- .../tests/unittests/test_skill_execution.py | 66 +++++++++ 9 files changed, 311 insertions(+), 43 deletions(-) diff --git a/go/adk/pkg/tools/shell.go b/go/adk/pkg/tools/shell.go index 4493fd019..0a337f396 100644 --- a/go/adk/pkg/tools/shell.go +++ b/go/adk/pkg/tools/shell.go @@ -16,6 +16,33 @@ import ( type CommandExecutor struct{} +// maxLineRunes is the longest line read_file and grep_file will emit before +// truncating. Counted in runes, not bytes -- see truncateRunes. +const maxLineRunes = 2000 + +// truncateRunes shortens s to at most maxRunes code points, appending "..." +// when it truncates. +// +// Slicing a Go string directly (s[:n]) cuts on a byte boundary, which can +// split a multi-byte UTF-8 sequence and leave a partial code point that +// renders as U+FFFD. It also makes the limit mean bytes, so the same line +// would truncate at a different point than in the Python runtime (whose str +// slicing is per-code-point) and than the tool descriptions promise, which +// say "characters". +func truncateRunes(s string, maxRunes int) string { + count := 0 + // Ranging a string yields one iteration per rune, with i the byte index + // where that rune starts -- so at the maxRunes'th rune, s[:i] holds + // exactly maxRunes complete runes. + for i := range s { + if count == maxRunes { + return s[:i] + "..." + } + count++ + } + return s +} + // GetSessionPath creates the working directory used by skill execution tools. func GetSessionPath(sessionID, skillsDirectory string) (string, error) { if sessionID == "" { @@ -55,9 +82,20 @@ func GetSessionPath(sessionID, skillsDirectory string) (string, error) { // ReadFileContent reads a file with line numbers. func ReadFileContent(path string, offset, limit int) (string, error) { + // Stat before opening: os.Open on a FIFO with no writer connected blocks + // indefinitely, and nothing on this path imposes a timeout. GrepContent + // rejects non-regular files for the same reason. + info, err := os.Stat(path) + if err != nil { + return "", fmt.Errorf("failed to stat %q: %w", path, err) + } + if !info.Mode().IsRegular() { + return "", fmt.Errorf("%q is not a regular file", path) + } + file, err := os.Open(path) if err != nil { - return "", err + return "", fmt.Errorf("failed to open %q: %w", path, err) } defer file.Close() @@ -69,10 +107,7 @@ func ReadFileContent(path string, offset, limit int) (string, error) { for scanner.Scan() { if lineNum >= start { - line := scanner.Text() - if len(line) > 2000 { - line = line[:2000] + "..." - } + line := truncateRunes(scanner.Text(), maxLineRunes) fmt.Fprintf(&result, "%6d|%s\n", lineNum, line) count++ if limit > 0 && count >= limit { @@ -162,16 +197,25 @@ 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. + // entry.IsDir() and entry.Info() both describe the entry itself + // (Lstat-like), so for a symlink they report the link rather than + // its target -- the target's type is lost, and Info().Size() is the + // length of the stored target path, not the file's size. os.Stat + // follows the link, so use it for both, matching Python's pathlib + // Path.is_dir()/Path.stat(), which follow symlinks by default. if entry.Type()&fs.ModeSymlink != 0 { - if target, statErr := os.Stat(filepath.Join(path, entry.Name())); statErr == nil && target.IsDir() { + target, statErr := os.Stat(filepath.Join(path, entry.Name())) + switch { + case statErr != nil: + // Broken link: there is no target to size. Print the bare + // name, as Python does when its stat() raises here. + fmt.Fprintf(&result, "%s\n", entry.Name()) + case target.IsDir(): fmt.Fprintf(&result, "%s/\n", entry.Name()) - continue + default: + fmt.Fprintf(&result, "%s\t%d\n", entry.Name(), target.Size()) } + continue } info, err := entry.Info() @@ -270,7 +314,7 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro info, err := os.Stat(path) if err != nil { - return "", err + return "", fmt.Errorf("failed to stat %q: %w", path, err) } var result strings.Builder @@ -287,10 +331,7 @@ 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) + fmt.Fprintf(&result, "%s:%d:%s\n", filePath, lineNum, truncateRunes(line, maxLineRunes)) } lineNum++ } @@ -307,7 +348,7 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro var root string root, err = filepath.EvalSymlinks(path) if err != nil { - return "", err + return "", fmt.Errorf("failed to resolve %q: %w", path, err) } // Walk the resolved root, not path: filepath.WalkDir uses Lstat on // its root argument, so if path itself were an unresolved directory @@ -350,7 +391,7 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro err = grepFile(path) } if err != nil { - return "", err + return "", fmt.Errorf("failed to search %q: %w", path, err) } if result.Len() == 0 { diff --git a/go/adk/pkg/tools/shell_test.go b/go/adk/pkg/tools/shell_test.go index 82c1fccd0..49f8a2cd6 100644 --- a/go/adk/pkg/tools/shell_test.go +++ b/go/adk/pkg/tools/shell_test.go @@ -2,12 +2,14 @@ package tools import ( "context" + "fmt" "os" "path/filepath" "strings" "syscall" "testing" "time" + "unicode/utf8" ) func createTempDir(t *testing.T) string { @@ -174,6 +176,78 @@ func TestReadFileContent(t *testing.T) { } } +func TestReadFileContent_FIFOReturnsErrorInsteadOfHanging(t *testing.T) { + fifoDir := createTempDir(t) + defer os.RemoveAll(fifoDir) + + fifoPath := filepath.Join(fifoDir, "pipe") + if err := syscall.Mkfifo(fifoPath, 0644); err != nil { + t.Skipf("FIFOs not supported: %v", err) + } + + // Opening a FIFO with no writer connected blocks forever, and nothing on + // this path has a timeout -- so run it off-goroutine and fail on the + // timeout rather than hanging the whole test binary. + done := make(chan struct{}) + var err error + go func() { + _, err = ReadFileContent(fifoPath, 0, 0) + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("ReadFileContent hung opening a FIFO instead of erroring") + } + + if err == nil { + t.Fatal("expected an error for a non-regular file target, got nil") + } + if !strings.Contains(err.Error(), "not a regular file") { + t.Errorf("expected a not-a-regular-file error, got %v", err) + } +} + +func TestTruncateRunes(t *testing.T) { + t.Run("truncates on a rune boundary and stays valid UTF-8", func(t *testing.T) { + // 3000 three-byte runes: byte-slicing at 2000 would land mid-rune. + long := strings.Repeat("世", 3000) + + got := truncateRunes(long, maxLineRunes) + + if !utf8.ValidString(got) { + t.Error("expected truncated output to be valid UTF-8") + } + if strings.ContainsRune(got, utf8.RuneError) { + t.Error("expected no U+FFFD replacement char from a split rune") + } + body := strings.TrimSuffix(got, "...") + if n := utf8.RuneCountInString(body); n != maxLineRunes { + t.Errorf("expected exactly %d runes before the ellipsis, got %d", maxLineRunes, n) + } + if !strings.HasSuffix(got, "...") { + t.Errorf("expected truncated output to end with '...', got %q", got) + } + }) + + t.Run("leaves a short string untouched", func(t *testing.T) { + if got := truncateRunes("héllo", maxLineRunes); got != "héllo" { + t.Errorf("expected input returned unchanged, got %q", got) + } + }) + + t.Run("counts characters not bytes, matching the Python runtime", func(t *testing.T) { + // 2000 multi-byte runes is 6000 bytes -- well over a byte-based + // limit, but exactly at the rune limit, so it must NOT truncate. + exact := strings.Repeat("世", maxLineRunes) + if got := truncateRunes(exact, maxLineRunes); got != exact { + t.Errorf("expected a string of exactly %d runes to be left alone, got %d runes", + maxLineRunes, utf8.RuneCountInString(got)) + } + }) +} + func TestWriteFileContent(t *testing.T) { tmpDir := createTempDir(t) defer os.RemoveAll(tmpDir) @@ -362,6 +436,58 @@ func TestListDirContent(t *testing.T) { t.Errorf("expected symlinked directory not to be listed as a file, got %q", result) } }) + + t.Run("symlink to a file reports the target's size, not the link's", func(t *testing.T) { + linkDir := createTempDir(t) + defer os.RemoveAll(linkDir) + + // The target's contents must be longer than the symlink's own + // "size" (the byte length of the stored target path) so the two are + // unambiguous: Lstat would report len(target path), Stat reports 20. + target := filepath.Join(linkDir, "target.txt") + contents := strings.Repeat("x", 20) + if err := os.WriteFile(target, []byte(contents), 0644); err != nil { + t.Fatalf("Failed to write target file: %v", err) + } + linkPath := filepath.Join(linkDir, "file-link") + if err := os.Symlink(target, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + result, err := ListDirContent(linkDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + if !strings.Contains(result, fmt.Sprintf("file-link\t%d", len(contents))) { + t.Errorf("expected symlinked file to report the target's size (%d), got %q", len(contents), result) + } + if strings.Contains(result, fmt.Sprintf("file-link\t%d", len(target))) { + t.Errorf("expected not to report the symlink's own size (%d), got %q", len(target), result) + } + }) + + t.Run("broken symlink is listed without a size", func(t *testing.T) { + linkDir := createTempDir(t) + defer os.RemoveAll(linkDir) + + linkPath := filepath.Join(linkDir, "broken-link") + if err := os.Symlink(filepath.Join(linkDir, "does-not-exist"), linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + result, err := ListDirContent(linkDir) + if err != nil { + t.Fatalf("ListDirContent() error = %v", err) + } + // Bare name, no size and no trailing slash -- matching Python's + // list_dir_content, whose stat() raises on a dangling link. + if !strings.Contains(result, "broken-link") { + t.Errorf("expected broken symlink to be listed, got %q", result) + } + if strings.Contains(result, "broken-link\t") || strings.Contains(result, "broken-link/") { + t.Errorf("expected broken symlink to be listed with no size and no trailing slash, got %q", result) + } + }) } func TestGrepContent(t *testing.T) { diff --git a/go/adk/pkg/tools/skills.go b/go/adk/pkg/tools/skills.go index 4e5fb4175..4af01fb8c 100644 --- a/go/adk/pkg/tools/skills.go +++ b/go/adk/pkg/tools/skills.go @@ -13,13 +13,15 @@ import ( ) // 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. +// are opt-in (disabled by default): they let an agent enumerate and search +// the filesystem under its session/skills roots without invoking a shell, so +// deployments that want to grant that visibility deliberately can, rather +// than having it enabled implicitly. // // Also registered (separately, for `kagent env` CLI discoverability only, -// not read here) as KagentEnableFileSearchTools in go/core/pkg/env/kagent.go -// -- keep both string literals in sync if this name ever changes. +// not read here) as KagentEnableFileSearchTools in go/core/pkg/env/kagent.go. +// TestEnableFileSearchToolsEnvMatchesRegistry pins the two literals together +// so they cannot drift. const enableFileSearchToolsEnv = "KAGENT_ENABLE_FILE_SEARCH_TOOLS" // fileSearchToolsEnabled accepts the same case-insensitive true-values as @@ -230,8 +232,8 @@ func NewSkillExecutionTools(skillsDirectory string) ([]tool.Tool, error) { tools := []tool.Tool{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. + // visibility, so deployments enable them deliberately. Note this gate is + // theirs alone -- bash below is always registered. fileSearchEnabled := fileSearchToolsEnabled() if fileSearchEnabled { listFilesTool, err := functiontool.New(functiontool.Config{ diff --git a/go/adk/pkg/tools/skills_test.go b/go/adk/pkg/tools/skills_test.go index d39d3ed41..8358f5a25 100644 --- a/go/adk/pkg/tools/skills_test.go +++ b/go/adk/pkg/tools/skills_test.go @@ -7,11 +7,27 @@ import ( "strings" "testing" + "github.com/kagent-dev/kagent/go/core/pkg/env" adkagent "google.golang.org/adk/v2/agent" "google.golang.org/adk/v2/tool" "google.golang.org/adk/v2/tool/toolconfirmation" ) +// TestEnableFileSearchToolsEnvMatchesRegistry pins this package's env var +// literal to the `kagent env` registry entry in go/core/pkg/env. The name is +// declared twice on purpose -- the runtime reads the raw variable here, while +// the registry exists solely so `kagent env` can document it -- so without +// this assertion nothing would catch the two drifting apart. +// +// This is a test-only import: it does not put a dependency on the +// control-plane module into the shipped agent runtime. +func TestEnableFileSearchToolsEnvMatchesRegistry(t *testing.T) { + if got, want := enableFileSearchToolsEnv, env.KagentEnableFileSearchTools.Name(); got != want { + t.Fatalf("env var name drift: %s has %q, go/core/pkg/env registry has %q", + "go/adk/pkg/tools/skills.go", got, want) + } +} + // 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. diff --git a/go/core/pkg/env/kagent.go b/go/core/pkg/env/kagent.go index f9d19001e..d76205861 100644 --- a/go/core/pkg/env/kagent.go +++ b/go/core/pkg/env/kagent.go @@ -90,14 +90,15 @@ var ( // 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. + // in go/adk/pkg/tools/skills.go's enableFileSearchToolsEnv. The two + // literals are pinned together by that package's + // TestEnableFileSearchToolsEnvMatchesRegistry. KagentEnableFileSearchTools = RegisterBoolVar( "KAGENT_ENABLE_FILE_SEARCH_TOOLS", false, "When true, enables the list_files and grep_file skills tools, which let an agent "+ - "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.", + "enumerate and search the filesystem under its session/skills roots without a "+ + "shell. Disabled by default; set on the Agent's env to opt in.", ComponentAgentRuntime, ) 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 aea3b70a2..eab692ac4 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 @@ -53,8 +53,8 @@ def add_skills_tool_to_agent( logger.debug(f"Added edit file tool to agent: {agent.name}") # list_files/grep_file are opt-in: they give an agent broad filesystem - # visibility, so some deployments want them off unless explicitly - # enabled, same as bash. + # visibility, so deployments enable them deliberately. Note this gate is + # theirs alone -- bash above is always registered. if file_search_tools_enabled(): if "list_files" not in existing_tool_names: agent.tools.append(ListFilesTool(skills_directory)) 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 579d588e1..b6f60e140 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/skills_toolset.py @@ -54,11 +54,11 @@ def __init__(self, skills_directory: str | Path): self.write_file_tool = WriteFileTool() self.edit_file_tool = EditFileTool() # list_files/grep_file are opt-in: they give an agent broad - # filesystem visibility, so 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. + # filesystem visibility, so deployments enable them deliberately. + # Note this gate is theirs alone -- bash below is always registered. + # A single list (rather than two separately-nullable attributes) + # keeps "both present or both absent" a structural guarantee instead + # of a convention the two attributes have to be kept in sync by hand. self._file_search_tools: list[BaseTool] = ( [ListFilesTool(skills_directory), GrepFileTool(skills_directory)] if file_search_tools_enabled() else [] ) diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index cd4b3d8aa..b335bd984 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -227,18 +227,34 @@ def grep_file(file_path: Path) -> list[str]: 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). + # is_file() follows symlinks and checks S_ISREG, so a False here + # covers two cases that deserve different treatment -- matching + # the split Go makes in classifyWalkEntry: + # - a broken symlink (or a symlink loop) is a genuine read + # failure, so count it, as Go's walkEntryUnreadable does. + # Otherwise a tree of dangling links reports a confidently + # empty "no matches found". + # - a FIFO/socket/device is excluded by policy, not failure + # (opening one can block indefinitely), so stay silent, as + # Go's walkEntrySkip does. if not entry.is_file(): + if entry.is_symlink() and not entry.exists(): + skipped += 1 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. + # Keep the read inside the session/skills sandbox. safe_entry = _validate_path(entry, allowed_root) except PermissionError: continue + # allowed_root is the whole session plus the skills dir, which is + # wider than the directory being searched -- so it alone would let + # a symlink here pull in a file from a sibling directory the caller + # never asked to search. Go bounds each entry by the search root + # (WithinRoot), and both the tool description and the README + # promise that behavior, so bound it here too. Silent, matching + # Go's walkEntrySkip for an out-of-root symlink. + if not safe_entry.is_relative_to(file_or_dir_path): + continue try: results.extend(grep_file(safe_entry)) except OSError: 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 c3592920d..f7a56cf54 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 @@ -394,6 +394,72 @@ def test_grep_content_recursive_skips_symlinks_that_escape_root(tmp_path): outside_dir.rmdir() +def test_grep_content_recursive_skips_symlinks_that_escape_the_searched_directory(tmp_path): + """A symlink may not pull in a file from outside the directory being searched. + + allowed_root in production is the whole session dir plus the skills dir -- + wider than the search root -- so validating against it alone would let a + link in the searched subdirectory read a sibling the caller never asked + about. Go bounds each entry by the search root; this pins the same rule. + """ + sibling = tmp_path / "sibling" + sibling.mkdir() + (sibling / "secret.txt").write_text("sibling foo\n") + + searched = tmp_path / "searched" + searched.mkdir() + (searched / "own.txt").write_text("own foo\n") + link = searched / "escape.txt" + + try: + link.symlink_to(sibling / "secret.txt") + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported") + + # allowed_root is tmp_path (the shared parent), so the link's target IS + # inside the sandbox -- only the search-root check can exclude it. + result = grep_content(searched, "foo", recursive=True, allowed_root=tmp_path) + + assert "own foo" in result + assert "sibling foo" not in result + + +def test_grep_content_counts_broken_symlink_as_unreadable(tmp_path): + """A dangling link is a genuine read failure and must not vanish silently.""" + (tmp_path / "match.txt").write_text("foo readable\n") + link = tmp_path / "broken.txt" + + try: + link.symlink_to(tmp_path / "does-not-exist.txt") + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported") + + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + + assert "match.txt:1:foo readable" in result + assert "could not be read" in result + + +def test_grep_content_does_not_count_fifo_as_unreadable(tmp_path): + """A FIFO is excluded by policy, not failure -- it must stay silent. + + Counting it would make every session dir containing a pipe report a + spurious "N entries could not be read", which is exactly the confusing + signal the annotation exists to avoid. + """ + (tmp_path / "match.txt").write_text("foo readable\n") + fifo_path = tmp_path / "pipe" + try: + os.mkfifo(fifo_path) + except (AttributeError, NotImplementedError, OSError): + pytest.skip("FIFOs not supported") + + result = grep_content(tmp_path, "foo", recursive=True, allowed_root=tmp_path) + + assert "match.txt:1:foo readable" in result + assert "could not be read" not in result + + def _call_with_timeout(fn, *args, timeout=5, **kwargs): """Run fn in a worker thread and fail loudly if it doesn't return in time. From d5fb009024aad8d266221a19ee5265fd54c9ac6d Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 26 Aug 2026 13:30:28 -0400 Subject: [PATCH 09/15] fix: share one bounded reader between read_file and grep_file grep_file's scanner set a 1MB line buffer; ReadFileContent kept bufio's 64KB default. A file with one longer line -- a minified bundle, a single-line JSON blob -- therefore failed read_file outright, losing every other line in the file, while grep_file handled the same file fine and read_file's own tool description promises such lines are truncated. Python truncated correctly throughout, so this was a Go-only regression introduced alongside grep_file. Extract scanFileLines as the single reader behind both. It owns the non-regular-file rejection (previously duplicated), the buffer cap, and error wrapping, so the two paths can no longer drift on any of the three. Lines past maxLineBytes still error rather than truncate: uncapping would mean buffering an arbitrarily long line in a sandbox reading untrusted files. Also in this change: - Wrap ListDirContent's os.ReadDir error, the last bare return beside a wrapped one. - Correct file_search_tools_enabled()'s docstring, which still claimed list_files/grep_file are "disabled by default, same as bash". Bash's gate was removed upstream in #2498; this was the last of six such sites. - Drop the _validate_path call in grep_content's entry loop. The search-root bound added beside it is strictly narrower, and file_or_dir_path is already validated against allowed_root, so the wider check is dead. - Give Python the _MAX_LINE_CHARS/_truncate_line pair Go already had, replacing four repetitions of the literal 2000. - Express the three path resolvers as pathPolicy values. allowSkillsRoot had been picking the denial message as a side effect, which would misdescribe any future resolver that denied the skills root for a reason other than writability. - Fold TestResolveReadPath_AllowsSymlinkedSkillsDirectory and TestResolveWritePath_BlocksSkillsSymlink into TestResolvePathContainment, which already covered both cells of that matrix. - Correct the grep_file call site's no-timeout rationale, which cited RE2's linearity -- an answer about the match, not the walk that GrepContent's own doc comment identifies as the unbounded part. Signed-off-by: brandonkeung --- go/adk/pkg/tools/shell.go | 122 +++++++++------ go/adk/pkg/tools/shell_test.go | 139 +++++++++++++++--- go/adk/pkg/tools/skills.go | 125 +++++++++------- go/adk/pkg/tools/skills_test.go | 132 +++++++++++++---- .../kagent-skills/src/kagent/skills/shell.py | 53 ++++--- .../tests/unittests/test_skill_execution.py | 23 +++ 6 files changed, 424 insertions(+), 170 deletions(-) diff --git a/go/adk/pkg/tools/shell.go b/go/adk/pkg/tools/shell.go index 0a337f396..63934a790 100644 --- a/go/adk/pkg/tools/shell.go +++ b/go/adk/pkg/tools/shell.go @@ -20,6 +20,53 @@ type CommandExecutor struct{} // truncating. Counted in runes, not bytes -- see truncateRunes. const maxLineRunes = 2000 +// maxLineBytes caps how much of a single line scanFileLines will buffer. +// Lines are emitted truncated to maxLineRunes, but grep has to see the whole +// line to match against it, so the read limit is far larger than the emit +// limit. A line longer than this makes its file unreadable, which callers +// surface as an error (read_file) or as a skip count (grep_file). +const maxLineBytes = 1024 * 1024 + +// scanFileLines opens path and hands each line to visit, stopping early if +// visit returns false. Line numbers are 1-based. +// +// This is the single reader behind read_file and grep_file. Both need the +// same three guarantees -- reject non-regular files, cap how much of a line +// is buffered, and surface failures wrapped -- and keeping them in one place +// is what stops the two from drifting apart: an earlier revision set the +// scanner buffer only in the grep path, which left read_file failing outright +// on any file with a line over bufio's 64KB default. +func scanFileLines(path string, visit func(lineNum int, line string) bool) error { + // Stat before opening: os.Open on a FIFO with no writer connected blocks + // indefinitely, and nothing on these paths imposes a timeout. Doing it + // here rather than at each call site means no future caller can omit it. + info, err := os.Stat(path) + if err != nil { + return fmt.Errorf("failed to stat %q: %w", path, err) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("%q is not a regular file", path) + } + + file, err := os.Open(path) + if err != nil { + return fmt.Errorf("failed to open %q: %w", path, err) + } + defer file.Close() + + scanner := bufio.NewScanner(file) + scanner.Buffer(make([]byte, 64*1024), maxLineBytes) + for lineNum := 1; scanner.Scan(); lineNum++ { + if !visit(lineNum, scanner.Text()) { + return nil + } + } + if err := scanner.Err(); err != nil { + return fmt.Errorf("failed to read %q: %w", path, err) + } + return nil +} + // truncateRunes shortens s to at most maxRunes code points, appending "..." // when it truncates. // @@ -82,42 +129,19 @@ func GetSessionPath(sessionID, skillsDirectory string) (string, error) { // ReadFileContent reads a file with line numbers. func ReadFileContent(path string, offset, limit int) (string, error) { - // Stat before opening: os.Open on a FIFO with no writer connected blocks - // indefinitely, and nothing on this path imposes a timeout. GrepContent - // rejects non-regular files for the same reason. - info, err := os.Stat(path) - if err != nil { - return "", fmt.Errorf("failed to stat %q: %w", path, err) - } - if !info.Mode().IsRegular() { - return "", fmt.Errorf("%q is not a regular file", path) - } - - file, err := os.Open(path) - if err != nil { - return "", fmt.Errorf("failed to open %q: %w", path, err) - } - defer file.Close() - var result strings.Builder - scanner := bufio.NewScanner(file) - lineNum := 1 start := max(offset, 1) count := 0 - for scanner.Scan() { - if lineNum >= start { - line := truncateRunes(scanner.Text(), maxLineRunes) - fmt.Fprintf(&result, "%6d|%s\n", lineNum, line) - count++ - if limit > 0 && count >= limit { - break - } + err := scanFileLines(path, func(lineNum int, line string) bool { + if lineNum < start { + return true } - lineNum++ - } - - if err := scanner.Err(); err != nil { + fmt.Fprintf(&result, "%6d|%s\n", lineNum, truncateRunes(line, maxLineRunes)) + count++ + return limit <= 0 || count < limit + }) + if err != nil { return "", err } @@ -183,7 +207,7 @@ func EditFileContent(path string, oldString, newString string, replaceAll bool) func ListDirContent(path string) (string, error) { entries, err := os.ReadDir(path) if err != nil { - return "", err + return "", fmt.Errorf("failed to read directory %q: %w", path, err) } if len(entries) == 0 { @@ -302,7 +326,13 @@ func classifyWalkEntry(root, p string, d fs.DirEntry) (walkEntryAction, string) // 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) { +// +// A recursive search walks the whole tree under path, which is unbounded from +// this function's point of view, so ctx is honored between entries and lets a +// caller abort one. Note Go needs no equivalent of the Python runtime's regex +// timeout: this uses RE2, which is linear-time and cannot backtrack +// catastrophically, so it is the walk rather than the match that can run long. +func GrepContent(ctx context.Context, path, pattern string, recursive, ignoreCase bool) (string, error) { expr := pattern if ignoreCase { expr = "(?i)" + expr @@ -320,22 +350,12 @@ 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 { - return err - } - 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) { + return scanFileLines(filePath, func(lineNum int, line string) bool { + if re.MatchString(line) { fmt.Fprintf(&result, "%s:%d:%s\n", filePath, lineNum, truncateRunes(line, maxLineRunes)) } - lineNum++ - } - return scanner.Err() + return true + }) } if info.IsDir() { @@ -355,6 +375,11 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro // symlink, WalkDir would see a non-directory at the root and never // descend into it at all. err = filepath.WalkDir(root, func(p string, d fs.DirEntry, walkErr error) error { + // Checked per entry rather than per line: a single file's scan is + // bounded by its size, but the number of entries is not. + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } if walkErr != nil { if p == root { // The search root itself couldn't be read (e.g. @@ -385,6 +410,9 @@ func GrepContent(path, pattern string, recursive, ignoreCase bool) (string, erro return nil }) } else { + // scanFileLines rejects non-regular files too, but reusing the stat + // already taken above lets an explicitly-targeted FIFO report the + // plain reason rather than one wrapped in "failed to search". if !info.Mode().IsRegular() { return "", fmt.Errorf("%q is not a regular file", path) } diff --git a/go/adk/pkg/tools/shell_test.go b/go/adk/pkg/tools/shell_test.go index 49f8a2cd6..1522364e6 100644 --- a/go/adk/pkg/tools/shell_test.go +++ b/go/adk/pkg/tools/shell_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -176,6 +177,63 @@ func TestReadFileContent(t *testing.T) { } } +// TestReadFileContent_LongLineIsTruncatedNotFatal pins the buffer that +// scanFileLines sets. bufio.Scanner's default cap is 64KB, and exceeding it +// fails the *whole* scan -- so before the shared reader existed, one long line +// (a minified bundle, a single-line JSON blob) made every other line in the +// file unreadable too, even though grep_file handled the same file fine and +// read_file's own tool description promises such lines are truncated. +func TestReadFileContent_LongLineIsTruncatedNotFatal(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + // Well past bufio's 64KB default, well under scanFileLines' maxLineBytes. + longLine := strings.Repeat("a", 100_000) + path := filepath.Join(tmpDir, "bundle.min.js") + if err := os.WriteFile(path, []byte(longLine+"\nsecond line\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + result, err := ReadFileContent(path, 0, 0) + if err != nil { + t.Fatalf("ReadFileContent() error = %v, want the long line truncated instead", err) + } + + lines := strings.Split(result, "\n") + if len(lines) != 2 { + t.Fatalf("expected 2 lines, got %d: %q", len(lines), result) + } + // Six-column line number, "|", then the truncated body and the ellipsis. + body := strings.TrimSuffix(strings.SplitN(lines[0], "|", 2)[1], "...") + if n := utf8.RuneCountInString(body); n != maxLineRunes { + t.Errorf("expected the long line truncated to %d runes, got %d", maxLineRunes, n) + } + if !strings.HasSuffix(lines[0], "...") { + t.Error("expected the truncated line to end with '...'") + } + // The whole point: the rest of the file survives. + if !strings.Contains(lines[1], "second line") { + t.Errorf("expected the line after the long one to still be read, got %q", lines[1]) + } +} + +// TestReadFileContent_LineOverMaxBytesErrors documents the deliberate residual +// limit: maxLineBytes exists so a sandboxed runtime can't be made to buffer an +// arbitrarily long line, so a line past it still fails rather than truncating. +func TestReadFileContent_LineOverMaxBytesErrors(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + path := filepath.Join(tmpDir, "huge.txt") + if err := os.WriteFile(path, []byte(strings.Repeat("a", maxLineBytes+1)+"\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + if _, err := ReadFileContent(path, 0, 0); err == nil { + t.Fatal("expected a line over maxLineBytes to error, got nil") + } +} + func TestReadFileContent_FIFOReturnsErrorInsteadOfHanging(t *testing.T) { fifoDir := createTempDir(t) defer os.RemoveAll(fifoDir) @@ -506,7 +564,7 @@ func TestGrepContent(t *testing.T) { } t.Run("matches within a single file", func(t *testing.T) { - result, err := GrepContent(filepath.Join(tmpDir, "a.txt"), "hello", false, false) + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "hello", false, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -516,7 +574,7 @@ func TestGrepContent(t *testing.T) { }) t.Run("no matches", func(t *testing.T) { - result, err := GrepContent(filepath.Join(tmpDir, "a.txt"), "nope", false, false) + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "nope", false, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -526,7 +584,7 @@ func TestGrepContent(t *testing.T) { }) t.Run("ignore case", func(t *testing.T) { - result, err := GrepContent(filepath.Join(tmpDir, "a.txt"), "foo", false, true) + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "foo", false, true) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -536,13 +594,13 @@ func TestGrepContent(t *testing.T) { }) t.Run("directory requires recursive", func(t *testing.T) { - if _, err := GrepContent(tmpDir, "foo", false, false); err == nil { + if _, err := GrepContent(context.Background(), tmpDir, "foo", false, false); err == nil { t.Fatal("expected error when searching a directory without recursive=true") } }) t.Run("recursive searches subdirectories", func(t *testing.T) { - result, err := GrepContent(tmpDir, "foo", true, true) + result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -552,7 +610,7 @@ func TestGrepContent(t *testing.T) { }) t.Run("invalid pattern", func(t *testing.T) { - if _, err := GrepContent(filepath.Join(tmpDir, "a.txt"), "(", false, false); err == nil { + if _, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "(", false, false); err == nil { t.Fatal("expected error for invalid regex pattern") } }) @@ -571,7 +629,7 @@ func TestGrepContent(t *testing.T) { } defer os.Remove(linkPath) - result, err := GrepContent(tmpDir, "foo", true, true) + result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -591,7 +649,7 @@ func TestGrepContent(t *testing.T) { } defer os.Remove(linkPath) - result, err := GrepContent(subDir, "foo via symlink", true, false) + result, err := GrepContent(context.Background(), subDir, "foo via symlink", true, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -638,7 +696,7 @@ func TestGrepContent(t *testing.T) { t.Fatalf("Failed to write test file: %v", err) } - result, err := GrepContent(walkDir, "foo", true, false) + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -666,7 +724,7 @@ func TestGrepContent(t *testing.T) { // Pass the unresolved symlink directly, as a caller that doesn't // pre-resolve its path would. - result, err := GrepContent(linkRoot, "foo", true, false) + result, err := GrepContent(context.Background(), linkRoot, "foo", true, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -694,7 +752,7 @@ func TestGrepContent(t *testing.T) { var result string var err error go func() { - result, err = GrepContent(fifoDir, "foo", true, false) + result, err = GrepContent(context.Background(), fifoDir, "foo", true, false) close(done) }() @@ -726,7 +784,7 @@ func TestGrepContent(t *testing.T) { done := make(chan struct{}) var err error go func() { - _, err = GrepContent(fifoPath, "foo", false, false) + _, err = GrepContent(context.Background(), fifoPath, "foo", false, false) close(done) }() @@ -769,7 +827,7 @@ func TestGrepContent(t *testing.T) { } defer os.Chmod(noPermSub, 0755) - result, err := GrepContent(walkDir, "foo", true, false) + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) if err != nil { t.Fatalf("GrepContent() error = %v, expected the unreadable subdirectory to be skipped rather than aborting the whole search", err) } @@ -798,7 +856,7 @@ func TestGrepContent(t *testing.T) { } defer os.Chmod(noPermSub, 0755) - result, err := GrepContent(walkDir, "foo", true, false) + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -835,7 +893,7 @@ func TestGrepContent(t *testing.T) { } defer os.Chmod(noPermSub, 0755) - result, err := GrepContent(walkDir, "foo", true, false) + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -862,7 +920,7 @@ func TestGrepContent(t *testing.T) { } defer os.Chmod(walkDir, 0755) - result, err := GrepContent(walkDir, "foo", true, false) + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) if err == nil { t.Fatalf("expected an error when the search root itself is unreadable, got a result instead: %q", result) } @@ -877,7 +935,7 @@ func TestGrepContent(t *testing.T) { t.Fatalf("Failed to write test file: %v", err) } - result, err := GrepContent(filepath.Join(tmpDir, "long.txt"), "foo", false, false) + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "long.txt"), "foo", false, false) if err != nil { t.Fatalf("GrepContent() error = %v", err) } @@ -888,6 +946,53 @@ func TestGrepContent(t *testing.T) { t.Errorf("expected result to be truncated to roughly 2000 chars, got length %d", len(result)) } }) + + t.Run("recursive search honors a cancelled context", func(t *testing.T) { + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + // Enough entries that the walk is guaranteed to consult ctx at least + // once after cancellation rather than finishing outright. + for i := range 50 { + name := filepath.Join(walkDir, fmt.Sprintf("file_%02d.txt", i)) + if err := os.WriteFile(name, []byte("foo\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // already cancelled before the walk starts + + _, err := GrepContent(ctx, walkDir, "foo", true, false) + if err == nil { + t.Fatal("expected a cancelled context to abort the search, got no error") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("expected the error to wrap context.Canceled, got %v", err) + } + }) + + t.Run("a non-recursive search is unaffected by context", func(t *testing.T) { + // Single-file reads are bounded by file size, so they intentionally + // don't consult ctx -- pinning that so the cancellation check isn't + // later "helpfully" moved somewhere it would break normal reads. + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + if err := os.WriteFile(filepath.Join(tmpDir, "a.txt"), []byte("foo\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + result, err := GrepContent(ctx, filepath.Join(tmpDir, "a.txt"), "foo", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "a.txt:1:foo") { + t.Errorf("expected the single-file match, got %q", result) + } + }) } func TestExecuteCommand(t *testing.T) { diff --git a/go/adk/pkg/tools/skills.go b/go/adk/pkg/tools/skills.go index 4af01fb8c..35a235189 100644 --- a/go/adk/pkg/tools/skills.go +++ b/go/adk/pkg/tools/skills.go @@ -276,7 +276,19 @@ func NewSkillExecutionTools(skillsDirectory string) ([]tool.Tool, error) { return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil } - content, err := GrepContent(path, in.Pattern, in.Recursive, in.IgnoreCase) + // ctx is the ADK tool context, which embeds context.Context, so a + // recursive search is abortable by whatever deadline or + // cancellation the caller already set -- and the walk, not the + // match, is the part that can run long here. + // + // No fixed deadline is added on top of that. The Python runtime + // caps grep at 30s, but that bound exists for a hazard Go doesn't + // have: Python's `re` backtracks, so an adversarial pattern can + // hang on a single line, while RE2 is linear-time. Since only the + // walk is unbounded, and its cost scales with the tree the + // deployment itself provisioned, a fixed cap here would break + // large legitimate searches without closing a distinct risk. + content, err := GrepContent(ctx, path, in.Pattern, in.Recursive, in.IgnoreCase) if err != nil { return fmt.Sprintf("Error searching %s: %v", strings.TrimSpace(in.Path), err), nil } @@ -323,7 +335,38 @@ func NewSkillExecutionTools(skillsDirectory string) ([]tool.Tool, error) { return tools, nil } -func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, error) { +// pathPolicy is the sandbox contract for one class of filesystem access. +// The three resolvers below are the same code differing only in these +// fields, so they are expressed as data rather than as flags threaded +// through a shared function. +type pathPolicy struct { + // resolve maps the requested leaf to a real path. EvalSymlinks requires + // it to already exist; resolvePathWithExistingParents tolerates a + // not-yet-created leaf, which only writes need. + resolve func(string) (string, error) + // allowSkillsRoot lets the read-only skills directory count as an allowed + // root. Reads may reach it; edits and writes must not, or an agent could + // modify the skills it was given. + allowSkillsRoot bool + // denied names the boundary in the error an out-of-bounds path gets. It + // is its own field rather than being derived from allowSkillsRoot: the + // two happen to correlate today, but a resolver that denied the skills + // root for a reason other than writability would otherwise be described + // wrongly. + denied string +} + +// TestResolvePathContainment pins this matrix for all three policies. +var ( + readPolicy = pathPolicy{filepath.EvalSymlinks, true, "the allowed roots"} + editPolicy = pathPolicy{filepath.EvalSymlinks, false, "the writable session directory"} + writePolicy = pathPolicy{resolvePathWithExistingParents, false, "the writable session directory"} +) + +// resolveSandboxedPath maps a requested path onto the session directory, +// resolves it under policy, and then requires the result to land inside an +// allowed root -- the check that keeps an agent inside its sandbox. +func resolveSandboxedPath(sessionID, skillsDirectory, requestedPath string, policy pathPolicy) (string, error) { sessionPath, err := GetSessionPath(sessionID, skillsDirectory) if err != nil { return "", err @@ -334,7 +377,7 @@ func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, return "", err } - resolvedCandidate, err := filepath.EvalSymlinks(candidate) + resolvedCandidate, err := policy.resolve(candidate) if err != nil { return "", err } @@ -343,70 +386,38 @@ func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, if err != nil { return "", err } - skillsRoot, err := filepath.EvalSymlinks(skillsDirectory) - if err != nil { - return "", err - } - - if !WithinRoot(resolvedCandidate, sessionRoot) && !WithinRoot(resolvedCandidate, skillsRoot) { - return "", fmt.Errorf("path %q is outside the allowed roots", requestedPath) - } + roots := []string{sessionRoot} - return resolvedCandidate, nil -} - -func resolveEditPath(sessionID, skillsDirectory, requestedPath string) (string, error) { - sessionPath, err := GetSessionPath(sessionID, skillsDirectory) - if err != nil { - return "", err + if policy.allowSkillsRoot { + // Resolved eagerly rather than only when the session root misses, so + // an unresolvable skills directory still surfaces as an error the way + // it did before these three were merged into one function. + skillsRoot, err := filepath.EvalSymlinks(skillsDirectory) + if err != nil { + return "", err + } + roots = append(roots, skillsRoot) } - candidate, err := resolveRequestedPath(sessionPath, requestedPath) - if err != nil { - return "", err + for _, root := range roots { + if WithinRoot(resolvedCandidate, root) { + return resolvedCandidate, nil + } } - resolvedCandidate, err := filepath.EvalSymlinks(candidate) - if err != nil { - return "", err - } + return "", fmt.Errorf("path %q is outside %s", requestedPath, policy.denied) +} - sessionRoot, err := filepath.EvalSymlinks(sessionPath) - if err != nil { - return "", err - } - if !WithinRoot(resolvedCandidate, sessionRoot) { - return "", fmt.Errorf("path %q is outside the writable session directory", requestedPath) - } +func resolveReadPath(sessionID, skillsDirectory, requestedPath string) (string, error) { + return resolveSandboxedPath(sessionID, skillsDirectory, requestedPath, readPolicy) +} - return resolvedCandidate, nil +func resolveEditPath(sessionID, skillsDirectory, requestedPath string) (string, error) { + return resolveSandboxedPath(sessionID, skillsDirectory, requestedPath, editPolicy) } func resolveWritePath(sessionID, skillsDirectory, requestedPath string) (string, error) { - sessionPath, err := GetSessionPath(sessionID, skillsDirectory) - if err != nil { - return "", err - } - - candidate, err := resolveRequestedPath(sessionPath, requestedPath) - if err != nil { - return "", err - } - - resolvedCandidate, err := resolvePathWithExistingParents(candidate) - if err != nil { - return "", err - } - - sessionRoot, err := filepath.EvalSymlinks(sessionPath) - if err != nil { - return "", err - } - if !WithinRoot(resolvedCandidate, sessionRoot) { - return "", fmt.Errorf("path %q is outside the writable session directory", requestedPath) - } - - return resolvedCandidate, nil + return resolveSandboxedPath(sessionID, skillsDirectory, requestedPath, writePolicy) } func resolveRequestedPath(basePath, requestedPath string) (string, error) { diff --git a/go/adk/pkg/tools/skills_test.go b/go/adk/pkg/tools/skills_test.go index 8358f5a25..8bf16e815 100644 --- a/go/adk/pkg/tools/skills_test.go +++ b/go/adk/pkg/tools/skills_test.go @@ -65,38 +65,112 @@ func runTool(t *testing.T, tl tool.Tool, ctx adkagent.Context, args map[string]a return text } -func TestResolveReadPath_AllowsSymlinkedSkillsDirectory(t *testing.T) { - t.Setenv("TMPDIR", t.TempDir()) - skillsDir := t.TempDir() - skillFile := filepath.Join(skillsDir, "script.py") - if err := os.WriteFile(skillFile, []byte("print('ok')\n"), 0644); err != nil { - t.Fatalf("failed to write skill file: %v", err) +// TestResolvePathContainment pins the sandbox boundary each resolver +// enforces. The three differ along exactly two axes -- whether the skills +// directory is an allowed root, and whether a not-yet-existing leaf is +// tolerated -- and those differences are the whole security contract: +// +// session dir skills dir missing leaf +// resolveReadPath allow allow reject +// resolveEditPath allow REJECT reject +// resolveWritePath allow REJECT allow +// +// resolveEditPath in particular had no direct coverage before this, so a +// refactor could have silently granted it the skills root -- letting an +// agent edit read-only skill files -- with nothing failing. +func TestResolvePathContainment(t *testing.T) { + resolvers := map[string]struct { + fn func(sessionID, skillsDirectory, requestedPath string) (string, error) + allowsSkillsDir bool + allowsNewLeaf bool + // deniedContains is the boundary each policy names when it rejects a + // path, kept here so the wording stays tied to the resolver it + // describes rather than drifting into a generic message. + deniedContains string + }{ + "read": {resolveReadPath, true, false, "outside the allowed roots"}, + "edit": {resolveEditPath, false, false, "outside the writable session directory"}, + "write": {resolveWritePath, false, true, "outside the writable session directory"}, } - sessionID := fmt.Sprintf("%s-read", t.Name()) - resolved, err := resolveReadPath(sessionID, skillsDir, "skills/script.py") - if err != nil { - t.Fatalf("resolveReadPath() error = %v", err) - } - want, err := filepath.EvalSymlinks(skillFile) - if err != nil { - t.Fatalf("EvalSymlinks(skillFile) error = %v", err) - } - if resolved != want { - t.Fatalf("resolveReadPath() = %q, want %q", resolved, want) - } -} + for name, r := range resolvers { + t.Run(name, func(t *testing.T) { + t.Setenv("TMPDIR", t.TempDir()) + skillsDir := t.TempDir() + if err := os.WriteFile(filepath.Join(skillsDir, "script.py"), []byte("print('ok')\n"), 0644); err != nil { + t.Fatalf("failed to write skill file: %v", err) + } + sessionID := fmt.Sprintf("%s-%s", t.Name(), name) -func TestResolveWritePath_BlocksSkillsSymlink(t *testing.T) { - t.Setenv("TMPDIR", t.TempDir()) - skillsDir := t.TempDir() - sessionID := fmt.Sprintf("%s-write", t.Name()) - _, err := resolveWritePath(sessionID, skillsDir, "skills/new-file.txt") - if err == nil { - t.Fatal("expected write through skills symlink to be rejected") - } - if !strings.Contains(err.Error(), "outside the writable session directory") { - t.Fatalf("unexpected error: %v", err) + // Seed a real file in the session dir so the "existing leaf" + // cases have something to resolve to. + sessionPath, err := GetSessionPath(sessionID, skillsDir) + if err != nil { + t.Fatalf("GetSessionPath() error = %v", err) + } + if err := os.WriteFile(filepath.Join(sessionPath, "notes.txt"), []byte("hi\n"), 0644); err != nil { + t.Fatalf("failed to seed session file: %v", err) + } + + t.Run("allows a path inside the session directory", func(t *testing.T) { + if _, err := r.fn(sessionID, skillsDir, "notes.txt"); err != nil { + t.Errorf("expected session-dir path to be allowed, got %v", err) + } + }) + + t.Run("rejects a path outside every root", func(t *testing.T) { + _, err := r.fn(sessionID, skillsDir, "/etc/passwd") + if err == nil { + t.Fatal("expected a path outside the allowed roots to be rejected") + } + if !strings.Contains(err.Error(), r.deniedContains) { + t.Errorf("expected the error to name %q, got %v", r.deniedContains, err) + } + }) + + t.Run("skills directory", func(t *testing.T) { + resolved, err := r.fn(sessionID, skillsDir, "skills/script.py") + if r.allowsSkillsDir { + if err != nil { + t.Fatalf("expected the skills dir to be readable, got %v", err) + } + // The skills dir is reached through a symlink in the + // session dir, so a resolver that returned the unresolved + // path would still look "allowed" -- assert it resolved + // through to the real file. + want, evalErr := filepath.EvalSymlinks(filepath.Join(skillsDir, "script.py")) + if evalErr != nil { + t.Fatalf("EvalSymlinks() error = %v", evalErr) + } + if resolved != want { + t.Errorf("resolved to %q, want %q", resolved, want) + } + return + } + if err == nil { + t.Fatal("expected the read-only skills dir to be rejected for mutation") + } + if !strings.Contains(err.Error(), r.deniedContains) { + t.Errorf("expected the error to name %q, got %v", r.deniedContains, err) + } + }) + + t.Run("not-yet-existing leaf", func(t *testing.T) { + _, err := r.fn(sessionID, skillsDir, "brand-new-file.txt") + if r.allowsNewLeaf && err != nil { + t.Errorf("expected a new file in the session dir to be allowed, got %v", err) + } + if !r.allowsNewLeaf && err == nil { + t.Error("expected a nonexistent path to be rejected") + } + }) + + t.Run("rejects traversal escaping the session directory", func(t *testing.T) { + if _, err := r.fn(sessionID, skillsDir, "../../../etc/passwd"); err == nil { + t.Error("expected ../ traversal out of the session dir to be rejected") + } + }) + }) } } diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index b335bd984..8a3698acb 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -13,6 +13,19 @@ # --- File Operation Tools --- +# Longest line read_file and grep_file will emit before truncating, counted in +# characters. Mirrors maxLineRunes in go/adk/pkg/tools/shell.go, and the +# "Lines longer than 2000 characters are truncated" both runtimes' read_file +# descriptions promise (see prompts.py). +_MAX_LINE_CHARS = 2000 + + +def _truncate_line(line: str) -> str: + """Shorten line to at most _MAX_LINE_CHARS characters, marking a cut with "...".""" + if len(line) > _MAX_LINE_CHARS: + return line[:_MAX_LINE_CHARS] + "..." + return line + def _validate_path( file_path: Path, @@ -57,9 +70,7 @@ def read_file_content( result_lines = [] for i, line in enumerate(lines[start:end], start=start + 1): - if len(line) > 2000: - line = line[:2000] + "..." - result_lines.append(f"{i:6d}|{line}") + result_lines.append(f"{i:6d}|{_truncate_line(line)}") if not result_lines: return "File is empty." @@ -191,9 +202,7 @@ 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}") + matches.append(f"{file_path}:{line_num}:{_truncate_line(line)}") return matches results: list[str] = [] @@ -241,18 +250,19 @@ def grep_file(file_path: Path) -> list[str]: if entry.is_symlink() and not entry.exists(): skipped += 1 continue - try: - # Keep the read inside the session/skills sandbox. - safe_entry = _validate_path(entry, allowed_root) - except PermissionError: - continue - # allowed_root is the whole session plus the skills dir, which is - # wider than the directory being searched -- so it alone would let - # a symlink here pull in a file from a sibling directory the caller - # never asked to search. Go bounds each entry by the search root - # (WithinRoot), and both the tool description and the README - # promise that behavior, so bound it here too. Silent, matching - # Go's walkEntrySkip for an out-of-root symlink. + # Bound each entry by the directory actually being searched, not + # by allowed_root: allowed_root is the whole session plus the + # skills dir, so on its own it would let a symlink here pull in a + # file from a sibling directory the caller never asked to search. + # Go bounds each entry the same way (WithinRoot in + # classifyWalkEntry), and both the tool description and the README + # promise that behavior. Silent, matching Go's walkEntrySkip for an + # out-of-root symlink. + # + # This subsumes a _validate_path(entry, allowed_root) check: + # file_or_dir_path was itself validated against allowed_root above, + # so anything relative to it is transitively inside a root. + safe_entry = entry.resolve() if not safe_entry.is_relative_to(file_or_dir_path): continue try: @@ -318,8 +328,11 @@ def _sanitize_env(env: dict[str, str] | None = None) -> dict[str, str]: 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. + Opt-in (disabled by default): they let an agent enumerate and search the + filesystem under its session/skills roots without invoking a shell, so + deployments that want to grant that visibility do so deliberately rather + than having it enabled implicitly. Note this gate is theirs alone -- bash + is always registered. """ return os.environ.get(_ENABLE_FILE_SEARCH_TOOLS_ENV, "").strip().lower() in ("1", "t", "true") 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 f7a56cf54..f26a50e15 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 @@ -237,6 +237,29 @@ def test_read_file_allows_path_inside_root(tmp_path): assert "hello world" in result +def test_read_file_truncates_a_long_line_without_failing_the_file(tmp_path): + """A line past the truncation limit is shortened, not fatal. + + The Go twin of this (TestReadFileContent_LongLineIsTruncatedNotFatal) is + the one that actually caught a bug: Go's scanner had a hard 64KB cap that + failed the entire read. Keeping both pins the promise each runtime's + read_file description makes -- "Lines longer than 2000 characters are + truncated" -- to the same observable behavior. + """ + f = tmp_path / "bundle.min.js" + f.write_text("a" * 100_000 + "\nsecond line\n") + + result = read_file_content(f, allowed_root=tmp_path) + + lines = result.split("\n") + assert len(lines) == 2 + body = lines[0].split("|", 1)[1] + assert body.endswith("...") + assert len(body.removesuffix("...")) == 2000 + # The whole point: the rest of the file survives. + assert "second line" in lines[1] + + def test_read_file_allows_multiple_roots(tmp_path): """Read should succeed when the file is inside any of the allowed roots.""" skills_dir = tmp_path / "skills" From 8064ed81ba6327a3a974c27cba72821cbba56767 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 26 Aug 2026 13:47:50 -0400 Subject: [PATCH 10/15] refactor: split the grep subsystem out of shell.go Pure move -- no logic changes. The bodies are byte-identical to their previous location except for three added comment lines on WithinRoot, noting that resolveSandboxedPath is its second caller. shell.go is named for shell execution but had accumulated a directory-walking search engine: walkEntryAction, classifyWalkEntry, WithinRoot, and GrepContent share no state and no callers with CommandExecutor or the file primitives. shell_test.go had also reached 1152 lines, 446 of them TestGrepContent alone. shell.go 492 -> 308 grep.go (new) 197 shell_test.go 1152 -> 704 grep_test.go (new) 460 shell.go drops its regexp import and shell_test.go its errors import; both were used only by the moved code. WithinRoot moves with grep rather than staying behind, where it would have had no local caller left. Signed-off-by: brandonkeung --- go/adk/pkg/tools/grep.go | 197 ++++++++++++++ go/adk/pkg/tools/grep_test.go | 460 +++++++++++++++++++++++++++++++++ go/adk/pkg/tools/shell.go | 184 ------------- go/adk/pkg/tools/shell_test.go | 448 -------------------------------- 4 files changed, 657 insertions(+), 632 deletions(-) create mode 100644 go/adk/pkg/tools/grep.go create mode 100644 go/adk/pkg/tools/grep_test.go diff --git a/go/adk/pkg/tools/grep.go b/go/adk/pkg/tools/grep.go new file mode 100644 index 000000000..10f864559 --- /dev/null +++ b/go/adk/pkg/tools/grep.go @@ -0,0 +1,197 @@ +package tools + +import ( + "context" + "fmt" + "io/fs" + "os" + "path/filepath" + "regexp" + "strings" +) + +// WithinRoot reports whether resolved (an already symlink-resolved path) is +// root itself or nested under it. Callers are responsible for resolving +// symlinks on both arguments first; this is a pure path-containment check. +// +// It lives here with classifyWalkEntry, its main caller, but is also the +// containment test behind resolveSandboxedPath in skills.go. +func WithinRoot(resolved, root string) bool { + resolved = filepath.Clean(resolved) + root = filepath.Clean(root) + return resolved == root || strings.HasPrefix(resolved, root+string(filepath.Separator)) +} + +type walkEntryAction int + +const ( + // walkEntryGrep: a regular, in-bounds file that should be grepped. + walkEntryGrep walkEntryAction = iota + // walkEntrySkip: silently excluded by policy, not a read failure -- a + // directory, a symlinked directory, a non-regular file (FIFO/socket/ + // device), or a symlink whose target escapes the search root. + walkEntrySkip + // walkEntryUnreadable: a genuine read/stat failure on this entry. + walkEntryUnreadable +) + +// classifyWalkEntry decides how GrepContent's WalkDir callback should treat +// p, given the resolved search root. It never opens p for reading -- the +// caller is responsible for that once this returns walkEntryGrep, and must +// read the returned resolved path rather than p itself: p is the symlink +// as walked, and re-resolving or reopening it separately from this check +// would reintroduce a TOCTOU window where the symlink's target could change +// between the check and the read. +// +// This narrows that window but doesn't eliminate it: resolved is still a +// path string, so the later os.Open on it is a fresh lookup, not an +// operation on a captured file handle. A path component of resolved could +// still be swapped out between this check and that open. Closing that +// residual race fully would need platform-specific work (e.g. Linux's +// openat2 with RESOLVE_NO_SYMLINKS), which isn't done anywhere else in this +// file either -- this function only guarantees "reads what was just +// verified", not "reads atomically with no concurrent tampering". +func classifyWalkEntry(root, p string, d fs.DirEntry) (walkEntryAction, string) { + if d.IsDir() { + return walkEntrySkip, "" + } + resolved, err := filepath.EvalSymlinks(p) + if err != nil { + return walkEntryUnreadable, "" + } + fi, statErr := os.Stat(resolved) + if statErr != nil { + return walkEntryUnreadable, "" + } + if fi.IsDir() { + // p is a symlink to a directory: WalkDir doesn't recurse into + // symlinked directories, and grepFile would fail trying to read + // one as a file, so skip it rather than treating it as an error. + return walkEntrySkip, "" + } + if !fi.Mode().IsRegular() { + // Skip non-regular files (FIFOs, sockets, devices): opening one + // for reading can block indefinitely (e.g. a FIFO with no writer + // connected), and grep has no business reading them. + return walkEntrySkip, "" + } + if !WithinRoot(resolved, root) { + // The symlink-resolved target escapes the root being searched, so + // a symlink can't be used to read files outside the requested + // directory. + return walkEntrySkip, "" + } + return walkEntryGrep, resolved +} + +// GrepContent searches path for lines matching a regular expression pattern. +// If path is a directory, recursive must be true to search its files. +// +// A recursive search walks the whole tree under path, which is unbounded from +// this function's point of view, so ctx is honored between entries and lets a +// caller abort one. Note Go needs no equivalent of the Python runtime's regex +// timeout: this uses RE2, which is linear-time and cannot backtrack +// catastrophically, so it is the walk rather than the match that can run long. +func GrepContent(ctx context.Context, path, pattern string, recursive, ignoreCase bool) (string, error) { + expr := pattern + if ignoreCase { + expr = "(?i)" + expr + } + re, err := regexp.Compile(expr) + if err != nil { + return "", fmt.Errorf("invalid pattern: %w", err) + } + + info, err := os.Stat(path) + if err != nil { + return "", fmt.Errorf("failed to stat %q: %w", path, err) + } + + var result strings.Builder + var skipped int + grepFile := func(filePath string) error { + return scanFileLines(filePath, func(lineNum int, line string) bool { + if re.MatchString(line) { + fmt.Fprintf(&result, "%s:%d:%s\n", filePath, lineNum, truncateRunes(line, maxLineRunes)) + } + return true + }) + } + + if info.IsDir() { + if !recursive { + return "", fmt.Errorf("%q is a directory; set recursive=true to search directories", path) + } + // Reuse the outer err (rather than := , which would shadow it in this + // block) so a WalkDir failure below is actually observed by the + // err != nil check after this if/else. + var root string + root, err = filepath.EvalSymlinks(path) + if err != nil { + return "", fmt.Errorf("failed to resolve %q: %w", path, err) + } + // Walk the resolved root, not path: filepath.WalkDir uses Lstat on + // its root argument, so if path itself were an unresolved directory + // symlink, WalkDir would see a non-directory at the root and never + // descend into it at all. + err = filepath.WalkDir(root, func(p string, d fs.DirEntry, walkErr error) error { + // Checked per entry rather than per line: a single file's scan is + // bounded by its size, but the number of entries is not. + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } + if walkErr != nil { + if p == root { + // The search root itself couldn't be read (e.g. + // permission denied): the search never actually ran, so + // surface a real error instead of a misleadingly + // confident "no matches found". + return walkErr + } + // WalkDir surfaces ReadDir/Lstat failures (e.g. a + // permission-denied subdirectory) through this err rather + // than via grepFile, but it deserves the same treatment: one + // unreadable subtree shouldn't discard matches already found + // in its siblings. + skipped++ + return nil + } + switch action, resolved := classifyWalkEntry(root, p, d); action { + case walkEntryUnreadable: + skipped++ + case walkEntryGrep: + // A read error on one file (permission denied, a line + // exceeding the scan buffer, etc.) shouldn't abort matches + // already found elsewhere in the tree. + if grepErr := grepFile(resolved); grepErr != nil { + skipped++ + } + } + return nil + }) + } else { + // scanFileLines rejects non-regular files too, but reusing the stat + // already taken above lets an explicitly-targeted FIFO report the + // plain reason rather than one wrapped in "failed to search". + if !info.Mode().IsRegular() { + return "", fmt.Errorf("%q is not a regular file", path) + } + err = grepFile(path) + } + if err != nil { + return "", fmt.Errorf("failed to search %q: %w", path, err) + } + + if result.Len() == 0 { + if skipped > 0 { + return fmt.Sprintf("no matches found (%d entries could not be read)", skipped), nil + } + return "no matches found", nil + } + + matches := strings.TrimSuffix(result.String(), "\n") + if skipped > 0 { + matches += fmt.Sprintf("\n\n(%d entries could not be read)", skipped) + } + return matches, nil +} diff --git a/go/adk/pkg/tools/grep_test.go b/go/adk/pkg/tools/grep_test.go new file mode 100644 index 000000000..b13e22c4d --- /dev/null +++ b/go/adk/pkg/tools/grep_test.go @@ -0,0 +1,460 @@ +package tools + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + "testing" + "time" +) + +func TestGrepContent(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + if err := os.WriteFile(filepath.Join(tmpDir, "a.txt"), []byte("hello world\nFOO bar\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + subDir := filepath.Join(tmpDir, "sub") + if err := os.Mkdir(subDir, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(subDir, "b.txt"), []byte("another foo line\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + t.Run("matches within a single file", func(t *testing.T) { + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "hello", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "a.txt:1:hello world") { + t.Errorf("expected match with path:line:content, got %q", result) + } + }) + + t.Run("no matches", func(t *testing.T) { + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "nope", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if result != "no matches found" { + t.Errorf("expected no matches message, got %q", result) + } + }) + + t.Run("ignore case", func(t *testing.T) { + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "foo", false, true) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "FOO bar") { + t.Errorf("expected case-insensitive match, got %q", result) + } + }) + + t.Run("directory requires recursive", func(t *testing.T) { + if _, err := GrepContent(context.Background(), tmpDir, "foo", false, false); err == nil { + t.Fatal("expected error when searching a directory without recursive=true") + } + }) + + t.Run("recursive searches subdirectories", func(t *testing.T) { + result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "b.txt:1:another foo line") { + t.Errorf("expected match from subdirectory, got %q", result) + } + }) + + t.Run("invalid pattern", func(t *testing.T) { + if _, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "(", false, false); err == nil { + t.Fatal("expected error for invalid regex pattern") + } + }) + + t.Run("recursive search skips symlinks that escape the root", func(t *testing.T) { + outsideDir := createTempDir(t) + defer os.RemoveAll(outsideDir) + secretPath := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(secretPath, []byte("top secret foo\n"), 0644); err != nil { + t.Fatalf("Failed to write outside file: %v", err) + } + + linkPath := filepath.Join(subDir, "escape.txt") + if err := os.Symlink(secretPath, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if strings.Contains(result, "top secret") { + t.Errorf("expected symlinked file outside root to be skipped, got %q", result) + } + }) + + t.Run("recursive search greps the resolved target of an in-bounds file symlink", func(t *testing.T) { + realFile := filepath.Join(subDir, "real_target.txt") + if err := os.WriteFile(realFile, []byte("foo via symlink\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + linkPath := filepath.Join(subDir, "file_link.txt") + if err := os.Symlink(realFile, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + result, err := GrepContent(context.Background(), subDir, "foo via symlink", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + // The match must be reported against the resolved target path + // (real_target.txt), not the walked symlink path (file_link.txt): + // classifyWalkEntry verifies the resolved target is in-bounds, and + // the actual read must use that same resolved value rather than + // re-deriving/reopening the raw symlink path, or the verified-safe + // check and the actual read could diverge (TOCTOU). + if !strings.Contains(result, "real_target.txt:1:foo via symlink") { + t.Errorf("expected match to be reported against the resolved target path, got %q", result) + } + if strings.Contains(result, "file_link.txt:") { + t.Errorf("expected match not to be reported against the unresolved symlink path, got %q", result) + } + }) + + t.Run("recursive search does not abort on an in-bounds directory symlink", func(t *testing.T) { + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + if err := os.WriteFile(filepath.Join(walkDir, "aaa_first.txt"), []byte("foo one\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + realSub := filepath.Join(walkDir, "real_sub") + if err := os.Mkdir(realSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + + // A symlink to an in-bounds directory, lexically sorted between the + // two files below, so an incorrect abort partway through the walk + // would silently drop "zzz_sub"'s match. + linkPath := filepath.Join(walkDir, "mmm_link") + if err := os.Symlink(realSub, linkPath); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + defer os.Remove(linkPath) + + zzzSub := filepath.Join(walkDir, "zzz_sub") + if err := os.Mkdir(zzzSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(zzzSub, "zzz_last.txt"), []byte("foo two\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "aaa_first.txt:1:foo one") { + t.Errorf("expected match before the symlink, got %q", result) + } + if !strings.Contains(result, "zzz_last.txt:1:foo two") { + t.Errorf("expected match after the symlink (walk must not abort on it), got %q", result) + } + }) + + t.Run("recursive search resolves the root itself when it is an unresolved symlink", func(t *testing.T) { + realDir := createTempDir(t) + defer os.RemoveAll(realDir) + if err := os.WriteFile(filepath.Join(realDir, "match.txt"), []byte("foo inside\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + parentDir := createTempDir(t) + defer os.RemoveAll(parentDir) + linkRoot := filepath.Join(parentDir, "link-root") + if err := os.Symlink(realDir, linkRoot); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + // Pass the unresolved symlink directly, as a caller that doesn't + // pre-resolve its path would. + result, err := GrepContent(context.Background(), linkRoot, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "match.txt:1:foo inside") { + t.Errorf("expected GrepContent to resolve a symlinked root and recurse into it, got %q", result) + } + }) + + t.Run("recursive search does not hang on a FIFO and finds matches around it", func(t *testing.T) { + fifoDir := createTempDir(t) + defer os.RemoveAll(fifoDir) + + if err := os.WriteFile(filepath.Join(fifoDir, "aaa_before.txt"), []byte("foo before\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + fifoPath := filepath.Join(fifoDir, "mmm_pipe") + if err := syscall.Mkfifo(fifoPath, 0644); err != nil { + t.Skipf("FIFOs not supported: %v", err) + } + if err := os.WriteFile(filepath.Join(fifoDir, "zzz_after.txt"), []byte("foo after\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + done := make(chan struct{}) + var result string + var err error + go func() { + result, err = GrepContent(context.Background(), fifoDir, "foo", true, false) + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("GrepContent hung on a FIFO instead of skipping it") + } + + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "aaa_before.txt:1:foo before") { + t.Errorf("expected match before the FIFO, got %q", result) + } + if !strings.Contains(result, "zzz_after.txt:1:foo after") { + t.Errorf("expected match after the FIFO (walk must not hang or abort on it), got %q", result) + } + }) + + t.Run("a single target FIFO returns an error instead of hanging", func(t *testing.T) { + fifoDir := createTempDir(t) + defer os.RemoveAll(fifoDir) + fifoPath := filepath.Join(fifoDir, "pipe") + if err := syscall.Mkfifo(fifoPath, 0644); err != nil { + t.Skipf("FIFOs not supported: %v", err) + } + + done := make(chan struct{}) + var err error + go func() { + _, err = GrepContent(context.Background(), fifoPath, "foo", false, false) + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("GrepContent hung opening a FIFO directly instead of erroring") + } + + if err == nil { + t.Fatal("expected an error for a non-regular file target, got nil") + } + }) + + t.Run("recursive search does not abort or discard matches on an unreadable subdirectory", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + okSub := filepath.Join(walkDir, "aaa_ok") + if err := os.Mkdir(okSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(okSub, "match.txt"), []byte("foo readable\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + noPermSub := filepath.Join(walkDir, "mmm_noperm") + if err := os.Mkdir(noPermSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(noPermSub, 0000); err != nil { + t.Fatalf("Failed to chmod subdir: %v", err) + } + defer os.Chmod(noPermSub, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v, expected the unreadable subdirectory to be skipped rather than aborting the whole search", err) + } + if !strings.Contains(result, "match.txt:1:foo readable") { + t.Errorf("expected match from the readable sibling directory to survive an unreadable subdirectory elsewhere in the tree, got %q", result) + } + }) + + t.Run("no matches found is annotated when entries could not be read", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + noPermSub := filepath.Join(walkDir, "noperm") + if err := os.Mkdir(noPermSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(noPermSub, 0000); err != nil { + t.Fatalf("Failed to chmod subdir: %v", err) + } + defer os.Chmod(noPermSub, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "no matches found") || !strings.Contains(result, "could not be read") { + t.Errorf("expected an annotated no-matches message noting unreadable entries, got %q", result) + } + }) + + t.Run("matches are annotated with the skip count when some entries could not be read", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + okSub := filepath.Join(walkDir, "aaa_ok") + if err := os.Mkdir(okSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(okSub, "match.txt"), []byte("foo readable\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + noPermSub := filepath.Join(walkDir, "mmm_noperm") + if err := os.Mkdir(noPermSub, 0755); err != nil { + t.Fatalf("Failed to create subdir: %v", err) + } + if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(noPermSub, 0000); err != nil { + t.Fatalf("Failed to chmod subdir: %v", err) + } + defer os.Chmod(noPermSub, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "match.txt:1:foo readable") { + t.Errorf("expected the real match to still be reported, got %q", result) + } + if !strings.Contains(result, "could not be read") { + t.Errorf("expected the skip count to be reported alongside real matches, not silently dropped, got %q", result) + } + }) + + t.Run("recursive search on a fully unreadable root returns an error, not a confident empty result", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions; cannot exercise this case") + } + + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + if err := os.WriteFile(filepath.Join(walkDir, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + if err := os.Chmod(walkDir, 0000); err != nil { + t.Fatalf("Failed to chmod root: %v", err) + } + defer os.Chmod(walkDir, 0755) + + result, err := GrepContent(context.Background(), walkDir, "foo", true, false) + if err == nil { + t.Fatalf("expected an error when the search root itself is unreadable, got a result instead: %q", result) + } + }) + + t.Run("matched lines are truncated to 2000 characters", func(t *testing.T) { + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + + longLine := "foo " + strings.Repeat("x", 3000) + if err := os.WriteFile(filepath.Join(tmpDir, "long.txt"), []byte(longLine+"\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "long.txt"), "foo", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.HasSuffix(result, "...") { + t.Errorf("expected truncated line to end with '...', got %q", result) + } + if len(result) > 2100 { + t.Errorf("expected result to be truncated to roughly 2000 chars, got length %d", len(result)) + } + }) + + t.Run("recursive search honors a cancelled context", func(t *testing.T) { + walkDir := createTempDir(t) + defer os.RemoveAll(walkDir) + + // Enough entries that the walk is guaranteed to consult ctx at least + // once after cancellation rather than finishing outright. + for i := range 50 { + name := filepath.Join(walkDir, fmt.Sprintf("file_%02d.txt", i)) + if err := os.WriteFile(name, []byte("foo\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // already cancelled before the walk starts + + _, err := GrepContent(ctx, walkDir, "foo", true, false) + if err == nil { + t.Fatal("expected a cancelled context to abort the search, got no error") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("expected the error to wrap context.Canceled, got %v", err) + } + }) + + t.Run("a non-recursive search is unaffected by context", func(t *testing.T) { + // Single-file reads are bounded by file size, so they intentionally + // don't consult ctx -- pinning that so the cancellation check isn't + // later "helpfully" moved somewhere it would break normal reads. + tmpDir := createTempDir(t) + defer os.RemoveAll(tmpDir) + if err := os.WriteFile(filepath.Join(tmpDir, "a.txt"), []byte("foo\n"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + result, err := GrepContent(ctx, filepath.Join(tmpDir, "a.txt"), "foo", false, false) + if err != nil { + t.Fatalf("GrepContent() error = %v", err) + } + if !strings.Contains(result, "a.txt:1:foo") { + t.Errorf("expected the single-file match, got %q", result) + } + }) +} diff --git a/go/adk/pkg/tools/shell.go b/go/adk/pkg/tools/shell.go index 63934a790..a730775eb 100644 --- a/go/adk/pkg/tools/shell.go +++ b/go/adk/pkg/tools/shell.go @@ -9,7 +9,6 @@ import ( "os" "os/exec" "path/filepath" - "regexp" "strings" "time" ) @@ -253,189 +252,6 @@ 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)) -} - -type walkEntryAction int - -const ( - // walkEntryGrep: a regular, in-bounds file that should be grepped. - walkEntryGrep walkEntryAction = iota - // walkEntrySkip: silently excluded by policy, not a read failure -- a - // directory, a symlinked directory, a non-regular file (FIFO/socket/ - // device), or a symlink whose target escapes the search root. - walkEntrySkip - // walkEntryUnreadable: a genuine read/stat failure on this entry. - walkEntryUnreadable -) - -// classifyWalkEntry decides how GrepContent's WalkDir callback should treat -// p, given the resolved search root. It never opens p for reading -- the -// caller is responsible for that once this returns walkEntryGrep, and must -// read the returned resolved path rather than p itself: p is the symlink -// as walked, and re-resolving or reopening it separately from this check -// would reintroduce a TOCTOU window where the symlink's target could change -// between the check and the read. -// -// This narrows that window but doesn't eliminate it: resolved is still a -// path string, so the later os.Open on it is a fresh lookup, not an -// operation on a captured file handle. A path component of resolved could -// still be swapped out between this check and that open. Closing that -// residual race fully would need platform-specific work (e.g. Linux's -// openat2 with RESOLVE_NO_SYMLINKS), which isn't done anywhere else in this -// file either -- this function only guarantees "reads what was just -// verified", not "reads atomically with no concurrent tampering". -func classifyWalkEntry(root, p string, d fs.DirEntry) (walkEntryAction, string) { - if d.IsDir() { - return walkEntrySkip, "" - } - resolved, err := filepath.EvalSymlinks(p) - if err != nil { - return walkEntryUnreadable, "" - } - fi, statErr := os.Stat(resolved) - if statErr != nil { - return walkEntryUnreadable, "" - } - if fi.IsDir() { - // p is a symlink to a directory: WalkDir doesn't recurse into - // symlinked directories, and grepFile would fail trying to read - // one as a file, so skip it rather than treating it as an error. - return walkEntrySkip, "" - } - if !fi.Mode().IsRegular() { - // Skip non-regular files (FIFOs, sockets, devices): opening one - // for reading can block indefinitely (e.g. a FIFO with no writer - // connected), and grep has no business reading them. - return walkEntrySkip, "" - } - if !WithinRoot(resolved, root) { - // The symlink-resolved target escapes the root being searched, so - // a symlink can't be used to read files outside the requested - // directory. - return walkEntrySkip, "" - } - return walkEntryGrep, resolved -} - -// GrepContent searches path for lines matching a regular expression pattern. -// If path is a directory, recursive must be true to search its files. -// -// A recursive search walks the whole tree under path, which is unbounded from -// this function's point of view, so ctx is honored between entries and lets a -// caller abort one. Note Go needs no equivalent of the Python runtime's regex -// timeout: this uses RE2, which is linear-time and cannot backtrack -// catastrophically, so it is the walk rather than the match that can run long. -func GrepContent(ctx context.Context, path, pattern string, recursive, ignoreCase bool) (string, error) { - expr := pattern - if ignoreCase { - expr = "(?i)" + expr - } - re, err := regexp.Compile(expr) - if err != nil { - return "", fmt.Errorf("invalid pattern: %w", err) - } - - info, err := os.Stat(path) - if err != nil { - return "", fmt.Errorf("failed to stat %q: %w", path, err) - } - - var result strings.Builder - var skipped int - grepFile := func(filePath string) error { - return scanFileLines(filePath, func(lineNum int, line string) bool { - if re.MatchString(line) { - fmt.Fprintf(&result, "%s:%d:%s\n", filePath, lineNum, truncateRunes(line, maxLineRunes)) - } - return true - }) - } - - if info.IsDir() { - if !recursive { - return "", fmt.Errorf("%q is a directory; set recursive=true to search directories", path) - } - // Reuse the outer err (rather than := , which would shadow it in this - // block) so a WalkDir failure below is actually observed by the - // err != nil check after this if/else. - var root string - root, err = filepath.EvalSymlinks(path) - if err != nil { - return "", fmt.Errorf("failed to resolve %q: %w", path, err) - } - // Walk the resolved root, not path: filepath.WalkDir uses Lstat on - // its root argument, so if path itself were an unresolved directory - // symlink, WalkDir would see a non-directory at the root and never - // descend into it at all. - err = filepath.WalkDir(root, func(p string, d fs.DirEntry, walkErr error) error { - // Checked per entry rather than per line: a single file's scan is - // bounded by its size, but the number of entries is not. - if ctxErr := ctx.Err(); ctxErr != nil { - return ctxErr - } - if walkErr != nil { - if p == root { - // The search root itself couldn't be read (e.g. - // permission denied): the search never actually ran, so - // surface a real error instead of a misleadingly - // confident "no matches found". - return walkErr - } - // WalkDir surfaces ReadDir/Lstat failures (e.g. a - // permission-denied subdirectory) through this err rather - // than via grepFile, but it deserves the same treatment: one - // unreadable subtree shouldn't discard matches already found - // in its siblings. - skipped++ - return nil - } - switch action, resolved := classifyWalkEntry(root, p, d); action { - case walkEntryUnreadable: - skipped++ - case walkEntryGrep: - // A read error on one file (permission denied, a line - // exceeding the scan buffer, etc.) shouldn't abort matches - // already found elsewhere in the tree. - if grepErr := grepFile(resolved); grepErr != nil { - skipped++ - } - } - return nil - }) - } else { - // scanFileLines rejects non-regular files too, but reusing the stat - // already taken above lets an explicitly-targeted FIFO report the - // plain reason rather than one wrapped in "failed to search". - if !info.Mode().IsRegular() { - return "", fmt.Errorf("%q is not a regular file", path) - } - err = grepFile(path) - } - if err != nil { - return "", fmt.Errorf("failed to search %q: %w", path, err) - } - - if result.Len() == 0 { - if skipped > 0 { - return fmt.Sprintf("no matches found (%d entries could not be read)", skipped), nil - } - return "no matches found", nil - } - - matches := strings.TrimSuffix(result.String(), "\n") - if skipped > 0 { - matches += fmt.Sprintf("\n\n(%d entries could not be read)", skipped) - } - return matches, nil -} - func NewCommandExecutor() *CommandExecutor { return &CommandExecutor{} } diff --git a/go/adk/pkg/tools/shell_test.go b/go/adk/pkg/tools/shell_test.go index 1522364e6..2c42ae482 100644 --- a/go/adk/pkg/tools/shell_test.go +++ b/go/adk/pkg/tools/shell_test.go @@ -2,7 +2,6 @@ package tools import ( "context" - "errors" "fmt" "os" "path/filepath" @@ -548,453 +547,6 @@ func TestListDirContent(t *testing.T) { }) } -func TestGrepContent(t *testing.T) { - tmpDir := createTempDir(t) - defer os.RemoveAll(tmpDir) - - if err := os.WriteFile(filepath.Join(tmpDir, "a.txt"), []byte("hello world\nFOO bar\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - subDir := filepath.Join(tmpDir, "sub") - if err := os.Mkdir(subDir, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - if err := os.WriteFile(filepath.Join(subDir, "b.txt"), []byte("another foo line\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - t.Run("matches within a single file", func(t *testing.T) { - result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "hello", false, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "a.txt:1:hello world") { - t.Errorf("expected match with path:line:content, got %q", result) - } - }) - - t.Run("no matches", func(t *testing.T) { - result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "nope", false, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if result != "no matches found" { - t.Errorf("expected no matches message, got %q", result) - } - }) - - t.Run("ignore case", func(t *testing.T) { - result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "foo", false, true) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "FOO bar") { - t.Errorf("expected case-insensitive match, got %q", result) - } - }) - - t.Run("directory requires recursive", func(t *testing.T) { - if _, err := GrepContent(context.Background(), tmpDir, "foo", false, false); err == nil { - t.Fatal("expected error when searching a directory without recursive=true") - } - }) - - t.Run("recursive searches subdirectories", func(t *testing.T) { - result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "b.txt:1:another foo line") { - t.Errorf("expected match from subdirectory, got %q", result) - } - }) - - t.Run("invalid pattern", func(t *testing.T) { - if _, err := GrepContent(context.Background(), filepath.Join(tmpDir, "a.txt"), "(", false, false); err == nil { - t.Fatal("expected error for invalid regex pattern") - } - }) - - t.Run("recursive search skips symlinks that escape the root", func(t *testing.T) { - outsideDir := createTempDir(t) - defer os.RemoveAll(outsideDir) - secretPath := filepath.Join(outsideDir, "secret.txt") - if err := os.WriteFile(secretPath, []byte("top secret foo\n"), 0644); err != nil { - t.Fatalf("Failed to write outside file: %v", err) - } - - linkPath := filepath.Join(subDir, "escape.txt") - if err := os.Symlink(secretPath, linkPath); err != nil { - t.Skipf("symlinks not supported: %v", err) - } - defer os.Remove(linkPath) - - result, err := GrepContent(context.Background(), tmpDir, "foo", true, true) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if strings.Contains(result, "top secret") { - t.Errorf("expected symlinked file outside root to be skipped, got %q", result) - } - }) - - t.Run("recursive search greps the resolved target of an in-bounds file symlink", func(t *testing.T) { - realFile := filepath.Join(subDir, "real_target.txt") - if err := os.WriteFile(realFile, []byte("foo via symlink\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - linkPath := filepath.Join(subDir, "file_link.txt") - if err := os.Symlink(realFile, linkPath); err != nil { - t.Skipf("symlinks not supported: %v", err) - } - defer os.Remove(linkPath) - - result, err := GrepContent(context.Background(), subDir, "foo via symlink", true, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - // The match must be reported against the resolved target path - // (real_target.txt), not the walked symlink path (file_link.txt): - // classifyWalkEntry verifies the resolved target is in-bounds, and - // the actual read must use that same resolved value rather than - // re-deriving/reopening the raw symlink path, or the verified-safe - // check and the actual read could diverge (TOCTOU). - if !strings.Contains(result, "real_target.txt:1:foo via symlink") { - t.Errorf("expected match to be reported against the resolved target path, got %q", result) - } - if strings.Contains(result, "file_link.txt:") { - t.Errorf("expected match not to be reported against the unresolved symlink path, got %q", result) - } - }) - - t.Run("recursive search does not abort on an in-bounds directory symlink", func(t *testing.T) { - walkDir := createTempDir(t) - defer os.RemoveAll(walkDir) - - if err := os.WriteFile(filepath.Join(walkDir, "aaa_first.txt"), []byte("foo one\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - realSub := filepath.Join(walkDir, "real_sub") - if err := os.Mkdir(realSub, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - - // A symlink to an in-bounds directory, lexically sorted between the - // two files below, so an incorrect abort partway through the walk - // would silently drop "zzz_sub"'s match. - linkPath := filepath.Join(walkDir, "mmm_link") - if err := os.Symlink(realSub, linkPath); err != nil { - t.Skipf("symlinks not supported: %v", err) - } - defer os.Remove(linkPath) - - zzzSub := filepath.Join(walkDir, "zzz_sub") - if err := os.Mkdir(zzzSub, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - if err := os.WriteFile(filepath.Join(zzzSub, "zzz_last.txt"), []byte("foo two\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - result, err := GrepContent(context.Background(), walkDir, "foo", true, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "aaa_first.txt:1:foo one") { - t.Errorf("expected match before the symlink, got %q", result) - } - if !strings.Contains(result, "zzz_last.txt:1:foo two") { - t.Errorf("expected match after the symlink (walk must not abort on it), got %q", result) - } - }) - - t.Run("recursive search resolves the root itself when it is an unresolved symlink", func(t *testing.T) { - realDir := createTempDir(t) - defer os.RemoveAll(realDir) - if err := os.WriteFile(filepath.Join(realDir, "match.txt"), []byte("foo inside\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - parentDir := createTempDir(t) - defer os.RemoveAll(parentDir) - linkRoot := filepath.Join(parentDir, "link-root") - if err := os.Symlink(realDir, linkRoot); err != nil { - t.Skipf("symlinks not supported: %v", err) - } - - // Pass the unresolved symlink directly, as a caller that doesn't - // pre-resolve its path would. - result, err := GrepContent(context.Background(), linkRoot, "foo", true, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "match.txt:1:foo inside") { - t.Errorf("expected GrepContent to resolve a symlinked root and recurse into it, got %q", result) - } - }) - - t.Run("recursive search does not hang on a FIFO and finds matches around it", func(t *testing.T) { - fifoDir := createTempDir(t) - defer os.RemoveAll(fifoDir) - - if err := os.WriteFile(filepath.Join(fifoDir, "aaa_before.txt"), []byte("foo before\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - fifoPath := filepath.Join(fifoDir, "mmm_pipe") - if err := syscall.Mkfifo(fifoPath, 0644); err != nil { - t.Skipf("FIFOs not supported: %v", err) - } - if err := os.WriteFile(filepath.Join(fifoDir, "zzz_after.txt"), []byte("foo after\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - done := make(chan struct{}) - var result string - var err error - go func() { - result, err = GrepContent(context.Background(), fifoDir, "foo", true, false) - close(done) - }() - - select { - case <-done: - case <-time.After(5 * time.Second): - t.Fatal("GrepContent hung on a FIFO instead of skipping it") - } - - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "aaa_before.txt:1:foo before") { - t.Errorf("expected match before the FIFO, got %q", result) - } - if !strings.Contains(result, "zzz_after.txt:1:foo after") { - t.Errorf("expected match after the FIFO (walk must not hang or abort on it), got %q", result) - } - }) - - t.Run("a single target FIFO returns an error instead of hanging", func(t *testing.T) { - fifoDir := createTempDir(t) - defer os.RemoveAll(fifoDir) - fifoPath := filepath.Join(fifoDir, "pipe") - if err := syscall.Mkfifo(fifoPath, 0644); err != nil { - t.Skipf("FIFOs not supported: %v", err) - } - - done := make(chan struct{}) - var err error - go func() { - _, err = GrepContent(context.Background(), fifoPath, "foo", false, false) - close(done) - }() - - select { - case <-done: - case <-time.After(5 * time.Second): - t.Fatal("GrepContent hung opening a FIFO directly instead of erroring") - } - - if err == nil { - t.Fatal("expected an error for a non-regular file target, got nil") - } - }) - - t.Run("recursive search does not abort or discard matches on an unreadable subdirectory", func(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("root bypasses directory permissions; cannot exercise this case") - } - - walkDir := createTempDir(t) - defer os.RemoveAll(walkDir) - - okSub := filepath.Join(walkDir, "aaa_ok") - if err := os.Mkdir(okSub, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - if err := os.WriteFile(filepath.Join(okSub, "match.txt"), []byte("foo readable\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - noPermSub := filepath.Join(walkDir, "mmm_noperm") - if err := os.Mkdir(noPermSub, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - if err := os.Chmod(noPermSub, 0000); err != nil { - t.Fatalf("Failed to chmod subdir: %v", err) - } - defer os.Chmod(noPermSub, 0755) - - result, err := GrepContent(context.Background(), walkDir, "foo", true, false) - if err != nil { - t.Fatalf("GrepContent() error = %v, expected the unreadable subdirectory to be skipped rather than aborting the whole search", err) - } - if !strings.Contains(result, "match.txt:1:foo readable") { - t.Errorf("expected match from the readable sibling directory to survive an unreadable subdirectory elsewhere in the tree, got %q", result) - } - }) - - t.Run("no matches found is annotated when entries could not be read", func(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("root bypasses directory permissions; cannot exercise this case") - } - - walkDir := createTempDir(t) - defer os.RemoveAll(walkDir) - - noPermSub := filepath.Join(walkDir, "noperm") - if err := os.Mkdir(noPermSub, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - if err := os.Chmod(noPermSub, 0000); err != nil { - t.Fatalf("Failed to chmod subdir: %v", err) - } - defer os.Chmod(noPermSub, 0755) - - result, err := GrepContent(context.Background(), walkDir, "foo", true, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "no matches found") || !strings.Contains(result, "could not be read") { - t.Errorf("expected an annotated no-matches message noting unreadable entries, got %q", result) - } - }) - - t.Run("matches are annotated with the skip count when some entries could not be read", func(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("root bypasses directory permissions; cannot exercise this case") - } - - walkDir := createTempDir(t) - defer os.RemoveAll(walkDir) - - okSub := filepath.Join(walkDir, "aaa_ok") - if err := os.Mkdir(okSub, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - if err := os.WriteFile(filepath.Join(okSub, "match.txt"), []byte("foo readable\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - noPermSub := filepath.Join(walkDir, "mmm_noperm") - if err := os.Mkdir(noPermSub, 0755); err != nil { - t.Fatalf("Failed to create subdir: %v", err) - } - if err := os.WriteFile(filepath.Join(noPermSub, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - if err := os.Chmod(noPermSub, 0000); err != nil { - t.Fatalf("Failed to chmod subdir: %v", err) - } - defer os.Chmod(noPermSub, 0755) - - result, err := GrepContent(context.Background(), walkDir, "foo", true, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "match.txt:1:foo readable") { - t.Errorf("expected the real match to still be reported, got %q", result) - } - if !strings.Contains(result, "could not be read") { - t.Errorf("expected the skip count to be reported alongside real matches, not silently dropped, got %q", result) - } - }) - - t.Run("recursive search on a fully unreadable root returns an error, not a confident empty result", func(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("root bypasses directory permissions; cannot exercise this case") - } - - walkDir := createTempDir(t) - defer os.RemoveAll(walkDir) - if err := os.WriteFile(filepath.Join(walkDir, "hidden.txt"), []byte("foo hidden\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - if err := os.Chmod(walkDir, 0000); err != nil { - t.Fatalf("Failed to chmod root: %v", err) - } - defer os.Chmod(walkDir, 0755) - - result, err := GrepContent(context.Background(), walkDir, "foo", true, false) - if err == nil { - t.Fatalf("expected an error when the search root itself is unreadable, got a result instead: %q", result) - } - }) - - t.Run("matched lines are truncated to 2000 characters", func(t *testing.T) { - tmpDir := createTempDir(t) - defer os.RemoveAll(tmpDir) - - longLine := "foo " + strings.Repeat("x", 3000) - if err := os.WriteFile(filepath.Join(tmpDir, "long.txt"), []byte(longLine+"\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - result, err := GrepContent(context.Background(), filepath.Join(tmpDir, "long.txt"), "foo", false, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.HasSuffix(result, "...") { - t.Errorf("expected truncated line to end with '...', got %q", result) - } - if len(result) > 2100 { - t.Errorf("expected result to be truncated to roughly 2000 chars, got length %d", len(result)) - } - }) - - t.Run("recursive search honors a cancelled context", func(t *testing.T) { - walkDir := createTempDir(t) - defer os.RemoveAll(walkDir) - - // Enough entries that the walk is guaranteed to consult ctx at least - // once after cancellation rather than finishing outright. - for i := range 50 { - name := filepath.Join(walkDir, fmt.Sprintf("file_%02d.txt", i)) - if err := os.WriteFile(name, []byte("foo\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - } - - ctx, cancel := context.WithCancel(context.Background()) - cancel() // already cancelled before the walk starts - - _, err := GrepContent(ctx, walkDir, "foo", true, false) - if err == nil { - t.Fatal("expected a cancelled context to abort the search, got no error") - } - if !errors.Is(err, context.Canceled) { - t.Errorf("expected the error to wrap context.Canceled, got %v", err) - } - }) - - t.Run("a non-recursive search is unaffected by context", func(t *testing.T) { - // Single-file reads are bounded by file size, so they intentionally - // don't consult ctx -- pinning that so the cancellation check isn't - // later "helpfully" moved somewhere it would break normal reads. - tmpDir := createTempDir(t) - defer os.RemoveAll(tmpDir) - if err := os.WriteFile(filepath.Join(tmpDir, "a.txt"), []byte("foo\n"), 0644); err != nil { - t.Fatalf("Failed to write test file: %v", err) - } - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - result, err := GrepContent(ctx, filepath.Join(tmpDir, "a.txt"), "foo", false, false) - if err != nil { - t.Fatalf("GrepContent() error = %v", err) - } - if !strings.Contains(result, "a.txt:1:foo") { - t.Errorf("expected the single-file match, got %q", result) - } - }) -} - func TestExecuteCommand(t *testing.T) { tmpDir := createTempDir(t) defer os.RemoveAll(tmpDir) From abb3889d4f73eb74a188a5a0be4cb893afd636e1 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 26 Aug 2026 14:54:05 -0400 Subject: [PATCH 11/15] test: cover long line and symlink fixes through ADK tool path Signed-off-by: brandonkeung --- go/adk/pkg/tools/skills_test.go | 72 +++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 3 deletions(-) diff --git a/go/adk/pkg/tools/skills_test.go b/go/adk/pkg/tools/skills_test.go index 8bf16e815..a0b20442b 100644 --- a/go/adk/pkg/tools/skills_test.go +++ b/go/adk/pkg/tools/skills_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "unicode/utf8" "github.com/kagent-dev/kagent/go/core/pkg/env" adkagent "google.golang.org/adk/v2/agent" @@ -287,17 +288,21 @@ func TestListFilesAndGrepFileTools_RunThroughADK(t *testing.T) { t.Fatalf("NewSkillExecutionTools() error = %v", err) } - var listFilesTool, grepFileTool tool.Tool + var listFilesTool, grepFileTool, readFileTool, bashTool tool.Tool for _, tl := range tools { switch tl.Name() { case "list_files": listFilesTool = tl case "grep_file": grepFileTool = tl + case "read_file": + readFileTool = tl + case "bash": + bashTool = tl } } - if listFilesTool == nil || grepFileTool == nil { - t.Fatal("expected list_files and grep_file tools to be present") + if listFilesTool == nil || grepFileTool == nil || readFileTool == nil || bashTool == nil { + t.Fatal("expected list_files, grep_file, read_file and bash tools to be present") } sessionID := fmt.Sprintf("%s-session", t.Name()) @@ -347,4 +352,65 @@ func TestListFilesAndGrepFileTools_RunThroughADK(t *testing.T) { t.Errorf("list_files result = %q, want an outside-allowed-roots error", result) } }) + + // Exercised here, through the registered tool rather than against + // ReadFileContent directly, because the failure this guards was invisible + // at the function level: read_file and grep_file scanned with different + // buffer limits, so a file grep_file handled fine made read_file fail + // outright. Driving the same tree through both tools is what makes that + // asymmetry observable. + t.Run("read_file and grep_file agree on a line past bufio's default", func(t *testing.T) { + longLine := strings.Repeat("a", 100_000) + bundle := filepath.Join(sessionPath, "bundle.min.js") + if err := os.WriteFile(bundle, []byte(longLine+"\nneedle here\n"), 0644); err != nil { + t.Fatalf("failed to seed long-line file: %v", err) + } + + read := runTool(t, readFileTool, ctx, map[string]any{"file_path": "bundle.min.js"}) + if strings.Contains(read, "Error reading file") { + t.Fatalf("read_file failed on a long line instead of truncating: %q", read) + } + readLines := strings.Split(read, "\n") + if len(readLines) != 2 { + t.Fatalf("read_file returned %d lines, want 2: %.200q", len(readLines), read) + } + body := strings.TrimSuffix(strings.SplitN(readLines[0], "|", 2)[1], "...") + if n := utf8.RuneCountInString(body); n != maxLineRunes { + t.Errorf("read_file truncated line 1 to %d runes, want %d", n, maxLineRunes) + } + if !strings.Contains(readLines[1], "needle here") { + t.Errorf("read_file lost the line after the long one: %q", readLines[1]) + } + + // The same file must remain greppable -- this is the side that always + // worked, and it is what read_file is now consistent with. + grep := runTool(t, grepFileTool, ctx, map[string]any{ + "pattern": "needle here", + "path": "bundle.min.js", + }) + if !strings.Contains(grep, "bundle.min.js:2:needle here") { + t.Errorf("grep_file result = %q, want the match on line 2", grep) + } + }) + + t.Run("list_files reports a symlink's target size, not the link's", func(t *testing.T) { + linkDir := filepath.Join(sessionPath, "linkdir") + if err := os.Mkdir(linkDir, 0755); err != nil { + t.Fatalf("failed to create linkdir: %v", err) + } + target := filepath.Join(linkDir, "target.txt") + if err := os.WriteFile(target, []byte(strings.Repeat("x", 5000)), 0644); err != nil { + t.Fatalf("failed to write symlink target: %v", err) + } + // A relative target, so the stored path string is far shorter than the + // file -- an Lstat-based size would report 10, not 5000. + if err := os.Symlink("target.txt", filepath.Join(linkDir, "file-link")); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + + result := runTool(t, listFilesTool, ctx, map[string]any{"path": "linkdir"}) + if !strings.Contains(result, "file-link\t5000") { + t.Errorf("list_files result = %q, want file-link sized by its target (5000)", result) + } + }) } From 9652ae127609d015a9723dfc3db7495bd97acff6 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Wed, 26 Aug 2026 16:42:48 -0400 Subject: [PATCH 12/15] style: satisfy ruff format on the file-search tool additions CI runs `ruff format --diff .` and fails on any diff. Neither line this branch added had been through the formatter -- only `ruff check`, which passes on both. The prompts.py change joins an implicitly-concatenated string literal; the resulting bash description is byte-identical, verified against the exact expected suffix. Signed-off-by: brandonkeung --- .../packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py | 4 +++- python/packages/kagent-skills/src/kagent/skills/prompts.py | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) 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 eab692ac4..45ba4f83b 100644 --- a/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py +++ b/python/packages/kagent-adk/src/kagent/adk/tools/skills_plugin.py @@ -64,4 +64,6 @@ def add_skills_tool_to_agent( 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)") + 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-skills/src/kagent/skills/prompts.py b/python/packages/kagent-skills/src/kagent/skills/prompts.py index 1f47e633d..192feeca0 100644 --- a/python/packages/kagent-skills/src/kagent/skills/prompts.py +++ b/python/packages/kagent-skills/src/kagent/skills/prompts.py @@ -147,7 +147,6 @@ def get_bash_description() -> str: # 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" + "\nAlso available: list_files and grep_file, for exploring the filesystem without a full shell command.\n" ) return description From 97a23d8e72c626fb32775602931361e43ee5c74b Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Thu, 27 Aug 2026 12:00:17 -0400 Subject: [PATCH 13/15] perf: stop pre-allocating 64KB of scan buffer per file scanFileLines asked bufio for a fixed 64KB initial buffer. That size was never deliberate: it arrived in 27fb17cb while addressing a review ask to *cap* grep's line length, where the 1MB maximum was the point and the initial size was incidental. Consolidating the two readers then spread it from grep_file to read_file as well, so every file read paid it too. A nil initial buffer keeps the same 1MB cap -- bufio grows from 4KB by doubling -- so long lines are unaffected, verified with a 900,000-char line reading back intact. Isolating just the buffer argument: fixed 64KB 18820 ns/op 65746 B/op nil (grows) 10978 ns/op 4297 B/op Roughly 15x less allocated per file. It matters because grep_file walks whole trees: a 10,000-file search was allocating ~650MB of transient buffer to read files that are mostly a few KB. Signed-off-by: brandonkeung --- go/adk/pkg/tools/shell.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/go/adk/pkg/tools/shell.go b/go/adk/pkg/tools/shell.go index a730775eb..4456ee7b4 100644 --- a/go/adk/pkg/tools/shell.go +++ b/go/adk/pkg/tools/shell.go @@ -54,7 +54,11 @@ func scanFileLines(path string, visit func(lineNum int, line string) bool) error defer file.Close() scanner := bufio.NewScanner(file) - scanner.Buffer(make([]byte, 64*1024), maxLineBytes) + // nil initial buffer, not a fixed 64KB one: bufio grows from 4KB by + // doubling, so a long line still reaches maxLineBytes, but the common + // case -- a recursive grep over a tree of small files -- stops paying + // 64KB of allocation per file. Measured at ~15x less allocated per file. + scanner.Buffer(nil, maxLineBytes) for lineNum := 1; scanner.Scan(); lineNum++ { if !visit(lineNum, scanner.Text()) { return nil From 0e303ee3161d3e36ebb351ba4cefdadb89a83e8b Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Thu, 27 Aug 2026 12:06:20 -0400 Subject: [PATCH 14/15] refactor: give the Python walk classifier the shape Go already has Go decides how to treat each entry of a recursive grep in a named function with three named outcomes -- classifyWalkEntry returning walkEntryGrep, walkEntrySkip or walkEntryUnreadable. Python made the same decision inline, held together by comments that referenced the Go symbols by name: # matching the split Go makes in classifyWalkEntry: # ... as Go's walkEntryUnreadable does. # ... as Go's walkEntrySkip does. That is a cross-runtime contract enforced by English. Renaming the Go function would silently falsify it -- no test fails, nothing rebuilds. It matters here specifically because runtime drift is this feature's recurring defect: six of the ten findings in the last review were the two implementations having quietly diverged. Extract _classify_walk_entry with the same three outcomes, the same argument order and the same return shape, so the two can be diffed by reading them side by side rather than by trusting prose. The entry loop drops from 38 lines (25 of them comment, 65%) to 14 (3 comment, 21%), and the reasoning moves onto the function it actually describes. The docstring is explicit that the two are not branch-for-branch identical and should not be forced to be -- Go tests IsDir, EvalSymlinks, Stat and IsRegular separately because WalkDir hands it directories and unresolvable links, while os.walk yields only filenames and Path.is_file() collapses those cases into one call. It also records the one divergence we know of and could not construct: Go counts a failed Stat on a non-symlink as UNREADABLE, where Path.is_file() swallows the OSError and reaches SKIP. Behavior is unchanged. Each outcome is independently pinned, verified by inverting it and confirming the suite catches it: UNREADABLE -> SKIP (broken links uncounted) caught SKIP -> GREP (escaping symlinks searched) caught GREP -> SKIP (regular files never read) caught SKIP -> UNREADABLE (FIFOs counted) caught Adds a direct table test for the classifier, which the inline form could not have: the three-way split is now assertable without going through grep_content, including the symlink-loop case. Co-Authored-By: Claude Opus 5 Signed-off-by: brandonkeung --- .../kagent-skills/src/kagent/skills/shell.py | 97 +++++++++++++------ .../tests/unittests/test_skill_execution.py | 53 +++++++++- 2 files changed, 120 insertions(+), 30 deletions(-) diff --git a/python/packages/kagent-skills/src/kagent/skills/shell.py b/python/packages/kagent-skills/src/kagent/skills/shell.py index 8a3698acb..330d6c9dc 100644 --- a/python/packages/kagent-skills/src/kagent/skills/shell.py +++ b/python/packages/kagent-skills/src/kagent/skills/shell.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import enum import logging import os import re @@ -27,6 +28,68 @@ def _truncate_line(line: str) -> str: return line +class _WalkEntryAction(enum.Enum): + """How a recursive grep should treat one entry it walked.""" + + #: A regular, in-bounds file that should be grepped. + GREP = enum.auto() + #: Silently excluded by policy, not a read failure -- a non-regular file + #: (FIFO/socket/device), or a symlink whose target escapes the search root. + SKIP = enum.auto() + #: A genuine read/stat failure on this entry. + UNREADABLE = enum.auto() + + +def _classify_walk_entry(root: Path, entry: Path) -> tuple[_WalkEntryAction, Path | None]: + """Decide how grep_content should treat entry, given the resolved search root. + + This is the Python half of a contract the Go runtime implements as + classifyWalkEntry in go/adk/pkg/tools/grep.go. Both must sort a tree into + the same three outcomes, and reading them side by side is the only + practical way to confirm they still do -- so they share these names, this + argument order, and this return shape. Prefer changing both, or neither. + + They are not branch-for-branch identical, and should not be forced to be. + Go tests IsDir, EvalSymlinks, Stat and IsRegular separately because + filepath.WalkDir hands it directories and unresolvable links; here + os.walk yields only filenames, and Path.is_file() already collapses + "exists, resolves, and is a regular file" into one call. The outcomes + agree; the number of branches reaching them does not. + + One known divergence, not reachable through any case we could construct: + Go counts a failed Stat on a non-symlink as UNREADABLE, while + Path.is_file() swallows the OSError and lands here on SKIP. Reaching it + needs a stat that fails on an entry os.walk just listed -- EIO, a stale + NFS handle, a misbehaving FUSE mount. Symlink loops and broken links are + unaffected; both are UNREADABLE in either runtime. + + Returns the resolved path alongside GREP so the caller reads what was + just checked rather than re-resolving the symlink separately. + """ + # is_file() follows symlinks and checks S_ISREG, so a False covers two + # cases that deserve different treatment. + if not entry.is_file(): + # A broken symlink (or a symlink loop) is a genuine read failure. + # Counting it is what stops a tree of dangling links from reporting a + # confidently empty "no matches found". + if entry.is_symlink() and not entry.exists(): + return _WalkEntryAction.UNREADABLE, None + # A FIFO/socket/device is excluded by policy, not failure: opening one + # can block indefinitely, and grep has no business reading it. + return _WalkEntryAction.SKIP, None + + resolved = entry.resolve() + # Bound each entry by the directory actually being searched. The caller's + # allowed_root is the whole session plus the skills dir, so it alone would + # let a symlink here pull in a file from a sibling directory nobody asked + # to search. Containment against root subsumes it: root was itself + # validated against allowed_root, so anything under root is inside a root. + if not resolved.is_relative_to(root): + return _WalkEntryAction.SKIP, None + + return _WalkEntryAction.GREP, resolved + + def _validate_path( file_path: Path, allowed_roots: Path | list[Path] | None, @@ -236,43 +299,19 @@ def grep_file(file_path: Path) -> list[str]: skipped += len(walk_errors) for entry in sorted(entries): - # is_file() follows symlinks and checks S_ISREG, so a False here - # covers two cases that deserve different treatment -- matching - # the split Go makes in classifyWalkEntry: - # - a broken symlink (or a symlink loop) is a genuine read - # failure, so count it, as Go's walkEntryUnreadable does. - # Otherwise a tree of dangling links reports a confidently - # empty "no matches found". - # - a FIFO/socket/device is excluded by policy, not failure - # (opening one can block indefinitely), so stay silent, as - # Go's walkEntrySkip does. - if not entry.is_file(): - if entry.is_symlink() and not entry.exists(): - skipped += 1 + action, safe_entry = _classify_walk_entry(file_or_dir_path, entry) + if action is _WalkEntryAction.SKIP: continue - # Bound each entry by the directory actually being searched, not - # by allowed_root: allowed_root is the whole session plus the - # skills dir, so on its own it would let a symlink here pull in a - # file from a sibling directory the caller never asked to search. - # Go bounds each entry the same way (WithinRoot in - # classifyWalkEntry), and both the tool description and the README - # promise that behavior. Silent, matching Go's walkEntrySkip for an - # out-of-root symlink. - # - # This subsumes a _validate_path(entry, allowed_root) check: - # file_or_dir_path was itself validated against allowed_root above, - # so anything relative to it is transitively inside a root. - safe_entry = entry.resolve() - if not safe_entry.is_relative_to(file_or_dir_path): + if action is _WalkEntryAction.UNREADABLE: + skipped += 1 continue try: results.extend(grep_file(safe_entry)) except OSError: # A read error on one file shouldn't abort matches already # found elsewhere in the tree, but it also shouldn't look - # identical to a genuinely empty search -- see the note below. + # identical to a genuinely empty search -- hence the count. skipped += 1 - continue else: if not file_or_dir_path.is_file(): raise OSError(f"{file_or_dir_path} is not a regular file") 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 f26a50e15..5206f5a72 100644 --- a/python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py +++ b/python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py @@ -21,7 +21,7 @@ write_file_content, ) from kagent.skills.prompts import get_bash_description -from kagent.skills.shell import _sanitize_env +from kagent.skills.shell import _classify_walk_entry, _sanitize_env, _WalkEntryAction @pytest.fixture @@ -463,6 +463,57 @@ def test_grep_content_counts_broken_symlink_as_unreadable(tmp_path): assert "could not be read" in result +def test_classify_walk_entry_covers_each_outcome(tmp_path): + """Pin the three-way split directly, not just through grep_content. + + This is the Python half of the contract go/adk/pkg/tools/grep.go states as + classifyWalkEntry. Asserting it at the unit level is what makes the two + runtimes comparable side by side: same three outcomes, same inputs, same + answers. A divergence should fail here rather than surface later as a + grep that quietly returns different results in one runtime. + """ + root = tmp_path / "root" + root.mkdir() + + plain = root / "plain.txt" + plain.write_text("hello\n") + + inside_link = root / "inside-link" + inside_link.symlink_to(plain) + + outside = tmp_path / "outside.txt" + outside.write_text("secret\n") + escaping_link = root / "escaping-link" + escaping_link.symlink_to(outside) + + broken_link = root / "broken-link" + broken_link.symlink_to(tmp_path / "does-not-exist") + + assert _classify_walk_entry(root, plain) == (_WalkEntryAction.GREP, plain.resolve()) + # A symlink that stays inside the root is greppable, and resolves through + # to its target so the caller reads what was just checked. + assert _classify_walk_entry(root, inside_link) == (_WalkEntryAction.GREP, plain.resolve()) + # Escaping the searched directory is a policy exclusion, so it is silent. + assert _classify_walk_entry(root, escaping_link) == (_WalkEntryAction.SKIP, None) + # A dangling link is a real read failure and must be counted. + assert _classify_walk_entry(root, broken_link) == (_WalkEntryAction.UNREADABLE, None) + # A symlink loop is the same class of failure as a dangling link. + loop = root / "loop-link" + loop.symlink_to(loop) + assert _classify_walk_entry(root, loop) == (_WalkEntryAction.UNREADABLE, None) + + +def test_classify_walk_entry_treats_a_fifo_as_a_silent_skip(tmp_path): + """A FIFO is excluded by policy, not failure, so it must not be counted.""" + fifo_path = tmp_path / "pipe" + try: + os.mkfifo(fifo_path) + except (AttributeError, NotImplementedError, OSError): + pytest.skip("FIFOs not supported") + + assert _classify_walk_entry(tmp_path, fifo_path) == (_WalkEntryAction.SKIP, None) + + def test_grep_content_does_not_count_fifo_as_unreadable(tmp_path): """A FIFO is excluded by policy, not failure -- it must stay silent. From c565a30b441fb95fdb6e1d382bd3a3e5834861e8 Mon Sep 17 00:00:00 2001 From: brandonkeung Date: Thu, 27 Aug 2026 17:16:32 -0400 Subject: [PATCH 15/15] test: pin Go's walk classifier directly, mirroring the Python table classifyWalkEntry had no direct test -- grep for it in *_test.go and the only hit was inside a comment. Its three-way split was covered indirectly through TestGrepContent, so a change to the classifier surfaced as an integration assertion failing somewhere downstream rather than as the specific case that broke. That asymmetry undercut the previous commit. Python got both the extracted classifier and a table test asserting all its outcomes; Go had the structure but nothing equivalent to compare against, which is precisely the side-by-side reading the two are supposed to support. Same cases, same order, same expected outcomes as test_classify_walk_entry_covers_each_outcome in kagent-skills. The directory case has no Python counterpart on purpose, and says so: filepath.WalkDir hands this function directories while os.walk yields only filenames, so only Go can reach it. Verified it guards the boundary rather than just passing: disabling the WithinRoot check makes symlink_escaping_the_root fail with the resolved out-of-root path it would have leaked. Signed-off-by: brandonkeung --- go/adk/pkg/tools/grep_test.go | 111 ++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/go/adk/pkg/tools/grep_test.go b/go/adk/pkg/tools/grep_test.go index b13e22c4d..e42c7dcf0 100644 --- a/go/adk/pkg/tools/grep_test.go +++ b/go/adk/pkg/tools/grep_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -12,6 +13,116 @@ import ( "time" ) +// TestClassifyWalkEntry pins the three-way split directly rather than only +// through TestGrepContent's integration assertions, so a change to the +// classifier fails here with the specific case that broke. +// +// It is the deliberate mirror of Python's +// test_classify_walk_entry_covers_each_outcome (kagent-skills, shell.py): +// same cases, same order, same expected outcomes. The two runtimes must sort +// a tree identically, and reading these two tests side by side is how that +// stays true. Prefer changing both, or neither. +// +// The directory case has no Python counterpart on purpose -- filepath.WalkDir +// hands this function directories, while Python's os.walk yields only +// filenames, so only Go can reach it. +func TestClassifyWalkEntry(t *testing.T) { + root := createTempDir(t) + defer os.RemoveAll(root) + + plain := filepath.Join(root, "plain.txt") + if err := os.WriteFile(plain, []byte("hello\n"), 0644); err != nil { + t.Fatalf("failed to write test file: %v", err) + } + if err := os.Mkdir(filepath.Join(root, "subdir"), 0755); err != nil { + t.Fatalf("failed to create subdir: %v", err) + } + // A sibling temp dir, not filepath.Dir(root): that would be the shared + // system temp directory, where a fixed filename races other runs of this + // package. + outsideDir := createTempDir(t) + defer os.RemoveAll(outsideDir) + outside := filepath.Join(outsideDir, "outside.txt") + if err := os.WriteFile(outside, []byte("secret\n"), 0644); err != nil { + t.Fatalf("failed to write out-of-root file: %v", err) + } + + for name, target := range map[string]string{ + "inside-link": "plain.txt", + "escaping-link": outside, + "broken-link": filepath.Join(root, "does-not-exist"), + } { + if err := os.Symlink(target, filepath.Join(root, name)); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + } + loop := filepath.Join(root, "loop-link") + if err := os.Symlink(loop, loop); err != nil { + t.Skipf("symlinks not supported: %v", err) + } + fifoErr := syscall.Mkfifo(filepath.Join(root, "pipe"), 0644) + + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatalf("EvalSymlinks(root) error = %v", err) + } + resolvedPlain, err := filepath.EvalSymlinks(plain) + if err != nil { + t.Fatalf("EvalSymlinks(plain) error = %v", err) + } + + entries := map[string]fs.DirEntry{} + dirEntries, err := os.ReadDir(resolvedRoot) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + for _, d := range dirEntries { + entries[d.Name()] = d + } + + tests := []struct { + name string + entry string + wantAction walkEntryAction + wantResolved string + why string + }{ + {"regular file", "plain.txt", walkEntryGrep, resolvedPlain, + "a normal in-bounds file is the whole point"}, + {"symlink inside the root", "inside-link", walkEntryGrep, resolvedPlain, + "resolves through to its target so the caller reads what was just checked"}, + {"symlink escaping the root", "escaping-link", walkEntrySkip, "", + "a symlink must not be usable to read outside the searched directory"}, + {"broken symlink", "broken-link", walkEntryUnreadable, "", + "a dangling link is a real read failure, not a policy exclusion"}, + {"symlink loop", "loop-link", walkEntryUnreadable, "", + "same class of failure as a dangling link"}, + {"directory", "subdir", walkEntrySkip, "", + "WalkDir descends on its own; Python never reaches this case"}, + {"fifo", "pipe", walkEntrySkip, "", + "excluded by policy, not failure -- opening one can block forever"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.entry == "pipe" && fifoErr != nil { + t.Skipf("FIFOs not supported: %v", fifoErr) + } + d, ok := entries[tt.entry] + if !ok { + t.Fatalf("test setup: no dir entry named %q", tt.entry) + } + action, resolved := classifyWalkEntry(resolvedRoot, filepath.Join(resolvedRoot, tt.entry), d) + if action != tt.wantAction { + t.Errorf("classifyWalkEntry() action = %v, want %v (%s)", action, tt.wantAction, tt.why) + } + if resolved != tt.wantResolved { + t.Errorf("classifyWalkEntry() resolved = %q, want %q", resolved, tt.wantResolved) + } + }) + } +} + func TestGrepContent(t *testing.T) { tmpDir := createTempDir(t) defer os.RemoveAll(tmpDir)