Skip to content

Commit 0d4142a

Browse files
julianknutsenclaude
andcommitted
fix: strengthen option_defaults e2e tests per review findings
- Add unshadowed provider-only default (output_format) that no agent overrides, proving the provider layer independently participates in the merge pipeline (was: provider default always shadowed by agent) - Add overlapping model key to agent inline option_defaults so the patch proves overwrite semantics, not just additive insertion - Rewrite rig override test to round-trip through TOML via LoadWithIncludes instead of programmatic City{} struct, matching the E2E pattern established in the TOML patch test - Remove redundant ExpandPacks call (LoadWithIncludes already calls it internally) and add duplicate-agent guard to catch double expansion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 2837f62 commit 0d4142a

1 file changed

Lines changed: 103 additions & 44 deletions

File tree

internal/config/resolve_test.go

Lines changed: 103 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -925,17 +925,40 @@ default = "plan"
925925
label = "Unrestricted"
926926
flag_args = ["--dangerously-skip-permissions"]
927927
928-
# Provider-level override: model defaults to "sonnet" instead of "opus".
928+
[[providers.testprov.options_schema]]
929+
key = "output_format"
930+
label = "Output Format"
931+
type = "select"
932+
default = "text"
933+
934+
[[providers.testprov.options_schema.choices]]
935+
value = "text"
936+
label = "Text"
937+
flag_args = ["--output", "text"]
938+
939+
[[providers.testprov.options_schema.choices]]
940+
value = "json"
941+
label = "JSON"
942+
flag_args = ["--output", "json"]
943+
944+
# Provider-level overrides: model "sonnet" (instead of schema "opus"),
945+
# output_format "json" (instead of schema "text").
946+
# output_format is provider-only — no agent overrides it, proving the
947+
# provider layer independently participates in the merge.
929948
[providers.testprov.option_defaults]
930949
model = "sonnet"
950+
output_format = "json"
931951
932952
[[agent]]
933953
name = "worker"
934954
provider = "testprov"
935955
936-
# Agent-level override: permission_mode defaults to "unrestricted".
956+
# Agent-level overrides: permission_mode and model.
957+
# model = "sonnet" here will be overwritten by the patch (model = "haiku"),
958+
# proving patch-wins-over-agent overwrite semantics (not just additive insertion).
937959
[agent.option_defaults]
938960
permission_mode = "unrestricted"
961+
model = "sonnet"
939962
`)
940963

941964
// Patch fragment: override agent's model to "haiku".
@@ -980,18 +1003,28 @@ model = "haiku"
9801003

9811004
// Layer 1 (schema default "opus") overridden by Layer 2 (provider "sonnet"),
9821005
// then overridden by Layer 3 (agent "haiku" via patch).
1006+
// This also proves overwrite semantics: agent inline had model = "sonnet",
1007+
// but the patch overwrites it to "haiku".
9831008
if got := rp.EffectiveDefaults["model"]; got != "haiku" {
984-
t.Errorf("EffectiveDefaults[model] = %q, want %q (agent patch should override provider default)", got, "haiku")
1009+
t.Errorf("EffectiveDefaults[model] = %q, want %q (agent patch should override agent inline and provider default)", got, "haiku")
9851010
}
9861011

9871012
// Layer 1 (schema default "plan") overridden by Layer 3 (agent "unrestricted").
9881013
if got := rp.EffectiveDefaults["permission_mode"]; got != "unrestricted" {
9891014
t.Errorf("EffectiveDefaults[permission_mode] = %q, want %q (agent default should override schema default)", got, "unrestricted")
9901015
}
1016+
1017+
// Layer 2 (provider "json") is NOT overridden by any agent-level source.
1018+
// This proves the provider layer independently participates in the merge —
1019+
// without it, output_format would remain at schema default "text".
1020+
if got := rp.EffectiveDefaults["output_format"]; got != "json" {
1021+
t.Errorf("EffectiveDefaults[output_format] = %q, want %q (provider default should override schema default)", got, "json")
1022+
}
9911023
}
9921024

9931025
// TestOptionDefaultsRigOverrideThroughResolve exercises the rig-level override
994-
// path: pack agent → ExpandPacks with AgentOverride → ResolveProvider → EffectiveDefaults.
1026+
// path: TOML config → LoadWithIncludes (which internally calls ExpandPacks,
1027+
// applying AgentOverride) → ResolveProvider → EffectiveDefaults.
9951028
//
9961029
// This complements TestOptionDefaultsTOMLThroughResolve which tests the patch path.
9971030
// The rig override path is a separate code flow through applyAgentOverride (pack.go).
@@ -1008,58 +1041,84 @@ name = "coder"
10081041
provider = "testprov"
10091042
`)
10101043

