diff --git a/CHANGELOG.md b/CHANGELOG.md index 93da2eb..1a0e6f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- Agent adapters now consume one protocol-aware model connection selected from + provider configuration. OpenAI- and Anthropic-compatible endpoints under the + same provider no longer pass through the legacy flattened credential view, + while absent credentials and endpoints continue to delegate to agent-local + login and routing. + +### Fixed +- Agent judges selecting a different provider no longer inherit runner credentials + or endpoints, preventing judge keys from being sent to a runner endpoint. + ## [0.10.0] - 2026-09-01 ### Added diff --git a/docs/design/agent-configuration.md b/docs/design/agent-configuration.md index a54fa21..27938ef 100644 --- a/docs/design/agent-configuration.md +++ b/docs/design/agent-configuration.md @@ -44,20 +44,23 @@ The runner path resolves values in these stages: 3. Build one role-aware `ResolvedAgentConfig`. Legacy slash disambiguation is performed once at this point instead of mutating and later repairing the loaded eval config. -4. Resolve provider-scoped `MODEL`, `API_KEY`, and `BASE_URL` values. A - provider-scoped model environment variable currently overrides the YAML - model; provider environment credentials override the credential file. +4. Resolve the provider-scoped `MODEL` value. A provider-scoped model + environment variable currently overrides the YAML model. 5. Preserve explicit CLI `--model` and `--api-key` precedence. -6. Apply the selected adapter's declared protocol and capability contract. +6. Apply the selected adapter's declared protocol and capability contract, + then resolve credentials and endpoints for that `(provider, protocol)`. Keep the requested model intact, compute the applied model once, remove unsupported kwargs, and emit warnings for ignored or invalid explicit settings before case execution. -7. Pass the applied value to the selected adapter, which constructs its - command and environment without repeating model normalization. +7. Pass the applied connection to the selected adapter, which constructs its + command and environment without repeating credential or model resolution. Runner and judge roles use the same resolution flow. Until an explicit judge engine schema is introduced, the judge inherits the runner engine lifecycle and kwargs, while resolving its provider/model and credentials as a separate role. +Connection inheritance is limited to the same provider namespace. A judge that +selects a different provider resolves its own key and endpoint by protocol; +missing values delegate to agent-local state, never to the runner connection. Reports use the resolved runner identity rather than reconstructing it from a CLI-mutated eval config. `result.json` retains the legacy `engine_name` and `model_name` fields while also recording credential-free @@ -86,12 +89,11 @@ configuration are related but distinct layers: local login databases remain owned by the agent and may be opaque to skill-up. -`ResolvedAgentConfig` currently spans the first two steps and the capability -pass computes what skill-up can apply. A later protocol-aware resolver may -introduce an internal `ResolvedModelConnection`, but it should not be confused -with observed runtime state: it describes the connection selected for the -adapter, not proof of the provider, model, or credential ultimately used by the -agent. +`ResolvedAgentConfig` retains requested values and its `AppliedConnection` +records the protocol-aware `ResolvedModelConnection` selected by the capability +pass. The connection must not be confused with observed runtime state: it +describes what skill-up materializes for the adapter, not proof of the provider, +model, credential, or local login ultimately used by the agent. This layering is informed by Harbor's [`ProviderAccess`, `ModelConnectionSpec`, and `ResolvedModelConnection`](https://github.com/harbor-framework/harbor/blob/71180a2e6fb40626b661c13f261b1d44517ad91a/src/harbor/agents/model_connection.py), @@ -145,7 +147,8 @@ Tagged Actions and historical commands therefore remain valid. ## Known gaps for later phases -- Nested provider endpoints are flattened before the adapter protocol is known. +- The legacy flattened credential lookup remains available to older internal + callers, but adapter construction uses the protocol-aware connection. - Provider-scoped `MODEL` currently overrides an explicit YAML model. - `engine.version`, `engine.entry`, and `engine.model.params` now produce warnings when ineffective, but their final implementation/removal is deferred diff --git a/internal/agent/README.md b/internal/agent/README.md index e949282..953608b 100644 --- a/internal/agent/README.md +++ b/internal/agent/README.md @@ -124,9 +124,11 @@ type SessionResult struct { ## Adapter capability resolution `ResolveAdapterConfig` runs after YAML/CLI/credential resolution and before -agent construction. It keeps the requested model intact, records the model -that skill-up will forward as `AppliedModel`, and emits warnings for -explicit settings that the selected adapter does not consume. +agent construction. It selects credentials and endpoints for the adapter's +protocol, keeps the requested model intact, records the model that skill-up will +forward as `AppliedModel`, and emits warnings for explicit settings that the +selected adapter does not consume. Factory construction consumes the resulting +`AppliedConnection` rather than resolving provider configuration again. Applied values describe the invocation, not the CLI's final runtime choice. Local configuration may override them. `SessionResult.Model` remains empty diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index a20b1c0..eeb6643 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -546,10 +546,48 @@ func TestProbeAndMergePATH_SkipsMergeOnEmptyStdout(t *testing.T) { } } +func detectMaterializedAgentForTest(params credential.ResolvedAgentConfig) (Agent, error) { + return DetectAgentWithResolvedConfig(ResolveAdapterConfig(params, nil)) +} + +func TestDetectAgentWithResolvedConfig_RequiresMaterialization(t *testing.T) { + t.Parallel() + + _, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{Engine: "codex"}) + if err == nil || !strings.Contains(err.Error(), "was not materialized") { + t.Fatalf("error = %v, want materialization error", err) + } +} + +func TestDetectAgentWithResolvedConfig_ConsumesAppliedConnection(t *testing.T) { + t.Parallel() + + params := ResolveAdapterConfig(credential.ResolvedAgentConfig{ + Engine: "claude_code", + Provider: "anthropic", + APIKey: "connection-key", + BaseURL: "https://connection.example.test", + }, nil) + params.AppliedAPIKey = "stale-key" + params.AppliedBaseURL = "https://stale.example.test" + + ag, err := DetectAgentWithResolvedConfig(params) + if err != nil { + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) + } + claudeAgent, ok := ag.(*ClaudeCodeAgent) + if !ok { + t.Fatalf("agent = %T, want *ClaudeCodeAgent", ag) + } + if claudeAgent.Cfg.APIKey != "connection-key" || claudeAgent.Cfg.BaseURL != "https://connection.example.test" { + t.Fatalf("adapter config = key %q base URL %q, want applied connection values", claudeAgent.Cfg.APIKey, claudeAgent.Cfg.BaseURL) + } +} + func TestDetectAgentWithResolvedConfig_SetsTypedCredentialFields(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "codex", Provider: "openai", Model: "gpt-5.4", @@ -590,7 +628,7 @@ func TestDetectAgentWithResolvedConfig_SetsTypedCredentialFields(t *testing.T) { func TestDetectAgentWithResolvedConfig_CodexFallbackOmitsRejectedProviderCredential(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ //nolint:gosec // dummy test credential + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ //nolint:gosec // dummy test credential Engine: "codex", Provider: testDashscopeProvider, Model: "qwen3.6-plus", @@ -618,7 +656,7 @@ func TestDetectAgentWithResolvedConfig_QoderMapsAPIKeyToRuntimeEnv(t *testing.T) token := "qoder-runtime-token" //nolint:gosec // test credential, not real t.Setenv(credential.EnvQoderPersonalAccessToken, token) - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "qoder-cli", Provider: "qoder", Model: "auto", @@ -641,7 +679,7 @@ func TestDetectAgentWithResolvedConfig_QoderCNMapsKeychainAliasToOfficialEnv(t * t.Setenv(credential.EnvQoderCNAccessToken, token) t.Setenv(credential.EnvQoderCNPersonalAccessToken, "") - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "qoder-cli", Provider: "qoder", Model: "auto", @@ -667,7 +705,7 @@ func TestDetectAgentWithResolvedConfig_QoderCNPrefersOfficialEnv(t *testing.T) { t.Setenv(credential.EnvQoderCNPersonalAccessToken, "official-token") t.Setenv(credential.EnvQoderCNAccessToken, "alias-token") - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "qoder-cli", Kwargs: map[string]string{KwargEdition: qoderEditionCN}, }) @@ -686,7 +724,7 @@ func TestDetectAgentWithResolvedConfig_QoderCNPrefersOfficialEnv(t *testing.T) { func TestDetectAgentWithResolvedConfig_QoderIgnoresParamsAPIKey(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ //nolint:gosec // test dummy key + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ //nolint:gosec // test dummy key Engine: "qoder-cli", Provider: "anthropic", Model: "auto", @@ -731,7 +769,7 @@ func TestUnsupportedAgentError(t *testing.T) { func TestDetectAgentWithResolvedConfig_UsesResolvedModelWithoutNormalization(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "claude-code", Model: "", }) @@ -751,7 +789,7 @@ func TestDetectAgentWithResolvedConfig_UsesResolvedModelWithoutNormalization(t * func TestDetectAgentWithResolvedConfig_PreservesAutoForQoderCLI(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "qoder-cli", Model: "auto", }) @@ -772,7 +810,7 @@ func TestDetectAgentWithResolvedConfig_ForwardsKwargs(t *testing.T) { t.Parallel() kwargs := map[string]string{KwargBypassSandbox: "true", "future_key": "x"} - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "codex", Provider: "openai", Model: "gpt-5.4", diff --git a/internal/agent/capabilities.go b/internal/agent/capabilities.go index 39f0ff8..b75fd8a 100644 --- a/internal/agent/capabilities.go +++ b/internal/agent/capabilities.go @@ -95,26 +95,113 @@ func CapabilitiesForEngine(engineName string) Capabilities { // records the model that skill-up will forward to the CLI. It does not claim // that the CLI ultimately selected that model: local CLI configuration may // override it, and most adapters do not report their final runtime choice. -func ResolveAdapterConfig(params credential.ResolvedAgentConfig) credential.ResolvedAgentConfig { +func ResolveAdapterConfig(params credential.ResolvedAgentConfig, resolver *credential.Resolver) credential.ResolvedAgentConfig { if params.Role == "" { params.Role = credential.AgentRoleRunner } params.Kwargs = maps.Clone(params.Kwargs) params.ModelParams = maps.Clone(params.ModelParams) params.Warnings = slices.Clone(params.Warnings) - params.AppliedAPIKey = params.APIKey - params.AppliedBaseURL = params.BaseURL capabilities := CapabilitiesForEngine(params.Engine) params.Protocol = string(capabilities.Protocol) + connection := resolveModelConnection(params, resolver, capabilities.Protocol) + params.AppliedAPIKey = connection.APIKey + params.AppliedBaseURL = connection.BaseURL params.AppliedProvider = resolveAppliedProvider(¶ms, capabilities) params.AppliedModel = resolveAppliedModel(¶ms, capabilities) validateBaseURL(¶ms, capabilities) validateDeferredFields(¶ms, capabilities) validateKwargs(¶ms, capabilities) + params.AppliedConnection = appliedModelConnection(params, connection) return params } +func resolveModelConnection( + params credential.ResolvedAgentConfig, + resolver *credential.Resolver, + protocol credential.Protocol, +) credential.ResolvedModelConnection { + if params.AppliedConnection.Protocol == protocol { + return params.AppliedConnection + } + if resolver == nil { + resolved := credential.ResolvedModelConnection{ + Provider: params.Provider, + Protocol: protocol, + APIKey: params.APIKey, + BaseURL: params.BaseURL, + APIKeySet: params.APIKeySource != "" || params.APIKey != "", + BaseURLSet: params.BaseURLSource != "" || params.BaseURL != "", + APIKeySource: params.APIKeySource, + BaseURLSource: params.BaseURLSource, + AuthMode: credential.AuthModeAgentLocal, + RoutingMode: credential.RoutingModeAgentLocal, + } + if resolved.APIKey != "" { + resolved.AuthMode = credential.AuthModeInjected + } + if resolved.BaseURL != "" { + resolved.RoutingMode = credential.RoutingModeExplicit + } + return resolved + } + + spec := credential.ModelConnectionSpec{ + Provider: params.Provider, + Protocol: protocol, + } + if value, ok := explicitConnectionValue(params.APIKey, params.APIKeySource); ok { + spec.APIKey = value + } + if value, ok := explicitConnectionValue(params.BaseURL, params.BaseURLSource); ok { + spec.BaseURL = value + } + return resolver.ResolveModelConnection(spec) +} + +func explicitConnectionValue(value string, source credential.ValueSource) (credential.ConnectionValue, bool) { + if source == "" && value == "" { + return credential.ConnectionValue{}, false + } + if source == credential.ValueSourceEnv || source == credential.ValueSourceResolver { + return credential.ConnectionValue{}, false + } + return credential.ExplicitConnectionValue(value, source), true +} + +func appliedModelConnection( + params credential.ResolvedAgentConfig, + resolved credential.ResolvedModelConnection, +) credential.ResolvedModelConnection { + apiKeyChanged := resolved.APIKey != params.AppliedAPIKey + baseURLChanged := resolved.BaseURL != params.AppliedBaseURL + resolved.Provider = params.AppliedProvider + resolved.APIKey = params.AppliedAPIKey + resolved.BaseURL = params.AppliedBaseURL + if apiKeyChanged { + resolved.APIKeySet = params.AppliedAPIKey != "" + if !resolved.APIKeySet { + resolved.APIKeySource = "" + } + } + if baseURLChanged { + resolved.BaseURLSet = params.AppliedBaseURL != "" + if !resolved.BaseURLSet { + resolved.BaseURLSource = "" + } + } + resolved.AuthMode = credential.AuthModeAgentLocal + if resolved.APIKey != "" { + resolved.AuthMode = credential.AuthModeInjected + } + resolved.RoutingMode = credential.RoutingModeAgentLocal + if resolved.BaseURL != "" { + resolved.RoutingMode = credential.RoutingModeExplicit + } + return resolved +} + func resolveAppliedProvider(params *credential.ResolvedAgentConfig, capabilities Capabilities) string { switch capabilities.ModelPolicy { case ModelPolicyQoderTier: @@ -125,7 +212,7 @@ func resolveAppliedProvider(params *credential.ResolvedAgentConfig, capabilities params.AppliedBaseURL = "" return "" case ModelPolicyCodexProvider: - if reason := codexCustomProviderUnavailableReason(params.Provider, params.BaseURL); reason != "" { + if reason := codexCustomProviderUnavailableReason(params.Provider, params.AppliedBaseURL); reason != "" { params.Warnings = appendUniqueWarning(params.Warnings, fmt.Sprintf( "engine %q cannot apply provider %q: provider %s; the provider override, endpoint, and provider-scoped credential are omitted and local Codex settings will be used", params.Engine, params.Provider, reason, @@ -134,7 +221,7 @@ func resolveAppliedProvider(params *credential.ResolvedAgentConfig, capabilities params.AppliedBaseURL = "" return "" } - if params.BaseURL == "" { + if params.AppliedBaseURL == "" { // Codex receives no model_provider override in this case. Even an // explicit provider namespace cannot prove which local provider Codex // ultimately selects. @@ -164,7 +251,7 @@ func resolveAppliedModel(params *credential.ResolvedAgentConfig, capabilities Ca )) return "" case ModelPolicyCodexProvider: - if reason := codexCustomProviderUnavailableReason(params.Provider, params.BaseURL); reason != "" { + if reason := codexCustomProviderUnavailableReason(params.Provider, params.AppliedBaseURL); reason != "" { return "" } } @@ -173,7 +260,7 @@ func resolveAppliedModel(params *credential.ResolvedAgentConfig, capabilities Ca } func validateBaseURL(params *credential.ResolvedAgentConfig, capabilities Capabilities) { - if params.BaseURL == "" || capabilities.SupportsBaseURL { + if params.AppliedBaseURL == "" || capabilities.SupportsBaseURL { return } params.Warnings = appendUniqueWarning(params.Warnings, fmt.Sprintf( diff --git a/internal/agent/capabilities_test.go b/internal/agent/capabilities_test.go index ec4ec92..bea5259 100644 --- a/internal/agent/capabilities_test.go +++ b/internal/agent/capabilities_test.go @@ -2,10 +2,13 @@ package agent import ( "context" + "os" + "path/filepath" "slices" "strings" "testing" + "github.com/alibaba/skill-up/internal/config" "github.com/alibaba/skill-up/internal/credential" "github.com/alibaba/skill-up/internal/logging" ) @@ -102,7 +105,7 @@ func TestResolveAdapterConfig_ModelPolicies(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := ResolveAdapterConfig(tt.params) + got := ResolveAdapterConfig(tt.params, nil) if got.Protocol != string(tt.wantProtocol) || got.AppliedProvider != tt.wantProvider || got.AppliedModel != tt.wantApplied { t.Fatalf("ResolveAdapterConfig() protocol/provider/model = %q/%q/%q, want %q/%q/%q", got.Protocol, got.AppliedProvider, got.AppliedModel, tt.wantProtocol, tt.wantProvider, tt.wantApplied) } @@ -116,18 +119,222 @@ func TestResolveAdapterConfig_ModelPolicies(t *testing.T) { } } +func TestResolveAdapterConfig_SelectsConnectionForAdapterProtocol(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "credentials.yaml") + content := []byte(` +providers: + adapter_gateway: + api_key: flat-key + base_url: https://flat.example.test + openai: + api_key: openai-key + base_url: https://openai.example.test/v1 + anthropic: + api_key: anthropic-key + base_url: https://anthropic.example.test +`) + if err := os.WriteFile(path, content, 0o600); err != nil { + t.Fatalf("write credentials: %v", err) + } + resolver := credential.NewResolver(path) + if err := resolver.Load(); err != nil { + t.Fatalf("load credentials: %v", err) + } + + tests := []struct { + engine string + protocol credential.Protocol + apiKey string + baseURL string + provider string + }{ + {engine: "codex", protocol: credential.ProtocolOpenAI, apiKey: "openai-key", baseURL: "https://openai.example.test/v1", provider: "adapter_gateway"}, + {engine: "claude_code", protocol: credential.ProtocolAnthropic, apiKey: "anthropic-key", baseURL: "https://anthropic.example.test", provider: "adapter_gateway"}, //nolint:gosec // test credential + } + for _, tt := range tests { + t.Run(tt.engine, func(t *testing.T) { + t.Parallel() + got := ResolveAdapterConfig(credential.ResolvedAgentConfig{ + Engine: tt.engine, + Provider: tt.provider, + APIKey: "flat-key", + BaseURL: "https://flat.example.test", + APIKeySource: credential.ValueSourceResolver, + BaseURLSource: credential.ValueSourceResolver, + }, resolver) + connection := got.AppliedConnection + if connection.Protocol != tt.protocol || connection.APIKey != tt.apiKey || connection.BaseURL != tt.baseURL { + t.Fatalf("applied connection = %#v, want %s endpoint", connection, tt.protocol) + } + if connection.Provider != tt.provider || connection.AuthMode != credential.AuthModeInjected || connection.RoutingMode != credential.RoutingModeExplicit { + t.Fatalf("applied connection metadata = %#v", connection) + } + }) + } +} + +func TestResolveAdapterConfig_PreservesExplicitEmptyConnectionDelegation(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "credentials.yaml") + if err := os.WriteFile(path, []byte(` +providers: + local_gateway: + api_key: flat-key + base_url: https://flat.example.test + anthropic: + api_key: "" + base_url: "" +`), 0o600); err != nil { + t.Fatalf("write credentials: %v", err) + } + resolver := credential.NewResolver(path) + if err := resolver.Load(); err != nil { + t.Fatalf("load credentials: %v", err) + } + + got := ResolveAdapterConfig(credential.ResolvedAgentConfig{ + Engine: "claude_code", + Provider: "local_gateway", + }, resolver) + connection := got.AppliedConnection + if connection.APIKey != "" || connection.BaseURL != "" || !connection.APIKeySet || !connection.BaseURLSet { + t.Fatalf("explicit empty connection was not preserved: %#v", connection) + } + if connection.AuthMode != credential.AuthModeAgentLocal || connection.RoutingMode != credential.RoutingModeAgentLocal { + t.Fatalf("delegated modes = %q/%q", connection.AuthMode, connection.RoutingMode) + } +} + +func TestResolveAdapterConfig_InheritedJudgeKeepsRunnerProtocolConnection(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "credentials.yaml") + if err := os.WriteFile(path, []byte(` +providers: + judge_gateway: + openai: + api_key: openai-key + base_url: https://openai.example.test/v1 + anthropic: + api_key: anthropic-key + base_url: https://anthropic.example.test +`), 0o600); err != nil { + t.Fatalf("write credentials: %v", err) + } + resolver := credential.NewResolver(path) + if err := resolver.Load(); err != nil { + t.Fatalf("load credentials: %v", err) + } + + runner := credential.ResolveRunnerConfig(config.EngineConfig{ + Name: "claude_code", + Model: config.ModelConfig{ + Provider: "judge_gateway", + Name: "claude-sonnet-4-6", + }, + }, resolver, credential.CLIOverrides{}) + runner = ResolveAdapterConfig(runner, resolver) + judge := credential.ResolveJudgeConfig(config.JudgeConfig{Type: "agent_judge"}, runner, resolver) + judge = ResolveAdapterConfig(judge, resolver) + + if judge.AppliedConnection.Protocol != credential.ProtocolAnthropic || judge.AppliedConnection.APIKey != "anthropic-key" || judge.AppliedConnection.BaseURL != "https://anthropic.example.test" { + t.Fatalf("judge connection = %#v, want inherited Anthropic connection", judge.AppliedConnection) + } +} + +func TestResolveAdapterConfig_JudgeProviderConnectionIsolation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + fields string + apiKey string + baseURL string + keySet bool + urlSet bool + }{ + {"flat", " api_key: judge-key\n base_url: https://judge.example.test\n", "judge-key", "https://judge.example.test", true, true}, + {"protocol", " api_key: flat-key\n base_url: https://flat.example.test\n PROTOCOL:\n api_key: judge-key\n base_url: https://judge.example.test\n", "judge-key", "https://judge.example.test", true, true}, + {"missing_key", " base_url: https://judge.example.test\n", "", "https://judge.example.test", false, true}, + {"missing_endpoint", " api_key: judge-key\n", "judge-key", "", true, false}, + {"missing_both", " {}\n", "", "", false, false}, + {"explicit_empty", " api_key: flat-key\n base_url: https://flat.example.test\n PROTOCOL:\n api_key: \"\"\n base_url: \"\"\n", "", "", true, true}, + } + for _, engine := range []string{"claude_code", "qwen_code"} { + for _, tt := range tests { + t.Run(engine+"/"+tt.name, func(t *testing.T) { + t.Parallel() + protocol := CapabilitiesForEngine(engine).Protocol + path := filepath.Join(t.TempDir(), "credentials.yaml") + data := "providers:\n isolated_judge:\n" + strings.ReplaceAll(tt.fields, "PROTOCOL", string(protocol)) + if err := os.WriteFile(path, []byte(data), 0o600); err != nil { + t.Fatal(err) + } + resolver := credential.NewResolver(path) + if err := resolver.Load(); err != nil { + t.Fatal(err) + } + runner := credential.ResolveRunnerConfig(config.EngineConfig{ + Name: engine, + Model: config.ModelConfig{Provider: "isolated_runner", Name: "runner-model", BaseURL: "https://runner.example.test"}, + }, resolver, credential.CLIOverrides{APIKey: "runner-key"}) + runner = ResolveAdapterConfig(runner, resolver) + judge := credential.ResolveJudgeConfig(config.JudgeConfig{Type: "agent_judge", Model: "isolated_judge/judge-model"}, runner, resolver) + judge = ResolveAdapterConfig(judge, resolver) + connection := judge.AppliedConnection + if connection.Provider != "isolated_judge" || connection.Protocol != protocol || connection.APIKey != tt.apiKey || connection.BaseURL != tt.baseURL || connection.APIKeySet != tt.keySet || connection.BaseURLSet != tt.urlSet || judge.APIKeySource == credential.ValueSourceRunner || judge.BaseURLSource == credential.ValueSourceRunner { + t.Fatalf("unexpected judge connection: %#v", connection) + } + if (connection.AuthMode == credential.AuthModeInjected) != (tt.apiKey != "") || (connection.RoutingMode == credential.RoutingModeExplicit) != (tt.baseURL != "") { + t.Fatalf("unexpected auth/routing modes: %s/%s", connection.AuthMode, connection.RoutingMode) + } + assertJudgeAdapterEnvironment(t, judge, tt.apiKey, tt.baseURL) + }) + } + } +} + +func assertJudgeAdapterEnvironment(t *testing.T, judge credential.ResolvedAgentConfig, apiKey, baseURL string) { + t.Helper() + ag, err := DetectAgentWithResolvedConfig(judge) + if err != nil { + t.Fatal(err) + } + var env map[string]string + var keyEnv, urlEnv string + switch a := ag.(type) { + case *ClaudeCodeAgent: + keyEnv, urlEnv = credential.EnvAnthropicAPIKey, credential.EnvAnthropicBaseURL + env = a.credentialEnvVars(keyEnv, urlEnv) + case *QwenCodeAgent: + keyEnv, urlEnv = credential.EnvOpenAIAPIKey, credential.EnvOpenAIBaseURL + env = a.credentialEnvVars(keyEnv, urlEnv) + default: + t.Fatalf("unexpected agent type %T", ag) + } + if env[keyEnv] != apiKey || env[urlEnv] != baseURL { + t.Fatal("adapter environment does not match the independent judge connection") + } +} + func TestResolveAdapterConfig_CodexFallbackDoesNotForwardProviderCredential(t *testing.T) { t.Parallel() got := ResolveAdapterConfig(credential.ResolvedAgentConfig{ //nolint:gosec // dummy test credential Engine: "codex", Provider: testDashscopeProvider, APIKey: "dashscope-test-key", - }) + }, nil) if got.Provider != testDashscopeProvider || got.APIKey != "dashscope-test-key" { t.Fatalf("requested provider credential was mutated: %+v", got) } if got.AppliedProvider != "" || got.AppliedModel != "" || got.AppliedAPIKey != "" || got.AppliedBaseURL != "" { t.Fatalf("fallback applied config = provider %q model %q key %q baseURL %q, want all omitted", got.AppliedProvider, got.AppliedModel, got.AppliedAPIKey, got.AppliedBaseURL) } + if got.AppliedConnection.Provider != "" || got.AppliedConnection.APIKeySet || got.AppliedConnection.BaseURLSet || got.AppliedConnection.APIKeySource != "" || got.AppliedConnection.BaseURLSource != "" { + t.Fatalf("fallback applied connection retained rejected provider configuration: %#v", got.AppliedConnection) + } if !containsWarning(got.Warnings, "provider-scoped credential are omitted") { t.Fatalf("Warnings = %v, want provider fallback warning even without a model", got.Warnings) } @@ -150,7 +357,7 @@ func TestResolveAdapterConfig_ValidatesExplicitSettingsWithoutAliasing(t *testin ModelParams: map[string]string{"reasoning": "high"}, } - got := ResolveAdapterConfig(params) + got := ResolveAdapterConfig(params, nil) for _, key := range []string{KwargBypassSandbox, KwargMaxJSONLRecordBytes, "typo"} { if _, ok := got.Kwargs[key]; ok { t.Fatalf("invalid or unsupported kwarg %q was not removed: %v", key, got.Kwargs) @@ -181,7 +388,7 @@ func TestResolveAdapterConfig_QoderNormalizesUnsupportedEdition(t *testing.T) { got := ResolveAdapterConfig(credential.ResolvedAgentConfig{ Engine: "qodercli", Kwargs: map[string]string{KwargEdition: "enterprise"}, - }) + }, nil) if got.Kwargs[KwargEdition] != qoderEditionGlobal { t.Fatalf("edition = %q, want %q", got.Kwargs[KwargEdition], qoderEditionGlobal) } @@ -197,7 +404,7 @@ func TestResolveAdapterConfig_CustomRejectsUnusedTopLevelSettings(t *testing.T) Engine: "custom-agent", BaseURL: "https://unused.example.test", Kwargs: map[string]string{"profile": "unused"}, - }) + }, nil) if got.AppliedBaseURL != "" || len(got.Kwargs) != 0 { t.Fatalf("custom applied unused top-level settings: baseURL=%q kwargs=%v", got.AppliedBaseURL, got.Kwargs) } @@ -241,7 +448,7 @@ func TestLogAdapterConfig_DoesNotExposeCredentialsOrEndpoint(t *testing.T) { Model: "gpt-5.4", APIKey: "secret-api-key", BaseURL: "https://user:secret@example.test/v1", - }) + }, nil) output := captureStdout(t, func() { LogAdapterConfig(context.Background(), params) }) for _, forbidden := range []string{"secret-api-key", "user:secret", "example.test"} { if strings.Contains(output, forbidden) { diff --git a/internal/agent/custom_test.go b/internal/agent/custom_test.go index 66f911d..7ea31f3 100644 --- a/internal/agent/custom_test.go +++ b/internal/agent/custom_test.go @@ -911,7 +911,7 @@ func TestDetectAgentWithResolvedConfig_KeepsAutoModelForCustom(t *testing.T) { Transport: "local", Local: &config.CustomLocalConfig{Command: "/opt/agent"}, } - ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + ag, err := detectMaterializedAgentForTest(credential.ResolvedAgentConfig{ Engine: "my-agent", Model: modelAuto, Custom: custom, diff --git a/internal/agent/factory.go b/internal/agent/factory.go index 18a2c9a..0ee4bf3 100644 --- a/internal/agent/factory.go +++ b/internal/agent/factory.go @@ -1,6 +1,7 @@ package agent import ( + "fmt" "os" "github.com/alibaba/skill-up/internal/agentkind" @@ -36,7 +37,10 @@ func DetectAgent(engineName string, cfg Config) (Agent, error) { // DetectAgentWithResolvedConfig maps a resolved role configuration into the // selected adapter without reinterpreting raw YAML or CLI values. func DetectAgentWithResolvedConfig(params credential.ResolvedAgentConfig) (Agent, error) { - params = ResolveAdapterConfig(params) + connection := params.AppliedConnection + if connection.Protocol == "" { + return nil, fmt.Errorf("agent config for engine %q was not materialized", params.Engine) + } engineName := params.Engine cfg := Config{ Name: engineName, @@ -45,11 +49,11 @@ func DetectAgentWithResolvedConfig(params credential.ResolvedAgentConfig) (Agent ModelName: params.AppliedModel, RequestedModelName: params.Model, RequestedProvider: params.Provider, - ModelProvider: params.AppliedProvider, - Protocol: params.Protocol, + ModelProvider: connection.Provider, + Protocol: string(connection.Protocol), Warnings: params.Warnings, - APIKey: params.AppliedAPIKey, - BaseURL: params.AppliedBaseURL, + APIKey: connection.APIKey, + BaseURL: connection.BaseURL, EnvVars: make(map[string]string), Kwargs: params.Kwargs, Custom: params.Custom, diff --git a/internal/cli/run.go b/internal/cli/run.go index d3612e5..1328b4f 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -260,7 +260,7 @@ func loadCredentialsAndAgent(cmd *cobra.Command, evalCfg *config.EvalConfig) (ag resolver, credential.CLIOverrides{Provider: cliProvider, Model: cliModel, APIKey: cliAPIKey}, ) - runnerConfig = agent.ResolveAdapterConfig(runnerConfig) + runnerConfig = agent.ResolveAdapterConfig(runnerConfig, resolver) agent.LogAdapterConfig(cmd.Context(), runnerConfig) ag, err := agent.DetectAgentWithResolvedConfig(runnerConfig) diff --git a/internal/credential/README.md b/internal/credential/README.md index dec9edc..c9b7efe 100644 --- a/internal/credential/README.md +++ b/internal/credential/README.md @@ -35,6 +35,7 @@ type ResolvedAgentConfig struct { AppliedModel string AppliedAPIKey string AppliedBaseURL string + AppliedConnection ResolvedModelConnection APIKey string BaseURL string Kwargs map[string]string @@ -46,8 +47,8 @@ type ResolvedAgentConfig struct { This value is the boundary between raw YAML/CLI/credential inputs and adapter construction. Map fields are cloned while resolving, so later mutations of the loaded eval config do not alter a resolved runner or judge configuration. -Protocol, applied model, and warnings are filled by the subsequent adapter -capability pass. +Protocol, applied model, applied connection, and warnings are filled by the +subsequent adapter capability pass. ## Two Pipelines @@ -70,6 +71,9 @@ Its configuration decisions should be handled separately from the runner agent: - If the judge has independent configuration, prefer the judge's own provider/model/api-key/base-url - If no independent configuration is provided, reuse the runner agent's final result +- Inherit the runner key and endpoint only when the resolved providers match. + A different judge provider resolves its own connection; missing values do not + fall back to the runner key or endpoint. It is recommended to treat the judge agent as a separate parameter resolution pipeline rather than "borrowing some fields" from the runner agent's initialization. @@ -194,9 +198,10 @@ Rules: The unified credential layer produces the requested `ResolvedAgentConfig`. `agent.ResolveAdapterConfig` then applies the selected adapter's declared -protocol and capability contract once, before construction. It records -`AppliedModel` separately and reports unsupported explicit settings instead -of leaving each adapter to reinterpret the same raw values during execution. +protocol and capability contract once, resolves a connection for that protocol, +and passes the resulting `AppliedConnection` to construction. It records +`AppliedModel` separately and reports unsupported explicit settings instead of +leaving each adapter to reinterpret the same raw values during execution. The historical non-Qoder `auto` normalization remains in credential resolution for compatibility. diff --git a/internal/credential/agent_init.go b/internal/credential/agent_init.go index 3c71130..9ee289c 100644 --- a/internal/credential/agent_init.go +++ b/internal/credential/agent_init.go @@ -50,17 +50,18 @@ type ResolvedAgentConfig struct { Entry string Protocol string - Provider string - Model string - AppliedProvider string - AppliedModel string - AppliedAPIKey string - AppliedBaseURL string - APIKey string - BaseURL string - Kwargs map[string]string - ModelParams map[string]string - Warnings []string + Provider string + Model string + AppliedProvider string + AppliedModel string + AppliedAPIKey string + AppliedBaseURL string + AppliedConnection ResolvedModelConnection `json:"-" yaml:"-"` + APIKey string + BaseURL string + Kwargs map[string]string + ModelParams map[string]string + Warnings []string // Custom carries the custom engine config when the engine name does not // match a built-in agent. It is nil for built-in agents. @@ -116,7 +117,7 @@ func ResolveJudgeConfig(judgeCfg config.JudgeConfig, runner ResolvedAgentConfig, fallback = &runner } - return resolveResolvedAgentConfig(agentResolveInput{ + resolved := resolveResolvedAgentConfig(agentResolveInput{ role: AgentRoleJudge, engine: config.EngineConfig{ Name: runner.Engine, @@ -134,6 +135,10 @@ func ResolveJudgeConfig(judgeCfg config.JudgeConfig, runner ResolvedAgentConfig, fallback: fallback, resolver: resolver, }) + if resolved.Provider == runner.Provider && runner.AppliedConnection.Protocol != "" { + resolved.AppliedConnection = runner.AppliedConnection + } + return resolved } func parseJudgeModel(value string) (provider, model string) { @@ -200,14 +205,15 @@ func applyFallback(params *ResolvedAgentConfig, fallback *ResolvedAgentConfig) { params.Model = fallback.Model params.ModelSource = ValueSourceRunner } - if params.BaseURL == "" && fallback.BaseURL != "" { + // Connection values belong to a provider namespace, unlike engine lifecycle. + if params.Provider == fallback.Provider && params.BaseURL == "" && fallback.BaseURL != "" { params.BaseURL = fallback.BaseURL params.BaseURLSource = ValueSourceRunner } } func applyFallbackCredentials(params *ResolvedAgentConfig, fallback *ResolvedAgentConfig) { - if fallback == nil { + if fallback == nil || params.Provider != fallback.Provider { return } if params.APIKey == "" && fallback.APIKey != "" { diff --git a/internal/credential/credential_test.go b/internal/credential/credential_test.go index b7ea54d..9fa3834 100644 --- a/internal/credential/credential_test.go +++ b/internal/credential/credential_test.go @@ -695,6 +695,32 @@ func TestResolveJudgeConfig_ParsesIndependentJudgeModel(t *testing.T) { } } +func TestResolveJudgeConfig_ConnectionFallbackRequiresSameProvider(t *testing.T) { + t.Parallel() + for _, runnerProvider := range []string{"fallback_runner", ""} { + for _, judgeModel := range []string{"", "judge-model", "fallback_runner/judge-model", "fallback_judge/judge-model"} { + t.Run(runnerProvider+"/"+judgeModel, func(t *testing.T) { + t.Parallel() + runner := ResolvedAgentConfig{ + Engine: "claude_code", Provider: runnerProvider, Model: "runner-model", + APIKey: "runner-key", BaseURL: "https://runner.example.test", + } + got := ResolveJudgeConfig(config.JudgeConfig{Model: judgeModel}, runner, nil) + if got.Engine != runner.Engine { + t.Fatal("judge did not inherit runner engine") + } + if got.Provider == runner.Provider { + if got.APIKey != runner.APIKey || got.BaseURL != runner.BaseURL || got.APIKeySource != ValueSourceRunner || got.BaseURLSource != ValueSourceRunner { + t.Fatal("same-provider judge did not inherit runner connection") + } + } else if got.APIKey != "" || got.BaseURL != "" || got.APIKeySource != "" || got.BaseURLSource != "" { + t.Fatal("different-provider judge inherited runner connection") + } + }) + } + } +} + func TestResolveJudgeConfig_PrefersProviderScopedModelEnv(t *testing.T) { t.Setenv("JUDGEPROVIDER_MODEL", "gpt-5.5-judge-env") diff --git a/internal/evaluator/evaluator.go b/internal/evaluator/evaluator.go index f905bd0..069bf37 100644 --- a/internal/evaluator/evaluator.go +++ b/internal/evaluator/evaluator.go @@ -826,7 +826,7 @@ func (e *defaultEvaluator) resolveJudgeAgent(ctx context.Context, judgeCfg confi judgeAgent := runAgent if e.evalCfg.Engine.Name != "" { resolvedJudge := credential.ResolveJudgeConfig(judgeCfg, e.runnerConfig, e.resolver) - resolvedJudge = agent.ResolveAdapterConfig(resolvedJudge) + resolvedJudge = agent.ResolveAdapterConfig(resolvedJudge, e.resolver) agent.LogAdapterConfig(ctx, resolvedJudge) var err error judgeAgent, err = agentDetectWithResolvedConfig(resolvedJudge) diff --git a/internal/runner/runner.go b/internal/runner/runner.go index db42015..3f388e5 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -512,7 +512,7 @@ func buildReportInput( ModelParams: evalCfg.Engine.Model.Params, } } - resolved = agent.ResolveAdapterConfig(resolved) + resolved = agent.ResolveAdapterConfig(resolved, nil) requested := &report.AgentConfiguration{ Role: string(resolved.Role), Engine: resolved.Engine,