diff --git a/docs/design/agent-configuration.md b/docs/design/agent-configuration.md index a2eca77b..57df46f8 100644 --- a/docs/design/agent-configuration.md +++ b/docs/design/agent-configuration.md @@ -36,20 +36,25 @@ the first validation of a delegated local login. ## Current resolution order -The current runner path resolves values in these stages: +The runner path resolves values in these stages: -1. Load the eval YAML and apply `--engine` and `--model` overrides. +1. Load the eval YAML, apply `--engine`, and retain the raw `--model` value. 2. Load `~/.skill-up/credentials.yaml` and provider-scoped environment values. -3. Resolve provider-scoped `MODEL`, `API_KEY`, and `BASE_URL` values. A +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. Apply explicit CLI `--model` and `--api-key` last. -5. Let the selected adapter normalize unsupported values and construct its +5. Preserve explicit CLI `--model` and `--api-key` precedence. +6. Pass the resolved value to the selected adapter, which constructs its command and environment. -This explains why requested and effective values can differ today. A later -phase should retain both instead of reconstructing effective configuration from -the eval YAML in reports. +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. +Reports use the resolved runner engine/model identity rather than reconstructing +it from a CLI-mutated eval config. ## Legacy slashed model compatibility @@ -88,14 +93,13 @@ These translations are covered by `action/main_test.py`. Any future explicit ## Known gaps for later phases -- Provider, protocol, credential source, and effective model are not yet held - in one immutable resolved configuration. +- Protocol and adapter capabilities are not yet declared on the resolved value. - Nested provider endpoints are flattened before the adapter protocol is known. - Provider-scoped `MODEL` currently overrides an explicit YAML model. - Adapters may ignore unsupported explicit values rather than failing before case execution. -- Reports do not consistently distinguish requested configuration from the - effective adapter configuration. +- Reports use the resolved runner identity but do not yet distinguish every + requested value from the adapter's effective configuration. See [Issue #196](https://github.com/alibaba/skill-up/issues/196) for the staged cleanup plan. diff --git a/internal/agent/README.md b/internal/agent/README.md index b6e315b9..97497743 100644 --- a/internal/agent/README.md +++ b/internal/agent/README.md @@ -46,7 +46,7 @@ Both are parsed by **`parseSessionFile`** in `internal/agent/claude_code.go` (`c ``` internal/agent/ ├── agent.go # Core interface definitions: Agent, SessionResult, BaseAgent -├── factory.go # DetectAgent / DetectAgentWithInitParams factory functions +├── factory.go # DetectAgent / DetectAgentWithResolvedConfig factory functions ├── claude_code.go # ClaudeCodeAgent implementation ├── qodercli.go # QoderCLIAgent implementation ├── codex.go # CodexAgent implementation (OpenAI Codex CLI) diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 166d0f4e..844aafb5 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -538,17 +538,18 @@ func TestProbeAndMergePATH_SkipsMergeOnEmptyStdout(t *testing.T) { } } -func TestDetectAgentWithInitParams_SetsTypedCredentialFields(t *testing.T) { +func TestDetectAgentWithResolvedConfig_SetsTypedCredentialFields(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithInitParams("codex", credential.AgentInitParams{ + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "codex", Provider: "openai", Model: "gpt-5.4", APIKey: "openai-test-token", BaseURL: "https://openai.example.com/v1", - }, nil) + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } codexAgent, ok := ag.(*CodexAgent) @@ -569,16 +570,17 @@ func TestDetectAgentWithInitParams_SetsTypedCredentialFields(t *testing.T) { } } -func TestDetectAgentWithInitParams_QoderMapsAPIKeyToRuntimeEnv(t *testing.T) { +func TestDetectAgentWithResolvedConfig_QoderMapsAPIKeyToRuntimeEnv(t *testing.T) { token := "qoder-runtime-token" //nolint:gosec // test credential, not real t.Setenv(credential.EnvQoderPersonalAccessToken, token) - ag, err := DetectAgentWithInitParams("qoder-cli", credential.AgentInitParams{ + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "qoder-cli", Provider: "qoder", Model: "auto", - }, nil) + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } qoderAgent, ok := ag.(*QoderCLIAgent) @@ -590,17 +592,19 @@ func TestDetectAgentWithInitParams_QoderMapsAPIKeyToRuntimeEnv(t *testing.T) { } } -func TestDetectAgentWithInitParams_QoderCNMapsKeychainAliasToOfficialEnv(t *testing.T) { +func TestDetectAgentWithResolvedConfig_QoderCNMapsKeychainAliasToOfficialEnv(t *testing.T) { token := "qoder-cn-runtime-token" //nolint:gosec // test credential, not real t.Setenv(credential.EnvQoderCNAccessToken, token) t.Setenv(credential.EnvQoderCNPersonalAccessToken, "") - ag, err := DetectAgentWithInitParams("qoder-cli", credential.AgentInitParams{ + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "qoder-cli", Provider: "qoder", Model: "auto", - }, map[string]string{KwargEdition: qoderEditionCN}) + Kwargs: map[string]string{KwargEdition: qoderEditionCN}, + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } qoderAgent, ok := ag.(*QoderCLIAgent) @@ -615,15 +619,16 @@ func TestDetectAgentWithInitParams_QoderCNMapsKeychainAliasToOfficialEnv(t *test } } -func TestDetectAgentWithInitParams_QoderCNPrefersOfficialEnv(t *testing.T) { +func TestDetectAgentWithResolvedConfig_QoderCNPrefersOfficialEnv(t *testing.T) { t.Setenv(credential.EnvQoderCNPersonalAccessToken, "official-token") t.Setenv(credential.EnvQoderCNAccessToken, "alias-token") - ag, err := DetectAgentWithInitParams("qoder-cli", credential.AgentInitParams{}, map[string]string{ - KwargEdition: qoderEditionCN, + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "qoder-cli", + Kwargs: map[string]string{KwargEdition: qoderEditionCN}, }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } qoderAgent, ok := ag.(*QoderCLIAgent) if !ok { @@ -634,16 +639,17 @@ func TestDetectAgentWithInitParams_QoderCNPrefersOfficialEnv(t *testing.T) { } } -func TestDetectAgentWithInitParams_QoderIgnoresParamsAPIKey(t *testing.T) { +func TestDetectAgentWithResolvedConfig_QoderIgnoresParamsAPIKey(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithInitParams("qoder-cli", credential.AgentInitParams{ //nolint:gosec // test dummy key + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ //nolint:gosec // test dummy key + Engine: "qoder-cli", Provider: "anthropic", Model: "auto", APIKey: "sk-ant-should-not-appear", - }, nil) + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } qoderAgent, ok := ag.(*QoderCLIAgent) @@ -678,14 +684,15 @@ func TestUnsupportedAgentError(t *testing.T) { } } -func TestDetectAgentWithInitParams_StripsAutoForNonQoderEngines(t *testing.T) { +func TestDetectAgentWithResolvedConfig_UsesResolvedModelWithoutNormalization(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithInitParams("claude-code", credential.AgentInitParams{ - Model: "auto", - }, nil) + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "claude-code", + Model: "", + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } ccAgent, ok := ag.(*ClaudeCodeAgent) @@ -693,18 +700,19 @@ func TestDetectAgentWithInitParams_StripsAutoForNonQoderEngines(t *testing.T) { t.Fatalf("expected *ClaudeCodeAgent, got %T", ag) } if got := ccAgent.Cfg.ModelName; got != "" { - t.Fatalf("ModelName = %q, want empty (auto should be stripped for claude-code)", got) + t.Fatalf("ModelName = %q, want the already-resolved empty value", got) } } -func TestDetectAgentWithInitParams_PreservesAutoForQoderCLI(t *testing.T) { +func TestDetectAgentWithResolvedConfig_PreservesAutoForQoderCLI(t *testing.T) { t.Parallel() - ag, err := DetectAgentWithInitParams("qoder-cli", credential.AgentInitParams{ - Model: "auto", - }, nil) + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "qoder-cli", + Model: "auto", + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } qoderAgent, ok := ag.(*QoderCLIAgent) @@ -716,16 +724,18 @@ func TestDetectAgentWithInitParams_PreservesAutoForQoderCLI(t *testing.T) { } } -func TestDetectAgentWithInitParams_ForwardsKwargs(t *testing.T) { +func TestDetectAgentWithResolvedConfig_ForwardsKwargs(t *testing.T) { t.Parallel() kwargs := map[string]string{KwargBypassSandbox: "true", "future_key": "x"} - ag, err := DetectAgentWithInitParams("codex", credential.AgentInitParams{ + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "codex", Provider: "openai", Model: "gpt-5.4", - }, kwargs) + Kwargs: kwargs, + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams failed: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig failed: %v", err) } codexAgent, ok := ag.(*CodexAgent) diff --git a/internal/agent/custom_test.go b/internal/agent/custom_test.go index 1f4b1c86..fb1ee434 100644 --- a/internal/agent/custom_test.go +++ b/internal/agent/custom_test.go @@ -882,18 +882,19 @@ func TestDetectAgent_NonBuiltinWithoutCustom(t *testing.T) { } } -func TestDetectAgentWithInitParams_KeepsAutoModelForCustom(t *testing.T) { +func TestDetectAgentWithResolvedConfig_KeepsAutoModelForCustom(t *testing.T) { t.Parallel() custom := &config.CustomEngineConfig{ Transport: "local", Local: &config.CustomLocalConfig{Command: "/opt/agent"}, } - ag, err := DetectAgentWithInitParams("my-agent", credential.AgentInitParams{ + ag, err := DetectAgentWithResolvedConfig(credential.ResolvedAgentConfig{ + Engine: "my-agent", Model: modelAuto, Custom: custom, - }, nil) + }) if err != nil { - t.Fatalf("DetectAgentWithInitParams: %v", err) + t.Fatalf("DetectAgentWithResolvedConfig: %v", err) } ca, ok := ag.(*CustomAgent) if !ok { diff --git a/internal/agent/factory.go b/internal/agent/factory.go index f8d8faa2..70f686c4 100644 --- a/internal/agent/factory.go +++ b/internal/agent/factory.go @@ -33,30 +33,21 @@ func DetectAgent(engineName string, cfg Config) (Agent, error) { } } -// DetectAgentWithInitParams maps resolved init params into an engine-specific agent config. -// kwargs carries engine.kwargs from eval.yaml (or --engine-kwarg overrides) and -// is forwarded as-is to the agent; each agent reads only the keys it understands. -func DetectAgentWithInitParams(engineName string, params credential.AgentInitParams, kwargs map[string]string) (Agent, error) { - model := params.Model - // "auto" is a QoderCLI-specific model tier; strip it for other built-in - // engines so they don't need to hard-code awareness of it. A custom engine - // keeps the user's configured value — it is exposed verbatim via ${model} - // and SessionInput.model. - if model == "auto" && !isQoderCLIEngine(engineName) && params.Custom == nil { - model = "" - } - +// DetectAgentWithResolvedConfig maps a resolved role configuration into the +// selected adapter without reinterpreting raw YAML or CLI values. +func DetectAgentWithResolvedConfig(params credential.ResolvedAgentConfig) (Agent, error) { + engineName := params.Engine cfg := Config{ Name: engineName, - ModelName: model, + ModelName: params.Model, ModelProvider: params.Provider, APIKey: params.APIKey, BaseURL: params.BaseURL, EnvVars: make(map[string]string), - Kwargs: kwargs, + Kwargs: params.Kwargs, Custom: params.Custom, } - logUnknownEngineKwargs(engineName, kwargs) + logUnknownEngineKwargs(engineName, params.Kwargs) switch engineName { case agentkind.QoderCLIAlias, agentkind.QoderAlias, agentkind.QoderCLI: @@ -64,7 +55,7 @@ func DetectAgentWithInitParams(engineName string, params credential.AgentInitPar // underlying model provider (e.g. anthropic). params.APIKey may hold a provider-scoped // key (e.g. ANTHROPIC_API_KEY) which must not be forwarded as the qodercli token. // See docs/bugfix/Bug_ QODER_PERSONAL_ACCESS_TOKEN is invalid.md for details. - profile := qoderProfileForKwargs(kwargs) + profile := qoderProfileForKwargs(params.Kwargs) sourceEnv := profile.credentialEnv token := os.Getenv(sourceEnv) if token == "" && profile.edition == qoderEditionCN { @@ -75,22 +66,13 @@ func DetectAgentWithInitParams(engineName string, params credential.AgentInitPar cfg.EnvVars[profile.credentialEnv] = token logging.Debugf( "AGENT_CONFIG kind=%s engine=%s edition=%s auth_env=%s source.auth=process_env source.env=%s", - params.Kind, engineName, profile.edition, profile.credentialEnv, sourceEnv, + params.Role, engineName, profile.edition, profile.credentialEnv, sourceEnv, ) } if params.BaseURL != "" { - logging.Debugf("AGENT_CONFIG kind=%s engine=%s ignored.base_url reason=unsupported_by_agent", params.Kind, engineName) + logging.Debugf("AGENT_CONFIG kind=%s engine=%s ignored.base_url reason=unsupported_by_agent", params.Role, engineName) } } return DetectAgent(engineName, cfg) } - -func isQoderCLIEngine(name string) bool { - switch name { - case agentkind.QoderCLIAlias, agentkind.QoderAlias, agentkind.QoderCLI: - return true - default: - return false - } -} diff --git a/internal/cli/run.go b/internal/cli/run.go index b6d07d6e..838f131b 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -31,7 +31,6 @@ import ( ) const ( - modelFormatParts = 2 maxParallelismOverride = 256 runtimeKwargFlagName = "runtime-kwarg" runtimeKwargAlias = "rk" @@ -147,13 +146,13 @@ func runEval(cmd *cobra.Command, args []string) error { } // --- Phase 2: Credentials & agent --- - ag, resolver, runnerParams, err := loadCredentialsAndAgent(cmd, evalCfg) + ag, resolver, runnerConfig, err := loadCredentialsAndAgent(cmd, evalCfg) if err != nil { return err } // --- Phase 3: Run evaluation --- - results, err := executeEvaluation(cmd, cases, evalCfg, loader, resolver, runnerParams, ag) + results, err := executeEvaluation(cmd, cases, evalCfg, loader, resolver, runnerConfig, ag) if err != nil { return err } @@ -196,18 +195,23 @@ func loadAndPrepareConfig(ctx context.Context, cmd *cobra.Command, args []string } engineName, _ := cmd.Flags().GetString("engine") - evalCfg = resolveEvalConfig(evalCfg, engineName, cmd) + evalCfg = resolveEvalConfig(evalCfg, engineName) if err := applyRunConfigOverrides(evalCfg, cmd); err != nil { //nolint:contextcheck // ctx accessed via cmd.Context() inside helpers return nil, nil, nil, err } + modelFlag, _ := cmd.Flags().GetString("model") // The loader defers engine.custom env resolution and validation until the // final engine name is known (it can be changed by --engine); process it - // now so an override is never blocked by an unrelated custom block. - if err := config.ResolveCustomEngineConfig(evalCfg); err != nil { + // now so an override is never blocked by an unrelated custom block. A CLI + // model supersedes the YAML provider/name, so stale references in those two + // fields are not expanded. Base URL and model params remain active. + if err := config.ResolveCustomEngineConfigWithOptions(evalCfg, config.ResolveCustomEngineOptions{ + SkipModelIdentity: modelFlag != "", + }); err != nil { return nil, nil, nil, fmt.Errorf("engine config: %w", err) } - modelRef := formatModelRef(evalCfg.Engine.Model.Provider, evalCfg.Engine.Model.Name) + modelRef := requestedModelRef(evalCfg.Engine.Model, modelFlag) span.SetAttributes( attribute.Int("skill_up.cases.count", len(cases)), attribute.String("skill_up.engine", evalCfg.Engine.Name), @@ -236,7 +240,7 @@ func maybeRunDryRun(cmd *cobra.Command, cases []*config.CaseConfig, evalCfg *con // loadCredentialsAndAgent executes Phase 2: load resolver, derive runner init // params, and instantiate the configured agent. -func loadCredentialsAndAgent(cmd *cobra.Command, evalCfg *config.EvalConfig) (agent.Agent, *credential.Resolver, credential.AgentInitParams, error) { +func loadCredentialsAndAgent(cmd *cobra.Command, evalCfg *config.EvalConfig) (agent.Agent, *credential.Resolver, credential.ResolvedAgentConfig, error) { ui.Blank() ui.Step("🔑", "Loading credentials...") cliModel, _ := cmd.Flags().GetString("model") @@ -244,34 +248,21 @@ func loadCredentialsAndAgent(cmd *cobra.Command, evalCfg *config.EvalConfig) (ag resolver := credential.NewResolver(credential.DefaultConfPath()) if err := resolver.Load(); err != nil { - return nil, nil, credential.AgentInitParams{}, fmt.Errorf("failed to load credentials: %w", err) - } - - // `--model provider/name` was tentatively split into Provider+Name by - // resolveEvalConfig (before the resolver was loaded). Now that we know - // which providers actually have configuration, collapse the split back - // when the provider is unknown: in that case the slashed string is more - // likely a literal model identifier the upstream API expects verbatim - // (e.g. anthropic-proxy gateways registering models under - // `anthropic_modelscope/deepseek-v4-pro`) than a credential-namespace - // prefix the agent should peel off. - collapseUnconfiguredProviderSplit(evalCfg, resolver, cliModel) - - runnerParams := credential.ResolveRunnerInitParams( - evalCfg.Engine.Name, - evalCfg.Engine.Model, - evalCfg.Engine.Custom, + return nil, nil, credential.ResolvedAgentConfig{}, fmt.Errorf("failed to load credentials: %w", err) + } + + runnerConfig := credential.ResolveRunnerConfig( + evalCfg.Engine, resolver, - normalizeCLIModelOverride(cliModel, resolver), - cliAPIKey, + credential.CLIOverrides{Model: cliModel, APIKey: cliAPIKey}, ) - ag, err := agent.DetectAgentWithInitParams(evalCfg.Engine.Name, runnerParams, evalCfg.Engine.Kwargs) + ag, err := agent.DetectAgentWithResolvedConfig(runnerConfig) if err != nil { - return nil, nil, credential.AgentInitParams{}, fmt.Errorf("failed to create agent: %w", err) + return nil, nil, credential.ResolvedAgentConfig{}, fmt.Errorf("failed to create agent: %w", err) } ui.Status("✅", "Credentials ready") - return ag, resolver, runnerParams, nil + return ag, resolver, runnerConfig, nil } // executeEvaluation executes Phase 3: build the runner, derive evaluate @@ -282,13 +273,13 @@ func executeEvaluation( evalCfg *config.EvalConfig, loader *config.Loader, resolver *credential.Resolver, - runnerParams credential.AgentInitParams, + runnerConfig credential.ResolvedAgentConfig, ag agent.Agent, ) ([]evaluator.EvalResult, error) { ui.Blank() ui.Stepf("🚀", "Running evaluation (%d cases)", len(cases)) - run := runner.NewRunner(evalCfg, loader, resolver, runnerParams) + run := runner.NewRunner(evalCfg, loader, resolver, runnerConfig) evaluateOpts, err := evaluateOptionsFromFlags(cmd) if err != nil { @@ -435,49 +426,8 @@ func evaluateOptionsFromFlags(cmd *cobra.Command) (runner.EvaluateOptions, error }, nil } -// collapseUnconfiguredProviderSplit re-runs credential.ResolveModelRef -// against the loaded resolver to undo the optimistic split that -// resolveEvalConfig performs on `--model provider/name` when the provider -// half turns out to be unconfigured. See ResolveModelRef for the -// rationale and the debug log emitted on collapse. -// -// Gate: only runs when `cliModel` contains `/`. eval.yaml-sourced pairs -// (where the user wrote provider and name as separate YAML keys) reach -// here without `/` in cliModel and must be preserved — they are -// user-authored explicit configuration, often relying on a CLI's -// persisted login state with no env footprint. -// -// CLI-hint signals (`--api-key`, `engine.model.base_url`) were once used -// to FORCE a split through, but that broke `--api-key K --model literal_ -// opaque/id` flows (proxy-registered ids like -// `anthropic_modelscope/deepseek-v4-pro`). credential.applyCLIOverrides -// no longer requires a non-empty Provider to apply the CLI key — each -// agent routes cfg.APIKey via its own hardcoded env (ANTHROPIC_API_KEY / -// OPENAI_API_KEY) regardless of Provider — so the literal-id case now -// passes through correctly. Users who really mean `provider as namespace` -// should configure that provider via env / credentials.yaml; that signal -// alone is enough for ResolveModelRef to preserve the split. -// -// Safe no-op when Provider is empty or already collapsed. -func collapseUnconfiguredProviderSplit(evalCfg *config.EvalConfig, resolver *credential.Resolver, cliModel string) { - if evalCfg == nil { - return - } - provider := evalCfg.Engine.Model.Provider - name := evalCfg.Engine.Model.Name - if provider == "" || name == "" { - return - } - if !strings.Contains(cliModel, "/") { - return - } - newProvider, newName := credential.ResolveModelRef(provider+"/"+name, resolver) - evalCfg.Engine.Model.Provider = newProvider - evalCfg.Engine.Model.Name = newName -} - // resolveEvalConfig resolves the engine name and ensures evalCfg is non-nil. -func resolveEvalConfig(evalCfg *config.EvalConfig, engineName string, cmd *cobra.Command) *config.EvalConfig { +func resolveEvalConfig(evalCfg *config.EvalConfig, engineName string) *config.EvalConfig { if engineName == "" { if evalCfg != nil { engineName = evalCfg.Engine.Name @@ -493,17 +443,6 @@ func resolveEvalConfig(evalCfg *config.EvalConfig, engineName string, cmd *cobra evalCfg.Engine.Name = engineName - if modelFlag, _ := cmd.Flags().GetString("model"); modelFlag != "" { - parts := strings.SplitN(modelFlag, "/", modelFormatParts) - if len(parts) == modelFormatParts && parts[0] != "" && parts[1] != "" { - evalCfg.Engine.Model.Provider = parts[0] - evalCfg.Engine.Model.Name = parts[1] - } else { - evalCfg.Engine.Model.Provider = "" - evalCfg.Engine.Model.Name = modelFlag - } - } - return evalCfg } @@ -674,18 +613,11 @@ func formatModelRef(provider, name string) string { } } -// normalizeCLIModelOverride returns the bare model identifier from a -// `--model` flag value, peeling off any `provider/` prefix only when the -// prefix is a configured provider per credential.ResolveModelRef. Kept -// in lockstep with collapseUnconfiguredProviderSplit so applyCLIOverrides -// never receives a model identifier that contradicts the post-collapse -// evalCfg state. -func normalizeCLIModelOverride(modelFlag string, resolver *credential.Resolver) string { - if modelFlag == "" { - return modelFlag +func requestedModelRef(model config.ModelConfig, cliModel string) string { + if cliModel != "" { + return cliModel } - _, name := credential.ResolveModelRef(modelFlag, resolver) - return name + return formatModelRef(model.Provider, model.Name) } func isDirectory(pathname string) bool { diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index e8438acd..ef5dda54 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -14,7 +14,6 @@ import ( "github.com/spf13/cobra" "github.com/alibaba/skill-up/internal/config" - "github.com/alibaba/skill-up/internal/credential" "github.com/alibaba/skill-up/internal/evaluator" "github.com/alibaba/skill-up/internal/judge" "github.com/alibaba/skill-up/internal/ui" @@ -28,13 +27,6 @@ const ( testFlagBoolTrue = "true" ) -// Test fixtures for the provider/model split disambiguation suite. -const ( - testProviderDashscope = "dashscope" - testModelClaudeSonnet = "claude-sonnet-4-6" - testModelRefDashscope = testProviderDashscope + "/" + testModelClaudeSonnet -) - func TestRunCommand_UsesUsageAwareArgs(t *testing.T) { t.Parallel() @@ -112,6 +104,47 @@ func TestRunEvalDryRunLoadsFiltersAndSkipsAgentSetup(t *testing.T) { } } +func TestRunEvalCLIModelSkipsSupersededYAMLModelReference(t *testing.T) { + t.Setenv("MISSING_MODEL", "") + root := t.TempDir() + writeAutoModeSkill(t, root) + evalsDir := filepath.Join(root, "evals") + casesDir := filepath.Join(evalsDir, "cases") + if err := os.MkdirAll(casesDir, 0o755); err != nil { + t.Fatal(err) + } + evalYAML := `schema_version: v1alpha1 +environment: + type: none +engine: + name: claude_code + model: + provider: anthropic + name: "${MISSING_MODEL:?must set}" +cases: + files: + - evals/cases/basic.yaml +` + if err := os.WriteFile(filepath.Join(evalsDir, "eval.yaml"), []byte(evalYAML), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(casesDir, "basic.yaml"), []byte("id: basic\ninput:\n prompt: hello\n"), 0o600); err != nil { + t.Fatal(err) + } + + cmd := newRunPhaseTestCommand(t) + if err := cmd.Flags().Set("dry-run", testFlagBoolTrue); err != nil { + t.Fatal(err) + } + if err := cmd.Flags().Set("model", "claude-sonnet-4-6"); err != nil { + t.Fatal(err) + } + + if _, err := captureStdout(t, func() error { return runEval(cmd, []string{root}) }); err != nil { + t.Fatalf("explicit --model should bypass stale YAML model reference: %v", err) + } +} + func TestRunEvalValidatesOnlySelectedCases(t *testing.T) { root := t.TempDir() writeAutoModeSkill(t, root) @@ -210,8 +243,8 @@ func TestLoadCredentialsAndAgentAppliesCLIModelAndAPIKey(t *testing.T) { if ag == nil || resolver == nil { t.Fatalf("agent/resolver = %v/%v, want non-nil", ag, resolver) } - if params.Model != "gpt-5" || params.Provider != "" || params.APIKey != "sk-test" { - t.Fatalf("runner params = %+v, want literal gpt-5 with cli API key", params) + if params.Model != "gpt-5" || params.Provider != "openai" || params.APIKey != "sk-test" { + t.Fatalf("runner params = %+v, want resolved openai/gpt-5 with cli API key", params) } } @@ -1218,35 +1251,19 @@ func TestEvaluateOptionsFromFlags_RejectsNegativeIteration(t *testing.T) { } } -// runResolveEvalConfigCase parametrises "set --model flag → resolveEvalConfig -// → assert provider/name". Extracted to silence dupl on near-identical bodies. -func runResolveEvalConfigCase(t *testing.T, modelFlag, engine, wantProvider, wantName string) { - t.Helper() - cmd := &cobra.Command{} - cmd.Flags().String("model", "", "") - if err := cmd.Flags().Set("model", modelFlag); err != nil { - t.Fatalf("set model: %v", err) - } - - cfg := resolveEvalConfig(config.DefaultEvalConfig(), engine, cmd) - if got := cfg.Engine.Model.Provider; got != wantProvider { - t.Fatalf("Engine.Model.Provider = %q, want %q", got, wantProvider) +func TestResolveEvalConfig_OnlyResolvesEngine(t *testing.T) { + t.Parallel() + cfg := config.DefaultEvalConfig() + cfg.Engine.Model = config.ModelConfig{Provider: "configured", Name: "model"} + resolved := resolveEvalConfig(cfg, "codex") + if resolved.Engine.Name != "codex" { + t.Fatalf("Engine.Name = %q, want codex", resolved.Engine.Name) } - if got := cfg.Engine.Model.Name; got != wantName { - t.Fatalf("Engine.Model.Name = %q, want %q", got, wantName) + if resolved.Engine.Model.Provider != "configured" || resolved.Engine.Model.Name != "model" { + t.Fatalf("resolveEvalConfig mutated model config: %#v", resolved.Engine.Model) } } -func TestResolveEvalConfig_AllowsRawModelOverrideForAnyEngine(t *testing.T) { - t.Parallel() - runResolveEvalConfigCase(t, "auto", "codex", "", "auto") -} - -func TestResolveEvalConfig_ParsesProviderQualifiedModel(t *testing.T) { - t.Parallel() - runResolveEvalConfigCase(t, "anthropic/auto", testEngineClaudeCode, "anthropic", "auto") -} - func TestApplyRunConfigOverrides_Parallelism(t *testing.T) { t.Parallel() @@ -1609,42 +1626,11 @@ func TestApplyRunConfigOverrides_RejectsInvalidParallelism(t *testing.T) { } } -func TestNormalizeCLIModelOverride_StripsProviderPrefix(t *testing.T) { - // "anthropic" is hardcoded as always-configured in HasProvider (the - // upstream API only accepts bare model ids), so this test no longer - // needs to set ANTHROPIC_API_KEY. +func TestRequestedModelRef_PrefersRawCLIValue(t *testing.T) { t.Parallel() - - if got := normalizeCLIModelOverride("anthropic/auto", nil); got != "auto" { - t.Fatalf("normalizeCLIModelOverride() = %q, want auto", got) - } -} - -func TestNormalizeCLIModelOverride_PreservesRawModel(t *testing.T) { - t.Parallel() - - if got := normalizeCLIModelOverride(testModelClaudeSonnet, nil); got != testModelClaudeSonnet { - t.Fatalf("normalizeCLIModelOverride() = %q, want %s", got, testModelClaudeSonnet) - } -} - -func TestNormalizeCLIModelOverride_UnconfiguredProviderKeepsFullString(t *testing.T) { - // Not parallel: unsets env to make sure the provider half is unknown. - for _, key := range []string{ - "ANTHROPIC_MODELSCOPE_API_KEY", - "ANTHROPIC_MODELSCOPE_BASE_URL", - } { - if err := os.Unsetenv(key); err != nil { - t.Fatal(err) - } - } - - // Anthropic-proxy gateways register models under provider/name identifiers - // (e.g. ducky's `anthropic_modelscope/deepseek-v4-pro`); stripping the - // prefix would defeat the un-split in collapseUnconfiguredProviderSplit. - got := normalizeCLIModelOverride("anthropic_modelscope/deepseek-v4-pro", nil) - if got != "anthropic_modelscope/deepseek-v4-pro" { - t.Fatalf("normalizeCLIModelOverride() = %q, want anthropic_modelscope/deepseek-v4-pro", got) + model := config.ModelConfig{Provider: "yaml-provider", Name: "yaml-model"} + if got := requestedModelRef(model, "opaque/model"); got != "opaque/model" { + t.Fatalf("requestedModelRef() = %q, want opaque/model", got) } } @@ -1672,161 +1658,3 @@ func TestFilterCases(t *testing.T) { t.Errorf("expected advanced-feature, got %s", filtered[0].ID) } } - -func TestCollapseUnconfiguredProviderSplit_CollapsesWhenProviderUnknown(t *testing.T) { - // Not parallel: depends on the absence of any anthropic_modelscope env. - for _, key := range []string{ - "ANTHROPIC_MODELSCOPE_API_KEY", - "ANTHROPIC_MODELSCOPE_BASE_URL", - } { - if err := os.Unsetenv(key); err != nil { - t.Fatal(err) - } - } - - cfg := config.DefaultEvalConfig() - cfg.Engine.Model.Provider = "anthropic_modelscope" - cfg.Engine.Model.Name = "deepseek-v4-pro" - - collapseUnconfiguredProviderSplit(cfg, credential.NewResolver(""), - "anthropic_modelscope/deepseek-v4-pro") - - if cfg.Engine.Model.Provider != "" { - t.Fatalf("Provider = %q, want \"\" (collapsed)", cfg.Engine.Model.Provider) - } - // Anthropic-proxy gateways need the full identifier; the un-split must - // glue the original `provider/name` back together so it reaches the - // upstream API verbatim. - if cfg.Engine.Model.Name != "anthropic_modelscope/deepseek-v4-pro" { - t.Fatalf("Name = %q, want anthropic_modelscope/deepseek-v4-pro", cfg.Engine.Model.Name) - } -} - -func TestCollapseUnconfiguredProviderSplit_KeepsSplitWhenProviderConfigured(t *testing.T) { - // Not parallel: relies on DASHSCOPE_API_KEY env to mark provider as configured. - t.Setenv("DASHSCOPE_API_KEY", "sk-test") - - cfg := config.DefaultEvalConfig() - cfg.Engine.Model.Provider = testProviderDashscope - cfg.Engine.Model.Name = testModelClaudeSonnet - - collapseUnconfiguredProviderSplit(cfg, credential.NewResolver(""), testModelRefDashscope) - - // `provider: dashscope, name: claude-sonnet-4-6` is the credential-namespace - // usage — DASHSCOPE_* env routes auth, the bare claude model id is what - // the upstream Anthropic-compatible endpoint expects. Must not collapse. - if cfg.Engine.Model.Provider != testProviderDashscope { - t.Fatalf("Provider = %q, want %s", cfg.Engine.Model.Provider, testProviderDashscope) - } - if cfg.Engine.Model.Name != testModelClaudeSonnet { - t.Fatalf("Name = %q, want %s", cfg.Engine.Model.Name, testModelClaudeSonnet) - } -} - -func TestCollapseUnconfiguredProviderSplit_NoopOnEmptyProvider(t *testing.T) { - t.Parallel() - - cfg := config.DefaultEvalConfig() - cfg.Engine.Model.Provider = "" - cfg.Engine.Model.Name = "claude-opus-4-7" - - collapseUnconfiguredProviderSplit(cfg, nil, "claude-opus-4-7") - - if cfg.Engine.Model.Provider != "" || cfg.Engine.Model.Name != "claude-opus-4-7" { - t.Fatalf("unexpected mutation: %+v", cfg.Engine.Model) - } -} - -type collapsePreservesSplitCase struct { - name string - unsetEnvs []string - setEnvs map[string]string - provider string - modelName string - baseURL string - cliModel string - failMsg string -} - -var collapsePreservesSplitCases = []collapsePreservesSplitCase{ - { - name: "EvalYamlPairWhenCliModelHasNoSlash", - // Non-framework, unconfigured provider so the only thing keeping - // the split is the cliModel-has-no-slash guard (eval.yaml-sourced - // pairs must not be rewritten). - unsetEnvs: []string{ - "ANTHROPIC_MODELSCOPE_API_KEY", - "ANTHROPIC_MODELSCOPE_BASE_URL", - }, - provider: "anthropic_modelscope", - modelName: "deepseek-v4-pro", - cliModel: "", - failMsg: "eval.yaml-sourced pair was rewritten", - }, - { - name: "QoderPATIsConfig", - // QODER_PERSONAL_ACCESS_TOKEN is the canonical Qoder credential — - // must count as "configured" for collapse. - setEnvs: map[string]string{"QODER_PERSONAL_ACCESS_TOKEN": "qpat-fixture"}, - provider: "qoder", - modelName: "auto", - cliModel: "qoder/auto", - failMsg: "qoder split was collapsed despite PAT env", - }, - { - name: "AnthropicFrameworkDefaultWithoutEnv", - // `--model anthropic/X` with no env (relying on `claude` CLI's - // persisted login state): "anthropic" is a framework default that - // MUST stay split. The upstream Anthropic API only knows bare - // model ids; collapsing would be rejected. - unsetEnvs: []string{ - "ANTHROPIC_API_KEY", - "ANTHROPIC_BASE_URL", - }, - provider: "anthropic", - modelName: testModelClaudeSonnet, - cliModel: "anthropic/" + testModelClaudeSonnet, - failMsg: "anthropic CLI split was collapsed despite framework-default status", - }, -} - -// TestCollapseUnconfiguredProviderSplit_PreservesSplit exercises the -// paths that must NOT collapse a `provider/name` pair: eval.yaml-sourced -// pairs (no cliModel slash), Qoder PAT env, framework-default providers -// (anthropic) relying on persisted CLI login state. Each subtest arranges -// the credential surface so collapse-via-resolver alone would otherwise -// fire, then asserts the pair survives. -// -// Note: --api-key and engine.model.base_url were once "preserve" signals, -// but that forced literal-opaque-id flows (`--model X/Y` where X isn't a -// configured namespace) to be wrongly split. CLI key now applies even -// with Provider="" (applyCLIOverrides no longer requires it), so the -// safer default is to let ResolveModelRef collapse when no persisted -// provider config exists. See TestCollapseUnconfiguredProviderSplit_ -// CollapsesEvenWithCLIAPIKey for the flipped behavior. -func TestCollapseUnconfiguredProviderSplit_PreservesSplit(t *testing.T) { - for _, tc := range collapsePreservesSplitCases { - t.Run(tc.name, func(t *testing.T) { - // Not parallel: env mutations are process-global. - for _, key := range tc.unsetEnvs { - if err := os.Unsetenv(key); err != nil { - t.Fatal(err) - } - } - for k, v := range tc.setEnvs { - t.Setenv(k, v) - } - - cfg := config.DefaultEvalConfig() - cfg.Engine.Model.Provider = tc.provider - cfg.Engine.Model.Name = tc.modelName - cfg.Engine.Model.BaseURL = tc.baseURL - - collapseUnconfiguredProviderSplit(cfg, credential.NewResolver(""), tc.cliModel) - - if cfg.Engine.Model.Provider != tc.provider || cfg.Engine.Model.Name != tc.modelName { - t.Fatalf("%s: %+v", tc.failMsg, cfg.Engine.Model) - } - }) - } -} diff --git a/internal/config/customengine.go b/internal/config/customengine.go index e184c00e..992f656e 100644 --- a/internal/config/customengine.go +++ b/internal/config/customengine.go @@ -166,7 +166,23 @@ func IsBuiltinTemplateVar(name string) bool { // so callers invoke this once that name is settled. It is a no-op for built-in // engines, which ignore any engine.custom block. func ResolveCustomEngineConfig(cfg *EvalConfig) error { - if err := resolveCustomEngineEnv(cfg); err != nil { + return ResolveCustomEngineConfigWithOptions(cfg, ResolveCustomEngineOptions{}) +} + +// ResolveCustomEngineOptions controls which raw engine fields are relevant to +// the current invocation. +type ResolveCustomEngineOptions struct { + // SkipModelIdentity leaves engine.model.provider/name untouched when an + // explicit CLI model has already superseded those YAML fields. BaseURL and + // Params remain active and are still resolved. + SkipModelIdentity bool +} + +// ResolveCustomEngineConfigWithOptions resolves and validates the active +// custom-engine configuration while allowing superseded YAML model fields to +// be excluded from environment expansion. +func ResolveCustomEngineConfigWithOptions(cfg *EvalConfig, opts ResolveCustomEngineOptions) error { + if err := resolveCustomEngineEnv(cfg, opts); err != nil { return err } if errs := validateEngine(cfg.Engine); len(errs) > 0 { @@ -183,12 +199,12 @@ func ResolveCustomEngineConfig(cfg *EvalConfig) error { // resolution keys off the resolved provider, and an unresolved // `${MODEL_PROVIDER:-...}` literal would silently break auth). Built-in // template variables are left intact for run-time resolution. -func resolveCustomEngineEnv(cfg *EvalConfig) error { +func resolveCustomEngineEnv(cfg *EvalConfig, opts ResolveCustomEngineOptions) error { // engine.model resolution must run regardless of whether engine.custom // is active, so a config that used both a custom block and a templated // model still has its model fields resolved after a --engine override // drops the custom block from the runtime path. - modelErrs := resolveModelEnv(&cfg.Engine.Model) + modelErrs := resolveModelEnv(&cfg.Engine.Model, opts) custom := cfg.Engine.Custom if custom == nil || IsBuiltinEngineName(cfg.Engine.Name) { @@ -314,17 +330,19 @@ func resolveHTTPEnv(h *customengine.HTTPConfig) []string { } // resolveModelEnv resolves env references in engine.model string values. -func resolveModelEnv(model *ModelConfig) []string { +func resolveModelEnv(model *ModelConfig, opts ResolveCustomEngineOptions) []string { var errs []string - if v, err := resolveEnvRefs(model.Provider); err != nil { - errs = append(errs, fmt.Sprintf("engine.model.provider: %s", err)) - } else { - model.Provider = v - } - if v, err := resolveEnvRefs(model.Name); err != nil { - errs = append(errs, fmt.Sprintf("engine.model.name: %s", err)) - } else { - model.Name = v + if !opts.SkipModelIdentity { + if v, err := resolveEnvRefs(model.Provider); err != nil { + errs = append(errs, fmt.Sprintf("engine.model.provider: %s", err)) + } else { + model.Provider = v + } + if v, err := resolveEnvRefs(model.Name); err != nil { + errs = append(errs, fmt.Sprintf("engine.model.name: %s", err)) + } else { + model.Name = v + } } if v, err := resolveEnvRefs(model.BaseURL); err != nil { errs = append(errs, fmt.Sprintf("engine.model.base_url: %s", err)) diff --git a/internal/config/customengine_test.go b/internal/config/customengine_test.go index 746edf5b..eaeeb567 100644 --- a/internal/config/customengine_test.go +++ b/internal/config/customengine_test.go @@ -43,7 +43,7 @@ func TestResolveCustomEngineEnv_AllForms(t *testing.T) { }, } - if err := resolveCustomEngineEnv(cfg); err != nil { + if err := resolveCustomEngineEnv(cfg, ResolveCustomEngineOptions{}); err != nil { t.Fatalf("resolveCustomEngineEnv: %v", err) } @@ -77,7 +77,7 @@ func TestResolveCustomEngineEnv_MissingRequiredVar(t *testing.T) { }, } - err := resolveCustomEngineEnv(cfg) + err := resolveCustomEngineEnv(cfg, ResolveCustomEngineOptions{}) if err == nil { t.Fatal("expected error for missing required env var") } @@ -97,7 +97,7 @@ func TestResolveCustomEngineEnv_ErrorForm(t *testing.T) { }, } - err := resolveCustomEngineEnv(cfg) + err := resolveCustomEngineEnv(cfg, ResolveCustomEngineOptions{}) if err == nil || !strings.Contains(err.Error(), "token is required") { t.Fatalf("error = %v, want custom error message", err) } @@ -105,7 +105,7 @@ func TestResolveCustomEngineEnv_ErrorForm(t *testing.T) { func TestResolveCustomEngineEnv_NoCustomIsNoop(t *testing.T) { cfg := &EvalConfig{Engine: EngineConfig{Name: "claude_code"}} - if err := resolveCustomEngineEnv(cfg); err != nil { + if err := resolveCustomEngineEnv(cfg, ResolveCustomEngineOptions{}); err != nil { t.Fatalf("resolveCustomEngineEnv: %v", err) } } @@ -122,11 +122,37 @@ func TestResolveCustomEngineEnv_BuiltinEngineSkipsResolution(t *testing.T) { }, }, } - if err := resolveCustomEngineEnv(cfg); err != nil { + if err := resolveCustomEngineEnv(cfg, ResolveCustomEngineOptions{}); err != nil { t.Fatalf("resolveCustomEngineEnv for built-in engine: %v", err) } } +func TestResolveCustomEngineConfigWithOptions_SkipsSupersededModelIdentity(t *testing.T) { + t.Setenv("MISSING_MODEL", "") + t.Setenv("MODEL_BASE_URL", "https://resolved.example.test") + cfg := &EvalConfig{ + Engine: EngineConfig{ + Name: "claude_code", + Model: ModelConfig{ + Provider: "anthropic", + Name: "${MISSING_MODEL:?must set}", + BaseURL: "${MODEL_BASE_URL}", + }, + }, + } + + err := ResolveCustomEngineConfigWithOptions(cfg, ResolveCustomEngineOptions{SkipModelIdentity: true}) + if err != nil { + t.Fatalf("explicit CLI model should bypass stale YAML model resolution: %v", err) + } + if cfg.Engine.Model.Name != "${MISSING_MODEL:?must set}" { + t.Fatalf("superseded YAML model was mutated: %q", cfg.Engine.Model.Name) + } + if got := cfg.Engine.Model.BaseURL; got != "https://resolved.example.test" { + t.Fatalf("active YAML base URL = %q, want resolved value", got) + } +} + func TestResolveCustomEngineConfig_ValidatesOverriddenEngine(t *testing.T) { // Simulates a --engine override turning a built-in engine (whose custom // block was skipped at load) into a custom one with an invalid config. diff --git a/internal/credential/README.md b/internal/credential/README.md index d26bdddf..cbfcb124 100644 --- a/internal/credential/README.md +++ b/internal/credential/README.md @@ -4,7 +4,7 @@ This document describes the **recommended approach and pipeline constraints**, focusing on: -- How the parameter dict eventually passed to agent initialization is decided +- How the resolved value passed to agent initialization is decided - How runner agent and judge agent configurations are differentiated - When a provider is configured, how environment variables override global configuration - How different agents consume these parameters @@ -20,18 +20,28 @@ Consolidate information scattered across the following sources into agent initia - The global credential configuration file - Process environment variables -Before entering `agent.DetectAgent(...)` / judge-agent initialization, produce a parameter dict **per agent kind**: +Before entering adapter construction, produce one resolved value **per agent role**: ```go -type AgentInitParams struct { - Provider string - Model string - APIKey string - BaseURL string +type ResolvedAgentConfig struct { + Role AgentRole + Engine string + Version string + Entry string + Provider string + Model string + APIKey string + BaseURL string + Kwargs map[string]string + ModelParams map[string]string } ``` -This dict is the "intermediate decision result" — it does not require every agent to consume all four fields verbatim. Each agent may use them selectively according to its own capabilities. +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. It +does not require every adapter to consume every field; capability validation +remains adapter-specific. ## Two Pipelines @@ -59,7 +69,7 @@ It is recommended to treat the judge agent as a separate parameter resolution pi ## Final Parameter Decisions -For each agent kind (`runner` / `judge`), compute the final values of: +For each agent role (`runner` / `judge`), compute the final values of: - `provider` - `model` @@ -111,47 +121,45 @@ Key points: ## Recommended Resolution Flow -A unified "resolve by role" entry point is recommended, e.g.: +A unified "resolve by role" flow is implemented through the runner and judge +entry points: ```go -type AgentKind string - -const ( - AgentKindRunner AgentKind = "runner" - AgentKindJudge AgentKind = "judge" -) - -func ResolveAgentInitParams( - kind AgentKind, - roleConfig AgentConfigSource, - fallback *AgentInitParams, +func ResolveRunnerConfig( + engine config.EngineConfig, resolver *Resolver, cli CLIOverrides, -) AgentInitParams +) ResolvedAgentConfig + +func ResolveJudgeConfig( + judge config.JudgeConfig, + runner ResolvedAgentConfig, + resolver *Resolver, +) ResolvedAgentConfig ``` Where: -- `roleConfig` is the role's own raw configuration -- `fallback` is only used when the judge agent reuses the runner agent's final result +- `engine` is the runner's complete raw engine configuration +- `runner` supplies the judge's inherited engine lifecycle and per-field fallback - `resolver` only provides provider-scoped credential lookup - `cli` only provides ad-hoc overrides Recommended execution order: -1. Determine the current role's final `provider` -2. Determine the role's own explicit `model` / `base_url` -3. If the provider is non-empty, uniformly read `${PROVIDER}_MODEL` / `${PROVIDER}_API_KEY` / `${PROVIDER}_BASE_URL` -4. If the `api-key` / `base-url` env var is missing, read from the resolver's global credential configuration -5. For the judge agent, fall back to the runner agent's final result for any missing fields -6. Apply CLI overrides last -7. Output the final `AgentInitParams` +1. Copy the role's YAML values and clone its kwargs/model params +2. Resolve a raw CLI `--model` once, including legacy slash disambiguation +3. For the judge agent, fill missing fields from the resolved runner config +4. If the final provider is non-empty, uniformly read `${PROVIDER}_MODEL` / `${PROVIDER}_API_KEY` / `${PROVIDER}_BASE_URL` +5. If the `api-key` / `base-url` env var is missing, read from the resolver's credential configuration +6. Apply the explicit CLI API key and preserve CLI model precedence +7. Apply compatibility normalization and output the final `ResolvedAgentConfig` Benefits: - Judge and runner share the same rules, only the input sources differ - Provider is decided first, avoiding cross-application of the wrong provider's env/config -- CLI overrides are applied last, making behavior the most direct +- CLI model identity is available before credential lookup while retaining the highest precedence ## Environment Variable Override Rules @@ -174,7 +182,7 @@ Rules: ## Agent Consumption Rules -The unified layer is responsible for producing `AgentInitParams`, but **how to use them is up to each agent's implementation**. +The unified layer is responsible for producing `ResolvedAgentConfig`, but **how to consume unsupported settings remains up to each adapter**. The historical non-Qoder `auto` normalization is centralized during resolution so the factory does not reinterpret raw configuration. ### claude-code @@ -299,13 +307,19 @@ These logs are particularly important for qodercli; otherwise users may mistaken ## Relation to the Current Implementation -The resolution pipeline described in this document is fully implemented in `agent_init.go`: +The resolution pipeline described in this document is implemented in `agent_init.go`: + +- `ResolveRunnerConfig()` — resolves the complete runner configuration from eval config and CLI overrides +- `ResolveJudgeConfig()` — resolves a judge role while inheriting the runner engine lifecycle and per-field fallbacks +- `resolveResolvedAgentConfig()` — shared resolution logic used by both pipelines -- `ResolveRunnerInitParams()` — resolves runner agent parameters from the eval config and CLI overrides -- `ResolveJudgeInitParams()` — resolves judge agent parameters, falling back to runner params when no independent judge config is set -- `resolveAgentInitParams()` — shared resolution logic used by both pipelines +The CLI no longer tentatively splits `--model` into `evalCfg` and later +collapses it. `ResolveRunnerConfig()` receives the raw flag and makes the slash +decision once, after provider configuration is available. The resulting value +is passed directly to `agent.DetectAgentWithResolvedConfig()` and is also used +for report engine/model identity. -`Resolver.Load()` emits "global discovery" logs (discovered providers from .env and config file). These are distinct from the per-role `[AGENT_CONFIG]` logs emitted by `logResolvedAgentConfig()` in `agent_init.go`, which describe the final resolved parameters and their sources for each agent kind. +`Resolver.Load()` emits "global discovery" logs (discovered providers from .env and config file). These are distinct from the per-role `[AGENT_CONFIG]` logs emitted by `logResolvedAgentConfig()` in `agent_init.go`, which describe the final resolved parameters and their sources for each agent role. ## Current Package Responsibilities diff --git a/internal/credential/agent_init.go b/internal/credential/agent_init.go index 27f2e260..407bb409 100644 --- a/internal/credential/agent_init.go +++ b/internal/credential/agent_init.go @@ -1,21 +1,24 @@ package credential import ( + "maps" "os" "strings" + "github.com/alibaba/skill-up/internal/agentkind" "github.com/alibaba/skill-up/internal/config" + "github.com/alibaba/skill-up/internal/customengine" "github.com/alibaba/skill-up/internal/logging" ) -// AgentKind identifies which evaluation agent a resolved config targets. -type AgentKind string +// AgentRole identifies which evaluation agent a resolved config targets. +type AgentRole string const ( - // AgentKindRunner is the primary agent that executes a case. - AgentKindRunner AgentKind = "runner" - // AgentKindJudge is the agent used by agent_judge evaluation. - AgentKindJudge AgentKind = "judge" + // AgentRoleRunner is the primary agent that executes a case. + AgentRoleRunner AgentRole = "runner" + // AgentRoleJudge is the agent used by agent_judge evaluation. + AgentRoleJudge AgentRole = "judge" ) // ValueSource records where a resolved config value came from. @@ -36,15 +39,22 @@ const ( ValueSourceCLI ValueSource = "cli" ) -// AgentInitParams is the resolved configuration passed into agent initialization. -type AgentInitParams struct { - Kind AgentKind - Engine string - - Provider string - Model string - APIKey string - BaseURL string +// ResolvedAgentConfig is the role-aware configuration passed into agent initialization. +// It is built once after YAML, CLI, environment, and credential-file inputs are +// available. Mutable data is cloned during construction so later mutations of +// EvalConfig cannot change an already resolved value. +type ResolvedAgentConfig struct { + Role AgentRole + Engine string + Version string + Entry string + + Provider string + Model string + APIKey string + BaseURL string + Kwargs map[string]string + ModelParams map[string]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. @@ -56,53 +66,66 @@ type AgentInitParams struct { BaseURLSource ValueSource } +// CLIOverrides contains explicit runner-only command-line overrides. +type CLIOverrides struct { + Model string + APIKey string +} + type agentResolveInput struct { - kind AgentKind - engine string + role AgentRole + engine config.EngineConfig provider string model string baseURL string valueSource ValueSource - fallback *AgentInitParams + fallback *ResolvedAgentConfig resolver *Resolver - cliModel string - cliAPIKey string - custom *config.CustomEngineConfig + cli CLIOverrides } -// ResolveRunnerInitParams resolves the final init params for the runner agent. -func ResolveRunnerInitParams(engine string, modelCfg config.ModelConfig, custom *config.CustomEngineConfig, resolver *Resolver, cliModel string, cliAPIKey string) AgentInitParams { - return resolveAgentInitParams(agentResolveInput{ - kind: AgentKindRunner, +// ResolveRunnerConfig resolves the final configuration for the runner agent. +func ResolveRunnerConfig(engine config.EngineConfig, resolver *Resolver, cli CLIOverrides) ResolvedAgentConfig { + return resolveResolvedAgentConfig(agentResolveInput{ + role: AgentRoleRunner, engine: engine, - provider: modelCfg.Provider, - model: modelCfg.Name, - baseURL: modelCfg.BaseURL, + provider: engine.Model.Provider, + model: engine.Model.Name, + baseURL: engine.Model.BaseURL, valueSource: ValueSourceConfig, resolver: resolver, - cliModel: cliModel, - cliAPIKey: cliAPIKey, - custom: custom, + cli: cli, }) } -// ResolveJudgeInitParams resolves the final init params for the judge agent. -func ResolveJudgeInitParams(engine string, judgeCfg config.JudgeConfig, runner AgentInitParams, resolver *Resolver) AgentInitParams { +// ResolveJudgeConfig resolves the final configuration for the judge agent. +// The judge inherits the runner engine lifecycle and kwargs until an explicit +// judge-engine schema is introduced, but its model/provider resolution is a +// separate role-aware pass. +func ResolveJudgeConfig(judgeCfg config.JudgeConfig, runner ResolvedAgentConfig, resolver *Resolver) ResolvedAgentConfig { provider, model := parseJudgeModel(judgeCfg.Model) - var fallback *AgentInitParams + var fallback *ResolvedAgentConfig if runner.Provider != "" || runner.Model != "" || runner.APIKey != "" || runner.BaseURL != "" { fallback = &runner } - return resolveAgentInitParams(agentResolveInput{ - kind: AgentKindJudge, - engine: engine, + return resolveResolvedAgentConfig(agentResolveInput{ + role: AgentRoleJudge, + engine: config.EngineConfig{ + Name: runner.Engine, + Version: runner.Version, + Entry: runner.Entry, + Kwargs: maps.Clone(runner.Kwargs), + Custom: runner.Custom, + Model: config.ModelConfig{ + Params: maps.Clone(runner.ModelParams), + }, + }, provider: provider, model: model, valueSource: ValueSourceJudge, fallback: fallback, resolver: resolver, - custom: runner.Custom, }) } @@ -115,21 +138,25 @@ func parseJudgeModel(value string) (provider, model string) { return "", value } -func resolveAgentInitParams(in agentResolveInput) AgentInitParams { +func resolveResolvedAgentConfig(in agentResolveInput) ResolvedAgentConfig { // A built-in engine ignores any engine.custom block, so it is not carried // into the init params — otherwise downstream logic (e.g. the model "auto" // strip) would mistake a built-in engine for a custom one. - custom := in.custom - if config.IsBuiltinEngineName(in.engine) { + custom := customengine.CloneConfig(in.engine.Custom) + if config.IsBuiltinEngineName(in.engine.Name) { custom = nil } - params := AgentInitParams{ - Kind: in.kind, - Engine: in.engine, - Provider: in.provider, - Model: in.model, - BaseURL: in.baseURL, - Custom: custom, + params := ResolvedAgentConfig{ + Role: in.role, + Engine: in.engine.Name, + Version: in.engine.Version, + Entry: in.engine.Entry, + Provider: in.provider, + Model: in.model, + BaseURL: in.baseURL, + Kwargs: maps.Clone(in.engine.Kwargs), + ModelParams: maps.Clone(in.engine.Model.Params), + Custom: custom, } if params.Provider != "" { params.ProviderSource = in.valueSource @@ -141,16 +168,18 @@ func resolveAgentInitParams(in agentResolveInput) AgentInitParams { params.BaseURLSource = in.valueSource } + applyCLIModelOverride(¶ms, in.cli.Model, in.resolver) applyFallback(¶ms, in.fallback) resolveProviderScopedFields(¶ms, in.resolver) applyFallbackCredentials(¶ms, in.fallback) - applyCLIOverrides(¶ms, in.cliModel, in.cliAPIKey) + applyCLIAPIKeyOverride(¶ms, in.cli.APIKey) + normalizeLegacyModel(¶ms) logResolvedAgentConfig(params) return params } -func applyFallback(params *AgentInitParams, fallback *AgentInitParams) { +func applyFallback(params *ResolvedAgentConfig, fallback *ResolvedAgentConfig) { if fallback == nil { return } @@ -168,7 +197,7 @@ func applyFallback(params *AgentInitParams, fallback *AgentInitParams) { } } -func applyFallbackCredentials(params *AgentInitParams, fallback *AgentInitParams) { +func applyFallbackCredentials(params *ResolvedAgentConfig, fallback *ResolvedAgentConfig) { if fallback == nil { return } @@ -178,11 +207,19 @@ func applyFallbackCredentials(params *AgentInitParams, fallback *AgentInitParams } } -func applyCLIOverrides(params *AgentInitParams, cliModel string, cliAPIKey string) { +func applyCLIModelOverride(params *ResolvedAgentConfig, cliModel string, resolver *Resolver) { if cliModel != "" { - params.Model = cliModel + params.Provider, params.Model = ResolveModelRef(cliModel, resolver) + if params.Provider != "" { + params.ProviderSource = ValueSourceCLI + } else { + params.ProviderSource = "" + } params.ModelSource = ValueSourceCLI } +} + +func applyCLIAPIKeyOverride(params *ResolvedAgentConfig, cliAPIKey string) { if cliAPIKey == "" { return } @@ -199,7 +236,19 @@ func applyCLIOverrides(params *AgentInitParams, cliModel string, cliAPIKey strin params.APIKeySource = ValueSourceCLI } -func resolveProviderScopedFields(params *AgentInitParams, resolver *Resolver) { +func normalizeLegacyModel(params *ResolvedAgentConfig) { + if params.Model != "auto" || params.Custom != nil { + return + } + switch params.Engine { + case agentkind.QoderCLI, agentkind.QoderAlias, agentkind.QoderCLIAlias: + return + default: + params.Model = "" + } +} + +func resolveProviderScopedFields(params *ResolvedAgentConfig, resolver *Resolver) { if params.Provider == "" { return } @@ -217,7 +266,12 @@ const ( valueBaseURL scopedValueKind = "BASE_URL" ) -func resolveValue(params *AgentInitParams, kind scopedValueKind, resolver *Resolver) { +func resolveValue(params *ResolvedAgentConfig, kind scopedValueKind, resolver *Resolver) { + // A CLI model is applied before provider lookup so its provider prefix can + // select credentials, but it must retain the historical highest precedence. + if kind == valueModel && params.ModelSource == ValueSourceCLI { + return + } if value, envVar, ok := lookupProviderEnv(params.Provider, kind); ok { setResolvedValue(params, kind, value, ValueSourceEnv) logProviderEnvResolution(params, kind, envVar) @@ -256,9 +310,9 @@ func lookupProviderEnv(provider string, kind scopedValueKind) (value, envVar str // Provider-existence and slashed-model disambiguation helpers live in // provider_query.go (Resolver.HasProvider, ResolveModelRef) so that -// agent_init.go stays focused on AgentInitParams construction. +// agent_init.go stays focused on ResolvedAgentConfig construction. -func setResolvedValue(params *AgentInitParams, kind scopedValueKind, value string, source ValueSource) { +func setResolvedValue(params *ResolvedAgentConfig, kind scopedValueKind, value string, source ValueSource) { switch kind { case valueModel: params.Model = value @@ -272,35 +326,38 @@ func setResolvedValue(params *AgentInitParams, kind scopedValueKind, value strin } } -func logProviderEnvResolution(params *AgentInitParams, kind scopedValueKind, envVar string) { +func logProviderEnvResolution(params *ResolvedAgentConfig, kind scopedValueKind, envVar string) { switch kind { case valueModel: logging.Debugf("AGENT_CONFIG kind=%s engine=%s provider=%s model_env=%s source.model=%s", - params.Kind, params.Engine, params.Provider, envVar, ValueSourceEnv) + params.Role, params.Engine, params.Provider, envVar, ValueSourceEnv) case valueAPIKey: - logging.Debugf("AGENT_CONFIG kind=%s engine=%s provider=%s auth_env=%s source.auth=%s", - params.Kind, params.Engine, params.Provider, envVar, ValueSourceEnv) + logging.Debugf("AGENT_CONFIG kind=%s engine=%s provider=%s auth_configured=true source.auth=%s", + params.Role, params.Engine, params.Provider, ValueSourceEnv) case valueBaseURL: logging.Debugf("AGENT_CONFIG kind=%s engine=%s provider=%s base_url_env=%s source.base_url=%s", - params.Kind, params.Engine, params.Provider, envVar, ValueSourceEnv) + params.Role, params.Engine, params.Provider, envVar, ValueSourceEnv) } } -func logResolvedAgentConfig(params AgentInitParams) { +func logResolvedAgentConfig(params ResolvedAgentConfig) { if params.Provider != "" { logging.Debugf("AGENT_CONFIG kind=%s engine=%s provider=%s source.provider=%s", - params.Kind, params.Engine, params.Provider, params.ProviderSource) + params.Role, params.Engine, params.Provider, params.ProviderSource) } if params.Model != "" { logging.Debugf("AGENT_CONFIG kind=%s engine=%s model=%s source.model=%s", - params.Kind, params.Engine, params.Model, params.ModelSource) + params.Role, params.Engine, params.Model, params.ModelSource) } if params.APIKey != "" { - logging.Debugf("AGENT_CONFIG kind=%s engine=%s api_key=%s source.api_key=%s", - params.Kind, params.Engine, MaskAPIKey(params.APIKey), params.APIKeySource) + // Do not log the credential, its masked form, or fields derived from its + // resolution path. The resolved config retains APIKeySource for callers + // that need programmatic diagnostics without placing it in log output. + logging.Debugf("AGENT_CONFIG kind=%s engine=%s auth_configured=true", + params.Role, params.Engine) } if params.BaseURL != "" { logging.Debugf("AGENT_CONFIG kind=%s engine=%s base_url=%s source.base_url=%s", - params.Kind, params.Engine, params.BaseURL, params.BaseURLSource) + params.Role, params.Engine, params.BaseURL, params.BaseURLSource) } } diff --git a/internal/credential/credential_test.go b/internal/credential/credential_test.go index 6ab5627c..158a7136 100644 --- a/internal/credential/credential_test.go +++ b/internal/credential/credential_test.go @@ -15,6 +15,21 @@ import ( var logCaptureMu sync.Mutex +func resolveRunnerConfigForTest( + engine string, + model config.ModelConfig, + custom *config.CustomEngineConfig, + resolver *Resolver, + cliModel string, + cliAPIKey string, +) ResolvedAgentConfig { + return ResolveRunnerConfig(config.EngineConfig{ + Name: engine, + Model: model, + Custom: custom, + }, resolver, CLIOverrides{Model: cliModel, APIKey: cliAPIKey}) +} + func TestMaskAPIKey(t *testing.T) { t.Parallel() @@ -210,7 +225,7 @@ func TestResolver_Load_DoesNotImportProcessEnv(t *testing.T) { } } -func TestResolveRunnerInitParams_PrefersProviderEnvOverResolver(t *testing.T) { +func TestResolveRunnerConfig_PrefersProviderEnvOverResolver(t *testing.T) { t.Setenv("OPENAI_MODEL", "gpt-5.5-env") t.Setenv("OPENAI_API_KEY", "sk-env-openai") t.Setenv("OPENAI_BASE_URL", "https://env.example.com/v1") @@ -222,13 +237,13 @@ func TestResolveRunnerInitParams_PrefersProviderEnvOverResolver(t *testing.T) { BaseURL: "https://file.example.com/v1", } - params := ResolveRunnerInitParams("codex", config.ModelConfig{ + params := resolveRunnerConfigForTest("codex", config.ModelConfig{ Provider: "openai", Name: "gpt-5.4", }, nil, r, "", "") - if params.Kind != AgentKindRunner { - t.Fatalf("Kind = %q, want %q", params.Kind, AgentKindRunner) + if params.Role != AgentRoleRunner { + t.Fatalf("Kind = %q, want %q", params.Role, AgentRoleRunner) } if params.Model != "gpt-5.5-env" { t.Fatalf("Model = %q, want provider env value", params.Model) @@ -244,12 +259,12 @@ func TestResolveRunnerInitParams_PrefersProviderEnvOverResolver(t *testing.T) { } } -func TestResolveRunnerInitParams_DoesNotScanProviderEnvWhenProviderMissing(t *testing.T) { +func TestResolveRunnerConfig_DoesNotScanProviderEnvWhenProviderMissing(t *testing.T) { t.Setenv("OPENAI_MODEL", "gpt-5.5-env") t.Setenv("OPENAI_API_KEY", "sk-env-openai") t.Setenv("OPENAI_BASE_URL", "https://env.example.com/v1") - params := ResolveRunnerInitParams("codex", config.ModelConfig{Name: "gpt-5.4"}, nil, nil, "", "") + params := resolveRunnerConfigForTest("codex", config.ModelConfig{Name: "gpt-5.4"}, nil, nil, "", "") if params.Provider != "" { t.Fatalf("Provider = %q, want empty", params.Provider) @@ -268,11 +283,11 @@ func TestResolveRunnerInitParams_DoesNotScanProviderEnvWhenProviderMissing(t *te } } -func TestResolveRunnerInitParams_PrefersCLIOverrides(t *testing.T) { +func TestResolveRunnerConfig_PrefersCLIOverrides(t *testing.T) { t.Setenv("OPENAI_MODEL", "gpt-5.5-env") t.Setenv("OPENAI_API_KEY", "sk-env-openai") - params := ResolveRunnerInitParams("codex", config.ModelConfig{ + params := resolveRunnerConfigForTest("codex", config.ModelConfig{ Provider: "openai", Name: "gpt-5.4", }, nil, nil, "gpt-5.6-cli", "sk-cli-openai") @@ -285,26 +300,85 @@ func TestResolveRunnerInitParams_PrefersCLIOverrides(t *testing.T) { } } -func TestResolveRunnerInitParams_LogsCLIAPIKeySource(t *testing.T) { +func TestResolveRunnerConfig_ResolvesCLIModelOnceWithoutMutatingInput(t *testing.T) { + t.Setenv("DASHSCOPE_API_KEY", "dashscope-env-key") + engine := config.EngineConfig{ + Name: "codex", + Version: "1.2.3", + Entry: "codex", + Model: config.ModelConfig{ + Provider: "anthropic", + Name: "yaml-model", + Params: map[string]string{"reasoning": "high"}, + }, + Kwargs: map[string]string{"bypass_sandbox": "true"}, + } + + resolved := ResolveRunnerConfig(engine, nil, CLIOverrides{ + Model: "dashscope/qwen3.6-plus", + }) + + if resolved.Provider != "dashscope" || resolved.Model != "qwen3.6-plus" { + t.Fatalf("resolved model = %q/%q, want dashscope/qwen3.6-plus", resolved.Provider, resolved.Model) + } + if resolved.ProviderSource != ValueSourceCLI || resolved.ModelSource != ValueSourceCLI { + t.Fatalf("CLI sources not retained: %#v", resolved) + } + if resolved.APIKey != "dashscope-env-key" || resolved.APIKeySource != ValueSourceEnv { + t.Fatalf("provider credentials were not resolved from final CLI provider: %#v", resolved) + } + if engine.Model.Provider != "anthropic" || engine.Model.Name != "yaml-model" { + t.Fatalf("input engine config was mutated: %#v", engine.Model) + } + engine.Kwargs["bypass_sandbox"] = "false" + engine.Model.Params["reasoning"] = "low" + if resolved.Kwargs["bypass_sandbox"] != "true" || resolved.ModelParams["reasoning"] != "high" { + t.Fatalf("resolved maps alias input config: kwargs=%v params=%v", resolved.Kwargs, resolved.ModelParams) + } +} + +func TestResolveRunnerConfig_PreservesOpaqueSlashedCLIModel(t *testing.T) { + for _, key := range []string{"ANTHROPIC_MODELSCOPE_API_KEY", "ANTHROPIC_MODELSCOPE_BASE_URL"} { + t.Setenv(key, "") + } + + resolved := ResolveRunnerConfig(config.EngineConfig{ + Name: "codex", + Model: config.ModelConfig{Provider: "openai", Name: "yaml-model"}, + }, nil, CLIOverrides{ //nolint:gosec // test credential, not a real secret + Model: "anthropic_modelscope/deepseek-v4-pro", + APIKey: "sk-cli-openai", + }) + + if resolved.Provider != "" || resolved.Model != "anthropic_modelscope/deepseek-v4-pro" { + t.Fatalf("opaque model was split: %#v", resolved) + } + if resolved.APIKey != "sk-cli-openai" || resolved.APIKeySource != ValueSourceCLI { + t.Fatalf("CLI key was not retained for provider-empty model: %#v", resolved) + } +} + +func TestResolveRunnerConfig_DoesNotLogCLIAPIKey(t *testing.T) { logging.SetVerbosity(1) defer logging.SetVerbosity(0) output := captureLogOutput(t, func() { - ResolveRunnerInitParams("codex", config.ModelConfig{ + resolveRunnerConfigForTest("codex", config.ModelConfig{ Provider: "openai", Name: "gpt-5.4", }, nil, nil, "", "sk-cli-openai") }) - if !strings.Contains(output, "source.api_key=cli") { - t.Fatalf("expected CLI api-key observability log, got %q", output) + if !strings.Contains(output, "auth_configured=true") { + t.Fatalf("expected credential-presence log, got %q", output) } - if !strings.Contains(output, "api_key=sk****ai") { - t.Fatalf("expected masked CLI api-key log, got %q", output) + if strings.Contains(output, "sk-cli-openai") || strings.Contains(output, "sk****ai") || + strings.Contains(output, "source.api_key") { + t.Fatalf("credential data or its resolution path leaked into log: %q", output) } } -func TestResolveRunnerInitParams_CLIAPIKeyAppliesWithoutProvider(t *testing.T) { +func TestResolveRunnerConfig_CLIAPIKeyAppliesWithoutProvider(t *testing.T) { // `--api-key K` must apply even when Provider is empty (e.g. user // passed `--model literal_opaque/id` and the prefix is not a // configured namespace, so ResolveModelRef returned Provider=""). @@ -314,7 +388,7 @@ func TestResolveRunnerInitParams_CLIAPIKeyAppliesWithoutProvider(t *testing.T) { // with a provider_required_for_cli_override warning — that guard was // defensive paranoia, not a structural requirement, and it broke // literal-opaque-id flows. - params := ResolveRunnerInitParams("codex", config.ModelConfig{ + params := resolveRunnerConfigForTest("codex", config.ModelConfig{ Name: "gpt-5.4", }, nil, nil, "", "sk-cli-openai") @@ -326,11 +400,11 @@ func TestResolveRunnerInitParams_CLIAPIKeyAppliesWithoutProvider(t *testing.T) { } } -func TestResolveRunnerInitParams_CustomEngineCLIAPIKeyApplies(t *testing.T) { +func TestResolveRunnerConfig_CustomEngineCLIAPIKeyApplies(t *testing.T) { // A custom engine references the CLI key explicitly via ${api_key} and // has no model provider. The key must still be applied (it reaches the // agent through engine.custom.env / ${api_key}). - params := ResolveRunnerInitParams("my-agent", config.ModelConfig{}, &config.CustomEngineConfig{ + params := resolveRunnerConfigForTest("my-agent", config.ModelConfig{}, &config.CustomEngineConfig{ Transport: "local", Local: &config.CustomLocalConfig{Command: "/opt/agent"}, }, nil, "", "sk-cli-custom") @@ -339,10 +413,80 @@ func TestResolveRunnerInitParams_CustomEngineCLIAPIKeyApplies(t *testing.T) { t.Fatalf("APIKey = %q, want sk-cli-custom for a custom engine", params.APIKey) } if params.Custom == nil { - t.Fatal("Custom config was not threaded into AgentInitParams") + t.Fatal("Custom config was not threaded into ResolvedAgentConfig") } } +func TestResolveRunnerConfig_DoesNotAliasCustomConfig(t *testing.T) { + const ( + sourceValue = "source" + mutatedValue = "mutated" + ) + required := true + source := &config.CustomEngineConfig{ + Transport: "http", + Env: map[string]string{"PROFILE": sourceValue}, + Kwargs: map[string]string{"region": sourceValue}, + Local: &config.CustomLocalConfig{ + Command: "agent", + Args: []string{"--source"}, + }, + HTTP: &config.CustomHTTPConfig{ + URL: "https://source.example.test", + Headers: map[string]string{"X-Profile": sourceValue}, + Files: []config.CustomHTTPFile{{ + Path: "source.txt", + Required: &required, + }}, + RequestBody: map[string]any{ + "items": []any{map[string]any{"profile": sourceValue}}, + }, + }, + } + resolved := ResolveRunnerConfig(config.EngineConfig{ + Name: "my-agent", + Custom: source, + }, nil, CLIOverrides{}) + + source.Env["PROFILE"] = mutatedValue + source.Kwargs["region"] = mutatedValue + source.Local.Args[0] = "--mutated" + source.HTTP.Headers["X-Profile"] = mutatedValue + source.HTTP.Files[0].Path = "mutated.txt" + *source.HTTP.Files[0].Required = false + requestBodyProfile(t, source.HTTP.RequestBody)["profile"] = mutatedValue + + if resolved.Custom == source || resolved.Custom.Local == source.Local || resolved.Custom.HTTP == source.HTTP { + t.Fatal("resolved custom config retains source pointers") + } + if resolved.Custom.Env["PROFILE"] != sourceValue || resolved.Custom.Kwargs["region"] != sourceValue || + resolved.Custom.Local.Args[0] != "--source" || resolved.Custom.HTTP.Headers["X-Profile"] != sourceValue || + resolved.Custom.HTTP.Files[0].Path != "source.txt" || !*resolved.Custom.HTTP.Files[0].Required { + t.Fatalf("resolved custom config changed after source mutation: %#v", resolved.Custom) + } + if got := requestBodyProfile(t, resolved.Custom.HTTP.RequestBody)["profile"]; got != sourceValue { + t.Fatalf("resolved request body profile = %v, want %s", got, sourceValue) + } +} + +func requestBodyProfile(t *testing.T, body any) map[string]any { + t.Helper() + + bodyMap, ok := body.(map[string]any) + if !ok { + t.Fatalf("request body = %T, want map[string]any", body) + } + items, ok := bodyMap["items"].([]any) + if !ok || len(items) == 0 { + t.Fatalf("request body items = %#v, want non-empty []any", bodyMap["items"]) + } + profile, ok := items[0].(map[string]any) + if !ok { + t.Fatalf("request body item = %T, want map[string]any", items[0]) + } + return profile +} + func captureLogOutput(t *testing.T, fn func()) string { t.Helper() @@ -358,14 +502,14 @@ func captureLogOutput(t *testing.T, fn func()) string { return buf.String() } -func TestResolveJudgeInitParams_FallsBackToRunnerWhenJudgeModelEmpty(t *testing.T) { +func TestResolveJudgeConfig_FallsBackToRunnerWhenJudgeModelEmpty(t *testing.T) { // Clear env vars that would override runner fallback. for _, key := range []string{"ANTHROPIC_API_KEY", "ANTHROPIC_BASE_URL", "ANTHROPIC_MODEL"} { t.Setenv(key, "") } - runner := AgentInitParams{ - Kind: AgentKindRunner, + runner := ResolvedAgentConfig{ + Role: AgentRoleRunner, Engine: "claude-code", Provider: "anthropic", Model: "claude-sonnet-4-6", @@ -377,10 +521,10 @@ func TestResolveJudgeInitParams_FallsBackToRunnerWhenJudgeModelEmpty(t *testing. BaseURLSource: ValueSourceResolver, } - params := ResolveJudgeInitParams("claude-code", config.JudgeConfig{}, runner, nil) + params := ResolveJudgeConfig(config.JudgeConfig{}, runner, nil) - if params.Kind != AgentKindJudge { - t.Fatalf("Kind = %q, want %q", params.Kind, AgentKindJudge) + if params.Role != AgentRoleJudge { + t.Fatalf("Kind = %q, want %q", params.Role, AgentRoleJudge) } if params.Provider != runner.Provider || params.Model != runner.Model || params.APIKey != runner.APIKey || params.BaseURL != runner.BaseURL { t.Fatalf("judge params = %#v, want runner values", params) @@ -390,14 +534,73 @@ func TestResolveJudgeInitParams_FallsBackToRunnerWhenJudgeModelEmpty(t *testing. } } -func TestResolveJudgeInitParams_FallsBackToRunnerBaseURLBeforeCredentialFallback(t *testing.T) { +func TestResolveJudgeConfig_InheritsRunnerLifecycleWithoutAliasingKwargs(t *testing.T) { + for _, key := range []string{"ANTHROPIC_MODEL", "ANTHROPIC_API_KEY", "ANTHROPIC_BASE_URL"} { + t.Setenv(key, "") + } + runner := ResolvedAgentConfig{ + Role: AgentRoleRunner, + Engine: "codex", + Version: "0.42.0", + Entry: "codex", + Provider: "openai", + Model: "gpt-5.4", + Kwargs: map[string]string{"bypass_sandbox": "true"}, + ModelParams: map[string]string{"reasoning": "high"}, + } + + resolved := ResolveJudgeConfig(config.JudgeConfig{ + Type: "agent_judge", + Model: "anthropic/claude-sonnet-4-6", + }, runner, nil) + + if resolved.Role != AgentRoleJudge || resolved.Engine != runner.Engine || resolved.Version != runner.Version || resolved.Entry != runner.Entry { + t.Fatalf("judge lifecycle config = %#v, want runner engine lifecycle", resolved) + } + if resolved.Provider != "anthropic" || resolved.Model != "claude-sonnet-4-6" { + t.Fatalf("judge role model was not independently resolved: %#v", resolved) + } + runner.Kwargs["bypass_sandbox"] = "false" + runner.ModelParams["reasoning"] = "low" + if resolved.Kwargs["bypass_sandbox"] != "true" || resolved.ModelParams["reasoning"] != "high" { + t.Fatalf("judge config aliases runner maps: kwargs=%v params=%v", resolved.Kwargs, resolved.ModelParams) + } +} + +func TestResolveJudgeConfig_DoesNotAliasRunnerCustomConfig(t *testing.T) { + runner := ResolvedAgentConfig{ + Role: AgentRoleRunner, + Engine: "my-agent", + Custom: &config.CustomEngineConfig{ + Transport: "local", + Env: map[string]string{"PROFILE": "runner"}, + Local: &config.CustomLocalConfig{ + Command: "agent", + Args: []string{"--runner"}, + }, + }, + } + + resolved := ResolveJudgeConfig(config.JudgeConfig{Type: "agent_judge"}, runner, nil) + runner.Custom.Env["PROFILE"] = "mutated" + runner.Custom.Local.Args[0] = "--mutated" + + if resolved.Custom == runner.Custom || resolved.Custom.Local == runner.Custom.Local { + t.Fatal("judge custom config retains runner pointers") + } + if resolved.Custom.Env["PROFILE"] != "runner" || resolved.Custom.Local.Args[0] != "--runner" { + t.Fatalf("judge custom config changed after runner mutation: %#v", resolved.Custom) + } +} + +func TestResolveJudgeConfig_FallsBackToRunnerBaseURLBeforeCredentialFallback(t *testing.T) { // Clear env vars that would override runner fallback. for _, key := range []string{"ANTHROPIC_API_KEY", "ANTHROPIC_BASE_URL", "ANTHROPIC_MODEL"} { t.Setenv(key, "") } - runner := AgentInitParams{ - Kind: AgentKindRunner, + runner := ResolvedAgentConfig{ + Role: AgentRoleRunner, Engine: "claude-code", Provider: "anthropic", Model: "claude-sonnet-4-6", @@ -405,7 +608,7 @@ func TestResolveJudgeInitParams_FallsBackToRunnerBaseURLBeforeCredentialFallback BaseURLSource: ValueSourceResolver, } - params := ResolveJudgeInitParams("claude-code", config.JudgeConfig{}, runner, nil) + params := ResolveJudgeConfig(config.JudgeConfig{}, runner, nil) if params.BaseURL != "https://runner.example.com" { t.Fatalf("BaseURL = %q, want runner base URL", params.BaseURL) @@ -415,7 +618,7 @@ func TestResolveJudgeInitParams_FallsBackToRunnerBaseURLBeforeCredentialFallback } } -func TestResolveJudgeInitParams_ParsesIndependentJudgeModel(t *testing.T) { +func TestResolveJudgeConfig_ParsesIndependentJudgeModel(t *testing.T) { const provider = "judgeprovider" r := NewResolver("") @@ -425,11 +628,11 @@ func TestResolveJudgeInitParams_ParsesIndependentJudgeModel(t *testing.T) { BaseURL: "https://judge.example.com/v1", } - params := ResolveJudgeInitParams("codex", config.JudgeConfig{ + params := ResolveJudgeConfig(config.JudgeConfig{ Type: "agent_judge", Model: provider + "/gpt-5.4", - }, AgentInitParams{ - Kind: AgentKindRunner, + }, ResolvedAgentConfig{ + Role: AgentRoleRunner, Engine: "codex", Provider: "anthropic", Model: "claude-sonnet-4-6", @@ -446,14 +649,14 @@ func TestResolveJudgeInitParams_ParsesIndependentJudgeModel(t *testing.T) { } } -func TestResolveJudgeInitParams_PrefersProviderScopedModelEnv(t *testing.T) { +func TestResolveJudgeConfig_PrefersProviderScopedModelEnv(t *testing.T) { t.Setenv("JUDGEPROVIDER_MODEL", "gpt-5.5-judge-env") - params := ResolveJudgeInitParams("codex", config.JudgeConfig{ + params := ResolveJudgeConfig(config.JudgeConfig{ Type: "agent_judge", Model: "judgeprovider/gpt-5.4", - }, AgentInitParams{ - Kind: AgentKindRunner, + }, ResolvedAgentConfig{ + Role: AgentRoleRunner, Engine: "codex", Provider: "anthropic", Model: "claude-sonnet-4-6", @@ -467,12 +670,12 @@ func TestResolveJudgeInitParams_PrefersProviderScopedModelEnv(t *testing.T) { } } -func TestResolveRunnerInitParams_UsesGenericProviderScopedEnv(t *testing.T) { +func TestResolveRunnerConfig_UsesGenericProviderScopedEnv(t *testing.T) { t.Setenv("DASHSCOPE_MODEL", "qwen-max-env") t.Setenv("DASHSCOPE_API_KEY", "dashscope-env-key") t.Setenv("DASHSCOPE_BASE_URL", "https://dashscope.example.com") - params := ResolveRunnerInitParams("custom", config.ModelConfig{ + params := resolveRunnerConfigForTest("custom", config.ModelConfig{ Provider: "dashscope", Name: "qwen-max", }, nil, nil, "", "") @@ -488,25 +691,35 @@ func TestResolveRunnerInitParams_UsesGenericProviderScopedEnv(t *testing.T) { } } -func TestResolveRunnerInitParams_DropsCustomForBuiltinEngine(t *testing.T) { +func TestResolveRunnerConfig_DropsCustomForBuiltinEngine(t *testing.T) { custom := &config.CustomEngineConfig{ Transport: "local", Local: &config.CustomLocalConfig{Command: "/opt/agent"}, } - params := ResolveRunnerInitParams("codex", config.ModelConfig{Name: "auto"}, custom, nil, "", "") + params := resolveRunnerConfigForTest("codex", config.ModelConfig{Name: "auto"}, custom, nil, "", "") // A built-in engine ignores engine.custom; it must not leak into params. if params.Custom != nil { t.Fatalf("Custom = %#v, want nil for a built-in engine", params.Custom) } + if params.Model != "" { + t.Fatalf("Model = %q, want legacy non-Qoder auto normalization", params.Model) + } +} + +func TestResolveRunnerConfig_PreservesAutoForQoderCLI(t *testing.T) { + params := resolveRunnerConfigForTest("qodercli", config.ModelConfig{Name: "auto"}, nil, nil, "", "") + if params.Model != "auto" { + t.Fatalf("Model = %q, want auto for qodercli", params.Model) + } } -func TestResolveRunnerInitParams_KeepsCustomForCustomEngine(t *testing.T) { +func TestResolveRunnerConfig_KeepsCustomForCustomEngine(t *testing.T) { custom := &config.CustomEngineConfig{ Transport: "local", Local: &config.CustomLocalConfig{Command: "/opt/agent"}, } - params := ResolveRunnerInitParams("my-agent", config.ModelConfig{}, custom, nil, "", "") + params := resolveRunnerConfigForTest("my-agent", config.ModelConfig{}, custom, nil, "", "") if params.Custom == nil { t.Fatal("Custom = nil, want it preserved for a custom engine") diff --git a/internal/customengine/config.go b/internal/customengine/config.go index b653ff7d..45855145 100644 --- a/internal/customengine/config.go +++ b/internal/customengine/config.go @@ -2,6 +2,11 @@ // primitives shared by the config loader and custom agent transports. package customengine +import ( + "maps" + "slices" +) + // Config describes a user-defined agent engine that is not a // built-in agent. It is read only when engine.name does not match a built-in // agent. See docs/design/custom-engine.md for the full contract. @@ -44,3 +49,61 @@ type HTTPFile struct { Path string `yaml:"path"` Required *bool `yaml:"required,omitempty"` // defaults to true } + +// CloneConfig returns a deep copy of cfg. RequestBody is cloned according to +// the map, sequence, and scalar shapes produced by YAML decoding. +func CloneConfig(cfg *Config) *Config { + if cfg == nil { + return nil + } + + cloned := *cfg + cloned.Env = maps.Clone(cfg.Env) + cloned.Kwargs = maps.Clone(cfg.Kwargs) + + if cfg.Local != nil { + local := *cfg.Local + local.Args = slices.Clone(cfg.Local.Args) + cloned.Local = &local + } + if cfg.HTTP != nil { + http := *cfg.HTTP + http.Headers = maps.Clone(cfg.HTTP.Headers) + http.Files = slices.Clone(cfg.HTTP.Files) + for i := range http.Files { + if cfg.HTTP.Files[i].Required != nil { + required := *cfg.HTTP.Files[i].Required + http.Files[i].Required = &required + } + } + http.RequestBody = cloneYAMLValue(cfg.HTTP.RequestBody) + cloned.HTTP = &http + } + + return &cloned +} + +func cloneYAMLValue(value any) any { + switch typed := value.(type) { + case map[string]any: + cloned := make(map[string]any, len(typed)) + for key, item := range typed { + cloned[key] = cloneYAMLValue(item) + } + return cloned + case map[any]any: + cloned := make(map[any]any, len(typed)) + for key, item := range typed { + cloned[key] = cloneYAMLValue(item) + } + return cloned + case []any: + cloned := make([]any, len(typed)) + for i, item := range typed { + cloned[i] = cloneYAMLValue(item) + } + return cloned + default: + return value + } +} diff --git a/internal/evaluator/evaluator.go b/internal/evaluator/evaluator.go index 354ea17b..6dd321d8 100644 --- a/internal/evaluator/evaluator.go +++ b/internal/evaluator/evaluator.go @@ -33,8 +33,8 @@ import ( ) var ( - sleepWithContext = sleepContext - agentDetectWithInitParams = agent.DetectAgentWithInitParams + sleepWithContext = sleepContext + agentDetectWithResolvedConfig = agent.DetectAgentWithResolvedConfig ) const judgeTypeAgentJudge = "agent_judge" @@ -58,7 +58,7 @@ type EvalOptions struct { Loader *config.Loader Resolver *credential.Resolver Agent agent.Agent - RunnerParams credential.AgentInitParams + RunnerConfig credential.ResolvedAgentConfig EvalCfg *config.EvalConfig Observer ProgressObserver @@ -142,7 +142,7 @@ type defaultEvaluator struct { loader *config.Loader resolver *credential.Resolver ag agent.Agent - runnerParams credential.AgentInitParams + runnerConfig credential.ResolvedAgentConfig fixtures *fixtureRegistry deleteWorkspace bool observer ProgressObserver @@ -174,7 +174,7 @@ func NewEvaluator(opts EvalOptions) Evaluator { loader: opts.Loader, resolver: opts.Resolver, ag: opts.Agent, - runnerParams: opts.RunnerParams, + runnerConfig: opts.RunnerConfig, fixtures: newFixtureRegistry(), deleteWorkspace: opts.DeleteWorkspace, observer: opts.Observer, @@ -825,9 +825,9 @@ func (e *defaultEvaluator) resolveJudgeAgent(ctx context.Context, judgeCfg confi judgeAgent := runAgent if e.evalCfg.Engine.Name != "" { - judgeParams := credential.ResolveJudgeInitParams(e.evalCfg.Engine.Name, judgeCfg, e.runnerParams, e.resolver) + resolvedJudge := credential.ResolveJudgeConfig(judgeCfg, e.runnerConfig, e.resolver) var err error - judgeAgent, err = agentDetectWithInitParams(e.evalCfg.Engine.Name, judgeParams, e.evalCfg.Engine.Kwargs) + judgeAgent, err = agentDetectWithResolvedConfig(resolvedJudge) if err != nil { return nil, fmt.Errorf("failed to create judge agent: %w", err) } diff --git a/internal/evaluator/evaluator_test.go b/internal/evaluator/evaluator_test.go index 75413776..d2995f55 100644 --- a/internal/evaluator/evaluator_test.go +++ b/internal/evaluator/evaluator_test.go @@ -1102,11 +1102,11 @@ func TestExecuteCase_AgentTimeoutDoesNotInvokeAgentJudge(t *testing.T) { }, }) - origDetect := agentDetectWithInitParams - agentDetectWithInitParams = func(_ string, _ credential.AgentInitParams, _ map[string]string) (agent.Agent, error) { + origDetect := agentDetectWithResolvedConfig + agentDetectWithResolvedConfig = func(_ credential.ResolvedAgentConfig) (agent.Agent, error) { return judgeAgent, nil } - defer func() { agentDetectWithInitParams = origDetect }() + defer func() { agentDetectWithResolvedConfig = origDetect }() caseCfg := &config.CaseConfig{ ID: "case-timeout-no-judge-salvage", @@ -1146,11 +1146,11 @@ func TestExecuteCase_InstallsJudgeSkillsOnJudgeAgentOnly(t *testing.T) { } runAgent := &mockAgent{name: "run", output: "main response"} - origDetect := agentDetectWithInitParams - agentDetectWithInitParams = func(_ string, _ credential.AgentInitParams, _ map[string]string) (agent.Agent, error) { + origDetect := agentDetectWithResolvedConfig + agentDetectWithResolvedConfig = func(_ credential.ResolvedAgentConfig) (agent.Agent, error) { return judgeAgent, nil } - defer func() { agentDetectWithInitParams = origDetect }() + defer func() { agentDetectWithResolvedConfig = origDetect }() e := newTestEvaluator(EvalOptions{ SkillDir: skillDir, @@ -1222,11 +1222,11 @@ func TestExecuteCase_JudgeSkillInstallFailureReturnsError(t *testing.T) { } runAgent := &mockAgent{name: "run", output: "main response"} - origDetect := agentDetectWithInitParams - agentDetectWithInitParams = func(_ string, _ credential.AgentInitParams, _ map[string]string) (agent.Agent, error) { + origDetect := agentDetectWithResolvedConfig + agentDetectWithResolvedConfig = func(_ credential.ResolvedAgentConfig) (agent.Agent, error) { return judgeAgent, nil } - defer func() { agentDetectWithInitParams = origDetect }() + defer func() { agentDetectWithResolvedConfig = origDetect }() e := newTestEvaluator(EvalOptions{ SkillDir: t.TempDir(), diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 2c87fa1c..031d3f58 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -52,19 +52,19 @@ type Runner struct { loader *config.Loader resolver *credential.Resolver - runnerParams credential.AgentInitParams + runnerConfig credential.ResolvedAgentConfig workspace *report.IterationWorkspace now func() time.Time } // NewRunner creates a new Runner with the given eval config and loader. -func NewRunner(evalCfg *config.EvalConfig, loader *config.Loader, resolver *credential.Resolver, runnerParams credential.AgentInitParams) *Runner { +func NewRunner(evalCfg *config.EvalConfig, loader *config.Loader, resolver *credential.Resolver, runnerConfig credential.ResolvedAgentConfig) *Runner { return &Runner{ evalCfg: evalCfg, loader: loader, resolver: resolver, - runnerParams: runnerParams, + runnerConfig: runnerConfig, now: time.Now, } } @@ -159,7 +159,7 @@ func (r *Runner) Evaluate(ctx context.Context, cases []*config.CaseConfig, ag ag Loader: r.loader, Resolver: r.resolver, Agent: ag, - RunnerParams: r.runnerParams, + RunnerConfig: r.runnerConfig, OutputDir: r.workspace.IterationDir(), Concurrency: concurrency, DeleteWorkspace: opts.DeleteWorkspace, @@ -301,7 +301,7 @@ func (r *Runner) WriteResults(ctx context.Context, results []evaluator.EvalResul return fmt.Errorf("failed to write benchmark.md: %w", err) } - input := buildReportInput(skillName, grouped, caseIDs, startTime, endTime, r.evalCfg) + input := buildReportInput(skillName, grouped, caseIDs, startTime, endTime, r.evalCfg, r.runnerConfig) // result.json is always written as the raw evaluation data source. resultJSON, err := json.MarshalIndent(input, "", " ") @@ -491,15 +491,29 @@ func resultToBenchmarkRun(res *evaluator.EvalResult, runNumber int) report.Bench } } -func buildReportInput(skillName string, grouped map[string]*caseResults, caseIDs []string, startTime, endTime time.Time, evalCfg *config.EvalConfig) report.Input { - modelName := evalCfg.Engine.Model.Name - if evalCfg.Engine.Model.Provider != "" && modelName != "" { - modelName = evalCfg.Engine.Model.Provider + "/" + modelName +func buildReportInput( + skillName string, + grouped map[string]*caseResults, + caseIDs []string, + startTime, endTime time.Time, + evalCfg *config.EvalConfig, + resolved credential.ResolvedAgentConfig, +) report.Input { + engineName := resolved.Engine + provider := resolved.Provider + modelName := resolved.Model + if engineName == "" { + engineName = evalCfg.Engine.Name + provider = evalCfg.Engine.Model.Provider + modelName = evalCfg.Engine.Model.Name + } + if provider != "" && modelName != "" { + modelName = provider + "/" + modelName } input := report.Input{ SkillName: skillName, SchemaVersion: evalCfg.SchemaVersion, - EngineName: evalCfg.Engine.Name, + EngineName: engineName, ModelName: modelName, StartTime: startTime, EndTime: endTime, diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 2f7219a5..10f7f8f2 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -21,14 +21,14 @@ func TestNewRunner(t *testing.T) { evalCfg := &config.EvalConfig{ Judge: config.JudgeConfig{Type: "rule_based"}, } - r := NewRunner(evalCfg, nil, nil, credential.AgentInitParams{}) + r := NewRunner(evalCfg, nil, nil, credential.ResolvedAgentConfig{}) if r == nil { t.Fatal("expected non-nil runner") } } func TestRunner_InitWorkspace(t *testing.T) { - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() err := r.InitWorkspace(tmpDir, "test-skill", 1) @@ -47,7 +47,7 @@ func TestRunner_InitWorkspace(t *testing.T) { } func TestRunner_WriteResults_WithSkillOnly(t *testing.T) { - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() err := r.InitWorkspace(tmpDir, "test-skill", 1) @@ -132,7 +132,7 @@ func TestRunner_WriteResults_WithSkillOnly(t *testing.T) { } func TestRunner_WriteResults_WithBaseline(t *testing.T) { - r := NewRunner(&config.EvalConfig{Benchmark: config.BenchmarkConfig{Enabled: true}}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{Benchmark: config.BenchmarkConfig{Enabled: true}}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() err := r.InitWorkspace(tmpDir, "test-skill", 1) @@ -199,7 +199,7 @@ func TestRunner_WriteResults_WithBaseline(t *testing.T) { } func TestRunner_WriteResults_UsesStderrWhenFinalMessageEmpty(t *testing.T) { - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() err := r.InitWorkspace(tmpDir, "test-skill", 1) @@ -258,7 +258,7 @@ func TestRunner_WriteResults_UsesStderrWhenFinalMessageEmpty(t *testing.T) { } func TestRunner_WriteResults_DoesNotUseStderrOnSuccessfulEmptyResponse(t *testing.T) { - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() err := r.InitWorkspace(tmpDir, "test-skill", 1) @@ -348,7 +348,7 @@ func TestBuildReportInput_EngineAndModelPopulated(t *testing.T) { }, } - input := buildReportInput("my-skill", grouped, []string{"case-1"}, now, end, evalCfg) + input := buildReportInput("my-skill", grouped, []string{"case-1"}, now, end, evalCfg, credential.ResolvedAgentConfig{}) if input.SchemaVersion != "v1alpha1" { t.Errorf("SchemaVersion = %q, want %q", input.SchemaVersion, "v1alpha1") @@ -419,7 +419,7 @@ func TestBuildReportInput_ModelNameVariants(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { grouped := map[string]*caseResults{} - input := buildReportInput("s", grouped, nil, time.Time{}, time.Time{}, tt.evalCfg) + input := buildReportInput("s", grouped, nil, time.Time{}, time.Time{}, tt.evalCfg, credential.ResolvedAgentConfig{}) if input.ModelName != tt.wantName { t.Errorf("ModelName = %q, want %q", input.ModelName, tt.wantName) } @@ -427,6 +427,25 @@ func TestBuildReportInput_ModelNameVariants(t *testing.T) { } } +func TestBuildReportInput_UsesResolvedRunnerIdentity(t *testing.T) { + evalCfg := &config.EvalConfig{ + Engine: config.EngineConfig{ + Name: "claude_code", + Model: config.ModelConfig{Provider: "anthropic", Name: "yaml-model"}, + }, + } + resolved := credential.ResolvedAgentConfig{ + Engine: "codex", + Provider: "dashscope", + Model: "qwen3.6-plus", + } + + input := buildReportInput("s", map[string]*caseResults{}, nil, time.Time{}, time.Time{}, evalCfg, resolved) + if input.EngineName != "codex" || input.ModelName != "dashscope/qwen3.6-plus" { + t.Fatalf("report identity = %s/%s, want resolved codex/dashscope/qwen3.6-plus", input.EngineName, input.ModelName) + } +} + func TestBuildReportInput_TokenAccumulation(t *testing.T) { evalCfg := &config.EvalConfig{Engine: config.EngineConfig{Name: "test"}} grouped := map[string]*caseResults{ @@ -454,7 +473,7 @@ func TestBuildReportInput_TokenAccumulation(t *testing.T) { }, } - input := buildReportInput("s", grouped, []string{"c1", "c2"}, time.Time{}, time.Time{}, evalCfg) + input := buildReportInput("s", grouped, []string{"c1", "c2"}, time.Time{}, time.Time{}, evalCfg, credential.ResolvedAgentConfig{}) // c1: (100+50) + (80+40) = 270, c2: (200+100) = 300, total = 570 if input.TotalTokens != 570 { @@ -493,7 +512,7 @@ func TestBuildReportInput_IncludesFailedJudgeSessionMetrics(t *testing.T) { }, } - input := buildReportInput("s", grouped, []string{"judge-error"}, time.Time{}, time.Time{}, evalCfg) + input := buildReportInput("s", grouped, []string{"judge-error"}, time.Time{}, time.Time{}, evalCfg, credential.ResolvedAgentConfig{}) if len(input.CaseResults) != 1 { t.Fatalf("CaseResults count = %d, want 1", len(input.CaseResults)) @@ -514,7 +533,7 @@ func TestBuildReportInput_IncludesFailedJudgeSessionMetrics(t *testing.T) { } func TestRunner_WriteResults_NoWorkspace(t *testing.T) { - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) err := r.WriteResults(context.Background(), nil, "", "", 1, nil, time.Time{}, time.Time{}) if err == nil { t.Error("expected error when workspace not initialized") diff --git a/internal/runner/workspace_options_test.go b/internal/runner/workspace_options_test.go index a24fa783..a3a09088 100644 --- a/internal/runner/workspace_options_test.go +++ b/internal/runner/workspace_options_test.go @@ -26,7 +26,7 @@ import ( func TestRunner_InitWorkspace_UsesExplicitRunNumber(t *testing.T) { t.Parallel() - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() if err := r.InitWorkspace(tmpDir, "test-skill", 99); err != nil { @@ -44,7 +44,7 @@ func TestRunner_InitWorkspace_UsesExplicitRunNumber(t *testing.T) { func TestRunner_InitWorkspace_DefaultRunKeepsOtherIterations(t *testing.T) { t.Parallel() - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() if err := os.MkdirAll(filepath.Join(tmpDir, "iteration-2"), 0o755); err != nil { @@ -66,7 +66,7 @@ func TestRunner_InitWorkspace_DefaultRunKeepsOtherIterations(t *testing.T) { func TestRunner_InitWorkspace_ExplicitRunCleansOnlyRequestedIteration(t *testing.T) { t.Parallel() - r := NewRunner(&config.EvalConfig{}, nil, nil, credential.AgentInitParams{}) + r := NewRunner(&config.EvalConfig{}, nil, nil, credential.ResolvedAgentConfig{}) tmpDir := t.TempDir() staleFile := filepath.Join(tmpDir, "iteration-99", "stale.txt") @@ -105,7 +105,7 @@ func TestRunner_Evaluate_DefaultsZeroIterationToOne(t *testing.T) { r := NewRunner(&config.EvalConfig{ Environment: config.Environment{Type: "none"}, Cases: config.CasesConfig{Parallelism: 1}, - }, loader, nil, credential.AgentInitParams{}) + }, loader, nil, credential.ResolvedAgentConfig{}) ag := &runnerTestAgent{} results, err := r.Evaluate(context.Background(), []*config.CaseConfig{{ID: "case-1", Title: "Case 1"}}, ag, EvaluateOptions{ @@ -138,7 +138,7 @@ func TestRunner_Evaluate_DefaultWorkspaceIsSiblingOfSkillDir(t *testing.T) { r := NewRunner(&config.EvalConfig{ Environment: config.Environment{Type: "none"}, Cases: config.CasesConfig{Parallelism: 1}, - }, loader, nil, credential.AgentInitParams{}) + }, loader, nil, credential.ResolvedAgentConfig{}) if _, err := r.Evaluate(context.Background(), []*config.CaseConfig{{ID: "case-1", Title: "Case 1"}}, &runnerTestAgent{}, EvaluateOptions{}); err != nil { t.Fatalf("Evaluate failed: %v", err) @@ -161,7 +161,7 @@ func TestRunner_Evaluate_RunsMultipleIterations(t *testing.T) { r := NewRunner(&config.EvalConfig{ Environment: config.Environment{Type: "none"}, Cases: config.CasesConfig{Parallelism: 1}, - }, loader, nil, credential.AgentInitParams{}) + }, loader, nil, credential.ResolvedAgentConfig{}) baseTime := time.Date(2026, time.August, 17, 10, 0, 0, 0, time.UTC) clockTimes := []time.Time{ baseTime, @@ -271,7 +271,7 @@ func TestRunner_Evaluate_MultipleIterationsPrintStabilitySummaryAndDoNotWriteFil r := NewRunner(&config.EvalConfig{ Environment: config.Environment{Type: "none"}, Cases: config.CasesConfig{Parallelism: 1}, - }, loader, nil, credential.AgentInitParams{}) + }, loader, nil, credential.ResolvedAgentConfig{}) origNewEvaluator := newEvaluator t.Cleanup(func() { newEvaluator = origNewEvaluator }) @@ -347,7 +347,7 @@ func TestRunner_Evaluate_AutoIterationAppendsWithoutStabilitySummary(t *testing. r := NewRunner(&config.EvalConfig{ Environment: config.Environment{Type: "none"}, Cases: config.CasesConfig{Parallelism: 1}, - }, loader, nil, credential.AgentInitParams{}) + }, loader, nil, credential.ResolvedAgentConfig{}) var output bytes.Buffer captureUIOutput(t, &output) @@ -411,7 +411,7 @@ func TestRunner_Evaluate_ReturnsPartialResultsWhenLaterRunFails(t *testing.T) { r := NewRunner(&config.EvalConfig{ Environment: config.Environment{Type: "none"}, Cases: config.CasesConfig{Parallelism: 1}, - }, loader, nil, credential.AgentInitParams{}) + }, loader, nil, credential.ResolvedAgentConfig{}) origNewEvaluator := newEvaluator t.Cleanup(func() { newEvaluator = origNewEvaluator }) @@ -513,7 +513,7 @@ func TestRunner_Evaluate_AutoIncrementsIteration(t *testing.T) { return NewRunner(&config.EvalConfig{ Environment: config.Environment{Type: "none"}, Cases: config.CasesConfig{Parallelism: 1}, - }, loader, nil, credential.AgentInitParams{}) + }, loader, nil, credential.ResolvedAgentConfig{}) } opts := EvaluateOptions{OutputDir: workspaceRoot, Iteration: 0, DeleteWorkspace: false}