feat(config): configure model and reasoning effort independently - #637
Conversation
…c defaults upgrade
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: b2328f914667
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
go:implementation-tests (1 finding)
Minor - internal/stagemodel/effort_map_test.go:12
Add a case proving effort_map overrides built-in effort presets when model_map is absent. Every case here configures all model tiers, so none exercises the BuiltInEffort branch; the existing built-in tests omit effort_map. Moving configuredEffort before built-in effort selection would therefore pass both suites while breaking the central precedence contract. Resolve a built-in small tier with effort_map.small=high and assert that the built-in model is retained while effort becomes high.
structure:repo-health (1 finding)
Major - internal/cmd/reviewcmd/review_defaults.go:16
The migration lock protects only concurrent migrations, although the migration rewrites the entire shared config. Config-edit commands, including mutateActiveLLM, load and save without acquiring this lock, and config.Save only provides atomic replacement. If an effort/model edit loads before migration and saves afterward, it restores the old version marker and mappings, causing another migration to overwrite the user's edit on the next review. Conversely, a migration can erase an unrelated config edit saved after its load. Put configuration read-modify-write operations behind a shared transaction lock used by migration and config mutations, and add a concurrency test covering migration against an ordinary config edit.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 16 assigned files (18 inspected across reviewers):internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/efforts.go,internal/cmd/configcmd/efforts_test.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_max_effort_test.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/review_defaults_test.go,internal/cmd/reviewcmd/reviewcmd.go,internal/config/config.go,internal/config/config_test.go,internal/config/effort_map_test.go,internal/config/review_defaults.go,internal/config/review_defaults_test.go,internal/stagemodel/effort_map_test.go,internal/stagemodel/resolver.go,internal/view/config.go; skipped: none; constraints: Default test builds failed because clang mishandled the workspace path containing spaces. Tests were rerun with CGO_ENABLED=0. Five affected packages passed; configcmd failed TestKeychainProbeManifestMatchesConfigShowContract. The new effort-command test passed separately. Review limited to assigned Go implementation and behavioral test coverage.policies:conventions— complete (broad); inspected 10 assigned files (18 inspected across reviewers):docs/architecture.md,docs/init-config-surface.md,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/efforts.go,internal/cmd/initcmd/initcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/config/config.go,internal/config/review_defaults.go,internal/view/config.go; skipped: none; constraints: Review limited to convention adherence in the assigned files, with supporting code and tests inspected for context. Shared standards and automation had no local convenience copies, and their canonical GitHub URLs could not be fetched; no assumptions were made about their contents. Tests were inspected but not executed.structure:repo-health— complete (broad); inspected 13 assigned files (18 inspected across reviewers):docs/architecture.md,docs/init-config-surface.md,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/efforts.go,internal/cmd/initcmd/initcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/review_defaults_test.go,internal/cmd/reviewcmd/reviewcmd.go,internal/config/config.go,internal/config/review_defaults.go,internal/config/review_defaults_test.go,internal/stagemodel/resolver.go,internal/view/config.go; skipped: none; constraints: Default test run failed because clang mishandled a cache path containing spaces. With CGO_ENABLED=0, four focused packages passed; configcmd failed TestKeychainProbeManifestMatchesConfigShowContract. Review focused on assigned files and configuration ownership, migration safety, and durable contracts.
Inspected files (18)
docs/architecture.mddocs/init-config-surface.mdinternal/cmd/configcmd/configcmd.gointernal/cmd/configcmd/efforts.gointernal/cmd/configcmd/efforts_test.gointernal/cmd/initcmd/initcmd.gointernal/cmd/initcmd/initcmd_max_effort_test.gointernal/cmd/reviewcmd/review_defaults.gointernal/cmd/reviewcmd/review_defaults_test.gointernal/cmd/reviewcmd/reviewcmd.gointernal/config/config.gointernal/config/config_test.gointernal/config/effort_map_test.gointernal/config/review_defaults.gointernal/config/review_defaults_test.gointernal/stagemodel/effort_map_test.gointernal/stagemodel/resolver.gointernal/view/config.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 03s | gpt-6.1-sol | cr 0.10.317
| Field | Value |
|---|---|
| Model | gpt-6.1-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-sol |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 3m 03s wall · 5m 17s compute |
| Cost | unavailable |
| Tokens | 868.6k in / 5.3k out |
Per-workstream usage
orchestrator-selection— gpt-6.1-sol- In: 39.0k
- Out: 755
- Cache read: 34.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 46s
go:implementation-tests— gpt-6.1-sol- In: 364.7k
- Out: 1.7k
- Cache read: 317.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 52s
policies:conventions— gpt-6.1-sol- In: 127.4k
- Out: 549
- Cache read: 97.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 47s
structure:repo-health— gpt-6.1-sol- In: 275.7k
- Out: 1.4k
- Cache read: 238.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 40s
orchestrator-rollup— gpt-6.1-sol- In: 61.8k
- Out: 892
- Cache read: 46.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 11s
| 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") |
There was a problem hiding this comment.
The migration lock protects only concurrent migrations, although the migration rewrites the entire shared config. Config-edit commands, including mutateActiveLLM, load and save without acquiring this lock, and config.Save only provides atomic replacement. If an effort/model edit loads before migration and saves afterward, it restores the old version marker and mappings, causing another migration to overwrite the user's edit on the next review. Conversely, a migration can erase an unrelated config edit saved after its load. Put configuration read-modify-write operations behind a shared transaction lock used by migration and config mutations, and add a concurrency test covering migration against an ordinary config edit.
Reply inline to this comment.
| 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"}, |
There was a problem hiding this comment.
Add a case proving effort_map overrides built-in effort presets when model_map is absent. Every case here configures all model tiers, so none exercises the BuiltInEffort branch; the existing built-in tests omit effort_map. Moving configuredEffort before built-in effort selection would therefore pass both suites while breaking the central precedence contract. Resolve a built-in small tier with effort_map.small=high and assert that the built-in model is retained while effort becomes high.
Reply inline to this comment.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: a49dfa6449d4
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 19 assigned files (21 inspected across reviewers):internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/configcmd_test.go,internal/cmd/configcmd/efforts.go,internal/cmd/configcmd/efforts_test.go,internal/cmd/initcmd/initcmd.go,internal/cmd/initcmd/initcmd_max_effort_test.go,internal/cmd/initcmd/initcmd_test.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/review_defaults_test.go,internal/cmd/reviewcmd/reviewcmd.go,internal/config/config.go,internal/config/config_test.go,internal/config/effort_map_test.go,internal/config/filelock.go,internal/config/review_defaults.go,internal/config/review_defaults_test.go,internal/stagemodel/effort_map_test.go,internal/stagemodel/resolver.go,internal/view/config.go; skipped: none; constraints: Review limited to assigned Go implementation and behavioral test coverage. Tests were inspected but not executed because the sandbox is read-only.policies:conventions— complete (constrained); inspected 10 assigned files (21 inspected across reviewers):docs/architecture.md,docs/init-config-surface.md,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/efforts.go,internal/cmd/initcmd/initcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/reviewcmd.go,internal/config/config.go,internal/config/review_defaults.go,internal/view/config.go; skipped: none; constraints: Review limited to convention adherence in assigned files, with supporting implementation and tests inspected for context. Shared standards and automation have no local convenience copies; canonical URLs were unavailable during the earlier review, so their contents were not assumed. Tests were inspected but not executed in the read-only environment.structure:repo-health— complete (constrained); inspected 13 assigned files (21 inspected across reviewers):docs/architecture.md,docs/init-config-surface.md,internal/cmd/configcmd/configcmd.go,internal/cmd/configcmd/efforts.go,internal/cmd/initcmd/initcmd.go,internal/cmd/reviewcmd/review_defaults.go,internal/cmd/reviewcmd/review_defaults_test.go,internal/cmd/reviewcmd/reviewcmd.go,internal/config/config.go,internal/config/review_defaults.go,internal/config/review_defaults_test.go,internal/stagemodel/resolver.go,internal/view/config.go; skipped: none; constraints: Focused on assigned files and the updated configuration conflict handling; inspected supporting lock code and regression tests. Read-only sandbox prevented running tests that create build caches, temporary config files, and advisory lock files.
Inspected files (21)
docs/architecture.mddocs/init-config-surface.mdinternal/cmd/configcmd/configcmd.gointernal/cmd/configcmd/configcmd_test.gointernal/cmd/configcmd/efforts.gointernal/cmd/configcmd/efforts_test.gointernal/cmd/initcmd/initcmd.gointernal/cmd/initcmd/initcmd_max_effort_test.gointernal/cmd/initcmd/initcmd_test.gointernal/cmd/reviewcmd/review_defaults.gointernal/cmd/reviewcmd/review_defaults_test.gointernal/cmd/reviewcmd/reviewcmd.gointernal/config/config.gointernal/config/config_test.gointernal/config/effort_map_test.gointernal/config/filelock.gointernal/config/review_defaults.gointernal/config/review_defaults_test.gointernal/stagemodel/effort_map_test.gointernal/stagemodel/resolver.gointernal/view/config.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 28s | gpt-6.1-sol | cr 0.10.317
| Field | Value |
|---|---|
| Model | gpt-6.1-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-sol |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 2m 28s wall · 3m 42s compute |
| Cost | unavailable |
| Tokens | 1.9M in / 7.7k out |
Per-workstream usage
go:implementation-tests— gpt-6.1-sol- In: 942.5k
- Out: 3.4k
- Cache read: 856.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 48s
policies:conventions— gpt-6.1-sol- In: 292.6k
- Out: 1.1k
- Cache read: 240.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 38s
structure:repo-health— gpt-6.1-sol- In: 594.1k
- Out: 2.3k
- Cache read: 528.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 06s
orchestrator-rollup— gpt-6.1-sol- In: 86.8k
- Out: 962
- Cache read: 69.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 8s
1 participant
Separate per-tier model names from reasoning effort via
effort_mapandcr config llm efforts list/set/unset. Explicit effort preferences take precedence over built-in presets; effort overrides and ceilings retain their existing behavior.On the first normal review, existing Codex CLI runtimes automatically migrate to small=gpt-6-luna/max, medium=gpt-6.1-sol/low, and large=gpt-6.1-sol/medium. The migration removes old ceilings, preserves a private backup, prints a one-time customization notice on stderr, and preserves later user changes. Dry runs and recovery invocations leave configuration unchanged. Config writes serialize and reject stale loaded drafts, preventing concurrent edits from undoing migration or being overwritten; migration reloads and retries after conflicts.
Validation: full Go suite; focused migration race tests; changed-code lint; CLI build.
Closes #636.