diff --git a/README.md b/README.md index ef4cd887..d1aba65f 100644 --- a/README.md +++ b/README.md @@ -629,6 +629,7 @@ Supported values: | `llm.auth` | `subscription`, `api_key` | | `llm.adapter` | `claude_cli`, `anthropic_api`, `openai_api`, `pi_rpc`, and `codex_cli` are usable for review. `codex_cli` requires `provider: openai` and `auth: subscription`, and is currently best-effort/beta because Codex does not yet expose an explicit all-tools-disabled flag. | | `llm.model_map` keys | `small`, `medium`, `large` | +| `llm.max_effort` keys | `small`, `medium`, `large`; values `low`, `medium`, `high` | | `llm.reviewer_model_tier` | `small`, `medium`, `large` | | `review_policy.major_event` | `comment`, `request_changes` | | `review_policy.resolve_threads` | `auto`, `never` | @@ -659,6 +660,39 @@ Migration note: older releases treated reviewer `model_tier` as a direct map lookup. Current releases treat it as a minimum acceptable tier, so profiles can raise the reviewer baseline without editing shared agent catalogs. +### Capping Effort Per Tier + +Agent catalogs declare an absolute `effort` (`low`, `medium`, `high`) that +becomes the provider's reasoning-effort setting. `llm.max_effort` caps that +value per tier so a deployment can bound spend on expensive models without +editing shared catalogs: + +```yaml +llm: + model_map: + large: openai-codex/gpt-5.6-sol + medium: openai-codex/gpt-5.6-terra + max_effort: + large: medium +``` + +A tier absent from `max_effort` is uncapped. The cap is a ceiling only: an agent +declaring `low` under a `medium` ceiling still runs at `low`. Caps are keyed by +the tier resolved after the floor calculation above, and they apply to internal +stages (selection, synthesis, thread analysis) as well as reviewers, so capping +`medium` affects more than reviewer agents. + +Four paths intentionally bypass the cap, because each is an explicit selection +of a concrete model or effort: + +- `--reviewer-effort` and `--reviewer-model` on `cr review` +- agent `model_id`, which selects an exact model and has no tier to cap +- `cr benchmark run`, where `stages.reviewers.effort` is required so candidates + stay comparable + +`cr init` preserves `max_effort` but cannot yet edit it; set it by hand in +`config.yml`. + Dry-run and no-post runs also record selected reviewer runtime resolution in `agent-sources.json` for auditability. Each selected agent may include `reviewer_runtime` with: diff --git a/docs/architecture.md b/docs/architecture.md index 60bb2cd0..d51f0140 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -56,6 +56,13 @@ This boundary exists so model catalog data, provider capabilities, token costs, and profile-level tier floors can be added without touching individual review stages. Runtime hard-coding bypasses user preference and is a bug. +The resolver also applies the profile's `llm.max_effort` ceiling, keyed by the +tier it resolved. Because the ceiling is tier-keyed, it does not apply to paths +that select a concrete model or effort directly: an explicit `ModelOverride` +returns before the clamp, and `agent.model_id` has no tier to key on. Callers +that override effort after the resolver returns, such as `--reviewer-effort`, +also win over the ceiling by construction. + Reviewer `agent.model_id` is an exact provider-specific model override. It must still enter runtime execution through `stagemodel.ResolveStageModel` as a model override rather than bypassing the resolver, but it intentionally bypasses the diff --git a/internal/cmd/initcmd/initcmd.go b/internal/cmd/initcmd/initcmd.go index a82f1b99..0ba4c488 100644 --- a/internal/cmd/initcmd/initcmd.go +++ b/internal/cmd/initcmd/initcmd.go @@ -430,6 +430,7 @@ type initLLMRuntimeDraft struct { CredentialStore string CredentialRef string ModelMap config.ModelMap + MaxEffort config.EffortMap ReviewerModelTier config.ModelTier } @@ -3404,6 +3405,9 @@ func buildNonInteractiveInitPlan(cmd *cobra.Command, opts *root.Options, flags i } profile.LLM.ModelMap = modelMap } + if previousProfile.LLM.MaxEffort != nil { + profile.LLM.MaxEffort = copyEffortMap(previousProfile.LLM.MaxEffort) + } if !cmd.Flags().Changed("agent-source") { profile.AgentSources = append([]string(nil), previousProfile.AgentSources...) } @@ -4541,6 +4545,7 @@ func initLLMRuntimeDraftFromConfig(llm config.LLMConfig) initLLMRuntimeDraft { CredentialStore: initCredentialStoreDraftValue(llm.Credential.Store), CredentialRef: strings.TrimSpace(llm.Credential.Name), ModelMap: copyModelMap(llm.ModelMap), + MaxEffort: copyEffortMap(llm.MaxEffort), ReviewerModelTier: llm.ReviewerModelTier, } if spec, ok := config.FindLLMRuntimeSpec(runtime.Provider, runtime.Auth, runtime.Adapter); ok && @@ -4559,6 +4564,7 @@ func (runtime initLLMRuntimeDraft) exportConfig() config.LLMConfig { Auth: runtime.Auth, Adapter: runtime.Adapter, ModelMap: copyModelMap(runtime.ModelMap), + MaxEffort: copyEffortMap(runtime.MaxEffort), ReviewerModelTier: runtime.ReviewerModelTier, } if runtime.Auth == config.LLMAuthAPIKey { @@ -4577,6 +4583,15 @@ func (runtime initLLMRuntimeDraft) identityKey() string { for _, tier := range modelKeys { models = append(models, tier+"="+strings.TrimSpace(runtime.ModelMap[tier])) } + effortKeys := make([]string, 0, len(runtime.MaxEffort)) + for tier := range runtime.MaxEffort { + effortKeys = append(effortKeys, tier) + } + sort.Strings(effortKeys) + efforts := make([]string, 0, len(effortKeys)) + for _, tier := range effortKeys { + efforts = append(efforts, tier+"="+strings.TrimSpace(runtime.MaxEffort[tier])) + } return strings.Join([]string{ string(runtime.Provider), string(runtime.Auth), @@ -4584,6 +4599,7 @@ func (runtime initLLMRuntimeDraft) identityKey() string { initCredentialStoreDraftValue(runtime.CredentialStore), strings.TrimSpace(runtime.CredentialRef), strings.Join(models, "\x1f"), + strings.Join(efforts, "\x1f"), string(runtime.ReviewerModelTier), }, "\x00") } @@ -4762,6 +4778,7 @@ func cloneInitLLMConfig(llm config.LLMConfig) config.LLMConfig { cloned.ModelMap[tier] = model } } + cloned.MaxEffort = copyEffortMap(llm.MaxEffort) return cloned } @@ -5814,6 +5831,17 @@ func initCredentialWritePlanSatisfiesEntry(entry initCredentialPlanEntry, target return true } +func copyEffortMap(effortMap config.EffortMap) config.EffortMap { + if len(effortMap) == 0 { + return nil + } + copied := make(config.EffortMap, len(effortMap)) + for tier, ceiling := range effortMap { + copied[tier] = ceiling + } + return copied +} + func copyModelMap(modelMap config.ModelMap) config.ModelMap { if len(modelMap) == 0 { return nil diff --git a/internal/cmd/initcmd/initcmd_max_effort_test.go b/internal/cmd/initcmd/initcmd_max_effort_test.go new file mode 100644 index 00000000..7735508f --- /dev/null +++ b/internal/cmd/initcmd/initcmd_max_effort_test.go @@ -0,0 +1,54 @@ +package initcmd + +import ( + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/config" +) + +// A runtime round trip must preserve every LLMConfig field init does not edit. +// max_effort has no init editor, so a drop here silently discards a user's +// hand-written cost ceiling. +func TestLLMRuntimeDraftRoundTripPreservesMaxEffort(t *testing.T) { + original := config.LLMConfig{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"large": "gpt-5.6-sol"}, + MaxEffort: config.EffortMap{"large": "medium"}, + } + + got := initLLMRuntimeDraftFromConfig(original).exportConfig() + + if len(got.MaxEffort) != 1 || got.MaxEffort["large"] != "medium" { + t.Fatalf("max_effort after round trip = %#v, want large=medium", got.MaxEffort) + } + if len(got.ModelMap) != 1 || got.ModelMap["large"] != "gpt-5.6-sol" { + t.Fatalf("model_map after round trip = %#v", got.ModelMap) + } +} + +func TestLLMRuntimeIdentityKeyDistinguishesMaxEffort(t *testing.T) { + base := initLLMRuntimeDraft{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"large": "gpt-5.6-sol"}, + } + capped := base + capped.MaxEffort = config.EffortMap{"large": "medium"} + + if base.identityKey() == capped.identityKey() { + t.Fatalf("identityKey collides for runtimes differing only by max_effort") + } +} + +func TestCloneInitLLMConfigDeepCopiesMaxEffort(t *testing.T) { + original := config.LLMConfig{MaxEffort: config.EffortMap{"large": "medium"}} + cloned := cloneInitLLMConfig(original) + cloned.MaxEffort["large"] = "high" + + if original.MaxEffort["large"] != "medium" { + t.Fatalf("clone aliased max_effort: original = %#v", original.MaxEffort) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 68f3c82d..53f44b0f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -18,6 +18,8 @@ import ( "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/cli-common/statedir" "gopkg.in/yaml.v3" + + "github.com/open-cli-collective/codereview-cli/internal/modelprefs" ) const ( @@ -356,12 +358,17 @@ type LLMConfig struct { Adapter LLMAdapter `yaml:"adapter" json:"adapter"` Credential CredentialLocation `yaml:"credential,omitempty" json:"credential,omitempty"` ModelMap ModelMap `yaml:"model_map,omitempty" json:"model_map,omitempty"` + MaxEffort EffortMap `yaml:"max_effort,omitempty" json:"max_effort,omitempty"` ReviewerModelTier ModelTier `yaml:"reviewer_model_tier,omitempty" json:"reviewer_model_tier,omitempty"` } // ModelMap maps portable model tiers to provider-specific model identifiers. type ModelMap map[string]string +// EffortMap caps reasoning effort per model tier. A tier absent from the map is +// uncapped, so the agent-declared or stage-default effort applies unchanged. +type EffortMap map[string]string + // ModelTier is a provider-neutral model slot. type ModelTier string @@ -688,6 +695,27 @@ func ResolveModelTier(llm LLMConfig, tier ModelTier) (ModelMapResolution, bool) return resolved, ok } +// ResolveMaxEffort returns the configured effort ceiling for one portable tier. +// It reports false when the tier is uncapped, which leaves the requested effort +// unchanged. +func ResolveMaxEffort(llm LLMConfig, tier ModelTier) (modelprefs.Effort, bool) { + tier = ModelTier(strings.TrimSpace(string(tier))) + if !tier.Valid() { + return "", false + } + for configured, ceiling := range llm.MaxEffort { + if ModelTier(strings.TrimSpace(configured)) != tier { + continue + } + effort := modelprefs.Effort(strings.TrimSpace(ceiling)) + if !effort.Valid() { + return "", false + } + return effort, true + } + return "", false +} + // ReviewMajorEvent identifies how major findings affect the review event. type ReviewMajorEvent string @@ -1415,6 +1443,18 @@ func validateLLMConfig(field string, llm LLMConfig) error { return invalid("%s.model_map.%s is required", field, tier) } } + for tier, ceiling := range llm.MaxEffort { + modelTier := ModelTier(tier) + if !modelTier.Valid() { + return invalid("%s.max_effort tier %q is invalid", field, tier) + } + if strings.TrimSpace(ceiling) == "" { + return invalid("%s.max_effort.%s is required", field, tier) + } + if !modelprefs.Effort(strings.TrimSpace(ceiling)).Valid() { + return invalid("%s.max_effort.%s %q is invalid; must be one of low, medium, high", field, tier, ceiling) + } + } if llm.ReviewerModelTier != "" && !llm.ReviewerModelTier.Valid() { return invalid("%s.reviewer_model_tier %q is invalid; must be one of small, medium, large", field, llm.ReviewerModelTier) } diff --git a/internal/config/config_max_effort_test.go b/internal/config/config_max_effort_test.go new file mode 100644 index 00000000..331a4ec4 --- /dev/null +++ b/internal/config/config_max_effort_test.go @@ -0,0 +1,61 @@ +package config + +import ( + "errors" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/modelprefs" +) + +func TestValidateAcceptsMaxEffortCeiling(t *testing.T) { + cfg := validFile() + runtime := cfg.LLMRuntimes["home-llm"] + runtime.MaxEffort = EffortMap{"large": "medium"} + cfg.LLMRuntimes["home-llm"] = runtime + if err := Validate(cfg); err != nil { + t.Fatalf("Validate error = %v, want nil", err) + } +} + +func TestValidateRejectsUnknownMaxEffortTier(t *testing.T) { + cfg := validFile() + runtime := cfg.LLMRuntimes["home-llm"] + runtime.MaxEffort = EffortMap{"enormous": "medium"} + cfg.LLMRuntimes["home-llm"] = runtime + err := Validate(cfg) + if !errors.Is(err, ErrInvalid) { + t.Fatalf("Validate error = %v, want ErrInvalid", err) + } + if !strings.Contains(err.Error(), "max_effort") { + t.Fatalf("Validate error = %v, want max_effort mention", err) + } +} + +func TestValidateRejectsUnknownMaxEffortValue(t *testing.T) { + cfg := validFile() + runtime := cfg.LLMRuntimes["home-llm"] + runtime.MaxEffort = EffortMap{"large": "xhigh"} + cfg.LLMRuntimes["home-llm"] = runtime + err := Validate(cfg) + if !errors.Is(err, ErrInvalid) { + t.Fatalf("Validate error = %v, want ErrInvalid", err) + } + if !strings.Contains(err.Error(), "low, medium, high") { + t.Fatalf("Validate error = %v, want valid-value mention", err) + } +} + +func TestResolveMaxEffortReportsUncappedTiers(t *testing.T) { + llm := LLMConfig{MaxEffort: EffortMap{"large": "medium"}} + got, ok := ResolveMaxEffort(llm, ModelTierLarge) + if !ok || got != modelprefs.EffortMedium { + t.Fatalf("ResolveMaxEffort(large) = %q, %v; want medium, true", got, ok) + } + if _, ok := ResolveMaxEffort(llm, ModelTierMedium); ok { + t.Fatalf("ResolveMaxEffort(medium) reported a ceiling, want uncapped") + } + if _, ok := ResolveMaxEffort(llm, ModelTier("bogus")); ok { + t.Fatalf("ResolveMaxEffort(bogus) reported a ceiling, want uncapped") + } +} diff --git a/internal/modelprefs/modelprefs.go b/internal/modelprefs/modelprefs.go index a14381fc..6d687251 100644 --- a/internal/modelprefs/modelprefs.go +++ b/internal/modelprefs/modelprefs.go @@ -20,3 +20,33 @@ func (e Effort) Valid() bool { return false } } + +// Rank orders effort values from cheapest to most expensive. Unknown values +// rank 0 so they never win a comparison against a valid effort. +func (e Effort) Rank() int { + switch e { + case EffortLow: + return 1 + case EffortMedium: + return 2 + case EffortHigh: + return 3 + default: + return 0 + } +} + +// MinEffort returns the cheaper of left and right. Invalid values are ignored +// so a missing ceiling leaves the requested effort untouched. +func MinEffort(left, right Effort) Effort { + if !left.Valid() { + return right + } + if !right.Valid() { + return left + } + if left.Rank() <= right.Rank() { + return left + } + return right +} diff --git a/internal/modelprefs/modelprefs_effort_test.go b/internal/modelprefs/modelprefs_effort_test.go new file mode 100644 index 00000000..f62b2363 --- /dev/null +++ b/internal/modelprefs/modelprefs_effort_test.go @@ -0,0 +1,33 @@ +package modelprefs + +import "testing" + +func TestEffortRankOrdersCheapestFirst(t *testing.T) { + if EffortLow.Rank() >= EffortMedium.Rank() || EffortMedium.Rank() >= EffortHigh.Rank() { + t.Fatalf("effort ranks are not ordered: low=%d medium=%d high=%d", EffortLow.Rank(), EffortMedium.Rank(), EffortHigh.Rank()) + } + if Effort("xhigh").Rank() != 0 { + t.Fatalf("unknown effort rank = %d, want 0", Effort("xhigh").Rank()) + } +} + +func TestMinEffort(t *testing.T) { + tests := []struct { + name string + left, right Effort + want Effort + }{ + {name: "ceiling lowers", left: EffortHigh, right: EffortMedium, want: EffortMedium}, + {name: "ceiling does not raise", left: EffortLow, right: EffortHigh, want: EffortLow}, + {name: "equal", left: EffortMedium, right: EffortMedium, want: EffortMedium}, + {name: "invalid left ignored", left: "", right: EffortHigh, want: EffortHigh}, + {name: "invalid right ignored", left: EffortHigh, right: "xhigh", want: EffortHigh}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := MinEffort(tt.left, tt.right); got != tt.want { + t.Fatalf("MinEffort(%q, %q) = %q, want %q", tt.left, tt.right, got, tt.want) + } + }) + } +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 019c0faa..d25eae6b 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -3141,7 +3141,7 @@ func resolveReviewerRuntimeConfig(req Request, agent agents.Agent) (llmRuntimeCo if err != nil { return llmRuntimeConfig{}, err } - return applyStageRuntimeOverrides(req.ReviewerModelOverride, req.ReviewerEffortOverride, resolved.ResolvedModel, agent.Effort), nil + return applyStageRuntimeOverrides(req.ReviewerModelOverride, req.ReviewerEffortOverride, resolved.ResolvedModel, resolved.ResolvedEffort), nil } func resolveReviewerFastMode(req Request, catalog agents.Catalog) (bool, string, error) { @@ -3178,8 +3178,9 @@ func resolveAgentModel(profile config.Profile, baselineOverride string, agent ag return reviewerRuntimeResolution{}, fmt.Errorf("pipeline: agent %s: %w", agent.ID, err) } return reviewerRuntimeResolution{ - Mode: "exact_model", - ResolvedModel: resolved.Model, + Mode: "exact_model", + ResolvedModel: resolved.Model, + ResolvedEffort: resolved.Effort, }, nil } floorTier := config.ModelTier(strings.TrimSpace(agent.ModelTier)) @@ -3206,6 +3207,7 @@ func resolveAgentModel(profile config.Profile, baselineOverride string, agent ag BaselineTier: string(baselineTier), EffectiveTier: string(resolved.Tier), ResolvedModel: resolved.Model, + ResolvedEffort: resolved.Effort, ModelMapSource: resolved.Source, }, nil } diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 224adf70..c48293a8 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1868,6 +1868,7 @@ func TestDryRunReviewerBaselineTierRaisesReviewerModelFloor(t *testing.T) { BaselineTier: "large", EffectiveTier: "large", ResolvedModel: "profile-large-model", + ResolvedEffort: "medium", ModelMapSource: config.ModelMapSourceConfig, }) } @@ -2727,6 +2728,7 @@ func TestDryRunReviewerModelTierOverrideAppliesOnlyToReviewers(t *testing.T) { BaselineTier: "large", EffectiveTier: "large", ResolvedModel: "profile-large-model", + ResolvedEffort: "medium", ModelMapSource: config.ModelMapSourceConfig, }) } @@ -2778,8 +2780,9 @@ func TestDryRunAgentModelIDBypassesModelMapForReviewer(t *testing.T) { } } assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ - Mode: "exact_model", - ResolvedModel: "agent-provider-model", + Mode: "exact_model", + ResolvedModel: "agent-provider-model", + ResolvedEffort: "medium", }) } @@ -2819,8 +2822,9 @@ func TestDryRunReviewerBaselineDoesNotAffectAgentModelID(t *testing.T) { } } assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ - Mode: "exact_model", - ResolvedModel: "agent-provider-model", + Mode: "exact_model", + ResolvedModel: "agent-provider-model", + ResolvedEffort: "medium", }) } @@ -2849,6 +2853,7 @@ func TestDryRunReviewerFloorsResolveIndependentlyPerAgent(t *testing.T) { BaselineTier: "small", EffectiveTier: "medium", ResolvedModel: "claude-sonnet-5", + ResolvedEffort: "medium", ModelMapSource: config.ModelMapSourceBuiltIn, }) { t.Fatalf("reviewer runtime = %#v", runtime) @@ -2859,6 +2864,7 @@ func TestDryRunReviewerFloorsResolveIndependentlyPerAgent(t *testing.T) { BaselineTier: "small", EffectiveTier: "large", ResolvedModel: "claude-opus-5", + ResolvedEffort: "medium", ModelMapSource: config.ModelMapSourceBuiltIn, }) { t.Fatalf("senior runtime = %#v", runtime) @@ -3016,6 +3022,7 @@ func TestDryRunFastFallsBackForUnsupportedModel(t *testing.T) { BaselineTier: "small", EffectiveTier: "medium", ResolvedModel: "claude-sonnet-5", + ResolvedEffort: "medium", ModelMapSource: config.ModelMapSourceBuiltIn, Fast: true, FastIgnored: true, @@ -7306,3 +7313,52 @@ func (noopStore) DeleteReviewerCohort(context.Context, ledger.ReviewerCohortScop func (noopStore) CompleteRun(context.Context, string, ledger.Outcome, time.Time) error { return nil } + +func TestReviewerRuntimeConfigCapsEffortAtConfiguredTierCeiling(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"small": "luna", "medium": "terra", "large": "sol"}, + MaxEffort: config.EffortMap{"large": "medium"}, + }} + large := agents.Agent{ID: "architecture:solid", ModelTier: "large", Effort: "high"} + medium := agents.Agent{ID: "policies:conventions", ModelTier: "medium", Effort: "high"} + + gotLarge, err := resolveReviewerRuntimeConfig(Request{Profile: profile}, large) + if err != nil { + t.Fatalf("resolveReviewerRuntimeConfig(large): %v", err) + } + if gotLarge.model != "sol" || gotLarge.effort != "medium" { + t.Fatalf("large reviewer = %+v, want model sol effort medium", gotLarge) + } + + gotMedium, err := resolveReviewerRuntimeConfig(Request{Profile: profile}, medium) + if err != nil { + t.Fatalf("resolveReviewerRuntimeConfig(medium): %v", err) + } + if gotMedium.model != "terra" || gotMedium.effort != "high" { + t.Fatalf("medium reviewer = %+v, want model terra effort high", gotMedium) + } +} + +// agent.model_id intentionally bypasses the tier map, so a tier-keyed ceiling +// has no tier to key on and must leave the agent's declared effort alone. +func TestReviewerRuntimeConfigLeavesAgentModelIDUncapped(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"large": "sol"}, + MaxEffort: config.EffortMap{"large": "medium"}, + }} + agent := agents.Agent{ID: "vendor:pinned", ModelID: "sol", ModelTier: "large", Effort: "high"} + + got, err := resolveReviewerRuntimeConfig(Request{Profile: profile}, agent) + if err != nil { + t.Fatalf("resolveReviewerRuntimeConfig: %v", err) + } + if got.model != "sol" || got.effort != "high" { + t.Fatalf("model_id reviewer = %+v, want model sol effort high (uncapped)", got) + } +} diff --git a/internal/pipeline/prompts.go b/internal/pipeline/prompts.go index d5aa9075..ef70ac71 100644 --- a/internal/pipeline/prompts.go +++ b/internal/pipeline/prompts.go @@ -616,6 +616,7 @@ type reviewerRuntimeResolution struct { BaselineTier string `json:"baseline_tier,omitempty"` EffectiveTier string `json:"effective_tier,omitempty"` ResolvedModel string `json:"resolved_model"` + ResolvedEffort string `json:"resolved_effort,omitempty"` ModelMapSource config.ModelMapSource `json:"model_map_source,omitempty"` Fast bool `json:"fast,omitempty"` FastIgnored bool `json:"fast_ignored,omitempty"` diff --git a/internal/stagemodel/resolver.go b/internal/stagemodel/resolver.go index 20be933d..64df0c3b 100644 --- a/internal/stagemodel/resolver.go +++ b/internal/stagemodel/resolver.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/modelprefs" ) // Stage identifies a durable LLM interaction point in the review system. @@ -95,11 +96,25 @@ func ResolveStageModel(req Request) (Result, error) { Stage: stage, Tier: resolved.Tier, Model: resolved.Model, - Effort: effort, + Effort: applyMaxEffort(req.Profile.LLM, resolved.Tier, effort), Source: resolved.Source, }, nil } +// applyMaxEffort clamps effort to the tier's configured ceiling. Tiers without a +// ceiling, and efforts this CLI does not recognize, pass through unchanged. +func applyMaxEffort(llm config.LLMConfig, tier config.ModelTier, effort string) string { + ceiling, ok := config.ResolveMaxEffort(llm, tier) + if !ok { + return effort + } + requested := modelprefs.Effort(strings.TrimSpace(effort)) + if !requested.Valid() { + return effort + } + return string(modelprefs.MinEffort(requested, ceiling)) +} + // ResolveFirstAvailable resolves the first mapped tier from tiers for req. func ResolveFirstAvailable(req Request, tiers ...config.ModelTier) (Result, bool) { if len(tiers) == 0 { diff --git a/internal/stagemodel/resolver_test.go b/internal/stagemodel/resolver_test.go index 1e930fe0..7b35fa2b 100644 --- a/internal/stagemodel/resolver_test.go +++ b/internal/stagemodel/resolver_test.go @@ -203,3 +203,75 @@ func TestResolveFirstAvailableUsesFirstConfiguredTier(t *testing.T) { t.Fatalf("Effort = %q, want low", got.Effort) } } + +func TestResolveStageModelCapsEffortAtTierCeiling(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"large": "expensive-model", "medium": "cheap-model"}, + MaxEffort: config.EffortMap{"large": "medium"}, + }} + + got, err := ResolveStageModel(Request{ + Profile: profile, + Stage: StageReviewer, + Tier: config.ModelTierLarge, + DefaultEffort: "high", + }) + if err != nil { + t.Fatalf("ResolveStageModel: %v", err) + } + if got.Model != "expensive-model" { + t.Fatalf("Model = %q, want expensive-model", got.Model) + } + if got.Effort != "medium" { + t.Fatalf("Effort = %q, want medium (capped)", got.Effort) + } +} + +func TestResolveStageModelLeavesUncappedTiersUntouched(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"large": "expensive-model", "medium": "cheap-model"}, + MaxEffort: config.EffortMap{"large": "medium"}, + }} + + got, err := ResolveStageModel(Request{ + Profile: profile, + Stage: StageReviewer, + Tier: config.ModelTierMedium, + DefaultEffort: "high", + }) + if err != nil { + t.Fatalf("ResolveStageModel: %v", err) + } + if got.Effort != "high" { + t.Fatalf("Effort = %q, want high (uncapped tier)", got.Effort) + } +} + +func TestResolveStageModelCeilingNeverRaisesEffort(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"large": "expensive-model"}, + MaxEffort: config.EffortMap{"large": "high"}, + }} + + got, err := ResolveStageModel(Request{ + Profile: profile, + Stage: StageReviewer, + Tier: config.ModelTierLarge, + DefaultEffort: "low", + }) + if err != nil { + t.Fatalf("ResolveStageModel: %v", err) + } + if got.Effort != "low" { + t.Fatalf("Effort = %q, want low; ceiling must not raise effort", got.Effort) + } +} diff --git a/internal/view/config.go b/internal/view/config.go index 4c9ac9a4..27ff8866 100644 --- a/internal/view/config.go +++ b/internal/view/config.go @@ -270,7 +270,11 @@ func renderConfigModelMap(w io.Writer, llm config.LLMConfig) error { if model == "" { model = "" } - if _, err := fmt.Fprintf(w, " %s: %s (%s)\n", row.Tier, model, row.Source); err != nil { + suffix := "" + if ceiling := strings.TrimSpace(llm.MaxEffort[row.Tier]); ceiling != "" { + suffix = fmt.Sprintf(" [max effort: %s]", ceiling) + } + if _, err := fmt.Fprintf(w, " %s: %s (%s)%s\n", row.Tier, model, row.Source, suffix); err != nil { return err } }