From 2f0df85c2865cec82284ad483d2f10af6002eee1 Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Wed, 26 Aug 2026 12:12:09 -0500 Subject: [PATCH 1/2] feat(cli): reject model overrides the agent's CLI won't accept MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ty create --model` accepted any string: IsValidModel returned true unconditionally, on the theory that the Claude CLI validates the name itself. It does — but inside tmux, after the task has launched, where nobody sees it. The task just sits there looking busy. The "claude" executor slug an early default baked into every row (see the clear-model repair in sqlite.go) is that failure mode. Validate the override up front instead. A model passes when it is one of the executor's aliases (opus/sonnet/haiku/fable/opusplan, with an optional [1m] variant) or a well-formed ID carrying the executor's vendor prefix, so claude-opus-5 and grok-4-fast work without ty having to track every release. Typos, other vendors' models, and the executor slug are rejected with a message naming the alternatives. Executors with no --model flag at all (codex, gemini, pi, opencode, openclaw) now say so rather than silently dropping the override. Two things stay unchecked on purpose: a task routed at a proxy — a CLAUDE_CONFIG_DIR override or ANTHROPIC_BASE_URL, the ollama shape — names the proxy's models (glm-5.2:cloud), which ty cannot know; and the TUI form, whose picker only offers valid values anyway. Workflow YAML gets the same check at parse time, with the same proxy escape hatch, since a bad model there stalls a step just as quietly. Shell completion now offers the chosen executor's models. Co-Authored-By: Claude Opus 5 --- cmd/task/cli_test.go | 87 ++++++++++++ cmd/task/main.go | 41 +++++- internal/db/models.go | 161 +++++++++++++++++++++++ internal/db/models_test.go | 190 +++++++++++++++++++++++++++ internal/db/tasks.go | 28 +--- internal/pipeline/custom_test.go | 87 ++++++++++++ internal/pipeline/definition_file.go | 12 ++ 7 files changed, 575 insertions(+), 31 deletions(-) create mode 100644 internal/db/models.go create mode 100644 internal/db/models_test.go diff --git a/cmd/task/cli_test.go b/cmd/task/cli_test.go index 0405eae6..56346f5b 100644 --- a/cmd/task/cli_test.go +++ b/cmd/task/cli_test.go @@ -1833,3 +1833,90 @@ func TestShouldSubmitInput(t *testing.T) { }) } } + +// TestProjectUsesCustomModelBackend 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 TestProjectUsesCustomModelBackend(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", "") + + if projectUsesCustomModelBackend(database, "stock") { + t.Error("a project with no config-dir override uses the stock backend") + } + if projectUsesCustomModelBackend(database, "") { + t.Error("no project means no per-project override") + } + if projectUsesCustomModelBackend(database, "does-not-exist") { + t.Error("an unknown project must not be treated as custom") + } + if !projectUsesCustomModelBackend(database, "ollama") { + t.Error("a project with a CLAUDE_CONFIG_DIR override is a custom backend") + } + + // An ambient ANTHROPIC_BASE_URL routes every project at a proxy. + t.Setenv("ANTHROPIC_BASE_URL", "http://127.0.0.1:11434") + if !projectUsesCustomModelBackend(database, "stock") { + t.Error("ANTHROPIC_BASE_URL in the environment is a custom backend") + } +} + +// 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..5cecc6a9 100644 --- a/cmd/task/main.go +++ b/cmd/task/main.go @@ -90,6 +90,25 @@ func openTaskDB(path string) (*db.DB, error) { return database, nil } +// projectUsesCustomModelBackend reports whether the project (or the ambient +// environment) points Claude at something other than Anthropic's API — a +// per-project CLAUDE_CONFIG_DIR override, or ANTHROPIC_BASE_URL aimed at a +// proxy like ollama. Model names are the proxy's there ("glm-5.2:cloud"), so a +// --model override must not be checked against Anthropic's model list. +func projectUsesCustomModelBackend(database *db.DB, project string) bool { + if db.ModelBackendIsCustom("", map[string]string{"ANTHROPIC_BASE_URL": os.Getenv("ANTHROPIC_BASE_URL")}) { + return true + } + if project == "" { + return false + } + p, err := database.GetProjectByName(project) + if err != nil || p == nil { + return false + } + return db.ModelBackendIsCustom(p.ClaudeConfigDir, nil) +} + // waitForEventHooks blocks until any in-flight hook scripts have completed. // CLI commands that mutate task state must defer this before exit, otherwise // the Go process terminates before the hook goroutine runs its subprocess. @@ -775,8 +794,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 +806,18 @@ 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. Skipped when the + // project routes Claude at a proxy (ollama and friends), where the model + // names belong to the proxy, not Anthropic. + if !projectUsesCustomModelBackend(database, project) { + if err := db.ValidateModel(taskExecutor, 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 +919,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 +935,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..124336bd --- /dev/null +++ b/internal/db/models.go @@ -0,0 +1,161 @@ +package db + +import ( + "fmt" + "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) +} + +// 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..111b8f69 --- /dev/null +++ b/internal/db/models_test.go @@ -0,0 +1,190 @@ +package db + +import ( + "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) + } + } +} 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, From 838fc4fd7ce7aaaf719a4ad4d51e3112b6339cfa Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Wed, 26 Aug 2026 12:21:38 -0500 Subject: [PATCH 2/2] feat(web): validate model overrides on the task update API too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI check left the HTTP path open: PATCH /api/tasks/{id} wrote whatever `model` it was handed, so the GUI (and any client) could still park a task on a model the agent's CLI rejects. Rather than repeat the CLI's escape-hatch logic, give both paths one entry point: db.ValidateTaskModel resolves the proxy hatch from the task's own config dir and env, its project's config dir, and an ambient ANTHROPIC_BASE_URL, then defers to ValidateModel. `ty create` now builds a probe task and calls it, dropping its local helper. The API validates only when the request is actually setting a model, and against the executor as it stands after the update — an edit that touches the title must not be blocked by a model stored before this check existed. MCP needs nothing: taskyou_create_task takes neither model nor executor, so there is no unvalidated write there. Co-Authored-By: Claude Opus 5 --- cmd/task/cli_test.go | 29 +++-- cmd/task/main.go | 37 ++---- internal/db/models.go | 37 ++++++ internal/db/models_test.go | 91 +++++++++++++++ internal/web/handlers.go | 7 ++ internal/web/model_validation_test.go | 160 ++++++++++++++++++++++++++ 6 files changed, 322 insertions(+), 39 deletions(-) create mode 100644 internal/web/model_validation_test.go diff --git a/cmd/task/cli_test.go b/cmd/task/cli_test.go index 56346f5b..a6f4f396 100644 --- a/cmd/task/cli_test.go +++ b/cmd/task/cli_test.go @@ -1834,11 +1834,11 @@ func TestShouldSubmitInput(t *testing.T) { } } -// TestProjectUsesCustomModelBackend covers the escape hatch that keeps +// 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 TestProjectUsesCustomModelBackend(t *testing.T) { +func TestCreateModelBackendEscapeHatch(t *testing.T) { tmpDir := t.TempDir() dbPath := filepath.Join(tmpDir, "test.db") @@ -1861,23 +1861,28 @@ func TestProjectUsesCustomModelBackend(t *testing.T) { t.Setenv("ANTHROPIC_BASE_URL", "") - if projectUsesCustomModelBackend(database, "stock") { - t.Error("a project with no config-dir override uses the stock backend") + // 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 projectUsesCustomModelBackend(database, "") { - t.Error("no project means no per-project override") + + 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 projectUsesCustomModelBackend(database, "does-not-exist") { - t.Error("an unknown project must not be treated as custom") + if err := probe("does-not-exist", "opuss"); err == nil { + t.Error("an unknown project must not be treated as a custom backend") } - if !projectUsesCustomModelBackend(database, "ollama") { - t.Error("a project with a CLAUDE_CONFIG_DIR override is 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 !projectUsesCustomModelBackend(database, "stock") { - t.Error("ANTHROPIC_BASE_URL in the environment is a custom backend") + if err := probe("stock", "glm-5.2:cloud"); err != nil { + t.Errorf("ANTHROPIC_BASE_URL in the environment should skip validation: %v", err) } } diff --git a/cmd/task/main.go b/cmd/task/main.go index 5cecc6a9..8ae1c9c9 100644 --- a/cmd/task/main.go +++ b/cmd/task/main.go @@ -90,25 +90,6 @@ func openTaskDB(path string) (*db.DB, error) { return database, nil } -// projectUsesCustomModelBackend reports whether the project (or the ambient -// environment) points Claude at something other than Anthropic's API — a -// per-project CLAUDE_CONFIG_DIR override, or ANTHROPIC_BASE_URL aimed at a -// proxy like ollama. Model names are the proxy's there ("glm-5.2:cloud"), so a -// --model override must not be checked against Anthropic's model list. -func projectUsesCustomModelBackend(database *db.DB, project string) bool { - if db.ModelBackendIsCustom("", map[string]string{"ANTHROPIC_BASE_URL": os.Getenv("ANTHROPIC_BASE_URL")}) { - return true - } - if project == "" { - return false - } - p, err := database.GetProjectByName(project) - if err != nil || p == nil { - return false - } - return db.ModelBackendIsCustom(p.ClaudeConfigDir, nil) -} - // waitForEventHooks blocks until any in-flight hook scripts have completed. // CLI commands that mutate task state must defer this before exit, otherwise // the Go process terminates before the hook goroutine runs its subprocess. @@ -808,14 +789,16 @@ 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. Skipped when the - // project routes Claude at a proxy (ollama and friends), where the model - // names belong to the proxy, not Anthropic. - if !projectUsesCustomModelBackend(database, project) { - if err := db.ValidateModel(taskExecutor, modelOverride); err != nil { - fmt.Fprintln(os.Stderr, errorStyle.Render("Error: "+err.Error())) - os.Exit(1) - } + // 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 diff --git a/internal/db/models.go b/internal/db/models.go index 124336bd..63e69aa6 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -2,6 +2,7 @@ package db import ( "fmt" + "os" "regexp" "strings" ) @@ -125,6 +126,42 @@ func ValidateModel(executor, model string) error { 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 diff --git a/internal/db/models_test.go b/internal/db/models_test.go index 111b8f69..3b651988 100644 --- a/internal/db/models_test.go +++ b/internal/db/models_test.go @@ -1,6 +1,7 @@ package db import ( + "path/filepath" "strings" "testing" ) @@ -188,3 +189,93 @@ func TestIsValidModel(t *testing.T) { } } } + +// 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/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") + } +}