feat: add maxtokensfield #2247
feat: add maxtokensfield #2247
Conversation
WalkthroughA new AI-related configuration key, Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
schema/settings.json (1)
50-52: Alias acceptance vs schema validation (same concern as presets schema)If config key aliases are intended in settings.json, they’ll currently be rejected by schema validation unless explicitly listed or normalization occurs pre-validation. See my note on schema/aipresets.json and apply the chosen approach in both places for consistency.
To audit, run:
#!/bin/bash # Verify whether aliases are mentioned anywhere in schema or load path rg -n --no-heading $'ai:maxtokens_field|ai:maxtokensparam|maxtokensfield' schema pkg/wconfig -A 3 -B 3
🧹 Nitpick comments (5)
schema/aipresets.json (1)
36-38: Add a schema default for better UX (and parity with docs)Set a JSON Schema default of "max_tokens" so tools and UIs can surface it automatically.
Apply:
- "ai:maxtokensfield": { - "type": "string" - }, + "ai:maxtokensfield": { + "type": "string", + "default": "max_tokens" + },schema/settings.json (1)
50-52: Add a schema default for consistency with docsMatch the documentation’s default.
Apply:
- "ai:maxtokensfield": { - "type": "string" - }, + "ai:maxtokensfield": { + "type": "string", + "default": "max_tokens" + },docs/docs/config.mdx (2)
49-49: Nit: format the default as codeMinor readability improvement.
Apply:
-| ai:maxtokensfield | string | name of the max tokens field in request to be compatible with the model api (default: max_tokens) | +| ai:maxtokensfield | string | name of the max tokens field in request to be compatible with the model api (default: `max_tokens`) |
49-49: Optional: add a minimal example and note on alias keys (if supported by loader)Consider appending a short example to help first-time users, and a brief note if aliases for the config key are accepted (and normalized) by the loader, to avoid confusion with schema validation.
Example:
"ai:maxtokensfield": "max_completion_tokens", "ai:maxtokens": 1500pkg/wconfig/settingsconfig.go (1)
475-491: normalizeSettingsAliases mutates the input map; consider returning a copy to avoid side effectsCurrent behavior is fine, but copying improves safety for callers that reuse the map.
Apply:
-func normalizeSettingsAliases(m waveobj.MetaMapType) waveobj.MetaMapType { - if m == nil { - return nil - } - for alias, canonical := range settingsAliasKeyMap { - if val, ok := m[alias]; ok { - if _, exists := m[canonical]; !exists { - m[canonical] = val - } - delete(m, alias) - } - } - return m -} +func normalizeSettingsAliases(m waveobj.MetaMapType) waveobj.MetaMapType { + if m == nil || len(m) == 0 { + return m + } + out := make(waveobj.MetaMapType, len(m)) + for k, v := range m { + out[k] = v + } + for alias, canonical := range settingsAliasKeyMap { + if val, ok := out[alias]; ok { + if _, exists := out[canonical]; !exists { + out[canonical] = val + } + delete(out, alias) + } + } + return out +}Optional: add a table-driven unit test to lock behavior:
// pkg/wconfig/settingsconfig_test.go package wconfig import ( "reflect" "testing" "github.com/wavetermdev/waveterm/pkg/waveobj" ) func TestNormalizeSettingsAliases(t *testing.T) { in := waveobj.MetaMapType{ "ai:max_tokens": 123, "ai:max_tokens_param": "max_completion_tokens", "ai:maxtokens": 456, // canonical should win over alias } out := normalizeSettingsAliases(in) if _, ok := out["ai:max_tokens"]; ok { t.Fatalf("alias key ai:max_tokens should be removed") } if got := out["ai:maxtokens"]; !reflect.DeepEqual(got, 456) { t.Fatalf("canonical value should win; got=%v want=456", got) } if got := out["ai:maxtokensfield"]; got != "max_completion_tokens" { t.Fatalf("alias should map to canonical; got=%v", got) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/config.mdx(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(5 hunks)schema/aipresets.json(1 hunks)schema/settings.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/wconfig/settingsconfig.go (1)
pkg/waveobj/metamap.go (1)
MetaMapType(8-8)
🔇 Additional comments (6)
pkg/wconfig/metaconsts.go (2)
24-24: Constant addition looks goodNew key aligns with the PR objective and existing naming patterns.
24-24: No further action needed: new key is fully integrated
References toConfigKey_AiMaxTokensFieldand"ai:maxtokensfield"are present in:
- JSON schemas (
schema/settings.json,schema/aipresets.json)- Go settings struct (
pkg/wconfig/settingsconfig.go) including alias map- Frontend types (
frontend/types/gotypes.d.ts)- Documentation (
docs/docs/config.mdx)There’s no local generator in this repo to update. Downstream usage is already picking up the new key.
frontend/types/gotypes.d.ts (2)
695-695: TS SettingsType update looks goodProperty is correctly added and optional, consistent with schema.
695-695: Verify generator updates for ai:maxtokensfieldThe generated types file (
frontend/types/gotypes.d.ts) already includes"ai:maxtokensfield"?: string;in the same block as the other AI keys, so MetaType is correct. However, the path
cmd/generatewasn’t found in the repo—please:• Locate your generation script (it may live under a different directory or filename)
• Confirm that it emits"ai:maxtokensfield"in both SettingsType and MetaTypepkg/wconfig/settingsconfig.go (2)
46-46: Add AiMaxTokensField to settings structs — looks correctString type with correct JSON tag and parity across AiSettingsType and SettingsType. No issues.
Also applies to: 70-70
595-595: Normalizing aliases in SetBaseConfigValue is correctEnsures type-checking and persistence use canonical keys. Good call.
| if jsonTag == "settings" { | ||
| configPart = normalizeSettingsAliases(configPart) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Alias normalization is only applied to "settings"; consider normalizing inside "presets" too
If users set aliases within presets (e.g., in aipresets.json), those aliases won’t be normalized here. Consider normalizing aliases for each preset entry to keep behavior consistent.
Proposed change:
- if jsonTag == "settings" {
- configPart = normalizeSettingsAliases(configPart)
- }
+ if jsonTag == "settings" {
+ configPart = normalizeSettingsAliases(configPart)
+ } else if jsonTag == "presets" {
+ // Normalize aliases within each preset map
+ for name, raw := range configPart {
+ if sub, ok := raw.(map[string]any); ok {
+ mm := waveobj.MetaMapType(sub)
+ configPart[name] = normalizeSettingsAliases(mm)
+ }
+ }
+ }To check whether presets rely on aliases today, run:
🏁 Script executed:
#!/bin/bash
# Scan for ai:maxtokensfield and aliases across repo to gauge current usage.
rg -n --hidden -S $'ai:maxtokensfield|ai:max_tokens_field|ai:max_tokens_param|ai:maxtokenfields|ai:max_tokens'Length of output: 1240
Ensure alias normalization for “presets” as well
Users can define the same ai:… aliases in your aipresets.json under the presets key, but our loader only normalizes aliases inside the top‐level settings section. We should apply the same normalizeSettingsAliases logic to each preset entry so that aliases like ai:maxtokensfield are handled consistently.
Files/locations to update:
- pkg/wconfig/settingsconfig.go (around lines 410–412)
Suggested diff:
@@ -409,6 +409,16 @@
if jsonTag == "settings" {
configPart = normalizeSettingsAliases(configPart)
}
+ else if jsonTag == "presets" {
+ // Normalize aliases in each preset entry
+ for name, raw := range configPart {
+ if subMap, ok := raw.(map[string]any); ok {
+ mm := waveobj.MetaMapType(subMap)
+ configPart[name] = normalizeSettingsAliases(mm)
+ }
+ }
+ }This ensures any alias mappings under "presets" get normalized exactly as they are under "settings".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if jsonTag == "settings" { | |
| configPart = normalizeSettingsAliases(configPart) | |
| } | |
| if jsonTag == "settings" { | |
| configPart = normalizeSettingsAliases(configPart) | |
| } else if jsonTag == "presets" { | |
| // Normalize aliases in each preset entry | |
| for name, raw := range configPart { | |
| if subMap, ok := raw.(map[string]any); ok { | |
| mm := waveobj.MetaMapType(subMap) | |
| configPart[name] = normalizeSettingsAliases(mm) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/wconfig/settingsconfig.go around lines 410 to 412, the code currently
normalizes aliases only for the "settings" section but not for the "presets"
section. To fix this, add logic to iterate over each entry in the "presets" key
and apply the normalizeSettingsAliases function to each preset entry, ensuring
alias normalization is consistent for both "settings" and "presets".
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
schema/aipresets.json (2)
36-38: Add default and basic validation for ai:maxtokensfield
Set an explicit default and prevent empty strings to tighten validation and reflect the documented default.- "ai:maxtokensfield": { - "type": "string" - }, + "ai:maxtokensfield": { + "type": "string", + "minLength": 1, + "default": "max_tokens", + "description": "Canonical key for the request field that carries the maximum tokens value (e.g., max_tokens, max_completion_tokens)." + },
33-35: Consider constraining ai:maxtokens to integer with a minimum
Token counts are integral by nature. Tighten the schema to avoid floats and non-positive values.- "ai:maxtokens": { - "type": "number" - }, + "ai:maxtokens": { + "type": "integer", + "minimum": 1, + "description": "Maximum number of tokens to generate." + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
schema/aipresets.json(1 hunks)
🔇 Additional comments (2)
schema/aipresets.json (2)
39-53: Aliases included in schema — resolves earlier validation concern
Including deprecated alias properties while keeping additionalProperties: false is the right call; this aligns schema validation with runtime normalization.
39-53: Alias normalization covers schema aliases
ThesettingsAliasKeyMapin pkg/wconfig/settingsconfig.go already includes mappings for all three schema aliases—ai:max_tokens_field,ai:max_tokens_param, andai:maxtokenfields—to the canonicalai:maxtokensfield, so no further changes are needed.• pkg/wconfig/settingsconfig.go:469–474 — defines:
“ai:max_tokens_param”: “ai:maxtokensfield”
“ai:max_tokens_field”: “ai:maxtokensfield”
“ai:maxtokenfields”: “ai:maxtokensfield”
Title
Add
maxtokensfieldconfig to support custom max token param namesDescription
Some models do not support the
max_tokensparameter and require a different field name (e.g.,max_completion_tokens).Currently, this causes errors such as:
{ "error": { "message": "Unsupported parameter: 'max_tokens' is not supported with this model. Use 'max_completion_tokens' instead.", "type": "invalid_request_error", "param": "max_tokens", "code": "unsupported_parameter" } }Changes in this PR
Added a new config field:
"ai:maxtokensfield"(string) → defines the name of the max tokens parameter to send in requests (default:"max_tokens")Updated schema, settings types, and docs accordingly
Added alias support for common variations (
max_tokens_param,max_tokens_field, etc.)Example
Impact
This change allows compatibility with models that require a different max tokens parameter name, avoiding request errors without hardcoding model-specific logic.
First time contributing here — happy to adjust anything if needed 🙏