Refactor filesystem tools to use a fileReadWriter interface for both host and sandboxed I/O, improving atomic writes and error handling.

pkg/tools/edit.go

@@ -2,8 +2,10 @@ package tools
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"strings"
 )
@@ -68,77 +70,19 @@ func (t *EditFileTool) Execute(ctx context.Context, args map[string]interface{})
 		return ErrorResult("new_text is required")
 	}
 
-	// If not restricted, perform operations directly
-	if !t.restrict {
-		content, err := os.ReadFile(path)
-		if err != nil {
-			if os.IsNotExist(err) {
-				return ErrorResult(fmt.Sprintf("file not found: %s", path))
+	if t.restrict {
+		return executeInRoot(t.allowedDir, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+			if err := editFileInRoot(root, relPath, oldText, newText); err != nil {
+				return nil, err
 			}
-			return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
-		}
-
-		contentStr := string(content)
-		if !strings.Contains(contentStr, oldText) {
-			return ErrorResult("old_text not found in file. Make sure it matches exactly")
-		}
-
-		count := strings.Count(contentStr, oldText)
-		if count > 1 {
-			return ErrorResult(fmt.Sprintf("old_text appears %d times. Please provide more context to make it unique", count))
-		}
-
-		newContent := strings.Replace(contentStr, oldText, newText, 1)
-
-		if err := os.WriteFile(path, []byte(newContent), 0644); err != nil {
-			return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
-		}
-
-		return SilentResult(fmt.Sprintf("File edited: %s", path))
+			return SilentResult(fmt.Sprintf("File edited: %s", path)), nil
+		})
 	}
 
-	// Use executeInRoot to safely access the file
-	return executeInRoot(t.allowedDir, path, func(root *os.Root, relPath string) (*ToolResult, error) {
-		f, err := root.Open(relPath)
-		if err != nil {
-			if os.IsNotExist(err) {
-				return nil, fmt.Errorf("file not found: %s", path)
-			}
-			return nil, fmt.Errorf("failed to open file: %w", err)
-		}
-
-		content, err := io.ReadAll(f)
-		f.Close()
-
-		if err != nil {
-			return nil, fmt.Errorf("failed to read file: %v", err)
-		}
-
-		contentStr := string(content)
-
-		if !strings.Contains(contentStr, oldText) {
-			return nil, fmt.Errorf("old_text not found in file. Make sure it matches exactly")
-		}
-
-		count := strings.Count(contentStr, oldText)
-		if count > 1 {
-			return nil, fmt.Errorf("old_text appears %d times. Please provide more context to make it unique", count)
-		}
-
-		newContent := strings.Replace(contentStr, oldText, newText, 1)
-
-		fw, err := root.Create(relPath)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create file for writing: %w", err)
-		}
-		defer fw.Close()
-
-		if _, err := fw.Write([]byte(newContent)); err != nil {
-			return nil, fmt.Errorf("failed to write file: %v", err)
-		}
-
-		return SilentResult(fmt.Sprintf("File edited: %s", path)), nil
-	})
+	if err := editFile(&hostRW{}, path, oldText, newText); err != nil {
+		return ErrorResult(err.Error())
+	}
+	return SilentResult(fmt.Sprintf("File edited: %s", path))
 }
 
 type AppendFileTool struct {
@@ -181,38 +125,108 @@ func (t *AppendFileTool) Execute(ctx context.Context, args map[string]interface{
 		return ErrorResult("path is required")
 	}
 
-	content, ok := args["content"].(string)
+	appendContent, ok := args["content"].(string)
 	if !ok {
 		return ErrorResult("content is required")
 	}
 
-	// If not restricted, perform operations directly
-	if !t.restrict {
-		f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
-		if err != nil {
-			return ErrorResult(fmt.Sprintf("failed to open file: %v", err))
-		}
-		defer f.Close()
-
-		if _, err := f.WriteString(content); err != nil {
-			return ErrorResult(fmt.Sprintf("failed to append to file: %v", err))
-		}
-
-		return SilentResult(fmt.Sprintf("Appended to %s", path))
+	var rw fileReadWriter
+	if t.restrict {
+		return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+			if err := appendFileWithRW(&rootRW{root: root}, relPath, appendContent); err != nil {
+				return nil, err
+			}
+			return SilentResult(fmt.Sprintf("Appended to %s", path)), nil
+		})
 	}
 
-	// Use executeInRoot to safely access the file
-	return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
-		f, err := root.OpenFile(relPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
-		if err != nil {
-			return nil, fmt.Errorf("failed to open file: %w", err)
-		}
-		defer f.Close()
-
-		if _, err := f.WriteString(content); err != nil {
-			return nil, fmt.Errorf("failed to append to file: %w", err)
-		}
-
-		return SilentResult(fmt.Sprintf("Appended to %s", path)), nil
-	})
+	rw = &hostRW{}
+	if err := appendFileWithRW(rw, path, appendContent); err != nil {
+		return ErrorResult(err.Error())
+	}
+	return SilentResult(fmt.Sprintf("Appended to %s", path))
+}
+
+// editFile reads the file via rw, performs the replacement, and writes back.
+// It uses a fileReadWriter, allowing the same logic for both restricted and unrestricted modes.
+func editFile(rw fileReadWriter, path, oldText, newText string) error {
+	content, err := rw.Read(path)
+	if err != nil {
+		return err
+	}
+
+	newContent, err := replaceEditContent(content, oldText, newText)
+	if err != nil {
+		return err
+	}
+
+	return rw.Write(path, newContent)
+}
+
+// editFileInRoot performs an in-place edit within an os.Root using a single open call.
+// By opening with O_RDWR and reusing the same file descriptor for both read and write,
+// we narrow the TOCTOU window compared to two separate open calls.
+func editFileInRoot(root *os.Root, relPath, oldText, newText string) error {
+	f, err := root.OpenFile(relPath, os.O_RDWR, 0)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("failed to read file: file not found: %w", err)
+		}
+		if os.IsPermission(err) || strings.Contains(err.Error(), "escapes from parent") {
+			return fmt.Errorf("failed to read file: access denied: %w", err)
+		}
+		return fmt.Errorf("failed to open file for editing: %w", err)
+	}
+	defer f.Close()
+
+	content, err := io.ReadAll(f)
+	if err != nil {
+		return fmt.Errorf("failed to read file content: %w", err)
+	}
+
+	newContent, err := replaceEditContent(content, oldText, newText)
+	if err != nil {
+		return err
+	}
+
+	// Truncate the file and seek back to the beginning before writing.
+	if err := f.Truncate(0); err != nil {
+		return fmt.Errorf("failed to truncate file for in-place edit: %w", err)
+	}
+	if _, err := f.Seek(0, io.SeekStart); err != nil {
+		return fmt.Errorf("failed to seek to beginning of file: %w", err)
+	}
+
+	if _, err := f.Write(newContent); err != nil {
+		return fmt.Errorf("failed to write edited content: %w", err)
+	}
+	return nil
+}
+
+// appendFileWithRW reads the existing content (if any) via rw, appends new content, and writes back.
+func appendFileWithRW(rw fileReadWriter, path, appendContent string) error {
+	content, err := rw.Read(path)
+	if err != nil && !errors.Is(err, fs.ErrNotExist) {
+		return err
+	}
+
+	newContent := append(content, []byte(appendContent)...)
+	return rw.Write(path, newContent)
+}
+
+// replaceEditContent handles the core logic of finding and replacing a single occurrence of oldText.
+func replaceEditContent(content []byte, oldText, newText string) ([]byte, error) {
+	contentStr := string(content)
+
+	if !strings.Contains(contentStr, oldText) {
+		return nil, fmt.Errorf("old_text not found in file. Make sure it matches exactly")
+	}
+
+	count := strings.Count(contentStr, oldText)
+	if count > 1 {
+		return nil, fmt.Errorf("old_text appears %d times. Please provide more context to make it unique", count)
+	}
+
+	newContent := strings.Replace(contentStr, oldText, newText, 1)
+	return []byte(newContent), nil
 }

