From 4d3f0a4e1da0bf5a0addbe6b46cfee51e2e8b3bb Mon Sep 17 00:00:00 2001 From: Zezae Oh Date: Tue, 7 Jul 2026 19:52:59 +0900 Subject: [PATCH] fix(providers): tolerate unknown Claude CLI deny rules (#1381) --- internal/providers/claude_cli.go | 23 +- internal/providers/claude_cli_chat.go | 319 ++++++++++-------- internal/providers/claude_cli_session.go | 46 ++- internal/providers/claude_cli_session_test.go | 46 +++ 4 files changed, 274 insertions(+), 160 deletions(-) diff --git a/internal/providers/claude_cli.go b/internal/providers/claude_cli.go index ac69beb9..5f54b2b6 100644 --- a/internal/providers/claude_cli.go +++ b/internal/providers/claude_cli.go @@ -51,17 +51,18 @@ const OptAllowedToolNames = "allowed_tool_names" // It acts as a thin proxy: CLI manages session history, tool execution, and context. // GoClaw only forwards the latest user message and streams back the response. type ClaudeCLIProvider struct { - name string // provider name (default: "claude-cli") - cliPath string // path to claude binary (default: "claude") - defaultModel string // default: "sonnet" - baseWorkDir string // base dir for agent workspaces - mcpConfigData *MCPConfigData // per-session MCP config data - permMode string // permission mode (default: "bypassPermissions") - hooksSettingsPath string // generated settings.json with security hooks (empty = no hooks) - hooksCleanup func() // cleanup function for hooks temp files - mu sync.Mutex // protects workdir creation - sessionMu sync.Map // key: string, value: *sync.Mutex — per-session lock - mcpConfigDirs sync.Map // key: string (dir path), value: struct{} — tracks per-session MCP config dirs for cleanup + name string // provider name (default: "claude-cli") + cliPath string // path to claude binary (default: "claude") + defaultModel string // default: "sonnet" + baseWorkDir string // base dir for agent workspaces + mcpConfigData *MCPConfigData // per-session MCP config data + permMode string // permission mode (default: "bypassPermissions") + hooksSettingsPath string // generated settings.json with security hooks (empty = no hooks) + hooksCleanup func() // cleanup function for hooks temp files + mu sync.Mutex // protects workdir creation + sessionMu sync.Map // key: string, value: *sync.Mutex — per-session lock + mcpConfigDirs sync.Map // key: string (dir path), value: struct{} — tracks per-session MCP config dirs for cleanup + invalidDisallowedToolNames sync.Map // key: string, value: struct{} — Claude CLI deny rules rejected by this CLI version } // ClaudeCLIOption configures the provider. diff --git a/internal/providers/claude_cli_chat.go b/internal/providers/claude_cli_chat.go index b1e74aaa..6af2c555 100644 --- a/internal/providers/claude_cli_chat.go +++ b/internal/providers/claude_cli_chat.go @@ -56,27 +56,40 @@ func (p *ClaudeCLIProvider) Chat(ctx context.Context, req ChatRequest) (*ChatRes args = append(args, "--", userMsg) } - cmd := exec.CommandContext(ctx, p.cliPath, args...) - cmd.Dir = workDir - cmd.Env = filterCLIEnv(os.Environ()) - if effortLevel != "" && effortLevel != "off" { - // Explicit --effort flag takes precedence; drop env to avoid ambiguity. - cmd.Env = removeEnvKey(cmd.Env, "CLAUDE_CODE_EFFORT_LEVEL") - } - if stdin != nil { - cmd.Stdin = stdin - } + for { + cmd := exec.CommandContext(ctx, p.cliPath, args...) + cmd.Dir = workDir + cmd.Env = filterCLIEnv(os.Environ()) + if effortLevel != "" && effortLevel != "off" { + // Explicit --effort flag takes precedence; drop env to avoid ambiguity. + cmd.Env = removeEnvKey(cmd.Env, "CLAUDE_CODE_EFFORT_LEVEL") + } + if stdin != nil { + cmd.Stdin = stdin + } - var stderr bytes.Buffer - cmd.Stderr = &stderr + var stderr bytes.Buffer + cmd.Stderr = &stderr - slog.Debug("claude-cli exec", "cmd", fmt.Sprintf("%s %s", p.cliPath, strings.Join(args, " ")), "workdir", workDir) - output, err := cmd.Output() - if err != nil { - return nil, fmt.Errorf("claude-cli: %w (stderr: %s)", err, stderr.String()) + slog.Debug("claude-cli exec", "cmd", fmt.Sprintf("%s %s", p.cliPath, strings.Join(args, " ")), "workdir", workDir) + output, err := cmd.Output() + if err != nil { + if tool, retry := p.noteInvalidDisallowedTool(stderr.String()); retry { + slog.Warn("claude-cli: retrying without unsupported --disallowedTools rule", "tool", tool) + args = p.buildArgs(model, workDir, mcpPath, cliSessionID, outputFmt, len(images) > 0, disableTools, effortLevel, allowedToolNames) + if len(images) == 0 { + args = append(args, "--", userMsg) + } + if len(images) > 0 { + stdin = buildStreamJSONInput(userMsg, images) + } + continue + } + return nil, fmt.Errorf("claude-cli: %w (stderr: %s)", err, stderr.String()) + } + + return parseJSONResponse(output) } - - return parseJSONResponse(output) } // ChatStream runs the CLI with stream-json output, calling onChunk for each text delta. @@ -119,149 +132,161 @@ func (p *ClaudeCLIProvider) ChatStream(ctx context.Context, req ChatRequest, onC args = append(args, "--", userMsg) } - cmd := exec.CommandContext(ctx, p.cliPath, args...) - cmd.WaitDelay = 5 * time.Second // force-close pipes if process lingers after kill - cmd.Dir = workDir - cmd.Env = filterCLIEnv(os.Environ()) - if effortLevel != "" && effortLevel != "off" { - cmd.Env = removeEnvKey(cmd.Env, "CLAUDE_CODE_EFFORT_LEVEL") - } - if stdin != nil { - cmd.Stdin = stdin - } - - var stderrBuf bytes.Buffer - cmd.Stderr = &stderrBuf - - stdout, err := cmd.StdoutPipe() - if err != nil { - return nil, fmt.Errorf("claude-cli stdout pipe: %w", err) - } - - fullCmd := fmt.Sprintf("%s %s", p.cliPath, strings.Join(args, " ")) - slog.Debug("claude-cli stream exec", "cmd", fullCmd, "workdir", workDir) - if err := cmd.Start(); err != nil { - return nil, fmt.Errorf("claude-cli start: %w", err) - } - - // Debug log file: only enabled when GOCLAW_DEBUG=1 - var debugFile *os.File - if os.Getenv("GOCLAW_DEBUG") == "1" { - debugLogPath := filepath.Join(workDir, "cli-debug.log") - debugFile, _ = os.OpenFile(debugLogPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) - if debugFile != nil { - fmt.Fprintf(debugFile, "=== CMD: %s\n=== WORKDIR: %s\n=== TIME: %s\n\n", fullCmd, workDir, time.Now().Format(time.RFC3339)) - defer debugFile.Close() + for { + cmd := exec.CommandContext(ctx, p.cliPath, args...) + cmd.WaitDelay = 5 * time.Second // force-close pipes if process lingers after kill + cmd.Dir = workDir + cmd.Env = filterCLIEnv(os.Environ()) + if effortLevel != "" && effortLevel != "off" { + cmd.Env = removeEnvKey(cmd.Env, "CLAUDE_CODE_EFFORT_LEVEL") } - } - - // Parse stream-json line-by-line - scanner := bufio.NewScanner(stdout) - scanner.Buffer(make([]byte, 0, StdioScanBufInit), StdioScanBufMax) - - var finalResp ChatResponse - var contentBuf strings.Builder - var streamErrMsg string // error message from stream-json result event - - for scanner.Scan() { - if ctx.Err() != nil { - break // context cancelled (abort) → exit immediately - } - line := scanner.Bytes() - if len(line) == 0 { - continue + if stdin != nil { + cmd.Stdin = stdin } - // Write raw line to debug log - if debugFile != nil { - fmt.Fprintf(debugFile, "%s\n", line) + var stderrBuf bytes.Buffer + cmd.Stderr = &stderrBuf + + stdout, err := cmd.StdoutPipe() + if err != nil { + return nil, fmt.Errorf("claude-cli stdout pipe: %w", err) } - var ev cliStreamEvent - if err := json.Unmarshal(line, &ev); err != nil { - slog.Debug("claude-cli: skip malformed stream line", "error", err) - continue + fullCmd := fmt.Sprintf("%s %s", p.cliPath, strings.Join(args, " ")) + slog.Debug("claude-cli stream exec", "cmd", fullCmd, "workdir", workDir) + if err := cmd.Start(); err != nil { + return nil, fmt.Errorf("claude-cli start: %w", err) } - switch ev.Type { - case "assistant": - if ev.Message == nil { + // Debug log file: only enabled when GOCLAW_DEBUG=1 + var debugFile *os.File + if os.Getenv("GOCLAW_DEBUG") == "1" { + debugLogPath := filepath.Join(workDir, "cli-debug.log") + debugFile, _ = os.OpenFile(debugLogPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) + if debugFile != nil { + fmt.Fprintf(debugFile, "=== CMD: %s\n=== WORKDIR: %s\n=== TIME: %s\n\n", fullCmd, workDir, time.Now().Format(time.RFC3339)) + defer debugFile.Close() + } + } + + // Parse stream-json line-by-line + scanner := bufio.NewScanner(stdout) + scanner.Buffer(make([]byte, 0, StdioScanBufInit), StdioScanBufMax) + + var finalResp ChatResponse + var contentBuf strings.Builder + var streamErrMsg string // error message from stream-json result event + + for scanner.Scan() { + if ctx.Err() != nil { + break // context cancelled (abort) → exit immediately + } + line := scanner.Bytes() + if len(line) == 0 { continue } - text, thinking := extractStreamContent(ev.Message) - if text != "" { - contentBuf.WriteString(text) - onChunk(StreamChunk{Content: text}) - } - if thinking != "" { - onChunk(StreamChunk{Thinking: thinking}) + + // Write raw line to debug log + if debugFile != nil { + fmt.Fprintf(debugFile, "%s\n", line) } - case "result": - if ev.Result != "" { - finalResp.Content = ev.Result - } else { - finalResp.Content = contentBuf.String() + var ev cliStreamEvent + if err := json.Unmarshal(line, &ev); err != nil { + slog.Debug("claude-cli: skip malformed stream line", "error", err) + continue } + + switch ev.Type { + case "assistant": + if ev.Message == nil { + continue + } + text, thinking := extractStreamContent(ev.Message) + if text != "" { + contentBuf.WriteString(text) + onChunk(StreamChunk{Content: text}) + } + if thinking != "" { + onChunk(StreamChunk{Thinking: thinking}) + } + + case "result": + if ev.Result != "" { + finalResp.Content = ev.Result + } else { + finalResp.Content = contentBuf.String() + } + finalResp.FinishReason = "stop" + if ev.Subtype == "error" || ev.IsError { + finalResp.FinishReason = "error" + // Prefer ev.Error (result may be empty for usage/rate limit errors). + if ev.Error != "" { + streamErrMsg = ev.Error + } else if ev.Result != "" { + streamErrMsg = ev.Result + } + } + if ev.Usage != nil { + finalResp.Usage = &Usage{ + PromptTokens: ev.Usage.InputTokens, + CompletionTokens: ev.Usage.OutputTokens, + TotalTokens: ev.Usage.InputTokens + ev.Usage.OutputTokens, + } + } + } + } + + // Context cancelled (abort): best-effort reap (bounded by WaitDelay), then return. + if ctx.Err() != nil { + _ = cmd.Wait() + return nil, ctx.Err() + } + + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("claude-cli: stream read error: %w", err) + } + + if err := cmd.Wait(); err != nil { + if debugFile != nil { + fmt.Fprintf(debugFile, "\n=== STDERR:\n%s\n=== EXIT ERROR: %v\n", stderrBuf.String(), err) + } + // If we got partial content, return it with the error. + if finalResp.Content != "" { + return &finalResp, nil + } + stderrStr := strings.TrimSpace(stderrBuf.String()) + if tool, retry := p.noteInvalidDisallowedTool(stderrStr); retry { + slog.Warn("claude-cli: retrying without unsupported --disallowedTools rule", "tool", tool) + args = p.buildArgs(model, workDir, mcpPath, cliSessionID, "stream-json", len(images) > 0, disableTools, effortLevel, allowedToolNames) + if len(images) == 0 { + args = append(args, "--", userMsg) + } else { + stdin = buildStreamJSONInput(userMsg, images) + } + continue + } + if stderrStr == "" && finalResp.FinishReason == "error" { + // claude-cli communicates API errors via stream-json stdout, not stderr. + if streamErrMsg != "" { + stderrStr = "stream: " + streamErrMsg + } else { + stderrStr = "stream error (no message)" + } + } + return nil, fmt.Errorf("claude-cli: %w (stderr: %s)", err, stderrStr) + } + if debugFile != nil && stderrBuf.Len() > 0 { + fmt.Fprintf(debugFile, "\n=== STDERR:\n%s\n", stderrBuf.String()) + } + + // Fallback if no "result" event was received + if finalResp.Content == "" { + finalResp.Content = contentBuf.String() finalResp.FinishReason = "stop" - if ev.Subtype == "error" || ev.IsError { - finalResp.FinishReason = "error" - // Prefer ev.Error (result may be empty for usage/rate limit errors). - if ev.Error != "" { - streamErrMsg = ev.Error - } else if ev.Result != "" { - streamErrMsg = ev.Result - } - } - if ev.Usage != nil { - finalResp.Usage = &Usage{ - PromptTokens: ev.Usage.InputTokens, - CompletionTokens: ev.Usage.OutputTokens, - TotalTokens: ev.Usage.InputTokens + ev.Usage.OutputTokens, - } - } } - } - // Context cancelled (abort): best-effort reap (bounded by WaitDelay), then return. - if ctx.Err() != nil { - _ = cmd.Wait() - return nil, ctx.Err() + onChunk(StreamChunk{Done: true}) + return &finalResp, nil } - - if err := scanner.Err(); err != nil { - return nil, fmt.Errorf("claude-cli: stream read error: %w", err) - } - - if err := cmd.Wait(); err != nil { - if debugFile != nil { - fmt.Fprintf(debugFile, "\n=== STDERR:\n%s\n=== EXIT ERROR: %v\n", stderrBuf.String(), err) - } - // If we got partial content, return it with the error - if finalResp.Content != "" { - return &finalResp, nil - } - stderrStr := strings.TrimSpace(stderrBuf.String()) - if stderrStr == "" && finalResp.FinishReason == "error" { - // claude-cli communicates API errors via stream-json stdout, not stderr. - if streamErrMsg != "" { - stderrStr = "stream: " + streamErrMsg - } else { - stderrStr = "stream error (no message)" - } - } - return nil, fmt.Errorf("claude-cli: %w (stderr: %s)", err, stderrStr) - } - if debugFile != nil && stderrBuf.Len() > 0 { - fmt.Fprintf(debugFile, "\n=== STDERR:\n%s\n", stderrBuf.String()) - } - - // Fallback if no "result" event was received - if finalResp.Content == "" { - finalResp.Content = contentBuf.String() - finalResp.FinishReason = "stop" - } - - onChunk(StreamChunk{Done: true}) - return &finalResp, nil } diff --git a/internal/providers/claude_cli_session.go b/internal/providers/claude_cli_session.go index 7b6f7e2c..488ebdf1 100644 --- a/internal/providers/claude_cli_session.go +++ b/internal/providers/claude_cli_session.go @@ -8,6 +8,7 @@ import ( "log/slog" "os" "path/filepath" + "regexp" "slices" "strings" @@ -41,6 +42,8 @@ var cliNativeToolsAlwaysBlocked = []string{ "Glob", "Grep", "TodoWrite", "NotebookEdit", } +var unknownDisallowedToolRe = regexp.MustCompile(`Permission deny rule "([^"]+)" matches no known tool`) + // disallowedCLITools computes the --disallowedTools value for the Claude CLI // subprocess from the agent's policy-filtered allowed GoClaw tool names. // allowedToolNames == nil is treated as "no tools allowed" (fail closed): @@ -62,6 +65,42 @@ func disallowedCLITools(allowedToolNames []string) []string { return blocked } +// filterInvalidDisallowedTools removes deny rules that the local Claude CLI has +// already rejected as unknown. Claude Code occasionally changes native tool +// names; retrying without rejected names keeps the provider usable while still +// denying every rule the installed CLI recognizes. +func (p *ClaudeCLIProvider) filterInvalidDisallowedTools(tools []string) []string { + if len(tools) == 0 { + return tools + } + filtered := tools[:0] + for _, tool := range tools { + if _, invalid := p.invalidDisallowedToolNames.Load(tool); invalid { + continue + } + filtered = append(filtered, tool) + } + return filtered +} + +// noteInvalidDisallowedTool records a Claude CLI deny rule rejected by the +// installed CLI. It returns true only the first time the tool is observed so +// callers can bound retries. +func (p *ClaudeCLIProvider) noteInvalidDisallowedTool(stderr string) (string, bool) { + match := unknownDisallowedToolRe.FindStringSubmatch(stderr) + if len(match) != 2 { + return "", false + } + tool := match[1] + if tool == "" { + return "", false + } + if _, loaded := p.invalidDisallowedToolNames.LoadOrStore(tool, struct{}{}); loaded { + return tool, false + } + return tool, true +} + // validCLIModels lists accepted model aliases for the Claude CLI. var validCLIModels = map[string]bool{ "sonnet": true, "opus": true, "haiku": true, @@ -124,11 +163,14 @@ func (p *ClaudeCLIProvider) buildArgs(model, workDir, mcpConfigPath string, cliS // This must never be skipped — omitting it previously let the CLI // subprocess run with its full native toolset unrestricted whenever no MCP // config was resolved. + var blockedTools []string if disableTools { - args = append(args, "--disallowedTools", strings.Join(disallowedCLITools(nil), ",")) + blockedTools = disallowedCLITools(nil) } else { - args = append(args, "--disallowedTools", strings.Join(disallowedCLITools(allowedToolNames), ",")) + blockedTools = disallowedCLITools(allowedToolNames) } + blockedTools = p.filterInvalidDisallowedTools(blockedTools) + args = append(args, "--disallowedTools", strings.Join(blockedTools, ",")) if p.hooksSettingsPath != "" { args = append(args, "--settings", p.hooksSettingsPath) diff --git a/internal/providers/claude_cli_session_test.go b/internal/providers/claude_cli_session_test.go index 2c5361a9..0aba0ec2 100644 --- a/internal/providers/claude_cli_session_test.go +++ b/internal/providers/claude_cli_session_test.go @@ -149,3 +149,49 @@ func TestDisallowedCLITools_NoStaleToolNames(t *testing.T) { } } } + +func TestClaudeCLIRemembersUnknownDisallowedToolRules(t *testing.T) { + p := NewClaudeCLIProvider("claude") + + if _, retry := p.noteInvalidDisallowedTool(`Permission deny rule "NotebookRead" matches no known tool — check for typos.`); !retry { + t.Fatalf("first unknown tool observation should request retry") + } + if _, retry := p.noteInvalidDisallowedTool(`Permission deny rule "NotebookRead" matches no known tool — check for typos.`); retry { + t.Fatalf("repeated unknown tool observation should not request another retry") + } + + filtered := p.filterInvalidDisallowedTools([]string{"Bash", "NotebookRead", "TodoWrite"}) + want := []string{"Bash", "TodoWrite"} + if len(filtered) != len(want) { + t.Fatalf("filtered tools = %v, want %v", filtered, want) + } + for i := range want { + if filtered[i] != want[i] { + t.Fatalf("filtered tools = %v, want %v", filtered, want) + } + } +} + +func TestClaudeCLIDisallowedToolsExcludeRemovedReadOnlyTools(t *testing.T) { + blocked := disallowedCLITools(nil) + for _, removed := range []string{"NotebookRead", "TodoRead"} { + for _, got := range blocked { + if got == removed { + t.Fatalf("disallowedCLITools included removed Claude CLI tool %q in %v", removed, blocked) + } + } + } + + for _, want := range []string{"NotebookEdit", "TodoWrite"} { + found := false + for _, got := range blocked { + if got == want { + found = true + break + } + } + if !found { + t.Fatalf("disallowedCLITools missing still-supported tool %q in %v", want, blocked) + } + } +}