feat(tools): wire up dormant file_read/write/edit/patch builtins for general agents (#268)

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 |

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)
+			}
+		}
+	})
+}

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)

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)
+		}
+	})
+}

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)