refactor(tool): extract safety helpers into internal/toolsafety

internal/engine/search/url_scraper.go

@@ -13,7 +13,7 @@ import (
 	"sync"
 	"time"
 
-	"github.com/GrayCodeAI/rho/internal/tool"
+	"github.com/GrayCodeAI/rho/internal/toolsafety"
 )
 
 // URLScraper detects URLs in conversation text and fetches/extracts their content.
@@ -113,7 +113,7 @@ func (s *URLScraper) Fetch(ctx context.Context, rawURL string) (*ScrapeResult, e
 	// SSRF guard: reject private/link-local targets and pin the resolved IP
 	// to prevent DNS rebinding. Without this, a scraped URL could reach cloud
 	// metadata endpoints or internal services.
-	pinnedURL, origHost, err := tool.ValidateURLPublic(ctx, rawURL)
+	pinnedURL, origHost, err := toolsafety.ValidateURLPublic(ctx, rawURL)
 	if err != nil {
 		return nil, err
 	}
@@ -128,7 +128,7 @@ func (s *URLScraper) Fetch(ctx context.Context, rawURL string) (*ScrapeResult, e
 	req.Header.Set("User-Agent", s.UserAgent)
 	req.Header.Set("Accept", "text/html, application/json, text/plain, */*")
 
-	client := tool.SSRFSafeClient(ctx, s.Timeout)
+	client := toolsafety.SSRFSafeClient(ctx, s.Timeout)
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("fetching URL: %w", err)

internal/engine/search/url_scraper_test.go

@@ -10,7 +10,7 @@ import (
 	"time"
 
 	"github.com/GrayCodeAI/rho/internal/testutil"
-	"github.com/GrayCodeAI/rho/internal/tool"
+	"github.com/GrayCodeAI/rho/internal/toolsafety"
 )
 
 func TestDetectURLs_HTTPAndHTTPS(t *testing.T) {
@@ -330,7 +330,7 @@ func TestCachePreventsRefetch(t *testing.T) {
 	defer ts.Close()
 
 	scraper := NewURLScraper()
-	ctx := tool.WithSSRFSkip(context.Background())
+	ctx := toolsafety.WithSSRFSkip(context.Background())
 
 	// First fetch should hit server.
 	_, err := scraper.Fetch(ctx, ts.URL)
@@ -359,7 +359,7 @@ func TestFetch_HTMLContent(t *testing.T) {
 	defer ts.Close()
 
 	scraper := NewURLScraper()
-	result, err := scraper.Fetch(tool.WithSSRFSkip(context.Background()), ts.URL)
+	result, err := scraper.Fetch(toolsafety.WithSSRFSkip(context.Background()), ts.URL)
 	if err != nil {
 		t.Fatalf("fetch failed: %v", err)
 	}
@@ -386,7 +386,7 @@ func TestFetch_JSONContent(t *testing.T) {
 	defer ts.Close()
 
 	scraper := NewURLScraper()
-	result, err := scraper.Fetch(tool.WithSSRFSkip(context.Background()), ts.URL)
+	result, err := scraper.Fetch(toolsafety.WithSSRFSkip(context.Background()), ts.URL)
 	if err != nil {
 		t.Fatalf("fetch failed: %v", err)
 	}
@@ -494,7 +494,7 @@ func TestTokenEstimate(t *testing.T) {
 	defer ts.Close()
 
 	scraper := NewURLScraper()
-	result, err := scraper.Fetch(tool.WithSSRFSkip(context.Background()), ts.URL)
+	result, err := scraper.Fetch(toolsafety.WithSSRFSkip(context.Background()), ts.URL)
 	if err != nil {
 		t.Fatalf("fetch failed: %v", err)
 	}

internal/tool/bash.go

@@ -180,118 +180,6 @@ func (BashTool) Parameters() map[string]interface{} {
 // bashSchema is the single source of truth for Bash's input schema.
 var bashSchema = BashTool{}.Schema()
 
-// SegmentCommand splits a command string on &&, ||, ;, and | (respecting quotes
-// and heredocs) into individual segments for independent analysis.
-func SegmentCommand(cmd string) []string {
-	var segments []string
-	var current strings.Builder
-	inSingle, inDouble := false, false
-	inHeredoc := false
-	heredocDelim := ""
-	runes := []rune(cmd)
-	for i := 0; i < len(runes); i++ {
-		ch := runes[i]
-
-		// If inside a heredoc body, consume until we find the delimiter on its own line
-		if inHeredoc {
-			current.WriteRune(ch)
-			if ch == '\n' {
-				lineStart := i + 1
-				lineEnd := lineStart
-				for lineEnd < len(runes) && runes[lineEnd] != '\n' {
-					lineEnd++
-				}
-				line := strings.TrimSpace(string(runes[lineStart:lineEnd]))
-				if line == heredocDelim {
-					for j := lineStart; j <= lineEnd && j < len(runes); j++ {
-						current.WriteRune(runes[j])
-					}
-					i = lineEnd
-					inHeredoc = false
-					heredocDelim = ""
-				}
-			}
-			continue
-		}
-
-		if ch == '\'' && !inDouble {
-			inSingle = !inSingle
-			current.WriteRune(ch)
-			continue
-		}
-		if ch == '"' && !inSingle {
-			inDouble = !inDouble
-			current.WriteRune(ch)
-			continue
-		}
-		if inSingle || inDouble {
-			current.WriteRune(ch)
-			continue
-		}
-
-		// Detect heredoc: <<EOF, << EOF, <<-EOF
-		if ch == '<' && i+1 < len(runes) && runes[i+1] == '<' {
-			j := i + 2
-			if j < len(runes) && runes[j] == '-' {
-				j++
-			}
-			for j < len(runes) && runes[j] == ' ' {
-				j++
-			}
-			if j < len(runes) {
-				delimStart := j
-				delimQuote := rune(0)
-				if runes[j] == '\'' || runes[j] == '"' {
-					delimQuote = runes[j]
-					j++
-					delimStart = j
-					for j < len(runes) && runes[j] != delimQuote {
-						j++
-					}
-				} else {
-					for j < len(runes) && runes[j] != ' ' && runes[j] != '\n' && runes[j] != '<' && runes[j] != '>' && runes[j] != '|' && runes[j] != '&' && runes[j] != ';' {
-						j++
-					}
-				}
-				if j > delimStart {
-					heredocDelim = string(runes[delimStart:j])
-					inHeredoc = true
-					for k := i; k < j; k++ {
-						current.WriteRune(runes[k])
-					}
-					i = j - 1
-					continue
-				}
-			}
-		}
-
-		// Check for &&, ||
-		if i+1 < len(runes) && ((ch == '&' && runes[i+1] == '&') || (ch == '|' && runes[i+1] == '|')) {
-			if s := strings.TrimSpace(current.String()); s != "" {
-				segments = append(segments, s)
-			}
-			current.Reset()
-			i++ // skip second char
-			continue
-		}
-		// Check for ; or single |
-		if ch == ';' || ch == '|' {
-			if s := strings.TrimSpace(current.String()); s != "" {
-				segments = append(segments, s)
-			}
-			current.Reset()
-			continue
-		}
-		current.WriteRune(ch)
-	}
-	if s := strings.TrimSpace(current.String()); s != "" {
-		segments = append(segments, s)
-	}
-	return segments
-}
-
-// IsSuspicious returns true if the command needs a permission prompt.
-// This is fail-closed: anything we can't confidently classify as safe gets flagged.
 func IsSuspicious(command string) bool {
 	// Whole-command checks that apply regardless of segmentation
 	if strings.Contains(command, "\r") {
@@ -500,23 +388,23 @@ func (BashTool) Execute(ctx context.Context, input json.RawMessage) (string, err
 	if tc := GetToolContext(ctx); tc != nil && tc.WorkingDir != "" {
 		cmd.Dir = tc.WorkingDir
 	}
-	// Use a limitedWriter to cap output at maxOutputBytes instead of
+	// Use a limitedWriter to cap output at MaxOutputBytes instead of
 	// CombinedOutput, which buffers the entire output in memory. A command
 	// like `yes` or `cat /dev/urandom` can produce GBs before the timeout
 	// kills it; the limitedWriter keeps memory bounded while the command
 	// continues to run (writes are silently discarded after the cap).
 	var lw limitedWriter
-	// Cap one byte above maxOutputBytes so that TruncateOutput's > branch
-	// fires when the cap is reached. At exactly maxOutputBytes (no discard)
+	// Cap one byte above MaxOutputBytes so that TruncateOutput's > branch
+	// fires when the cap is reached. At exactly MaxOutputBytes (no discard)
 	// TruncateOutput returns unchanged, which is correct.
-	lw.maxBytes = maxOutputBytes + 1
+	lw.maxBytes = MaxOutputBytes + 1
 	cmd.Stdout = &lw
 	cmd.Stderr = &lw
 	err := cmd.Run()
 	result := lw.buf.String()
 
 	// Apply safety output truncation (50KB) — the limitedWriter may have
-	// captured up to maxOutputBytes (500KB), so we still truncate for the
+	// captured up to MaxOutputBytes (500KB), so we still truncate for the
 	// final result returned to the model.
 	result = TruncateOutput(result)
 	result = strings.TrimRight(result, "\n")

internal/tool/download.go

@@ -50,12 +50,12 @@ func (DownloadTool) Execute(ctx context.Context, input json.RawMessage) (string,
 	if reason := IsSensitivePath(p.Destination); reason != "" {
 		return "", fmt.Errorf("write blocked: %s", reason)
 	}
-	pinnedURL, origHost, err := validateURLPublic(ctx, p.URL)
+	pinnedURL, origHost, err := ValidateURLPublic(ctx, p.URL)
 	if err != nil {
 		return "", err
 	}
 
-	client := ssrfSafeClient(ctx, 2*time.Minute)
+	client := SSRFSafeClient(ctx, 2*time.Minute)
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, pinnedURL, nil)
 	if err != nil {
 		return "", fmt.Errorf("create request: %w", err)

internal/tool/safety_integration_test.go

@@ -0,0 +1,137 @@
+package tool
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// These tests exercise the file tools' use of the sensitive-path policy, so
+// they live in package tool (the policy itself is tested in toolsafety).
+
+func TestFileToolsBlockFluxProviderConfig(t *testing.T) {
+	fluxDir := filepath.Join(t.TempDir(), "flux")
+	t.Setenv("FLUX_CONFIG_DIR", fluxDir)
+	providerPath := filepath.Join(fluxDir, "provider.json")
+
+	readInput, _ := json.Marshal(map[string]string{"path": providerPath})
+	editInput, _ := json.Marshal(map[string]string{
+		"path": providerPath, "old_str": "old", "new_str": "new",
+	})
+	writeInput, _ := json.Marshal(map[string]string{
+		"path": providerPath, "content": "safe routing metadata",
+	})
+	tests := []struct {
+		name string
+		run  func() error
+	}{
+		{name: "Read", run: func() error {
+			_, err := (FileReadTool{}).Execute(testCtx(), readInput)
+			return err
+		}},
+		{name: "Edit", run: func() error {
+			_, err := (FileEditTool{}).Execute(testCtx(), editInput)
+			return err
+		}},
+		{name: "Write", run: func() error {
+			_, err := (FileWriteTool{}).Execute(testCtx(), writeInput)
+			return err
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.run()
+			if err == nil || !strings.Contains(err.Error(), "blocked") {
+				t.Fatalf("%s error = %v, want sensitive-path block", tt.name, err)
+			}
+		})
+	}
+}
+
+// TestFileRead_BlocksSymlinkToSensitiveFile verifies the read tool resolves
+// symlinks before opening (M13): reading through a symlink that points at a
+// sensitive target is blocked, while a symlink to an ordinary file works.
+func TestFileRead_BlocksSymlinkToSensitiveFile(t *testing.T) {
+	fluxDir := filepath.Join(t.TempDir(), "flux")
+	if err := os.MkdirAll(fluxDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("FLUX_CONFIG_DIR", fluxDir)
+	providerPath := filepath.Join(fluxDir, "provider.json")
+	if err := os.WriteFile(providerPath, []byte(`{"key":"x"}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	workDir := t.TempDir()
+	link := filepath.Join(workDir, "readme.md")
+	if err := os.Symlink(providerPath, link); err != nil {
+		t.Fatal(err)
+	}
+	in, _ := json.Marshal(map[string]string{"path": link})
+	_, err := (FileReadTool{}).Execute(testCtx(), in)
+	if err == nil || !strings.Contains(err.Error(), "blocked") {
+		t.Fatalf("expected sensitive-path block for symlinked provider config, got %v", err)
+	}
+
+	// A symlink to an ordinary file must still read fine.
+	plain := filepath.Join(workDir, "plain.txt")
+	if err := os.WriteFile(plain, []byte("hello"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	link2 := filepath.Join(workDir, "link2.txt")
+	if err := os.Symlink(plain, link2); err != nil {
+		t.Fatal(err)
+	}
+	in2, _ := json.Marshal(map[string]string{"path": link2})
+	out, err := (FileReadTool{}).Execute(testCtx(), in2)
+	if err != nil {
+		t.Fatalf("expected symlinked plain file to read, got %v", err)
+	}
+	if !strings.Contains(out, "hello") {
+		t.Fatalf("expected content through symlink, got %q", out)
+	}
+}
+
+// TestIsSensitivePath_SecretBasenames verifies the expanded basename blocklist
+// (secrets.txt, .git-credentials, private keys, …) applies anywhere, not just
+// under the home directory.
+
+func TestBashSensitivePathIntegration(t *testing.T) {
+	if !IsSuspicious("cat ~/.ssh/id_rsa") {
+		t.Error("IsSuspicious should flag reads of SSH private keys")
+	}
+	if !isHardDeny("cat ~/.aws/credentials") {
+		t.Error("isHardDeny should block credential reads when prompts are bypassed")
+	}
+	if isHardDeny("go build ./...") {
+		t.Error("isHardDeny should not block ordinary commands")
+	}
+}
+
+func TestCommandReferencesSensitivePath_FluxConfigDir(t *testing.T) {
+	fluxDir := filepath.Join(t.TempDir(), "flux config with spaces")
+	t.Setenv("FLUX_CONFIG_DIR", fluxDir)
+
+	commands := []string{
+		`cat "` + filepath.Join(fluxDir, "provider.json") + `"`,
+		`cat "$FLUX_CONFIG_DIR/provider.json"`,
+		`cat "${FLUX_CONFIG_DIR}/provider.json"`,
+		`cat "${FLUX_CONFIG_DIR%/}/provider.json"`,
+		`printf '%s\n' "$FLUX_CONFIG_DIR"`,
+		"cat " + strings.ReplaceAll(filepath.Join(fluxDir, "provider.json"), " ", `\ `),
+	}
+	for _, command := range commands {
+		if reason := CommandReferencesSensitivePath(command); reason == "" {
+			t.Fatalf("CommandReferencesSensitivePath(%q) = empty, want blocked", command)
+		}
+		if !IsSuspicious(command) {
+			t.Fatalf("IsSuspicious(%q) = false, want Bash prompt", command)
+		}
+		if !isHardDeny(command) {
+			t.Fatalf("isHardDeny(%q) = false, want Bash block", command)
+		}
+	}
+}