diff --git a/server/pkg/agent/thinking.go b/server/pkg/agent/thinking.go index 537862a0f2..fba40d1645 100644 --- a/server/pkg/agent/thinking.go +++ b/server/pkg/agent/thinking.go @@ -166,8 +166,9 @@ func loadClaudeThinkingByModel(ctx context.Context, executablePath string) map[s } // claudeEffortSuperset returns the parsed `--effort` value list. When -// parsing fails it returns the static fallback rather than nothing so -// callers can still render a usable picker. +// the help output can't be captured at all it returns the static +// fallback rather than nothing so callers can still render a usable +// picker. func claudeEffortSuperset(ctx context.Context, executablePath string) []string { cmd := exec.CommandContext(ctx, executablePath, "--help") hideAgentWindow(cmd) @@ -175,14 +176,30 @@ func claudeEffortSuperset(ctx context.Context, executablePath string) []string { if err != nil { return append([]string(nil), claudeStaticEffortFallback...) } - parsed := parseClaudeEffortHelp(string(out)) - if len(parsed) == 0 { - // Help format drifted — fall back to the last known good - // superset rather than the conservative subset, so newer - // levels are still offered until we hand-edit the fallback. + return claudeEffortLevelsFromHelp(string(out)) +} + +// claudeEffortLevelsFromHelp decides the effort superset from a +// successfully captured `claude --help`. Three cases: +// - the value list parsed → use it verbatim; +// - `--effort` is advertised but the value list didn't parse → help +// format drifted; fall back to the last known good superset so +// newer levels are still offered until we hand-edit the fallback; +// - `--effort` is absent entirely → the installed CLI predates the +// flag. Return no levels: offering any would let the daemon pass +// ValidateThinkingLevel and inject --effort, which such a binary +// rejects with `error: unknown option '--effort'` — hard-failing +// every task for an agent with a persisted thinking_level instead +// of degrading to a plain run. +func claudeEffortLevelsFromHelp(helpText string) []string { + parsed := parseClaudeEffortHelp(helpText) + if len(parsed) > 0 { + return parsed + } + if strings.Contains(helpText, "--effort") { return append([]string(nil), claudeStaticEffortFullSuperset...) } - return parsed + return nil } // parseClaudeEffortHelp extracts the comma-separated value list from a diff --git a/server/pkg/agent/thinking_test.go b/server/pkg/agent/thinking_test.go index e9b1767b89..639645cbd6 100644 --- a/server/pkg/agent/thinking_test.go +++ b/server/pkg/agent/thinking_test.go @@ -58,6 +58,39 @@ Options: } } +func TestClaudeEffortLevelsFromHelp_DriftedFormatFallsBackToFullSuperset(t *testing.T) { + t.Parallel() + // The flag is advertised but the parenthesised value list is gone — + // genuine help drift, so keep offering the last known good superset. + help := `Usage: claude [options] + +Options: + --effort Choose how hard the model thinks +` + got := claudeEffortLevelsFromHelp(help) + if !reflect.DeepEqual(got, claudeStaticEffortFullSuperset) { + t.Fatalf("claudeEffortLevelsFromHelp: got %v, want full superset %v", got, claudeStaticEffortFullSuperset) + } +} + +func TestClaudeEffortLevelsFromHelp_PreEffortCLIReturnsNoLevels(t *testing.T) { + t.Parallel() + // A CLI released before --effort existed (e.g. claude 2.1.2) has no + // mention of the flag anywhere in --help. This must yield NO levels — + // the old fallback-to-full-superset here made the daemon inject + // --effort, which the binary rejects with "unknown option", failing + // the task outright. + help := `Usage: claude [options] + +Options: + --model Model to use + --verbose +` + if got := claudeEffortLevelsFromHelp(help); got != nil { + t.Fatalf("claudeEffortLevelsFromHelp: expected nil for pre-effort CLI, got %v", got) + } +} + func TestProjectClaudeLevels_PerModelSubset(t *testing.T) { t.Parallel() superset := []string{"low", "medium", "high", "xhigh", "max"} @@ -376,6 +409,42 @@ func TestValidateThinkingLevel_ExplicitModel(t *testing.T) { } } +func TestValidateThinkingLevel_PreEffortCLIRejectsAllLevels(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("shell-script fake binary requires a POSIX shell") + } + t.Parallel() + + // End-to-end guard for the daemon's pre-execution check against a CLI + // that predates --effort: the catalog must offer no levels, so any + // persisted thinking_level is dropped (with a warning) instead of being + // injected as a flag the binary rejects with "unknown option". + fakeClaude := writeFakeClaudePreEffortHelpBinary(t) + resetThinkingCacheForTests() + defer resetThinkingCacheForTests() + + ctx := context.Background() + + for _, level := range []string{"low", "medium", "high", "xhigh", "max"} { + ok, err := ValidateThinkingLevel(ctx, "claude", fakeClaude, "claude-fable-5", level) + if err != nil { + t.Fatalf("unexpected err for %q: %v", level, err) + } + if ok { + t.Errorf("level %q must be invalid on a pre-effort CLI; got true", level) + } + } + + // Empty value still means "use runtime default" and must stay valid. + ok, err := ValidateThinkingLevel(ctx, "claude", fakeClaude, "claude-fable-5", "") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if !ok { + t.Errorf("empty value must always be valid") + } +} + func TestValidateThinkingLevel_OpenCodeEmptyModelUsesAdvertisedVariants(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("shell-script fake binary requires a POSIX shell") @@ -451,6 +520,25 @@ func writeFakeClaudeHelpBinary(t *testing.T) string { return path } +// writeFakeClaudePreEffortHelpBinary mimics a Claude Code release from +// before the --effort flag existed (e.g. 2.1.2): --help succeeds but has +// no --effort line at all. +func writeFakeClaudePreEffortHelpBinary(t *testing.T) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "claude") + script := "#!/bin/sh\n" + + "cat <<'EOF'\n" + + "Usage: claude [options]\n" + + "\n" + + "Options:\n" + + " --model Model to use\n" + + " --verbose\n" + + "EOF\n" + writeTestExecutable(t, path, []byte(script)) + return path +} + // ── Cache key invalidation ─────────────────────────────────────────── func TestThinkingCacheKeyDistinct(t *testing.T) {