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

[initializ-mk]

Closes #268.

Problem

Forge shipped four fully-implemented, unit-tested file builtins that the runtime never registered — file_read, file_write, file_edit, file_patch. They were reachable only through CodeAgent*Tools() constructors that nothing in the runtime called (only RegisterCodeAgentSearchTools was wired), so they were effectively dead code.

Consequence: a general (non-code-agent) agent got grep_search / glob_search / directory_tree + file_create, but no general file read-back, edit, or overwrite. Real editing existed only inside the code-agent skill via its project-scoped code_agent_* SKILL.md tools.

Change

Register file_read / file_write / file_edit / file_patch for general agents, confined to the same searchRoot the search tools use, so read/edit and grep/glob share one PathValidator (#235) confinement boundary.

• builtins/register.go — new FileTools() / RegisterFileTools() helper (mirrors the existing CodeAgentSearchTools pattern).
• runner.go — registerGeneralFileTools() registers the four right after the search tools.

Code-agent precedence

The general file_* builtins are skipped when the code-agent skill is active — its project-scoped code_agent_* tools (registered from SKILL.md) are the specialized file surface, so registering the general builtins too would show the LLM two overlapping file surfaces. Skill tools win. (The names differ — file_* vs code_agent_* — so there's no registry collision either way; this is a surface-clarity choice, not conflict avoidance.)

Acceptance criteria

• file_read / file_write / file_edit / file_patch registered by the runtime for general agents, path-confined to the working root.
• Confinement test: each tool rejects a ../../etc/passwd-style traversal (guards the [ASI02] cli_execute workDir path confinement — reads escape to /etc/passwd (GAP-PATH) #235-class cli_execute escape through the newly-wired surface).
• No regression / no double-registration conflict when the code-agent skill is active (general file_* skipped; code_agent_* unaffected).
• Docs note the general file tools vs. the code-agent code_agent_* tools.

On #223 (destructive-action approval)

The issue asks the mutating tools to honor the destructive-action approval / dry-run posture. #223 is still open — there is no such posture yet. The safety boundary that exists today is path confinement (#235), which these tools already enforce via PathValidator — the same boundary file_create and cli_execute rely on. When #223 lands it applies to all mutating tools uniformly at BeforeToolExec (like the platform command guard in #304), covering these automatically. Registering them now under the existing confinement is consistent with file_create already being a default write tool.

Tests

• TestRegisterFileTools_RegistersTheFour — the four are registered (no longer dead code).
• TestFileTools_ConfinePathTraversal — each rejects ../../…/etc/passwd with a confinement error.
• TestRegisterGeneralFileTools — general agent registers all four; code-agent active skips them.

Full forge-core/tools/... + forge-cli/runtime suites pass; lint clean.

Docs

docs/core-concepts/tools-and-builtins.md — renamed the section to File & Search Tools, documents that the file surface is registered by default, and adds a table contrasting the general file_* builtins with the code-agent skill's code_agent_* tools (two non-colliding surfaces).

[initializ-mk]

Review: wire up dormant file_read/write/edit/patch builtins

Small, well-scoped, correctly reasoned. The FileTools()/RegisterFileTools() helper mirrors the existing CodeAgentSearchTools pattern exactly, the confinement boundary is shared (not re-derived), and CI is fully green. Two things I verified as positives:

• Emergent #304 coverage: because the mutating tools now flow through BeforeToolExec, the platform denied_command_patterns guard matches against their JSON args (including decoded values, after the #304 fix). So an operator can already write a pattern blocking file_write to sensitive paths — the "all mutating tools covered uniformly at BeforeToolExec" posture the PR body gestures at for #223 is partially real today for the command-deny axis.
• #223 deferral is honest and correct: path confinement is the current boundary, consistent with file_create already being a default write tool. No pretense that destructive-action approval exists.

Three minors, all inline. Verdict below.

Verdict

Merge-ready. All three findings are minor — finding 1 (the double hasSkill computation) is the one I'd actually fold in since it's a correctness-adjacent consistency fix, not just perf; findings 2–3 are test quality and a docs line. The core wiring is exactly right.

[initializ-mk]

Minor: hasSkill("code-agent") is computed twice, and the two calls MUST agree.

This is the second call — line ~786 already computed it to scope searchRoot to workspace/. Each call re-runs discoverSkillFiles() + ParseFileWithMetadata over every skill file on disk (redundant I/O in the registration path). More importantly, these are two reads of the same fact that must stay consistent: if they ever diverged (a caching change, a race with hot-reload), you'd get file tools confined to workspace/ but registered as if code-agent were inactive, or vice versa.

Fix: hoist once — codeAgentActive := r.hasSkill("code-agent") above the search-tool registration — and pass it to both the searchRoot decision and registerGeneralFileTools. One computation, provably consistent.

[initializ-mk]

Minor (test quality): the file_patch case passes by accident of evaluation order.

"action": "write" isn't a valid action (the enum is add/update/delete/move). This case passes only because file_patch resolves op.Path BEFORE the action switch, so the traversal error fires before the invalid action is ever evaluated — right thing tested, wrong reason.

Use a valid action (update), and separately add a move case with a traversal in new_path: file_patch resolves NewPath too (file_patch.go ~107), and that destination-confinement path currently has no test. A move that reads a confined source but writes ../../etc/cron.d/x is exactly the escape worth pinning.

[initializ-mk]

Minor (docs): the scope cell says "WorkDir (or workspace/ when a code-agent workspace exists)" — but the paragraph below says the general file_* builtins are skipped when the code-agent skill is active. So the general builtins are never actually scoped to workspace/; that parenthetical describes a state that can't occur for them (it's true for the search tools, which register in both cases — looks like a copy from that row). Drop the parenthetical from this row to avoid implying a code-agent-active general-file-tool exists.

[initializ-mk]

Thanks — all three folded in (e3505ce).

1 (hasSkill computed twice) — hoisted. codeAgentActive := r.hasSkill("code-agent") is now computed once above the search-tool registration and passed to both the searchRoot scoping and registerGeneralFileTools. One discoverSkillFiles() pass instead of two, and the two reads of the same fact are now provably consistent (searchRoot=workspace/ ⇔ general file tools skipped) — no way to diverge under a future caching/hot-reload change.

2 (file_patch test passed by accident) — fixed + extended. The traversal case now uses a valid action (update), so the rejection is the path-confinement check rather than an unknown-action error firing first. Added a dedicated move case pinning destination confinement: a move with a confined path but an escaping new_path (../../…/etc/cron.d/x) must be rejected — file_patch resolves NewPath too, and that path had no test. Asserts the error names new_path + "outside the working directory".

3 (docs scope cell) — dropped the parenthetical. The general-file-tools row now just says WorkDir; the (or workspace/ …) was copied from the search-tools row and can't apply here (general file_* are skipped under code-agent, so they're never scoped to workspace/).

Full tools/... + runtime suites green, lint clean. Also noted your two positives — the emergent #304 command-deny coverage over these mutating tools' JSON args, and the #223 deferral being honest.

[initializ-mk]

Re-review of e3505ce — all three findings resolved, CI green

Finding 1 (double hasSkill) — hoisted exactly as suggested. codeAgentActive := r.hasSkill("code-agent") computed once above the search-tool block, passed to both the searchRoot scoping and registerGeneralFileTools. The comment states the invariant flagged in review — "they must agree (searchRoot=workspace/ ⇔ general file tools skipped)" — so the consistency guarantee is now documented, not incidental. Redundant discoverSkillFiles() I/O gone.

Finding 2 (test quality) — both halves fixed. The file_patch case now uses the valid update action, so the rejection genuinely exercises path confinement rather than an unknown-action error firing first. The new file_patch move rejects escaping new_path subtest pins the destination-confinement path — a move with a confined source but an escaping new_path, asserting the error names both new_path and the confinement string. Exactly the gap closed, and specific enough to catch a regression that only checked the source path.

Finding 3 (docs) — parenthetical dropped. The general-builtins scope cell now reads just WorkDir; the workspace/ note stays only where it's true (the search tools, registered in both cases).

Verdict

All findings resolved — merge-ready (all 10 checks green). Clean cycle: the correctness-adjacent finding got the hoist plus an invariant comment, and the test that passed by accident now passes for the right reason with the move-destination escape explicitly pinned.