From 03005b5d299b62fc1ed7eee55506c5a687243948 Mon Sep 17 00:00:00 2001 From: hkfires <10558748+hkfires@users.noreply.github.com> Date: Sun, 18 Jan 2026 11:30:53 +0800 Subject: [PATCH] refactor(thinking): add Gemini family provider grouping for strict validation --- internal/logging/global_logger.go | 2 +- internal/thinking/validate.go | 107 +++++++++++-------- sdk/translator/registry.go | 62 +++-------- test/thinking_conversion_test.go | 168 +++++++++++++++++++++++++++--- 4 files changed, 230 insertions(+), 109 deletions(-) diff --git a/internal/logging/global_logger.go b/internal/logging/global_logger.go index 63c7af46..a5630964 100644 --- a/internal/logging/global_logger.go +++ b/internal/logging/global_logger.go @@ -30,7 +30,7 @@ var ( type LogFormatter struct{} // logFieldOrder defines the display order for common log fields. -var logFieldOrder = []string{"provider", "model", "mode", "budget", "level", "original_value", "min", "max", "clamped_to", "error"} +var logFieldOrder = []string{"provider", "model", "mode", "budget", "level", "original_value", "original_level", "min", "max", "clamped_to", "error"} // Format renders a single log entry with custom formatting. func (m *LogFormatter) Format(entry *log.Entry) ([]byte, error) { diff --git a/internal/thinking/validate.go b/internal/thinking/validate.go index 853e187d..5ce113f7 100644 --- a/internal/thinking/validate.go +++ b/internal/thinking/validate.go @@ -35,7 +35,6 @@ import ( // - Hybrid model → preserve original format func ValidateConfig(config ThinkingConfig, modelInfo *registry.ModelInfo, fromFormat, toFormat string) (*ThinkingConfig, error) { fromFormat, toFormat = strings.ToLower(strings.TrimSpace(fromFormat)), strings.ToLower(strings.TrimSpace(toFormat)) - normalized := config model := "unknown" support := (*registry.ThinkingSupport)(nil) if modelInfo != nil { @@ -49,106 +48,108 @@ func ValidateConfig(config ThinkingConfig, modelInfo *registry.ModelInfo, fromFo if config.Mode != ModeNone { return nil, NewThinkingErrorWithModel(ErrThinkingNotSupported, "thinking not supported for this model", model) } - return &normalized, nil + return &config, nil } allowClampUnsupported := isBudgetBasedProvider(fromFormat) && isLevelBasedProvider(toFormat) - strictBudget := fromFormat != "" && fromFormat == toFormat + strictBudget := fromFormat != "" && isSameProviderFamily(fromFormat, toFormat) + budgetDerivedFromLevel := false capability := detectModelCapability(modelInfo) switch capability { case CapabilityBudgetOnly: - if normalized.Mode == ModeLevel { - if normalized.Level == LevelAuto { + if config.Mode == ModeLevel { + if config.Level == LevelAuto { break } - budget, ok := ConvertLevelToBudget(string(normalized.Level)) + budget, ok := ConvertLevelToBudget(string(config.Level)) if !ok { - return nil, NewThinkingError(ErrUnknownLevel, fmt.Sprintf("unknown level: %s", normalized.Level)) + return nil, NewThinkingError(ErrUnknownLevel, fmt.Sprintf("unknown level: %s", config.Level)) } - normalized.Mode = ModeBudget - normalized.Budget = budget - normalized.Level = "" + config.Mode = ModeBudget + config.Budget = budget + config.Level = "" + budgetDerivedFromLevel = true } case CapabilityLevelOnly: - if normalized.Mode == ModeBudget { - level, ok := ConvertBudgetToLevel(normalized.Budget) + if config.Mode == ModeBudget { + level, ok := ConvertBudgetToLevel(config.Budget) if !ok { - return nil, NewThinkingError(ErrUnknownLevel, fmt.Sprintf("budget %d cannot be converted to a valid level", normalized.Budget)) + return nil, NewThinkingError(ErrUnknownLevel, fmt.Sprintf("budget %d cannot be converted to a valid level", config.Budget)) } // When converting Budget -> Level for level-only models, clamp the derived standard level // to the nearest supported level. Special values (none/auto) are preserved. - normalized.Mode = ModeLevel - normalized.Level = clampLevel(ThinkingLevel(level), modelInfo, toFormat) - normalized.Budget = 0 + config.Mode = ModeLevel + config.Level = clampLevel(ThinkingLevel(level), modelInfo, toFormat) + config.Budget = 0 } case CapabilityHybrid: } - if normalized.Mode == ModeLevel && normalized.Level == LevelNone { - normalized.Mode = ModeNone - normalized.Budget = 0 - normalized.Level = "" + if config.Mode == ModeLevel && config.Level == LevelNone { + config.Mode = ModeNone + config.Budget = 0 + config.Level = "" } - if normalized.Mode == ModeLevel && normalized.Level == LevelAuto { - normalized.Mode = ModeAuto - normalized.Budget = -1 - normalized.Level = "" + if config.Mode == ModeLevel && config.Level == LevelAuto { + config.Mode = ModeAuto + config.Budget = -1 + config.Level = "" } - if normalized.Mode == ModeBudget && normalized.Budget == 0 { - normalized.Mode = ModeNone - normalized.Level = "" + if config.Mode == ModeBudget && config.Budget == 0 { + config.Mode = ModeNone + config.Level = "" } - if len(support.Levels) > 0 && normalized.Mode == ModeLevel { - if !isLevelSupported(string(normalized.Level), support.Levels) { + if len(support.Levels) > 0 && config.Mode == ModeLevel { + if !isLevelSupported(string(config.Level), support.Levels) { if allowClampUnsupported { - normalized.Level = clampLevel(normalized.Level, modelInfo, toFormat) + config.Level = clampLevel(config.Level, modelInfo, toFormat) } - if !isLevelSupported(string(normalized.Level), support.Levels) { + if !isLevelSupported(string(config.Level), support.Levels) { // User explicitly specified an unsupported level - return error // (budget-derived levels may be clamped based on source format) validLevels := normalizeLevels(support.Levels) - message := fmt.Sprintf("level %q not supported, valid levels: %s", strings.ToLower(string(normalized.Level)), strings.Join(validLevels, ", ")) + message := fmt.Sprintf("level %q not supported, valid levels: %s", strings.ToLower(string(config.Level)), strings.Join(validLevels, ", ")) return nil, NewThinkingError(ErrLevelNotSupported, message) } } } - if strictBudget && normalized.Mode == ModeBudget { + if strictBudget && config.Mode == ModeBudget && !budgetDerivedFromLevel { min, max := support.Min, support.Max if min != 0 || max != 0 { - if normalized.Budget < min || normalized.Budget > max || (normalized.Budget == 0 && !support.ZeroAllowed) { - message := fmt.Sprintf("budget %d out of range [%d,%d]", normalized.Budget, min, max) + if config.Budget < min || config.Budget > max || (config.Budget == 0 && !support.ZeroAllowed) { + message := fmt.Sprintf("budget %d out of range [%d,%d]", config.Budget, min, max) return nil, NewThinkingError(ErrBudgetOutOfRange, message) } } } // Convert ModeAuto to mid-range if dynamic not allowed - if normalized.Mode == ModeAuto && !support.DynamicAllowed { - normalized = convertAutoToMidRange(normalized, support, toFormat, model) + if config.Mode == ModeAuto && !support.DynamicAllowed { + config = convertAutoToMidRange(config, support, toFormat, model) } - if normalized.Mode == ModeNone && toFormat == "claude" { + if config.Mode == ModeNone && toFormat == "claude" { // Claude supports explicit disable via thinking.type="disabled". // Keep Budget=0 so applier can omit budget_tokens. - normalized.Budget = 0 - normalized.Level = "" + config.Budget = 0 + config.Level = "" } else { - switch normalized.Mode { + switch config.Mode { case ModeBudget, ModeAuto, ModeNone: - normalized.Budget = clampBudget(normalized.Budget, modelInfo, toFormat) + config.Budget = clampBudget(config.Budget, modelInfo, toFormat) } // ModeNone with clamped Budget > 0: set Level to lowest for Level-only/Hybrid models // This ensures Apply layer doesn't need to access support.Levels - if normalized.Mode == ModeNone && normalized.Budget > 0 && len(support.Levels) > 0 { - normalized.Level = ThinkingLevel(support.Levels[0]) + if config.Mode == ModeNone && config.Budget > 0 && len(support.Levels) > 0 { + config.Level = ThinkingLevel(support.Levels[0]) } } - return &normalized, nil + return &config, nil } // convertAutoToMidRange converts ModeAuto to a mid-range value when dynamic is not allowed. @@ -340,6 +341,22 @@ func isLevelBasedProvider(provider string) bool { } } +func isGeminiFamily(provider string) bool { + switch provider { + case "gemini", "gemini-cli", "antigravity": + return true + default: + return false + } +} + +func isSameProviderFamily(from, to string) bool { + if from == to { + return true + } + return isGeminiFamily(from) && isGeminiFamily(to) +} + func abs(x int) int { if x < 0 { return -x diff --git a/sdk/translator/registry.go b/sdk/translator/registry.go index 882e80f6..ace97137 100644 --- a/sdk/translator/registry.go +++ b/sdk/translator/registry.go @@ -38,31 +38,15 @@ func (r *Registry) Register(from, to Format, request RequestTransform, response r.responses[from][to] = response } -// formatAliases returns compatible aliases for a format, ordered by preference. -func formatAliases(format Format) []Format { - switch format { - case "codex": - return []Format{"codex", "openai-response"} - case "openai-response": - return []Format{"openai-response", "codex"} - default: - return []Format{format} - } -} - // TranslateRequest converts a payload between schemas, returning the original payload // if no translator is registered. func (r *Registry) TranslateRequest(from, to Format, model string, rawJSON []byte, stream bool) []byte { r.mu.RLock() defer r.mu.RUnlock() - for _, fromFormat := range formatAliases(from) { - if byTarget, ok := r.requests[fromFormat]; ok { - for _, toFormat := range formatAliases(to) { - if fn, isOk := byTarget[toFormat]; isOk && fn != nil { - return fn(model, rawJSON, stream) - } - } + if byTarget, ok := r.requests[from]; ok { + if fn, isOk := byTarget[to]; isOk && fn != nil { + return fn(model, rawJSON, stream) } } return rawJSON @@ -73,13 +57,9 @@ func (r *Registry) HasResponseTransformer(from, to Format) bool { r.mu.RLock() defer r.mu.RUnlock() - for _, toFormat := range formatAliases(to) { - if byTarget, ok := r.responses[toFormat]; ok { - for _, fromFormat := range formatAliases(from) { - if _, isOk := byTarget[fromFormat]; isOk { - return true - } - } + if byTarget, ok := r.responses[from]; ok { + if _, isOk := byTarget[to]; isOk { + return true } } return false @@ -90,13 +70,9 @@ func (r *Registry) TranslateStream(ctx context.Context, from, to Format, model s r.mu.RLock() defer r.mu.RUnlock() - for _, toFormat := range formatAliases(to) { - if byTarget, ok := r.responses[toFormat]; ok { - for _, fromFormat := range formatAliases(from) { - if fn, isOk := byTarget[fromFormat]; isOk && fn.Stream != nil { - return fn.Stream(ctx, model, originalRequestRawJSON, requestRawJSON, rawJSON, param) - } - } + if byTarget, ok := r.responses[to]; ok { + if fn, isOk := byTarget[from]; isOk && fn.Stream != nil { + return fn.Stream(ctx, model, originalRequestRawJSON, requestRawJSON, rawJSON, param) } } return []string{string(rawJSON)} @@ -107,13 +83,9 @@ func (r *Registry) TranslateNonStream(ctx context.Context, from, to Format, mode r.mu.RLock() defer r.mu.RUnlock() - for _, toFormat := range formatAliases(to) { - if byTarget, ok := r.responses[toFormat]; ok { - for _, fromFormat := range formatAliases(from) { - if fn, isOk := byTarget[fromFormat]; isOk && fn.NonStream != nil { - return fn.NonStream(ctx, model, originalRequestRawJSON, requestRawJSON, rawJSON, param) - } - } + if byTarget, ok := r.responses[to]; ok { + if fn, isOk := byTarget[from]; isOk && fn.NonStream != nil { + return fn.NonStream(ctx, model, originalRequestRawJSON, requestRawJSON, rawJSON, param) } } return string(rawJSON) @@ -124,13 +96,9 @@ func (r *Registry) TranslateTokenCount(ctx context.Context, from, to Format, cou r.mu.RLock() defer r.mu.RUnlock() - for _, toFormat := range formatAliases(to) { - if byTarget, ok := r.responses[toFormat]; ok { - for _, fromFormat := range formatAliases(from) { - if fn, isOk := byTarget[fromFormat]; isOk && fn.TokenCount != nil { - return fn.TokenCount(ctx, count) - } - } + if byTarget, ok := r.responses[to]; ok { + if fn, isOk := byTarget[from]; isOk && fn.TokenCount != nil { + return fn.TokenCount(ctx, count) } } return string(rawJSON) diff --git a/test/thinking_conversion_test.go b/test/thinking_conversion_test.go index 91490fa2..397bbbff 100644 --- a/test/thinking_conversion_test.go +++ b/test/thinking_conversion_test.go @@ -921,10 +921,10 @@ func TestThinkingE2EMatrix_Suffix(t *testing.T) { expectValue: "8192", expectErr: false, }, - // Case 78: Codex to Gemini budget 8192 → passthrough → 8192 + // Case 78: OpenAI-Response to Gemini budget 8192 → passthrough → 8192 { name: "78", - from: "codex", + from: "openai-response", to: "gemini", model: "user-defined-model(8192)", inputJSON: `{"model":"user-defined-model(8192)","input":[{"role":"user","content":"hi"}]}`, @@ -933,10 +933,10 @@ func TestThinkingE2EMatrix_Suffix(t *testing.T) { includeThoughts: "true", expectErr: false, }, - // Case 79: Codex to Claude budget 8192 → passthrough → 8192 + // Case 79: OpenAI-Response to Claude budget 8192 → passthrough → 8192 { name: "79", - from: "codex", + from: "openai-response", to: "claude", model: "user-defined-model(8192)", inputJSON: `{"model":"user-defined-model(8192)","input":[{"role":"user","content":"hi"}]}`, @@ -968,10 +968,10 @@ func TestThinkingE2EMatrix_Suffix(t *testing.T) { expectField: "", expectErr: true, }, - // Case 82: Codex to Codex, level high → passthrough reasoning.effort + // Case 82: OpenAI-Response to Codex, level high → passthrough reasoning.effort { name: "82", - from: "codex", + from: "openai-response", to: "codex", model: "level-model(high)", inputJSON: `{"model":"level-model(high)","input":[{"role":"user","content":"hi"}]}`, @@ -979,10 +979,10 @@ func TestThinkingE2EMatrix_Suffix(t *testing.T) { expectValue: "high", expectErr: false, }, - // Case 83: Codex to Codex, level xhigh → out of range error + // Case 83: OpenAI-Response to Codex, level xhigh → out of range error { name: "83", - from: "codex", + from: "openai-response", to: "codex", model: "level-model(xhigh)", inputJSON: `{"model":"level-model(xhigh)","input":[{"role":"user","content":"hi"}]}`, @@ -1232,6 +1232,74 @@ func TestThinkingE2EMatrix_Suffix(t *testing.T) { expectValue: "false", expectErr: false, }, + + // Gemini Family Cross-Channel Consistency (Cases 106-114) + // Tests that gemini/gemini-cli/antigravity as same API family should have consistent validation behavior + + // Case 106: Gemini to Antigravity, budget 64000 → exceeds Max error (same family strict validation) + { + name: "106", + from: "gemini", + to: "antigravity", + model: "gemini-budget-model(64000)", + inputJSON: `{"model":"gemini-budget-model(64000)","contents":[{"role":"user","parts":[{"text":"hi"}]}]}`, + expectField: "", + expectErr: true, + }, + // Case 107: Gemini to Gemini-CLI, budget 64000 → exceeds Max error (same family strict validation) + { + name: "107", + from: "gemini", + to: "gemini-cli", + model: "gemini-budget-model(64000)", + inputJSON: `{"model":"gemini-budget-model(64000)","contents":[{"role":"user","parts":[{"text":"hi"}]}]}`, + expectField: "", + expectErr: true, + }, + // Case 108: Gemini-CLI to Antigravity, budget 64000 → exceeds Max error (same family strict validation) + { + name: "108", + from: "gemini-cli", + to: "antigravity", + model: "gemini-budget-model(64000)", + inputJSON: `{"model":"gemini-budget-model(64000)","request":{"contents":[{"role":"user","parts":[{"text":"hi"}]}]}}`, + expectField: "", + expectErr: true, + }, + // Case 109: Gemini-CLI to Gemini, budget 64000 → exceeds Max error (same family strict validation) + { + name: "109", + from: "gemini-cli", + to: "gemini", + model: "gemini-budget-model(64000)", + inputJSON: `{"model":"gemini-budget-model(64000)","request":{"contents":[{"role":"user","parts":[{"text":"hi"}]}]}}`, + expectField: "", + expectErr: true, + }, + // Case 110: Gemini to Antigravity, budget 8192 → passthrough (normal value) + { + name: "110", + from: "gemini", + to: "antigravity", + model: "gemini-budget-model(8192)", + inputJSON: `{"model":"gemini-budget-model(8192)","contents":[{"role":"user","parts":[{"text":"hi"}]}]}`, + expectField: "request.generationConfig.thinkingConfig.thinkingBudget", + expectValue: "8192", + includeThoughts: "true", + expectErr: false, + }, + // Case 111: Gemini-CLI to Antigravity, budget 8192 → passthrough (normal value) + { + name: "111", + from: "gemini-cli", + to: "antigravity", + model: "gemini-budget-model(8192)", + inputJSON: `{"model":"gemini-budget-model(8192)","request":{"contents":[{"role":"user","parts":[{"text":"hi"}]}]}}`, + expectField: "request.generationConfig.thinkingConfig.thinkingBudget", + expectValue: "8192", + includeThoughts: "true", + expectErr: false, + }, } runThinkingTests(t, cases) @@ -2122,10 +2190,10 @@ func TestThinkingE2EMatrix_Body(t *testing.T) { expectValue: "8192", expectErr: false, }, - // Case 78: Codex reasoning.effort=medium to Gemini → 8192 + // Case 78: OpenAI-Response reasoning.effort=medium to Gemini → 8192 { name: "78", - from: "codex", + from: "openai-response", to: "gemini", model: "user-defined-model", inputJSON: `{"model":"user-defined-model","input":[{"role":"user","content":"hi"}],"reasoning":{"effort":"medium"}}`, @@ -2134,10 +2202,10 @@ func TestThinkingE2EMatrix_Body(t *testing.T) { includeThoughts: "true", expectErr: false, }, - // Case 79: Codex reasoning.effort=medium to Claude → 8192 + // Case 79: OpenAI-Response reasoning.effort=medium to Claude → 8192 { name: "79", - from: "codex", + from: "openai-response", to: "claude", model: "user-defined-model", inputJSON: `{"model":"user-defined-model","input":[{"role":"user","content":"hi"}],"reasoning":{"effort":"medium"}}`, @@ -2169,10 +2237,10 @@ func TestThinkingE2EMatrix_Body(t *testing.T) { expectField: "", expectErr: true, }, - // Case 82: Codex to Codex, reasoning.effort=high → passthrough + // Case 82: OpenAI-Response to Codex, reasoning.effort=high → passthrough { name: "82", - from: "codex", + from: "openai-response", to: "codex", model: "level-model", inputJSON: `{"model":"level-model","input":[{"role":"user","content":"hi"}],"reasoning":{"effort":"high"}}`, @@ -2180,10 +2248,10 @@ func TestThinkingE2EMatrix_Body(t *testing.T) { expectValue: "high", expectErr: false, }, - // Case 83: Codex to Codex, reasoning.effort=xhigh → out of range error + // Case 83: OpenAI-Response to Codex, reasoning.effort=xhigh → out of range error { name: "83", - from: "codex", + from: "openai-response", to: "codex", model: "level-model", inputJSON: `{"model":"level-model","input":[{"role":"user","content":"hi"}],"reasoning":{"effort":"xhigh"}}`, @@ -2433,6 +2501,74 @@ func TestThinkingE2EMatrix_Body(t *testing.T) { expectValue: "false", expectErr: false, }, + + // Gemini Family Cross-Channel Consistency (Cases 106-114) + // Tests that gemini/gemini-cli/antigravity as same API family should have consistent validation behavior + + // Case 106: Gemini to Antigravity, thinkingBudget=64000 → exceeds Max error (same family strict validation) + { + name: "106", + from: "gemini", + to: "antigravity", + model: "gemini-budget-model", + inputJSON: `{"model":"gemini-budget-model","contents":[{"role":"user","parts":[{"text":"hi"}]}],"generationConfig":{"thinkingConfig":{"thinkingBudget":64000}}}`, + expectField: "", + expectErr: true, + }, + // Case 107: Gemini to Gemini-CLI, thinkingBudget=64000 → exceeds Max error (same family strict validation) + { + name: "107", + from: "gemini", + to: "gemini-cli", + model: "gemini-budget-model", + inputJSON: `{"model":"gemini-budget-model","contents":[{"role":"user","parts":[{"text":"hi"}]}],"generationConfig":{"thinkingConfig":{"thinkingBudget":64000}}}`, + expectField: "", + expectErr: true, + }, + // Case 108: Gemini-CLI to Antigravity, thinkingBudget=64000 → exceeds Max error (same family strict validation) + { + name: "108", + from: "gemini-cli", + to: "antigravity", + model: "gemini-budget-model", + inputJSON: `{"model":"gemini-budget-model","request":{"contents":[{"role":"user","parts":[{"text":"hi"}]}],"generationConfig":{"thinkingConfig":{"thinkingBudget":64000}}}}`, + expectField: "", + expectErr: true, + }, + // Case 109: Gemini-CLI to Gemini, thinkingBudget=64000 → exceeds Max error (same family strict validation) + { + name: "109", + from: "gemini-cli", + to: "gemini", + model: "gemini-budget-model", + inputJSON: `{"model":"gemini-budget-model","request":{"contents":[{"role":"user","parts":[{"text":"hi"}]}],"generationConfig":{"thinkingConfig":{"thinkingBudget":64000}}}}`, + expectField: "", + expectErr: true, + }, + // Case 110: Gemini to Antigravity, thinkingBudget=8192 → passthrough (normal value) + { + name: "110", + from: "gemini", + to: "antigravity", + model: "gemini-budget-model", + inputJSON: `{"model":"gemini-budget-model","contents":[{"role":"user","parts":[{"text":"hi"}]}],"generationConfig":{"thinkingConfig":{"thinkingBudget":8192}}}`, + expectField: "request.generationConfig.thinkingConfig.thinkingBudget", + expectValue: "8192", + includeThoughts: "true", + expectErr: false, + }, + // Case 111: Gemini-CLI to Antigravity, thinkingBudget=8192 → passthrough (normal value) + { + name: "111", + from: "gemini-cli", + to: "antigravity", + model: "gemini-budget-model", + inputJSON: `{"model":"gemini-budget-model","request":{"contents":[{"role":"user","parts":[{"text":"hi"}]}],"generationConfig":{"thinkingConfig":{"thinkingBudget":8192}}}}`, + expectField: "request.generationConfig.thinkingConfig.thinkingBudget", + expectValue: "8192", + includeThoughts: "true", + expectErr: false, + }, } runThinkingTests(t, cases)