pkg/tools/edit_test.go

@@ -288,3 +288,140 @@ func TestEditTool_AppendFile_MissingContent(t *testing.T) {
 		t.Errorf("Expected error when content is missing")
 	}
 }
+
+// TestReplaceEditContent verifies the helper function replaceEditContent
+func TestReplaceEditContent(t *testing.T) {
+	tests := []struct {
+		name        string
+		content     []byte
+		oldText     string
+		newText     string
+		expected    []byte
+		expectError bool
+	}{
+		{
+			name:        "successful replacement",
+			content:     []byte("hello world"),
+			oldText:     "world",
+			newText:     "universe",
+			expected:    []byte("hello universe"),
+			expectError: false,
+		},
+		{
+			name:        "old text not found",
+			content:     []byte("hello world"),
+			oldText:     "golang",
+			newText:     "rust",
+			expected:    nil,
+			expectError: true,
+		},
+		{
+			name:        "multiple matches found",
+			content:     []byte("test text test"),
+			oldText:     "test",
+			newText:     "done",
+			expected:    nil,
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := replaceEditContent(tt.content, tt.oldText, tt.newText)
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.expected, result)
+			}
+		})
+	}
+}
+
+// TestAppendFileTool_AppendToNonExistent_Restricted verifies that AppendFileTool in restricted mode
+// can append to a file that does not yet exist — it should silently create the file.
+// This exercises the errors.Is(err, fs.ErrNotExist) path in appendFileWithRW + rootRW.
+func TestAppendFileTool_AppendToNonExistent_Restricted(t *testing.T) {
+	workspace := t.TempDir()
+	tool := NewAppendFileTool(workspace, true)
+	ctx := context.Background()
+
+	args := map[string]interface{}{
+		"path":    "brand_new_file.txt",
+		"content": "first content",
+	}
+
+	result := tool.Execute(ctx, args)
+	assert.False(t, result.IsError, "Expected success when appending to non-existent file in restricted mode, got: %s", result.ForLLM)
+
+	// Verify the file was created with correct content
+	data, err := os.ReadFile(filepath.Join(workspace, "brand_new_file.txt"))
+	assert.NoError(t, err)
+	assert.Equal(t, "first content", string(data))
+}
+
+// TestAppendFileTool_Restricted_Success verifies that AppendFileTool in restricted mode
+// correctly appends to an existing file within the sandbox.
+func TestAppendFileTool_Restricted_Success(t *testing.T) {
+	workspace := t.TempDir()
+	testFile := "existing.txt"
+	err := os.WriteFile(filepath.Join(workspace, testFile), []byte("initial"), 0644)
+	assert.NoError(t, err)
+
+	tool := NewAppendFileTool(workspace, true)
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path":    testFile,
+		"content": " appended",
+	}
+
+	result := tool.Execute(ctx, args)
+	assert.False(t, result.IsError, "Expected success, got: %s", result.ForLLM)
+	assert.True(t, result.Silent)
+
+	data, err := os.ReadFile(filepath.Join(workspace, testFile))
+	assert.NoError(t, err)
+	assert.Equal(t, "initial appended", string(data))
+}
+
+// TestEditFileTool_Restricted_InPlaceEdit verifies that EditFileTool in restricted mode
+// correctly edits a file using the single-open editFileInRoot path.
+func TestEditFileTool_Restricted_InPlaceEdit(t *testing.T) {
+	workspace := t.TempDir()
+	testFile := "edit_target.txt"
+	err := os.WriteFile(filepath.Join(workspace, testFile), []byte("Hello World"), 0644)
+	assert.NoError(t, err)
+
+	tool := NewEditFileTool(workspace, true)
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path":     testFile,
+		"old_text": "World",
+		"new_text": "Go",
+	}
+
+	result := tool.Execute(ctx, args)
+	assert.False(t, result.IsError, "Expected success, got: %s", result.ForLLM)
+	assert.True(t, result.Silent)
+
+	data, err := os.ReadFile(filepath.Join(workspace, testFile))
+	assert.NoError(t, err)
+	assert.Equal(t, "Hello Go", string(data))
+}
+
+// TestEditFileTool_Restricted_FileNotFound verifies that editFileInRoot returns a proper
+// error message when the target file does not exist.
+func TestEditFileTool_Restricted_FileNotFound(t *testing.T) {
+	workspace := t.TempDir()
+	tool := NewEditFileTool(workspace, true)
+	ctx := context.Background()
+	args := map[string]interface{}{
+		"path":     "no_such_file.txt",
+		"old_text": "old",
+		"new_text": "new",
+	}
+
+	result := tool.Execute(ctx, args)
+	assert.True(t, result.IsError)
+	assert.Contains(t, result.ForLLM, "not found")
+}

