diff --git a/docs/core-concepts/tools-and-builtins.md b/docs/core-concepts/tools-and-builtins.md index e18e01a..690ef7e 100644 --- a/docs/core-concepts/tools-and-builtins.md +++ b/docs/core-concepts/tools-and-builtins.md @@ -39,29 +39,41 @@ Tools are capabilities that an LLM agent can invoke during execution. Forge prov Register all builtins with `builtins.RegisterAll(registry)`. -## Code-Agent Tools +## File & Search Tools -When the `code-agent` skill is active, Forge registers additional tools for autonomous code generation and modification. These tools are **not** registered by default — they are conditionally added when the skill requires them. - -All code-agent tools use a `PathValidator` that confines resolved paths within the agent's working directory, preventing directory traversal attacks. +Every Forge agent gets a file read/write/edit/patch surface plus search, registered by default and confined to the agent's working directory (`WorkDir`) via a `PathValidator`. All resolved paths are confined within the working directory, preventing directory-traversal attacks. | Tool | Description | |------|-------------| | `file_read` | Read file contents with optional line offset/limit, or list directory entries | -| `file_write` | Create or overwrite files in the project directory | +| `file_write` | Create or overwrite files in the working directory | | `file_edit` | Edit files by exact string matching with unified diff output | | `file_patch` | Batch file operations (add, update, delete, move) in a single call | | `glob_search` | Find files by glob pattern (e.g., `**/*.go`), sorted by modification time | | `grep_search` | Search file contents with regex; uses `rg` if available, falls back to Go | | `directory_tree` | Display tree-formatted directory listing (default max depth: 3) | +`file_read` / `file_write` / `file_edit` / `file_patch` are registered for general agents by the runtime (#268); previously they were reachable only when a skill wired them up. They give a general agent a real file-editing surface — not just `file_create` + search. + +### General file tools vs. the code-agent skill + +There are two distinct file surfaces, and they do **not** collide: + +| Surface | Tools | Scope | When | +|---------|-------|-------|------| +| General builtins (#268) | `file_read` / `file_write` / `file_edit` / `file_patch` | `WorkDir` | Every agent, **except** when the code-agent skill is active | +| Code-agent skill | `code_agent_read` / `code_agent_write` / `code_agent_run` (from the skill's `SKILL.md`) | the skill's `project_dir` | Only when the `code-agent` skill is active | + +When the `code-agent` skill is active, the general `file_*` builtins are **skipped** — the skill's project-scoped `code_agent_*` tools are the specialized file surface (skill tools win), so the LLM never sees two overlapping file surfaces. Search tools (`grep_search` / `glob_search` / `directory_tree`) are registered in both cases, scoped to `workspace/` under the code-agent skill and to `WorkDir` otherwise. + ### Registration Groups -Code-agent tools are registered in layered groups, allowing skills to request only the capabilities they need: +These tools are constructed and registered in layered groups (`forge-core/tools/builtins/register.go`), so the runtime and skills can request only the capabilities they need: | Group | Tools | Purpose | |-------|-------|---------| -| `CodeAgentSearchTools` | `grep_search`, `glob_search`, `directory_tree` | Read-only exploration | +| `FileTools` | `file_read`, `file_write`, `file_edit`, `file_patch` | General file surface — registered for non-code-agent agents (#268) | +| `CodeAgentSearchTools` | `grep_search`, `glob_search`, `directory_tree` | Read-only exploration — registered for every Forge agent | | `CodeAgentReadTools` | `file_read` + search tools | Safe reading | | `CodeAgentWriteTools` | `file_write`, `file_edit`, `file_patch` | Modification | | `CodeAgentTools` | All read + write tools | Full code-agent capability | diff --git a/forge-cli/runtime/file_tools_register_test.go b/forge-cli/runtime/file_tools_register_test.go new file mode 100644 index 0000000..ddd5fc7 --- /dev/null +++ b/forge-cli/runtime/file_tools_register_test.go @@ -0,0 +1,36 @@ +package runtime + +import ( + "testing" + + "github.com/initializ/forge/forge-core/tools" +) + +// TestRegisterGeneralFileTools pins the #268 registration decision: a general +// agent gets the file read/write/edit/patch builtins; when the code-agent +// skill is active they are skipped (its code_agent_* tools are the file +// surface — skill tools win, no double surface). +func TestRegisterGeneralFileTools(t *testing.T) { + r := &Runner{logger: nopLogger{}} + fileToolNames := []string{"file_read", "file_write", "file_edit", "file_patch"} + + t.Run("general agent registers all four", func(t *testing.T) { + reg := tools.NewRegistry() + r.registerGeneralFileTools(reg, t.TempDir(), false /* codeAgentActive */) + for _, name := range fileToolNames { + if reg.Get(name) == nil { + t.Errorf("general agent should have %q registered", name) + } + } + }) + + t.Run("code-agent active skips them", func(t *testing.T) { + reg := tools.NewRegistry() + r.registerGeneralFileTools(reg, t.TempDir(), true /* codeAgentActive */) + for _, name := range fileToolNames { + if reg.Get(name) != nil { + t.Errorf("code-agent agent should NOT register general %q (code_agent_* wins)", name) + } + } + }) +} diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index 7636957..b193cdc 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -780,10 +780,15 @@ func (r *Runner) Run(ctx context.Context) error { } // Register search/exploration tools (grep, glob, tree). + // Compute the code-agent state ONCE — both the searchRoot scoping + // and the general-file-tool decision below read it, and they must + // agree (searchRoot=workspace/ ⇔ general file tools skipped). + codeAgentActive := r.hasSkill("code-agent") + // When code-agent skill is active, scope them to workspace/ so searches // default to cloned repos. Otherwise scope to the main working directory. searchRoot := r.cfg.WorkDir - if r.hasSkill("code-agent") { + if codeAgentActive { codeDir := filepath.Join(r.cfg.WorkDir, "workspace") if mkErr := os.MkdirAll(codeDir, 0o755); mkErr != nil { r.logger.Warn("failed to create code workspace directory", map[string]any{"error": mkErr.Error()}) @@ -797,6 +802,10 @@ func (r *Runner) Run(ctx context.Context) error { r.logger.Warn("failed to register search tools", map[string]any{"error": err.Error()}) } + // Register the general file read/write/edit/patch builtins (#268), + // confined to the same searchRoot as the search tools. + r.registerGeneralFileTools(reg, searchRoot, codeAgentActive) + // Register read_skill tool for lazy-loading skill instructions readSkill := builtins.NewReadSkillTool(r.cfg.WorkDir) if regErr := reg.Register(readSkill); regErr != nil { @@ -3193,6 +3202,25 @@ func ensureGitignore(workDir string) { // hasSkill checks whether a skill with the given name is present in the project's // discovered skill files. Checks both ## Tool: entry names and frontmatter name. +// registerGeneralFileTools wires the general file read/write/edit/patch +// builtins (#268) into reg, confined to root (the same searchRoot the search +// tools use, so read/edit and grep/glob share one #235 confinement boundary). +// +// Skipped when the code-agent skill is active: its project-scoped code_agent_* +// tools (registered from SKILL.md) are the specialized file surface, so adding +// the general file_* builtins too would present the LLM two overlapping file +// surfaces — skill tools win. (The names differ, so there is no registry +// collision either way; this is a surface-clarity decision, not a conflict +// avoidance.) +func (r *Runner) registerGeneralFileTools(reg *tools.Registry, root string, codeAgentActive bool) { + if codeAgentActive { + return + } + if err := builtins.RegisterFileTools(reg, root); err != nil { + r.logger.Warn("failed to register file tools", map[string]any{"error": err.Error()}) + } +} + func (r *Runner) hasSkill(name string) bool { for _, sf := range r.discoverSkillFiles() { entries, meta, err := cliskills.ParseFileWithMetadata(sf) diff --git a/forge-core/tools/builtins/file_tools_register_test.go b/forge-core/tools/builtins/file_tools_register_test.go new file mode 100644 index 0000000..71fdcf9 --- /dev/null +++ b/forge-core/tools/builtins/file_tools_register_test.go @@ -0,0 +1,80 @@ +package builtins + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/initializ/forge/forge-core/tools" +) + +// TestRegisterFileTools_RegistersTheFour pins that the #268 wiring exposes the +// general file read/write/edit/patch builtins (previously dead code, reachable +// only from tests). +func TestRegisterFileTools_RegistersTheFour(t *testing.T) { + reg := tools.NewRegistry() + if err := RegisterFileTools(reg, t.TempDir()); err != nil { + t.Fatalf("RegisterFileTools: %v", err) + } + for _, name := range []string{"file_read", "file_write", "file_edit", "file_patch"} { + if reg.Get(name) == nil { + t.Errorf("expected %q to be registered", name) + } + } +} + +// TestFileTools_ConfinePathTraversal is the #235-class guard the issue asks +// for: every general file tool must reject a `../../etc/passwd`-style escape +// out of the confinement root. Regression guard against re-opening the +// cli_execute-class escape through the newly-wired file surface. +func TestFileTools_ConfinePathTraversal(t *testing.T) { + root := t.TempDir() + ft := FileTools(root) + + // Each tool's minimal args plus a traversal path. All must error before + // touching /etc/passwd. file_patch uses a VALID action (update) so the + // rejection is the path-confinement check, not an accidental unknown-action + // error firing first. + cases := map[string]map[string]any{ + "file_read": {"path": "../../../../../../etc/passwd"}, + "file_write": {"path": "../../../../../../etc/passwd", "content": "x"}, + "file_edit": {"path": "../../../../../../etc/passwd", "old_text": "a", "new_text": "b"}, + "file_patch": {"operations": []map[string]any{ + {"action": "update", "path": "../../../../../../etc/passwd", "content": "x"}, + }}, + } + + for _, tool := range ft { + args, ok := cases[tool.Name()] + if !ok { + t.Fatalf("no traversal case for tool %q", tool.Name()) + } + raw, _ := json.Marshal(args) + _, err := tool.Execute(context.Background(), raw) + if err == nil { + t.Errorf("%s: traversal path was NOT rejected (path confinement regressed)", tool.Name()) + continue + } + if !strings.Contains(err.Error(), "outside the working directory") { + t.Errorf("%s: expected a confinement error, got %v", tool.Name(), err) + } + } + + // file_patch resolves the move DESTINATION (new_path) too — a move whose + // source is confined but whose new_path escapes must also be rejected. + // Guards the destination-confinement path directly. + t.Run("file_patch move rejects escaping new_path", func(t *testing.T) { + patch := &filePatchTool{pathValidator: NewPathValidator(root)} + raw, _ := json.Marshal(map[string]any{"operations": []map[string]any{ + {"action": "move", "path": "src.txt", "new_path": "../../../../../../etc/cron.d/x"}, + }}) + _, err := patch.Execute(context.Background(), raw) + if err == nil { + t.Fatal("move with an escaping new_path was NOT rejected") + } + if !strings.Contains(err.Error(), "new_path") || !strings.Contains(err.Error(), "outside the working directory") { + t.Errorf("expected a new_path confinement error, got %v", err) + } + }) +} diff --git a/forge-core/tools/builtins/register.go b/forge-core/tools/builtins/register.go index dc7b366..f0f6b37 100644 --- a/forge-core/tools/builtins/register.go +++ b/forge-core/tools/builtins/register.go @@ -80,6 +80,33 @@ func RegisterCodeAgentSearchTools(reg *tools.Registry, workDir string) error { return nil } +// FileTools returns the general file read/write/edit/patch builtins, +// path-confined to workDir via a PathValidator (the same #235 confinement the +// search tools use). These are the general-agent counterpart to the code-agent +// skill's project-scoped code_agent_* tools (#268): the runtime registers them +// for a general agent so it has a real file read-back / edit / overwrite +// surface, not just search + file_create. +func FileTools(workDir string) []tools.Tool { + pv := NewPathValidator(workDir) + return []tools.Tool{ + &fileReadTool{pathValidator: pv}, + &fileWriteTool{pathValidator: pv}, + &fileEditTool{pathValidator: pv}, + &filePatchTool{pathValidator: pv}, + } +} + +// RegisterFileTools registers the general file read/write/edit/patch builtins +// (#268), confined to workDir. +func RegisterFileTools(reg *tools.Registry, workDir string) error { + for _, t := range FileTools(workDir) { + if err := reg.Register(t); err != nil { + return err + } + } + return nil +} + // CodeAgentReadTools returns read-only coding tools (file_read + search). func CodeAgentReadTools(workDir string) []tools.Tool { pv := NewPathValidator(workDir)