1011-
// City defines the provider (with options_schema but no provider-level
1012-
// option_defaults — so Layer 2 is empty and only schema defaults apply).
1013-
cfg := &City{
1014-
Workspace: Workspace{Name: "test"},
1015-
Providers: map[string]ProviderSpec{
1016-
"testprov": {
1017-
Command: "testprov",
1018-
PromptMode: "arg",
1019-
OptionsSchema: []ProviderOption{
1020-
{
1021-
Key: "model", Label: "Model", Type: "select", Default: "opus",
1022-
Choices: []OptionChoice{
1023-
{Value: "opus", Label: "Opus", FlagArgs: []string{"--model", "opus"}},
1024-
{Value: "haiku", Label: "Haiku", FlagArgs: []string{"--model", "haiku"}},
1025-
},
1026-
},
1027-
{
1028-
Key: "permission_mode", Label: "Permission Mode", Type: "select", Default: "plan",
1029-
Choices: []OptionChoice{
1030-
{Value: "plan", Label: "Plan", FlagArgs: []string{"--permission-mode", "plan"}},
1031-
{Value: "unrestricted", Label: "Unrestricted", FlagArgs: []string{"--dangerously-skip-permissions"}},
1032-
},
1033-
},
1034-
},
1035-
},
1036-
},
1037-
Rigs: []Rig{{
1038-
Name: "myrig",
1039-
Path: "/repo",
1040-
Includes: []string{"packs/svc"},
1041-
Overrides: []AgentOverride{{
1042-
Agent: "coder",
1043-
OptionDefaults: map[string]string{"model": "haiku", "permission_mode": "unrestricted"},
1044-
}},
1045-
}},
1046-
}
1044+
// city.toml: provider with options_schema + rig with override option_defaults.
1045+
// No provider-level option_defaults — only schema defaults + agent overrides.
1046+
fs.Files["/city/city.toml"] = []byte(`
1047+
[workspace]
1048+
name = "test"
1049+
1050+
[providers.testprov]
1051+
command = "testprov"
1052+
prompt_mode = "arg"
1053+
1054+
[[providers.testprov.options_schema]]
1055+
key = "model"
1056+
label = "Model"
1057+
type = "select"
1058+
default = "opus"
1059+
1060+
[[providers.testprov.options_schema.choices]]
1061+
value = "opus"
1062+
label = "Opus"
1063+
flag_args = ["--model", "opus"]
1064+
1065+
[[providers.testprov.options_schema.choices]]
1066+
value = "haiku"
1067+
label = "Haiku"
1068+
flag_args = ["--model", "haiku"]
1069+
1070+
[[providers.testprov.options_schema]]
1071+
key = "permission_mode"
1072+
label = "Permission Mode"
1073+
type = "select"
1074+
default = "plan"
1075+
1076+
[[providers.testprov.options_schema.choices]]
1077+
value = "plan"
1078+
label = "Plan"
1079+
flag_args = ["--permission-mode", "plan"]
10471080
1048-
if err := ExpandPacks(cfg, fs, "/city", nil); err != nil {
1049-
t.Fatalf("ExpandPacks: %v", err)
1081+
[[providers.testprov.options_schema.choices]]
1082+
value = "unrestricted"
1083+
label = "Unrestricted"
1084+
flag_args = ["--dangerously-skip-permissions"]
1085+
1086+
[[rigs]]
1087+
name = "myrig"
1088+
path = "/repo"
1089+
includes = ["packs/svc"]
1090+
1091+
[[rigs.overrides]]
1092+
agent = "coder"
1093+
1094+
[rigs.overrides.option_defaults]
1095+
model = "haiku"
1096+
permission_mode = "unrestricted"
1097+
`)
1098+
1099+
// LoadWithIncludes handles the full pipeline: parse TOML → apply patches →
1100+
// ExpandPacks (which applies rig overrides). No separate ExpandPacks call needed.
1101+
cfg, _, err := LoadWithIncludes(fs, "/city/city.toml")
1102+
if err != nil {
1103+
t.Fatalf("LoadWithIncludes: %v", err)
10501104
}
10511105

1052-
// Find the expanded agent.
1106+
// Find the expanded agent — verify exactly one exists (LoadWithIncludes
1107+
// already expanded packs; a duplicate would indicate double expansion).
10531108
var coder *Agent
1109+
coderCount := 0
10541110
for i := range cfg.Agents {
10551111
if cfg.Agents[i].Name == "coder" {
10561112
coder = &cfg.Agents[i]
1057-
break
1113+
coderCount++
10581114
}
10591115
}
10601116
if coder == nil {
10611117
t.Fatal("coder agent not found after expansion")
10621118
}
1119+
if coderCount != 1 {
1120+
t.Fatalf("expected exactly 1 coder agent, got %d (double expansion?)", coderCount)
1121+
}
10631122

10641123
// Override should have set agent.OptionDefaults.
10651124
if got := coder.OptionDefaults["model"]; got != "haiku" {

0 commit comments

Comments
 (0)