diff --git a/README.md b/README.md index f72885af..2722253d 100644 --- a/README.md +++ b/README.md @@ -634,6 +634,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_runtimes..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` | @@ -664,17 +665,85 @@ 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. +### Model-Tier Floors and Effort Ceilings + +Agent catalogs declare an absolute `effort` (`low`, `medium`, `high`) that +becomes the provider's reasoning-effort setting. For reviewer resolution, +`agent.model_tier` and `llm.reviewer_model_tier` are minimum floors for model +selection. The selected runtime's `max_effort` +(`llm_runtimes..max_effort`) is a ceiling for default effort at the final +resolved tier; it does not raise effort or select a model by itself. + +For reviewer resolution, `cr` applies this order: + +1. Resolve the effective reviewer tier as the higher of the agent tier and the + profile reviewer-tier floor. +2. Resolve `model_map[effective tier]`, including provider built-ins. +3. Cap the default effort with `max_effort[effective tier]`. +4. Apply an explicit effort override, which wins over the ceiling. + +Other tier-resolved internal stages use their own stage tier; `max_effort` is +applied at that stage's final resolved tier, without the reviewer floors. + +Configure ceilings manually in `config.yml`: + +```yaml +llm_runtimes: + review: + provider: openai + auth: subscription + adapter: codex_cli + model_map: + large: openai-codex/gpt-5.6-sol + medium: openai-codex/gpt-5.6-terra + max_effort: + large: medium +profiles: + default: + llm_runtime: review +``` + +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`. Reviewer floors +apply only to reviewer resolution; other tier-resolved internal stages use +their own final tier for the ceiling. + +The complete precedence and bypass table is: + +| Input or path | Model selection | Default effort | `max_effort` on selected runtime | +|---------------|-----------------|----------------|-----------------| +| Reviewer resolution with no explicit override | `max(llm.reviewer_model_tier, agent.model_tier)`, then `model_map` | Agent effort | Caps the default at the final reviewer tier | +| `--reviewer-model-tier` | Raises the reviewer baseline before the agent floor is applied | Agent effort | Caps at the final resolved tier | +| Other tier-resolved internal stage | That stage's own tier, then `model_map` | Stage effort | Caps the default at the stage's final tier | +| `--selection-effort` or `--reviewer-effort` | Normal tier or exact-model selection | Requested effort | Explicit effort wins after the ceiling | +| `--selection-model` or `--reviewer-model` | Exact requested model ID | Stage/agent effort or explicit effort | Bypassed; exact model overrides intentionally bypass tier resolution and the cap | +| Agent `model_id` | Exact agent model ID | Agent effort | Bypassed; exact model selection intentionally bypasses tier resolution and the cap | +| `cr benchmark run` stage model/effort overrides | Exact benchmark model when supplied; otherwise normal tier selection | Explicit benchmark effort when supplied | Explicit benchmark overrides bypass the profile ceiling | + +For example, with `agent.model_tier: small`, `effort: high`, +`llm.reviewer_model_tier: large`, and the selected runtime's +`max_effort.large: medium`, the reviewer +runs with the large model at medium effort. Adding `--reviewer-effort high` +runs that same large model at high effort. Adding +`--reviewer-model my-provider/model` selects that exact model and keeps high +effort without applying the tier ceiling. `--selection-effort high` follows +the same post-ceiling override rule for selection. + +`cr init` preserves runtime `max_effort` but cannot yet edit it; set +`llm_runtimes..max_effort` 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: | Field | Meaning | |-------|---------| -| `mode` | `tier_floor` for portable tier resolution, `exact_model` for agent `model_id` passthrough | +| `mode` | `tier_floor` for portable tier resolution, `exact_model` for agent `model_id` passthrough, or `override` for `--reviewer-model` | | `floor_tier` | Declared agent `model_tier` floor when `mode=tier_floor` | | `baseline_tier` | Effective operator baseline tier used for this run | | `effective_tier` | Higher of baseline and agent floor | -| `resolved_model` | Resolved provider model, from the active model map for `tier_floor` or from agent `model_id` for `exact_model` | +| `resolved_model` | Actual provider model, from the active model map for `tier_floor`, agent `model_id` for `exact_model`, or `--reviewer-model` for `override` | +| `resolved_effort` | Actual reviewer effort after the tier ceiling and any explicit reviewer-effort override | | `model_map_source` | `built_in` or `config` for the resolved tier mapping | Built-in model maps: diff --git a/docs/architecture.md b/docs/architecture.md index 60bb2cd0..582a89c8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -43,28 +43,47 @@ session row, but they still use the same metadata schema and lifecycle runner. Runtime model choice must be resolved through `internal/stagemodel`. Code that executes an LLM stage must not hard-code model IDs and must not call -`config.ResolveModelTier` directly. +`config.ResolveModelTier` or `config.ResolveMaxEffort` directly. `stagemodel.ResolveStageModel` is the single runtime path from profile preferences and command overrides to a concrete model and effort. The request must include the named stage, requested tier, default effort, and any explicit -operator override. The resolver applies user profile `llm.model_map` values, +operator override. The resolver applies the selected runtime's `model_map` values, built-in provider defaults, and configured tier floors before returning the concrete runtime choice. 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. +and reviewer-resolution tier floors can be added without touching individual +review stages. Runtime hard-coding bypasses user preference and is a bug. + +For reviewer tier-based requests, the resolver's authoritative ordering is: +resolve the effective tier after applying the profile reviewer-tier floor and +agent floor; resolve the model for that tier; cap the default effort with the +selected runtime's `max_effort` entry for that final tier; then apply +`EffortOverride`. This means `--reviewer-model-tier` is still capped at the tier +it ultimately resolves, while `--selection-effort` and `--reviewer-effort` win +after the ceiling. Other tier-resolved internal stages use their own stage +tier before applying `max_effort` at that final tier. + +An explicit `ModelOverride` returns with its requested effort or default effort +while intentionally bypassing tier resolution and the `max_effort` cap. +`--selection-model`, `--reviewer-model`, and agent `model_id` use this +exact-model path. Benchmark stage model and effort overrides are explicit +runtime inputs and retain the same ceiling bypass. 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 tier map because the agent author selected a concrete model. -The direct `config.ResolveModelTier` exception is config inspection and the -resolver implementation itself. -`internal/architecture/model_resolution_test.go` enforces that direct -`config.ResolveModelTier` calls stay inside approved packages. Hard-coded +The reviewer execution request, cohort member, session row, and +`agent-sources.json` reviewer provenance must all use the same final resolved +model and effort, including explicit reviewer model and effort overrides. + +Direct `config.ResolveModelTier` and `config.ResolveMaxEffort` calls are +allowed only for config inspection and inside the resolver implementation. +`internal/architecture/model_resolution_test.go` enforces that both direct +calls stay inside approved packages. Hard-coded runtime model IDs remain a code-review concern until model-catalog guardrails exist. diff --git a/docs/init-config-surface.md b/docs/init-config-surface.md index 7b56bc27..7238e7ac 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -181,6 +181,25 @@ Retention is global config under `data.retention`, not profile config. #178 must add retention command tests for omitted/default vs explicit zero. #184 must use the same validation and reset behavior in interactive init. +## LLM Effort Ceiling Ownership + +The canonical ceiling path is `llm_runtimes..max_effort`, and +`profiles..llm_runtime` selects that runtime. The legacy +`profiles..llm.max_effort` path is compatibility/projection only, not the +canonical storage location. The map accepts `small`, `medium`, and `large` +tier keys with `low`, `medium`, or `high` ceiling values. Interactive and +non-interactive `cr init` must preserve an existing map, including when the +profile or selected LLM runtime is staged and saved; init does not edit or +remove it. Configure it by editing `config.yml` directly. Model-map JSON-row +parity and init editing for this field are out of scope. + +At review time, reviewer floors apply only to reviewer resolution. Other +tier-resolved internal stages use their own stage tier, and default effort is +capped only after that final tier is resolved. Explicit `--selection-effort` +and `--reviewer-effort` values win after the cap. Exact +`--selection-model`, `--reviewer-model`, agent `model_id`, and benchmark stage +model/effort overrides intentionally bypass tier resolution and the cap. + ## Scripted Install Ownership Scripted installs should remain readable. The intended shape is: diff --git a/internal/architecture/model_resolution_test.go b/internal/architecture/model_resolution_test.go index 4462f877..6a6b1b5e 100644 --- a/internal/architecture/model_resolution_test.go +++ b/internal/architecture/model_resolution_test.go @@ -53,7 +53,7 @@ func TestRuntimeModelResolutionGoesThroughStageResolver(t *testing.T) { return true } selector, ok := call.Fun.(*ast.SelectorExpr) - if !ok || selector.Sel.Name != "ResolveModelTier" { + if !ok || selector.Sel.Name != "ResolveModelTier" && selector.Sel.Name != "ResolveMaxEffort" { return true } ident, ok := selector.X.(*ast.Ident) @@ -61,7 +61,7 @@ func TestRuntimeModelResolutionGoesThroughStageResolver(t *testing.T) { return true } pos := fset.Position(selector.Pos()) - t.Fatalf("%s calls config.ResolveModelTier directly; runtime model selection must use internal/stagemodel", pos) + t.Fatalf("%s calls config.%s directly; runtime model and effort resolution must use internal/stagemodel", pos, selector.Sel.Name) return false }) return nil diff --git a/internal/cmd/initcmd/initcmd.go b/internal/cmd/initcmd/initcmd.go index a82f1b99..8467c2df 100644 --- a/internal/cmd/initcmd/initcmd.go +++ b/internal/cmd/initcmd/initcmd.go @@ -164,6 +164,8 @@ type initDraft struct { Routes []configedit.RepositoryRouteSpec ModelMapSet bool ModelMap config.ModelMap + MaxEffortSet bool + MaxEffort config.EffortMap AgentSourcesSet bool AgentSources []string ReviewPolicySet bool @@ -430,6 +432,7 @@ type initLLMRuntimeDraft struct { CredentialStore string CredentialRef string ModelMap config.ModelMap + MaxEffort config.EffortMap ReviewerModelTier config.ModelTier } @@ -1116,6 +1119,12 @@ func completeInteractiveInitProfileV2Draft(ctx initPromptContext, draft initDraf } draft.ModelMapSet = true } + if !draft.MaxEffortSet { + if ctx.ExistingProfile != nil { + draft.MaxEffort = copyEffortMap(ctx.ExistingProfile.LLM.MaxEffort) + } + draft.MaxEffortSet = true + } if !draft.AgentSourcesSet { if ctx.ExistingProfile != nil { draft.AgentSources = append([]string(nil), ctx.ExistingProfile.AgentSources...) @@ -2333,10 +2342,11 @@ func initReviewerModelTierOptions() []huh.Option[string] { func initProfileEditorModelMapLLM(draft initDraft, selectedLLMRuntime string, runtimes map[string]initLLMRuntimeDraft) config.LLMConfig { llm := config.LLMConfig{ - Provider: config.LLMProvider(draft.LLMProvider), - Auth: config.LLMAuth(draft.LLMAuth), - Adapter: config.LLMAdapter(draft.LLMAdapter), - ModelMap: copyModelMap(draft.ModelMap), + Provider: config.LLMProvider(draft.LLMProvider), + Auth: config.LLMAuth(draft.LLMAuth), + Adapter: config.LLMAdapter(draft.LLMAdapter), + ModelMap: copyModelMap(draft.ModelMap), + MaxEffort: copyEffortMap(draft.MaxEffort), } if runtime, ok := runtimes[selectedLLMRuntime]; ok { llm.Provider = runtime.Provider @@ -2540,6 +2550,7 @@ func initLLMRuntimeDraftFromSeedDraft(draft initDraft) initLLMRuntimeDraft { Adapter: config.LLMAdapter(draft.LLMAdapter), Credential: initCredentialLocationIfName(draft.LLMCredentialStore, draft.LLMCredentialRef), ModelMap: copyModelMap(draft.ModelMap), + MaxEffort: copyEffortMap(draft.MaxEffort), ReviewerModelTier: config.ModelTier(strings.TrimSpace(draft.LLMReviewerModelTier)), }) } @@ -2699,6 +2710,8 @@ func applyLLMRuntimeInventorySelection(draft *initDraft, selection string, runti draft.LLMAdapter = string(runtime.Adapter) draft.ModelMap = copyModelMap(runtime.ModelMap) draft.ModelMapSet = true + draft.MaxEffort = copyEffortMap(runtime.MaxEffort) + draft.MaxEffortSet = true draft.LLMReviewerModelTier = string(runtime.ReviewerModelTier) if !draft.AdvancedStorageLabels { draft.LLMCredentialStore = initCredentialStoreDraftValue(runtime.CredentialStore) @@ -3105,6 +3118,8 @@ func seedInteractiveInitDraft(requestedProfileName string, existingProfileName s draft.LLMCredentialStore = initCredentialStoreDraftValue(existingProfile.LLM.Credential.Store) draft.LLMCredentialRef = existingProfile.LLM.Credential.Name draft.ModelMap = copyModelMap(existingProfile.LLM.ModelMap) + draft.MaxEffort = copyEffortMap(existingProfile.LLM.MaxEffort) + draft.MaxEffortSet = true draft.AgentSources = append([]string(nil), existingProfile.AgentSources...) draft.ReviewPolicy = existingProfile.ReviewPolicy if existingProfile.Reviewer.GitHubAppInstallation != nil { @@ -3404,6 +3419,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 +4559,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 +4578,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 +4597,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 +4613,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 +4792,7 @@ func cloneInitLLMConfig(llm config.LLMConfig) config.LLMConfig { cloned.ModelMap[tier] = model } } + cloned.MaxEffort = copyEffortMap(llm.MaxEffort) return cloned } @@ -4848,6 +4879,9 @@ func synthesizeInteractiveProfile(flags initOptions, profileName string, previou profile.LLM.Auth = config.LLMAuth(draft.LLMAuth) profile.LLM.Adapter = config.LLMAdapter(draft.LLMAdapter) profile.LLM.ReviewerModelTier = config.ModelTier(strings.TrimSpace(draft.LLMReviewerModelTier)) + if draft.MaxEffortSet { + profile.LLM.MaxEffort = copyEffortMap(draft.MaxEffort) + } if profile.LLM.Auth == config.LLMAuthAPIKey { llmRef := strings.TrimSpace(draft.LLMCredentialRef) if llmRef == "" { @@ -5814,6 +5848,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..7d91f3ac --- /dev/null +++ b/internal/cmd/initcmd/initcmd_max_effort_test.go @@ -0,0 +1,88 @@ +package initcmd + +import ( + "os" + "path/filepath" + "strings" + "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) + } +} + +func TestInitNonInteractivePreservesMaxEffortThroughConfigRoundTrip(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yml") + existing := basicProfile("work") + existing.LLM.ModelMap = config.ModelMap{"large": "gpt-5.6-sol"} + existing.LLM.MaxEffort = config.EffortMap{"large": "medium"} + if err := config.Save(path, config.File{Profiles: map[string]config.Profile{"work": existing}}); err != nil { + t.Fatalf("Save initial config: %v", err) + } + data, err := os.ReadFile(path) // #nosec G304 -- test path is controlled by t.TempDir. + if err != nil { + t.Fatalf("Read initial config: %v", err) + } + if !strings.Contains(string(data), "max_effort:") { + t.Fatalf("initial config = %q, want max_effort YAML", data) + } + + flags := defaultNonInteractiveInitOptionsForTest() + flags.replaceProfile = true + _, _, err = runNonInteractiveInitWithFakeStore(t, path, "work", strings.NewReader(""), flags, newFakeInitStore(nil)) + if err != nil { + t.Fatalf("non-interactive init: %v", err) + } + loaded, err := config.Load(path) + if err != nil { + t.Fatalf("Load saved config: %v", err) + } + if got := loaded.Profiles["work"].LLM.MaxEffort["large"]; got != "medium" { + t.Fatalf("saved max_effort.large = %q, want medium", got) + } +} diff --git a/internal/cmd/initcmd/initcmd_test.go b/internal/cmd/initcmd/initcmd_test.go index 0b7c2330..2ee43f55 100644 --- a/internal/cmd/initcmd/initcmd_test.go +++ b/internal/cmd/initcmd/initcmd_test.go @@ -6501,6 +6501,7 @@ func TestLoopInteractiveInitProfileV2DoesNotPromptForSelectedPrimitiveCredential func TestLoopInteractiveInitProfileV2AppliesInlineDetailDraftParity(t *testing.T) { path := filepath.Join(t.TempDir(), "config.yml") existing := basicProfile("work") + existing.LLM.MaxEffort = config.EffortMap{"large": "medium"} cfg := config.File{ Profiles: map[string]config.Profile{ "work": existing, @@ -6577,6 +6578,9 @@ func TestLoopInteractiveInitProfileV2AppliesInlineDetailDraftParity(t *testing.T if !reflect.DeepEqual(profile.LLM.ModelMap, config.ModelMap{"medium": "gpt-custom"}) { t.Fatalf("model_map = %#v, want v2 model-map edit", profile.LLM.ModelMap) } + if profile.LLM.MaxEffort["large"] != "medium" { + t.Fatalf("max_effort = %#v, want preserved large=medium", profile.LLM.MaxEffort) + } if !reflect.DeepEqual(profile.AgentSources, []string{"/tmp/agents"}) { t.Fatalf("agent_sources = %#v, want normalized v2 agent sources", profile.AgentSources) } diff --git a/internal/config/config.go b/internal/config/config.go index 1989bb37..e001f0cc 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 @@ -689,6 +696,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 @@ -1416,6 +1444,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) } @@ -1942,6 +1982,15 @@ func llmRuntimeIdentityKey(llm LLMConfig) string { for _, tier := range modelKeys { models = append(models, tier+"="+strings.TrimSpace(llm.ModelMap[tier])) } + effortKeys := make([]string, 0, len(llm.MaxEffort)) + for tier := range llm.MaxEffort { + effortKeys = append(effortKeys, tier) + } + sort.Strings(effortKeys) + efforts := make([]string, 0, len(effortKeys)) + for _, tier := range effortKeys { + efforts = append(efforts, tier+"="+strings.TrimSpace(llm.MaxEffort[tier])) + } return strings.Join([]string{ string(llm.Provider), string(llm.Auth), @@ -1949,6 +1998,7 @@ func llmRuntimeIdentityKey(llm LLMConfig) string { llm.Credential.Store, llm.Credential.Name, strings.Join(models, "\x1f"), + strings.Join(efforts, "\x1f"), string(llm.ReviewerModelTier), }, "\x00") } @@ -2245,6 +2295,13 @@ func (l LLMConfig) normalized() LLMConfig { } l.ModelMap = modelMap } + if len(l.MaxEffort) > 0 { + maxEffort := make(EffortMap, len(l.MaxEffort)) + for tier, effort := range l.MaxEffort { + maxEffort[strings.TrimSpace(tier)] = strings.TrimSpace(effort) + } + l.MaxEffort = maxEffort + } return l } @@ -2254,6 +2311,7 @@ func (l LLMConfig) empty() bool { strings.TrimSpace(string(l.Adapter)) == "" && l.Credential.empty() && len(l.ModelMap) == 0 && + len(l.MaxEffort) == 0 && strings.TrimSpace(string(l.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..09cc175b --- /dev/null +++ b/internal/config/config_max_effort_test.go @@ -0,0 +1,101 @@ +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") + } +} + +func TestLLMConfigNormalizedTrimsAndCopiesMaxEffort(t *testing.T) { + original := LLMConfig{MaxEffort: EffortMap{" large ": " medium "}} + normalized := original.normalized() + if got := normalized.MaxEffort["large"]; got != "medium" { + t.Fatalf("normalized max_effort = %q, want trimmed medium", got) + } + normalized.MaxEffort["large"] = "high" + + if got := original.MaxEffort[" large "]; got != " medium " { + t.Fatalf("original max_effort changed through normalized copy: %q", got) + } + if got := normalized.MaxEffort["large"]; got != "high" { + t.Fatalf("normalized max_effort = %q, want independent copy", got) + } +} + +func TestNormalizeProjectsInlineRuntimesWithDistinctMaxEffort(t *testing.T) { + base := Profile{LLM: LLMConfig{ + Provider: LLMProviderOpenAI, + Auth: LLMAuthSubscription, + Adapter: LLMAdapterCodexCLI, + ModelMap: ModelMap{"large": "sol"}, + }} + capped := base + capped.LLM.MaxEffort = EffortMap{"large": "medium"} + + normalized := Normalize(File{Profiles: map[string]Profile{ + "base": base, + "capped": capped, + }}) + baseRuntime := normalized.Profiles["base"].LLMRuntime + cappedRuntime := normalized.Profiles["capped"].LLMRuntime + if baseRuntime == "" || cappedRuntime == "" || baseRuntime == cappedRuntime { + t.Fatalf("inline runtime identities = %q/%q, want distinct runtimes", baseRuntime, cappedRuntime) + } + if got := normalized.LLMRuntimes[cappedRuntime].MaxEffort["large"]; got != "medium" { + t.Fatalf("capped runtime max_effort = %q, want medium", got) + } +} 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/artifacts.go b/internal/pipeline/artifacts.go index 901c37a4..7e515cb6 100644 --- a/internal/pipeline/artifacts.go +++ b/internal/pipeline/artifacts.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "path/filepath" - "strings" "github.com/open-cli-collective/codereview-cli/internal/agents" "github.com/open-cli-collective/codereview-cli/internal/fsatomic" @@ -136,16 +135,6 @@ func reviewerRuntimeArtifact(req Request, catalog agents.Catalog, selection llm. if fastRequested && fastDelivered != "fast" && fastDelivered != "standard" { fastDelivered = "unknown" } - if strings.TrimSpace(req.ReviewerModelOverride) != "" { - if !fastRequested { - return nil - } - out := make(map[string]reviewerRuntimeResolution, len(selection.SelectedAgents)) - for _, selected := range selection.SelectedAgents { - out[selected.AgentID] = reviewerRuntimeResolution{Mode: "override", ResolvedModel: strings.TrimSpace(req.ReviewerModelOverride), Fast: true, FastIgnored: fastIgnored, FastDelivered: fastDelivered} - } - return out - } if len(selection.SelectedAgents) == 0 { return nil } @@ -159,7 +148,7 @@ func reviewerRuntimeArtifact(req Request, catalog agents.Catalog, selection llm. if !ok { continue } - resolution, err := resolveAgentModel(req.Profile, req.ReviewerModelTierOverride, agent) + resolution, err := resolveReviewerRuntime(req, agent) if err != nil { continue } diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 841b89c3..cbabd0a0 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -3128,6 +3128,14 @@ func resolveSynthesisRuntimeConfig(req Request) (llmRuntimeConfig, error) { } func resolveReviewerRuntimeConfig(req Request, agent agents.Agent) (llmRuntimeConfig, error) { + resolved, err := resolveReviewerRuntime(req, agent) + if err != nil { + return llmRuntimeConfig{}, err + } + return llmRuntimeConfig{model: resolved.ResolvedModel, effort: resolved.ResolvedEffort}, nil +} + +func resolveReviewerRuntime(req Request, agent agents.Agent) (reviewerRuntimeResolution, error) { if strings.TrimSpace(req.ReviewerModelOverride) != "" { resolved, err := stagemodel.ResolveStageModel(stagemodel.Request{ Profile: req.Profile, @@ -3137,15 +3145,22 @@ func resolveReviewerRuntimeConfig(req Request, agent agents.Agent) (llmRuntimeCo DefaultEffort: agent.Effort, }) if err != nil { - return llmRuntimeConfig{}, err + return reviewerRuntimeResolution{}, err } - return llmRuntimeConfig{model: resolved.Model, effort: resolved.Effort}, nil + return reviewerRuntimeResolution{ + Mode: "override", + ResolvedModel: resolved.Model, + ResolvedEffort: resolved.Effort, + }, nil } resolved, err := resolveAgentModel(req.Profile, req.ReviewerModelTierOverride, agent) if err != nil { - return llmRuntimeConfig{}, err + return reviewerRuntimeResolution{}, err + } + if effort := strings.TrimSpace(req.ReviewerEffortOverride); effort != "" { + resolved.ResolvedEffort = effort } - return applyStageRuntimeOverrides(req.ReviewerModelOverride, req.ReviewerEffortOverride, resolved.ResolvedModel, agent.Effort), nil + return resolved, nil } func resolveReviewerFastMode(req Request, catalog agents.Catalog) (bool, string, error) { @@ -3182,8 +3197,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)) @@ -3210,6 +3226,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 } @@ -3232,16 +3249,6 @@ func resolveReviewerBaselineTier(profile config.Profile, override string) (confi return tier, nil } -func applyStageRuntimeOverrides(modelOverride, effortOverride, model, effort string) llmRuntimeConfig { - if override := strings.TrimSpace(modelOverride); override != "" { - model = override - } - if override := strings.TrimSpace(effortOverride); override != "" { - effort = override - } - return llmRuntimeConfig{model: model, effort: effort} -} - func sameIdentity(left, right gitprovider.Identity) bool { if strings.TrimSpace(left.ID) != "" && strings.TrimSpace(right.ID) != "" { return left.ID == right.ID diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 2cbb814b..e682619d 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1879,6 +1879,7 @@ func TestDryRunReviewerBaselineTierRaisesReviewerModelFloor(t *testing.T) { BaselineTier: "large", EffectiveTier: "large", ResolvedModel: "profile-large-model", + ResolvedEffort: "medium", ModelMapSource: config.ModelMapSourceConfig, }) } @@ -2074,19 +2075,19 @@ func TestDryRunSelectionOverridesApplyOnlyToSelection(t *testing.T) { modelOverride: "bench-model", effortOverride: "high", wantModels: []string{"bench-model", "claude-sonnet-5", "claude-sonnet-5"}, - wantEfforts: []string{"high", "medium", "medium"}, + wantEfforts: []string{"high", "low", "low"}, }, { name: "model only", modelOverride: "bench-model", wantModels: []string{"bench-model", "claude-sonnet-5", "claude-sonnet-5"}, - wantEfforts: []string{"medium", "medium", "medium"}, + wantEfforts: []string{"medium", "low", "low"}, }, { name: "effort only", effortOverride: "high", wantModels: []string{"claude-sonnet-5", "claude-sonnet-5", "claude-sonnet-5"}, - wantEfforts: []string{"high", "medium", "medium"}, + wantEfforts: []string{"high", "low", "low"}, }, } for _, tt := range tests { @@ -2095,6 +2096,7 @@ func TestDryRunSelectionOverridesApplyOnlyToSelection(t *testing.T) { store := openPipelineStore(t) defer closeStore(t, store) provider, req := dryRunHarness(t) + req.Profile.LLM.MaxEffort = config.EffortMap{"medium": "low"} req.SelectionModelOverride = tt.modelOverride req.SelectionEffortOverride = tt.effortOverride adapter := &llm.FakeAdapter{NameValue: "fake-llm"} @@ -2156,8 +2158,8 @@ func TestDryRunReviewerOverridesApplyOnlyToReviewers(t *testing.T) { store := openPipelineStore(t) defer closeStore(t, store) provider, req := dryRunHarness(t) - req.ReviewerModelOverride = "bench-reviewer-model" - req.ReviewerEffortOverride = "low" + req.Profile.LLM.MaxEffort = config.EffortMap{"medium": "low"} + req.ReviewerEffortOverride = "high" adapter := &llm.FakeAdapter{NameValue: "fake-llm"} adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) @@ -2179,8 +2181,8 @@ func TestDryRunReviewerOverridesApplyOnlyToReviewers(t *testing.T) { t.Fatalf("DryRun: %v", err) } - wantModels := []string{"claude-sonnet-5", "bench-reviewer-model", "claude-sonnet-5"} - wantEfforts := []string{"medium", "low", "medium"} + wantModels := []string{"claude-sonnet-5", "claude-sonnet-5", "claude-sonnet-5"} + wantEfforts := []string{"low", "high", "low"} requests := adapter.Requests() for i, request := range requests { if request.Model != wantModels[i] || request.Effort != wantEfforts[i] { @@ -2196,6 +2198,15 @@ func TestDryRunReviewerOverridesApplyOnlyToReviewers(t *testing.T) { t.Fatalf("session[%d] = model:%q effort:%v, want %s/%s", i, session.Model, session.Effort, wantModels[i], wantEfforts[i]) } } + assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ + Mode: "tier_floor", + FloorTier: "medium", + BaselineTier: "small", + EffectiveTier: "medium", + ResolvedModel: "claude-sonnet-5", + ResolvedEffort: "high", + ModelMapSource: config.ModelMapSourceBuiltIn, + }) } func TestDryRunReviewerFailureIsolation(t *testing.T) { @@ -2707,6 +2718,7 @@ func TestDryRunReviewerModelTierOverrideAppliesOnlyToReviewers(t *testing.T) { defer closeStore(t, store) provider, req := dryRunHarness(t) req.Profile.LLM.ModelMap = config.ModelMap{"large": "profile-large-model"} + req.Profile.LLM.MaxEffort = config.EffortMap{"large": "low"} req.ReviewerModelTierOverride = "large" adapter := &llm.FakeAdapter{NameValue: "fake-llm"} adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) @@ -2730,9 +2742,10 @@ func TestDryRunReviewerModelTierOverrideAppliesOnlyToReviewers(t *testing.T) { } wantModels := []string{"claude-sonnet-5", "profile-large-model", "claude-sonnet-5"} + wantEfforts := []string{"medium", "low", "medium"} for i, request := range adapter.Requests() { - if request.Model != wantModels[i] { - t.Fatalf("request[%d].Model = %q, want %q", i, request.Model, wantModels[i]) + if request.Model != wantModels[i] || request.Effort != wantEfforts[i] { + t.Fatalf("request[%d] = model:%q effort:%q, want %s/%s", i, request.Model, request.Effort, wantModels[i], wantEfforts[i]) } } assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ @@ -2741,6 +2754,7 @@ func TestDryRunReviewerModelTierOverrideAppliesOnlyToReviewers(t *testing.T) { BaselineTier: "large", EffectiveTier: "large", ResolvedModel: "profile-large-model", + ResolvedEffort: "low", ModelMapSource: config.ModelMapSourceConfig, }) } @@ -2792,8 +2806,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", }) } @@ -2833,8 +2848,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", }) } @@ -2863,6 +2879,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) @@ -2873,6 +2890,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) @@ -2913,13 +2931,67 @@ func TestDryRunReviewerModelOverrideBypassesAgentModelID(t *testing.T) { t.Fatalf("request[%d] = model:%q effort:%q, want %s/medium", i, request.Model, request.Effort, wantModels[i]) } } - data, err := os.ReadFile(result.Artifacts.AgentSourcesJSON) // #nosec G304 -- test reads artifact paths returned by the pipeline under t.TempDir. + assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ + Mode: "override", + ResolvedModel: "override-model", + ResolvedEffort: "medium", + }) +} + +func TestDryRunReviewerModelAndEffortOverridesBypassMaxEffortProvenance(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + req.Profile.LLM.MaxEffort = config.EffortMap{"medium": "low"} + req.ReviewerModelOverride = "override-model" + req.ReviewerEffortOverride = "high" + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) + adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) + + result, err := dryRunForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewRunID: func() string { return "run-reviewer-model-effort-override" }, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, + }, req) if err != nil { - t.Fatalf("ReadFile(%s): %v", result.Artifacts.AgentSourcesJSON, err) + t.Fatalf("DryRun: %v", err) } - if strings.Contains(string(data), "override-model") { - t.Fatalf("agent source artifact contains runtime override model: %s", data) + + requests := adapter.Requests() + if len(requests) != 3 { + t.Fatalf("requests len = %d, want selection/reviewer/rollup", len(requests)) + } + if request := requests[1]; request.Model != "override-model" || request.Effort != "high" { + t.Fatalf("reviewer request = model:%q effort:%q, want override-model/high", request.Model, request.Effort) } + + sessions, err := store.ListSessionsForRun(ctx, result.Run.RunID) + if err != nil { + t.Fatalf("ListSessionsForRun: %v", err) + } + reviewerSession, ok := sessionWithProviderID(sessions, "reviewer-session") + if !ok { + t.Fatalf("sessions = %#v, want reviewer-session", sessions) + } + if reviewerSession.Model != "override-model" || reviewerSession.Effort == nil || *reviewerSession.Effort != "high" { + t.Fatalf("reviewer session = model:%q effort:%v, want override-model/high", reviewerSession.Model, reviewerSession.Effort) + } + + assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ + Mode: "override", + ResolvedModel: "override-model", + ResolvedEffort: "high", + }) } func TestDryRunFastAppliesOnlyToReviewerAndRecordsArtifact(t *testing.T) { @@ -2956,11 +3028,12 @@ func TestDryRunFastAppliesOnlyToReviewerAndRecordsArtifact(t *testing.T) { t.Fatalf("requests = %#v, want fast only on reviewer", requests) } assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ - Mode: "override", - ResolvedModel: "claude-opus-4-8", - Fast: true, - FastIgnored: false, - FastDelivered: "standard", + Mode: "override", + ResolvedModel: "claude-opus-4-8", + ResolvedEffort: "medium", + Fast: true, + FastIgnored: false, + FastDelivered: "standard", }) } @@ -3030,6 +3103,7 @@ func TestDryRunFastFallsBackForUnsupportedModel(t *testing.T) { BaselineTier: "small", EffectiveTier: "medium", ResolvedModel: "claude-sonnet-5", + ResolvedEffort: "medium", ModelMapSource: config.ModelMapSourceBuiltIn, Fast: true, FastIgnored: true, @@ -3108,11 +3182,12 @@ func TestDryRunFastFallsBackForUnsupportedRuntime(t *testing.T) { t.Fatalf("requests = %#v, want normal-speed fallback", requests) } assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ - Mode: "override", - ResolvedModel: "pi-model", - Fast: true, - FastIgnored: true, - FastDelivered: "unknown", + Mode: "override", + ResolvedModel: "pi-model", + ResolvedEffort: "medium", + Fast: true, + FastIgnored: true, + FastDelivered: "unknown", }) } @@ -7435,3 +7510,62 @@ 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) + } + gotOverride, err := resolveReviewerRuntimeConfig(Request{ + Profile: profile, + ReviewerEffortOverride: "high", + }, large) + if err != nil { + t.Fatalf("resolveReviewerRuntimeConfig(override): %v", err) + } + if gotOverride.model != "sol" || gotOverride.effort != "high" { + t.Fatalf("overridden reviewer = %+v, want model sol effort high", gotOverride) + } + + 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 6d867146..4d201cb9 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. @@ -59,11 +60,12 @@ func ResolveStageModel(req Request) (Result, error) { return Result{}, fmt.Errorf("stagemodel: stage %q is invalid", req.Stage) } tier := config.ModelTier(strings.TrimSpace(string(req.Tier))) - effort := strings.TrimSpace(req.EffortOverride) - if effort == "" { - effort = strings.TrimSpace(req.DefaultEffort) - } + effortOverride := strings.TrimSpace(req.EffortOverride) if model := strings.TrimSpace(req.ModelOverride); model != "" { + effort := strings.TrimSpace(req.DefaultEffort) + if effortOverride != "" { + effort = effortOverride + } return Result{ Stage: stage, Tier: tier, @@ -91,6 +93,10 @@ func ResolveStageModel(req Request) (Result, error) { llmConfig := req.Profile.LLM return Result{}, fmt.Errorf("stagemodel: stage %s: model_tier %q is not mapped for provider %q adapter %q; add llm.model_map.%s to the profile's LLM runtime", stage, tier, llmConfig.Provider, llmConfig.Adapter, tier) } + effort := applyMaxEffort(req.Profile.LLM, resolved.Tier, strings.TrimSpace(req.DefaultEffort)) + if effortOverride != "" { + effort = effortOverride + } return Result{ Stage: stage, Tier: resolved.Tier, @@ -100,6 +106,20 @@ func ResolveStageModel(req Request) (Result, error) { }, 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 cad301c7..6a7357ce 100644 --- a/internal/stagemodel/resolver_test.go +++ b/internal/stagemodel/resolver_test.go @@ -46,9 +46,10 @@ func TestResolveStageModelUsesConfiguredTierMapping(t *testing.T) { func TestResolveStageModelAppliesEffortOverrideWithoutBypassingTier(t *testing.T) { profile := config.Profile{LLM: config.LLMConfig{ - Provider: config.LLMProviderOpenAI, - Auth: config.LLMAuthSubscription, - Adapter: config.LLMAdapterCodexCLI, + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + MaxEffort: config.EffortMap{"medium": "low"}, }} got, err := ResolveStageModel(Request{ @@ -106,17 +107,19 @@ func TestResolveStageModelAppliesTierFloor(t *testing.T) { func TestResolveStageModelBypassesTierForExplicitOverride(t *testing.T) { profile := config.Profile{LLM: config.LLMConfig{ - Provider: config.LLMProviderPi, - Auth: config.LLMAuthSubscription, - Adapter: config.LLMAdapterPiRPC, + Provider: config.LLMProviderPi, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterPiRPC, + MaxEffort: config.EffortMap{"large": "medium"}, }} got, err := ResolveStageModel(Request{ - Profile: profile, - Stage: StageThreadAnalysis, - Tier: config.ModelTierLarge, - ModelOverride: "operator-chosen-model", - DefaultEffort: "low", + Profile: profile, + Stage: StageThreadAnalysis, + Tier: config.ModelTierLarge, + ModelOverride: "operator-chosen-model", + EffortOverride: "high", + DefaultEffort: "low", }) if err != nil { t.Fatalf("ResolveStageModel: %v", err) @@ -124,8 +127,8 @@ func TestResolveStageModelBypassesTierForExplicitOverride(t *testing.T) { if got.Model != "operator-chosen-model" { t.Fatalf("Model = %q, want operator-chosen-model", got.Model) } - if got.Effort != "low" { - t.Fatalf("Effort = %q, want low", got.Effort) + if got.Effort != "high" { + t.Fatalf("Effort = %q, want high", got.Effort) } if !got.Override { t.Fatalf("Override = false, want true") @@ -232,3 +235,99 @@ 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 TestResolveStageModelCapsUsingPostFloorTier(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": "medium"}, + }} + + got, err := ResolveStageModel(Request{ + Profile: profile, + Stage: StageReviewer, + Tier: config.ModelTierSmall, + FloorTier: config.ModelTierLarge, + DefaultEffort: "high", + }) + if err != nil { + t.Fatalf("ResolveStageModel: %v", err) + } + if got.Tier != config.ModelTierLarge || got.Model != "expensive-model" || got.Effort != "medium" { + t.Fatalf("resolved = %#v, want post-floor large model with medium effort", got) + } +} + +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 } } diff --git a/internal/view/config_test.go b/internal/view/config_test.go index 9eb52565..2d3def2f 100644 --- a/internal/view/config_test.go +++ b/internal/view/config_test.go @@ -190,7 +190,9 @@ func TestRenderConfigTextAgentSourceStatus(t *testing.T) { func TestRenderConfigTextExactHomeShape(t *testing.T) { var out bytes.Buffer - show := NewConfigShow("home", homeProfile(), dataConfig(), []CredentialStatus{ + profile := homeProfile() + profile.LLM.MaxEffort = config.EffortMap{"medium": "low"} + show := NewConfigShow("home", profile, dataConfig(), []CredentialStatus{ credentialStatus("git", "codereview/home", "pat", "git_token", false), }) @@ -210,7 +212,7 @@ LLM: Credential name: adapter-managed; not stored by cr Model map: small: claude-haiku-4-5 (built_in) - medium: claude-sonnet-5 (built_in) + medium: claude-sonnet-5 (built_in) [max effort: low] large: claude-opus-5 (built_in) Credentials: - git: codereview/home (pat)