feat：add --model flag to override LLM model per review#122
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| entry := cfg.CustomProviders[result.provider] | ||
| entry.Model = result.model | ||
| if len(result.models) > 0 { | ||
| entry.Models = mergeModelLists([]string{result.model}, result.models) |
There was a problem hiding this comment.
Bug: This discards any previously configured models for this custom provider. The first argument should be entry.Models (the existing models) rather than []string{result.model}, to match the pattern used in applyOfficialProviderConfig (line 161). As written, every time the user reconfigures a custom provider through the TUI, any models that were previously in entry.Models but not in result.models will be lost.
For example, if a custom provider had models ["a", "b", "c"] and the user selects model "d" with result.models = ["d"], the result would be ["d"] instead of ["a", "b", "c", "d"].
Suggestion:
| entry.Models = mergeModelLists([]string{result.model}, result.models) | |
| entry.Models = mergeModelLists(entry.Models, result.models) |
| if _, isPreset := llm.LookupProvider(parts[1]); !isPreset { | ||
| return setCustomProviderField(cfg, parts[1], parts[2], key, value) | ||
| } |
There was a problem hiding this comment.
Potential misrouting of provider configuration: When providers.<name>.<field> is used with a name that is NOT a recognized preset (via llm.LookupProvider), the value is silently written to cfg.CustomProviders instead of cfg.Providers. This means:
- If a user typos a preset provider name (e.g.,
providers.anthopic.model), the config goes intoCustomProviderswithout any warning, and the intended preset override is lost. - The
providers.*prefix semantically implies preset providers, whilecustom_providers.*is for custom ones. Mixing them silently can lead to confusing behavior where the user setsproviders.foo.modelbut later finds it undercustom_providersin the saved config.
Consider returning an error when the name is not a known preset, guiding users to use custom_providers.<name>.<field> instead. Alternatively, at minimum log a warning.
| if strings.HasPrefix(value, "[") { | ||
| var models []string | ||
| if err := json.Unmarshal([]byte(value), &models); err != nil { | ||
| if !strings.HasSuffix(value, "]") { | ||
| return nil, err | ||
| } | ||
| value = strings.TrimSpace(strings.TrimSuffix(strings.TrimPrefix(value, "["), "]")) | ||
| return normalizeModelList(strings.Split(value, ",")), nil | ||
| } | ||
| return normalizeModelList(models), nil | ||
| } |
There was a problem hiding this comment.
Fallback parsing may silently corrupt data: When JSON unmarshalling fails AND the value ends with ], the code falls back to stripping brackets and splitting by comma. This means inputs like [model-a, "model-b"] (partially valid JSON) or [model with spaces] would pass through the fallback and produce unexpected results (e.g., '"model-b"' with literal quotes).
The intent seems to be supporting both JSON arrays (["a","b"]) and simple bracketed comma lists ([a, b]). Consider being more explicit: try JSON first, and only fall back if the content between brackets contains no JSON-specific characters (quotes, nested structures). Or simply require one format and return a clear error for invalid input.
|
config.json template： { |
|
Hi @lizhengfeng101,referring to your previous reply, I have added the --model option in the latest version and performed local verification. If you have time, could you please review the code? Please let me know if there are any issues. |
|
@lizhengfeng101，I have closed the other PR, thank you. |
|
Hi @lizhengfeng101，I've closed the other PR, which was based on the new version of the project. If you have some time, could you please help review this one? Feel free to reach out if you have any questions. Looking forward to your reply. |
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Nit on parseModelListValue (config_cmd.go): the fallback logic when the value starts with [ but is not valid JSON has a subtle branch — it checks whether the string ends with ] to decide between returning the raw json.Unmarshal error or falling through to comma-split. This means [foo,bar (missing closing bracket) surfaces a raw JSON error, which is not very user-friendly.
Consider always stripping brackets and comma-splitting when JSON parsing fails, which removes one branch and produces consistent behavior:
func parseModelListValue(value string) ([]string, error) {
value = strings.TrimSpace(value)
if value == "" {
return nil, nil
}
if strings.HasPrefix(value, "[") {
var models []string
if err := json.Unmarshal([]byte(value), &models); err == nil {
return normalizeModelList(models), nil
}
// Not valid JSON — strip brackets and fall through to comma-split.
value = strings.TrimSpace(strings.TrimSuffix(strings.TrimPrefix(value, "["), "]"))
}
return normalizeModelList(strings.Split(value, ",")), nil
}This is simpler and more forgiving for shell users who might type [a,b without the closing bracket. The existing tests still pass with this change.
…to add-custom-model-flag
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Suggestion: validate --model against the provider's known model list before sending the request
Currently, tryProviderConfig applies the model override unconditionally (line 228–229):
if modelOverride != "" {
model = modelOverride
}No validation is performed against preset.Models or entry.Models. If the user passes a typo like --model claude-opsu-4-6, it silently goes through to the API and fails with a cryptic remote error (e.g., HTTP 404).
The expected behavior should be:
-
Provider-based configuration (preset or custom with a
modelslist): IfmodelOverrideis not empty and the provider has a known model list (preset.Modelsmerged withentry.Models), check whether the override is in that list. If not, return an early, clear error such as:model "claude-opsu-4-6" is not available for provider "anthropic"; available models: claude-opus-4-6, claude-sonnet-4-6, ...This catches typos and misconfiguration before making any network call.
-
Manual / legacy configuration (
tryLegacyLlmConfig), environment variable strategies (tryOCREnv,tryCCEnv,tryShellRC): These have no model list to validate against, so the override should be passed through as-is — the remote API is the only authority.
This gives users fast, actionable feedback when a known model list is available, while still allowing unconstrained override in contexts where no list exists.
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
The main branch has had quite a few updates recently. Could you rebase or merge main into your branch to bring it up to date? This will help avoid potential conflicts and make the final merge smoother.
|
I have made modifications on the latest main branch, ensuring that no conflicts will occur during the merge. |
feat: simplify the parseModelListValue function - Remove conditional branch checking for closing bracket - Always strip brackets and fall through to comma-split when JSON parsing fails - Produces more user-friendly behavior for malformed bracket lists like [foo,bar Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> @
|
Hi，@lizhengfeng101. I have made code modifications based on your suggestions. Just simplify the parseModelListValue function. 
And I am working on your other suggestion. It might take some time. Once I have completed the modifications, I will ask you to review the code. |
feat: validate --model flag against provider model list - Validate model override against preset.Models and entry.Models before sending request - Return early, clear error with available models when validation fails - Only validate when a known model list exists (provider-based config) - Skip validation for legacy llm config and env-based strategies (no model list available) - Add comprehensive test coverage for validation scenarios This catches typos and misconfigurations before making network calls, providing fast, actionable feedback to users. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> @
Hi，@lizhengfeng101. Thank you for your suggestions, Now, invalid models be caught prior to making the API call, accompanied by a clear error message and the list of available models. If you have any questions, please feel free to contact me. |
2 participants
Description
Adds a
--modelflag toocr review, allowing users to select or override the configured LLM model for a single review run without changing~/.opencodereview/config.json.This builds on the current provider-based configuration flow by adding support for optional model lists on provider entries, including custom providers. Users can declare which models a custom provider supports, switch models interactively via
ocr config model, or override the model per review run.Examples:
Custom provider model list example:
The flag and provider model list configuration are documented in all README language variants.
Type of Change
How Has This Been Tested?
Tested locally with:
Also manually verified:
ocr review --helpincludes the new--modelflag.custom_providers.<name>.modelscan be configured.ocr config modelsupports custom providers with model lists.ocr review --model <model>overrides the configured model for a single review run.Checklist
gofmt)Related Issues
Closes #83
Verify
Windows 11
Support switching models via TUI—— ocr config model.
Use the true gateway.
Test model——ocr llm test.
Test review
If you use an unsupported model name, an error will be reported.
Support switching provider via TUI——ocr config provider