From 9c9d751480d4cd92866e5db754241dea67c34d02 Mon Sep 17 00:00:00 2001 From: MK Date: Mon, 13 Jul 2026 20:36:05 -0400 Subject: [PATCH 1/2] feat(tools): wire up dormant file_read/write/edit/patch builtins for general agents (closes #268) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Forge shipped four fully-implemented, unit-tested file builtins that the runtime never registered — reachable only from tests (effectively dead code). A general (non-code-agent) agent got grep/glob/tree + file_create but no general file read-back, edit, or overwrite. Register file_read/file_write/file_edit/file_patch in the runtime for general agents, confined to the same searchRoot the search tools use so read/edit and grep/glob share one #235 PathValidator confinement boundary. - builtins/register.go: new FileTools() / RegisterFileTools() helper. - runner.go: registerGeneralFileTools() registers the four after the search tools. Skipped when the code-agent skill is active — its project-scoped code_agent_* tools are the specialized file surface, so registering the general file_* builtins too would show the LLM two overlapping file surfaces (skill tools win; names differ so there is no registry collision either way). Tests: - RegisterFileTools registers the four named tools. - Each general file tool rejects a ../../etc/passwd traversal (guards the #235-class cli_execute escape through the newly-wired surface). - registerGeneralFileTools: general agent gets all four; code-agent active skips them. Docs: tools-and-builtins.md documents the general file surface and the general-vs-code-agent distinction (two non-colliding surfaces). Note: mutating tools inherit the #235 path confinement that file_create / cli_execute already rely on; #223 (action-level approval / dry-run) is still open and will layer over all mutating tools via BeforeToolExec when it lands. --- docs/core-concepts/tools-and-builtins.md | 26 +++++--- forge-cli/runtime/file_tools_register_test.go | 36 +++++++++++ forge-cli/runtime/runner.go | 23 +++++++ .../builtins/file_tools_register_test.go | 61 +++++++++++++++++++ forge-core/tools/builtins/register.go | 27 ++++++++ 5 files changed, 166 insertions(+), 7 deletions(-) create mode 100644 forge-cli/runtime/file_tools_register_test.go create mode 100644 forge-core/tools/builtins/file_tools_register_test.go diff --git a/docs/core-concepts/tools-and-builtins.md b/docs/core-concepts/tools-and-builtins.md index e18e01a4..20d9ec1d 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` (or `workspace/` when a code-agent workspace exists) | 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 00000000..ddd5fc75 --- /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 76369576..81f84e7a 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -797,6 +797,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, r.hasSkill("code-agent")) + // Register read_skill tool for lazy-loading skill instructions readSkill := builtins.NewReadSkillTool(r.cfg.WorkDir) if regErr := reg.Register(readSkill); regErr != nil { @@ -3193,6 +3197,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 00000000..4f1c5a71 --- /dev/null +++ b/forge-core/tools/builtins/file_tools_register_test.go @@ -0,0 +1,61 @@ +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. + 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": {"path": "../../../../../../etc/passwd", "operations": []map[string]any{ + {"action": "write", "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) + } + } +} diff --git a/forge-core/tools/builtins/register.go b/forge-core/tools/builtins/register.go index dc7b366e..f0f6b374 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) From e3505ce25a6647f131742b31aba3f29150391ffd Mon Sep 17 00:00:00 2001 From: MK Date: Tue, 14 Jul 2026 01:04:51 -0400 Subject: [PATCH 2/2] fix(tools): hoist code-agent check + fix file_patch confinement tests (#306 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - runner.go: compute hasSkill("code-agent") ONCE into codeAgentActive and pass it to both the searchRoot scoping and registerGeneralFileTools — removes redundant discoverSkillFiles() I/O and guarantees the two reads of the same fact can't diverge (searchRoot=workspace/ <=> file tools skipped). - file_patch confinement test: use a VALID action (update) so the rejection is the path-confinement check, not an unknown-action error firing first (the old 'write' action passed by accident of evaluation order). Add a dedicated move case that pins destination confinement — a move with a confined source but an escaping new_path must be rejected (file_patch resolves new_path too; that path had no test). - docs: drop the misleading '(or workspace/ ...)' parenthetical from the general-file-tools scope cell — general file_* are skipped under code-agent, so they are never scoped to workspace/. --- docs/core-concepts/tools-and-builtins.md | 2 +- forge-cli/runtime/runner.go | 9 +++++-- .../builtins/file_tools_register_test.go | 25 ++++++++++++++++--- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/docs/core-concepts/tools-and-builtins.md b/docs/core-concepts/tools-and-builtins.md index 20d9ec1d..690ef7ed 100644 --- a/docs/core-concepts/tools-and-builtins.md +++ b/docs/core-concepts/tools-and-builtins.md @@ -61,7 +61,7 @@ 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` (or `workspace/` when a code-agent workspace exists) | Every agent, **except** when the code-agent skill is active | +| 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. diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index 81f84e7a..b193cdcd 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()}) @@ -799,7 +804,7 @@ func (r *Runner) Run(ctx context.Context) error { // Register the general file read/write/edit/patch builtins (#268), // confined to the same searchRoot as the search tools. - r.registerGeneralFileTools(reg, searchRoot, r.hasSkill("code-agent")) + r.registerGeneralFileTools(reg, searchRoot, codeAgentActive) // Register read_skill tool for lazy-loading skill instructions readSkill := builtins.NewReadSkillTool(r.cfg.WorkDir) diff --git a/forge-core/tools/builtins/file_tools_register_test.go b/forge-core/tools/builtins/file_tools_register_test.go index 4f1c5a71..71fdcf9a 100644 --- a/forge-core/tools/builtins/file_tools_register_test.go +++ b/forge-core/tools/builtins/file_tools_register_test.go @@ -33,13 +33,15 @@ func TestFileTools_ConfinePathTraversal(t *testing.T) { ft := FileTools(root) // Each tool's minimal args plus a traversal path. All must error before - // touching /etc/passwd. + // 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": {"path": "../../../../../../etc/passwd", "operations": []map[string]any{ - {"action": "write", "path": "../../../../../../etc/passwd", "content": "x"}, + "file_patch": {"operations": []map[string]any{ + {"action": "update", "path": "../../../../../../etc/passwd", "content": "x"}, }}, } @@ -58,4 +60,21 @@ func TestFileTools_ConfinePathTraversal(t *testing.T) { 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) + } + }) }