From 2108c57d682ee4a4877d391ab89cbad17b7c271e Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Sun, 1 Mar 2026 01:33:07 +0900 Subject: [PATCH] fix: include rule details in safety guard error messages guardCommand() and validatePath() now surface the specific deny pattern, allowlist patterns, or offending path token so the agent can self-correct. Co-Authored-By: Claude Opus 4.6 --- pkg/tools/filesystem.go | 2 +- pkg/tools/filesystem_test.go | 12 +++++++ pkg/tools/shell.go | 15 ++++++-- pkg/tools/shell_test.go | 68 ++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 4 deletions(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 79a5fe972..072512b36 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -36,7 +36,7 @@ func validatePath(path, workspace string, restrict bool) (string, error) { if restrict { if !isWithinWorkspace(absPath, absWorkspace) { - return "", fmt.Errorf("access denied: path is outside the workspace") + return "", fmt.Errorf("access denied: path outside workspace %s", absWorkspace) } var resolved string diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 9feac3e25..b3bc2affe 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -486,3 +486,15 @@ func TestSandboxFs_Write(t *testing.T) { assert.NoError(t, err) assert.Equal(t, newData, content) } + +// TestValidatePath_OutsideWorkspace_IncludesPath verifies that the access +// denied error includes the workspace path so the caller knows the boundary. +func TestValidatePath_OutsideWorkspace_IncludesPath(t *testing.T) { + workspace := t.TempDir() + outsidePath := filepath.Join(t.TempDir(), "secret.txt") + + _, err := validatePath(outsidePath, workspace, true) + assert.Error(t, err) + assert.Contains(t, err.Error(), "access denied") + assert.Contains(t, err.Error(), workspace) +} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index a2898092b..9f7091614 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -701,7 +701,7 @@ func (t *ExecTool) guardCommand(command, cwd string) string { for _, pattern := range t.denyPatterns { if pattern.MatchString(lower) { - return "Command blocked by safety guard (dangerous pattern detected)" + return fmt.Sprintf("Command blocked: deny pattern %s", pattern.String()) } } @@ -714,7 +714,16 @@ func (t *ExecTool) guardCommand(command, cwd string) string { } } if !allowed { - return "Command blocked by safety guard (not in allowlist)" + var b strings.Builder + b.WriteString("Command blocked: not in allowlist [") + for i, p := range t.allowPatterns { + if i > 0 { + b.WriteByte(',') + } + b.WriteString(p.String()) + } + b.WriteByte(']') + return b.String() } } @@ -764,7 +773,7 @@ func (t *ExecTool) guardCommand(command, cwd string) string { continue } } - return "Command blocked by safety guard (path outside working dir)" + return fmt.Sprintf("Command blocked: path outside working dir %s", p) } } } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index bc29fbf5c..6bf7e05a9 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -553,6 +553,74 @@ func TestGuardCommand_AgentCLISlashCommand(t *testing.T) { } } +// TestGuardCommand_DenyPattern_IncludesPattern verifies that deny-match +// error messages include the matched pattern string. +func TestGuardCommand_DenyPattern_IncludesPattern(t *testing.T) { + workspace := t.TempDir() + tool, _ := NewExecTool(workspace, true) + // Also add a custom deny pattern for precise matching. + tool.denyPatterns = append(tool.denyPatterns, regexp.MustCompile(`\bdangerous_cmd\b`)) + + result := tool.guardCommand("dangerous_cmd --force", workspace) + if result == "" { + t.Fatal("expected deny pattern to block the command") + } + if !strings.Contains(result, "deny pattern") { + t.Errorf("expected 'deny pattern' in message, got: %s", result) + } + if !strings.Contains(result, `\bdangerous_cmd\b`) { + t.Errorf("expected pattern string in message, got: %s", result) + } +} + +// TestGuardCommand_Allowlist_ShowsPatterns verifies that allowlist violation +// messages include all configured patterns. +func TestGuardCommand_Allowlist_ShowsPatterns(t *testing.T) { + workspace := t.TempDir() + tool, _ := NewExecTool(workspace, true) + err := tool.SetAllowPatterns([]string{`^go\b`, `^git\b`}) + if err != nil { + t.Fatalf("SetAllowPatterns failed: %v", err) + } + + result := tool.guardCommand("curl http://example.com", workspace) + if result == "" { + t.Fatal("expected allowlist to block the command") + } + if !strings.Contains(result, "not in allowlist") { + t.Errorf("expected 'not in allowlist' in message, got: %s", result) + } + if !strings.Contains(result, `^go\b`) || !strings.Contains(result, `^git\b`) { + t.Errorf("expected allowlist patterns in message, got: %s", result) + } +} + +// TestGuardCommand_PathOutside_IncludesPath verifies that workspace-escape +// messages include the offending path token. +func TestGuardCommand_PathOutside_IncludesPath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix absolute path test not applicable on Windows") + } + + workspace := t.TempDir() + externalDir := t.TempDir() + dataFile := filepath.Join(externalDir, "secret.txt") + os.WriteFile(dataFile, []byte("secret"), 0o644) + + tool, _ := NewExecTool(workspace, true) + + result := tool.guardCommand("cat "+dataFile, workspace) + if result == "" { + t.Fatal("expected path outside workspace to be blocked") + } + if !strings.Contains(result, "path outside working dir") { + t.Errorf("expected 'path outside working dir' in message, got: %s", result) + } + if !strings.Contains(result, dataFile) { + t.Errorf("expected offending path %q in message, got: %s", dataFile, result) + } +} + // --- Background process tests --- func TestExecTool_Bg_StartAndOutput(t *testing.T) {