diff --git a/cmd/task/cli_test.go b/cmd/task/cli_test.go index 0405eae6..a6f4f396 100644 --- a/cmd/task/cli_test.go +++ b/cmd/task/cli_test.go @@ -1833,3 +1833,95 @@ func TestShouldSubmitInput(t *testing.T) { }) } } + +// TestCreateModelBackendEscapeHatch covers the escape hatch that keeps +// `ty create --model` validation from breaking proxy-routed setups: when a +// project's Claude is pointed at ollama (or any other non-Anthropic backend), +// the model names are the proxy's and must not be checked against Anthropic's. +func TestCreateModelBackendEscapeHatch(t *testing.T) { + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "test.db") + + database, err := db.Open(dbPath) + if err != nil { + t.Fatalf("failed to open database: %v", err) + } + defer database.Close() + + if err := database.CreateProject(&db.Project{Name: "stock", Path: filepath.Join(tmpDir, "stock")}); err != nil { + t.Fatalf("create stock project: %v", err) + } + if err := database.CreateProject(&db.Project{ + Name: "ollama", + Path: filepath.Join(tmpDir, "ollama"), + ClaudeConfigDir: "~/.claude-ollama", + }); err != nil { + t.Fatalf("create ollama project: %v", err) + } + + t.Setenv("ANTHROPIC_BASE_URL", "") + + // This is the shape `ty create` builds to validate the --model flag. + probe := func(project, model string) error { + return database.ValidateTaskModel(&db.Task{Project: project, Model: model}) + } + + if err := probe("stock", "glm-5.2:cloud"); err == nil { + t.Error("a proxy-only model on a stock-backend project should be rejected") + } + if err := probe("ollama", "glm-5.2:cloud"); err != nil { + t.Errorf("a project with a CLAUDE_CONFIG_DIR override should skip validation: %v", err) + } + if err := probe("does-not-exist", "opuss"); err == nil { + t.Error("an unknown project must not be treated as a custom backend") + } + if err := probe("", "opuss"); err == nil { + t.Error("no project means no override, so validation still applies") + } + + // An ambient ANTHROPIC_BASE_URL routes every project at a proxy. + t.Setenv("ANTHROPIC_BASE_URL", "http://127.0.0.1:11434") + if err := probe("stock", "glm-5.2:cloud"); err != nil { + t.Errorf("ANTHROPIC_BASE_URL in the environment should skip validation: %v", err) + } +} + +// TestCreateModelValidation pins the rules `ty create --model` enforces: reject +// what the agent's CLI would reject, accept model IDs newer than this code, and +// never reject anything the UI picker offers. +func TestCreateModelValidation(t *testing.T) { + rejected := []struct { + executor string + model string + }{ + {db.ExecutorClaude, "opuss"}, // typo + {db.ExecutorClaude, "claude"}, // the executor slug, not a model + {db.ExecutorClaude, "gpt-5"}, // another vendor + {db.ExecutorGrok, "opus"}, // claude alias on grok + {db.ExecutorCodex, "gpt-5-codex"}, // executor has no --model flag + {"", "sonnet-4"}, // default executor, bad alias + } + for _, tt := range rejected { + if err := db.ValidateModel(tt.executor, tt.model); err == nil { + t.Errorf("--model %q with executor %q should be rejected", tt.model, tt.executor) + } + } + + accepted := []struct { + executor string + model string + }{ + {db.ExecutorClaude, ""}, // no override + {"", ""}, + {db.ExecutorClaude, "opus"}, + {db.ExecutorClaude, "claude-opus-5"}, + {db.ExecutorClaude, "claude-opus-9"}, // released after this code + {db.ExecutorGrok, "grok-4"}, + {db.ExecutorCodex, ""}, // modelless executor, no override + } + for _, tt := range accepted { + if err := db.ValidateModel(tt.executor, tt.model); err != nil { + t.Errorf("--model %q with executor %q should be accepted: %v", tt.model, tt.executor, err) + } + } +} diff --git a/cmd/task/main.go b/cmd/task/main.go index 3f853ddb..8ae1c9c9 100644 --- a/cmd/task/main.go +++ b/cmd/task/main.go @@ -775,8 +775,7 @@ Examples: os.Exit(1) } - // Normalize model override (empty = use Claude's global default). Any - // non-empty value is accepted; the Claude CLI validates the model name. + // Normalize model override (empty = use the agent's own default). modelOverride = strings.TrimSpace(modelOverride) // If project not specified, try to detect from cwd @@ -788,6 +787,20 @@ Examples: } } + // Reject a model the executor's CLI would not accept. A bad override is + // otherwise invisible: the task launches, the agent rejects the flag + // inside tmux, and the card sits there looking busy. ValidateTaskModel + // skips the check when this task would be routed at a proxy (ollama and + // friends), where the model names belong to the proxy, not Anthropic. + if err := database.ValidateTaskModel(&db.Task{ + Project: project, + Executor: taskExecutor, + Model: modelOverride, + }); err != nil { + fmt.Fprintln(os.Stderr, errorStyle.Render("Error: "+err.Error())) + os.Exit(1) + } + // Generate title from body if title is empty if strings.TrimSpace(title) == "" && strings.TrimSpace(body) != "" { var apiKey string @@ -889,7 +902,7 @@ Examples: createCmd.Flags().StringP("project", "p", "", "Project name (auto-detected from cwd if not specified)") createCmd.Flags().StringP("executor", "e", "", "Task executor: claude, codex, gemini, grok, pi, opencode, openclaw (default: claude)") createCmd.Flags().String("effort", "", "Per-task Claude effort override: low, medium, high, xhigh, max (default: Claude's global default)") - createCmd.Flags().String("model", "", "Per-task Claude model override: opus, sonnet, haiku, or a full model name (default: Claude's global default)") + createCmd.Flags().String("model", "", "Per-task model override: opus, sonnet, haiku, fable, or a full model ID like claude-opus-5 (default: the agent's own default). Claude and grok only") createCmd.Flags().BoolP("execute", "x", false, "Queue task for immediate execution") createCmd.Flags().Bool("dangerous", false, "Execute in dangerous mode (alias for --permission-mode dangerous)") createCmd.Flags().String("permission-mode", "", "Permission mode: default (prompt), accept-edits (auto-accept file edits), auto (Claude Code auto mode: auto-approve safe actions, block risky ones), dangerous (skip all). Defaults to the project's setting") @@ -905,7 +918,10 @@ Examples: return db.EffortLevels(), cobra.ShellCompDirectiveNoFileComp }) createCmd.RegisterFlagCompletionFunc("model", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - return db.ModelOptions(), cobra.ShellCompDirectiveNoFileComp + // Complete against the models the chosen executor actually accepts, so + // `-e grok --model ` doesn't offer Claude aliases. + executor, _ := cmd.Flags().GetString("executor") + return db.ModelsForExecutor(executor), cobra.ShellCompDirectiveNoFileComp }) rootCmd.AddCommand(createCmd) diff --git a/internal/db/models.go b/internal/db/models.go new file mode 100644 index 00000000..63e69aa6 --- /dev/null +++ b/internal/db/models.go @@ -0,0 +1,198 @@ +package db + +import ( + "fmt" + "os" + "regexp" + "strings" +) + +// Per-task model overrides pick which model an executor's CLI runs (its +// `--model` flag). An empty value means "no override": the task uses the +// agent's own default, leaving the user's global setting untouched. +// +// Overrides are validated before they are stored because a bad one is +// invisible: the task launches, the agent CLI rejects the flag inside tmux, +// and the card just sits there looking busy. An early default baked the +// executor slug "claude" into every row (see the clear-model repair in +// sqlite.go) — exactly that failure. +const ( + ModelFable = "fable" + ModelOpus = "opus" + ModelSonnet = "sonnet" + ModelHaiku = "haiku" + // ModelOpusPlan is Claude Code's planning alias (Opus to plan, Sonnet to + // execute). Accepted as an override but kept out of the UI picker. + ModelOpusPlan = "opusplan" +) + +// executorModelPrefix maps an executor to the ID prefix its models carry. Only +// executors listed here pass a --model flag to their CLI at all; for the rest +// (codex, gemini, pi, opencode, openclaw) a model override is dead config — +// the launch command never mentions it. +var executorModelPrefix = map[string]string{ + ExecutorClaude: "claude-", + ExecutorGrok: "grok-", +} + +// executorModelAliases lists the short names an executor's CLI accepts in place +// of a full model ID. Full IDs are matched by prefix + shape instead (see +// ValidateModel), so a newly released model needs no code change here; a new +// *alias* does — that's how "fable" arrived alongside opus/sonnet/haiku. +var executorModelAliases = map[string][]string{ + ExecutorClaude: {ModelOpus, ModelSonnet, ModelHaiku, ModelFable, ModelOpusPlan}, + // The grok CLI takes full IDs only; these are examples for help text and + // shell completion, not a closed set (any grok-* ID is accepted). + ExecutorGrok: {"grok-4", "grok-4-fast", "grok-code-fast-1"}, +} + +// modelIDShape matches the shape of a vendor model ID: dash-separated +// lowercase alphanumeric segments, with an optional bracketed variant suffix +// (Claude Code's 1M-context form, e.g. "claude-opus-5[1m]"). It deliberately +// says nothing about which models exist — the vendor prefix does that — so +// "claude-opus-9" passes and "claude opus" or "claude;rm -rf" does not. +var modelIDShape = regexp.MustCompile(`^[a-z0-9]+(\.[a-z0-9]+)*(-[a-z0-9]+(\.[a-z0-9]+)*)*(\[[a-z0-9]+\])?$`) + +// ModelOptions returns the per-task model override aliases offered in the UI +// picker. It is a curated shortlist, not the set ValidateModel accepts: full +// model IDs and opusplan are valid overrides but not picker entries. +func ModelOptions() []string { + return []string{ModelOpus, ModelSonnet, ModelHaiku, ModelFable} +} + +// ExecutorSupportsModel reports whether the executor's CLI takes a --model +// flag. An empty executor means the default (claude). +func ExecutorSupportsModel(executor string) bool { + _, ok := executorModelPrefix[resolveExecutor(executor)] + return ok +} + +// ModelCapableExecutors returns the executors that accept a model override, in +// KnownExecutors display order. +func ModelCapableExecutors() []string { + var out []string + for _, e := range KnownExecutors() { + if ExecutorSupportsModel(e) { + out = append(out, e) + } + } + return out +} + +// ModelsForExecutor returns the model names ty knows about for an executor, in +// display order — used for `ty create --model` shell completion and error +// messages. It is not exhaustive: any ID carrying the executor's vendor prefix +// is also accepted. Nil when the executor has no --model flag. +func ModelsForExecutor(executor string) []string { + aliases := executorModelAliases[resolveExecutor(executor)] + return append([]string(nil), aliases...) +} + +// ValidateModel reports whether model is a model the executor's CLI would +// actually accept. The empty string is always valid and means "no override". +// +// A model is accepted when it is one of the executor's aliases (optionally with +// a "[1m]" variant suffix) or a well-formed ID carrying the executor's vendor +// prefix — "claude-opus-5" and "grok-4-fast" pass without ty having to track +// every release. Everything else is rejected, which catches the two mistakes +// that actually happen: a typo ("opuss") and a model belonging to some other +// vendor ("gpt-5", or the "claude" executor slug). +// +// Callers must skip this check when the agent is routed at a non-stock backend +// — a CLAUDE_CONFIG_DIR override or ANTHROPIC_BASE_URL pointing at a proxy like +// ollama — because the model names are then the proxy's (e.g. "glm-5.2:cloud") +// and ty has no way to know them. See ModelBackendIsCustom. +func ValidateModel(executor, model string) error { + model = strings.TrimSpace(model) + if model == "" { + return nil + } + executor = resolveExecutor(executor) + if !ExecutorSupportsModel(executor) { + return fmt.Errorf("the %s executor has no --model flag, so %q would be silently ignored (model overrides work with: %s)", + executor, model, strings.Join(ModelCapableExecutors(), ", ")) + } + lower := strings.ToLower(model) + for _, alias := range executorModelAliases[executor] { + if lower == alias || strings.HasPrefix(lower, alias+"[") && modelIDShape.MatchString(lower) { + return nil + } + } + prefix := executorModelPrefix[executor] + if strings.HasPrefix(lower, prefix) && modelIDShape.MatchString(lower) { + return nil + } + return fmt.Errorf("unknown %s model %q — known models: %s (or any %s* model ID)", + executor, model, strings.Join(ModelsForExecutor(executor), ", "), prefix) +} + +// ValidateTaskModel checks a task's model override against the executor that +// will run it, skipping the check when the task is routed at a non-stock +// backend. This is the entry point every write path should use — it resolves +// the escape hatch (per-task config dir or env, the project's config dir, an +// ambient ANTHROPIC_BASE_URL) that a bare ValidateModel call cannot see. +func (db *DB) ValidateTaskModel(t *Task) error { + if t == nil || strings.TrimSpace(t.Model) == "" { + return nil + } + if db.taskModelBackendIsCustom(t) { + return nil + } + return ValidateModel(t.Executor, t.Model) +} + +// taskModelBackendIsCustom reports whether anything in a task's resolved +// configuration points Claude away from Anthropic's API: the task's own config +// dir or env, the project's config dir, or ANTHROPIC_BASE_URL in the +// environment ty itself is running in. +func (db *DB) taskModelBackendIsCustom(t *Task) bool { + if ModelBackendIsCustom(t.ClaudeConfigDir, t.EnvMap()) { + return true + } + if strings.TrimSpace(os.Getenv("ANTHROPIC_BASE_URL")) != "" { + return true + } + if t.Project == "" { + return false + } + p, err := db.GetProjectByName(t.Project) + if err != nil || p == nil { + return false + } + return ModelBackendIsCustom(p.ClaudeConfigDir, nil) +} + +// ModelBackendIsCustom reports whether a Claude run is pointed at something +// other than Anthropic's API — a CLAUDE_CONFIG_DIR override or an +// ANTHROPIC_BASE_URL env override (the two ways ty routes a task through a +// proxy such as ollama). Model names are the proxy's there, so overrides must +// not be validated against Anthropic's. +func ModelBackendIsCustom(configDir string, env map[string]string) bool { + if strings.TrimSpace(configDir) != "" { + return true + } + return strings.TrimSpace(env["ANTHROPIC_BASE_URL"]) != "" +} + +// IsValidModel reports whether s is an acceptable per-task model override for +// *some* executor. The empty string is valid and means "use the agent default". +// Prefer ValidateModel when the executor is known; this looser form exists for +// callers holding a remembered value with no executor in hand (the new-task +// form's per-project default). +func IsValidModel(s string) bool { + for _, e := range KnownExecutors() { + if ValidateModel(e, s) == nil { + return true + } + } + return false +} + +// resolveExecutor normalizes an executor slug, mapping "" to the default. +func resolveExecutor(executor string) string { + executor = strings.ToLower(strings.TrimSpace(executor)) + if executor == "" { + return DefaultExecutor() + } + return executor +} diff --git a/internal/db/models_test.go b/internal/db/models_test.go new file mode 100644 index 00000000..3b651988 --- /dev/null +++ b/internal/db/models_test.go @@ -0,0 +1,281 @@ +package db + +import ( + "path/filepath" + "strings" + "testing" +) + +// TestValidateModel covers the whole point of the check: a model the executor's +// CLI would actually reject must not be storable, while anything it accepts — +// including model IDs released after this code was written — must pass. +func TestValidateModel(t *testing.T) { + tests := []struct { + name string + executor string + model string + wantErr bool + }{ + // No override is always fine — it means "use the agent's own default". + {"empty is no override", ExecutorClaude, "", false}, + {"empty with no executor", "", "", false}, + + // Claude aliases. + {"opus", ExecutorClaude, ModelOpus, false}, + {"sonnet", ExecutorClaude, ModelSonnet, false}, + {"haiku", ExecutorClaude, ModelHaiku, false}, + {"fable", ExecutorClaude, ModelFable, false}, + {"opusplan", ExecutorClaude, ModelOpusPlan, false}, + {"alias with 1m variant", ExecutorClaude, "sonnet[1m]", false}, + {"alias is case-insensitive", ExecutorClaude, "Opus", false}, + {"empty executor defaults to claude", "", ModelOpus, false}, + + // Full Claude IDs, including ones newer than this code. + {"full id", ExecutorClaude, "claude-opus-5", false}, + {"full id with date", ExecutorClaude, "claude-haiku-4-5-20251001", false}, + {"full id with 1m variant", ExecutorClaude, "claude-opus-5[1m]", false}, + {"unreleased full id still passes", ExecutorClaude, "claude-opus-9", false}, + + // The mistakes that actually happen. + {"typo alias", ExecutorClaude, "opuss", true}, + {"executor slug as model", ExecutorClaude, "claude", true}, + {"another vendor's model", ExecutorClaude, "gpt-5", true}, + {"grok model on claude", ExecutorClaude, "grok-4", true}, + {"bare version", ExecutorClaude, "opus-4.5", true}, + {"shell metacharacters", ExecutorClaude, "claude-opus-5; rm -rf /", true}, + {"whitespace inside", ExecutorClaude, "claude opus", true}, + + // Grok takes full IDs only, no aliases. + {"grok id", ExecutorGrok, "grok-4", false}, + {"grok fast id", ExecutorGrok, "grok-code-fast-1", false}, + {"unreleased grok id", ExecutorGrok, "grok-9-turbo", false}, + {"claude alias on grok", ExecutorGrok, ModelOpus, true}, + {"claude id on grok", ExecutorGrok, "claude-opus-5", true}, + + // Executors with no --model flag: an override there is dead config. + {"codex takes no model", ExecutorCodex, "gpt-5-codex", true}, + {"gemini takes no model", ExecutorGemini, "gemini-2.5-pro", true}, + {"pi takes no model", ExecutorPi, ModelOpus, true}, + {"codex with no override is fine", ExecutorCodex, "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateModel(tt.executor, tt.model) + if tt.wantErr && err == nil { + t.Fatalf("ValidateModel(%q, %q) = nil, want an error", tt.executor, tt.model) + } + if !tt.wantErr && err != nil { + t.Fatalf("ValidateModel(%q, %q) = %v, want nil", tt.executor, tt.model, err) + } + }) + } +} + +// TestValidateModelErrorNamesAlternatives checks the rejection actually tells +// the user what to type instead — the error is the whole user-facing surface. +func TestValidateModelErrorNamesAlternatives(t *testing.T) { + err := ValidateModel(ExecutorClaude, "opuss") + if err == nil { + t.Fatal("expected an error for an unknown claude model") + } + for _, want := range []string{"opuss", ModelOpus, ModelSonnet, "claude-"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q should mention %q", err, want) + } + } + + err = ValidateModel(ExecutorCodex, "gpt-5-codex") + if err == nil { + t.Fatal("expected an error setting a model on a modelless executor") + } + // It must name the executors that DO take a model, or the user is stuck. + for _, want := range []string{ExecutorCodex, ExecutorClaude, ExecutorGrok} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q should mention %q", err, want) + } + } +} + +// TestExecutorSupportsModel pins which executors pass --model to their CLI. +// Adding a --model flag to another executor must update executorModelPrefix, +// or overrides for it stay silently ignored. +func TestExecutorSupportsModel(t *testing.T) { + supported := map[string]bool{ExecutorClaude: true, ExecutorGrok: true} + for _, e := range KnownExecutors() { + if got := ExecutorSupportsModel(e); got != supported[e] { + t.Errorf("ExecutorSupportsModel(%q) = %v, want %v", e, got, supported[e]) + } + } + if !ExecutorSupportsModel("") { + t.Error("empty executor should resolve to the default (claude), which supports models") + } + if got := ModelCapableExecutors(); len(got) != 2 || got[0] != ExecutorClaude || got[1] != ExecutorGrok { + t.Errorf("ModelCapableExecutors() = %v, want [claude grok]", got) + } +} + +// TestModelsForExecutor verifies the completion/help list is executor-specific +// and that every entry it advertises actually validates. +func TestModelsForExecutor(t *testing.T) { + for _, e := range ModelCapableExecutors() { + models := ModelsForExecutor(e) + if len(models) == 0 { + t.Errorf("ModelsForExecutor(%q) is empty", e) + } + for _, m := range models { + if err := ValidateModel(e, m); err != nil { + t.Errorf("ModelsForExecutor(%q) advertises %q but ValidateModel rejects it: %v", e, m, err) + } + } + } + if got := ModelsForExecutor(ExecutorCodex); got != nil { + t.Errorf("ModelsForExecutor(codex) = %v, want nil (no --model flag)", got) + } + // The returned slice must not alias the package-level table. + models := ModelsForExecutor(ExecutorClaude) + models[0] = "mutated" + if ModelsForExecutor(ExecutorClaude)[0] == "mutated" { + t.Error("ModelsForExecutor returned a slice aliasing the package table") + } +} + +// TestModelOptionsAreValid guards the UI picker: every value it offers must be +// one the CLI check accepts, or the TUI and CLI disagree about what's legal. +func TestModelOptionsAreValid(t *testing.T) { + for _, m := range ModelOptions() { + if err := ValidateModel(ExecutorClaude, m); err != nil { + t.Errorf("ModelOptions() offers %q but ValidateModel rejects it: %v", m, err) + } + } +} + +// TestModelBackendIsCustom covers the escape hatch: a task routed at a proxy +// names the proxy's models, which ty cannot check against Anthropic's. +func TestModelBackendIsCustom(t *testing.T) { + if ModelBackendIsCustom("", nil) { + t.Error("stock backend (no config dir, no env) should not be custom") + } + if ModelBackendIsCustom(" ", map[string]string{"ANTHROPIC_BASE_URL": " "}) { + t.Error("whitespace-only overrides should not count as custom") + } + if !ModelBackendIsCustom("~/.claude-ollama", nil) { + t.Error("a CLAUDE_CONFIG_DIR override is a custom backend") + } + if !ModelBackendIsCustom("", map[string]string{"ANTHROPIC_BASE_URL": "http://127.0.0.1:11434"}) { + t.Error("an ANTHROPIC_BASE_URL override is a custom backend") + } + // The point of the hatch: an ollama model name is unknowable to ty, so it + // must never be validated. + if err := ValidateModel(ExecutorClaude, "glm-5.2:cloud"); err == nil { + t.Error("a proxy model name should fail the strict check (callers must skip it, not rely on it passing)") + } +} + +// TestIsValidModel covers the looser executor-less form used by the new-task +// form's remembered per-project default. +func TestIsValidModel(t *testing.T) { + valid := []string{"", ModelOpus, "claude-opus-5", "grok-4"} + for _, m := range valid { + if !IsValidModel(m) { + t.Errorf("IsValidModel(%q) = false, want true", m) + } + } + // "claude" is the executor slug an early default baked into every row; it is + // not a model and must not survive as a remembered default. + invalid := []string{"claude", "opuss", "gpt-5"} + for _, m := range invalid { + if IsValidModel(m) { + t.Errorf("IsValidModel(%q) = true, want false", m) + } + } +} + +// TestValidateTaskModel covers the write-path entry point: the same rules as +// ValidateModel, but with the proxy escape hatch resolved from the task's own +// config, its project's config, and the ambient environment. +func TestValidateTaskModel(t *testing.T) { + tmpDir := t.TempDir() + database, err := Open(filepath.Join(tmpDir, "test.db")) + if err != nil { + t.Fatalf("open db: %v", err) + } + defer database.Close() + + if err := database.CreateProject(&Project{Name: "stock", Path: filepath.Join(tmpDir, "stock")}); err != nil { + t.Fatalf("create stock project: %v", err) + } + if err := database.CreateProject(&Project{ + Name: "ollama", + Path: filepath.Join(tmpDir, "ollama"), + ClaudeConfigDir: "~/.claude-ollama", + }); err != nil { + t.Fatalf("create ollama project: %v", err) + } + + t.Setenv("ANTHROPIC_BASE_URL", "") + + t.Run("nil and empty are no-ops", func(t *testing.T) { + if err := database.ValidateTaskModel(nil); err != nil { + t.Errorf("nil task: %v", err) + } + if err := database.ValidateTaskModel(&Task{Project: "stock"}); err != nil { + t.Errorf("no override: %v", err) + } + if err := database.ValidateTaskModel(&Task{Project: "stock", Model: " "}); err != nil { + t.Errorf("blank override: %v", err) + } + }) + + t.Run("stock backend is validated", func(t *testing.T) { + if err := database.ValidateTaskModel(&Task{Project: "stock", Model: "opuss"}); err == nil { + t.Error("expected a typo'd model to be rejected") + } + if err := database.ValidateTaskModel(&Task{Project: "stock", Model: ModelOpus}); err != nil { + t.Errorf("a valid alias should pass: %v", err) + } + if err := database.ValidateTaskModel(&Task{Project: "stock", Executor: ExecutorCodex, Model: ModelOpus}); err == nil { + t.Error("a modelless executor should be rejected") + } + }) + + t.Run("per-task config dir skips validation", func(t *testing.T) { + task := &Task{Project: "stock", Model: "glm-5.2:cloud", ClaudeConfigDir: "~/.claude-ollama"} + if err := database.ValidateTaskModel(task); err != nil { + t.Errorf("a task-level config dir should skip validation: %v", err) + } + }) + + t.Run("per-task env skips validation", func(t *testing.T) { + task := &Task{ + Project: "stock", + Model: "glm-5.2:cloud", + EnvJSON: `{"ANTHROPIC_BASE_URL":"http://127.0.0.1:11434"}`, + } + if err := database.ValidateTaskModel(task); err != nil { + t.Errorf("a task-level ANTHROPIC_BASE_URL should skip validation: %v", err) + } + }) + + t.Run("project config dir skips validation", func(t *testing.T) { + if err := database.ValidateTaskModel(&Task{Project: "ollama", Model: "glm-5.2:cloud"}); err != nil { + t.Errorf("a project-level config dir should skip validation: %v", err) + } + // Same model, stock project: still rejected. + if err := database.ValidateTaskModel(&Task{Project: "stock", Model: "glm-5.2:cloud"}); err == nil { + t.Error("the escape hatch must not leak across projects") + } + }) + + t.Run("unknown project is not a custom backend", func(t *testing.T) { + if err := database.ValidateTaskModel(&Task{Project: "nope", Model: "opuss"}); err == nil { + t.Error("an unresolvable project must not disable validation") + } + }) + + t.Run("ambient base url skips validation", func(t *testing.T) { + t.Setenv("ANTHROPIC_BASE_URL", "http://127.0.0.1:11434") + if err := database.ValidateTaskModel(&Task{Project: "stock", Model: "glm-5.2:cloud"}); err != nil { + t.Errorf("an ambient ANTHROPIC_BASE_URL should skip validation: %v", err) + } + }) +} diff --git a/internal/db/tasks.go b/internal/db/tasks.go index 94128845..0663661b 100644 --- a/internal/db/tasks.go +++ b/internal/db/tasks.go @@ -285,33 +285,7 @@ func (t *Task) EnvMap() map[string]string { return out } -// Model overrides are per-task selections for Claude's model (claude --model). -// An empty value means "no override" — the task uses Claude's global default, -// leaving the user's global setting untouched. The aliases below are accepted by -// the Claude CLI's --model flag; a full model name (e.g. "claude-opus-4-8") is -// also valid and passed through unchanged. -const ( - ModelFable = "fable" - ModelOpus = "opus" - ModelSonnet = "sonnet" - ModelHaiku = "haiku" -) - -// ModelOptions returns the per-task model override aliases offered in the UI. -// The Claude CLI also accepts full model names, so this is a convenience list, -// not an exhaustive set. Keep it in sync with the aliases the Claude CLI supports -// as new models ship (e.g. "fable" was added alongside opus/sonnet/haiku). -func ModelOptions() []string { - return []string{ModelOpus, ModelSonnet, ModelHaiku, ModelFable} -} - -// IsValidModel reports whether s is an acceptable per-task model override. The -// empty string is valid and means "use the global/Claude default" (no per-task -// override). Any non-empty value is accepted because the Claude CLI validates -// the model name itself and supports both aliases and full model IDs. -func IsValidModel(s string) bool { - return true -} +// Per-task model overrides (constants, options and validation) live in models.go. // Port allocation constants const ( diff --git a/internal/pipeline/custom_test.go b/internal/pipeline/custom_test.go index f30e4249..d5d949df 100644 --- a/internal/pipeline/custom_test.go +++ b/internal/pipeline/custom_test.go @@ -294,3 +294,90 @@ func contains(ss []string, want string) bool { } return false } + +// TestParseDefinitionRejectsUnknownModel covers the workflow-file half of model +// validation. A step whose model its CLI won't accept fails silently at launch +// — the agent rejects the flag inside tmux and the step stalls looking busy — +// so the file must be rejected while it is being read. +func TestParseDefinitionRejectsUnknownModel(t *testing.T) { + tests := []struct { + name string + yaml string + want string // substring the error must carry + }{ + { + name: "typo in a claude alias", + yaml: "name: k\nsteps:\n - {name: Plan, model: opuss, prompt: Plan it.}\n", + want: "opuss", + }, + { + name: "executor slug used as a model", + yaml: "name: k\nsteps:\n - {name: Plan, model: claude, prompt: Plan it.}\n", + want: "claude", + }, + { + name: "model on an executor with no --model flag", + yaml: "name: k\nsteps:\n - {name: QA, executor: codex, model: gpt-5-codex, prompt: QA it.}\n", + want: "codex", + }, + { + name: "claude model on a grok step", + yaml: "name: k\nsteps:\n - {name: Plan, executor: grok, model: opus, prompt: Plan it.}\n", + want: "grok", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ParseDefinition([]byte(tt.yaml)) + if err == nil { + t.Fatal("ParseDefinition accepted a step with an unusable model") + } + if !strings.Contains(err.Error(), tt.want) { + t.Errorf("error %q should mention %q", err, tt.want) + } + // The step name locates the problem in a multi-step file. + if !strings.Contains(err.Error(), "step ") { + t.Errorf("error %q should name the offending step", err) + } + }) + } +} + +// TestParseDefinitionAllowsProxyModels is the escape hatch: a step routed at a +// proxy (a config_dir or an ANTHROPIC_BASE_URL env override — the ollama shape) +// names the proxy's models, which ty has no way to check. +func TestParseDefinitionAllowsProxyModels(t *testing.T) { + viaConfigDir := "name: k\nsteps:\n - {name: Code, model: glm-5.2:cloud, config_dir: \"~/.claude-ollama\", prompt: Do it.}\n" + if _, err := ParseDefinition([]byte(viaConfigDir)); err != nil { + t.Errorf("config_dir-routed step should skip model validation: %v", err) + } + + viaEnv := "name: k\nsteps:\n - {name: Code, model: glm-5.2:cloud, env: {ANTHROPIC_BASE_URL: \"http://127.0.0.1:11434\"}, prompt: Do it.}\n" + if _, err := ParseDefinition([]byte(viaEnv)); err != nil { + t.Errorf("ANTHROPIC_BASE_URL-routed step should skip model validation: %v", err) + } + + // Same model with no routing override is still a hard error. + bare := "name: k\nsteps:\n - {name: Code, model: glm-5.2:cloud, prompt: Do it.}\n" + if _, err := ParseDefinition([]byte(bare)); err == nil { + t.Error("an unrouted step with a proxy-only model should be rejected") + } +} + +// TestParseDefinitionAcceptsRealModels guards against the check being too +// strict: full model IDs, including ones newer than this code, must pass. +func TestParseDefinitionAcceptsRealModels(t *testing.T) { + for _, model := range []string{"opus", "sonnet", "haiku", "fable", "claude-opus-5", "claude-opus-5[1m]", "claude-opus-9"} { + // Quoted: a bracketed variant like claude-opus-5[1m] is a YAML flow + // sequence otherwise. + yaml := "name: k\nsteps:\n - {name: Plan, model: \"" + model + "\", prompt: Plan it.}\n" + if _, err := ParseDefinition([]byte(yaml)); err != nil { + t.Errorf("ParseDefinition rejected valid model %q: %v", model, err) + } + } + // db is imported by the sample above; keep the executor-specific case honest. + yaml := "name: k\nsteps:\n - {name: Plan, executor: " + db.ExecutorGrok + ", model: grok-4, prompt: Plan it.}\n" + if _, err := ParseDefinition([]byte(yaml)); err != nil { + t.Errorf("ParseDefinition rejected a valid grok model: %v", err) + } +} diff --git a/internal/pipeline/definition_file.go b/internal/pipeline/definition_file.go index 40e2b5b7..15ab148e 100644 --- a/internal/pipeline/definition_file.go +++ b/internal/pipeline/definition_file.go @@ -7,6 +7,8 @@ import ( "strings" "gopkg.in/yaml.v3" + + "github.com/bborn/workflow/internal/db" ) // Custom workflows are authored as plain YAML files — one workflow per file — @@ -107,6 +109,16 @@ func ParseDefinition(data []byte) (Definition, error) { if exec == "" { exec = "claude" } + // A model the step's CLI won't accept fails silently at launch (the agent + // rejects the flag inside tmux and the step stalls), so catch it while the + // file is being read. Steps routed at a proxy — a config_dir or an + // ANTHROPIC_BASE_URL env override, the ollama shape — name the proxy's + // models, which ty can't check. + if !db.ModelBackendIsCustom(s.ConfigDir, s.Env) { + if err := db.ValidateModel(exec, s.Model); err != nil { + return Definition{}, fmt.Errorf("step %q: %w", name, err) + } + } step := Step{ Name: name, Kind: kind, diff --git a/internal/web/handlers.go b/internal/web/handlers.go index 23ec3733..8750985e 100644 --- a/internal/web/handlers.go +++ b/internal/web/handlers.go @@ -273,6 +273,13 @@ func (s *Server) handleUpdateTask(w http.ResponseWriter, r *http.Request) { } if req.Model != nil { task.Model = *req.Model + // Validated only when the request is actually setting a model, and against + // the executor as it stands after this update. Updates that leave the model + // alone must keep working even if the stored value predates this check. + if err := s.db.ValidateTaskModel(task); err != nil { + jsonErr(w, err.Error(), http.StatusBadRequest) + return + } } if err := s.db.UpdateTask(task); err != nil { diff --git a/internal/web/model_validation_test.go b/internal/web/model_validation_test.go new file mode 100644 index 00000000..1227ba6a --- /dev/null +++ b/internal/web/model_validation_test.go @@ -0,0 +1,160 @@ +package web + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/bborn/workflow/internal/db" +) + +// patchTask issues a PATCH /api/tasks/{id} with the given JSON body. +func patchTask(t *testing.T, srv *Server, id int64, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest("PATCH", fmt.Sprintf("/api/tasks/%d", id), strings.NewReader(body)) + req.SetPathValue("id", fmt.Sprint(id)) + w := httptest.NewRecorder() + srv.handleUpdateTask(w, req) + return w +} + +// TestHandleUpdateTask_RejectsUnknownModel closes the API half of model +// validation: the GUI and any HTTP client go through here, and a model the +// agent's CLI won't accept stalls the task exactly as it would from the CLI. +func TestHandleUpdateTask_RejectsUnknownModel(t *testing.T) { + srv, database, _ := setupServer(t) + task := createTestTask(t, database, &db.Task{Title: "t", Status: db.StatusBacklog, Type: db.TypeCode}) + + t.Setenv("ANTHROPIC_BASE_URL", "") + + tests := []struct { + name string + body string + }{ + {"typo", `{"model":"opuss"}`}, + {"executor slug as model", `{"model":"claude"}`}, + {"other vendor", `{"model":"gpt-5"}`}, + {"model on a modelless executor", `{"executor":"codex","model":"opus"}`}, + {"claude alias on grok", `{"executor":"grok","model":"opus"}`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := patchTask(t, srv, task.ID, tt.body) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d (%s)", w.Code, w.Body.String()) + } + // The rejection must not have been persisted. + got, err := database.GetTask(task.ID) + if err != nil { + t.Fatalf("get task: %v", err) + } + if got.Model != "" { + t.Errorf("rejected model was stored anyway: %q", got.Model) + } + }) + } +} + +// TestHandleUpdateTask_AcceptsRealModels guards against over-strictness on the +// API path, including model IDs newer than this code. +func TestHandleUpdateTask_AcceptsRealModels(t *testing.T) { + srv, database, _ := setupServer(t) + t.Setenv("ANTHROPIC_BASE_URL", "") + + tests := []struct { + name string + body string + want string + }{ + {"alias", `{"model":"opus"}`, "opus"}, + {"full id", `{"model":"claude-opus-5"}`, "claude-opus-5"}, + {"unreleased id", `{"model":"claude-opus-9"}`, "claude-opus-9"}, + {"grok id with executor", `{"executor":"grok","model":"grok-4"}`, "grok-4"}, + {"clearing the override", `{"model":""}`, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + task := createTestTask(t, database, &db.Task{Title: tt.name, Status: db.StatusBacklog, Type: db.TypeCode}) + w := patchTask(t, srv, task.ID, tt.body) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d (%s)", w.Code, w.Body.String()) + } + // The response echoes the task; the row is what actually matters. + var echoed map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &echoed); err != nil { + t.Fatalf("decode response: %v", err) + } + if echoed["model"] != nil && echoed["model"] != tt.want { + t.Errorf("response model = %v, want %q", echoed["model"], tt.want) + } + stored, err := database.GetTask(task.ID) + if err != nil { + t.Fatalf("get task: %v", err) + } + if stored.Model != tt.want { + t.Errorf("stored model = %q, want %q", stored.Model, tt.want) + } + }) + } +} + +// TestHandleUpdateTask_LeavesStoredModelAlone is the compatibility guard: a +// task carrying a model this check would now reject (stored before the check +// existed) must still accept unrelated edits. Validation runs only when the +// request is actually setting a model. +func TestHandleUpdateTask_LeavesStoredModelAlone(t *testing.T) { + srv, database, _ := setupServer(t) + t.Setenv("ANTHROPIC_BASE_URL", "") + + task := createTestTask(t, database, &db.Task{Title: "legacy", Status: db.StatusBacklog, Type: db.TypeCode}) + // Write a now-invalid model straight to the row, the way an older build could. + if _, err := database.Exec(`UPDATE tasks SET model = 'claude' WHERE id = ?`, task.ID); err != nil { + t.Fatalf("seed legacy model: %v", err) + } + + w := patchTask(t, srv, task.ID, `{"title":"renamed"}`) + if w.Code != http.StatusOK { + t.Fatalf("a title-only edit must not be blocked by a legacy model: %d (%s)", w.Code, w.Body.String()) + } + stored, err := database.GetTask(task.ID) + if err != nil { + t.Fatalf("get task: %v", err) + } + if stored.Title != "renamed" { + t.Errorf("title = %q, want %q", stored.Title, "renamed") + } + if stored.Model != "claude" { + t.Errorf("stored model should be untouched, got %q", stored.Model) + } +} + +// TestHandleUpdateTask_ProxyModelNeedsRouting mirrors the CLI escape hatch on +// the API: a proxy-only model is rejected on a stock backend and accepted once +// the task is actually routed at the proxy. +func TestHandleUpdateTask_ProxyModelNeedsRouting(t *testing.T) { + srv, database, _ := setupServer(t) + t.Setenv("ANTHROPIC_BASE_URL", "") + + task := createTestTask(t, database, &db.Task{Title: "ollama", Status: db.StatusBacklog, Type: db.TypeCode}) + if w := patchTask(t, srv, task.ID, `{"model":"glm-5.2:cloud"}`); w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for a proxy model on a stock backend, got %d", w.Code) + } + + // Route the task at the proxy, then the same model is legitimate. + if _, err := database.Exec(`UPDATE tasks SET claude_config_dir = '~/.claude-ollama' WHERE id = ?`, task.ID); err != nil { + t.Fatalf("route task at proxy: %v", err) + } + if w := patchTask(t, srv, task.ID, `{"model":"glm-5.2:cloud"}`); w.Code != http.StatusOK { + t.Fatalf("expected 200 for a proxy model on a routed task, got %d (%s)", w.Code, w.Body.String()) + } + stored, err := database.GetTask(task.ID) + if err != nil { + t.Fatalf("get task: %v", err) + } + if stored.Model != "glm-5.2:cloud" { + t.Errorf("stored model = %q, want %q", stored.Model, "glm-5.2:cloud") + } +}