diff --git a/docs/architecture.md b/docs/architecture.md index a51fa6a2..ac6f8101 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -58,15 +58,16 @@ 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 and its built-in effort preset, if any, for that -tier; cap the selected effort with the selected runtime's `max_effort` entry for -that final tier; then apply +agent floor; resolve the model for that tier; select the runtime `effort_map` +entry for that tier (falling back to built-in presets for built-in models, then agent/stage effort); cap that +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` bypasses model-map resolution. Explicit effort +An explicit `ModelOverride` bypasses model-map resolution, but a request with +a tier still uses its independent `effort_map` preference. 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 diff --git a/docs/init-config-surface.md b/docs/init-config-surface.md index d1bb20a6..49b1f9c2 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -276,3 +276,44 @@ The following flags are intentionally not durable init configuration. - #185: interactive repository routes and host reconciliation. - #186: scripted installer documentation. - #187: maintainable non-interactive init parity flags. + +## Independent Reasoning Effort Preferences + +`llm_runtimes..effort_map` selects reasoning effort independently of +`model_map`. It uses the same tier keys and runtime effort validation as +`max_effort`. A configured preference replaces the agent/stage effort before +applying the ceiling; an explicit CLI effort override wins over both. Exact +model overrides with no tier retain the agent/stage effort. + +```yaml +model_map: + small: gpt-6-luna + medium: gpt-6.1-sol + large: gpt-6.1-sol +effort_map: + small: max + medium: low + large: medium +``` + +Inspect with `cr config llm efforts list --json`, set an entry with +`cr config llm efforts set small max`, or remove it with +`cr config llm efforts unset small`. These commands edit the selected profile's +shared LLM runtime, so other profiles referencing it see the same changes. +Both interactive and non-interactive init preserve the map. + +The first normal live `cr review` for a Codex CLI runtime automatically sets +small to `gpt-6-luna` / `max`, medium to `gpt-6.1-sol` / `low`, and large to +`gpt-6.1-sol` / `medium`. It removes the old effort ceilings, saves the original +config beside `config.yml` as `config.yml.before-review-defaults-v1`, and saves +`review_defaults_version: 1` on the shared runtime. A one-time stderr notice +reports the changes and the model/effort edit commands before runtime startup. +The notice stays off JSON stdout and is shown even with `--quiet`. + +The version marker survives init, so later model pins, effort edits, unsets, +and ceilings are preserved. Other profiles using that runtime share the upgrade. +Dry-run/no-post, posting recovery, other commands, and non-Codex runtimes do not +trigger it. A backup or save failure stops before starting the review; the next +invocation can retry the upgrade. + +Config saves serialize writes and reject stale loaded drafts with a retry message, so a concurrent edit cannot undo migration or be erased by it. The automatic migration reloads and retries when another config edit wins. diff --git a/internal/cmd/configcmd/configcmd.go b/internal/cmd/configcmd/configcmd.go index f8e01eee..519a47f4 100644 --- a/internal/cmd/configcmd/configcmd.go +++ b/internal/cmd/configcmd/configcmd.go @@ -715,7 +715,7 @@ func newLLMCommand(opts *root.Options) *cobra.Command { root.AddJSONFlag(resolveCmd, &resolveJSON) modelsCmd.AddCommand(listCmd, setCmd, unsetCmd, resetCmd, resolveCmd) - llmCmd.AddCommand(modelsCmd) + llmCmd.AddCommand(modelsCmd, newEffortsCommand(opts)) return llmCmd } @@ -890,6 +890,12 @@ func modelMapResult(profileName string, profile config.Profile) modelMapResultVi } func mutateActiveModelMap(opts *root.Options, mutate func(config.Profile, *config.ModelMap) error) (string, error) { + return mutateActiveLLM(opts, func(profile config.Profile, runtime *config.LLMConfig) error { + return mutate(profile, &runtime.ModelMap) + }) +} + +func mutateActiveLLM(opts *root.Options, mutate func(config.Profile, *config.LLMConfig) error) (string, error) { path, cfg, profileName, profile, err := loadActiveProfile(opts) if err != nil { return "", err @@ -898,7 +904,7 @@ func mutateActiveModelMap(opts *root.Options, mutate func(config.Profile, *confi if err != nil { return "", cmderr.Config(err) } - if err := mutate(profile, &runtime.ModelMap); err != nil { + if err := mutate(profile, &runtime); err != nil { return "", err } cfg.LLMRuntimes[runtimeName] = runtime diff --git a/internal/cmd/configcmd/configcmd_test.go b/internal/cmd/configcmd/configcmd_test.go index d1ff93b1..19520c3d 100644 --- a/internal/cmd/configcmd/configcmd_test.go +++ b/internal/cmd/configcmd/configcmd_test.go @@ -1864,7 +1864,15 @@ func TestConfigAgentSourcePreservesUnrelatedProfileFields(t *testing.T) { wantHome := want.Profiles["home"] wantHome.AgentSources = []string{"home-agents", "team/agents"} want.Profiles["home"] = wantHome - if !reflect.DeepEqual(cfg, want) { + cfgJSON, err := json.Marshal(cfg) + if err != nil { + t.Fatal(err) + } + wantJSON, err := json.Marshal(want) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(cfgJSON, wantJSON) { t.Fatalf("config changed unexpectedly:\n got %#v\nwant %#v", cfg, want) } } diff --git a/internal/cmd/configcmd/efforts.go b/internal/cmd/configcmd/efforts.go new file mode 100644 index 00000000..54d4c721 --- /dev/null +++ b/internal/cmd/configcmd/efforts.go @@ -0,0 +1,101 @@ +package configcmd + +import ( + "fmt" + "io" + "strings" + + "github.com/spf13/cobra" + + "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/view" +) + +func newEffortsCommand(opts *root.Options) *cobra.Command { + cmd := &cobra.Command{Use: "efforts", Short: "Inspect and update reasoning effort tier mappings"} + var asJSON bool + list := &cobra.Command{ + Use: "list", Short: "List configured reasoning efforts and ceilings", + Args: exitcode.NoArgs("config llm efforts list takes no arguments"), + RunE: func(_ *cobra.Command, _ []string) error { + _, _, name, profile, err := loadActiveProfile(opts) + if err != nil { + return err + } + result := struct { + ActiveProfile string `json:"active_profile"` + EffortMap config.EffortMap `json:"effort_map"` + MaxEffort config.EffortMap `json:"max_effort"` + }{name, profile.LLM.EffortMap, profile.LLM.MaxEffort} + return view.Render(opts.Stdout, asJSON, result, func(w io.Writer) error { + for _, tier := range config.ModelTiers() { + effort := profile.LLM.EffortMap[string(tier)] + if effort == "" { + effort = "built-in or agent/stage default" + } + ceiling := profile.LLM.MaxEffort[string(tier)] + if ceiling == "" { + ceiling = "uncapped" + } + if _, err := fmt.Fprintf(w, "%s: %s (max: %s)\n", tier, effort, ceiling); err != nil { + return err + } + } + return nil + }) + }, + } + root.AddJSONFlag(list, &asJSON) + set := &cobra.Command{ + Use: "set ", Short: "Set reasoning effort independently of the model", + Args: exitcode.ExactArgs(2, "config llm efforts set requires and "), + RunE: func(_ *cobra.Command, args []string) error { + tier, err := parseModelTierArg(args[0]) + if err != nil { + return err + } + effort := strings.TrimSpace(args[1]) + _, err = mutateActiveLLM(opts, func(_ config.Profile, runtime *config.LLMConfig) error { + if err := config.ValidateEffortForRuntime(*runtime, effort); err != nil { + return exitcode.Usage(err) + } + if effort == "" { + return exitcode.Usage(fmt.Errorf("effort must be non-empty")) + } + if runtime.EffortMap == nil { + runtime.EffortMap = config.EffortMap{} + } + runtime.EffortMap[string(tier)] = effort + return nil + }) + if err != nil { + return err + } + _, err = fmt.Fprintf(opts.Stdout, "Set %s effort: %s\n", tier, effort) + return err + }, + } + unset := &cobra.Command{ + Use: "unset ", Short: "Restore default reasoning effort for a tier", + Args: exitcode.ExactArgs(1, "config llm efforts unset requires "), + RunE: func(_ *cobra.Command, args []string) error { + tier, err := parseModelTierArg(args[0]) + if err != nil { + return err + } + _, err = mutateActiveLLM(opts, func(_ config.Profile, runtime *config.LLMConfig) error { + delete(runtime.EffortMap, string(tier)) + return nil + }) + if err != nil { + return err + } + _, err = fmt.Fprintf(opts.Stdout, "Unset %s effort\n", tier) + return err + }, + } + cmd.AddCommand(list, set, unset) + return cmd +} diff --git a/internal/cmd/configcmd/efforts_test.go b/internal/cmd/configcmd/efforts_test.go new file mode 100644 index 00000000..3f20e878 --- /dev/null +++ b/internal/cmd/configcmd/efforts_test.go @@ -0,0 +1,100 @@ +package configcmd + +import ( + "errors" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" +) + +func TestEffortCommandCannotUndoConcurrentMigration(t *testing.T) { + cfg := config.Normalize(testConfig()) + runtimeName := cfg.Profiles["home"].LLMRuntime + cfg.LLMRuntimes[runtimeName] = config.LLMConfig{Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, Adapter: config.LLMAdapterCodexCLI} + path := saveTestConfig(t, cfg) + previousSave := saveConfigFile + t.Cleanup(func() { saveConfigFile = previousSave }) + saveConfigFile = func(path string, draft config.File) error { + // Interleave migration after the command loads its draft, before its save. + latest, err := config.Load(path) + if err != nil { + return err + } + upgraded, _ := config.UpgradeReviewDefaults(latest, runtimeName) + if err := config.Save(path, upgraded); err != nil { + return err + } + return config.Save(path, draft) + } + cmd, _ := newTestCommand(path) + err := root.Execute(cmd, []string{"--profile", "home", "config", "llm", "efforts", "set", "small", "high"}) + if !errors.Is(err, config.ErrChanged) { + t.Fatalf("stale command must fail safely: %v", err) + } + saveConfigFile = previousSave + cmd, _ = newTestCommand(path) + if err := root.Execute(cmd, []string{"--profile", "home", "config", "llm", "efforts", "set", "small", "high"}); err != nil { + t.Fatal(err) + } + latest, err := config.Load(path) + if err != nil { + t.Fatal(err) + } + llm := latest.LLMRuntimes[runtimeName] + if llm.DefaultsVersion != 1 || llm.ModelMap["medium"] != "gpt-6.1-sol" || llm.EffortMap["small"] != "high" { + t.Fatalf("retry must preserve migration and customization: %#v", llm) + } +} + +func TestEffortCommandsPreserveModelsAndSharedRuntime(t *testing.T) { + cfg := config.Normalize(testConfig()) + home := cfg.Profiles["home"] + llm := config.LLMConfig{Provider: config.LLMProviderOpenAI, Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, ModelMap: config.ModelMap{"small": "gpt-6-luna"}, + MaxEffort: config.EffortMap{"large": "medium"}} + cfg.LLMRuntimes[home.LLMRuntime] = llm + home.LLM = llm + cfg.Profiles["home"] = home + cfg.Profiles["shared"] = home + path := saveTestConfig(t, cfg) + for _, action := range []string{"set", "unset"} { + args := []string{"--profile", "home", "config", "llm", "efforts", action, "small"} + if action == "set" { + args = append(args, "max") + } + cmd, _ := newTestCommand(path) + if err := root.Execute(cmd, args); err != nil { + t.Fatalf("%s: %v", action, err) + } + loaded, err := config.Load(path) + if err != nil { + t.Fatal(err) + } + want := "" + if action == "set" { + want = "max" + } + for _, name := range []string{"home", "shared"} { + got := loaded.Profiles[name].LLM + if got.EffortMap["small"] != want || got.ModelMap["small"] != "gpt-6-luna" || got.MaxEffort["large"] != "medium" { + t.Fatalf("%s after %s: %#v", name, action, got) + } + } + cmd, out := newTestCommand(path) + if err := root.Execute(cmd, []string{"--profile", "home", "config", "llm", "efforts", "list", "--json"}); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), `"effort_map"`) || !strings.Contains(out.String(), `"max_effort"`) { + t.Fatalf("list: %s", out) + } + } + for _, args := range [][]string{{"small", "ultra"}, {"huge", "max"}, {"small", ""}} { + cmd, _ := newTestCommand(path) + if err := root.Execute(cmd, append([]string{"--profile", "home", "config", "llm", "efforts", "set"}, args...)); err == nil { + t.Fatalf("accepted invalid setting: %v", args) + } + } +} diff --git a/internal/cmd/initcmd/initcmd.go b/internal/cmd/initcmd/initcmd.go index 6b1dd642..4c074437 100644 --- a/internal/cmd/initcmd/initcmd.go +++ b/internal/cmd/initcmd/initcmd.go @@ -164,6 +164,9 @@ type initDraft struct { Routes []configedit.RepositoryRouteSpec ModelMapSet bool ModelMap config.ModelMap + DefaultsVersion int + EffortMapSet bool + EffortMap config.EffortMap MaxEffortSet bool MaxEffort config.EffortMap AgentSourcesSet bool @@ -432,8 +435,10 @@ type initLLMRuntimeDraft struct { CredentialStore string CredentialRef string ModelMap config.ModelMap + EffortMap config.EffortMap MaxEffort config.EffortMap ReviewerModelTier config.ModelTier + DefaultsVersion int } type initStore interface { @@ -1125,6 +1130,13 @@ func completeInteractiveInitProfileV2Draft(ctx initPromptContext, draft initDraf } draft.MaxEffortSet = true } + if !draft.EffortMapSet { + if ctx.ExistingProfile != nil { + draft.EffortMap = copyEffortMap(ctx.ExistingProfile.LLM.EffortMap) + draft.DefaultsVersion = ctx.ExistingProfile.LLM.DefaultsVersion + } + draft.EffortMapSet = true + } if !draft.AgentSourcesSet { if ctx.ExistingProfile != nil { draft.AgentSources = append([]string(nil), ctx.ExistingProfile.AgentSources...) @@ -2347,6 +2359,7 @@ func initProfileEditorModelMapLLM(draft initDraft, selectedLLMRuntime string, ru Adapter: config.LLMAdapter(draft.LLMAdapter), ModelMap: copyModelMap(draft.ModelMap), MaxEffort: copyEffortMap(draft.MaxEffort), + EffortMap: copyEffortMap(draft.EffortMap), } if runtime, ok := runtimes[selectedLLMRuntime]; ok { llm.Provider = runtime.Provider @@ -2551,7 +2564,9 @@ func initLLMRuntimeDraftFromSeedDraft(draft initDraft) initLLMRuntimeDraft { Credential: initCredentialLocationIfName(draft.LLMCredentialStore, draft.LLMCredentialRef), ModelMap: copyModelMap(draft.ModelMap), MaxEffort: copyEffortMap(draft.MaxEffort), + EffortMap: copyEffortMap(draft.EffortMap), ReviewerModelTier: config.ModelTier(strings.TrimSpace(draft.LLMReviewerModelTier)), + DefaultsVersion: draft.DefaultsVersion, }) } @@ -2711,8 +2726,11 @@ func applyLLMRuntimeInventorySelection(draft *initDraft, selection string, runti draft.ModelMap = copyModelMap(runtime.ModelMap) draft.ModelMapSet = true draft.MaxEffort = copyEffortMap(runtime.MaxEffort) + draft.EffortMap = copyEffortMap(runtime.EffortMap) draft.MaxEffortSet = true + draft.EffortMapSet = true draft.LLMReviewerModelTier = string(runtime.ReviewerModelTier) + draft.DefaultsVersion = runtime.DefaultsVersion if !draft.AdvancedStorageLabels { draft.LLMCredentialStore = initCredentialStoreDraftValue(runtime.CredentialStore) draft.LLMCredentialRef = runtime.CredentialRef @@ -3115,11 +3133,14 @@ func seedInteractiveInitDraft(requestedProfileName string, existingProfileName s draft.LLMAuth = string(existingProfile.LLM.Auth) draft.LLMAdapter = string(existingProfile.LLM.Adapter) draft.LLMReviewerModelTier = string(existingProfile.LLM.ReviewerModelTier) + draft.DefaultsVersion = existingProfile.LLM.DefaultsVersion 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.EffortMap = copyEffortMap(existingProfile.LLM.EffortMap) draft.MaxEffortSet = true + draft.EffortMapSet = true draft.AgentSources = append([]string(nil), existingProfile.AgentSources...) draft.ReviewPolicy = existingProfile.ReviewPolicy if existingProfile.Reviewer.GitHubAppInstallation != nil { @@ -3411,6 +3432,7 @@ func buildNonInteractiveInitPlan(cmd *cobra.Command, opts *root.Options, flags i } if previousProfile != nil { profile.LLMRuntime = previousProfile.LLMRuntime + profile.LLM.DefaultsVersion = previousProfile.LLM.DefaultsVersion profile.Git.IdentityCache = previousProfile.Git.IdentityCache if previousProfile.LLM.ModelMap != nil { modelMap := make(config.ModelMap, len(previousProfile.LLM.ModelMap)) @@ -3422,6 +3444,9 @@ func buildNonInteractiveInitPlan(cmd *cobra.Command, opts *root.Options, flags i if previousProfile.LLM.MaxEffort != nil { profile.LLM.MaxEffort = copyEffortMap(previousProfile.LLM.MaxEffort) } + if previousProfile.LLM.EffortMap != nil { + profile.LLM.EffortMap = copyEffortMap(previousProfile.LLM.EffortMap) + } if !cmd.Flags().Changed("agent-source") { profile.AgentSources = append([]string(nil), previousProfile.AgentSources...) } @@ -4560,7 +4585,9 @@ func initLLMRuntimeDraftFromConfig(llm config.LLMConfig) initLLMRuntimeDraft { CredentialRef: strings.TrimSpace(llm.Credential.Name), ModelMap: copyModelMap(llm.ModelMap), MaxEffort: copyEffortMap(llm.MaxEffort), + EffortMap: copyEffortMap(llm.EffortMap), ReviewerModelTier: llm.ReviewerModelTier, + DefaultsVersion: llm.DefaultsVersion, } if spec, ok := config.FindLLMRuntimeSpec(runtime.Provider, runtime.Auth, runtime.Adapter); ok && (spec.Auth == runtime.Auth || spec.Auth == "" && runtime.Auth == config.LLMAuthSubscription) { @@ -4579,7 +4606,9 @@ func (runtime initLLMRuntimeDraft) exportConfig() config.LLMConfig { Adapter: runtime.Adapter, ModelMap: copyModelMap(runtime.ModelMap), MaxEffort: copyEffortMap(runtime.MaxEffort), + EffortMap: copyEffortMap(runtime.EffortMap), ReviewerModelTier: runtime.ReviewerModelTier, + DefaultsVersion: runtime.DefaultsVersion, } if runtime.Auth == config.LLMAuthAPIKey { llm.Credential = initCredentialLocation(runtime.CredentialStore, runtime.CredentialRef) @@ -4606,6 +4635,15 @@ func (runtime initLLMRuntimeDraft) identityKey() string { for _, tier := range effortKeys { efforts = append(efforts, tier+"="+strings.TrimSpace(runtime.MaxEffort[tier])) } + preferenceKeys := make([]string, 0, len(runtime.EffortMap)) + for tier := range runtime.EffortMap { + preferenceKeys = append(preferenceKeys, tier) + } + sort.Strings(preferenceKeys) + preferences := make([]string, 0, len(preferenceKeys)) + for _, tier := range preferenceKeys { + preferences = append(preferences, tier+"="+strings.TrimSpace(runtime.EffortMap[tier])) + } return strings.Join([]string{ string(runtime.Provider), string(runtime.Auth), @@ -4614,7 +4652,9 @@ func (runtime initLLMRuntimeDraft) identityKey() string { strings.TrimSpace(runtime.CredentialRef), strings.Join(models, "\x1f"), strings.Join(efforts, "\x1f"), + strings.Join(preferences, "\x1f"), string(runtime.ReviewerModelTier), + strconv.Itoa(runtime.DefaultsVersion), }, "\x00") } @@ -4793,6 +4833,7 @@ func cloneInitLLMConfig(llm config.LLMConfig) config.LLMConfig { } } cloned.MaxEffort = copyEffortMap(llm.MaxEffort) + cloned.EffortMap = copyEffortMap(llm.EffortMap) return cloned } @@ -4879,9 +4920,13 @@ 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)) + profile.LLM.DefaultsVersion = draft.DefaultsVersion if draft.MaxEffortSet { profile.LLM.MaxEffort = copyEffortMap(draft.MaxEffort) } + if draft.EffortMapSet { + profile.LLM.EffortMap = copyEffortMap(draft.EffortMap) + } 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 7d91f3ac..0540eb79 100644 --- a/internal/cmd/initcmd/initcmd_max_effort_test.go +++ b/internal/cmd/initcmd/initcmd_max_effort_test.go @@ -14,14 +14,19 @@ import ( // 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"}, + Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"large": "gpt-5.6-sol"}, + MaxEffort: config.EffortMap{"large": "medium"}, + EffortMap: config.EffortMap{"small": "max", "medium": "low"}, + DefaultsVersion: config.CurrentReviewDefaultsVersion, } got := initLLMRuntimeDraftFromConfig(original).exportConfig() + if got.EffortMap["small"] != "max" || got.EffortMap["medium"] != "low" || got.DefaultsVersion != config.CurrentReviewDefaultsVersion { + t.Fatalf("effort_map after round trip = %#v", got.EffortMap) + } if len(got.MaxEffort) != 1 || got.MaxEffort["large"] != "medium" { t.Fatalf("max_effort after round trip = %#v, want large=medium", got.MaxEffort) @@ -61,6 +66,8 @@ func TestInitNonInteractivePreservesMaxEffortThroughConfigRoundTrip(t *testing.T existing := basicProfile("work") existing.LLM.ModelMap = config.ModelMap{"large": "gpt-5.6-sol"} existing.LLM.MaxEffort = config.EffortMap{"large": "medium"} + existing.LLM.EffortMap = config.EffortMap{"small": "high"} + existing.LLM.DefaultsVersion = config.CurrentReviewDefaultsVersion if err := config.Save(path, config.File{Profiles: map[string]config.Profile{"work": existing}}); err != nil { t.Fatalf("Save initial config: %v", err) } @@ -82,7 +89,33 @@ func TestInitNonInteractivePreservesMaxEffortThroughConfigRoundTrip(t *testing.T if err != nil { t.Fatalf("Load saved config: %v", err) } + if loaded.Profiles["work"].LLM.DefaultsVersion != config.CurrentReviewDefaultsVersion { + t.Fatal("init discarded the one-time upgrade marker") + } + if got := loaded.Profiles["work"].LLM.EffortMap["small"]; got != "high" { + t.Fatalf("saved effort_map.small = %q, want high", got) + } if got := loaded.Profiles["work"].LLM.MaxEffort["large"]; got != "medium" { t.Fatalf("saved max_effort.large = %q, want medium", got) } } + +func TestEffortMapSurvivesInteractiveDraftAndDoesNotAlias(t *testing.T) { + profile := basicProfile("work") + profile.LLM.EffortMap = config.EffortMap{"small": "high"} + profile.LLM.DefaultsVersion = config.CurrentReviewDefaultsVersion + draft := seedInteractiveInitDraft("work", "work", &profile) + if draft.EffortMap["small"] != "high" || !draft.EffortMapSet || draft.DefaultsVersion != config.CurrentReviewDefaultsVersion { + t.Fatalf("seeded effort map = %#v", draft.EffortMap) + } + cloned := cloneInitLLMConfig(profile.LLM) + cloned.EffortMap["small"] = "low" + if profile.LLM.EffortMap["small"] != "high" { + t.Fatal("clone aliases the effort map") + } + base := initLLMRuntimeDraftFromConfig(profile.LLM) + changed := initLLMRuntimeDraftFromConfig(cloned) + if base.identityKey() == changed.identityKey() { + t.Fatal("runtime identity ignores effort_map") + } +} diff --git a/internal/cmd/initcmd/initcmd_test.go b/internal/cmd/initcmd/initcmd_test.go index 8e5c1521..e500d73f 100644 --- a/internal/cmd/initcmd/initcmd_test.go +++ b/internal/cmd/initcmd/initcmd_test.go @@ -3,6 +3,7 @@ package initcmd import ( "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -1127,7 +1128,15 @@ func TestInitPlanApplyPreservesUnrelatedExistingConfig(t *testing.T) { want.LLMRuntimes[name] = runtime } want.LLMRuntimes[after.Profiles["work"].LLMRuntime] = after.LLMRuntimes[after.Profiles["work"].LLMRuntime] - if !reflect.DeepEqual(after, want) { + afterJSON, err := json.Marshal(after) + if err != nil { + t.Fatal(err) + } + wantJSON, err := json.Marshal(want) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(afterJSON, wantJSON) { t.Fatalf("config after init = %#v, want only work profile/runtime added to %#v", after, before) } } diff --git a/internal/cmd/reviewcmd/review_defaults.go b/internal/cmd/reviewcmd/review_defaults.go new file mode 100644 index 00000000..849b7298 --- /dev/null +++ b/internal/cmd/reviewcmd/review_defaults.go @@ -0,0 +1,72 @@ +package reviewcmd + +import ( + "context" + "errors" + "os" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/runlock" +) + +func upgradeReviewDefaults(ctx context.Context, path, runtimeName string) (config.File, bool, error) { + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + lock, err := runlock.Acquire(path + ".review-defaults.lock") + for errors.Is(err, runlock.ErrHeld) { + select { + case <-ctx.Done(): + return config.File{}, false, ctx.Err() + case <-time.After(10 * time.Millisecond): + lock, err = runlock.Acquire(path + ".review-defaults.lock") + } + } + if err != nil { + return config.File{}, false, err + } + defer func() { _ = lock.Release() }() + // Concurrent reviews must re-read the marker and config under the same lock. + cfg, err := config.Load(path) + if err != nil { + return config.File{}, false, err + } + upgraded, changed := config.UpgradeReviewDefaults(cfg, runtimeName) + if !changed { + return cfg, false, nil + } + if err := saveReviewDefaults(path, upgraded); errors.Is(err, config.ErrChanged) { + // A normal config edit won the write lock. Re-read rather than overwrite it. + _ = lock.Release() + return upgradeReviewDefaults(ctx, path, runtimeName) + } else if err != nil { + return config.File{}, false, err + } + loaded, err := config.Load(path) + return loaded, err == nil, err +} + +// Keep the original preferences before the one-time automatic upgrade. Config +// Save already stages and atomically renames the updated file. +func saveReviewDefaults(path string, cfg config.File) error { + // #nosec G304 -- path is the resolved config path. + body, err := os.ReadFile(path) + if err != nil { + return err + } + // #nosec G304 -- backup stays beside the resolved config path. + backup, err := os.OpenFile(path+".before-review-defaults-v1", os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil && !errors.Is(err, os.ErrExist) { + return err + } + if err == nil { + _, writeErr := backup.Write(body) + closeErr := backup.Close() + if err := errors.Join(writeErr, closeErr); err != nil { + // A partial backup must not become a successful backup on retry. + _ = os.Remove(path + ".before-review-defaults-v1") + return err + } + } + return config.Save(path, cfg) +} diff --git a/internal/cmd/reviewcmd/review_defaults_test.go b/internal/cmd/reviewcmd/review_defaults_test.go new file mode 100644 index 00000000..3d74097f --- /dev/null +++ b/internal/cmd/reviewcmd/review_defaults_test.go @@ -0,0 +1,191 @@ +package reviewcmd + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/open-cli-collective/codereview-cli/internal/app" + "github.com/open-cli-collective/codereview-cli/internal/cmd/cmdtest" + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" +) + +func TestReviewAutomaticallyUpgradesCodexBeforeRuntimeAndOnlyOnce(t *testing.T) { + for _, flags := range [][]string{nil, {"--dry-run"}, {"--no-post"}, {"--retry-posts"}} { + t.Run(strings.Join(flags, " "), func(t *testing.T) { + cfg := config.Normalize(testConfig()) + home := cfg.Profiles["home"] + cfg.LLMRuntimes[home.LLMRuntime] = config.LLMConfig{ + Provider: config.LLMProviderOpenAI, Auth: config.LLMAuthSubscription, Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"small": "old-small", "medium": "old-medium"}, MaxEffort: config.EffortMap{"small": "low"}, + } + cfg.Profiles["shared"] = home + path := filepath.Join(t.TempDir(), "config.yml") + if err := config.Save(path, cfg); err != nil { + t.Fatal(err) + } + // #nosec G304 -- config and backup paths are controlled by t.TempDir. + before, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + wantUpgrade := len(flags) == 0 + for attempt := 0; attempt < 2; attempt++ { + var out, errOut *bytes.Buffer + opts := &root.Options{ConfigPath: path, Quiet: true} + called := false + stop := errors.New("stop before review/network activity") + var cmd *cobra.Command + cmd, out, errOut = cmdtest.New(opts, func(cmd *cobra.Command, opts *root.Options) { + RegisterWithFactory(cmd, opts, func(_ context.Context, req app.OpenRequest) (app.Runtime, error) { + called = true + if wantUpgrade { + wantEffort := "max" + if attempt == 1 { + wantEffort = "high" + } + if req.Profile.LLM.EffortMap["small"] != wantEffort || req.Profile.LLM.ModelMap["medium"] != "gpt-6.1-sol" { + t.Fatalf("runtime opened with stale settings: %#v", req.Profile.LLM) + } + if attempt == 0 && !strings.Contains(errOut.String(), "Updated Codex review settings") { + t.Fatal("runtime opened before upgrade notice") + } + } + return app.Runtime{}, stop + }) + }) + args := append([]string{"review", "https://github.com/open-cli-collective/codereview-cli/pull/29"}, flags...) + if err := root.Execute(cmd, args); !errors.Is(err, stop) || !called { + t.Fatalf("Execute: %v, runtime called=%v", err, called) + } + noticed := strings.Contains(errOut.String(), "Updated Codex review settings") + if noticed != (wantUpgrade && attempt == 0) || strings.Contains(out.String(), "Updated Codex") { + t.Fatalf("notice count/output: stdout=%q stderr=%q", out.String(), errOut.String()) + } + loaded, err := config.Load(path) + if err != nil { + t.Fatal(err) + } + if wantUpgrade { + if loaded.Profiles["shared"].LLM.DefaultsVersion != config.CurrentReviewDefaultsVersion { + t.Fatal("shared runtime upgrade was not saved") + } + // #nosec G304 -- config and backup paths are controlled by t.TempDir. + backup, err := os.ReadFile(path + ".before-review-defaults-v1") + if err != nil || !bytes.Equal(backup, before) { + t.Fatalf("original config backup: %v", err) + } + llm := loaded.LLMRuntimes[home.LLMRuntime] + llm.EffortMap["small"] = "high" + loaded.LLMRuntimes[home.LLMRuntime] = llm + if err := config.Save(path, loaded); err != nil { + t.Fatal(err) + } + } else { + // #nosec G304 -- config and backup paths are controlled by t.TempDir. + after, err := os.ReadFile(path) + if err != nil || !bytes.Equal(before, after) { + t.Fatal("dry-run or recovery changed config") + } + } + } + }) + } +} + +func TestConcurrentReviewUpgradesDoNotLoseRuntimeUpdates(t *testing.T) { + cfg := config.Normalize(testConfig()) + cfg.LLMRuntimes["first"] = config.LLMConfig{Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, Adapter: config.LLMAdapterCodexCLI} + cfg.LLMRuntimes["second"] = cfg.LLMRuntimes["first"] + path := filepath.Join(t.TempDir(), "config.yml") + if err := config.Save(path, cfg); err != nil { + t.Fatal(err) + } + errs := make(chan error, 4) + changes := make(chan bool, 4) + for _, name := range []string{"first", "second", "first", "second"} { + go func() { + _, changed, err := upgradeReviewDefaults(context.Background(), path, name) + changes <- changed + errs <- err + }() + } + count := 0 + for range 4 { + if err := <-errs; err != nil { + t.Fatal(err) + } + if <-changes { + count++ + } + } + loaded, err := config.Load(path) + if err != nil || count != 2 || loaded.LLMRuntimes["first"].DefaultsVersion != 1 || loaded.LLMRuntimes["second"].DefaultsVersion != 1 { + t.Fatalf("concurrent upgrade: %v, changes=%d, runtimes=%#v", err, count, loaded.LLMRuntimes) + } +} + +func TestFailedDefaultsSavePreservesOriginalConfig(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yml") + if err := config.Save(path, testConfig()); err != nil { + t.Fatal(err) + } + // #nosec G304 -- path is controlled by t.TempDir. + before, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if err := saveReviewDefaults(path, config.File{}); err == nil { + t.Fatal("invalid config save succeeded") + } + // #nosec G304 -- path is controlled by t.TempDir. + after, err := os.ReadFile(path) + if err != nil || !bytes.Equal(before, after) { + t.Fatal("failed upgrade changed the original config") + } +} + +func TestMigrationAndOrdinaryConfigEditsRejectStaleDrafts(t *testing.T) { + cfg := config.Normalize(testConfig()) + cfg.LLMRuntimes["codex"] = config.LLMConfig{Provider: config.LLMProviderOpenAI, + Auth: config.LLMAuthSubscription, Adapter: config.LLMAdapterCodexCLI} + path := filepath.Join(t.TempDir(), "config.yml") + if err := config.Save(path, cfg); err != nil { + t.Fatal(err) + } + staleEdit, err := config.Load(path) + if err != nil { + t.Fatal(err) + } + if _, _, err := upgradeReviewDefaults(context.Background(), path, "codex"); err != nil { + t.Fatal(err) + } + staleEdit.Data.KeepWorkbench = true + if err := config.Save(path, staleEdit); !errors.Is(err, config.ErrChanged) { + t.Fatalf("stale edit must not undo migration: %v", err) + } + latest, err := config.Load(path) + if err != nil { + t.Fatal(err) + } + staleMigration := latest + latest.Data.KeepWorkbench = true + if err := config.Save(path, latest); err != nil { + t.Fatal(err) + } + if err := saveReviewDefaults(path, staleMigration); !errors.Is(err, config.ErrChanged) { + t.Fatalf("stale migration must not erase ordinary edit: %v", err) + } + final, err := config.Load(path) + if err != nil || !final.Data.KeepWorkbench || final.LLMRuntimes["codex"].DefaultsVersion != 1 { + t.Fatalf("migration and ordinary edit not preserved: %#v, %v", final, err) + } +} diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 4949407a..00c1a185 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -290,6 +290,21 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact if reviewerFast && flags.retryPosts { return exitcode.Usage(fmt.Errorf("fast mode cannot be used with --retry-posts")) } + if !flags.dryRun && !flags.retryPosts { + if _, needed := config.UpgradeReviewDefaults(cfg, profile.LLMRuntime); needed { + var changed bool + cfg, changed, err = upgradeReviewDefaults(ctx, path, profile.LLMRuntime) + if err != nil { + return cmderr.Config(err) + } + profile.LLM = cfg.LLMRuntimes[profile.LLMRuntime] + if changed { + if _, err := fmt.Fprintf(opts.Stderr, "Updated Codex review settings: small=gpt-6-luna/max, medium=gpt-6.1-sol/low, large=gpt-6.1-sol/medium; removed old effort ceilings. Customize with cr --profile %q config llm models set or cr --profile %q config llm efforts set .\n", profileName, profileName); err != nil { + return err + } + } + } + } keepWorkbench := cfg.Data.KeepWorkbench if cmd.Flags().Changed("keep-workbench") { keepWorkbench = flags.keepWorkbench diff --git a/internal/config/config.go b/internal/config/config.go index 986c8e1e..a9e2f65c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -3,6 +3,8 @@ package config import ( "bytes" + "context" + "crypto/sha256" "errors" "fmt" "io" @@ -72,6 +74,9 @@ func (e RepositoryProfileAmbiguityError) Unwrap() error { // File is the root config.yml schema. type File struct { + sourcePath string + sourceDigest [sha256.Size]byte + Secrets SecretsConfig `yaml:"secrets,omitempty" json:"secrets,omitempty"` RepositoryAccess map[string]RepositoryAccessConfig `yaml:"repository_access,omitempty" json:"repository_access,omitempty"` LLMRuntimes map[string]LLMConfig `yaml:"llm_runtimes,omitempty" json:"llm_runtimes,omitempty"` @@ -361,15 +366,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"` + EffortMap EffortMap `yaml:"effort_map,omitempty" json:"effort_map,omitempty"` MaxEffort EffortMap `yaml:"max_effort,omitempty" json:"max_effort,omitempty"` ReviewerModelTier ModelTier `yaml:"reviewer_model_tier,omitempty" json:"reviewer_model_tier,omitempty"` + DefaultsVersion int `yaml:"review_defaults_version,omitempty" json:"review_defaults_version,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. +// EffortMap maps portable tiers to reasoning effort values. Missing entries +// preserve the agent-declared or stage-default effort. type EffortMap map[string]string // ModelTier is a provider-neutral model slot. @@ -924,10 +931,12 @@ func Load(path string) (File, error) { return File{}, err } cfg = cfg.normalized() + cfg.sourcePath = path + cfg.sourceDigest = sha256.Sum256(body) return cfg, nil } -// Save validates and atomically writes config.yml. +// Save validates and atomically writes config.yml, rejecting stale loaded drafts. func Save(path string, cfg File) error { if strings.TrimSpace(path) == "" { return invalid("path is required") @@ -939,6 +948,21 @@ func Save(path string, cfg File) error { return err } cfg = cfg.normalized() + lock, err := lockFile(context.Background(), path) + if err != nil { + return err + } + defer func() { _ = lock.Release() }() + if cfg.sourcePath == path { + // #nosec G304 -- path is the caller-selected config file. + body, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) || (err == nil && sha256.Sum256(body) != cfg.sourceDigest) { + return ErrChanged + } + if err != nil { + return err + } + } dir := filepath.Dir(path) if err := os.MkdirAll(dir, dirPerm); err != nil { @@ -1494,18 +1518,23 @@ 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 err := ValidateEffortForRuntime(llm, ceiling); err != nil { - return invalid("%s.max_effort.%s: %v", field, tier, err) + for name, efforts := range map[string]EffortMap{"effort_map": llm.EffortMap, "max_effort": llm.MaxEffort} { + for tier, ceiling := range efforts { + modelTier := ModelTier(tier) + if !modelTier.Valid() { + return invalid("%s.%s tier %q is invalid", field, name, tier) + } + if strings.TrimSpace(ceiling) == "" { + return invalid("%s.%s.%s is required", field, name, tier) + } + if err := ValidateEffortForRuntime(llm, ceiling); err != nil { + return invalid("%s.%s.%s: %v", field, name, tier, err) + } } } + if llm.DefaultsVersion < 0 { + return invalid("%s.review_defaults_version must be non-negative", field) + } 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) } @@ -2041,6 +2070,15 @@ func llmRuntimeIdentityKey(llm LLMConfig) string { for _, tier := range effortKeys { efforts = append(efforts, tier+"="+strings.TrimSpace(llm.MaxEffort[tier])) } + preferenceKeys := make([]string, 0, len(llm.EffortMap)) + for tier := range llm.EffortMap { + preferenceKeys = append(preferenceKeys, tier) + } + sort.Strings(preferenceKeys) + preferences := make([]string, 0, len(preferenceKeys)) + for _, tier := range preferenceKeys { + preferences = append(preferences, tier+"="+strings.TrimSpace(llm.EffortMap[tier])) + } return strings.Join([]string{ string(llm.Provider), string(llm.Auth), @@ -2049,7 +2087,9 @@ func llmRuntimeIdentityKey(llm LLMConfig) string { llm.Credential.Name, strings.Join(models, "\x1f"), strings.Join(efforts, "\x1f"), + strings.Join(preferences, "\x1f"), string(llm.ReviewerModelTier), + strconv.Itoa(llm.DefaultsVersion), }, "\x00") } @@ -2352,6 +2392,13 @@ func (l LLMConfig) normalized() LLMConfig { } l.MaxEffort = maxEffort } + if len(l.EffortMap) > 0 { + effortMap := make(EffortMap, len(l.EffortMap)) + for tier, effort := range l.EffortMap { + effortMap[strings.TrimSpace(tier)] = strings.TrimSpace(effort) + } + l.EffortMap = effortMap + } return l } @@ -2362,6 +2409,8 @@ func (l LLMConfig) empty() bool { l.Credential.empty() && len(l.ModelMap) == 0 && len(l.MaxEffort) == 0 && + len(l.EffortMap) == 0 && + l.DefaultsVersion == 0 && strings.TrimSpace(string(l.ReviewerModelTier)) == "" } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c9a40bab..33cff83c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -42,6 +42,9 @@ func TestPathUsesCodereviewConfigScope(t *testing.T) { func TestSaveLoadRoundTrip(t *testing.T) { path := filepath.Join(t.TempDir(), "config.yml") want := validFile() + llm := want.LLMRuntimes["home-llm"] + llm.EffortMap = EffortMap{"small": "high", "medium": "low"} + want.LLMRuntimes["home-llm"] = llm if err := Save(path, want); err != nil { t.Fatalf("Save: %v", err) @@ -50,6 +53,7 @@ func TestSaveLoadRoundTrip(t *testing.T) { if err != nil { t.Fatalf("Load: %v", err) } + got.sourcePath, got.sourceDigest = "", [32]byte{} if !reflect.DeepEqual(got, want.normalized()) { t.Fatalf("Load = %#v, want %#v", got, want.normalized()) } diff --git a/internal/config/effort_map_test.go b/internal/config/effort_map_test.go new file mode 100644 index 00000000..100ea9d9 --- /dev/null +++ b/internal/config/effort_map_test.go @@ -0,0 +1,31 @@ +package config + +import "testing" + +func TestEffortMapValidationAndRuntimeIdentity(t *testing.T) { + for _, tc := range []struct { + tier, effort string + valid bool + }{ + {"small", "max", true}, {"medium", "low", true}, {"large", "medium", true}, + {"huge", "low", false}, {"small", "ultra", false}, {"small", "", false}, + } { + cfg := validFile() + cfg.LLMRuntimes["home-llm"] = LLMConfig{Provider: LLMProviderOpenAI, Auth: LLMAuthSubscription, + Adapter: LLMAdapterCodexCLI, EffortMap: EffortMap{tc.tier: tc.effort}} + if err := Validate(cfg); (err == nil) != tc.valid { + t.Fatalf("%s/%s: %v, want valid=%v", tc.tier, tc.effort, err, tc.valid) + } + } + base := LLMConfig{Provider: LLMProviderOpenAI, Auth: LLMAuthSubscription, Adapter: LLMAdapterCodexCLI} + changed := base + changed.EffortMap = EffortMap{" small ": " max "} + normalized := changed.normalized() + if normalized.EffortMap["small"] != "max" || llmRuntimeIdentityKey(base) == llmRuntimeIdentityKey(changed) { + t.Fatal("effort_map must normalize and distinguish runtime identities") + } + normalized.EffortMap["small"] = "low" + if changed.EffortMap[" small "] != " max " { + t.Fatal("normalization aliases effort_map") + } +} diff --git a/internal/config/filelock.go b/internal/config/filelock.go new file mode 100644 index 00000000..ad3290c2 --- /dev/null +++ b/internal/config/filelock.go @@ -0,0 +1,44 @@ +package config + +import ( + "context" + "crypto/sha256" + "errors" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/runlock" +) + +// ErrChanged means another writer changed a loaded draft before it was saved. +var ErrChanged = errors.New("config: changed since loading; retry the command") + +// lockFile serializes config writes across processes. Loaded drafts are also +// checked by Save so waiting for a writer cannot silently overwrite its edits. +func lockFile(ctx context.Context, path string) (*runlock.Lock, error) { + abs, err := filepath.Abs(path) + if err != nil { + return nil, err + } + cache, err := os.UserCacheDir() + if err != nil { + return nil, err + } + // Keep persistent advisory locks outside the removable config directory. + lockPath := filepath.Join(cache, "codereview", "config-locks", fmt.Sprintf("%x.lock", sha256.Sum256([]byte(abs)))) + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + for { + lock, err := runlock.Acquire(lockPath) + if !errors.Is(err, runlock.ErrHeld) { + return lock, err + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(10 * time.Millisecond): + } + } +} diff --git a/internal/config/review_defaults.go b/internal/config/review_defaults.go new file mode 100644 index 00000000..8959e62e --- /dev/null +++ b/internal/config/review_defaults.go @@ -0,0 +1,21 @@ +package config + +// CurrentReviewDefaultsVersion marks the one-time Codex model/effort upgrade. +const CurrentReviewDefaultsVersion = 1 + +// UpgradeReviewDefaults updates a shared Codex runtime once. Subsequent user +// edits, including removing mappings or adding ceilings, remain untouched. +func UpgradeReviewDefaults(cfg File, runtimeName string) (File, bool) { + runtime, ok := cfg.LLMRuntimes[runtimeName] + if !ok || runtime.Provider != LLMProviderOpenAI || runtime.Adapter != LLMAdapterCodexCLI || + runtime.DefaultsVersion >= CurrentReviewDefaultsVersion { + return cfg, false + } + cfg = cfg.normalized() + runtime.ModelMap = ModelMap{"small": "gpt-6-luna", "medium": "gpt-6.1-sol", "large": "gpt-6.1-sol"} + runtime.EffortMap = EffortMap{"small": "max", "medium": "low", "large": "medium"} + runtime.MaxEffort = nil + runtime.DefaultsVersion = CurrentReviewDefaultsVersion + cfg.LLMRuntimes[runtimeName] = runtime + return cfg, true +} diff --git a/internal/config/review_defaults_test.go b/internal/config/review_defaults_test.go new file mode 100644 index 00000000..69fa5ebe --- /dev/null +++ b/internal/config/review_defaults_test.go @@ -0,0 +1,36 @@ +package config + +import "testing" + +func TestReviewDefaultsUpgradeRunsOncePerCodexRuntime(t *testing.T) { + cfg := validFile() + cfg.LLMRuntimes["codex"] = LLMConfig{Provider: LLMProviderOpenAI, Auth: LLMAuthSubscription, + Adapter: LLMAdapterCodexCLI, ModelMap: ModelMap{"medium": "gpt-6-sol"}, + MaxEffort: EffortMap{"small": "low"}} + upgraded, changed := UpgradeReviewDefaults(cfg, "codex") + runtime := upgraded.LLMRuntimes["codex"] + if !changed || runtime.ModelMap["medium"] != "gpt-6.1-sol" || runtime.EffortMap["small"] != "max" || runtime.MaxEffort != nil { + t.Fatalf("upgrade: %#v, changed=%v", runtime, changed) + } + if cfg.LLMRuntimes["codex"].DefaultsVersion != 0 { + t.Fatal("upgrade mutated its input before it was saved") + } + runtime.ModelMap["medium"] = "custom-model" + delete(runtime.EffortMap, "small") + runtime.MaxEffort = EffortMap{"large": "low"} + upgraded.LLMRuntimes["codex"] = runtime + again, changed := UpgradeReviewDefaults(upgraded, "codex") + runtime = again.LLMRuntimes["codex"] + if changed || runtime.ModelMap["medium"] != "custom-model" || runtime.EffortMap["small"] != "" || runtime.MaxEffort["large"] != "low" { + t.Fatal("repeat upgrade overwrote later user edits") + } + for _, name := range []string{"home-llm", "work-llm", "missing"} { + if _, changed := UpgradeReviewDefaults(cfg, name); changed { + t.Fatalf("upgraded non-Codex runtime %s", name) + } + } + cfg.LLMRuntimes["api"] = LLMConfig{Provider: LLMProviderOpenAI, Auth: LLMAuthAPIKey, Adapter: LLMAdapterOpenAIAPI} + if _, changed := UpgradeReviewDefaults(cfg, "api"); changed { + t.Fatal("upgraded the API adapter without verifying its model/tool compatibility") + } +} diff --git a/internal/stagemodel/effort_map_test.go b/internal/stagemodel/effort_map_test.go new file mode 100644 index 00000000..a4400083 --- /dev/null +++ b/internal/stagemodel/effort_map_test.go @@ -0,0 +1,59 @@ +package stagemodel + +import ( + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/config" +) + +func TestIndependentModelAndEffortMaps(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, Auth: config.LLMAuthSubscription, Adapter: config.LLMAdapterCodexCLI, + ModelMap: config.ModelMap{"small": "gpt-6-luna", "medium": "gpt-6.1-sol", "large": "gpt-6.1-sol"}, + EffortMap: config.EffortMap{"small": "max", "medium": "low", "large": "medium"}, + MaxEffort: config.EffortMap{"large": "medium"}, + }} + for _, tc := range []struct { + name string + stage Stage + tier, floor config.ModelTier + modelOverride, effortOverride, wantModel, wantEffort string + }{ + {"small reviewer", StageReviewer, config.ModelTierSmall, "", "", "", "gpt-6-luna", "max"}, + {"medium reviewer", StageReviewer, config.ModelTierMedium, "", "", "", "gpt-6.1-sol", "low"}, + {"large reviewer", StageReviewer, config.ModelTierLarge, "", "", "", "gpt-6.1-sol", "medium"}, + {"selection", StageSelection, config.ModelTierMedium, "", "", "", "gpt-6.1-sol", "low"}, + {"synthesis", StageSynthesis, config.ModelTierMedium, "", "", "", "gpt-6.1-sol", "low"}, + {"thread analysis", StageThreadAnalysis, config.ModelTierMedium, "", "", "", "gpt-6.1-sol", "low"}, + {"approval classifier", StageApprovalOverride, config.ModelTierSmall, "", "", "", "gpt-6-luna", "max"}, + {"post-floor effort", StageReviewer, config.ModelTierSmall, config.ModelTierLarge, "", "", "gpt-6.1-sol", "medium"}, + {"independent model override", StageReviewer, config.ModelTierSmall, "", "custom-model", "", "custom-model", "max"}, + {"effort override wins", StageReviewer, config.ModelTierLarge, "", "", "max", "gpt-6.1-sol", "max"}, + {"both overrides", StageSelection, config.ModelTierMedium, "", "custom-model", "max", "custom-model", "max"}, + {"exact model without tier", StageReviewer, "", "", "custom-model", "", "custom-model", "low"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := ResolveStageModel(Request{Profile: profile, Stage: tc.stage, Tier: tc.tier, FloorTier: tc.floor, + ModelOverride: tc.modelOverride, EffortOverride: tc.effortOverride, DefaultEffort: "low"}) + if err != nil || got.Model != tc.wantModel || got.Effort != tc.wantEffort { + t.Fatalf("got %#v, %v; want %s / %s", got, err, tc.wantModel, tc.wantEffort) + } + }) + } + profile.LLM.MaxEffort["small"] = "medium" + got, err := ResolveStageModel(Request{Profile: profile, Stage: StageReviewer, Tier: config.ModelTierSmall, DefaultEffort: "low"}) + if err != nil || got.Effort != "medium" { + t.Fatalf("ceiling must cap mapped max effort: %#v, %v", got, err) + } +} + +func TestEffortPreferenceOverridesBuiltInPreset(t *testing.T) { + profile := config.Profile{LLM: config.LLMConfig{ + Provider: config.LLMProviderOpenAI, Auth: config.LLMAuthSubscription, Adapter: config.LLMAdapterCodexCLI, + EffortMap: config.EffortMap{"small": "high"}, + }} + got, err := ResolveStageModel(Request{Profile: profile, Stage: StageReviewer, Tier: config.ModelTierSmall, DefaultEffort: "low"}) + if err != nil || got.Model != "gpt-6-luna" || got.Effort != "high" || got.Source != config.ModelMapSourceBuiltIn { + t.Fatalf("built-in model with configured effort: %#v, %v", got, err) + } +} diff --git a/internal/stagemodel/resolver.go b/internal/stagemodel/resolver.go index 61d8c0da..2fc291cd 100644 --- a/internal/stagemodel/resolver.go +++ b/internal/stagemodel/resolver.go @@ -75,7 +75,7 @@ func ResolveStageModel(req Request) (Result, error) { tier = maxModelTier(tier, floorTier) } if model := strings.TrimSpace(req.ModelOverride); model != "" { - effort := strings.TrimSpace(req.DefaultEffort) + effort := configuredEffort(req.Profile.LLM, tier, req.DefaultEffort) if effortOverride != "" { effort = effortOverride } else if stage == StageReviewer && tier != "" { @@ -104,6 +104,7 @@ func ResolveStageModel(req Request) (Result, error) { effort = string(builtIn) } } + effort = configuredEffort(req.Profile.LLM, resolved.Tier, effort) effort = applyMaxEffort(req.Profile.LLM, resolved.Tier, effort) if effortOverride != "" { effort = effortOverride @@ -120,6 +121,14 @@ func ResolveStageModel(req Request) (Result, error) { }, nil } +// configuredEffort selects a runtime preference independently of the model name. +func configuredEffort(llm config.LLMConfig, tier config.ModelTier, fallback string) string { + if effort := strings.TrimSpace(llm.EffortMap[string(tier)]); effort != "" { + return effort + } + return strings.TrimSpace(fallback) +} + // 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 { diff --git a/internal/view/config.go b/internal/view/config.go index 27ff8866..19a12b26 100644 --- a/internal/view/config.go +++ b/internal/view/config.go @@ -271,8 +271,11 @@ func renderConfigModelMap(w io.Writer, llm config.LLMConfig) error { model = "" } suffix := "" + if effort := strings.TrimSpace(llm.EffortMap[row.Tier]); effort != "" { + suffix = fmt.Sprintf(" [effort: %s]", effort) + } if ceiling := strings.TrimSpace(llm.MaxEffort[row.Tier]); ceiling != "" { - suffix = fmt.Sprintf(" [max effort: %s]", 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