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 1/7] 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 } } From d08ca4072a0cbe0b6d7903e56fad1d9859bdcd6c Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 12:17:46 -0400 Subject: [PATCH 2/7] fix: enforce max effort ceilings consistently --- internal/cmd/initcmd/initcmd.go | 25 +++++- .../cmd/initcmd/initcmd_max_effort_test.go | 34 +++++++++ internal/cmd/initcmd/initcmd_test.go | 4 + internal/config/config.go | 18 +++++ internal/config/config_max_effort_test.go | 40 ++++++++++ internal/pipeline/artifacts.go | 13 +--- internal/pipeline/pipeline.go | 33 ++++---- internal/pipeline/pipeline_test.go | 76 ++++++++++++------- internal/stagemodel/resolver.go | 15 ++-- internal/stagemodel/resolver_test.go | 53 +++++++++---- internal/view/config_test.go | 6 +- 11 files changed, 240 insertions(+), 77 deletions(-) diff --git a/internal/cmd/initcmd/initcmd.go b/internal/cmd/initcmd/initcmd.go index 0ba4c488..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 @@ -1117,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...) @@ -2334,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 @@ -2541,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)), }) } @@ -2700,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) @@ -3106,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 { @@ -4865,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 == "" { diff --git a/internal/cmd/initcmd/initcmd_max_effort_test.go b/internal/cmd/initcmd/initcmd_max_effort_test.go index 7735508f..7d91f3ac 100644 --- a/internal/cmd/initcmd/initcmd_max_effort_test.go +++ b/internal/cmd/initcmd/initcmd_max_effort_test.go @@ -1,6 +1,9 @@ package initcmd import ( + "os" + "path/filepath" + "strings" "testing" "github.com/open-cli-collective/codereview-cli/internal/config" @@ -52,3 +55,34 @@ func TestCloneInitLLMConfigDeepCopiesMaxEffort(t *testing.T) { 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 95576b92..e001f0cc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1982,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), @@ -1989,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") } @@ -2285,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 } @@ -2294,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 index 331a4ec4..09cc175b 100644 --- a/internal/config/config_max_effort_test.go +++ b/internal/config/config_max_effort_test.go @@ -59,3 +59,43 @@ func TestResolveMaxEffortReportsUncappedTiers(t *testing.T) { 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/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 544e9ccc..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 } - return applyStageRuntimeOverrides(req.ReviewerModelOverride, req.ReviewerEffortOverride, resolved.ResolvedModel, resolved.ResolvedEffort), nil + if effort := strings.TrimSpace(req.ReviewerEffortOverride); effort != "" { + resolved.ResolvedEffort = effort + } + return resolved, nil } func resolveReviewerFastMode(req Request, catalog agents.Catalog) (bool, string, error) { @@ -3234,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 9d3b0dbe..ffb0ac85 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -2075,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 { @@ -2096,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"} @@ -2157,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)) @@ -2180,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] { @@ -2197,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) { @@ -2708,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)) @@ -2731,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{ @@ -2742,7 +2754,7 @@ func TestDryRunReviewerModelTierOverrideAppliesOnlyToReviewers(t *testing.T) { BaselineTier: "large", EffectiveTier: "large", ResolvedModel: "profile-large-model", - ResolvedEffort: "medium", + ResolvedEffort: "low", ModelMapSource: config.ModelMapSourceConfig, }) } @@ -2919,13 +2931,11 @@ 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. - if err != nil { - t.Fatalf("ReadFile(%s): %v", result.Artifacts.AgentSourcesJSON, err) - } - if strings.Contains(string(data), "override-model") { - t.Fatalf("agent source artifact contains runtime override model: %s", data) - } + assertReviewerRuntimeArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer", reviewerRuntimeResolution{ + Mode: "override", + ResolvedModel: "override-model", + ResolvedEffort: "medium", + }) } func TestDryRunFastAppliesOnlyToReviewerAndRecordsArtifact(t *testing.T) { @@ -2962,11 +2972,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", }) } @@ -3115,11 +3126,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", }) } @@ -7461,6 +7473,16 @@ func TestReviewerRuntimeConfigCapsEffortAtConfiguredTierCeiling(t *testing.T) { 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 { diff --git a/internal/stagemodel/resolver.go b/internal/stagemodel/resolver.go index 01c1ca20..4d201cb9 100644 --- a/internal/stagemodel/resolver.go +++ b/internal/stagemodel/resolver.go @@ -60,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, @@ -92,11 +93,15 @@ 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, Model: resolved.Model, - Effort: applyMaxEffort(req.Profile.LLM, resolved.Tier, effort), + Effort: effort, Source: resolved.Source, }, nil } diff --git a/internal/stagemodel/resolver_test.go b/internal/stagemodel/resolver_test.go index 6e63a264..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") @@ -259,6 +262,30 @@ func TestResolveStageModelCapsEffortAtTierCeiling(t *testing.T) { } } +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, 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) From c7f8896644c861e294d4d60996913b3b8d22d6ec Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 12:17:55 -0400 Subject: [PATCH 3/7] docs: document max effort precedence --- README.md | 59 ++++++++++++++++++++++++++----------- docs/architecture.md | 22 ++++++++++---- docs/init-config-surface.md | 15 ++++++++++ 3 files changed, 72 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index e206b115..4752936c 100644 --- a/README.md +++ b/README.md @@ -665,12 +665,24 @@ 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 +### Model-Tier Floors and Effort Ceilings 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: +becomes the provider's reasoning-effort setting. Model tiers are different: +`agent.model_tier` and `llm.reviewer_model_tier` are minimum floors for model +selection, while `llm.max_effort` is a ceiling for the default effort at the +resolved tier. They do not raise an agent's effort or select a model by +themselves. + +For a tier-based stage, `cr` applies this order: + +1. Resolve the effective 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 agent or stage default effort with `max_effort[effective tier]`. +4. Apply an explicit effort override, which wins over the ceiling. + +Configure ceilings manually in `config.yml`: ```yaml llm: @@ -682,18 +694,28 @@ llm: ``` 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 +declaring `low` under a `medium` ceiling still runs at `low`. Caps use the tier +after floor resolution and apply to internal stages (selection, synthesis, and +thread analysis) as well as reviewers. + +The complete precedence and bypass table is: + +| Input or path | Model selection | Default effort | `llm.max_effort` | +|---------------|-----------------|----------------|-----------------| +| Agent/stage tier with no explicit override | Effective tier after floors, then `model_map` | Agent/stage effort | Caps the default at the effective tier | +| `--reviewer-model-tier` | Raises the reviewer baseline before the agent floor is applied | Agent effort | Caps at the final resolved 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; no tier is available | +| Agent `model_id` | Exact agent model ID | Agent effort | Bypassed; no tier is available | +| `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 `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 `max_effort` but cannot yet edit it; set it by hand in `config.yml`. @@ -704,11 +726,12 @@ Dry-run and no-post runs also record selected reviewer runtime resolution in | 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 d51f0140..8f384ab3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -56,18 +56,28 @@ 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. +The resolver's authoritative ordering for tier-based requests 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 +`llm.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. + +An explicit `ModelOverride` returns with its requested effort or default effort +without applying a tier ceiling. `--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 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. + The direct `config.ResolveModelTier` exception is config inspection and the resolver implementation itself. `internal/architecture/model_resolution_test.go` enforces that direct diff --git a/docs/init-config-surface.md b/docs/init-config-surface.md index 7b56bc27..449191d2 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -181,6 +181,21 @@ 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 + +`profiles..llm.max_effort` is manual configuration, not an init field or +an init flag. It 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, tier-based default effort is capped only after the final tier +is resolved. Explicit `--selection-effort` and `--reviewer-effort` values win +after that cap. Exact `--selection-model`, `--reviewer-model`, agent `model_id`, +and benchmark stage model/effort overrides remain outside tier-keyed ceilings. + ## Scripted Install Ownership Scripted installs should remain readable. The intended shape is: From e27c3d8a612627e03b118836e5edc2d6312cd6fd Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 12:28:13 -0400 Subject: [PATCH 4/7] docs: clarify effort ceiling scope --- README.md | 62 ++++++++++++++++++++++--------------- docs/architecture.md | 30 +++++++++--------- docs/init-config-surface.md | 28 ++++++++++------- 3 files changed, 69 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 4752936c..2722253d 100644 --- a/README.md +++ b/README.md @@ -634,7 +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.max_effort` keys | `small`, `medium`, `large`; values `low`, `medium`, `high` | +| `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` | @@ -668,57 +668,69 @@ 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. Model tiers are different: +becomes the provider's reasoning-effort setting. For reviewer resolution, `agent.model_tier` and `llm.reviewer_model_tier` are minimum floors for model -selection, while `llm.max_effort` is a ceiling for the default effort at the -resolved tier. They do not raise an agent's effort or select a model by -themselves. +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 a tier-based stage, `cr` applies this order: +For reviewer resolution, `cr` applies this order: -1. Resolve the effective tier as the higher of the agent tier and the profile - reviewer-tier floor. +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 agent or stage default effort with `max_effort[effective tier]`. +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: - model_map: - large: openai-codex/gpt-5.6-sol - medium: openai-codex/gpt-5.6-terra - max_effort: - large: medium +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`. Caps use the tier -after floor resolution and apply to internal stages (selection, synthesis, and -thread analysis) as well as reviewers. +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 | `llm.max_effort` | +| Input or path | Model selection | Default effort | `max_effort` on selected runtime | |---------------|-----------------|----------------|-----------------| -| Agent/stage tier with no explicit override | Effective tier after floors, then `model_map` | Agent/stage effort | Caps the default at the effective tier | +| 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; no tier is available | -| Agent `model_id` | Exact agent model ID | Agent effort | Bypassed; no tier is available | +| `--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 `max_effort.large: medium`, the reviewer +`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 `max_effort` but cannot yet edit it; set it by hand in -`config.yml`. +`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 diff --git a/docs/architecture.md b/docs/architecture.md index 8f384ab3..017b7fae 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -48,26 +48,28 @@ executes an LLM stage must not hard-code model IDs and must not call `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. - -The resolver's authoritative ordering for tier-based requests 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 -`llm.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. +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 -without applying a tier ceiling. `--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. +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 diff --git a/docs/init-config-surface.md b/docs/init-config-surface.md index 449191d2..7238e7ac 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -183,18 +183,22 @@ Retention is global config under `data.retention`, not profile config. ## LLM Effort Ceiling Ownership -`profiles..llm.max_effort` is manual configuration, not an init field or -an init flag. It 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, tier-based default effort is capped only after the final tier -is resolved. Explicit `--selection-effort` and `--reviewer-effort` values win -after that cap. Exact `--selection-model`, `--reviewer-model`, agent `model_id`, -and benchmark stage model/effort overrides remain outside tier-keyed ceilings. +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 From ba724ecef2a254b501f01a5dd36f917683fd1352 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 14:04:32 -0400 Subject: [PATCH 5/7] test: cover exact reviewer override provenance --- internal/pipeline/pipeline_test.go | 56 ++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index ffb0ac85..e682619d 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -2938,6 +2938,62 @@ func TestDryRunReviewerModelOverrideBypassesAgentModelID(t *testing.T) { }) } +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("DryRun: %v", err) + } + + 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) { ctx := context.Background() store := openPipelineStore(t) From 70f9200785e1e024c7b135e9277f0c951b107042 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 14:11:41 -0400 Subject: [PATCH 6/7] test: guard effort ceiling resolution boundary --- internal/architecture/model_resolution_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 From 07cd4038efd07e6866698d46d598813b3d603415 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 14:18:02 -0400 Subject: [PATCH 7/7] docs: align model resolution guardrail --- docs/architecture.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 017b7fae..582a89c8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -43,7 +43,7 @@ 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 @@ -80,10 +80,10 @@ 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. -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 +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.