pkg/tools/filesystem.go

@@ -3,10 +3,10 @@ package tools
 import (
 	"context"
 	"fmt"
-	"io"
 	"os"
 	"path/filepath"
 	"strings"
+	"time"
 )
 
 type ReadFileTool struct {
@@ -45,31 +45,21 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]interface{})
 		return ErrorResult("path is required")
 	}
 
-	// If restriction is disabled, fall back to standard os interactions (insecure but intended)
-	if !t.restrict {
-		content, err := os.ReadFile(path)
-		if err != nil {
-			return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
-		}
-		return NewToolResult(string(content))
+	if t.restrict {
+		return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+			content, err := (&rootRW{root: root}).Read(relPath)
+			if err != nil {
+				return nil, err
+			}
+			return NewToolResult(string(content)), nil
+		})
 	}
 
-	return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
-		f, err := root.Open(relPath)
-		if err != nil {
-			if os.IsNotExist(err) {
-				return nil, fmt.Errorf("failed to read file: file not found: %s", path)
-			}
-			return nil, fmt.Errorf("access denied or failed to open: %w", err)
-		}
-		defer f.Close()
-
-		content, err := io.ReadAll(f)
-		if err != nil {
-			return nil, fmt.Errorf("failed to read file: %v", err)
-		}
-		return NewToolResult(string(content)), nil
-	})
+	content, err := (&hostRW{}).Read(path)
+	if err != nil {
+		return ErrorResult(err.Error())
+	}
+	return NewToolResult(string(content))
 }
 
 type WriteFileTool struct {
@@ -117,37 +107,20 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]interface{}
 		return ErrorResult("content is required")
 	}
 
