Files
zero/internal/tools/apply_patch.go
75f15da82a Tools: arg-tolerance, ask_user, secret scrubbing, structured results, hardening (#119)
* Tools: arg-tolerance, ask_user, secret scrubbing, structured results, hardening

First module of the tools->agent->tui chain that leads to the TUI shell (off main). Scope: internal/tools (+ a 1-line specialist vocabulary update).

3-way merged onto main's specialist drift (RunOptions tool-context fields, Safety.AdvertiseInAuto, optionsAwareTool dispatch, PropertySchema, base structured Result) and grafted the runtime-core additions:

- Shared arg-tolerance layer (argtolerance.go: alias-aware/type-strict arg resolution + slice coercion) adopted across all tools so weak models' non-canonical arg shapes are accepted; bool/int coercion in args.go.

- ask_user tool (interactive clarifying questions) wired into CoreReadOnlyTools; nested PropertySchema (Properties/Required) for its object questions + update_plan.

- Secret scrubbing at the registry boundary (scrubResultSecrets via internal/redaction) applied to every tool-execution path, incl. the optionsAwareTool dispatch; structured Result (Display/Redacted/ChangedFiles).

- mutation_targets (paths a mutating tool will write, for session checkpointing) + hardened grep (symlink confinement), apply_patch, bash, edit/write.

EXCLUDED per the module split: the blueprint task tool (subagents — another dev's area; the merged specialist Task is separate) and the skill tool (ships with the later skills module); plus their subagent-only registry helpers (Without/isolateForChild).

ask_user is now a core tool, so internal/specialist knownToolNames adds "ask_user" to keep the TestKnownToolNamesMatchCoreRegistry cross-package invariant. No new deps; build/vet/-race/windows/full-suite green.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* Address #119 review (CodeRabbit): scrub Display, validate apply_patch mutation paths, narrow test skips

- registry.go: scrubResultSecrets now redacts Result.Display.Summary too (not just Output), so a caller preferring Display can't bypass the boundary redaction.

- mutation_targets.go: apply_patch targets now run through validatePatchPaths(applyRoot, ...) before being returned, so a traversal patch (../x) can't yield an out-of-workspace checkpoint target. Regression case added to TestMutationTargetsRejectsEscapingPaths.

- argtolerance_test/write_tools_test: the apply_patch tests no longer Skipf on ANY non-OK result (which masked real diff/apply regressions); they skip ONLY when the git binary is unavailable (gitApplyUnavailable helper checks exec's 'executable file not found') and otherwise Fatalf.

build/vet/-race/full-suite + GOOS=windows build green.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* Address #119 re-review (CodeRabbit): best-effort multiSelect + Display.Summary scrub coverage

- ask_user: an uncoercible multiSelect no longer fails the whole call — it's a UI hint, so it defaults to false (best-effort, mirroring the lenient options path). Test: TestParseAskUserQuestionsLenientOptions extended.

- registry_test: TestRunWithOptionsScrubsSecretsForAllCallers now also leaks a secret via Result.Display.Summary and asserts it is redacted, locking in the boundary scrub for the structured-result path (secretTool gained a display field).

build/vet/-race/full-suite + GOOS=windows build green.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* Address #119 review (@Vasanthdev2004): scrub ALL registry return paths + harden intArg

[blocker] RunWithOptions now scrubs every return path, not just tool-execution: a named return + a single 'defer scrubResultSecrets(result)' covers the unknown-tool, sandbox-deny, sandbox-approval-required, permission-required and permission-denied early returns too. Previously a sandbox denial that echoed a secret-bearing path/arg returned it raw with Redacted=false. Regression: TestRunWithOptionsScrubsSecretsOnDenialPaths (permission-denied reason carrying a token -> scrubbed + Redacted).

[non-blocking] intArg fails closed on NaN/Inf/out-of-range floats (and their string forms) via a floatToInt helper, before any implementation-defined cast. Tests added for NaN/Inf/NaN-string/non-integral.

build/vet/-race/full-suite + GOOS=windows build green.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* Address #119 review (CodeRabbit grep TOCTOU + anandh8x dependency blockers)

- grep: close the confinement TOCTOU — confineGrepFile now returns the symlink-RESOLVED path and collectGrepMatches reads THAT (not the raw candidate), so a symlink swapped in between check and read can't escape the workspace.

- ask_user: removed from CoreReadOnlyTools. It needs the agent loop's interactive intercept (OnAskUser), which is in the (unmerged) agent module; without it the tool only returns the non-interactive fallback. The tool still ships (NewAskUserTool); the agent module registers it in core + wires the intercept together. Reverted the specialist knownToolNames 'ask_user' entry + the ask_user-in-core test accordingly.

- update_plan: reverted the plan schema to a flat array (item structure documented in the description). The structured nested-object Items schema depends on the agent's PropertySchema serializer (propertyToRuntimeMap) passing nested Properties/Required through to providers, which is in the agent module; a nested schema here would be silently dropped. PropertySchema.Properties/Required fields are kept (used by ask_user.go's own schema + the later MCP module).

build/vet/-race/full-suite + GOOS=windows build green; no new deps.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* Harden #119 from adversarial self-review: sandbox alias-gate, Meta scrub, intArg 2^63

A multi-dimension adversarial review of the tools diff surfaced 3 real issues beyond the reviewer rounds:

- [major/security] write_file/edit_file path aliases file_path/filename/filepath bypassed the sandbox workspace+symlink gate: sandbox.requestPaths only inspected path/cwd/file/dir, so an aliased target skipped enforcement (same class as audit H4). Aligned requestPaths with the tools' path-alias lists (internal/sandbox/risk.go). The tools' own resolveWorkspaceTargetPath still confined, but the sandbox LAYER/audit/symlink-check was silently skipped. Regression: TestRegistrySandboxGatesPathAliasKeys.

- [minor] scrubResultSecrets skipped Result.Meta, which is forwarded to the transcript and carries model-controlled strings (glob pattern, bash cwd). Now scrubs Meta values too. Covered in TestRunWithOptionsScrubsSecretsForAllCallers.

- [minor] floatToInt used f > float64(MaxInt); that constant rounds up to 2^63 so exactly 2^63 slipped to an out-of-range cast. Changed to >=. Test added for 2^63 float+string.

build/vet/-race/full-suite + GOOS=windows build green; no new deps.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

---------

Co-authored-by: KRATOS <[email protected]>
Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
2026-06-07 17:13:27 +05:30

179 lines
5.4 KiB
Go

package tools
import (
"context"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
)
type applyPatchTool struct {
baseTool
workspaceRoot string
}
func NewApplyPatchTool(workspaceRoot string) Tool {
return applyPatchTool{
baseTool: baseTool{
name: "apply_patch",
description: "Apply a unified diff patch inside the workspace.",
parameters: Schema{
Type: "object",
Properties: map[string]PropertySchema{
"patch": {Type: "string", Description: "Unified diff patch to apply."},
"cwd": {Type: "string", Description: "Directory where the patch should be applied. Defaults to workspace root.", Default: "."},
},
Required: []string{"patch"},
AdditionalProperties: false,
},
safety: promptSafety(SideEffectWrite, "Applies patch hunks that can create, edit, or delete files."),
},
workspaceRoot: normalizeWorkspaceRoot(workspaceRoot),
}
}
func (tool applyPatchTool) Run(ctx context.Context, args map[string]any) Result {
patch, err := aliasedStringArg(args, []string{"patch", "diff"}, "", true, false)
if err != nil {
return errorResult("Error: Invalid arguments for apply_patch: " + err.Error())
}
cwd, err := stringArg(args, "cwd", ".", false)
if err != nil {
return errorResult("Error: Invalid arguments for apply_patch: " + err.Error())
}
applyRoot, relativeRoot, err := resolveWorkspacePath(tool.workspaceRoot, cwd)
if err != nil {
return errorResult("Error applying patch: " + err.Error())
}
if err := validatePatchPaths(applyRoot, patch); err != nil {
return errorResult("Error applying patch: " + err.Error())
}
tempFile, err := os.CreateTemp("", "zero-patch-*.patch")
if err != nil {
return errorResult("Error applying patch: " + err.Error())
}
patchPath := tempFile.Name()
defer func() {
_ = os.Remove(patchPath)
}()
if _, err := tempFile.WriteString(patch); err != nil {
_ = tempFile.Close()
return errorResult("Error applying patch: " + err.Error())
}
if err := tempFile.Close(); err != nil {
return errorResult("Error applying patch: " + err.Error())
}
if err := recheckPatchWriteTargets(applyRoot, patch); err != nil {
return errorResult("Error applying patch: " + err.Error())
}
command := exec.CommandContext(ctx, "git", "apply", "--whitespace=nowarn", patchPath)
command.Dir = applyRoot
output, err := command.CombinedOutput()
if err != nil {
message := strings.TrimSpace(string(output))
if message == "" {
message = err.Error()
}
return errorResult("Error applying patch: " + message)
}
summary := "Patch applied successfully."
if relativeRoot != "." {
summary = "Patch applied successfully in " + relativeRoot + "."
}
result := okResult(summary)
result.ChangedFiles = changedFilesFromPatch(relativeRoot, patch)
result.Display = Display{Summary: summary, Kind: "diff"}
return result
}
// changedFilesFromPatch extracts the unique, WORKSPACE-relative paths a patch
// touches, reusing the same per-line parser used for validation. Patch paths are
// relative to the apply cwd, so relativeRoot (the workspace-relative cwd, e.g.
// "sub/dir", or "." for the workspace root) is prefixed so callers get true
// workspace-relative paths regardless of cwd.
func changedFilesFromPatch(relativeRoot string, patch string) []string {
seen := map[string]bool{}
var paths []string
for _, line := range strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") {
for _, path := range patchPathsFromLine(line) {
if path == "" || path == "/dev/null" {
continue
}
workspacePath := path
if relativeRoot != "" && relativeRoot != "." {
workspacePath = filepath.ToSlash(filepath.Join(relativeRoot, path))
}
if seen[workspacePath] {
continue
}
seen[workspacePath] = true
paths = append(paths, workspacePath)
}
}
return paths
}
func validatePatchPaths(root string, patch string) error {
for _, line := range strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") {
for _, path := range patchPathsFromLine(line) {
if path == "" || path == "/dev/null" {
continue
}
if filepath.IsAbs(path) || path == ".." || strings.HasPrefix(path, "../") {
return fmt.Errorf("patch path %q must stay inside the workspace", path)
}
if _, _, err := resolveWorkspaceTargetPath(root, path); err != nil {
return err
}
}
}
return nil
}
func recheckPatchWriteTargets(root string, patch string) error {
for _, line := range strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") {
for _, path := range patchPathsFromLine(line) {
if path == "" || path == "/dev/null" {
continue
}
if err := recheckWorkspaceWriteTarget(root, path); err != nil {
return err
}
}
}
return nil
}
func patchPathsFromLine(line string) []string {
if strings.HasPrefix(line, "diff --git ") {
fields := strings.Fields(line)
if len(fields) >= 4 {
return []string{stripPatchPrefix(fields[2]), stripPatchPrefix(fields[3])}
}
}
if strings.HasPrefix(line, "--- ") || strings.HasPrefix(line, "+++ ") {
fields := strings.Fields(line)
if len(fields) >= 2 {
return []string{stripPatchPrefix(fields[1])}
}
}
return nil
}
func stripPatchPrefix(path string) string {
path = strings.TrimSpace(path)
// A unified-diff path carries exactly one of the a/ or b/ prefixes; strip a
// single one so a real directory literally named "a" or "b" is preserved.
if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") {
path = path[2:]
}
return filepath.ToSlash(path)
}