From db734837271fb399af838bd5b1a7018b892e1a38 Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Wed, 5 Aug 2026 15:13:35 -0400 Subject: [PATCH] feat(config): cap reviewer effort per model tier Agent catalogs declare an absolute effort that becomes the provider's reasoning-effort setting. A deployment that wants to bound spend on an expensive tier previously had only one lever: editing the shared catalog, which changes the agent's declared intent for every consumer. Add llm.max_effort, a per-tier ceiling resolved alongside model_map: max_effort: large: medium A tier absent from the map is uncapped. The cap is a ceiling only, so an agent declaring low under a medium ceiling still runs at low. The clamp lives in stagemodel.ResolveStageModel, the single documented path from profile preferences to a concrete model and effort, so every stage picks it up without per-stage changes. Reviewer resolution previously discarded the resolver's effort and passed agent.Effort straight through, which would have left reviewers - the only path that reaches the large tier - silently uncapped. The resolved effort is now threaded through reviewerRuntimeResolution. cr init has no editor for max_effort, so the runtime round trip is extended to preserve it: dropping the field would silently discard a hand-written ceiling on any later init pass. identityKey now includes the map so two runtimes differing only by ceiling no longer collide. Four paths intentionally bypass the ceiling because each is an explicit selection of a concrete model or effort: --reviewer-effort, --reviewer-model, agent model_id, and benchmark suites, where stages.reviewers.effort is required so candidates stay comparable. README and docs/architecture.md name all four. --- README.md | 34 +++++++++ docs/architecture.md | 7 ++ internal/cmd/initcmd/initcmd.go | 28 ++++++++ .../cmd/initcmd/initcmd_max_effort_test.go | 54 ++++++++++++++ internal/config/config.go | 40 +++++++++++ internal/config/config_max_effort_test.go | 61 ++++++++++++++++ internal/modelprefs/modelprefs.go | 30 ++++++++ internal/modelprefs/modelprefs_effort_test.go | 33 +++++++++ internal/pipeline/pipeline.go | 8 ++- internal/pipeline/pipeline_test.go | 64 +++++++++++++++-- internal/pipeline/prompts.go | 1 + internal/stagemodel/resolver.go | 17 ++++- internal/stagemodel/resolver_test.go | 72 +++++++++++++++++++ internal/view/config.go | 6 +- 14 files changed, 446 insertions(+), 9 deletions(-) create mode 100644 internal/cmd/initcmd/initcmd_max_effort_test.go create mode 100644 internal/config/config_max_effort_test.go create mode 100644 internal/modelprefs/modelprefs_effort_test.go 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 } }