-	if !t.restrict {
-		if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
-			return ErrorResult(fmt.Sprintf("failed to create directory: %v", err))
-		}
-		if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-			return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
-		}
-		return SilentResult(fmt.Sprintf("File written: %s", path))
+	if t.restrict {
+		return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
+			if err := (&rootRW{root: root}).Write(relPath, []byte(content)); err != nil {
+				return nil, err
+			}
+			return SilentResult(fmt.Sprintf("File written: %s", path)), nil
+		})
 	}
 
-	return executeInRoot(t.workspace, path, func(root *os.Root, relPath string) (*ToolResult, error) {
-		// Ensure parent directory exists within root using recursive creation
-		dir := filepath.Dir(relPath)
-		if dir != "." && dir != "/" {
-			if err := mkdirAllInRoot(root, dir); err != nil {
-				return nil, fmt.Errorf("failed to create parent directories: %w", err)
-			}
-		}
+	if err := (&hostRW{}).Write(path, []byte(content)); err != nil {
+		return ErrorResult(err.Error())
+	}
 
-		f, err := root.Create(relPath)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create file: %w", err)
-		}
-		defer f.Close()
-
-		_, err = f.WriteString(content)
-		if err != nil {
-			return nil, fmt.Errorf("failed to write file: %w", err)
-		}
-		return SilentResult(fmt.Sprintf("File written: %s", path)), nil
-	})
+	return SilentResult(fmt.Sprintf("File written: %s", path))
 }
 
 type ListDirTool struct {
@@ -222,6 +195,102 @@ func formatDirEntries(entries []os.DirEntry) *ToolResult {
 	return NewToolResult(result.String())
 }
 
+// fileReadWriter abstracts reading and writing files, allowing both unrestricted
+// (host filesystem) and sandbox (os.Root) implementations to share the same logic.
+type fileReadWriter interface {
+	Read(path string) ([]byte, error)
+	Write(path string, data []byte) error
+}
+
+// hostRW is an unrestricted fileReadWriter that operates directly on the host filesystem.
+type hostRW struct{}
+
+func (h *hostRW) Read(path string) ([]byte, error) {
+	content, err := os.ReadFile(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil, fmt.Errorf("failed to read file: file not found: %w", err)
+		}
+		if os.IsPermission(err) {
+			return nil, fmt.Errorf("failed to read file: access denied: %w", err)
+		}
+		return nil, fmt.Errorf("failed to read file: %w", err)
+	}
+	return content, nil
+}
+
+func (h *hostRW) Write(path string, data []byte) error {
+	dir := filepath.Dir(path)
+	if err := os.MkdirAll(dir, 0755); err != nil {
+		return fmt.Errorf("failed to create parent directories: %w", err)
+	}
+
+	tmpPath := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano())
+	if err := os.WriteFile(tmpPath, data, 0644); err != nil {
+		return fmt.Errorf("failed to write temp file: %w", err)
+	}
+
+	if err := os.Rename(tmpPath, path); err != nil {
+		os.Remove(tmpPath)
+		return fmt.Errorf("failed to replace original file: %w", err)
+	}
+	return nil
+}
+
+// rootRW is a sandboxed fileReadWriter that operates within an os.Root boundary.
+// All paths passed to Read/Write must be relative to the root.
+type rootRW struct {
+	root *os.Root
+}
+
+func (r *rootRW) Read(path string) ([]byte, error) {
+	content, err := r.root.ReadFile(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil, fmt.Errorf("failed to read file: file not found: %w", err)
+		}
+		// os.Root returns "escapes from parent" for paths outside the root
+		if os.IsPermission(err) || strings.Contains(err.Error(), "escapes from parent") || strings.Contains(err.Error(), "permission denied") {
+			return nil, fmt.Errorf("failed to read file: access denied: %w", err)
+		}
+		return nil, fmt.Errorf("failed to read file: %w", err)
+	}
+	return content, nil
+}
+
+func (r *rootRW) Write(path string, data []byte) error {
+	dir := filepath.Dir(path)
+	if dir != "." && dir != "/" {
+		// Use native root.MkdirAll which handles the "file exists at path" check internally.
+		if err := r.root.MkdirAll(dir, 0755); err != nil {
+			return fmt.Errorf("failed to create parent directories: %w", err)
+		}
+	}
+
+	tmpRelPath := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano())
+	fw, err := r.root.Create(tmpRelPath)
+	if err != nil {
+		return fmt.Errorf("failed to create temp file for writing: %w", err)
+	}
+
+	if _, err := fw.Write(data); err != nil {
+		fw.Close()
+		r.root.Remove(tmpRelPath)
+		return fmt.Errorf("failed to write to temp file: %w", err)
+	}
+
+	if err := fw.Close(); err != nil {
+		r.root.Remove(tmpRelPath)
+		return fmt.Errorf("failed to close temp file: %w", err)
+	}
+
+	if err := r.root.Rename(tmpRelPath, path); err != nil {
+		r.root.Remove(tmpRelPath)
+		return fmt.Errorf("failed to rename temp file over target: %w", err)
+	}
+	return nil
+}
+
 // Helper to get a safe relative path for os.Root usage
 func getSafeRelPath(workspace, path string) (string, error) {
 	if workspace == "" {
@@ -240,7 +309,7 @@ func getSafeRelPath(workspace, path string) (string, error) {
 		path = rel
 	}
 
-	// Check for escape
+	// Check for escape manually (defense-in-depth, as os.Root also rejects paths that escape the root)
 	if path == ".." || strings.HasPrefix(path, "../") {
 		return "", fmt.Errorf("path escapes workspace: %s", path)
 	}
@@ -275,35 +344,3 @@ func executeInRoot(workspace string, path string, fn func(root *os.Root, relPath
 
 	return result
 }
-
-// mkdirAllInRoot mimics os.MkdirAll but within os.Root
-func mkdirAllInRoot(root *os.Root, relPath string) error {
-	relPath = filepath.Clean(relPath)
-	if relPath == "." || relPath == "/" {
-		return nil
-	}
-
-	dir := filepath.Dir(relPath)
-	if dir != "." && dir != "/" {
-		if err := mkdirAllInRoot(root, dir); err != nil {
-			return err
-		}
-	}
-
-	err := root.Mkdir(relPath, 0755)
-	if err != nil {
-		if os.IsExist(err) {
-			// Check if it's a directory
-			st, statErr := root.Stat(relPath)
-			if statErr != nil {
-				return statErr
-			}
-			if !st.IsDir() {
-				return fmt.Errorf("%s: not a directory", relPath)
-			}
-			return nil
-		}
-		return err
-	}
-	return nil
-}

pkg/tools/filesystem_test.go

@@ -2,6 +2,7 @@ package tools
 
 import (
 	"context"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -304,7 +305,9 @@ func TestFilesystemTool_EmptyWorkspace_AccessDenied(t *testing.T) {
 	assert.Contains(t, result.ForLLM, "workspace is not defined", "Expected 'workspace is not defined' error")
 }
 
-func TestMkdirAllInRoot(t *testing.T) {
+// TestRootMkdirAll verifies that root.MkdirAll (used by atomicWriteFileInRoot) handles all cases:
+// single dir, deeply nested dirs, already-existing dirs, and a file blocking a directory path.
+func TestRootMkdirAll(t *testing.T) {
 	workspace := t.TempDir()
 	root, err := os.OpenRoot(workspace)
 	if err != nil {
@@ -312,27 +315,27 @@ func TestMkdirAllInRoot(t *testing.T) {
 	}
 	defer root.Close()
 
-	// Test case 1: Single directory
-	err = mkdirAllInRoot(root, "dir1")
+	// Case 1: Single directory
+	err = root.MkdirAll("dir1", 0755)
 	assert.NoError(t, err)
 	_, err = os.Stat(filepath.Join(workspace, "dir1"))
 	assert.NoError(t, err)
 
-	// Test case 2: Deeply nested directory
-	err = mkdirAllInRoot(root, "a/b/c/d")
+	// Case 2: Deeply nested directory
+	err = root.MkdirAll("a/b/c/d", 0755)
 	assert.NoError(t, err)
 	_, err = os.Stat(filepath.Join(workspace, "a/b/c/d"))
 	assert.NoError(t, err)
 
-	// Test case 3: Already exists
-	err = mkdirAllInRoot(root, "a/b/c/d")
+	// Case 3: Already exists — must be idempotent
+	err = root.MkdirAll("a/b/c/d", 0755)
 	assert.NoError(t, err)
 
-	// Test case 4: File exists with same name (should fail)
+	// Case 4: A regular file blocks directory creation — must error
 	err = os.WriteFile(filepath.Join(workspace, "file_exists"), []byte("data"), 0644)
 	assert.NoError(t, err)
-	err = mkdirAllInRoot(root, "file_exists")
-	assert.Error(t, err)
+	err = root.MkdirAll("file_exists", 0755)
+	assert.Error(t, err, "expected error when a file exists at the directory path")
 }
 
 func TestFilesystemTool_WriteFile_Restricted_CreateDir(t *testing.T) {
@@ -356,3 +359,129 @@ func TestFilesystemTool_WriteFile_Restricted_CreateDir(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, content, string(data))
 }
+
+// TestHostRW_Read_PermissionDenied verifies that hostRW.Read surfaces access denied errors.
+func TestHostRW_Read_PermissionDenied(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("skipping permission test: running as root")
+	}
+	tmpDir := t.TempDir()
+	protected := filepath.Join(tmpDir, "protected.txt")
+	err := os.WriteFile(protected, []byte("secret"), 0000)
+	assert.NoError(t, err)
+	defer os.Chmod(protected, 0644) // ensure cleanup
+
+	_, err = (&hostRW{}).Read(protected)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "access denied")
+}
+
+// TestHostRW_Read_Directory verifies that hostRW.Read returns an error when given a directory path.
+func TestHostRW_Read_Directory(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	_, err := (&hostRW{}).Read(tmpDir)
+	assert.Error(t, err, "expected error when reading a directory as a file")
+}
+
+// TestRootRW_Read_Directory verifies that rootRW.Read returns an error when given a directory.
+func TestRootRW_Read_Directory(t *testing.T) {
+	workspace := t.TempDir()
+	root, err := os.OpenRoot(workspace)
+	assert.NoError(t, err)
+	defer root.Close()
+
+	// Create a subdirectory
+	err = root.Mkdir("subdir", 0755)
+	assert.NoError(t, err)
+
+	_, err = (&rootRW{root: root}).Read("subdir")
+	assert.Error(t, err, "expected error when reading a directory as a file")
+}
+
+// TestHostRW_Write_ParentDirMissing verifies that hostRW.Write creates parent dirs automatically.
+func TestHostRW_Write_ParentDirMissing(t *testing.T) {
+	tmpDir := t.TempDir()
+	target := filepath.Join(tmpDir, "a", "b", "c", "file.txt")
+
+	err := (&hostRW{}).Write(target, []byte("hello"))
+	assert.NoError(t, err)
+
+	data, err := os.ReadFile(target)
+	assert.NoError(t, err)
+	assert.Equal(t, "hello", string(data))
+}
+
+// TestRootRW_Write_ParentDirMissing verifies that rootRW.Write creates
+// nested parent directories automatically within the sandbox.
+func TestRootRW_Write_ParentDirMissing(t *testing.T) {
+	workspace := t.TempDir()
+	root, err := os.OpenRoot(workspace)
+	assert.NoError(t, err)
+	defer root.Close()
+
+	relPath := "x/y/z/file.txt"
+	err = (&rootRW{root: root}).Write(relPath, []byte("nested"))
+	assert.NoError(t, err)
+
+	data, err := os.ReadFile(filepath.Join(workspace, relPath))
+	assert.NoError(t, err)
+	assert.Equal(t, "nested", string(data))
+}
+
+// TestHostRW_Write verifies the hostRW.Write helper function
+func TestHostRW_Write(t *testing.T) {
+	tmpDir := t.TempDir()
+	testFile := filepath.Join(tmpDir, "atomic_test.txt")
+	testData := []byte("atomic test content")
+
+	err := (&hostRW{}).Write(testFile, testData)
+	assert.NoError(t, err)
+
+	content, err := os.ReadFile(testFile)
+	assert.NoError(t, err)
+	assert.Equal(t, testData, content)
+
+	// Verify it overwrites correctly
+	newData := []byte("new atomic content")
+	err = (&hostRW{}).Write(testFile, newData)
+	assert.NoError(t, err)
+
+	content, err = os.ReadFile(testFile)
+	assert.NoError(t, err)
+	assert.Equal(t, newData, content)
+}
+
+// TestRootRW_Write verifies the rootRW.Write helper function
+func TestRootRW_Write(t *testing.T) {
+	tmpDir := t.TempDir()
+	root, err := os.OpenRoot(tmpDir)
+	assert.NoError(t, err)
+	defer root.Close()
+
+	relPath := "atomic_root_test.txt"
+	testData := []byte("atomic root test content")
+
+	erw := &rootRW{root: root}
+	err = erw.Write(relPath, testData)
+	assert.NoError(t, err)
+
+	f, err := root.Open(relPath)
+	assert.NoError(t, err)
+	content, err := io.ReadAll(f)
+	assert.NoError(t, err)
+	f.Close()
+	assert.Equal(t, testData, content)
+
+	// Verify it overwrites correctly
+	newData := []byte("new root atomic content")
+	err = erw.Write(relPath, newData)
+	assert.NoError(t, err)
+
+	f, err = root.Open(relPath)
+	assert.NoError(t, err)
+	content, err = io.ReadAll(f)
+	assert.NoError(t, err)
+	f.Close()
+	assert.Equal(t, newData, content)
+}