diff --git a/BENCHMARKING.md b/BENCHMARKING.md index a49c5472..948acd4d 100644 --- a/BENCHMARKING.md +++ b/BENCHMARKING.md @@ -152,8 +152,12 @@ Candidate `profile` must reference a configured profile. Candidate PR hosts must match the candidate profile's Git host. For the current full-pipeline `validate`, `doctor`, and `run` commands, candidates must declare explicit selection `model` and `effort`. Reviewer candidates must declare reviewer -`effort`, `agent_dirs`, and one reviewer model selector: either exact +`agent_dirs` and one reviewer model selector: either exact `stages.reviewers.model` or floor-based `stages.reviewers.model_tier`. +Reviewer `effort` is optional. When omitted, the benchmark does not pass a +`--reviewer-effort` override, so each selected agent keeps the effort resolved +from its catalog entry and the profile runtime's `max_effort` ceiling. When +present, reviewer `effort` is an explicit override. Selector-only `benchmark select` still requires explicit `stages.selection.model` and `stages.selection.effort`, but it allows the reviewer stage to be omitted. `stages.selection.prompt` is optional, but when @@ -255,7 +259,7 @@ When set on the candidate, `run` also passes: | `stages.selection.prompt` | `--selection-prompt ` | | `stages.reviewers.model` | `--reviewer-model ` | | `stages.reviewers.model_tier` | `--reviewer-model-tier ` | -| `stages.reviewers.effort` | `--reviewer-effort ` | +| `stages.reviewers.effort` | `--reviewer-effort ` when present; omitted to inherit agent/profile resolution | | `stages.reviewers.agent_dirs[]` | `--agents-dir ` | | `max_agents` | `--max-agents ` | | `max_concurrency` | `--max-concurrency ` | @@ -267,7 +271,15 @@ When set on the case, `run` also passes: | `review_base_sha` | `--review-base-sha ` | | `review_head_sha` | `--review-head-sha ` | -Unset fields are omitted. Posting, retry, approval, thread-resolution, session, +Unset fields are omitted. Benchmark candidate artifacts record reviewer +`effort_source` as `inherited` or `override`, so reports can distinguish the +two execution recipes even when the inherited effort is resolved later per +agent. Effort values are `low`, `medium`, `high`, `xhigh`, or `max`; validation +rejects levels unsupported by the candidate profile's runtime. Pi RPC supports +the full range, while the other built-in runtimes currently support through +`high`. + +Posting, retry, approval, thread-resolution, session, and live-review flags are never taken from the suite. `--cr-bin ` selects the binary used for child review runs. If omitted, diff --git a/README.md b/README.md index 2722253d..57be896b 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_runtimes..max_effort` keys | `small`, `medium`, `large`; values `low`, `medium`, `high` | +| `llm_runtimes..max_effort` keys | `small`, `medium`, `large`; values `low`, `medium`, `high`, plus `xhigh` and `max` for runtimes that support them (currently `pi_rpc`) | | `llm.reviewer_model_tier` | `small`, `medium`, `large` | | `review_policy.major_event` | `comment`, `request_changes` | | `review_policy.resolve_threads` | `auto`, `never` | @@ -667,7 +667,8 @@ 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 +Agent catalogs declare an absolute `effort` (`low`, `medium`, `high`, `xhigh`, +or `max`) 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` @@ -716,18 +717,20 @@ The complete precedence and bypass table is: | `--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 | +| `--selection-model` | Exact requested model ID | Stage effort or explicit effort | Bypassed; selection exact-model overrides do not carry a reviewer tier | +| `--reviewer-model` | Exact requested model ID | Agent effort or explicit effort | Inherited agent effort is capped at the effective reviewer tier; explicit effort bypasses 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 | +| `cr benchmark run` stage model/effort overrides | Exact benchmark model when supplied; otherwise normal tier selection | Explicit benchmark effort when supplied; inherited agent effort when reviewer effort is omitted | Explicit benchmark overrides bypass the profile ceiling; inherited reviewer effort retains it | 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. +runs that same large model at high effort. Adding only +`--reviewer-model my-provider/model` selects that exact model while retaining +the inherited medium ceiling; combining it with `--reviewer-effort high` +selects the exact model at high effort. `--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`. @@ -1209,11 +1212,11 @@ Review selection and execution flags: | `--max-agents ` | Set a hard total reviewer limit. Omit the flag or pass `0` to run all applicable repo-local reviewers and all matching `required_on_match` reviewers, plus up to 5 optional shared reviewers. A positive value below that combined required set fails. Negative values are rejected. | | `--max-concurrency ` | Limit concurrent reviewer agents. Omit the flag or pass `0` for the default limit of 5. Negative values are rejected. | | `--selection-model ` | Exact provider model ID passthrough for the selection stage only. Bypasses the default medium-tier selection model resolution. Requires `--dry-run` or `--no-post`. | -| `--selection-effort ` | Override selection-stage effort only with `low`, `medium`, or `high`. Requires `--dry-run` or `--no-post`. | +| `--selection-effort ` | Override selection-stage effort with `low`, `medium`, `high`, `xhigh`, or `max`, subject to runtime support. Requires `--dry-run` or `--no-post`. | | `--selection-prompt ` | Load selection-stage instruction text from a file while preserving the structured JSON selection protocol. Requires `--dry-run` or `--no-post`. | | `--reviewer-model ` | Exact provider model ID passthrough for reviewer stages only. Bypasses reviewer agent `model_tier`, `model_id`, and profile model-map resolution. Available for dry-run, no-post, and live reviews. | | `--reviewer-model-tier ` | Override the reviewer baseline tier only with `small`, `medium`, or `large`. This still respects higher agent `model_tier` floors. Requires `--dry-run` or `--no-post`. | -| `--reviewer-effort ` | Override reviewer-stage effort only with `low`, `medium`, or `high`. Available for dry-run, no-post, and live reviews. | +| `--reviewer-effort ` | Override reviewer-stage effort with `low`, `medium`, `high`, `xhigh`, or `max`, subject to runtime support. Available for dry-run, no-post, and live reviews. | | `--review-base-sha ` | Review this base commit SHA instead of the PR's current base SHA. Requires `--review-head-sha` and `--dry-run` or `--no-post`. | | `--review-head-sha ` | Review this head commit SHA instead of the PR's current head SHA. Requires `--review-base-sha` and `--dry-run` or `--no-post`. | | `--session ` | Override the PR's default orchestrator session with a named live-review session. Reviewer cohorts remain PR-scoped. Not allowed with `--dry-run`, `--no-post`, or `--retry-posts`. | @@ -1341,8 +1344,9 @@ and `stages.selection.effort` map to `--selection-model` and `--selection-effort`; optional `stages.selection.prompt` maps to `--selection-prompt`. `stages.reviewers.model` remains the exact-model bypass, while `stages.reviewers.model_tier` maps to `--reviewer-model-tier`. -`stages.reviewers.effort` and `stages.reviewers.agent_dirs[]` map to -`--reviewer-effort` and repeated `--agents-dir` flags. Case YAML fields +When present, `stages.reviewers.effort` maps to `--reviewer-effort`; when +omitted, reviewer effort is inherited from normal agent/profile resolution. +`stages.reviewers.agent_dirs[]` maps to repeated `--agents-dir` flags. Case YAML fields `review_base_sha` and `review_head_sha` pin the exact base/head commit pair reviewed by the dry-run child command. Optional `stages.synthesis` metadata is reserved for future benchmark support and does not change `benchmark run` diff --git a/docs/architecture.md b/docs/architecture.md index 582a89c8..85e3e358 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -65,11 +65,13 @@ 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. +An explicit `ModelOverride` bypasses model-map resolution. Explicit effort +still bypasses `max_effort`; inherited reviewer effort is capped when the +request carries an effective reviewer tier. `--reviewer-model` uses that tier, +including benchmark reviewer model overrides. `--selection-model` and agent +`model_id` do not carry one and remain uncapped. Runtime capability validation +happens after stage resolution, before adapter startup, so unsupported extended +effort levels fail without opening a model session. 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 7238e7ac..36d814b2 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -187,7 +187,8 @@ 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 +tier keys with `low`, `medium`, or `high` ceiling values for all runtimes; +`pi_rpc` also accepts `xhigh` and `max`. 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 @@ -196,9 +197,10 @@ 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. +and `--reviewer-effort` values win after the cap. Exact model overrides bypass +model-map resolution. `--selection-model` and agent `model_id` remain uncapped; +`--reviewer-model` retains the effective reviewer-tier cap when effort is +inherited, including benchmark reviewer model overrides. ## Scripted Install Ownership diff --git a/internal/agents/agents_test.go b/internal/agents/agents_test.go index 1fd2bb36..cacd741e 100644 --- a/internal/agents/agents_test.go +++ b/internal/agents/agents_test.go @@ -107,8 +107,8 @@ func TestLoadRejectsInvalidAgentModelMetadata(t *testing.T) { }, { name: "invalid effort", - index: "name: reviewer\ndescription: desc\nmodel_tier: medium\neffort: xhigh\n", - want: `effort "xhigh" is invalid`, + index: "name: reviewer\ndescription: desc\nmodel_tier: medium\neffort: ultra\n", + want: `effort "ultra" is invalid`, }, { name: "legacy model field", diff --git a/internal/benchmark/suite.go b/internal/benchmark/suite.go index 301991b7..97b2b13a 100644 --- a/internal/benchmark/suite.go +++ b/internal/benchmark/suite.go @@ -61,6 +61,12 @@ type CandidateStages struct { synthesisSet bool } +// ReviewersConfigured reports whether a reviewer-stage recipe was present or +// was constructed programmatically with reviewer values. +func (s CandidateStages) ReviewersConfigured() bool { + return s.reviewersSet || s.Reviewers.Model != "" || s.Reviewers.ModelTier != "" || s.Reviewers.Effort != "" || s.Reviewers.AgentDirs != nil +} + // SelectionStage configures the benchmark selection/orchestration phase. type SelectionStage struct { Model string `yaml:"model,omitempty" json:"model,omitempty"` @@ -403,7 +409,8 @@ func validateCandidates(candidates []Candidate, cfg config.File) error { if candidate.Profile == "" { return fmt.Errorf("%w: candidate %q profile is required", ErrInvalid, candidate.ID) } - if _, ok := cfg.Profiles[candidate.Profile]; !ok { + profile, ok := cfg.Profiles[candidate.Profile] + if !ok { return fmt.Errorf("%w: candidate %q references unknown profile %q", ErrInvalid, candidate.ID, candidate.Profile) } if !candidate.stagesSet { @@ -418,6 +425,9 @@ func validateCandidates(candidates []Candidate, cfg config.File) error { if err := validateSynthesisStageStructural(candidate.ID, candidate.Stages); err != nil { return err } + if err := validateCandidateEfforts(candidate, profile); err != nil { + return err + } if candidate.MaxAgents < 0 { return fmt.Errorf("%w: candidate %q max_agents must be non-negative", ErrInvalid, candidate.ID) } @@ -428,6 +438,26 @@ func validateCandidates(candidates []Candidate, cfg config.File) error { return nil } +func validateCandidateEfforts(candidate Candidate, profile config.Profile) error { + stages := []struct { + name string + effort string + }{ + {name: "selection", effort: candidate.Stages.Selection.Effort}, + {name: "reviewers", effort: candidate.Stages.Reviewers.Effort}, + {name: "synthesis", effort: candidate.Stages.Synthesis.Effort}, + } + for _, stage := range stages { + if strings.TrimSpace(stage.effort) == "" { + continue + } + if err := config.ValidateEffortForRuntime(profile.LLM, stage.effort); err != nil { + return fmt.Errorf("%w: candidate %q stages.%s.effort: %w", ErrInvalid, candidate.ID, stage.name, err) + } + } + return nil +} + func validateSelectionStage(candidateID string, stages CandidateStages) error { if !stages.selectionSet { return fmt.Errorf("%w: candidate %q stages.selection is required", ErrInvalid, candidateID) @@ -442,7 +472,7 @@ func validateSelectionStage(candidateID string, stages CandidateStages) error { return fmt.Errorf("%w: candidate %q stages.selection.prompt must be non-empty when present", ErrInvalid, candidateID) } if stages.Selection.Effort != "" && !modelprefs.Effort(stages.Selection.Effort).Valid() { - return fmt.Errorf("%w: candidate %q stages.selection.effort must be one of low, medium, high", ErrInvalid, candidateID) + return fmt.Errorf("%w: candidate %q stages.selection.effort must be one of low, medium, high, xhigh, max", ErrInvalid, candidateID) } return nil } @@ -467,7 +497,7 @@ func validateReviewerStageStructural(candidateID string, stages CandidateStages) return fmt.Errorf("%w: candidate %q stages.reviewers.effort must be non-empty when present", ErrInvalid, candidateID) } if stages.Reviewers.Effort != "" && !modelprefs.Effort(stages.Reviewers.Effort).Valid() { - return fmt.Errorf("%w: candidate %q stages.reviewers.effort must be one of low, medium, high", ErrInvalid, candidateID) + return fmt.Errorf("%w: candidate %q stages.reviewers.effort must be one of low, medium, high, xhigh, max", ErrInvalid, candidateID) } if !stages.Reviewers.agentDirsSet { return nil @@ -494,7 +524,7 @@ func validateSynthesisStageStructural(candidateID string, stages CandidateStages return fmt.Errorf("%w: candidate %q stages.synthesis.prompt must be non-empty when present", ErrInvalid, candidateID) } if !modelprefs.Effort(stages.Synthesis.Effort).Valid() { - return fmt.Errorf("%w: candidate %q stages.synthesis.effort must be one of low, medium, high", ErrInvalid, candidateID) + return fmt.Errorf("%w: candidate %q stages.synthesis.effort must be one of low, medium, high, xhigh, max", ErrInvalid, candidateID) } return nil } @@ -511,9 +541,6 @@ func validateRunCandidates(suite SuiteFile) error { if candidate.Stages.Reviewers.Model == "" && candidate.Stages.Reviewers.ModelTier == "" { return fmt.Errorf("%w: candidate %q stages.reviewers.model or stages.reviewers.model_tier is required for benchmark run", ErrInvalid, candidate.ID) } - if candidate.Stages.Reviewers.Effort == "" { - return fmt.Errorf("%w: candidate %q stages.reviewers.effort is required for benchmark run", ErrInvalid, candidate.ID) - } if !candidate.Stages.Reviewers.agentDirsSet { return fmt.Errorf("%w: candidate %q stages.reviewers.agent_dirs is required for benchmark run", ErrInvalid, candidate.ID) } diff --git a/internal/benchmark/suite_test.go b/internal/benchmark/suite_test.go index 786426d5..67f97183 100644 --- a/internal/benchmark/suite_test.go +++ b/internal/benchmark/suite_test.go @@ -114,6 +114,31 @@ func TestValidateForRunAcceptsEmptyReviewerAgentDirsField(t *testing.T) { } } +func TestValidateForRunAcceptsInheritedReviewerEffort(t *testing.T) { + suite := loadSuite(t, strings.Replace(validSuiteYAML(), " effort: high\n agent_dirs:", " agent_dirs:", 1)) + if err := ValidateForRun(suite, testConfig()); err != nil { + t.Fatalf("ValidateForRun: %v", err) + } +} + +func TestValidateForRunAcceptsExtendedPiEffort(t *testing.T) { + body := strings.Replace(validSuiteYAML(), "model: gpt-5.4\n effort: high\n prompt:", "model: openai-codex/gpt-5.6-luna\n effort: xhigh\n prompt:", 1) + body = strings.Replace(body, "model: gpt-5.4\n effort: high\n agent_dirs:", "model: openai-codex/gpt-5.6-luna\n effort: max\n agent_dirs:", 1) + suite := loadSuite(t, body) + if err := ValidateForRun(suite, runtimeTestConfig(config.LLMProviderPi, config.LLMAuthSubscription, config.LLMAdapterPiRPC)); err != nil { + t.Fatalf("ValidateForRun: %v", err) + } +} + +func TestValidateForRunRejectsExtendedEffortForUnsupportedRuntime(t *testing.T) { + body := strings.Replace(validSuiteYAML(), " effort: high\n agent_dirs:", " effort: xhigh\n agent_dirs:", 1) + suite := loadSuite(t, body) + err := ValidateForRun(suite, runtimeTestConfig(config.LLMProviderAnthropic, config.LLMAuthSubscription, config.LLMAdapterClaudeCLI)) + if err == nil || !strings.Contains(err.Error(), `candidate "cand1" stages.reviewers.effort: config: unsupported effort: effort "xhigh" is unsupported`) { + t.Fatalf("ValidateForRun error = %v", err) + } +} + func TestValidateForRunAcceptsReviewerModelTierWithoutReviewerModel(t *testing.T) { body := strings.Replace(validSuiteYAML(), " reviewers:\n model: gpt-5.4\n effort: high\n", " reviewers:\n model_tier: medium\n effort: high\n", 1) suite := loadSuite(t, body) @@ -340,9 +365,9 @@ cases: {name: "blank selection model when present", body: replaceSuiteLine(validSuiteYAML(), " model: gpt-5.4", ` model: " "`), want: "stages.selection.model must be non-empty"}, {name: "blank selection effort when present", body: replaceSuiteLine(validSuiteYAML(), " effort: high", ` effort: " "`), want: "stages.selection.effort must be non-empty"}, {name: "missing synthesis model", body: withRawSynthesisStage(validSuiteYAML(), " synthesis:\n effort: low\n"), want: "stages.synthesis.model is required when stages.synthesis is set"}, - {name: "invalid synthesis effort", body: withRawSynthesisStage(validSuiteYAML(), " synthesis:\n model: gpt-5.4\n effort: invalid\n"), want: "stages.synthesis.effort must be one of low, medium, high"}, + {name: "invalid synthesis effort", body: withRawSynthesisStage(validSuiteYAML(), " synthesis:\n model: gpt-5.4\n effort: invalid\n"), want: "stages.synthesis.effort must be one of low, medium, high, xhigh, max"}, {name: "blank synthesis prompt", body: withRawSynthesisStage(validSuiteYAML(), " synthesis:\n model: gpt-5.4\n effort: low\n prompt: \" \"\n"), want: "stages.synthesis.prompt must be non-empty when present"}, - {name: "invalid reviewer effort", body: strings.Replace(validSuiteYAML(), " effort: high\n agent_dirs:", " effort: invalid\n agent_dirs:", 1), want: "stages.reviewers.effort must be one of low, medium, high"}, + {name: "invalid reviewer effort", body: strings.Replace(validSuiteYAML(), " effort: high\n agent_dirs:", " effort: invalid\n agent_dirs:", 1), want: "stages.reviewers.effort must be one of low, medium, high, xhigh, max"}, {name: "invalid review base sha", body: replaceSuiteLine(validSuiteYAML(), " review_base_sha: 1111111", " review_base_sha: notsha"), want: "review_base_sha"}, {name: "blank review head sha", body: replaceSuiteLine(validSuiteYAML(), " review_head_sha: 2222222", ` review_head_sha: " "`), want: "review_head_sha must be non-empty"}, {name: "missing review head sha", body: replaceSuiteLine(validSuiteYAML(), " review_head_sha: 2222222\n", ""), want: "must be set together"}, @@ -520,6 +545,21 @@ func testConfig() config.File { return configtest.File( configtest.WithoutSecrets(), configtest.WithoutRepositoryProfiles(), - configtest.HomeProfile(config.Profile{Git: config.GitConfig{Host: "github.com"}}), + configtest.HomeProfile(config.Profile{ + Git: config.GitConfig{Host: "github.com"}, + LLM: config.LLMConfig{ + Provider: config.LLMProviderAnthropic, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterClaudeCLI, + }, + }), ) } + +func runtimeTestConfig(provider config.LLMProvider, auth config.LLMAuth, adapter config.LLMAdapter) config.File { + cfg := testConfig() + profile := cfg.Profiles["home"] + profile.LLM = config.LLMConfig{Provider: provider, Auth: auth, Adapter: adapter} + cfg.Profiles["home"] = profile + return cfg +} diff --git a/internal/cmd/benchmarkcmd/benchmarkcmd.go b/internal/cmd/benchmarkcmd/benchmarkcmd.go index 7b56b420..3422f463 100644 --- a/internal/cmd/benchmarkcmd/benchmarkcmd.go +++ b/internal/cmd/benchmarkcmd/benchmarkcmd.go @@ -61,10 +61,11 @@ type doctorSelectionStage struct { } type doctorReviewerStage struct { - Model string `json:"model,omitempty"` - ModelTier string `json:"model_tier,omitempty"` - Effort string `json:"effort,omitempty"` - AgentDirs []doctorAgentDir `json:"agent_dirs"` + Model string `json:"model,omitempty"` + ModelTier string `json:"model_tier,omitempty"` + Effort string `json:"effort,omitempty"` + EffortSource string `json:"effort_source,omitempty"` + AgentDirs []doctorAgentDir `json:"agent_dirs"` } type doctorAgentDir struct { @@ -201,6 +202,10 @@ func buildDoctorReport(suite benchmark.SuiteFile, cfg config.File, flags doctorF suiteDir := filepath.Dir(suite.Path) for _, candidate := range selectedCandidates { profile, ok := cfg.Profiles[candidate.Profile] + effortSource := "" + if candidate.Stages.ReviewersConfigured() { + effortSource = reviewerEffortSource(candidate.Stages.Reviewers.Effort) + } out := doctorCandidate{ ID: candidate.ID, Profile: candidate.Profile, @@ -212,10 +217,11 @@ func buildDoctorReport(suite benchmark.SuiteFile, cfg config.File, flags doctorF Prompt: candidate.Stages.Selection.Prompt, }, Reviewers: doctorReviewerStage{ - Model: candidate.Stages.Reviewers.Model, - ModelTier: candidate.Stages.Reviewers.ModelTier, - Effort: candidate.Stages.Reviewers.Effort, - AgentDirs: make([]doctorAgentDir, 0, len(candidate.Stages.Reviewers.AgentDirs)), + Model: candidate.Stages.Reviewers.Model, + ModelTier: candidate.Stages.Reviewers.ModelTier, + Effort: candidate.Stages.Reviewers.Effort, + EffortSource: effortSource, + AgentDirs: make([]doctorAgentDir, 0, len(candidate.Stages.Reviewers.AgentDirs)), }, Synthesis: summarizeDoctorOptionalStage(candidate.Stages.Synthesis), }, @@ -262,6 +268,10 @@ func renderDoctorText(opts *root.Options, report doctorReport) error { if candidate.Stages.Reviewers.ModelTier != "" { reviewerModel = "tier:" + candidate.Stages.Reviewers.ModelTier } + reviewerEffort := candidate.Stages.Reviewers.Effort + if candidate.Stages.Reviewers.EffortSource == "inherited" { + reviewerEffort = "inherited" + } w.printf( "- candidate %s profile=%s available=%t selection=%s/%s reviewers=%s/%s reviewer_agent_dirs=%d%s\n", candidate.ID, @@ -270,7 +280,7 @@ func renderDoctorText(opts *root.Options, report doctorReport) error { candidate.Stages.Selection.Model, candidate.Stages.Selection.Effort, reviewerModel, - candidate.Stages.Reviewers.Effort, + reviewerEffort, len(candidate.Stages.Reviewers.AgentDirs), synthesisText, ) diff --git a/internal/cmd/benchmarkcmd/benchmarkcmd_test.go b/internal/cmd/benchmarkcmd/benchmarkcmd_test.go index 9823711e..6d78eccd 100644 --- a/internal/cmd/benchmarkcmd/benchmarkcmd_test.go +++ b/internal/cmd/benchmarkcmd/benchmarkcmd_test.go @@ -88,7 +88,8 @@ func TestDoctorJSONReportsSelectedReadiness(t *testing.T) { got.Candidates[0].Stages.Selection.Model != "kimi" || got.Candidates[0].Stages.Selection.Effort != "low" || got.Candidates[0].Stages.Reviewers.Model != "kimi" || - got.Candidates[0].Stages.Reviewers.Effort != "low" { + got.Candidates[0].Stages.Reviewers.Effort != "low" || + got.Candidates[0].Stages.Reviewers.EffortSource != "override" { t.Fatalf("candidates = %#v, want selected second", got.Candidates) } if !got.Candidates[0].ProfileAvailable || got.Candidates[0].GitHost != "github.com" { @@ -395,6 +396,60 @@ func TestDoctorTextUsesDefaultResultsDir(t *testing.T) { } } +func TestDoctorReportsInheritedReviewerEffort(t *testing.T) { + body := strings.Replace(validBenchmarkSuite(t), " effort: high\n agent_dirs:", " agent_dirs:", 1) + suitePath := writeBenchmarkSuite(t, body) + + cmd, out := newTestCommand(t) + if err := root.Execute(cmd, []string{"benchmark", "doctor", suitePath, "--candidate", "first", "--json"}); err != nil { + t.Fatalf("Execute JSON: %v", err) + } + var got doctorReport + if err := json.Unmarshal(out.Bytes(), &got); err != nil { + t.Fatalf("Unmarshal JSON: %v\n%s", err, out.String()) + } + if len(got.Candidates) != 1 || got.Candidates[0].Stages.Reviewers.Effort != "" || got.Candidates[0].Stages.Reviewers.EffortSource != "inherited" { + t.Fatalf("reviewer stage = %#v, want inherited effort", got.Candidates) + } + + cmd, out = newTestCommand(t) + if err := root.Execute(cmd, []string{"benchmark", "doctor", suitePath, "--candidate", "first"}); err != nil { + t.Fatalf("Execute text: %v", err) + } + if !strings.Contains(out.String(), "reviewers=claude-sonnet-4-6/inherited") { + t.Fatalf("stdout = %q, want inherited reviewer effort", out.String()) + } +} + +func TestDoctorReportOmitsReviewerEffortSourceWithoutReviewerRecipe(t *testing.T) { + suite := benchmark.SuiteFile{ + Path: filepath.Join(t.TempDir(), "suite.yml"), + Suite: benchmark.Suite{ID: "selector-only"}, + Candidates: []benchmark.Candidate{{ + ID: "selector", + Profile: "home", + Stages: benchmark.CandidateStages{ + Selection: benchmark.SelectionStage{Model: "selector", Effort: "medium"}, + }, + }}, + } + + report, err := buildDoctorReport(suite, testConfig(), doctorFlags{}) + if err != nil { + t.Fatalf("buildDoctorReport: %v", err) + } + if len(report.Candidates) != 1 || report.Candidates[0].Stages.Reviewers.EffortSource != "" { + t.Fatalf("reviewer stage = %#v, want no effort source", report.Candidates) + } + data, err := json.Marshal(report) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + if strings.Contains(string(data), `"effort_source"`) { + t.Fatalf("doctor JSON = %s, want no reviewer effort source", data) + } +} + func TestRunExecutesSelectedMatrixAndWritesArtifacts(t *testing.T) { cmd, out := newTestCommand(t) suitePath := writeBenchmarkSuite(t, validBenchmarkSuite(t)) @@ -1234,6 +1289,18 @@ func TestReviewArgsMapsExplicitStageRecipesToReviewFlags(t *testing.T) { required: []string{"--selection-model", "claude-sonnet-4-6", "--selection-effort", "high", "--reviewer-model-tier", "large", "--reviewer-effort", "low", "--agents-dir", filepath.Join(suiteDir, "agents")}, forbidden: []string{"--reviewer-model"}, }, + { + name: "inherited reviewer effort", + candidate: benchmark.Candidate{ + Profile: "home", + Stages: benchmark.CandidateStages{ + Selection: benchmark.SelectionStage{Model: "selector", Effort: "medium"}, + Reviewers: benchmark.ReviewerStage{ModelTier: "small", AgentDirs: []string{}}, + }, + }, + required: []string{"--selection-model", "selector", "--selection-effort", "medium", "--reviewer-model-tier", "small"}, + forbidden: []string{"--reviewer-effort"}, + }, { name: "review shas", candidate: benchmark.Candidate{ @@ -1264,6 +1331,51 @@ func TestReviewArgsMapsExplicitStageRecipesToReviewFlags(t *testing.T) { } } +func TestSummarizeCandidatesRecordsReviewerEffortSource(t *testing.T) { + candidates := []benchmark.Candidate{ + { + ID: "inherited", + Stages: benchmark.CandidateStages{ + Selection: benchmark.SelectionStage{Model: "selector", Effort: "medium"}, + Reviewers: benchmark.ReviewerStage{ModelTier: "small", AgentDirs: []string{}}, + }, + }, + { + ID: "override", + Stages: benchmark.CandidateStages{ + Selection: benchmark.SelectionStage{Model: "selector", Effort: "medium"}, + Reviewers: benchmark.ReviewerStage{Model: "reviewer", Effort: "max", AgentDirs: []string{}}, + }, + }, + } + + got := summarizeCandidates(t.TempDir(), candidates) + if got[0].Stages.Reviewers.EffortSource != "inherited" || got[0].Stages.Reviewers.Effort != "" { + t.Fatalf("inherited reviewer summary = %#v", got[0].Stages.Reviewers) + } + if got[1].Stages.Reviewers.EffortSource != "override" || got[1].Stages.Reviewers.Effort != "max" { + t.Fatalf("override reviewer summary = %#v", got[1].Stages.Reviewers) + } +} + +func TestSummarizeCandidatesOmitsReviewerEffortSourceWithoutReviewerRecipe(t *testing.T) { + candidates := []benchmark.Candidate{{ + ID: "selector-only", + Stages: benchmark.CandidateStages{ + Selection: benchmark.SelectionStage{Model: "selector", Effort: "medium"}, + }, + }} + + got := summarizeCandidates(t.TempDir(), candidates) + data, err := json.Marshal(got) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + if strings.Contains(string(data), `"effort_source"`) { + t.Fatalf("candidate JSON = %s, want no reviewer effort source", data) + } +} + func writeExecutableCRBin(t *testing.T) string { t.Helper() name := "cr" diff --git a/internal/cmd/benchmarkcmd/executor_test.go b/internal/cmd/benchmarkcmd/executor_test.go index fff67ea6..0049916e 100644 --- a/internal/cmd/benchmarkcmd/executor_test.go +++ b/internal/cmd/benchmarkcmd/executor_test.go @@ -148,6 +148,11 @@ func TestInProcessExecutorOpensAndCleansRuntimePerCell(t *testing.T) { if pipelineRequests[1].ReviewBaseSHA != "1111111" || pipelineRequests[1].ReviewHeadSHA != "2222222" { t.Fatalf("second pipeline request = %#v, want case SHAs", pipelineRequests[1]) } + for _, req := range pipelineRequests { + if req.ReviewerEffortOverride != "" { + t.Fatalf("reviewer effort override = %q, want inherited effort", req.ReviewerEffortOverride) + } + } } func TestInProcessExecutorRecoversPipelinePanicAfterCleanup(t *testing.T) { diff --git a/internal/cmd/benchmarkcmd/run.go b/internal/cmd/benchmarkcmd/run.go index b20bb8d2..44421c14 100644 --- a/internal/cmd/benchmarkcmd/run.go +++ b/internal/cmd/benchmarkcmd/run.go @@ -118,10 +118,11 @@ type benchmarkSelectionStage struct { type benchmarkSynthesisStage = benchmarkSelectionStage type benchmarkReviewerStage struct { - Model string `json:"model,omitempty"` - ModelTier string `json:"model_tier,omitempty"` - Effort string `json:"effort,omitempty"` - AgentDirs []benchmarkAgentDir `json:"agent_dirs,omitempty"` + Model string `json:"model,omitempty"` + ModelTier string `json:"model_tier,omitempty"` + Effort string `json:"effort,omitempty"` + EffortSource string `json:"effort_source,omitempty"` + AgentDirs []benchmarkAgentDir `json:"agent_dirs,omitempty"` } type benchmarkPromptFile struct { @@ -518,6 +519,10 @@ func summaryMode(summary benchmarkSuiteSummary) string { func summarizeCandidates(suiteDir string, candidates []benchmark.Candidate) []benchmarkCandidate { out := make([]benchmarkCandidate, 0, len(candidates)) for _, candidate := range candidates { + effortSource := "" + if candidate.Stages.ReviewersConfigured() { + effortSource = reviewerEffortSource(candidate.Stages.Reviewers.Effort) + } out = append(out, benchmarkCandidate{ ID: candidate.ID, Profile: candidate.Profile, @@ -528,10 +533,11 @@ func summarizeCandidates(suiteDir string, candidates []benchmark.Candidate) []be Prompt: summarizePromptFile(suiteDir, candidate.Stages.Selection.Prompt), }, Reviewers: benchmarkReviewerStage{ - Model: candidate.Stages.Reviewers.Model, - ModelTier: candidate.Stages.Reviewers.ModelTier, - Effort: candidate.Stages.Reviewers.Effort, - AgentDirs: summarizeAgentDirs(suiteDir, candidate.Stages.Reviewers.AgentDirs), + Model: candidate.Stages.Reviewers.Model, + ModelTier: candidate.Stages.Reviewers.ModelTier, + Effort: candidate.Stages.Reviewers.Effort, + EffortSource: effortSource, + AgentDirs: summarizeAgentDirs(suiteDir, candidate.Stages.Reviewers.AgentDirs), }, Synthesis: summarizeOptionalSynthesisStage(suiteDir, candidate.Stages.Synthesis), }, @@ -542,6 +548,13 @@ func summarizeCandidates(suiteDir string, candidates []benchmark.Candidate) []be return out } +func reviewerEffortSource(effort string) string { + if strings.TrimSpace(effort) == "" { + return "inherited" + } + return "override" +} + func summarizeOptionalSynthesisStage(suiteDir string, stage benchmark.SelectionStage) *benchmarkSynthesisStage { if !isOptionalStageConfigured(stage) { return nil diff --git a/internal/cmd/cmderr/cmderr.go b/internal/cmd/cmderr/cmderr.go index c00544d7..a6945284 100644 --- a/internal/cmd/cmderr/cmderr.go +++ b/internal/cmd/cmderr/cmderr.go @@ -16,7 +16,8 @@ import ( // Config maps config package errors to scriptable command errors. func Config(err error) error { switch { - case errors.Is(err, config.ErrInvalid): + case errors.Is(err, config.ErrInvalid), + errors.Is(err, config.ErrUnsupportedEffort): return exitcode.Usage(err) case errors.Is(err, config.ErrNotConfigured), errors.Is(err, config.ErrProfileNotFound), diff --git a/internal/cmd/cmdruntime/cmdruntime_test.go b/internal/cmd/cmdruntime/cmdruntime_test.go index c8d87f25..36754c9b 100644 --- a/internal/cmd/cmdruntime/cmdruntime_test.go +++ b/internal/cmd/cmdruntime/cmdruntime_test.go @@ -23,6 +23,7 @@ func TestMapRunError(t *testing.T) { is error }{ {name: "invalid config", err: config.ErrInvalid, want: exitcode.UsageError, is: config.ErrInvalid}, + {name: "unsupported effort", err: config.ErrUnsupportedEffort, want: exitcode.UsageError, is: config.ErrUnsupportedEffort}, {name: "missing config", err: config.ErrNotConfigured, want: exitcode.AuthConfigError, is: config.ErrNotConfigured}, {name: "ambiguous repository profile", err: config.ErrRepositoryProfileAmbiguous, want: exitcode.AuthConfigError, is: config.ErrRepositoryProfileAmbiguous}, {name: "unsafe agent source", err: agents.ErrUnsafeSource, want: exitcode.UsageError, is: agents.ErrUnsafeSource}, diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 28e57ce2..444ae5ca 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -161,7 +161,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact return exitcode.Usage(err) } if selectionEffortChanged && !modelprefs.Effort(selectionEffort).Valid() { - return exitcode.Usage(fmt.Errorf("--selection-effort must be one of low, medium, high")) + return exitcode.Usage(fmt.Errorf("--selection-effort must be one of low, medium, high, xhigh, max")) } if err := validateNonEmptyChangedFlags(cmd, [2]string{"selection-prompt", flags.selectionPrompt}, @@ -180,7 +180,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact return exitcode.Usage(err) } if reviewerEffortChanged && !modelprefs.Effort(reviewerEffort).Valid() { - return exitcode.Usage(fmt.Errorf("--reviewer-effort must be one of low, medium, high")) + return exitcode.Usage(fmt.Errorf("--reviewer-effort must be one of low, medium, high, xhigh, max")) } dryRunOnlyOverrideChanged := selectionModelChanged || selectionEffortChanged || selectionPromptChanged || reviewerModelTierChanged if dryRunOnlyOverrideChanged && !flags.dryRun { @@ -268,6 +268,16 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact if err := prref.MatchProvider(urlProvider, string(profile.Git.ProviderKind())); err != nil { return exitcode.Usage(profileSpan.End(err)) } + if selectionEffortChanged { + if err := config.ValidateEffortForRuntime(profile.LLM, selectionEffort); err != nil { + return exitcode.Usage(profileSpan.End(fmt.Errorf("--selection-effort: %w", err))) + } + } + if reviewerEffortChanged { + if err := config.ValidateEffortForRuntime(profile.LLM, reviewerEffort); err != nil { + return exitcode.Usage(profileSpan.End(fmt.Errorf("--reviewer-effort: %w", err))) + } + } _ = profileSpan.End(nil) reviewerFast := profile.Fast if cmd.Flags().Changed("fast") { diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index e58cf870..abd7ef29 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -652,8 +652,8 @@ func TestReviewEmptyStageOverrideErrorPrecedence(t *testing.T) { "--selection-effort", "xhigh", "--selection-prompt", " ", }) - if err == nil || err.Error() != "--selection-effort must be one of low, medium, high" { - t.Fatalf("Execute error = %v, want selection-effort precedence", err) + if err == nil || err.Error() != "--selection-prompt must be non-empty" { + t.Fatalf("Execute error = %v, want empty selection-prompt error", err) } } @@ -662,8 +662,8 @@ func TestReviewRejectsInvalidModelEffortBeforeRuntimeFactory(t *testing.T) { name string args []string }{ - {name: "selection", args: []string{"--dry-run", "--selection-effort", "xhigh"}}, - {name: "reviewer", args: []string{"--dry-run", "--reviewer-effort", "xhigh"}}, + {name: "selection", args: []string{"--dry-run", "--selection-effort", "ultra"}}, + {name: "reviewer", args: []string{"--dry-run", "--reviewer-effort", "ultra"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -687,6 +687,55 @@ func TestReviewRejectsInvalidModelEffortBeforeRuntimeFactory(t *testing.T) { } } +func TestReviewRejectsExtendedEffortUnsupportedByProfileBeforeRuntimeFactory(t *testing.T) { + var factoryCalled bool + cmd, _ := newTestCommand(t, testConfig(), func(context.Context, app.OpenRequest) (app.Runtime, error) { + factoryCalled = true + return app.Runtime{Runner: &fakeRunner{result: testPipelineResult(false)}}, nil + }) + + err := root.Execute(cmd, []string{ + "review", "https://github.com/open-cli-collective/codereview-cli/pull/29", + "--dry-run", "--reviewer-effort", "xhigh", + }) + if err == nil || !strings.Contains(err.Error(), `--reviewer-effort: config: unsupported effort: effort "xhigh" is unsupported`) { + t.Fatalf("Execute error = %v", err) + } + if factoryCalled { + t.Fatal("runtime factory was called for unsupported effort") + } +} + +func TestReviewPassesExtendedPiEffortOverrides(t *testing.T) { + cfg := testConfig() + profile := cfg.Profiles["home"] + profile.LLM = config.LLMConfig{ + Provider: config.LLMProviderPi, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterPiRPC, + } + cfg.Profiles["home"] = profile + runner := &fakeRunner{result: testPipelineResult(false)} + cmd, _ := newTestCommand(t, cfg, fakeFactory(runner)) + + err := root.Execute(cmd, []string{ + "review", "https://github.com/open-cli-collective/codereview-cli/pull/29", + "--dry-run", + "--selection-effort", "xhigh", + "--reviewer-effort", "max", + }) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if len(runner.requests) != 1 { + t.Fatalf("runner calls = %d, want 1", len(runner.requests)) + } + req := runner.requests[0] + if req.SelectionEffortOverride != "xhigh" || req.ReviewerEffortOverride != "max" { + t.Fatalf("effort overrides = %q/%q, want xhigh/max", req.SelectionEffortOverride, req.ReviewerEffortOverride) + } +} + func TestReviewRejectsInvalidReviewerModelTierBeforeRuntimeFactory(t *testing.T) { var factoryCalled bool cmd, _ := newTestCommand(t, testConfig(), func(context.Context, app.OpenRequest) (app.Runtime, error) { diff --git a/internal/config/config.go b/internal/config/config.go index e001f0cc..457fa951 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -43,6 +43,9 @@ var ( ErrInvalid = errors.New("config: invalid") // ErrUnsupported means the config uses a known v2-only option. ErrUnsupported = errors.New("config: not supported in v1") + // ErrUnsupportedEffort means the selected LLM runtime cannot honor an + // otherwise valid effort level. + ErrUnsupportedEffort = errors.New("config: unsupported effort") ) // IsConfigSelection reports whether err identifies an invalid or missing @@ -535,6 +538,7 @@ type LLMRuntimeSpec struct { DisplayName string BuiltInModelMap ModelMap FastModeModels []string + MaximumEffort modelprefs.Effort RequiresCredentialRef bool } @@ -551,6 +555,7 @@ var llmRuntimeSpecs = []LLMRuntimeSpec{ string(ModelTierLarge): "claude-opus-5", }, FastModeModels: []string{"claude-opus-5", "claude-opus-4-8"}, + MaximumEffort: modelprefs.EffortHigh, }, { Provider: LLMProviderAnthropic, @@ -560,6 +565,7 @@ var llmRuntimeSpecs = []LLMRuntimeSpec{ DisplayName: "Anthropic API", BuiltInModelMap: ModelMap{}, FastModeModels: []string{"claude-opus-5", "claude-opus-4-8"}, + MaximumEffort: modelprefs.EffortHigh, RequiresCredentialRef: true, }, { @@ -574,6 +580,7 @@ var llmRuntimeSpecs = []LLMRuntimeSpec{ string(ModelTierLarge): "gpt-5.5", }, FastModeModels: []string{"gpt-5.4", "gpt-5.5", "gpt-5.6-sol", "gpt-5.6-terra", "gpt-5.6-luna"}, + MaximumEffort: modelprefs.EffortHigh, }, { Provider: LLMProviderOpenAI, @@ -587,6 +594,7 @@ var llmRuntimeSpecs = []LLMRuntimeSpec{ string(ModelTierLarge): "gpt-5.5", }, RequiresCredentialRef: true, + MaximumEffort: modelprefs.EffortHigh, }, { Provider: LLMProviderPi, @@ -595,6 +603,7 @@ var llmRuntimeSpecs = []LLMRuntimeSpec{ SuggestedName: "pi-local", DisplayName: "Pi RPC", BuiltInModelMap: ModelMap{}, + MaximumEffort: modelprefs.EffortMax, }, } @@ -633,6 +642,30 @@ func (s LLMRuntimeSpec) SupportsFastMode(model string) bool { return false } +// ValidateEffortForRuntime checks a reasoning effort against the selected +// runtime before an adapter is opened or invoked. +func ValidateEffortForRuntime(llm LLMConfig, effort string) error { + effort = strings.TrimSpace(effort) + if effort == "" { + return nil + } + requested := modelprefs.Effort(effort) + if !requested.Valid() { + return fmt.Errorf("%w: effort %q is invalid; must be one of low, medium, high, xhigh, max", ErrInvalid, effort) + } + spec, ok := FindLLMRuntimeSpec(llm.Provider, llm.Auth, llm.Adapter) + if !ok && llm.Auth == "" { + spec, ok = findLLMRuntimeSpecByProviderAdapter(llm.Provider, llm.Adapter) + } + if !ok { + return fmt.Errorf("%w: effort %q cannot be validated for unknown runtime %s/%s/%s", ErrInvalid, effort, llm.Provider, llm.Auth, llm.Adapter) + } + if requested.Rank() > spec.MaximumEffort.Rank() { + return fmt.Errorf("%w: effort %q is unsupported for runtime %s/%s/%s; maximum is %s", ErrUnsupportedEffort, effort, llm.Provider, llm.Auth, llm.Adapter, spec.MaximumEffort) + } + return nil +} + func findLLMRuntimeSpecByProviderAdapter(provider LLMProvider, adapter LLMAdapter) (LLMRuntimeSpec, bool) { for _, spec := range llmRuntimeSpecs { if spec.Provider == provider && spec.Adapter == adapter { @@ -1452,8 +1485,8 @@ func validateLLMConfig(field string, llm LLMConfig) error { 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 err := ValidateEffortForRuntime(llm, ceiling); err != nil { + return invalid("%s.max_effort.%s: %v", field, tier, err) } } if llm.ReviewerModelTier != "" && !llm.ReviewerModelTier.Valid() { diff --git a/internal/config/config_effort_test.go b/internal/config/config_effort_test.go new file mode 100644 index 00000000..b802708a --- /dev/null +++ b/internal/config/config_effort_test.go @@ -0,0 +1,54 @@ +package config + +import ( + "errors" + "strings" + "testing" +) + +func TestValidateEffortForRuntimeAllowsExtendedPiEffort(t *testing.T) { + llm := LLMConfig{ + Provider: LLMProviderPi, + Auth: LLMAuthSubscription, + Adapter: LLMAdapterPiRPC, + } + for _, effort := range []string{"low", "medium", "high", "xhigh", "max"} { + if err := ValidateEffortForRuntime(llm, effort); err != nil { + t.Fatalf("ValidateEffortForRuntime(%q): %v", effort, err) + } + } +} + +func TestValidateEffortForRuntimeRejectsExtendedEffortForOtherRuntimes(t *testing.T) { + llm := LLMConfig{ + Provider: LLMProviderAnthropic, + Auth: LLMAuthSubscription, + Adapter: LLMAdapterClaudeCLI, + } + err := ValidateEffortForRuntime(llm, "xhigh") + if err == nil || !errors.Is(err, ErrUnsupportedEffort) || !strings.Contains(err.Error(), `effort "xhigh" is unsupported`) || !strings.Contains(err.Error(), "claude_cli") { + t.Fatalf("ValidateEffortForRuntime error = %v", err) + } +} + +func TestValidateEffortForRuntimeRejectsUnknownEffort(t *testing.T) { + llm := LLMConfig{ + Provider: LLMProviderPi, + Auth: LLMAuthSubscription, + Adapter: LLMAdapterPiRPC, + } + err := ValidateEffortForRuntime(llm, "ultra") + if err == nil || !errors.Is(err, ErrInvalid) || !strings.Contains(err.Error(), `effort "ultra" is invalid`) { + t.Fatalf("ValidateEffortForRuntime error = %v", err) + } +} + +func TestValidateEffortForRuntimeAllowsKnownProviderAdapterWithOmittedAuth(t *testing.T) { + llm := LLMConfig{ + Provider: LLMProviderOpenAI, + Adapter: LLMAdapterOpenAIAPI, + } + if err := ValidateEffortForRuntime(llm, "medium"); err != nil { + t.Fatalf("ValidateEffortForRuntime: %v", err) + } +} diff --git a/internal/config/config_max_effort_test.go b/internal/config/config_max_effort_test.go index 09cc175b..d727e350 100644 --- a/internal/config/config_max_effort_test.go +++ b/internal/config/config_max_effort_test.go @@ -35,17 +35,41 @@ func TestValidateRejectsUnknownMaxEffortTier(t *testing.T) { func TestValidateRejectsUnknownMaxEffortValue(t *testing.T) { cfg := validFile() runtime := cfg.LLMRuntimes["home-llm"] - runtime.MaxEffort = EffortMap{"large": "xhigh"} + runtime.MaxEffort = EffortMap{"large": "ultra"} 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") { + if !strings.Contains(err.Error(), "low, medium, high, xhigh, max") { t.Fatalf("Validate error = %v, want valid-value mention", err) } } +func TestValidateRejectsMaxEffortUnsupportedByRuntime(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) || !strings.Contains(err.Error(), `effort "xhigh" is unsupported`) { + t.Fatalf("Validate error = %v", err) + } +} + +func TestValidateAcceptsExtendedMaxEffortForPiRPC(t *testing.T) { + cfg := validFile() + cfg.LLMRuntimes["home-llm"] = LLMConfig{ + Provider: LLMProviderPi, + Auth: LLMAuthSubscription, + Adapter: LLMAdapterPiRPC, + MaxEffort: EffortMap{"small": "max"}, + } + if err := Validate(cfg); err != nil { + t.Fatalf("Validate error = %v", err) + } +} + func TestResolveMaxEffortReportsUncappedTiers(t *testing.T) { llm := LLMConfig{MaxEffort: EffortMap{"large": "medium"}} got, ok := ResolveMaxEffort(llm, ModelTierLarge) diff --git a/internal/llmadapters/pi_rpc_test.go b/internal/llmadapters/pi_rpc_test.go index 52669167..b5ebf2dc 100644 --- a/internal/llmadapters/pi_rpc_test.go +++ b/internal/llmadapters/pi_rpc_test.go @@ -30,7 +30,7 @@ func TestPiRPCLaunchSafetyAndSuccess(t *testing.T) { stream, err := adapter.Start(ctx, Request{ Model: "opencode-go/kimi-k2.6", - Effort: "high", + Effort: "max", Prompt: "review this diff", LogPath: logPath, }) @@ -69,6 +69,7 @@ func TestPiRPCLaunchSafetyAndSuccess(t *testing.T) { record := readPiRPCRecord(t, recordPath) assertFlagValue(t, record.AdapterArgs, "--mode", "rpc") assertFlagValue(t, record.AdapterArgs, "--model", "opencode-go/kimi-k2.6") + assertFlagValue(t, record.AdapterArgs, "--thinking", "max") assertFlagValue(t, record.AdapterArgs, "--system-prompt", piRPCSystemPrompt) for _, flag := range []string{"--no-tools", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { if !containsFlag(record.AdapterArgs, flag) { diff --git a/internal/modelprefs/modelprefs.go b/internal/modelprefs/modelprefs.go index 6d687251..78a3483d 100644 --- a/internal/modelprefs/modelprefs.go +++ b/internal/modelprefs/modelprefs.go @@ -9,12 +9,14 @@ const ( EffortLow Effort = "low" EffortMedium Effort = "medium" EffortHigh Effort = "high" + EffortXHigh Effort = "xhigh" + EffortMax Effort = "max" ) // Valid reports whether e is a known effort value. func (e Effort) Valid() bool { switch e { - case EffortLow, EffortMedium, EffortHigh: + case EffortLow, EffortMedium, EffortHigh, EffortXHigh, EffortMax: return true default: return false @@ -31,6 +33,10 @@ func (e Effort) Rank() int { return 2 case EffortHigh: return 3 + case EffortXHigh: + return 4 + case EffortMax: + return 5 default: return 0 } diff --git a/internal/modelprefs/modelprefs_effort_test.go b/internal/modelprefs/modelprefs_effort_test.go index f62b2363..75b32cc8 100644 --- a/internal/modelprefs/modelprefs_effort_test.go +++ b/internal/modelprefs/modelprefs_effort_test.go @@ -3,11 +3,14 @@ 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()) + ordered := []Effort{EffortLow, EffortMedium, EffortHigh, EffortXHigh, EffortMax} + for i := 1; i < len(ordered); i++ { + if ordered[i-1].Rank() >= ordered[i].Rank() { + t.Fatalf("effort ranks are not ordered: %q=%d %q=%d", ordered[i-1], ordered[i-1].Rank(), ordered[i], ordered[i].Rank()) + } } - if Effort("xhigh").Rank() != 0 { - t.Fatalf("unknown effort rank = %d, want 0", Effort("xhigh").Rank()) + if Effort("ultra").Rank() != 0 { + t.Fatalf("unknown effort rank = %d, want 0", Effort("ultra").Rank()) } } @@ -18,10 +21,11 @@ func TestMinEffort(t *testing.T) { want Effort }{ {name: "ceiling lowers", left: EffortHigh, right: EffortMedium, want: EffortMedium}, + {name: "extended ceiling lowers", left: EffortMax, right: EffortXHigh, want: EffortXHigh}, {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}, + {name: "invalid right ignored", left: EffortHigh, right: "ultra", want: EffortHigh}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 5b07ee58..63b66280 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -3228,9 +3228,15 @@ func resolveReviewerRuntimeConfig(req Request, agent agents.Agent) (llmRuntimeCo func resolveReviewerRuntime(req Request, agent agents.Agent) (reviewerRuntimeResolution, error) { if strings.TrimSpace(req.ReviewerModelOverride) != "" { + baselineTier, err := resolveReviewerBaselineTier(req.Profile, req.ReviewerModelTierOverride) + if err != nil { + return reviewerRuntimeResolution{}, fmt.Errorf("pipeline: agent %s: %w", agent.ID, err) + } resolved, err := stagemodel.ResolveStageModel(stagemodel.Request{ Profile: req.Profile, Stage: stagemodel.StageReviewer, + Tier: baselineTier, + FloorTier: config.ModelTier(strings.TrimSpace(agent.ModelTier)), ModelOverride: req.ReviewerModelOverride, EffortOverride: req.ReviewerEffortOverride, DefaultEffort: agent.Effort, @@ -3244,13 +3250,10 @@ func resolveReviewerRuntime(req Request, agent agents.Agent) (reviewerRuntimeRes ResolvedEffort: resolved.Effort, }, nil } - resolved, err := resolveAgentModel(req.Profile, req.ReviewerModelTierOverride, agent) + resolved, err := resolveAgentModel(req.Profile, req.ReviewerModelTierOverride, req.ReviewerEffortOverride, agent) if err != nil { return reviewerRuntimeResolution{}, err } - if effort := strings.TrimSpace(req.ReviewerEffortOverride); effort != "" { - resolved.ResolvedEffort = effort - } return resolved, nil } @@ -3276,13 +3279,14 @@ func resolveReviewerFastMode(req Request, catalog agents.Catalog) (bool, string, return warning == "", warning, nil } -func resolveAgentModel(profile config.Profile, baselineOverride string, agent agents.Agent) (reviewerRuntimeResolution, error) { +func resolveAgentModel(profile config.Profile, baselineOverride, effortOverride string, agent agents.Agent) (reviewerRuntimeResolution, error) { if modelID := strings.TrimSpace(agent.ModelID); modelID != "" { resolved, err := stagemodel.ResolveStageModel(stagemodel.Request{ - Profile: profile, - Stage: stagemodel.StageReviewer, - ModelOverride: modelID, - DefaultEffort: agent.Effort, + Profile: profile, + Stage: stagemodel.StageReviewer, + ModelOverride: modelID, + EffortOverride: effortOverride, + DefaultEffort: agent.Effort, }) if err != nil { return reviewerRuntimeResolution{}, fmt.Errorf("pipeline: agent %s: %w", agent.ID, err) @@ -3302,11 +3306,12 @@ func resolveAgentModel(profile config.Profile, baselineOverride string, agent ag return reviewerRuntimeResolution{}, fmt.Errorf("pipeline: agent %s: %w", agent.ID, err) } resolved, err := stagemodel.ResolveStageModel(stagemodel.Request{ - Profile: profile, - Stage: stagemodel.StageReviewer, - Tier: baselineTier, - FloorTier: floorTier, - DefaultEffort: agent.Effort, + Profile: profile, + Stage: stagemodel.StageReviewer, + Tier: baselineTier, + FloorTier: floorTier, + EffortOverride: effortOverride, + DefaultEffort: agent.Effort, }) if err != nil { return reviewerRuntimeResolution{}, fmt.Errorf("pipeline: agent %s: %w", agent.ID, err) diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 8bfd51e5..3a1a2974 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -7641,6 +7641,56 @@ func TestReviewerRuntimeConfigCapsEffortAtConfiguredTierCeiling(t *testing.T) { } } +func TestReviewerRuntimeConfigCapsInheritedEffortForExactModelOverride(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + MaxEffort: config.EffortMap{"large": "medium"}, + }} + agent := agents.Agent{ID: "architecture:solid", ModelTier: "large", Effort: "high"} + + got, err := resolveReviewerRuntimeConfig(Request{ + Profile: profile, + ReviewerModelOverride: "operator-model", + }, agent) + if err != nil { + t.Fatalf("resolveReviewerRuntimeConfig(inherited): %v", err) + } + if got.model != "operator-model" || got.effort != "medium" { + t.Fatalf("inherited exact-model reviewer = %+v, want operator-model/medium", got) + } + + got, err = resolveReviewerRuntimeConfig(Request{ + Profile: profile, + ReviewerModelOverride: "operator-model", + ReviewerEffortOverride: "high", + }, agent) + if err != nil { + t.Fatalf("resolveReviewerRuntimeConfig(explicit): %v", err) + } + if got.model != "operator-model" || got.effort != "high" { + t.Fatalf("explicit exact-model reviewer = %+v, want operator-model/high", got) + } +} + +func TestReviewerRuntimeConfigRejectsUnsupportedTierEffortOverride(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderAnthropic, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterClaudeCLI, + }} + agent := agents.Agent{ID: "go:implementation-tests", ModelTier: "small", Effort: "medium"} + + _, err := resolveReviewerRuntimeConfig(Request{ + Profile: profile, + ReviewerEffortOverride: "xhigh", + }, agent) + if err == nil || !errors.Is(err, config.ErrUnsupportedEffort) { + t.Fatalf("resolveReviewerRuntimeConfig error = %v, want unsupported effort", err) + } +} + // 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) { diff --git a/internal/stagemodel/resolver.go b/internal/stagemodel/resolver.go index 4d201cb9..d00068ae 100644 --- a/internal/stagemodel/resolver.go +++ b/internal/stagemodel/resolver.go @@ -61,19 +61,6 @@ func ResolveStageModel(req Request) (Result, error) { } tier := config.ModelTier(strings.TrimSpace(string(req.Tier))) 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, - Model: model, - Effort: effort, - Override: true, - }, nil - } if tier != "" && !tier.Valid() { return Result{}, fmt.Errorf("stagemodel: stage %s: model_tier %q is invalid; must be one of small, medium, large", stage, tier) } @@ -87,6 +74,24 @@ func ResolveStageModel(req Request) (Result, error) { } tier = maxModelTier(tier, floorTier) } + if model := strings.TrimSpace(req.ModelOverride); model != "" { + effort := strings.TrimSpace(req.DefaultEffort) + if effortOverride != "" { + effort = effortOverride + } else if stage == StageReviewer && tier != "" { + effort = applyMaxEffort(req.Profile.LLM, tier, effort) + } + if err := config.ValidateEffortForRuntime(req.Profile.LLM, effort); err != nil { + return Result{}, fmt.Errorf("stagemodel: stage %s: %w", stage, err) + } + return Result{ + Stage: stage, + Tier: tier, + Model: model, + Effort: effort, + Override: true, + }, nil + } resolved, ok := config.ResolveModelTier(req.Profile.LLM, tier) if !ok { @@ -97,6 +102,9 @@ func ResolveStageModel(req Request) (Result, error) { if effortOverride != "" { effort = effortOverride } + if err := config.ValidateEffortForRuntime(req.Profile.LLM, effort); err != nil { + return Result{}, fmt.Errorf("stagemodel: stage %s: %w", stage, err) + } return Result{ Stage: stage, Tier: resolved.Tier, diff --git a/internal/stagemodel/resolver_test.go b/internal/stagemodel/resolver_test.go index 6a7357ce..4b903432 100644 --- a/internal/stagemodel/resolver_test.go +++ b/internal/stagemodel/resolver_test.go @@ -138,6 +138,45 @@ func TestResolveStageModelBypassesTierForExplicitOverride(t *testing.T) { } } +func TestResolveStageModelAllowsExtendedPiEffort(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderPi, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterPiRPC, + }} + + got, err := ResolveStageModel(Request{ + Profile: profile, + Stage: StageReviewer, + ModelOverride: "openai-codex/gpt-5.6-luna", + EffortOverride: "max", + }) + if err != nil { + t.Fatalf("ResolveStageModel: %v", err) + } + if got.Effort != "max" { + t.Fatalf("Effort = %q, want max", got.Effort) + } +} + +func TestResolveStageModelRejectsExtendedEffortForUnsupportedRuntime(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderAnthropic, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterClaudeCLI, + }} + + _, err := ResolveStageModel(Request{ + Profile: profile, + Stage: StageReviewer, + ModelOverride: "claude-opus-5", + EffortOverride: "xhigh", + }) + if err == nil || !strings.Contains(err.Error(), `stage reviewer: config: unsupported effort: effort "xhigh" is unsupported`) { + t.Fatalf("ResolveStageModel error = %v", err) + } +} + func TestResolveStageModelErrorsForUnmappedTier(t *testing.T) { profile := config.Profile{LLM: config.LLMConfig{ Provider: config.LLMProviderPi,